| Message ID | 20170725080122.20548-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 25-07-17 om 10:01 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! > > 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 | 64 ++++++++++++++++--------------------- > 1 file changed, 28 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index 14e8a7738b06..efa525442e7d 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -498,39 +498,30 @@ 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 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. > + */ > + 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; > + else if (property == priv->zorder_prop) > + plane_state->zpos = val; > + else > + return -EINVAL; > > - return -EINVAL; > + return 0; > } > > static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, > @@ -538,16 +529,17 @@ 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; > + > + /* > + * Remap to the plane rotation/zorder property. We can peek at the plane > + * state directly since holding the crtc locks gives you a read-lock on > + * the plane state. > + */ > + if (property == crtc->primary->rotation_property) > + return crtc->primary->state->rotation; > + else if (property == priv->zorder_prop) > + return crtc->primary->state->zpos; > > return -EINVAL; > } acquire_ctx for crtc's getprop too then? The comment about read lock is only valid when the plane is bound to the crtc. In general this is not always the case. You can only peak at plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane)) is true. I think we might need -EDEADLK handling for getprop then, even though it's not optimal. :(
On Tue, Jul 25, 2017 at 10:47 AM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 25-07-17 om 10:01 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! >> >> 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 | 64 ++++++++++++++++--------------------- >> 1 file changed, 28 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c >> index 14e8a7738b06..efa525442e7d 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -498,39 +498,30 @@ 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 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. >> + */ >> + 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; >> + else if (property == priv->zorder_prop) >> + plane_state->zpos = val; >> + else >> + return -EINVAL; >> >> - return -EINVAL; >> + return 0; >> } >> >> static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, >> @@ -538,16 +529,17 @@ 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; >> + >> + /* >> + * Remap to the plane rotation/zorder property. We can peek at the plane >> + * state directly since holding the crtc locks gives you a read-lock on >> + * the plane state. >> + */ >> + if (property == crtc->primary->rotation_property) >> + return crtc->primary->state->rotation; >> + else if (property == priv->zorder_prop) >> + return crtc->primary->state->zpos; >> >> return -EINVAL; >> } > > acquire_ctx for crtc's getprop too then? > > The comment about read lock is only valid when the plane is bound to the crtc. In general this is not always the case. > You can only peak at plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane)) is true. > > I think we might need -EDEADLK handling for getprop then, even though it's not optimal. :( Well both the old an new way only worked because we grab all the locks unconditionally. And I'd really want to avoid get_prop being anything but a simple lookup. Unfortuantely that breaks omapdrm, so no idea what exactly to do here. In a way adding properties without standardizing them across drivers first was a really bad idea, because then we have disjoint sets of uapi expectations, and there's just no way to make that work. I guess one radical approach might be to make this the "standard", and just redirect rotation from the CRTC to the primary plane. Or omapdrm needs to duplicate the property properly, and update one if the other is set. I think that's probably the most workable approach. -Daniel
Hi Daniel, On Tuesday 25 Jul 2017 11:24:28 Daniel Vetter wrote: > On Tue, Jul 25, 2017 at 10:47 AM, Maarten Lankhorst wrote: > > Op 25-07-17 om 10:01 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! > >> > >> 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 | 64 ++++++++++--------------------- > >> 1 file changed, 28 insertions(+), 36 deletions(-) [snip] > > The comment about read lock is only valid when the plane is bound to the > > crtc. In general this is not always the case. You can only peak at > > plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane)) > > is true. > > > > I think we might need -EDEADLK handling for getprop then, even though it's > > not optimal. :( > > Well both the old an new way only worked because we grab all the locks > unconditionally. And I'd really want to avoid get_prop being anything > but a simple lookup. Unfortuantely that breaks omapdrm, so no idea > what exactly to do here. > > In a way adding properties without standardizing them across drivers > first was a really bad idea, because then we have disjoint sets of > uapi expectations, and there's just no way to make that work. > > I guess one radical approach might be to make this the "standard", and > just redirect rotation from the CRTC to the primary plane. > > Or omapdrm needs to duplicate the property properly, and update one if > the other is set. I think that's probably the most workable approach. Maybe the first question we should answer is whether this hack is still needed in the omapdrm driver. Tomi, do you know whether userspace still needs this ?
On 31/07/17 14:48, Laurent Pinchart wrote: >>> The comment about read lock is only valid when the plane is bound to the >>> crtc. In general this is not always the case. You can only peak at >>> plane->state when crtc->state->plane_mask & BIT(drm_plane_index(plane)) >>> is true. >>> >>> I think we might need -EDEADLK handling for getprop then, even though it's >>> not optimal. :( >> >> Well both the old an new way only worked because we grab all the locks >> unconditionally. And I'd really want to avoid get_prop being anything >> but a simple lookup. Unfortuantely that breaks omapdrm, so no idea >> what exactly to do here. >> >> In a way adding properties without standardizing them across drivers >> first was a really bad idea, because then we have disjoint sets of >> uapi expectations, and there's just no way to make that work. >> >> I guess one radical approach might be to make this the "standard", and >> just redirect rotation from the CRTC to the primary plane. >> >> Or omapdrm needs to duplicate the property properly, and update one if >> the other is set. I think that's probably the most workable approach. > > Maybe the first question we should answer is whether this hack is still needed > in the omapdrm driver. Tomi, do you know whether userspace still needs this ? The omap X driver uses legacy modesetting and the rotation property for the crtc. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..efa525442e7d 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -498,39 +498,30 @@ 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 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. + */ + 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; + else if (property == priv->zorder_prop) + plane_state->zpos = val; + else + return -EINVAL; - return -EINVAL; + return 0; } static int omap_crtc_atomic_get_property(struct drm_crtc *crtc, @@ -538,16 +529,17 @@ 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; + + /* + * Remap to the plane rotation/zorder property. We can peek at the plane + * state directly since holding the crtc locks gives you a read-lock on + * the plane state. + */ + if (property == crtc->primary->rotation_property) + return crtc->primary->state->rotation; + else if (property == priv->zorder_prop) + return crtc->primary->state->zpos; return -EINVAL; }
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! 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 | 64 ++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 36 deletions(-)