| Message ID | e6a52e0d-0ecd-5798-3453-0ca77610fa86@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maarten, Thank you for the patch. On Tuesday 10 Jan 2017 14:46:58 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> > --- > Forgot that there was a v2. It still broke on igt load detect tests, this > should be the fix. > > I also added a big WARN_ON before swapping to ensure old state is correct. > At this point we can no longer fix it because atomic_check is called with > the wrong items, but it will help limit the damage. This patch now conflicts with "[PATCH v2 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.". Could you fix that and repost the whole series ? > 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(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 6414bcf7f41b..1c1cbf436717 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -275,6 +275,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; > > @@ -691,6 +693,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", > @@ -1031,6 +1035,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 7b71ac48b8a4..b652ef1a25b1 > 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct > drm_atomic_state *state, int i; > long ret; > struct drm_connector *connector; > - struct drm_connector_state *conn_state; > + struct drm_connector_state *conn_state, *old_conn_state; > struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > + struct drm_crtc_state *crtc_state, *old_crtc_state; > struct drm_plane *plane; > - struct drm_plane_state *plane_state; > + struct drm_plane_state *plane_state, *old_plane_state; > struct drm_crtc_commit *commit; > > if (stall) { > @@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct > drm_atomic_state *state, } > } > > - for_each_connector_in_state(state, connector, conn_state, i) { > + for_each_oldnew_connector_in_state(state, connector, old_conn_state, > conn_state, i) { + WARN_ON(connector->state != old_conn_state); > + > connector->state->state = state; > swap(state->connectors[i].state, connector->state); > connector->state->state = NULL; > } > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) > { + WARN_ON(crtc->state != old_crtc_state); > + > crtc->state->state = state; > swap(state->crtcs[i].state, crtc->state); > crtc->state->state = NULL; > @@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct > drm_atomic_state *state, } > } > > - for_each_plane_in_state(state, plane, plane_state, i) { > + for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, > i) { + WARN_ON(plane->state != old_plane_state); > + > plane->state->state = state; > swap(state->planes[i].state, plane->state); > plane->state->state = NULL; > @@ -2471,7 +2477,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) > { > @@ -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. > + * > + * 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); > + > +/** > * drm_atomic_helper_resume - subsystem-level resume helper > * @dev: DRM device > * @state: atomic state to resume to > @@ -2534,9 +2581,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); 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 e2150a64860c..5364724df57f > 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3489,7 +3489,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; > @@ -3513,7 +3514,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); > > WARN_ON(ret == -EDEADLK); > return ret; > @@ -3607,7 +3608,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); > } > @@ -3627,7 +3628,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); > > @@ -11328,7 +11329,7 @@ void intel_release_load_detect_pipe(struct > drm_connector *connector, if (!state) > return; > > - ret = drm_atomic_commit(state); > + ret = drm_atomic_helper_commit_duplicated_state(state, ctx); > if (ret) > DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); > drm_atomic_state_put(state); > @@ -17210,7 +17211,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 f96220ed4004..6062e7f27325 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -137,12 +137,12 @@ 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; > s64 __user *out_fence_ptr; > unsigned last_vblank_count; > @@ -150,7 +150,7 @@ struct __drm_crtcs_state { > > struct __drm_connnectors_state { > struct drm_connector *ptr; > - struct drm_connector_state *state; > + struct drm_connector_state *state, *old_state, *new_state; > }; > > /** > @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct > drm_printer *p); (__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 && \ > @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct > drm_printer *p); (__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 && \ > @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct > drm_printer *p); (__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 4b2353dc34ba..e43d4de2ff8f 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -105,6 +105,8 @@ 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); > int drm_atomic_helper_resume(struct drm_device *dev, > struct drm_atomic_state *state); > > -- 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 15-01-17 om 20:56 schreef Laurent Pinchart: > Hi Maarten, > > Thank you for the patch. > > On Tuesday 10 Jan 2017 14:46:58 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> >> --- >> Forgot that there was a v2. It still broke on igt load detect tests, this >> should be the fix. >> >> I also added a big WARN_ON before swapping to ensure old state is correct. >> At this point we can no longer fix it because atomic_check is called with >> the wrong items, but it will help limit the damage. > This patch now conflicts with "[PATCH v2 4/7] drm/atomic: Fix atomic helpers > to use the new iterator macros.". Could you fix that and repost the whole > series ? Yeah I noticed. I wanted to resend because patch 7 had a small bug too, but trybot was failing on some tests. After doing some bisection by sending patches it turns out the error went away by itself. The failing test also didn't seem related to the series. With that issue gone, I'll resend. ~Maarten
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6414bcf7f41b..1c1cbf436717 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -275,6 +275,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; @@ -691,6 +693,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", @@ -1031,6 +1035,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 7b71ac48b8a4..b652ef1a25b1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, int i; long ret; struct drm_connector *connector; - struct drm_connector_state *conn_state; + struct drm_connector_state *conn_state, *old_conn_state; struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *crtc_state, *old_crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *plane_state, *old_plane_state; struct drm_crtc_commit *commit; if (stall) { @@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } } - for_each_connector_in_state(state, connector, conn_state, i) { + for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) { + WARN_ON(connector->state != old_conn_state); + connector->state->state = state; swap(state->connectors[i].state, connector->state); connector->state->state = NULL; } - for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { + WARN_ON(crtc->state != old_crtc_state); + crtc->state->state = state; swap(state->crtcs[i].state, crtc->state); crtc->state->state = NULL; @@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } } - for_each_plane_in_state(state, plane, plane_state, i) { + for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) { + WARN_ON(plane->state != old_plane_state); + plane->state->state = state; swap(state->planes[i].state, plane->state); plane->state->state = NULL; @@ -2471,7 +2477,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) { @@ -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. + * + * 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); + +/** * drm_atomic_helper_resume - subsystem-level resume helper * @dev: DRM device * @state: atomic state to resume to @@ -2534,9 +2581,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); 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 e2150a64860c..5364724df57f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3489,7 +3489,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; @@ -3513,7 +3514,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); WARN_ON(ret == -EDEADLK); return ret; @@ -3607,7 +3608,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); } @@ -3627,7 +3628,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); @@ -11328,7 +11329,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (!state) return; - ret = drm_atomic_commit(state); + ret = drm_atomic_helper_commit_duplicated_state(state, ctx); if (ret) DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); drm_atomic_state_put(state); @@ -17210,7 +17211,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 f96220ed4004..6062e7f27325 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,12 +137,12 @@ 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; s64 __user *out_fence_ptr; unsigned last_vblank_count; @@ -150,7 +150,7 @@ struct __drm_crtcs_state { struct __drm_connnectors_state { struct drm_connector *ptr; - struct drm_connector_state *state; + struct drm_connector_state *state, *old_state, *new_state; }; /** @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__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 && \ @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__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 && \ @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__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 4b2353dc34ba..e43d4de2ff8f 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -105,6 +105,8 @@ 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); 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. 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> --- Forgot that there was a v2. It still broke on igt load detect tests, this should be the fix. I also added a big WARN_ON before swapping to ensure old state is correct. At this point we can no longer fix it because atomic_check is called with the wrong items, but it will help limit the damage. 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(-) -- 2.7.4