Re: [PATCH RFC v4 3/7] drm/atomic: Move framebuffer checks to helper

[Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>]

On 6/29/2023 5:43 PM, Dmitry Baryshkov wrote:
> On 30/06/2023 03:25, Jessica Zhang wrote:
> > Currently framebuffer checks happen directly in
> > drm_atomic_plane_check(). Move these checks into their own helper
> > method.
> >
> > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/drm_atomic.c | 130 
> > ++++++++++++++++++++++++-------------------
> >   1 file changed, 74 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b4c6ffc438da..404b984d2d9f 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -580,6 +580,76 @@ plane_switching_crtc(const struct drm_plane_state 
> > *old_plane_state,
> >       return true;
> >   }
> > +static int drm_atomic_check_fb(const struct drm_plane_state *state)
> > +{
> > +    struct drm_plane *plane = state->plane;
> > +    const struct drm_framebuffer *fb = state->fb;
> > +    struct drm_mode_rect *clips;
> > +
> > +    uint32_t num_clips;
> > +    unsigned int fb_width, fb_height;
> > +    int ret;
> > +
> > +    /* Check whether this plane supports the fb pixel format. */
> > +    ret = drm_plane_check_pixel_format(plane, fb->format->format,
> > +                       fb->modifier);
> > +
> > +    if (ret) {
> > +        drm_dbg_atomic(plane->dev,
> > +                   "[PLANE:%d:%s] invalid pixel format %p4cc, 
> > modifier 0x%llx\n",
> > +                   plane->base.id, plane->name,
> > +                   &fb->format->format, fb->modifier);
> > +        return ret;
> > +    }
> > +
> > +    fb_width = fb->width << 16;
> > +    fb_height = fb->height << 16;
> > +
> > +    /* Make sure source coordinates are inside the fb. */
> > +    if (state->src_w > fb_width ||
> > +        state->src_x > fb_width - state->src_w ||
> > +        state->src_h > fb_height ||
> > +        state->src_y > fb_height - state->src_h) {
> > +        drm_dbg_atomic(plane->dev,
> > +                   "[PLANE:%d:%s] invalid source coordinates "
> > +                   "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> > +                   plane->base.id, plane->name,
> > +                   state->src_w >> 16,
> > +                   ((state->src_w & 0xffff) * 15625) >> 10,
> > +                   state->src_h >> 16,
> > +                   ((state->src_h & 0xffff) * 15625) >> 10,
> > +                   state->src_x >> 16,
> > +                   ((state->src_x & 0xffff) * 15625) >> 10,
> > +                   state->src_y >> 16,
> > +                   ((state->src_y & 0xffff) * 15625) >> 10,
> > +                   fb->width, fb->height);
> > +        return -ENOSPC;
> > +    }
> > +
> > +    clips = __drm_plane_get_damage_clips(state);
> > +    num_clips = drm_plane_get_damage_clips_count(state);
> > +
> > +    /* Make sure damage clips are valid and inside the fb. */
> > +    while (num_clips > 0) {
> > +        if (clips->x1 >= clips->x2 ||
> > +            clips->y1 >= clips->y2 ||
> > +            clips->x1 < 0 ||
> > +            clips->y1 < 0 ||
> > +            clips->x2 > fb_width ||
> > +            clips->y2 > fb_height) {
> > +            drm_dbg_atomic(plane->dev,
> > +                       "[PLANE:%d:%s] invalid damage clip %d %d %d 
> > %d\n",
> > +                       plane->base.id, plane->name, clips->x1,
> > +                       clips->y1, clips->x2, clips->y2);
> > +            return -EINVAL;
> > +        }
> > +        clips++;
> > +        num_clips--;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   /**
> >    * drm_atomic_plane_check - check plane state
> >    * @old_plane_state: old plane state to check
> > @@ -596,9 +666,6 @@ static int drm_atomic_plane_check(const struct 
> > drm_plane_state *old_plane_state,
> >       struct drm_plane *plane = new_plane_state->plane;
> >       struct drm_crtc *crtc = new_plane_state->crtc;
> >       const struct drm_framebuffer *fb = new_plane_state->fb;
> > -    unsigned int fb_width, fb_height;
> > -    struct drm_mode_rect *clips;
> > -    uint32_t num_clips;
> >       int ret;
> >       /* either *both* CRTC and FB must be set, or neither */
> > @@ -625,17 +692,6 @@ static int drm_atomic_plane_check(const struct 
> > drm_plane_state *old_plane_state,
> >           return -EINVAL;
> >       }
> > -    /* Check whether this plane supports the fb pixel format. */
> > -    ret = drm_plane_check_pixel_format(plane, fb->format->format,
> > -                       fb->modifier);
> > -    if (ret) {
> > -        drm_dbg_atomic(plane->dev,
> > -                   "[PLANE:%d:%s] invalid pixel format %p4cc, 
> > modifier 0x%llx\n",
> > -                   plane->base.id, plane->name,
> > -                   &fb->format->format, fb->modifier);
> > -        return ret;
> > -    }
> > -
> >       /* Give drivers some help against integer overflows */
> >       if (new_plane_state->crtc_w > INT_MAX ||
> >           new_plane_state->crtc_x > INT_MAX - (int32_t) 
> > new_plane_state->crtc_w ||
> > @@ -649,49 +705,11 @@ static int drm_atomic_plane_check(const struct 
> > drm_plane_state *old_plane_state,
> >           return -ERANGE;
> >       }
> > -    fb_width = fb->width << 16;
> > -    fb_height = fb->height << 16;
> > -    /* Make sure source coordinates are inside the fb. */
> > -    if (new_plane_state->src_w > fb_width ||
> > -        new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> > -        new_plane_state->src_h > fb_height ||
> > -        new_plane_state->src_y > fb_height - new_plane_state->src_h) {
> > -        drm_dbg_atomic(plane->dev,
> > -                   "[PLANE:%d:%s] invalid source coordinates "
> > -                   "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> > -                   plane->base.id, plane->name,
> > -                   new_plane_state->src_w >> 16,
> > -                   ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
> > -                   new_plane_state->src_h >> 16,
> > -                   ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
> > -                   new_plane_state->src_x >> 16,
> > -                   ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
> > -                   new_plane_state->src_y >> 16,
> > -                   ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
> > -                   fb->width, fb->height);
> > -        return -ENOSPC;
> > -    }
> > -
> > -    clips = __drm_plane_get_damage_clips(new_plane_state);
> > -    num_clips = drm_plane_get_damage_clips_count(new_plane_state);
> > -
> > -    /* Make sure damage clips are valid and inside the fb. */
> > -    while (num_clips > 0) {
> > -        if (clips->x1 >= clips->x2 ||
> > -            clips->y1 >= clips->y2 ||
> > -            clips->x1 < 0 ||
> > -            clips->y1 < 0 ||
> > -            clips->x2 > fb_width ||
> > -            clips->y2 > fb_height) {
> > -            drm_dbg_atomic(plane->dev,
> > -                       "[PLANE:%d:%s] invalid damage clip %d %d %d 
> > %d\n",
> > -                       plane->base.id, plane->name, clips->x1,
> > -                       clips->y1, clips->x2, clips->y2);
> > -            return -EINVAL;
> > -        }
> > -        clips++;
> > -        num_clips--;
> > +    if (fb) {
>
> This doesn't only move code, but also changes semantics, making the 
> checks optional if no FB is provided. Consider moving the condition to 
> the next patch. Otherwise LGTM.

Hi Dmitry,

Sounds good.

Thanks,

Jessica Zhang

> > +        ret = drm_atomic_check_fb(new_plane_state);
> > +        if (ret)
> > +            return ret;
> >       }
> >       if (plane_switching_crtc(old_plane_state, new_plane_state)) {
>
> --
> With best wishes
> Dmitry