Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN feature flag for DPU >= 5.0

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 6/16/2023 5:37 PM, Dmitry Baryshkov wrote:
> > On 17/06/2023 00:10, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
> >>> On 14/06/2023 23:39, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
> >>>>>
> >>>>>
> >>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
> >>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
> >>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
> >>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
> >>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data
> >>>>>>>>>> to be sent
> >>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>>>>>>>> ---
> >>>>>>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> >>>>>>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> >>>>>>>>>>    2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> index 36ba3f58dcdf..0be7bf0bfc41 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> @@ -103,7 +103,8 @@
> >>>>>>>>>>        (BIT(DPU_INTF_INPUT_CTRL) | \
> >>>>>>>>>>         BIT(DPU_INTF_TE) | \
> >>>>>>>>>>         BIT(DPU_INTF_STATUS_SUPPORTED) | \
> >>>>>>>>>> -     BIT(DPU_DATA_HCTL_EN))
> >>>>>>>>>> +     BIT(DPU_DATA_HCTL_EN) | \
> >>>>>>>>>> +     BIT(DPU_INTF_DATABUS_WIDEN))
> >>>>>>>>>
> >>>>>>>>> This doesn't work.  DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
> >>>>>>>>> In the
> >>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
> >>>>>>>>> only,
> >>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
> >>>>>>>>> corrupted.
> >>>>>>>
> >>>>
> >>>> I looked at the internal docs and also this change. This change is
> >>>> incorrect because this will try to enable widebus for DPU >= 5.0 and
> >>>> DSI  >= 2.5
> >>>>
> >>>> That was not the intended right condition as thats not what the docs
> >>>> say.
> >>>>
> >>>> We should enable for DPU >= 7.0 and DSI >= 2.5
> >>>>
> >>>> Is there any combination where this compatibility is broken? That
> >>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
> >>>> was incorrect)
> >>>>
> >>>> Part of this confusion is because of catalog macro re-use again.
> >>>>
> >>>> This series is a good candidate and infact I think we should only do
> >>>> core_revision based check on DPU and DSI to avoid bringing the
> >>>> catalog mess into this.
> >>>
> >>> I have just a single request here: can we please have the same
> >>> approach for both DSI and DP? I don't mind changing DP code if it
> >>> makes it better. If you don't have better reasons, I like the idea of
> >>> DSI/DP dictating whether wide bus should be used on the particular
> >>> interface. It allows us to handle possible errata or corner cases
> >>> there. Another option would be to make DPU tell DSI / DP whether the
> >>> wide bus is enabled or not, but I'd say, this is slightly worse
> >>> solution.
> >>>
> >>
> >> Today, DP's widebus does not check if DPU supports that or not.
> >>
> >> DPU encoder queries the DP whether widebus is available and enables it.
> >>
> >> We can also do the same thing for DSI.
> >>
> >> So for intf_type of DSI, DPU encoder will query DSI if it supports
> >> widebus.
> >
> > Not if it supports wide bus. But the check is whether enabling wide bus
> > is requested by the output driver (DSI/DP).
>
> Hi Dmitry,
>
> Can you explain what you mean by "requested by output driver"? FWIW, if
> the DSI version supports wide bus && if DSC is enabled, then wide bus
> will be enabled in DSI.

Like for DP, let DSI select whether a wide bus should be enabled or
not, then let DPU get this flag from DSI.

>
> Thanks,
>
> Jessica Zhang
>
> >
> >>
> >> Then DSI will do its version checks and for DSC it will say yes.
> >>
> >> This way, we will never check for the DPU's core revision for DSI and
> >> purely rely of DP/DSI's hw revisions.
> >>
> >> Thats fine with me because that way we again just rely on the hw
> >> revision to enable the feature.
> >>
> >> But as a result I am still going to drop this patch which adds widebus
> >> to the catalog as a dpu cap which I always wanted to do anyway as we
> >> will just rely on the DSI and DP hw revisions.
> >
> > Yep.
> >
> >>
> >>>>
> >>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
> >>>>>>> corrupted with this series so something else on this series might be
> >>>>>>> broken.
> >>>>>>>
> >>>>>
> >>>>> Missed this response. That seems strange.
> >>>>>
> >>>>> This series was tested on SM8350 HDK with a command mode panel.
> >>>>>
> >>>>> We will fix the DPU-DSI handshake and post a v2 but your issue
> >>>>> needs investigation in parallel.
> >>>>>
> >>>>> So another bug to track that would be great.
> >>>>>
> >>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
> >>>>>>>>> communicate
> >>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
> >>>>>>>>
> >>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
> >>>>>>>> asks the
> >>>>>>>> actual video output driver if widebus is to be enabled.
> >>>>>>>
> >>>>>>
> >>>>>> I was afraid of this. This series was made on an assumption that
> >>>>>> the DPU version of widebus and DSI version of widebus would be
> >>>>>> compatible but looks like already SM8150 is an outlier.
> >>>>>>
> >>>>>> Yes, I think we have to go with second approach.
> >>>>>>
> >>>>>> DPU queries DSI if it supports widebus and enables it.
> >>>>>>
> >>>>>> Thanks for your responses. We will post a v2.
> >>>>>>
> >>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
> >>>>>>> feature,
> >>>>>>> but the DSI does not until two revisions later?  Or is this
> >>>>>>> available on
> >>>>>>> every interface, but only for a different (probably DP) encoder
> >>>>>>> block?
> >>>>>>>
> >>>>>>
> >>>>>> Yes its strange.
> >>>>>>
> >>>>
> >>>> I have clarified this above. Its not strange but appeared strange
> >>>> because we were checking wrong conditions.
> >>>>
> >>>>
> >>>>>>> - Marijn
> >>>
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry