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

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

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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);
 		drm_modeset_backoff(&ctx);
 		goto retry;
 	}
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

[Patchwork]

== Series Details ==

Series: drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb
URL   : https://patchwork.freedesktop.org/series/26548/
State : failure

== Summary ==

Series 26548v1 drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb
https://patchwork.freedesktop.org/api/1.0/series/26548/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-pnv-d510) fdo#101597
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> INCOMPLETE (fi-kbl-7500u)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:355s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:540s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:506s
fi-byt-j1900     total:279  pass:253  dwarn:2   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:279  pass:249  dwarn:2   dfail:0   fail:0   skip:28  time:480s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:592s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:436s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:426s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
fi-kbl-7500u     total:255  pass:237  dwarn:0   dfail:0   fail:0   skip:17 
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:570s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:583s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:557s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:458s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:339s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:461s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:405s

85a692e2c6a7cf93082044d776e838cb9e9b2146 drm-tip: 2017y-06m-28d-14h-24m-59s UTC integration manifest
4cb4f62 drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5067/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

[Ville Syrjälä]

On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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?

>  		drm_modeset_backoff(&ctx);
>  		goto retry;
>  	}
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

[Ville Syrjälä]

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

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

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

Re: [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@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Add missing drm_atomic_state_clear to atomic_remove_fb

[Daniel Vetter]

On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

With a real commit message (just explain why it's needed):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Also needs

Fixes: db8f6403e88a ("drm: Convert drm_framebuffer_remove to atomic, v4.")
Cc: stable@vger.kernel.org # v4.12-rc1+

Please push to drm-misc-next-fixes so Sean can send a pull for it for
4.13.

Thanks, Daniel

> ---
>  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);
>  		drm_modeset_backoff(&ctx);
>  		goto retry;
>  	}
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 30-06-17 om 14:46 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 01:59:54PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> With a real commit message (just explain why it's needed):
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Also needs
>
> Fixes: db8f6403e88a ("drm: Convert drm_framebuffer_remove to atomic, v4.")
> Cc: stable@vger.kernel.org # v4.12-rc1+
>
> Please push to drm-misc-next-fixes so Sean can send a pull for it for
> 4.13.
Pushed, thanks. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel