Re: [PATCH v3 1/7] drm/atomic: Add new iterators over all state, v3.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maarten,

On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
> Op 17-01-17 om 00:11 schreef Laurent Pinchart:
> > On Monday 16 Jan 2017 10:37:38 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.
> >> 
> >> This requires extra fixups done when committing a state after
> >> duplicating, which in general isn't valid but is used by suspend/resume.
> >> To handle these, introduce drm_atomic_helper_commit_duplicated_state
> >> which performs those fixups before checking & committing the state.
> >> 
> >> Changes since v1:
> >> - Remove nonblock parameter for commit_duplicated_state.
> >> Changes since v2:
> >> - Use commit_duplicated_state for i915 load detection.
> >> - Add WARN_ON(old_state != obj->state) before swapping.
> >> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic.c         |  6 +++
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 65 +++++++++++++++++++++++++----
> >>  drivers/gpu/drm/i915/intel_display.c | 13 +++---
> >>  include/drm/drm_atomic.h             | 81 ++++++++++++++++++++++++++++--
> >>  include/drm/drm_atomic_helper.h      |  2 +
> >>  5 files changed, 149 insertions(+), 18 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921
> >> 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c

[snip]

> >> @@ -2512,6 +2518,47 @@ struct drm_atomic_state
> >> *drm_atomic_helper_suspend(struct drm_device *dev)
> >> 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.
> >> + *
> >> + * 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.
> > 
> > Can't you fix the state in drm_atomic_helper_suspend() to avoid the need
> > for this function ?
> 
> We would have to fix up other areas that duplicate state too, like i915
> suspend and load detect. This means moving it to a helper.
> 
> It was my original approach, but Daniel preferred this approach.

We have to fix it somewhere, that's for sure. I think it's better to fix it as 
close as possible to state duplication, instead of carrying a state that we 
know is invalid and fix it at the last minute. The drawback of this approach 
is that the window during which the state will be invalid is much larger, 
which could cause bugs later when new code will try to touch that state.

Daniel, is there a specific reason why you prefer this approach ?

> > Apart from this the patch looks good to me. I don't like the fact that we
> > now have two sets of states though (state and old_state/new_state). I
> > understand that you'd like to address this as part of a separate patch
> > series, which I'm fine with to avoid delaying this one, but I think we
> > should address this sooner rather than latter, or the API will become
> > very confusing. Yes, that means mass-patching drivers I'm afraid. Could
> > you confirm that this is your plan ?
> 
> Yes, working on it.
> 
> >> + * Returns:
> >> + * 0 on success or a negative error code on failure.
> >> + *
> >> + * See also:
> >> + * drm_atomic_helper_suspend()
> >> + */
> >> +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
> >> *state,
> >> +					      struct drm_modeset_acquire_ctx
> >> *ctx)
> >> +{
> >> +	int i;
> >> +	struct drm_plane *plane;
> >> +	struct drm_plane_state *plane_state;
> >> +	struct drm_connector *connector;
> >> +	struct drm_connector_state *conn_state;
> >> +	struct drm_crtc *crtc;
> >> +	struct drm_crtc_state *crtc_state;
> >> +
> >> +	state->acquire_ctx = ctx;
> >> +
> >> +	for_each_new_plane_in_state(state, plane, plane_state, i)
> >> +		state->planes[i].old_state = plane->state;
> >> +
> >> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> >> +		state->crtcs[i].old_state = crtc->state;
> >> +
> >> +	for_each_new_connector_in_state(state, connector, conn_state, i)
> >> +		state->connectors[i].old_state = connector->state;
> >> +
> >> +	return drm_atomic_commit(state);
> >> +}
> >> +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx