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

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 6/20/2023 3:11 PM, Dmitry Baryshkov wrote:
> 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.

Got it -- so basically have DSI do the version check *and* the DSC check 
in some `msm_dsi_is_widebus_enabled()` helper and have DPU use that 
helper to check if widebus is enabled.

I think that should be fine.

Thanks,

Jessica Zhang

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