| Message ID | 1484559464-27107-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maarten, Thank you for the patch. 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(-) > > 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 b26e3419027e..d153e8a72921 > 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. Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ? 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 ? > + * 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 f523256ef77c..74da1c380b32 > 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3488,7 +3488,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; > @@ -3512,7 +3513,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; > @@ -3606,7 +3607,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); > } > @@ -3626,7 +3627,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); > > @@ -11327,7 +11328,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 9afcd3810785..2c4549e98c16 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);
Op 17-01-17 om 00:11 schreef Laurent Pinchart: > Hi Maarten, > > Thank you for the patch. > > 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(-) >> >> 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 b26e3419027e..d153e8a72921 >> 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. > 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. > 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); >> + >> +/** >> * 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 f523256ef77c..74da1c380b32 >> 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3488,7 +3488,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; >> @@ -3512,7 +3513,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; >> @@ -3606,7 +3607,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); >> } >> @@ -3626,7 +3627,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); >> @@ -11327,7 +11328,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 9afcd3810785..2c4549e98c16 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);
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]
On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote: > 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: > > >> @@ -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 ? You can't fix it in suspend? The issue is that on resume, we have reset states (to reflect the hw state of everything being off), so we can only do this on commit. That holds in general for the duplicate, do something nasty, restore state pattern. And then I think a dedicated helper to commit duplicated state makes sense. The kernel-doc might need a bit of work to explain this better though. I think it should explain what exactly is partially invalid with duplicated states. -Daniel
Hi Daniel, On Monday 23 Jan 2017 09:48:54 Daniel Vetter wrote: > On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote: > > 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: > >>>> @@ -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 ? > > You can't fix it in suspend? The issue is that on resume, we have reset > states (to reflect the hw state of everything being off), so we can only > do this on commit. That holds in general for the duplicate, do something > nasty, restore state pattern. Ok, I got it now. You can't fix the state up at suspend time because you need to set the old_state pointers to the state allocated by the .reset() handlers, which are called at resume time. > And then I think a dedicated helper to commit duplicated state makes > sense. The kernel-doc might need a bit of work to explain this better > though. I think it should explain what exactly is partially invalid with > duplicated states. Yes, the documentation should be clarified, and include the above explanation in some form.
Hi Maarten, Thank you for the patch. 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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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(-)
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 b26e3419027e..d153e8a72921 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 f523256ef77c..74da1c380b32 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3488,7 +3488,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; @@ -3512,7 +3513,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; @@ -3606,7 +3607,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); } @@ -3626,7 +3627,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); @@ -11327,7 +11328,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 9afcd3810785..2c4549e98c16 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> --- 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(-)