Op 16-11-16 om 16:04 schreef Daniel Vetter:
> On Wed, Nov 16, 2016 at 04:35:45PM +0200, Ville Syrjälä wrote:
>> On Wed, Nov 16, 2016 at 02:58:06PM +0100, Maarten Lankhorst wrote:
>>> With checks! This will allow safe access to the current state,
>>> while ensuring that the correct locks are held.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> ---
>>> include/drm/drm_atomic.h | 66 ++++++++++++++++++++++++++++++++++++++++++
>>> include/drm/drm_modeset_lock.h | 21 ++++++++++++++
>>> 2 files changed, 87 insertions(+)
>>>
>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>> index e527684dd394..462408a2d1b8 100644
>>> --- a/include/drm/drm_atomic.h
>>> +++ b/include/drm/drm_atomic.h
>>> @@ -334,6 +334,72 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
>>> return plane->state;
>>> }
>>>
>>> +
>>> +/**
>>> + * drm_atomic_get_current_plane_state - get current plane state
>>> + * @plane: plane to grab
>>> + *
>>> + * This function returns the current plane state for the given plane,
>>> + * with extra locking checks to make sure that the plane state can be
>>> + * retrieved safely.
>>> + *
>>> + * Returns:
>>> + *
>>> + * Pointer to the current plane state.
>>> + */
>>> +static inline struct drm_plane_state *
>>> +drm_atomic_get_current_plane_state(struct drm_plane *plane)
>>> +{
>>> + struct drm_plane_state *plane_state = plane->state;
>>> + struct drm_crtc *crtc = plane_state ? plane_state->crtc : NULL;
>>> +
>>> + if (crtc)
>>> + drm_modeset_lock_assert_one_held(&plane->mutex, &crtc->mutex);
>>> + else
>>> + drm_modeset_lock_assert_held(&plane->mutex);
>> Hmm. Daniel recently smashed me on the head with a big clue bat to point
>> out that accessing object->state isn't safe unless you hold the object lock.
>> So this thing seems suspicious. What's the use case for this?
>>
>> I guess in this case it might be safe since a parallel update would lock
>> the crtc as well. But it does feel like promoting potentially dangerous
>> behaviour. Also there are no barriers so I'm not sure this would be
>> guaranteed to give us the correct answer anyway.
>>
>> As far as all of these functions go, should they return const*? Presumably
>> you wouldn't want to allow changes to the current state.
> Yep, need at least a lockdep check for the plane->mutex. And imo also a
> check that we're indeed in atomic_check per the idea I dropped on the
> cover letter.
>
> And +1 on const * for these, that seems like a very important check.
Well I allowed for crtc lock held because the __ function uses crtc->mutex as safety lock.
I thought of const, but some code like i915_page_flip manipulates the current state with the right locks held.
Perhaps we should disallow that. :)
~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
