| Message ID | 20230404-solid-fill-v4-3-f4ec0caa742d@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for Solid Fill Planes | expand |
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@quicinc.com> > --- > 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. > + ret = drm_atomic_check_fb(new_plane_state); > + if (ret) > + return ret; > } > > if (plane_switching_crtc(old_plane_state, new_plane_state)) { >
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@quicinc.com> >> --- >> 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 >
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) { + ret = drm_atomic_check_fb(new_plane_state); + if (ret) + return ret; } if (plane_switching_crtc(old_plane_state, new_plane_state)) {
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@quicinc.com> --- drivers/gpu/drm/drm_atomic.c | 130 ++++++++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 56 deletions(-)