[PATCH v3 21/27] drm/msm/dpu: simplify dpu_plane_validate_src()

[Abhinav Kumar]

On 2/6/2023 4:27 PM, Dmitry Baryshkov wrote:
> On 07/02/2023 00:40, Abhinav Kumar wrote:
>>
>>
>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>> Since the driver uses clipped src coordinates, there is no need to check
>>> against the fb coordinates. Remove corresponding checks and inline
>>> dpu_plane_validate_src().
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>
>> Can you please explain how the clipping in 
>> drm_atomic_helper_check_plane_state() can allow us to remove checking 
>> the fb co-ordinates?
>>
>> The clipping is done using the mode parameters.
>>
>> So lets say the FB being used is smaller than the source buffer by an 
>> incorrect usermode setting.
>>
>> Then the src sspp shall try to fetch the full image of src rectangle 
>> size from a FB which isnt that big leading to a fault.
> 
> This case is checked by the drm_atomic_plane_check().
> 
>>
>> How does the clipping avoid such a case?
> 
> clipping itself does not. However using clipped coordinates from 
> plane_state->src ensures that they already were validated against the FB 
> dimensions. I'll see if I can change the commit message to make it more 
> obvious.
> 

Ah okay, yeah the commit text confused me a bit to look at the clipping 
code in drm_atomic_helper_check_plane_state().

Yes, please change it to point to the helper which addresses this.

>>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 30 ++++++++---------------
>>>   1 file changed, 10 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index ecf5402ab61a..0986e740b978 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -894,25 +894,6 @@ static void dpu_plane_cleanup_fb(struct 
>>> drm_plane *plane,
>>>                   old_pstate->needs_dirtyfb);
>>>   }
>>> -static bool dpu_plane_validate_src(struct drm_rect *src,
>>> -                   struct drm_rect *fb_rect,
>>> -                   uint32_t min_src_size)
>>> -{
>>> -    /* Ensure fb size is supported */
>>> -    if (drm_rect_width(fb_rect) > MAX_IMG_WIDTH ||
>>> -        drm_rect_height(fb_rect) > MAX_IMG_HEIGHT)
>>> -        return false;
>>> -
>>> -    /* Ensure src rect is above the minimum size */
>>> -    if (drm_rect_width(src) < min_src_size ||
>>> -        drm_rect_height(src) < min_src_size)
>>> -        return false;
>>> -
>>> -    /* Ensure src is fully encapsulated in fb */
>>> -    return drm_rect_intersect(fb_rect, src) &&
>>> -        drm_rect_equals(fb_rect, src);
>>> -}
>>> -
>>>   static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu,
>>>                           const struct dpu_sspp_sub_blks *sblk,
>>>                           struct drm_rect src, const struct 
>>> dpu_format *fmt)
>>> @@ -998,6 +979,14 @@ static int dpu_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>       fb_rect.x2 = new_plane_state->fb->width;
>>>       fb_rect.y2 = new_plane_state->fb->height;
>>> +    /* Ensure fb size is supported */
>>> +    if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH ||
>>> +        drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) {
>>> +        DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n",
>>> +                DRM_RECT_ARG(&fb_rect));
>>> +        return -E2BIG;
>>> +    }
>>> +
>>>       max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>       fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>>> @@ -1012,7 +1001,8 @@ static int dpu_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>           return -EINVAL;
>>>       /* check src bounds */
>>> -    } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, 
>>> &fb_rect, min_src_size)) {
>>> +    } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size ||
>>> +           drm_rect_height(&pipe_cfg->src_rect) < min_src_size) {
>>>           DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>>>                   DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>           return -E2BIG;
>