Re: [PATCH] drm/omap: Rework the rotation-on-crtc hack

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/omap: Rework the rotation-on-crtc hack
Date: Tue, 01 Aug 2017 13:20:59 +0300	[thread overview]
Message-ID: <2563110.xQjJWl2ggy@avalon> (raw)
In-Reply-To: <b03263d9-8bc5-8203-c097-f64369021f30@linux.intel.com>

Hi Maarten,

On Tuesday 01 Aug 2017 07:59:13 Maarten Lankhorst wrote:
> Op 31-07-17 om 17:42 schreef Daniel Vetter:
> > I want/need to rework the core property handling, and this hack is
> > getting in the way. But since it's a non-standard propety only used by
> > legacy userspace we know that this will only every be called from
> > ioctl code. And never on some other free-standing state struct, where
> > this old hack wouldn't work either.
> > 
> > v2: don't forget zorder and get_property!
> > 
> > v3: Shadow the legacy state to avoid locking issues in get_property
> > (Maarten).
> > 
> > v4: Review from Laurent
> > - Move struct omap_crtc_state into omap_crtc.c
> > - Clean up comments.
> > - Don't forget to copy the shadowed state over on duplicate.
> > 
> > v5: Don't forget to update the reset handler (Maarten).
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v4)
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 109 ++++++++++++++++++++-----------
> >  1 file changed, 72 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..9014085c33df
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c

[snip]

> >  static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
> >  					 struct drm_crtc_state *state,
> >  					 struct drm_property *property,
> >  					 uint64_t val)
> >  {
> > -	if (omap_crtc_is_plane_prop(crtc, property)) {
> > -		struct drm_plane_state *plane_state;
> > -		struct drm_plane *plane = crtc->primary;
> > -
> > -		/*
> > -		 * Delegate property set to the primary plane. Get the plane
> > -		 * state and set the property directly.
> > -		 */
> > -
> > -		plane_state = drm_atomic_get_plane_state(state->state, plane);
> > -		if (IS_ERR(plane_state))
> > -			return PTR_ERR(plane_state);
> > +	struct omap_drm_private *priv = crtc->dev->dev_private;
> > +	struct omap_crtc_state *omap_state = to_omap_crtc_state(state);
> > +	struct drm_plane_state *plane_state;
> > 
> > -		return drm_atomic_plane_set_property(plane, plane_state,
> > -				property, val);
> > +	/*
> > +	 * Delegate property set to the primary plane. Get the plane state and
> > +	 * set the property directly, but keep a shadow copy for the
> > +	 * atomic_get_property callback.
> > +	 */
> > +	plane_state = drm_atomic_get_plane_state(state->state, crtc->primary);
> > +	if (IS_ERR(plane_state))
> > +		return PTR_ERR(plane_state);
> > +
> > +	if (property == crtc->primary->rotation_property) {
> > +		plane_state->rotation = val;
> > +		omap_state->rotation = val;
> > +	} else if (property == priv->zorder_prop) {
> > +		plane_state->zpos = val;
> > +		omap_state->zpos = val;
> 
> With atomic we should try to always make the getprop values accurate, or
> compositors might have troubles restoring.
> 
> I would update the shadow values in omap_crtc_atomic_check through
> omap_crtc_atomic_check instead, with something like this:
> 
> +       pri_state = drm_atomic_get_new_plane_state(crtc->primary,
> state->state);
> +       if (pri_state) {
> +               struct omap_crtc_state *omap_crtc_state =
> +                       to_omap_crtc_state(state);
> +
> +               omap_crtc_state->zpos = pri_state->zpos;
> +               omap_crtc_state->rotation = pri_state->rotation;
> +       }
> 
> That way even when updating the property through the primary plane, it gets
> reflected correctly. For example when vt switching with fbdev.

Let's not make it over-complicated. This hack is only needed fo the legacy X 
OMAP modesetting driver. The CRTC zpos and rotation properties should not be 
used through the atomic API.

> > +	} else {
> > +		return -EINVAL;
> >  	}
> > 
> > -	return -EINVAL;
> > +	return 0;
> >  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel