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

Abhinav Kumar quic_abhinavk at quicinc.com
Tue Jan 30 06:03:28 UTC 2024 


On 1/29/2024 9:28 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> 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 :(
> 
> What about explicitly disabling it in such a case?
> I mean something like:
> 
> if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en))
>     intf_cfg |= INTF_CFG2_HCTL_EN;
> 

Condition is correct now. But we dont have enc or dpu version in this 
function.

We need to pass a new parameter called compression_en to 
dpu_hw_intf_timing_params and set it when dsc is used and then do this. 
We have widebus_en already in dpu_hw_intf_timing_params.

This was anyway part of the DSC over DP but we can add that here and 
then the if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) is indicative 
of dpu version >=5 so we can move this setup there.

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

More information about the dri-devel mailing list