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

[Maarten Lankhorst]

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.
>> 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