| Message ID | 20170731124516.29595-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, Thank you for the patch. On Monday 31 Jul 2017 14:45:16 Daniel Vetter wrote: > 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. > > 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> I wish we could do better, but I think that's the best we can achieve without removing the hack. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 95 ++++++++++++++++++++-------------- > 1 file changed, 59 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..16d8b291921b > 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,20 +544,37 @@ 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 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, > .set_config = drm_atomic_helper_set_config, > @@ -559,7 +582,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = { > .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,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..16d8b291921b 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,20 +544,37 @@ 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 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, .set_config = drm_atomic_helper_set_config, @@ -559,7 +582,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = { .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,
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. 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 | 95 +++++++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 36 deletions(-)