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@xxxxxxxxxxxxxxx>
> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
> ---
> 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,
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
