Message-ID: <6ebab21c-5b1a-f9d7-e0c6-6a091e27761a@quicinc.com>
Date:   Tue, 20 Jun 2023 14:37:47 -0700
From:   Jessica Zhang <quic_jesszhan@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Marijn Suijten <marijn.suijten@...ainline.org>
CC:     <freedreno@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        Sean Paul <sean@...rly.run>
Subject: Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN
 feature flag for DPU >= 5.0



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@...cinc.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.

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
> 

Powered by blists - more mailing lists