On Wed, Nov 02, 2016 at 09:28:46AM +0100, Maarten Lankhorst wrote:
> Op 01-11-16 om 14:41 schreef Ville Syrjälä:
> > 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 at 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?
>
> iirc that was the first approach I took, but then you shouldn't do anything
> with a duplicated state after
> creating a except commit it, so creating a commit_duplicated_state should be
> enough.
>
> Can you think of any case where you do the following?
>
> Duplicate state
> commit different state
> Look at duplicated state, tinker with it, commit it.
Not really. Oh, and we do still run the thing through the check phase
when we commit it, don't we? That sounds like it should let the driver
do whatever it needs to do.
So my only other concern is just the 'bool nonblock' parameter. It's a
bit funny looking that we pass that in, then branch off to the nonblocking
vs. blocking commits, which then just pass another bool parameter to the
commit hook. Should we have block vs. nonblocking variants of this
function as well or just remove drm_atomic_nonblocking_commit() and have
callers pass the parameter straight from the beginning?
drm_atomic_commit() and drm_atomic_nonblocking_commit() could also use
some code deduplication I think. Should we decice to keep them around.
--
Ville Syrjälä
Intel OTC
