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

[Dmitry Baryshkov]

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.

>
> 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?

>
> >>>
> >>>>
> >>>> 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