| Message ID | 1476707838-25253-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > + * > + * 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, > + bool nonblock) > +{ > + 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; > + > + if (nonblock) > + return drm_atomic_nonblocking_commit(state); > + else > + return drm_atomic_commit(state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); > + > +/** > * drm_atomic_helper_resume - subsystem-level resume helper > * @dev: DRM device > * @state: atomic state to resume to > @@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, > int err; > > drm_mode_config_reset(dev); > + > drm_modeset_lock_all(dev); > - state->acquire_ctx = config->acquire_ctx; > - err = drm_atomic_commit(state); > + err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx, false); > drm_modeset_unlock_all(dev); > > return err; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a12e093c54cf..3bd3f6a93c12 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3497,7 +3497,8 @@ static void intel_update_primary_planes(struct drm_device *dev) > > static int > __intel_display_resume(struct drm_device *dev, > - struct drm_atomic_state *state) > + struct drm_atomic_state *state, > + struct drm_modeset_acquire_ctx *ctx) > { > struct drm_crtc_state *crtc_state; > struct drm_crtc *crtc; > @@ -3521,7 +3522,7 @@ __intel_display_resume(struct drm_device *dev, > /* ignore any reset values/BIOS leftovers in the WM registers */ > to_intel_atomic_state(state)->skip_intermediate_wm = true; > > - ret = drm_atomic_commit(state); > + ret = drm_atomic_helper_commit_duplicated_state(state, ctx, false); > > WARN_ON(ret == -EDEADLK); > return ret; > @@ -3617,7 +3618,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) > */ > intel_update_primary_planes(dev); > } else { > - ret = __intel_display_resume(dev, state); > + ret = __intel_display_resume(dev, state, ctx); > if (ret) > DRM_ERROR("Restoring old state failed with %i\n", ret); > } > @@ -3636,7 +3637,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) > dev_priv->display.hpd_irq_setup(dev_priv); > spin_unlock_irq(&dev_priv->irq_lock); > > - ret = __intel_display_resume(dev, state); > + ret = __intel_display_resume(dev, state, ctx); > if (ret) > DRM_ERROR("Restoring old state failed with %i\n", ret); > > @@ -16937,7 +16938,7 @@ void intel_display_resume(struct drm_device *dev) > } > > if (!ret) > - ret = __intel_display_resume(dev, state); > + ret = __intel_display_resume(dev, state, &ctx); > > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index fc8af53b18aa..09fb6b316ca9 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -137,18 +137,18 @@ struct drm_crtc_commit { > > struct __drm_planes_state { > struct drm_plane *ptr; > - struct drm_plane_state *state; > + struct drm_plane_state *state, *old_state, *new_state; > }; > > struct __drm_crtcs_state { > struct drm_crtc *ptr; > - struct drm_crtc_state *state; > + struct drm_crtc_state *state, *old_state, *new_state; > struct drm_crtc_commit *commit; > }; > > struct __drm_connnectors_state { > struct drm_connector *ptr; > - struct drm_connector_state *state; > + struct drm_connector_state *state, *old_state, *new_state; > }; > > /** > @@ -372,6 +372,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > (__i)++) \ > for_each_if (connector) > > +#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_connector && \ > + ((connector) = (__state)->connectors[__i].ptr, \ > + (old_connector_state) = (__state)->connectors[__i].old_state, \ > + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (connector) > + > +#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_connector && \ > + ((connector) = (__state)->connectors[__i].ptr, \ > + (old_connector_state) = (__state)->connectors[__i].old_state, 1); \ > + (__i)++) \ > + for_each_if (connector) > + > +#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->num_connector && \ > + ((connector) = (__state)->connectors[__i].ptr, \ > + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (connector) > + > #define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ > for ((__i) = 0; \ > (__i) < (__state)->dev->mode_config.num_crtc && \ > @@ -380,6 +405,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > (__i)++) \ > for_each_if (crtc_state) > > +#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_crtc && \ > + ((crtc) = (__state)->crtcs[__i].ptr, \ > + (old_crtc_state) = (__state)->crtcs[__i].old_state, \ > + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (crtc) > + > +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_crtc && \ > + ((crtc) = (__state)->crtcs[__i].ptr, \ > + (old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \ > + (__i)++) \ > + for_each_if (crtc) > + > +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_crtc && \ > + ((crtc) = (__state)->crtcs[__i].ptr, \ > + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (crtc) > + > #define for_each_plane_in_state(__state, plane, plane_state, __i) \ > for ((__i) = 0; \ > (__i) < (__state)->dev->mode_config.num_total_plane && \ > @@ -388,6 +438,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); > (__i)++) \ > for_each_if (plane_state) > > +#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_total_plane && \ > + ((plane) = (__state)->planes[__i].ptr, \ > + (old_plane_state) = (__state)->planes[__i].old_state, \ > + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (plane) > + > +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_total_plane && \ > + ((plane) = (__state)->planes[__i].ptr, \ > + (old_plane_state) = (__state)->planes[__i].old_state, 1); \ > + (__i)++) \ > + for_each_if (plane) > + > +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ > + for ((__i) = 0; \ > + (__i) < (__state)->dev->mode_config.num_total_plane && \ > + ((plane) = (__state)->planes[__i].ptr, \ > + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ > + (__i)++) \ > + for_each_if (plane) > + > /** > * drm_atomic_crtc_needs_modeset - compute combined modeset need > * @state: &drm_crtc_state for the CRTC > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 7ff92b09fd9c..613bb731ba36 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -108,6 +108,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, > int drm_atomic_helper_disable_all(struct drm_device *dev, > struct drm_modeset_acquire_ctx *ctx); > struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); > +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > + struct drm_modeset_acquire_ctx *ctx, > + bool nonblock); > int drm_atomic_helper_resume(struct drm_device *dev, > struct drm_atomic_state *state); > > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. ~Maarten
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?
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@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. ~Maarten
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@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.
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. Kerneldoc should imo mention that suspend/resume is the only case this is valid, and even then it depends upon the driver. Proper atomic commits never keep drm_atomic_state around when dropping locks, so this can't happen with an atomic ioctl. Please also directly cross-reference all these functions. -Daniel > > ~Maarten > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-11-16 om 16:11 schreef Ville Syrjälä: > 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@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. I could have done the check and commit manually, but would rather use the dedicated atomic commands for it. However nowhere we end up using the nonblocking version, so I think I will remove the nonblock parameter and leave the decision on what to do with nonblocking commit for another day. :) ~Maarten
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. + * + * 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, + bool nonblock) +{ + 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; + + if (nonblock) + return drm_atomic_nonblocking_commit(state); + else + return drm_atomic_commit(state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); + +/** * drm_atomic_helper_resume - subsystem-level resume helper * @dev: DRM device * @state: atomic state to resume to @@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err; drm_mode_config_reset(dev); + drm_modeset_lock_all(dev); - state->acquire_ctx = config->acquire_ctx; - err = drm_atomic_commit(state); + err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx, false); drm_modeset_unlock_all(dev); return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a12e093c54cf..3bd3f6a93c12 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3497,7 +3497,8 @@ static void intel_update_primary_planes(struct drm_device *dev) static int __intel_display_resume(struct drm_device *dev, - struct drm_atomic_state *state) + struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx) { struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -3521,7 +3522,7 @@ __intel_display_resume(struct drm_device *dev, /* ignore any reset values/BIOS leftovers in the WM registers */ to_intel_atomic_state(state)->skip_intermediate_wm = true; - ret = drm_atomic_commit(state); + ret = drm_atomic_helper_commit_duplicated_state(state, ctx, false); WARN_ON(ret == -EDEADLK); return ret; @@ -3617,7 +3618,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else { - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret); } @@ -3636,7 +3637,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock); - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret); @@ -16937,7 +16938,7 @@ void intel_display_resume(struct drm_device *dev) } if (!ret) - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, &ctx); drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index fc8af53b18aa..09fb6b316ca9 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,18 +137,18 @@ struct drm_crtc_commit { struct __drm_planes_state { struct drm_plane *ptr; - struct drm_plane_state *state; + struct drm_plane_state *state, *old_state, *new_state; }; struct __drm_crtcs_state { struct drm_crtc *ptr; - struct drm_crtc_state *state; + struct drm_crtc_state *state, *old_state, *new_state; struct drm_crtc_commit *commit; }; struct __drm_connnectors_state { struct drm_connector *ptr; - struct drm_connector_state *state; + struct drm_connector_state *state, *old_state, *new_state; }; /** @@ -372,6 +372,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); (__i)++) \ for_each_if (connector) +#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + #define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc && \ @@ -380,6 +405,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); (__i)++) \ for_each_if (crtc_state) +#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + #define for_each_plane_in_state(__state, plane, plane_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_total_plane && \ @@ -388,6 +438,31 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); (__i)++) \ for_each_if (plane_state) +#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + /** * drm_atomic_crtc_needs_modeset - compute combined modeset need * @state: &drm_crtc_state for the CRTC diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 7ff92b09fd9c..613bb731ba36 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -108,6 +108,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx, + bool nonblock); int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state);
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(-)