[PATCH v2 2/4] drm/atomic: Add accessor macros for the current state.

[Maarten Lankhorst]

Op 17-11-16 om 13:26 schreef Ville Syrjälä:
> On Thu, Nov 17, 2016 at 12:58:00PM +0100, Maarten Lankhorst wrote:
>> Op 16-11-16 om 17:32 schreef Ville Syrjälä:
>>> On Wed, Nov 16, 2016 at 05:11:56PM +0100, Maarten Lankhorst wrote:
>>>> 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 at linux.intel.com>
>>>>>>> ---
>>>>>>>  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.
>>> What is this so called __ function exactly?
>> __drm_atomic_get_current_plane_state, which is only used by drm_atomic_crtc_state_for_each_plane_state.
>>
>> It iterates over crtc_state->plane_mask and then gets new_plane_state if available, or plane->state if the plane is not part of the state.
>> This is mostly used for watermark calculations.
> Sounds like we should kill that sucker and make things clearer by
> enforcing the "want to access foo->state? then grab foo->lock first"
> rule.
Except adding all planes to cursor updates would result in unintended behavior.
And there are already patches to use the sprite plane instead of the cursor plane for skylake, so it's not just theoretical. :)

Testcases:
flip-vs-cursor-busy-crc-legacy
flip-vs-cursor-busy-crc-atomic

~Maarten