On Thu, Nov 01, 2018 at 08:46:46PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Convert drm_atomic_plane_check() over to using explicit old vs. new > plane states. Avoids the confusion of "what does plane->state mean > again?". > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/drm_atomic.c | 90 ++++++++++++++++++------------------ > 1 file changed, 46 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index dde696181efe..46f8d798efaf 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -492,108 +492,109 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > EXPORT_SYMBOL(drm_atomic_get_plane_state); > > static bool > -plane_switching_crtc(struct drm_atomic_state *state, > - struct drm_plane *plane, > - struct drm_plane_state *plane_state) > +plane_switching_crtc(const struct drm_plane_state *old_plane_state, > + const struct drm_plane_state *new_plane_state) > { > - if (!plane->state->crtc || !plane_state->crtc) > - return false; > - > - if (plane->state->crtc == plane_state->crtc) > - return false; > - > /* This could be refined, but currently there's no helper or driver code > * to implement direct switching of active planes nor userspace to take > * advantage of more direct plane switching without the intermediate > * full OFF state. > */ > - return true; > + return old_plane_state->crtc && new_plane_state->crtc && > + old_plane_state->crtc != new_plane_state->crtc; I think the old layout was clearer, instead of an obscure monster condition. With the old layout here this is: Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > } > > /** > * drm_atomic_plane_check - check plane state > - * @plane: plane to check > - * @state: plane state to check > + * @old_plane_state: old plane state to check > + * @new_plane_state: new plane state to check > * > * Provides core sanity checks for plane state. > * > * RETURNS: > * Zero on success, error code on failure > */ > -static int drm_atomic_plane_check(struct drm_plane *plane, > - struct drm_plane_state *state) > +static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state, > + const struct drm_plane_state *new_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; > int ret; > > /* either *both* CRTC and FB must be set, or neither */ > - if (state->crtc && !state->fb) { > + if (crtc && !fb) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] CRTC set but no FB\n", > plane->base.id, plane->name); > return -EINVAL; > - } else if (state->fb && !state->crtc) { > + } else if (fb && !crtc) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] FB set but no CRTC\n", > plane->base.id, plane->name); > return -EINVAL; > } > > /* if disabled, we don't care about the rest of the state: */ > - if (!state->crtc) > + if (!crtc) > return 0; > > /* Check whether this plane is usable on this CRTC */ > - if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) { > + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { > DRM_DEBUG_ATOMIC("Invalid [CRTC:%d:%s] for [PLANE:%d:%s]\n", > - state->crtc->base.id, state->crtc->name, > + crtc->base.id, crtc->name, > plane->base.id, plane->name); > return -EINVAL; > } > > /* Check whether this plane supports the fb pixel format. */ > - ret = drm_plane_check_pixel_format(plane, state->fb->format->format, > - state->fb->modifier); > + ret = drm_plane_check_pixel_format(plane, fb->format->format, > + fb->modifier); > if (ret) { > struct drm_format_name_buf format_name; > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid pixel format %s, modifier 0x%llx\n", > plane->base.id, plane->name, > - drm_get_format_name(state->fb->format->format, > + drm_get_format_name(fb->format->format, > &format_name), > - state->fb->modifier); > + fb->modifier); > return ret; > } > > /* Give drivers some help against integer overflows */ > - if (state->crtc_w > INT_MAX || > - state->crtc_x > INT_MAX - (int32_t) state->crtc_w || > - state->crtc_h > INT_MAX || > - state->crtc_y > INT_MAX - (int32_t) state->crtc_h) { > + if (new_plane_state->crtc_w > INT_MAX || > + new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w || > + new_plane_state->crtc_h > INT_MAX || > + new_plane_state->crtc_y > INT_MAX - (int32_t) new_plane_state->crtc_h) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid CRTC coordinates %ux%u+%d+%d\n", > plane->base.id, plane->name, > - state->crtc_w, state->crtc_h, > - state->crtc_x, state->crtc_y); > + new_plane_state->crtc_w, new_plane_state->crtc_h, > + new_plane_state->crtc_x, new_plane_state->crtc_y); > return -ERANGE; > } > > - fb_width = state->fb->width << 16; > - fb_height = state->fb->height << 16; > + 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) { > + 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_DEBUG_ATOMIC("[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, > - state->fb->width, state->fb->height); > + 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; > } > > - if (plane_switching_crtc(state->state, plane, state)) { > + if (plane_switching_crtc(old_plane_state, new_plane_state)) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", > plane->base.id, plane->name); > return -EINVAL; > @@ -966,7 +967,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > struct drm_device *dev = state->dev; > struct drm_mode_config *config = &dev->mode_config; > struct drm_plane *plane; > - struct drm_plane_state *plane_state; > + struct drm_plane_state *old_plane_state; > + struct drm_plane_state *new_plane_state; > struct drm_crtc *crtc; > struct drm_crtc_state *old_crtc_state; > struct drm_crtc_state *new_crtc_state; > @@ -976,8 +978,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > - for_each_new_plane_in_state(state, plane, plane_state, i) { > - ret = drm_atomic_plane_check(plane, plane_state); > + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > + ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > if (ret) { > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n", > plane->base.id, plane->name); > -- > 2.18.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel