Re: [PATCH 01/19] drm/atomic: Add new iterators over all state

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 01/19] drm/atomic: Add new iterators over all state
Date: Tue, 1 Nov 2016 15:41:01 +0200	[thread overview]
Message-ID: <20161101134101.GX4617@intel.com> (raw)
In-Reply-To: <9d2955ef-a45e-6a04-958a-6b490bc08c65@linux.intel.com>

On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote:
> Op 01-11-16 om 14:09 schreef Ville Syrjälä:
> > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote:
> >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to
> >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip
> >> depth and getting rid of all xxx->state dereferences.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c         |  6 +++
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 52 +++++++++++++++++++++--
> >>  drivers/gpu/drm/i915/intel_display.c | 11 ++---
> >>  include/drm/drm_atomic.h             | 81 ++++++++++++++++++++++++++++++++++--
> >>  include/drm/drm_atomic_helper.h      |  3 ++
> >>  5 files changed, 142 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 5dd70540219c..120775fcfb70 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
> >>  		return ERR_PTR(-ENOMEM);
> >>  
> >>  	state->crtcs[index].state = crtc_state;
> >> +	state->crtcs[index].old_state = crtc->state;
> >> +	state->crtcs[index].new_state = crtc_state;
> >>  	state->crtcs[index].ptr = crtc;
> >>  	crtc_state->state = state;
> >>  
> >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
> >>  
> >>  	state->planes[index].state = plane_state;
> >>  	state->planes[index].ptr = plane;
> >> +	state->planes[index].old_state = plane->state;
> >> +	state->planes[index].new_state = plane_state;
> >>  	plane_state->state = state;
> >>  
> >>  	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
> >>  
> >>  	drm_connector_reference(connector);
> >>  	state->connectors[index].state = connector_state;
> >> +	state->connectors[index].old_state = connector->state;
> >> +	state->connectors[index].new_state = connector_state;
> >>  	state->connectors[index].ptr = connector;
> >>  	connector_state->state = state;
> >>  
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 07b432f43b98..fcb6e5b55217 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
> >>   *
> >>   * See also:
> >>   * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
> >> - * drm_atomic_helper_resume()
> >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
> >>   */
> >>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
> >>  {
> >> @@ -2536,6 +2536,52 @@ unlock:
> >>  EXPORT_SYMBOL(drm_atomic_helper_suspend);
> >>  
> >>  /**
> >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state
> >> + * @state: duplicated atomic state to commit
> >> + * @ctx: pointer to acquire_ctx to use for commit.
> >> + * @nonblock: commit the state non-blocking.
> >> + *
> >> + * The state returned by drm_atomic_helper_duplicate_state() and
> >> + * drm_atomic_helper_suspend() is partially invalid, and needs to
> >> + * be fixed up before commit.
> > So the old_state pointers are potentially invalid because whatever->state
> > may have changed since we duplicated the state?
> 
> Yes when you use drm_atomic_helper_duplicate_state, and commit a different state before committing the duplicated state.
> This only happens during suspend/resume and gpu reset.

Should we maybe have something like drm_atomic_refresh_old_state(state)?
Would allow the caller then to check something in the old vs. new state
before committing?

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