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

[Abhinav Kumar]

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.

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