| Message ID | 1681490777-15351-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/msm/dpu: always program DSC active bits | expand |
On Fri, 14 Apr 2023 at 19:46, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > In current code, the dsc active bits are set only if the cfg->dsc is set. > However, for displays which are hot-pluggable, there can be a use-case > of disconnecting a DSC supported sink and connecting a non-DSC sink. > > For those cases we need to clear DSC active bits during teardown. > > As discuss at [1], clear DSC active bit will handled at reset_intf_cfg() nit: discussed > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > [1] https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com/ > --- Changelog? This is v3 already, but it has no changes described. > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > 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 bbdc95c..88e4efe 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > + And the comment got dropped. Please restore it in some form. > + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 2023-04-14 09:46:17, Kuogee Hsieh wrote: > In current code, the dsc active bits are set only if the cfg->dsc is set. This is the old sentence from v1 again, did you accidentally send the wrong patch as the improvements from v2 are missing? > However, for displays which are hot-pluggable, there can be a use-case > of disconnecting a DSC supported sink and connecting a non-DSC sink. > > For those cases we need to clear DSC active bits during teardown. At least teardown is one word again, v2 had "tear down" which is wrong. > As discuss at [1], clear DSC active bit will handled at reset_intf_cfg() discussed* as pointed out by Dmitry, and make it clear that this is about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once that is moved to reset_intf_cfg(), this patch should be reverted as there is no need to write the registers once again when cfg->dsc equals 0. - Marijn > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > [1] https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com/ > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > 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 bbdc95c..88e4efe 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > + > + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 15/04/2023 02:02, Marijn Suijten wrote: > On 2023-04-14 09:46:17, Kuogee Hsieh wrote: >> In current code, the dsc active bits are set only if the cfg->dsc is set. > > This is the old sentence from v1 again, did you accidentally send the > wrong patch as the improvements from v2 are missing? > >> However, for displays which are hot-pluggable, there can be a use-case >> of disconnecting a DSC supported sink and connecting a non-DSC sink. >> >> For those cases we need to clear DSC active bits during teardown. > > At least teardown is one word again, v2 had "tear down" which is wrong. > >> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg() > > discussed* as pointed out by Dmitry, and make it clear that this is > about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once > that is moved to reset_intf_cfg(), this patch should be reverted as > there is no need to write the registers once again when cfg->dsc equals > 0. Kuogee, can we please get a proper v4? With all the relevant changes from v2, with the changelog, etc. Otherwise the present Reviewed-by tags are just incorrect. > > - Marijn > >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> >> >> [1] https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com/ >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> 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 bbdc95c..88e4efe 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, >> if (cfg->merge_3d) >> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >> BIT(cfg->merge_3d - MERGE_3D_0)); >> - if (cfg->dsc) { >> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> - } >> + >> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> } >> >> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >>
Hi Dmitry / Marijn On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote: > On 15/04/2023 02:02, Marijn Suijten wrote: >> On 2023-04-14 09:46:17, Kuogee Hsieh wrote: >>> In current code, the dsc active bits are set only if the cfg->dsc is >>> set. >> >> This is the old sentence from v1 again, did you accidentally send the >> wrong patch as the improvements from v2 are missing? >> >>> However, for displays which are hot-pluggable, there can be a use-case >>> of disconnecting a DSC supported sink and connecting a non-DSC sink. >>> >>> For those cases we need to clear DSC active bits during teardown. >> >> At least teardown is one word again, v2 had "tear down" which is wrong. >> >>> As discuss at [1], clear DSC active bit will handled at reset_intf_cfg() >> >> discussed* as pointed out by Dmitry, and make it clear that this is >> about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once >> that is moved to reset_intf_cfg(), this patch should be reverted as >> there is no need to write the registers once again when cfg->dsc equals >> 0. > > Kuogee, can we please get a proper v4? With all the relevant changes > from v2, with the changelog, etc. > > Otherwise the present Reviewed-by tags are just incorrect. > After looking into the DPU DSC changes internally which will be posted today/tomm, that piece of code is again touching this block, so I am now also not convinced this change should be made right now because it was again touching flush programming, so that again leaves only the active bits which as Marijn mentioned will be applicable for a use-case only with hot-pluggable display which we dont have till we land DP DSC. I think this is what we will do: -> post DPU DSC changes -> post the patch to improve reset_intf_cfg() to handle DSC -> post rest of DP DSC changes -> post rest of DSI DSC changes I think that way, we all are aligned. Apologies for posting this patch a bit ahead of time but if i had foreseen that DPU DSC changes will again touch the flush code, i would have held this patch back too. >> >> - Marijn >> >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> >>> >>> [1] >>> https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com/ >>> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> 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 bbdc95c..88e4efe 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >>> dpu_hw_ctl *ctx, >>> if (cfg->merge_3d) >>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >>> BIT(cfg->merge_3d - MERGE_3D_0)); >>> - if (cfg->dsc) { >>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>> - } >>> + >>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>> } >>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >>> -- >>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>> Forum, >>> a Linux Foundation Collaborative Project >>> >
On 20/04/2023 19:40, Abhinav Kumar wrote: > Hi Dmitry / Marijn > > On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote: >> On 15/04/2023 02:02, Marijn Suijten wrote: >>> On 2023-04-14 09:46:17, Kuogee Hsieh wrote: >>>> In current code, the dsc active bits are set only if the cfg->dsc is >>>> set. >>> >>> This is the old sentence from v1 again, did you accidentally send the >>> wrong patch as the improvements from v2 are missing? >>> >>>> However, for displays which are hot-pluggable, there can be a use-case >>>> of disconnecting a DSC supported sink and connecting a non-DSC sink. >>>> >>>> For those cases we need to clear DSC active bits during teardown. >>> >>> At least teardown is one word again, v2 had "tear down" which is wrong. >>> >>>> As discuss at [1], clear DSC active bit will handled at >>>> reset_intf_cfg() >>> >>> discussed* as pointed out by Dmitry, and make it clear that this is >>> about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically. Once >>> that is moved to reset_intf_cfg(), this patch should be reverted as >>> there is no need to write the registers once again when cfg->dsc equals >>> 0. >> >> Kuogee, can we please get a proper v4? With all the relevant changes >> from v2, with the changelog, etc. >> >> Otherwise the present Reviewed-by tags are just incorrect. >> > > After looking into the DPU DSC changes internally which will be posted > today/tomm, that piece of code is again touching this block, so I am now > also not convinced this change should be made right now because it was > again touching flush programming, so that again leaves only the active > bits which as Marijn mentioned will be applicable for a use-case only > with hot-pluggable display which we dont have till we land DP DSC. > > I think this is what we will do: > > -> post DPU DSC changes > -> post the patch to improve reset_intf_cfg() to handle DSC > -> post rest of DP DSC changes > -> post rest of DSI DSC changes > > I think that way, we all are aligned. > > Apologies for posting this patch a bit ahead of time but if i had > foreseen that DPU DSC changes will again touch the flush code, i would > have held this patch back too. Ack, sounds good. Thank you. > >>> >>> - Marijn >>> >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>> Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> >>>> >>>> [1] >>>> https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@quicinc.com/ >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> 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 bbdc95c..88e4efe 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >>>> dpu_hw_ctl *ctx, >>>> if (cfg->merge_3d) >>>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >>>> BIT(cfg->merge_3d - MERGE_3D_0)); >>>> - if (cfg->dsc) { >>>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>>> - } >>>> + >>>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>>> } >>>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >>>> Forum, >>>> a Linux Foundation Collaborative Project >>>> >>
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 bbdc95c..88e4efe 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, if (cfg->merge_3d) DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, BIT(cfg->merge_3d - MERGE_3D_0)); - if (cfg->dsc) { - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); - } + + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); } static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,