[Intel-gfx] [PATCH] drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

[Daniel Vetter]

On Thu, Jun 29, 2017 at 05:17:39PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 29, 2017 at 04:57:25PM +0300, Ville Syrjälä wrote:
> > On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote:
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_framebuffer.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > index fc8ef42203ec..b3ef4f1c2630 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -832,6 +832,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb)
> > >  		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> > >  
> > >  	if (ret == -EDEADLK) {
> > > +		drm_atomic_state_clear(state);
> > 
> > Hmm. We seem to be missing this all over. Do those other places need it
> > as well? Hard to say without a commit message explaining why we need it
> > here.
> > 
> > Should we just back it into drm_modeset_backoff() if it's always needed?
> 
> s/back/bake/

It's not needed everywhere, and that's because you can do the modeset_lock
dance without necessarily doing an atomic transaction (e.g. hw state
readout on boot or load detect). Or the atomic transaction is happening
somewhere else from where the ctx backoff is handled (e.g. for legacy
paths the core code handles the w/w dance since my recent rework, whereas
compat helpers handle the retry for the atomic side).

But yeah if you do an atomic commit, you must have the state_clear in the
EDEADLK path somewhere. Maybe we could do a trick with lockdep annotations
(make the state another ww mutex that nests within the modeset_lock class,
or something like that) to ensure that no one ever forgets to clear this
up? But that's a bit tricky, since on successful commit we hand the state
over to the driver and must _not_ clear it, this is only for the backoff
case (even on any other errors it's not needed, since we just kfree
everything).
-Daniel

> 
> > 
> > >  		drm_modeset_backoff(&ctx);
> > >  		goto retry;
> > >  	}
> > > -- 
> > > 2.11.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch