Messages in this thread | | | Date | Fri, 30 Jun 2023 10:59:12 -0700 | Subject | Re: [PATCH RFC v4 3/7] drm/atomic: Move framebuffer checks to helper | From | Jessica Zhang <> |
| |
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 >
| |