On Thu, Mar 22, 2018 at 05:22:52PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> drm_atomic_helper_shutdown() needs to release the reference held by
> plane->fb, so we want to use drm_atomic_clean_old_fb() in
> drm_atomic_helper_disable_all(). However during suspend/resume, gpu
> reset and load detection we should probably leave that stuff alone,
> as otherwise we'd have to make sure we put them back again when
> we restore the duplicated state to the device. Seems simpler to me
> to not touch any of it anyway.
>
> v2: Don't inflict the clean_old_fbs bool to drivers (Daniel)
>
> Cc: martin.peres@xxxxxxx
> Cc: chris@xxxxxxxxxxxxxxxxxx
> Cc: Dave Airlie <airlied@xxxxxxxxx> (v1)
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
I think this would be cleaner diff to read if you squash the first 2
patches together. Also avoids the bisect fail. With that (and I trust you
to come up with a suitably merged commit message):
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
I reviewed this by re-reading the analysis from 49d70aeaeca8f62b72b77 and
trusting my former self :-)
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 67 ++++++++++++++++++++++---------------
> 1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c48f187d08de..39a69508d8c9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2881,31 +2881,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> return 0;
> }
>
> -/**
> - * drm_atomic_helper_disable_all - disable all currently active outputs
> - * @dev: DRM device
> - * @ctx: lock acquisition context
> - *
> - * Loops through all connectors, finding those that aren't turned off and then
> - * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> - * that they are connected to.
> - *
> - * This is used for example in suspend/resume to disable all currently active
> - * functions when suspending. If you just want to shut down everything at e.g.
> - * driver unload, look at drm_atomic_helper_shutdown().
> - *
> - * Note that if callers haven't already acquired all modeset locks this might
> - * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - *
> - * See also:
> - * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> - * drm_atomic_helper_shutdown().
> - */
> -int drm_atomic_helper_disable_all(struct drm_device *dev,
> - struct drm_modeset_acquire_ctx *ctx)
> +static int __drm_atomic_helper_disable_all(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx,
> + bool clean_old_fbs)
> {
> struct drm_atomic_state *state;
> struct drm_connector_state *conn_state;
> @@ -2914,6 +2892,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
> struct drm_plane *plane;
> struct drm_crtc_state *crtc_state;
> struct drm_crtc *crtc;
> + unsigned int plane_mask = 0;
> int ret, i;
>
> state = drm_atomic_state_alloc(dev);
> @@ -2956,14 +2935,48 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
> goto free;
>
> drm_atomic_set_fb_for_plane(plane_state, NULL);
> +
> + if (clean_old_fbs) {
> + plane->old_fb = plane->fb;
> + plane_mask |= BIT(drm_plane_index(plane));
> + }
> }
>
> ret = drm_atomic_commit(state);
> free:
> + drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> drm_atomic_state_put(state);
> return ret;
> }
> -
> +/**
> + * drm_atomic_helper_disable_all - disable all currently active outputs
> + * @dev: DRM device
> + * @ctx: lock acquisition context
> + *
> + * Loops through all connectors, finding those that aren't turned off and then
> + * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> + * that they are connected to.
> + *
> + * This is used for example in suspend/resume to disable all currently active
> + * functions when suspending. If you just want to shut down everything at e.g.
> + * driver unload, look at drm_atomic_helper_shutdown().
> + *
> + * Note that if callers haven't already acquired all modeset locks this might
> + * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + *
> + * See also:
> + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> + * drm_atomic_helper_shutdown().
> + */
> +int drm_atomic_helper_disable_all(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx)
> +{
> + return __drm_atomic_helper_disable_all(dev, ctx, false);
> +}
> EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>
> /**
> @@ -2986,7 +2999,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
> while (1) {
> ret = drm_modeset_lock_all_ctx(dev, &ctx);
> if (!ret)
> - ret = drm_atomic_helper_disable_all(dev, &ctx);
> + ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
>
> if (ret != -EDEADLK)
> break;
> --
> 2.16.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
