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

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

On 14/06/2023 04:57, 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(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
  

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.

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?

- Marijn

>  #define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index b860784ade72..b9939e00f5e0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -182,6 +182,7 @@ enum {
>   *                                  than video timing
>   * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>   * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
> + * @DPU_INTF_DATABUS_WIDEN          INTF block has DATABUS_WIDEN register
>   * @DPU_INTF_MAX
>   */
>  enum {
> @@ -190,6 +191,7 @@ enum {
>  	DPU_DATA_HCTL_EN,
>  	DPU_INTF_STATUS_SUPPORTED,
>  	DPU_INTF_DATA_COMPRESS,
> +	DPU_INTF_DATABUS_WIDEN,
>  	DPU_INTF_MAX
>  };
>  
> 
> -- 
> 2.40.1
>
  

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

> 
> - Marijn
> 
>>   #define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index b860784ade72..b9939e00f5e0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -182,6 +182,7 @@ enum {
>>    *                                  than video timing
>>    * @DPU_INTF_STATUS_SUPPORTED       INTF block has INTF_STATUS register
>>    * @DPU_INTF_DATA_COMPRESS          INTF block has DATA_COMPRESS register
>> + * @DPU_INTF_DATABUS_WIDEN          INTF block has DATABUS_WIDEN register
>>    * @DPU_INTF_MAX
>>    */
>>   enum {
>> @@ -190,6 +191,7 @@ enum {
>>   	DPU_DATA_HCTL_EN,
>>   	DPU_INTF_STATUS_SUPPORTED,
>>   	DPU_INTF_DATA_COMPRESS,
>> +	DPU_INTF_DATABUS_WIDEN,
>>   	DPU_INTF_MAX
>>   };
>>   
>>
>> -- 
>> 2.40.1
>>
  

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.

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.

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

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?

- Marijn
  

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

> - Marijn
  

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

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.

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

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.

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

On 2023-06-14 13:39:57, 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

That makes more sense, DSI 2.5 is SM8350 which has DPU 7.0.

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

No clue if there are any interim SoCs...

> Part of this confusion is because of catalog macro re-use again.

Somewhat agreed.  SC7180 is a DPU 6.2 SoC, and for this mask to be used
across DPU 5.x and above it should have been renamed to SM8150 and as
suggested by Dmitry, have DPU_5_x_` as suffix.

As I've asked many times before, we should inline these masks (not just
the macros) (disclaimer: haven't reviewed if Dmitry's series actually
does so!).

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

No worries.  But don't forget to look at the comments on patch 2/3
either.  Some of it is a continuation of pclk scaling factor for DSC
which discussion seems to have ceased on.

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

Will see if I can set that up for you!

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

Fwiw SM8250 would have been an outlier as well :)

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

No hurry, btw.  As alluded to above, let's also look at the comments on
patch 2/3 and discuss how this affects pclk.

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

Hehe :)

- Marijn
  

On 2023-06-14 14:23:38, Marijn Suijten wrote:
<snip>
> 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.

Never mind, this was a bad conflict-resolve.  Jessica's original
BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
only writing, and the way I conflict-resolved that caused the write of
BURST_MODE to overwrite the RMW DATABUS_WIDEN.

If both are moved to dsi_ctrl_config(), we could do a read, add both
flags in conditionally, and write.

- Marijn
  

On 6/14/2023 3:49 PM, Marijn Suijten wrote:
> On 2023-06-14 14:23:38, Marijn Suijten wrote:
> <snip>
>> 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.
> 
> Never mind, this was a bad conflict-resolve.  Jessica's original
> BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
> only writing, and the way I conflict-resolved that caused the write of
> BURST_MODE to overwrite the RMW DATABUS_WIDEN.
> 
> If both are moved to dsi_ctrl_config(), we could do a read, add both
> flags in conditionally, and write.
> 

So just to confirm, there is no issue on your 8350 setup with widebus 
enabled right?

> - Marijn
  

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.

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.

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

On 6/14/2023 2:41 PM, Marijn Suijten wrote:
> On 2023-06-14 13:39:57, 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
> 
> That makes more sense, DSI 2.5 is SM8350 which has DPU 7.0.
> 
>> 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)
> 
> No clue if there are any interim SoCs...
> 
>> Part of this confusion is because of catalog macro re-use again.
> 
> Somewhat agreed.  SC7180 is a DPU 6.2 SoC, and for this mask to be used
> across DPU 5.x and above it should have been renamed to SM8150 and as
> suggested by Dmitry, have DPU_5_x_` as suffix.
> 
> As I've asked many times before, we should inline these masks (not just
> the macros) (disclaimer: haven't reviewed if Dmitry's series actually
> does so!).
> 

Yes it does inline it. I am halfway through that rework got stuck in one 
of the patches.

>> 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.
>>
>>>>> 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.
> 
> No worries.  But don't forget to look at the comments on patch 2/3
> either.  Some of it is a continuation of pclk scaling factor for DSC
> which discussion seems to have ceased on.
> 
>>> 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.
> 
> Will see if I can set that up for you!
> 

Now, it seems this is resolved so bug is not needed?

>>>>>>> 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.
> 
> Fwiw SM8250 would have been an outlier as well :)
> 
>>>> 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.
> 
> No hurry, btw.  As alluded to above, let's also look at the comments on
> patch 2/3 and discuss how this affects pclk.
> 
>>>>> 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.
> 
> Hehe :)
> 
> - Marijn
  

On 2023-06-16 14:13:22, Abhinav Kumar wrote:
<snip>
> > As I've asked many times before, we should inline these masks (not just
> > the macros) (disclaimer: haven't reviewed if Dmitry's series actually
> > does so!).
> > 
> 
> Yes it does inline it. I am halfway through that rework got stuck in one 
> of the patches.

Neat, I'm still going through it but there are some conflicts with other
reworks that make it harder to review and test in parallel.

> >>>>> 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.
> > 
> > No worries.  But don't forget to look at the comments on patch 2/3
> > either.  Some of it is a continuation of pclk scaling factor for DSC
> > which discussion seems to have ceased on.
> > 
> >>> 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.
> > 
> > Will see if I can set that up for you!
> > 
> 
> Now, it seems this is resolved so bug is not needed?

Indeed, as mentioned in another message.  Looking forward to v2.

- Marijn
  

On 2023-06-16 14:06:47, Abhinav Kumar wrote:
> On 6/14/2023 3:49 PM, Marijn Suijten wrote:
> > On 2023-06-14 14:23:38, Marijn Suijten wrote:
> > <snip>
> >> 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.
> > 
> > Never mind, this was a bad conflict-resolve.  Jessica's original
> > BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
> > only writing, and the way I conflict-resolved that caused the write of
> > BURST_MODE to overwrite the RMW DATABUS_WIDEN.
> > 
> > If both are moved to dsi_ctrl_config(), we could do a read, add both
> > flags in conditionally, and write.
> > 
> 
> So just to confirm, there is no issue on your 8350 setup with widebus 
> enabled right?

After properly conflict-resolving both series so that they either both
RMW, or combine the two bit-sets (the same you just discussed _not_
doing on the DPU side for DATABUS_WIDEN|COMPRESSION...) before a single
write, it works fine!

- Marijn
  

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

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

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.

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
>
  

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

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