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

[Abhinav Kumar]

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.

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