[Intel-gfx] [PATCH 1/8] drm/omap: Simplify the rotation-on-crtc hack

[Daniel Vetter]

On Tue, Jul 25, 2017 at 10:47 AM, Maarten Lankhorst
<maarten.lankhorst at 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 at ti.com
>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch