| Message ID | 20170731154235.6402-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > @@ -26,6 +26,16 @@ > > #include "omap_drv.h" > > +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base) > + > +struct omap_crtc_state { > + /* Must be first. */ > + struct drm_crtc_state base; > + /* Shadow values for legacy userspace support. */ > + unsigned int rotation; > + unsigned int zpos; > +}; > + > #define to_omap_crtc(x) container_of(x, struct omap_crtc, base) > > struct omap_crtc { > @@ -498,39 +508,35 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, > spin_unlock_irq(&crtc->dev->event_lock); > } > > -static bool omap_crtc_is_plane_prop(struct drm_crtc *crtc, > - struct drm_property *property) > -{ > - struct drm_device *dev = crtc->dev; > - struct omap_drm_private *priv = dev->dev_private; > - > - return property == priv->zorder_prop || > - property == crtc->primary->rotation_property; > -} > - > 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. > + } else { > + return -EINVAL; > } > > - return -EINVAL; > + return 0; > } > > static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, > @@ -538,28 +544,57 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, > struct drm_property *property, > uint64_t *val) > { > - if (omap_crtc_is_plane_prop(crtc, property)) { > - /* > - * Delegate property get to the primary plane. The > - * drm_atomic_plane_get_property() function isn't exported, but > - * can be called through drm_object_property_get_value() as that > - * will call drm_atomic_get_property() for atomic drivers. > - */ > - return drm_object_property_get_value(&crtc->primary->base, > - property, val); > - } > + struct omap_drm_private *priv = crtc->dev->dev_private; > + struct omap_crtc_state *omap_state = to_omap_crtc_state(state); > + > + if (property == crtc->primary->rotation_property) > + return omap_state->rotation; > + else if (property == priv->zorder_prop) > + return omap_state->zpos; > > return -EINVAL; > } > > +static void omap_crtc_reset(struct drm_crtc *crtc) > +{ > + if (crtc->state) > + __drm_atomic_helper_crtc_destroy_state(crtc->state); > + > + kfree(crtc->state); > + crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL); > + > + if (crtc->state) > + crtc->state->crtc = crtc; Hm, perhaps add a __drm_atomic_helper_crtc_reset, and (perhaps) for planes too? Similar to __drm_atomic_helper_connector_reset, in case we want to add something in the future that might need to be initialised. :) > +} > + > +static struct drm_crtc_state * > +omap_crtc_duplicate_state(struct drm_crtc *crtc) > +{ > + struct omap_crtc_state *state, *current_state; > + > + if (WARN_ON(!crtc->state)) > + return NULL; > + > + current_state = to_omap_crtc_state(crtc->state); > + > + state = kmalloc(sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); > + > + state->zpos = current_state->zpos; > + state->rotation = current_state->rotation; > + > + return &state->base; > +} > + > static const struct drm_crtc_funcs omap_crtc_funcs = { > - .reset = drm_atomic_helper_crtc_reset, > + .reset = omap_crtc_reset, > .set_config = drm_atomic_helper_set_config, > .destroy = omap_crtc_destroy, > .page_flip = drm_atomic_helper_page_flip, > .gamma_set = drm_atomic_helper_legacy_gamma_set, > .set_property = drm_atomic_helper_crtc_set_property, > - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > + .atomic_duplicate_state = omap_crtc_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > .atomic_set_property = omap_crtc_atomic_set_property, > .atomic_get_property = omap_crtc_atomic_get_property,
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; > > }
On Tue, Aug 1, 2017 at 12:20 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tuesday 01 Aug 2017 07:59:13 Maarten Lankhorst wrote: >> + 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. Marten is right, the atomic properties are all such that you can unconditionally restore them (e.g. fences report a no-op value), to make compositor switching easier. Not sure anyone implements that, but I think it's a useful idea to keep. I'll respin (or maybe Maarten simply submits his patch with a proper sob ...). -Daniel
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 @@ -26,6 +26,16 @@ #include "omap_drv.h" +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base) + +struct omap_crtc_state { + /* Must be first. */ + struct drm_crtc_state base; + /* Shadow values for legacy userspace support. */ + unsigned int rotation; + unsigned int zpos; +}; + #define to_omap_crtc(x) container_of(x, struct omap_crtc, base) struct omap_crtc { @@ -498,39 +508,35 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, spin_unlock_irq(&crtc->dev->event_lock); } -static bool omap_crtc_is_plane_prop(struct drm_crtc *crtc, - struct drm_property *property) -{ - struct drm_device *dev = crtc->dev; - struct omap_drm_private *priv = dev->dev_private; - - return property == priv->zorder_prop || - property == crtc->primary->rotation_property; -} - 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; + } else { + return -EINVAL; } - return -EINVAL; + return 0; } static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, @@ -538,28 +544,57 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, struct drm_property *property, uint64_t *val) { - if (omap_crtc_is_plane_prop(crtc, property)) { - /* - * Delegate property get to the primary plane. The - * drm_atomic_plane_get_property() function isn't exported, but - * can be called through drm_object_property_get_value() as that - * will call drm_atomic_get_property() for atomic drivers. - */ - return drm_object_property_get_value(&crtc->primary->base, - property, val); - } + struct omap_drm_private *priv = crtc->dev->dev_private; + struct omap_crtc_state *omap_state = to_omap_crtc_state(state); + + if (property == crtc->primary->rotation_property) + return omap_state->rotation; + else if (property == priv->zorder_prop) + return omap_state->zpos; return -EINVAL; } +static void omap_crtc_reset(struct drm_crtc *crtc) +{ + if (crtc->state) + __drm_atomic_helper_crtc_destroy_state(crtc->state); + + kfree(crtc->state); + crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL); + + if (crtc->state) + crtc->state->crtc = crtc; +} + +static struct drm_crtc_state * +omap_crtc_duplicate_state(struct drm_crtc *crtc) +{ + struct omap_crtc_state *state, *current_state; + + if (WARN_ON(!crtc->state)) + return NULL; + + current_state = to_omap_crtc_state(crtc->state); + + state = kmalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); + + state->zpos = current_state->zpos; + state->rotation = current_state->rotation; + + return &state->base; +} + static const struct drm_crtc_funcs omap_crtc_funcs = { - .reset = drm_atomic_helper_crtc_reset, + .reset = omap_crtc_reset, .set_config = drm_atomic_helper_set_config, .destroy = omap_crtc_destroy, .page_flip = drm_atomic_helper_page_flip, .gamma_set = drm_atomic_helper_legacy_gamma_set, .set_property = drm_atomic_helper_crtc_set_property, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, + .atomic_duplicate_state = omap_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, .atomic_set_property = omap_crtc_atomic_set_property, .atomic_get_property = omap_crtc_atomic_get_property,