| Message ID | 20170731105419.6505-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 12:54:19 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 s/propety/property/ > 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). > > 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 | 84 ++++++++++++++++++---------------- > drivers/gpu/drm/omapdrm/omap_drv.h | 9 ++++ > 2 files changed, 57 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..fc8b25748f10 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -498,39 +498,34 @@ 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. Nitpicking, you can reformat the comment to go towards the 80 columns limit. > + */ > + 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 +533,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); > + > + /* > + * 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. Isn't this comment outdated ? > + */ > + 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; > + > + if (WARN_ON(!crtc->state)) > + return NULL; > + > + state = kmalloc(sizeof(*state), GFP_KERNEL); > + if (state) > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); No need to copy the zpos and rotation fields ? > + 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 +571,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_drv.h > b/drivers/gpu/drm/omapdrm/omap_drv.h index 4bd1e9070b31..11703c174cd8 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.h > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h > @@ -115,6 +115,15 @@ static inline void omap_fbdev_free(struct drm_device > *dev) } > #endif > > +struct omap_crtc_state { > + /* must be first */ Capitalizing comments would be appreciated :-) > + struct drm_crtc_state base; > + /* shadow values for legacy userspace support */ > + unsigned int rotation; > + unsigned int zpos; > +}; > +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base) Could you please define this structure in omap_crtc.c, as it's internal to the CRTC handling code ? > struct videomode *omap_crtc_timings(struct drm_crtc *crtc); > enum omap_channel omap_crtc_channel(struct drm_crtc *crtc); > void omap_crtc_pre_init(void);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 14e8a7738b06..fc8b25748f10 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -498,39 +498,34 @@ 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. + */ + 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 +533,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); + + /* + * 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 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; + + if (WARN_ON(!crtc->state)) + return NULL; + + state = kmalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); + + 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 +571,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_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 4bd1e9070b31..11703c174cd8 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -115,6 +115,15 @@ static inline void omap_fbdev_free(struct drm_device *dev) } #endif +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_state(x) container_of(x, struct omap_crtc_state, base) + struct videomode *omap_crtc_timings(struct drm_crtc *crtc); enum omap_channel omap_crtc_channel(struct drm_crtc *crtc); void omap_crtc_pre_init(void);
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). 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 | 84 +++++++++++++++++++++---------------- drivers/gpu/drm/omapdrm/omap_drv.h | 9 ++++ 2 files changed, 57 insertions(+), 36 deletions(-)