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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 17, 2016 at 01:42:13PM +0100, Maarten Lankhorst wrote:
> 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@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.

That wasn't my suggestion.

> 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel