[PATCH 07/17] drm/msm/dpu: disallow widebus en in INTF_CONFIG2 when DP is YUV420

Tue Jan 30 05:28:26 UTC 2024

On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> >>>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan at quicinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> >>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> >>>>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
> >>>>>>>
> >>>>>>> I think this is handled in the DP driver, where we disallow wide bus
> >>>>>>> for YUV 4:2:0 modes.
> >>>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
> >>>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
> >>>>>> this check.
> >>>>>
> >>>>> As I wrote in my second email, I'd prefer to have one if which guards
> >>>>> HCTL_EN and another one for WIDEN
> >>>>>
> >>>> Its hard to separate out the conditions just for HCTL_EN . Its more
> >>>> about handling the various pixel per clock combinations.
> >>>>
> >>>> But, here is how I can best summarize it.
> >>>>
> >>>> Lets consider DSI and DP separately:
> >>>>
> >>>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
> >>>>
> >>>> This is same the same condition as widebus today in
> >>>> msm_dsi_host_is_wide_bus_enabled().
> >>>>
> >>>> Hence no changes needed for DSI.
> >>>
> >>> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
> >>> enabled, while you have written that HCTL_EN should be set in all
> >>> cases on a corresponding platform.
> >>>
> >>
> >> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
> >> widebus for the versions I wrote.
> >>
> >> Basically for the non-compressed case.
> >>
> >> I will write something up to fix this for DSI. I think this can go as a
> >> bug fix.
> >>
> >> But that does not change the DP conditions OR in other words, I dont see
> >> anything wrong with this patch yet.
> >>
> >>>>
> >>>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
> >>>> as they are independent cases. We dont support YUV420 + DSC case.
> >>>>
> >>>> There are other cases which fall outside of this bucket but they are
> >>>> optional ones. We only follow the "required" ones.
> >>>>
> >>>> With this summary in mind, I am fine with what we have except perhaps
> >>>> better documentation above this block.
> >>>>
> >>>> When DSC over DP gets added, I am expecting no changes to this block as
> >>>> it will fall under the widebus_en case.
> >>>>
> >>>> With this information, how else would you like the check?
> >>>
> >>> What does this bit really change?
> >>>
> >>
> >> This bit basically just tells that the data sent per line is programmed
> >> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
> >>
> >>           if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
> >>                   DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> >>                   DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
> >> display_data_hctl);
> >>                   DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
> >>           }
> >>
> >> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.
> >
> > Can we enable it unconditionally for DPU >= 5.0?
> >
>
> There is a corner-case that we should not enable it when compression is
> enabled without widebus as per the docs :(

What about explicitly disabling it in such a case?
I mean something like:

if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en))
   intf_cfg |= INTF_CFG2_HCTL_EN;

>
> For DP there will not be a case like that because compression and
> widebus go together but for DSI, it is possible.
>
> So I found that the reset value of this register does cover all cases
> for DPU >= 7.0 so below fix will address the DSI concern and will fix
> the issue even for YUV420 cases such as this one for DPU >= 7.0
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 6bba531d6dc4..cbd5ebd516cd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -168,6 +168,8 @@ static void dpu_hw_intf_setup_timing_engine(struct
> dpu_hw_intf *ctx,
>           * video timing. It is recommended to enable it for all cases,
> except
>           * if compression is enabled in 1 pixel per clock mode
>           */
> +
> +       intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2);
>          if (p->wide_bus_en)
>                  intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN |
> INTF_CFG2_DATA_HCTL_EN;
>
>
> But, this does not still work for DPU < 7.0 such as sc7180 if we try
> YUV420 over DP on that because its DPU version is 6.2 so we will have to
> keep this patch for those cases.
>
> >>
> >>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >>>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> index 6bba531d6dc41..bfb93f02fe7c1 100644
> >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> @@ -168,7 +168,9 @@ static void
> >>>>>>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> >>>>>>>>           * video timing. It is recommended to enable it for all cases,
> >>>>>>>> except
> >>>>>>>>           * if compression is enabled in 1 pixel per clock mode
> >>>>>>>>           */
> >>>>>>>> -    if (p->wide_bus_en)
> >>>>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> >>>>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> >>>>>>>> +    else if (p->wide_bus_en)
> >>>>>>>>              intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >>>>>>>>            data_width = p->width;
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >

-- 
With best wishes
Dmitry