Re: [PATCH 02/23] drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Mar 26, 2018 at 10:28:06PM +0200, Daniel Vetter wrote:
> On Thu, Mar 22, 2018 at 05:22:52PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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@free.fr
> > Cc: chris@chris-wilson.co.uk
> > Cc: Dave Airlie <airlied@gmail.com> (v1)
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@ffwll.ch>

Squashed, and commit message rescribbeled. And with sufficient confidence
from a local smoke test I proceeded to push the easy ones 1-13 (except msm), 
and 22-23 (the load detect stuff for i915). I'll have to figure out the
correct merge order for the rest next week.

Thanks for the reviews.

> 
> 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx