| Message ID | 20211007070900.456044-7-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 07/10/2021 10:08, Vinod Koul wrote: > We cannot enable mode_3d when we are using the DSC. So pass > configuration to detect DSC is enabled and not enable mode_3d > when we are using DSC > > We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc > enabled and pass this to .setup_intf_cfg() > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > Changes since > v1: > - Move this patch from 7 to 6 > - Update the changelog > - Make dsc as int and store the DSC indices > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++ > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++ > 4 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > index e7270eb6b84b..fca07ed03317 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode( > return BLEND_3D_NONE; > } > > +static inline bool dpu_encoder_helper_get_dsc_mode(struct dpu_encoder_phys *phys_enc) > +{ > + struct drm_encoder *drm_enc = phys_enc->parent; > + struct msm_drm_private *priv = drm_enc->dev->dev_private; > + > + if (priv->dsc) > + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */ Please use defined values here rater than just BIT(). > + > + return 0; > +} > + > /** > * dpu_encoder_helper_split_config - split display configuration helper function > * This helper function may be used by physical encoders to configure > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > index aa01698d6b25..8e5c0911734c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; > intf_cfg.stream_sel = cmd_enc->stream_sel; > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); > + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc); > + > ctl->ops.setup_intf_cfg(ctl, &intf_cfg); > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > index 64740ddb983e..3c79bd9c2fe5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct dpu_hw_ctl *ctx) > return ctx->pending_flush_mask; > } > > -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > { > > if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX)) > @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > > intf_cfg |= (cfg->intf & 0xF) << 4; > > - if (cfg->mode_3d) { > + /* In DSC we can't set merge, so check for dsc too */ > + if (cfg->mode_3d && !cfg->dsc) { The more I think about this hunk, the more I'm unsure about it. Downstream has the following topoligies defined: * @SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC: 2 LM, 2 PP, 3DMux, 1 DSC, 1 INTF/WB * @SDE_RM_TOPOLOGY_QUADPIPE_3DMERGE_DSC 4 LM, 4 PP, 3DMux, 3 DSC, 2 INTF While the latter is not supported on sdm845, the former one should be (by the hardware). So in the driver I think we should make sure that mode_3d does not get set rather than disallowing it here. > intf_cfg |= BIT(19); > intf_cfg |= (cfg->mode_3d - 0x1) << 20; > } > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > index 806c171e5df2..5dfac5994bd4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { > * @mode_3d: 3d mux configuration > * @merge_3d: 3d merge block used > * @intf_mode_sel: Interface mode, cmd / vid > + * @dsc: DSC BIT masks > * @stream_sel: Stream selection for multi-stream interfaces > */ > struct dpu_hw_intf_cfg { > @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { > enum dpu_3d_blend_mode mode_3d; > enum dpu_merge_3d merge_3d; > enum dpu_ctl_mode_sel intf_mode_sel; > + unsigned int dsc; > int stream_sel; > }; > > -- With best wishes Dmitry
On 14/10/2021 16:41, Dmitry Baryshkov wrote: > On 07/10/2021 10:08, Vinod Koul wrote: >> We cannot enable mode_3d when we are using the DSC. So pass >> configuration to detect DSC is enabled and not enable mode_3d >> when we are using DSC >> >> We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc >> enabled and pass this to .setup_intf_cfg() >> >> Signed-off-by: Vinod Koul <vkoul@kernel.org> >> --- >> Changes since >> v1: >> - Move this patch from 7 to 6 >> - Update the changelog >> - Make dsc as int and store the DSC indices >> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++ >> 4 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> index e7270eb6b84b..fca07ed03317 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >> @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode >> dpu_encoder_helper_get_3d_blend_mode( >> return BLEND_3D_NONE; >> } >> +static inline bool dpu_encoder_helper_get_dsc_mode(struct >> dpu_encoder_phys *phys_enc) >> +{ >> + struct drm_encoder *drm_enc = phys_enc->parent; >> + struct msm_drm_private *priv = drm_enc->dev->dev_private; >> + >> + if (priv->dsc) >> + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */ > > Please use defined values here rater than just BIT(). Ah, it's a list of DSC blocks used. So the function name is misleading (as it's not a mode). I think we'd better pass DSC_n names here. What about using an array for cfg->dsc? > >> + >> + return 0; >> +} >> + >> /** >> * dpu_encoder_helper_split_config - split display configuration >> helper function >> * This helper function may be used by physical encoders to >> configure >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >> index aa01698d6b25..8e5c0911734c 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >> @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( >> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; >> intf_cfg.stream_sel = cmd_enc->stream_sel; >> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); >> + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc); >> + >> ctl->ops.setup_intf_cfg(ctl, &intf_cfg); >> } >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> index 64740ddb983e..3c79bd9c2fe5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct >> dpu_hw_ctl *ctx) >> return ctx->pending_flush_mask; >> } >> -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >> +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >> { >> if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX)) >> @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl >> *ctx, >> intf_cfg |= (cfg->intf & 0xF) << 4; >> - if (cfg->mode_3d) { >> + /* In DSC we can't set merge, so check for dsc too */ >> + if (cfg->mode_3d && !cfg->dsc) { > > The more I think about this hunk, the more I'm unsure about it. > Downstream has the following topoligies defined: > * @SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC: 2 LM, 2 PP, 3DMux, 1 DSC, 1 > INTF/WB > * @SDE_RM_TOPOLOGY_QUADPIPE_3DMERGE_DSC 4 LM, 4 PP, 3DMux, 3 DSC, 2 INTF > > While the latter is not supported on sdm845, the former one should be > (by the hardware). So in the driver I think we should make sure that > mode_3d does not get set rather than disallowing it here. > >> intf_cfg |= BIT(19); >> intf_cfg |= (cfg->mode_3d - 0x1) << 20; >> } >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> index 806c171e5df2..5dfac5994bd4 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >> @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { >> * @mode_3d: 3d mux configuration >> * @merge_3d: 3d merge block used >> * @intf_mode_sel: Interface mode, cmd / vid >> + * @dsc: DSC BIT masks >> * @stream_sel: Stream selection for multi-stream interfaces >> */ >> struct dpu_hw_intf_cfg { >> @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { >> enum dpu_3d_blend_mode mode_3d; >> enum dpu_merge_3d merge_3d; >> enum dpu_ctl_mode_sel intf_mode_sel; >> + unsigned int dsc; I think this should be: enum dpu_dsc dsc[MAX_DSCS]; unsigned int num_dsc; >> int stream_sel; >> }; >> > > -- With best wishes Dmitry
On 14-10-21, 16:50, Dmitry Baryshkov wrote: > On 14/10/2021 16:41, Dmitry Baryshkov wrote: > > On 07/10/2021 10:08, Vinod Koul wrote: > > > We cannot enable mode_3d when we are using the DSC. So pass > > > configuration to detect DSC is enabled and not enable mode_3d > > > when we are using DSC > > > > > > We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc > > > enabled and pass this to .setup_intf_cfg() > > > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > > --- > > > Changes since > > > v1: > > > - Move this patch from 7 to 6 > > > - Update the changelog > > > - Make dsc as int and store the DSC indices > > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++++++++++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++-- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++ > > > 4 files changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > index e7270eb6b84b..fca07ed03317 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode > > > dpu_encoder_helper_get_3d_blend_mode( > > > return BLEND_3D_NONE; > > > } > > > +static inline bool dpu_encoder_helper_get_dsc_mode(struct > > > dpu_encoder_phys *phys_enc) > > > +{ > > > + struct drm_encoder *drm_enc = phys_enc->parent; > > > + struct msm_drm_private *priv = drm_enc->dev->dev_private; > > > + > > > + if (priv->dsc) > > > + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */ > > > > Please use defined values here rater than just BIT(). > > Ah, it's a list of DSC blocks used. So the function name is misleading (as > it's not a mode). I think we'd better pass DSC_n names here. What about > using an array for cfg->dsc? Yeah I can do better names. > > > > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * dpu_encoder_helper_split_config - split display configuration > > > helper function > > > * This helper function may be used by physical encoders to > > > configure > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > index aa01698d6b25..8e5c0911734c 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( > > > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; > > > intf_cfg.stream_sel = cmd_enc->stream_sel; > > > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); > > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc); > > > + > > > ctl->ops.setup_intf_cfg(ctl, &intf_cfg); > > > } > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > > > index 64740ddb983e..3c79bd9c2fe5 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > > > @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct > > > dpu_hw_ctl *ctx) > > > return ctx->pending_flush_mask; > > > } > > > -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > > > +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) > > > { > > > if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX)) > > > @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct > > > dpu_hw_ctl *ctx, > > > intf_cfg |= (cfg->intf & 0xF) << 4; > > > - if (cfg->mode_3d) { > > > + /* In DSC we can't set merge, so check for dsc too */ > > > + if (cfg->mode_3d && !cfg->dsc) { > > > > The more I think about this hunk, the more I'm unsure about it. > > Downstream has the following topoligies defined: > > * @SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC: 2 LM, 2 PP, 3DMux, 1 DSC, 1 > > INTF/WB > > * @SDE_RM_TOPOLOGY_QUADPIPE_3DMERGE_DSC 4 LM, 4 PP, 3DMux, 3 DSC, 2 INTF > > > > While the latter is not supported on sdm845, the former one should be > > (by the hardware). So in the driver I think we should make sure that > > mode_3d does not get set rather than disallowing it here. > > > > > intf_cfg |= BIT(19); > > > intf_cfg |= (cfg->mode_3d - 0x1) << 20; > > > } > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > index 806c171e5df2..5dfac5994bd4 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { > > > * @mode_3d: 3d mux configuration > > > * @merge_3d: 3d merge block used > > > * @intf_mode_sel: Interface mode, cmd / vid > > > + * @dsc: DSC BIT masks > > > * @stream_sel: Stream selection for multi-stream interfaces > > > */ > > > struct dpu_hw_intf_cfg { > > > @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { > > > enum dpu_3d_blend_mode mode_3d; > > > enum dpu_merge_3d merge_3d; > > > enum dpu_ctl_mode_sel intf_mode_sel; > > > + unsigned int dsc; > > I think this should be: > enum dpu_dsc dsc[MAX_DSCS]; > unsigned int num_dsc; hmmm, how do we go about getting the num_dsc value here. dpu_encoder_phys does not know about that.. -- ~Vinod
On 20/10/2021 09:57, Vinod Koul wrote: > On 14-10-21, 16:50, Dmitry Baryshkov wrote: >> On 14/10/2021 16:41, Dmitry Baryshkov wrote: >>> On 07/10/2021 10:08, Vinod Koul wrote: >>>> We cannot enable mode_3d when we are using the DSC. So pass >>>> configuration to detect DSC is enabled and not enable mode_3d >>>> when we are using DSC >>>> >>>> We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc >>>> enabled and pass this to .setup_intf_cfg() >>>> >>>> Signed-off-by: Vinod Koul <vkoul@kernel.org> >>>> --- >>>> Changes since >>>> v1: >>>> - Move this patch from 7 to 6 >>>> - Update the changelog >>>> - Make dsc as int and store the DSC indices >>>> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++++++++++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++ >>>> 4 files changed, 18 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>> index e7270eb6b84b..fca07ed03317 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>>> @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode >>>> dpu_encoder_helper_get_3d_blend_mode( >>>> return BLEND_3D_NONE; >>>> } >>>> +static inline bool dpu_encoder_helper_get_dsc_mode(struct >>>> dpu_encoder_phys *phys_enc) >>>> +{ >>>> + struct drm_encoder *drm_enc = phys_enc->parent; >>>> + struct msm_drm_private *priv = drm_enc->dev->dev_private; >>>> + >>>> + if (priv->dsc) >>>> + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */ >>> >>> Please use defined values here rater than just BIT(). >> >> Ah, it's a list of DSC blocks used. So the function name is misleading (as >> it's not a mode). I think we'd better pass DSC_n names here. What about >> using an array for cfg->dsc? > > Yeah I can do better names. > >> >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * dpu_encoder_helper_split_config - split display configuration >>>> helper function >>>> * This helper function may be used by physical encoders to >>>> configure >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>>> index aa01698d6b25..8e5c0911734c 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>>> @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( >>>> intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; >>>> intf_cfg.stream_sel = cmd_enc->stream_sel; >>>> intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); >>>> + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc); >>>> + >>>> ctl->ops.setup_intf_cfg(ctl, &intf_cfg); >>>> } >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> index 64740ddb983e..3c79bd9c2fe5 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct >>>> dpu_hw_ctl *ctx) >>>> return ctx->pending_flush_mask; >>>> } >>>> -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >>>> +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) >>>> { >>>> if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX)) >>>> @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct >>>> dpu_hw_ctl *ctx, >>>> intf_cfg |= (cfg->intf & 0xF) << 4; >>>> - if (cfg->mode_3d) { >>>> + /* In DSC we can't set merge, so check for dsc too */ >>>> + if (cfg->mode_3d && !cfg->dsc) { >>> >>> The more I think about this hunk, the more I'm unsure about it. >>> Downstream has the following topoligies defined: >>> * @SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC: 2 LM, 2 PP, 3DMux, 1 DSC, 1 >>> INTF/WB >>> * @SDE_RM_TOPOLOGY_QUADPIPE_3DMERGE_DSC 4 LM, 4 PP, 3DMux, 3 DSC, 2 INTF >>> >>> While the latter is not supported on sdm845, the former one should be >>> (by the hardware). So in the driver I think we should make sure that >>> mode_3d does not get set rather than disallowing it here. >>> >>>> intf_cfg |= BIT(19); >>>> intf_cfg |= (cfg->mode_3d - 0x1) << 20; >>>> } >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>> index 806c171e5df2..5dfac5994bd4 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>> @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { >>>> * @mode_3d: 3d mux configuration >>>> * @merge_3d: 3d merge block used >>>> * @intf_mode_sel: Interface mode, cmd / vid >>>> + * @dsc: DSC BIT masks >>>> * @stream_sel: Stream selection for multi-stream interfaces >>>> */ >>>> struct dpu_hw_intf_cfg { >>>> @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { >>>> enum dpu_3d_blend_mode mode_3d; >>>> enum dpu_merge_3d merge_3d; >>>> enum dpu_ctl_mode_sel intf_mode_sel; >>>> + unsigned int dsc; >> >> I think this should be: >> enum dpu_dsc dsc[MAX_DSCS]; >> unsigned int num_dsc; > > hmmm, how do we go about getting the num_dsc value here. > dpu_encoder_phys does not know about that.. dpu_encoder_get_topology() can decide whether to use DSC or not and then set num_dsc. For now it will always set 2 if we are using DSC at all, but let's keep the decision in a single place rather than having it scattered all over the driver. -- With best wishes Dmitry
On 25-10-21, 17:40, Dmitry Baryshkov wrote: > On 20/10/2021 09:57, Vinod Koul wrote: > > On 14-10-21, 16:50, Dmitry Baryshkov wrote: > > > On 14/10/2021 16:41, Dmitry Baryshkov wrote: > > > > On 07/10/2021 10:08, Vinod Koul wrote: > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > > > index 806c171e5df2..5dfac5994bd4 100644 > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h > > > > > @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { > > > > > * @mode_3d: 3d mux configuration > > > > > * @merge_3d: 3d merge block used > > > > > * @intf_mode_sel: Interface mode, cmd / vid > > > > > + * @dsc: DSC BIT masks > > > > > * @stream_sel: Stream selection for multi-stream interfaces > > > > > */ > > > > > struct dpu_hw_intf_cfg { > > > > > @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { > > > > > enum dpu_3d_blend_mode mode_3d; > > > > > enum dpu_merge_3d merge_3d; > > > > > enum dpu_ctl_mode_sel intf_mode_sel; > > > > > + unsigned int dsc; > > > > > > I think this should be: > > > enum dpu_dsc dsc[MAX_DSCS]; > > > unsigned int num_dsc; > > > > hmmm, how do we go about getting the num_dsc value here. > > dpu_encoder_phys does not know about that.. > > dpu_encoder_get_topology() can decide whether to use DSC or not and then set > num_dsc. For now it will always set 2 if we are using DSC at all, but let's > keep the decision in a single place rather than having it scattered all over > the driver. Yes agree, but dpu_encoder_get_topology() is private to encoder. Am not sure how best to propagate the info into the hw_intf_cfg? -- ~Vinod
On 25/10/2021 19:10, Vinod Koul wrote: > On 25-10-21, 17:40, Dmitry Baryshkov wrote: >> On 20/10/2021 09:57, Vinod Koul wrote: >>> On 14-10-21, 16:50, Dmitry Baryshkov wrote: >>>> On 14/10/2021 16:41, Dmitry Baryshkov wrote: >>>>> On 07/10/2021 10:08, Vinod Koul wrote: > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>>>> index 806c171e5df2..5dfac5994bd4 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h >>>>>> @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { >>>>>> * @mode_3d: 3d mux configuration >>>>>> * @merge_3d: 3d merge block used >>>>>> * @intf_mode_sel: Interface mode, cmd / vid >>>>>> + * @dsc: DSC BIT masks >>>>>> * @stream_sel: Stream selection for multi-stream interfaces >>>>>> */ >>>>>> struct dpu_hw_intf_cfg { >>>>>> @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { >>>>>> enum dpu_3d_blend_mode mode_3d; >>>>>> enum dpu_merge_3d merge_3d; >>>>>> enum dpu_ctl_mode_sel intf_mode_sel; >>>>>> + unsigned int dsc; >>>> >>>> I think this should be: >>>> enum dpu_dsc dsc[MAX_DSCS]; >>>> unsigned int num_dsc; >>> >>> hmmm, how do we go about getting the num_dsc value here. >>> dpu_encoder_phys does not know about that.. >> >> dpu_encoder_get_topology() can decide whether to use DSC or not and then set >> num_dsc. For now it will always set 2 if we are using DSC at all, but let's >> keep the decision in a single place rather than having it scattered all over >> the driver. > > Yes agree, but dpu_encoder_get_topology() is private to encoder. Am not > sure how best to propagate the info into the hw_intf_cfg? Let dpu_encoder_get_topology() set num_dscs to 2 and merge_3d to 0 if the encoder has DSC information and to 0 otherwise. This will cover all topologies that we care about for now. Regarding getting the DSC config. Currently you use single priv->dsc pointer, which works for the simple case of single DSI output, but will break as soon as somebody has DSC DSI + DP config. Either we can introduce the array of DSC configs, or we can add a DSI-specific msm_dsi_get_dsc_config(), which will be later paired with the corresponding displayport function. -- With best wishes Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index e7270eb6b84b..fca07ed03317 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode( return BLEND_3D_NONE; } +static inline bool dpu_encoder_helper_get_dsc_mode(struct dpu_encoder_phys *phys_enc) +{ + struct drm_encoder *drm_enc = phys_enc->parent; + struct msm_drm_private *priv = drm_enc->dev->dev_private; + + if (priv->dsc) + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */ + + return 0; +} + /** * dpu_encoder_helper_split_config - split display configuration helper function * This helper function may be used by physical encoders to configure diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index aa01698d6b25..8e5c0911734c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; intf_cfg.stream_sel = cmd_enc->stream_sel; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc); + ctl->ops.setup_intf_cfg(ctl, &intf_cfg); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c index 64740ddb983e..3c79bd9c2fe5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct dpu_hw_ctl *ctx) return ctx->pending_flush_mask; } -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx) { if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX)) @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, intf_cfg |= (cfg->intf & 0xF) << 4; - if (cfg->mode_3d) { + /* In DSC we can't set merge, so check for dsc too */ + if (cfg->mode_3d && !cfg->dsc) { intf_cfg |= BIT(19); intf_cfg |= (cfg->mode_3d - 0x1) << 20; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h index 806c171e5df2..5dfac5994bd4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg { * @mode_3d: 3d mux configuration * @merge_3d: 3d merge block used * @intf_mode_sel: Interface mode, cmd / vid + * @dsc: DSC BIT masks * @stream_sel: Stream selection for multi-stream interfaces */ struct dpu_hw_intf_cfg { @@ -46,6 +47,7 @@ struct dpu_hw_intf_cfg { enum dpu_3d_blend_mode mode_3d; enum dpu_merge_3d merge_3d; enum dpu_ctl_mode_sel intf_mode_sel; + unsigned int dsc; int stream_sel; };
We cannot enable mode_3d when we are using the DSC. So pass configuration to detect DSC is enabled and not enable mode_3d when we are using DSC We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc enabled and pass this to .setup_intf_cfg() Signed-off-by: Vinod Koul <vkoul@kernel.org> --- Changes since v1: - Move this patch from 7 to 6 - Update the changelog - Make dsc as int and store the DSC indices drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++ 4 files changed, 18 insertions(+), 2 deletions(-) -- 2.31.1