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

[Abhinav Kumar]

On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
>>> 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.
>>>
>>
>> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
>> widebus for the versions I wrote.
>>
>> Basically for the non-compressed case.
>>
>> I will write something up to fix this for DSI. I think this can go as a
>> bug fix.
>>
>> But that does not change the DP conditions OR in other words, I dont see
>> anything wrong with this patch yet.
>>
>>>>
>>>> 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?
>>>
>>
>> This bit basically just tells that the data sent per line is programmed
>> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
>>
>>           if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
>>                   DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>>                   DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
>> display_data_hctl);
>>                   DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>>           }
>>
>> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.
> 
> Can we enable it unconditionally for DPU >= 5.0?
> 

There is a corner-case that we should not enable it when compression is 
enabled without widebus as per the docs :(

For DP there will not be a case like that because compression and 
widebus go together but for DSI, it is possible.

So I found that the reset value of this register does cover all cases 
for DPU >= 7.0 so below fix will address the DSI concern and will fix 
the issue even for YUV420 cases such as this one for DPU >= 7.0

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 6bba531d6dc4..cbd5ebd516cd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -168,6 +168,8 @@ 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
          */
+
+       intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2);
         if (p->wide_bus_en)
                 intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | 
INTF_CFG2_DATA_HCTL_EN;


But, this does not still work for DPU < 7.0 such as sc7180 if we try 
YUV420 over DP on that because its DPU version is 6.2 so we will have to 
keep this patch for those cases.

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