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