[Intel-gfx] [PATCH 3/3] drm/atomic: Use explicit old/new state in drm_atomic_plane_check()
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Nov 5 14:37:18 UTC 2018
On Mon, Nov 05, 2018 at 10:33:47AM +0100, Daniel Vetter wrote:
> On Thu, Nov 01, 2018 at 08:46:46PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > 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 at linux.intel.com>
> > ---
> > 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.
I guess it's a matter of taste. I think my new style is much more
succinct and reads more natural:
"if old exists and new exists and old is not new we're swithing crtcs"
vs.
"if old doesn't exist or new doesn't exist we're not switching
crtcs, otherwise if old is new we're not switching crtcs, otherwise
we're switching crtcs".
But meh. So I'll change it back.
>
> With the old layout here this is:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > }
> >
> > /**
> > * 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list