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@xxxxxxxxxxxxxxxx> (v4)
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
