[PATCH v5 6/8] drm/msm/dsi: Add check for slice_width in dsi_timing_setup

[Dmitry Baryshkov]

On 13/04/2023 01:40, Abhinav Kumar wrote:
> 
> 
> On 4/12/2023 12:24 PM, Dmitry Baryshkov wrote:
>> On 12/04/2023 22:09, Jessica Zhang wrote:
>>> Add a check for valid dsc->slice_width value in dsi_timing_setup.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 508577c596ff..6a6218a9655f 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -937,6 +937,12 @@ static void dsi_timing_setup(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>               return;
>>>           }
>>> +        if (!dsc->slice_width || (mode->hdisplay < dsc->slice_width)) {
>>
>> This is an erroneous condition, correct. Can we move it to a better 
>> place, where we can return an error instead of ignoring it?
>>
>> I'd say that we should validate dsc->slice_width at the 
>> dsi_host_attach(). It well might be a good idea to add a helper that 
>> validates required dsc properties (e.g. version, bpp/bpc, slice_width, 
>> slice_height, slice_count).
>>
>> As for the mode->hdisplay, we have the following code in 
>> msm_dsi_host_check_dsc() (where pic_width = mode->hdisplay):
>>
>> if (pic_width % dsc->slice_width) {...}
>>
>> This way the only way how mode->hdisplay can be less than 
>> dsc->slice_width is if mode->hdisplay is 0 (which is forbidden if I 
>> remember correctly). So the second part of the check is useless.
>>
> 
> Lets drop this from this series and come up with a better approach to 
> validate dsc params. We will take it up once dsc over dsi and dp lands.

Sure, why not.

> 
>>> +            pr_err("DSI: invalid slice width %d (pic_width: %d)\n",
>>> +                   dsc->slice_width, mode->hdisplay);
>>> +            return;
>>> +        }
>>> +
>>>           dsc->pic_width = mode->hdisplay;
>>>           dsc->pic_height = mode->vdisplay;
>>>           DBG("Mode %dx%d\n", dsc->pic_width, dsc->pic_height);
>>>
>>

-- 
With best wishes
Dmitry