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

[Dmitry Baryshkov]

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.

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

-- 
With best wishes
Dmitry