diff mbox series

[4/5] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

Message ID 20210602164149.391653-5-jason@jlekstrand.net (mailing list archive)
State New, archived
Headers show
Series drm/i915: Get rid of fence error propagation | expand

Commit Message

Jason Ekstrand June 2, 2021, 4:41 p.m. UTC 
This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
since that commit, we've been having issues where a hang in one client
can propagate to another.  In particular, a hang in an app can propagate
to the X server which causes the whole desktop to lock up.

Signed-off-by: Jason Ekstrand <jason.ekstrand@intel.com>
Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
Cc: <stable@vger.kernel.org> # v5.6+
Cc: Jason Ekstrand <jason.ekstrand@intel.com>
Cc: Marcin Slusarz <marcin.slusarz@intel.com>
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Daniel Vetter June 3, 2021, 8:24 a.m. UTC | #1 
On Wed, Jun 02, 2021 at 11:41:48AM -0500, Jason Ekstrand wrote:
> This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> since that commit, we've been having issues where a hang in one client
> can propagate to another.  In particular, a hang in an app can propagate
> to the X server which causes the whole desktop to lock up.

I think we need a note to backporters here:

"For backporters: Please note that you _must_ have a backport of
https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
for otherwise backporting just this patch opens up a security bug."
 
Or something like that.
-Daniel

> Signed-off-by: Jason Ekstrand <jason.ekstrand@intel.com>
> Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
> Cc: <stable@vger.kernel.org> # v5.6+
> Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> Cc: Marcin Slusarz <marcin.slusarz@intel.com>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_request.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 970d8f4986bbe..b796197c07722 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1426,10 +1426,8 @@ i915_request_await_execution(struct i915_request *rq,
>  
>  	do {
>  		fence = *child++;
> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  			continue;
> -		}
>  
>  		if (fence->context == rq->fence.context)
>  			continue;
> @@ -1527,10 +1525,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>  
>  	do {
>  		fence = *child++;
> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>  			continue;
> -		}
>  
>  		/*
>  		 * Requests on the same timeline are explicitly ordered, along
> -- 
> 2.31.1
>
Daniel Vetter June 3, 2021, 8:25 a.m. UTC | #2 
On Thu, Jun 03, 2021 at 10:24:21AM +0200, Daniel Vetter wrote:
> On Wed, Jun 02, 2021 at 11:41:48AM -0500, Jason Ekstrand wrote:
> > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> > since that commit, we've been having issues where a hang in one client
> > can propagate to another.  In particular, a hang in an app can propagate
> > to the X server which causes the whole desktop to lock up.
> 
> I think we need a note to backporters here:
> 
> "For backporters: Please note that you _must_ have a backport of
> https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
> for otherwise backporting just this patch opens up a security bug."
>  
> Or something like that.

Oh also reordering the patch set so the 2 reverts which are cc: stable are
first, then the other stuff on top that cleans up the fallout.
-Daniel

> -Daniel
> 
> > Signed-off-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.6+
> > Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> > Cc: Marcin Slusarz <marcin.slusarz@intel.com>
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> > Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 970d8f4986bbe..b796197c07722 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1426,10 +1426,8 @@ i915_request_await_execution(struct i915_request *rq,
> >  
> >  	do {
> >  		fence = *child++;
> > -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >  			continue;
> > -		}
> >  
> >  		if (fence->context == rq->fence.context)
> >  			continue;
> > @@ -1527,10 +1525,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> >  
> >  	do {
> >  		fence = *child++;
> > -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >  			continue;
> > -		}
> >  
> >  		/*
> >  		 * Requests on the same timeline are explicitly ordered, along
> > -- 
> > 2.31.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter June 3, 2021, 8:28 a.m. UTC | #3 
On Thu, Jun 03, 2021 at 10:25:00AM +0200, Daniel Vetter wrote:
> On Thu, Jun 03, 2021 at 10:24:21AM +0200, Daniel Vetter wrote:
> > On Wed, Jun 02, 2021 at 11:41:48AM -0500, Jason Ekstrand wrote:
> > > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> > > since that commit, we've been having issues where a hang in one client
> > > can propagate to another.  In particular, a hang in an app can propagate
> > > to the X server which causes the whole desktop to lock up.
> > 
> > I think we need a note to backporters here:
> > 
> > "For backporters: Please note that you _must_ have a backport of
> > https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
> > for otherwise backporting just this patch opens up a security bug."
> >  
> > Or something like that.
> 
> Oh also reordering the patch set so the 2 reverts which are cc: stable are
> first, then the other stuff on top that cleans up the fallout.

Oh also the longer commit message I've done would be nice to add. Or at
least link it or something like that.

https://lore.kernel.org/dri-devel/20210519101523.688398-1-daniel.vetter@ffwll.ch/

I think I mentioned this on irc, but got lost I guess.
-Daniel

> -Daniel
> 
> > -Daniel
> > 
> > > Signed-off-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > > Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
> > > Cc: <stable@vger.kernel.org> # v5.6+
> > > Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> > > Cc: Marcin Slusarz <marcin.slusarz@intel.com>
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> > > Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_request.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > index 970d8f4986bbe..b796197c07722 100644
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -1426,10 +1426,8 @@ i915_request_await_execution(struct i915_request *rq,
> > >  
> > >  	do {
> > >  		fence = *child++;
> > > -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > > -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > > +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > >  			continue;
> > > -		}
> > >  
> > >  		if (fence->context == rq->fence.context)
> > >  			continue;
> > > @@ -1527,10 +1525,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> > >  
> > >  	do {
> > >  		fence = *child++;
> > > -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > > -			i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > > +		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > >  			continue;
> > > -		}
> > >  
> > >  		/*
> > >  		 * Requests on the same timeline are explicitly ordered, along
> > > -- 
> > > 2.31.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Jason Ekstrand June 3, 2021, 3:28 p.m. UTC | #4 
On Thu, Jun 3, 2021 at 3:28 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jun 03, 2021 at 10:25:00AM +0200, Daniel Vetter wrote:
> > On Thu, Jun 03, 2021 at 10:24:21AM +0200, Daniel Vetter wrote:
> > > On Wed, Jun 02, 2021 at 11:41:48AM -0500, Jason Ekstrand wrote:
> > > > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> > > > since that commit, we've been having issues where a hang in one client
> > > > can propagate to another.  In particular, a hang in an app can propagate
> > > > to the X server which causes the whole desktop to lock up.
> > >
> > > I think we need a note to backporters here:
> > >
> > > "For backporters: Please note that you _must_ have a backport of
> > > https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
> > > for otherwise backporting just this patch opens up a security bug."

Done.

> > > Or something like that.
> >
> > Oh also reordering the patch set so the 2 reverts which are cc: stable are
> > first, then the other stuff on top that cleans up the fallout.

Done.

> Oh also the longer commit message I've done would be nice to add. Or at
> least link it or something like that.
>
> https://lore.kernel.org/dri-devel/20210519101523.688398-1-daniel.vetter@ffwll.ch/
>
> I think I mentioned this on irc, but got lost I guess.

Drp.  I thought I'd gotten that but I guess I failed.  Fixed now.

> -Daniel
>
> > -Daniel
> >
> > > -Daniel
> > >
> > > > Signed-off-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > > > Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
> > > > Cc: <stable@vger.kernel.org> # v5.6+
> > > > Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> > > > Cc: Marcin Slusarz <marcin.slusarz@intel.com>
> > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> > > > Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_request.c | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > > > index 970d8f4986bbe..b796197c07722 100644
> > > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > > @@ -1426,10 +1426,8 @@ i915_request_await_execution(struct i915_request *rq,
> > > >
> > > >   do {
> > > >           fence = *child++;
> > > > -         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > > > -                 i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > > > +         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > > >                   continue;
> > > > -         }
> > > >
> > > >           if (fence->context == rq->fence.context)
> > > >                   continue;
> > > > @@ -1527,10 +1525,8 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> > > >
> > > >   do {
> > > >           fence = *child++;
> > > > -         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > > > -                 i915_sw_fence_set_error_once(&rq->submit, fence->error);
> > > > +         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > > >                   continue;
> > > > -         }
> > > >
> > > >           /*
> > > >            * Requests on the same timeline are explicitly ordered, along
> > > > --
> > > > 2.31.1
> > > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 970d8f4986bbe..b796197c07722 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1426,10 +1426,8 @@  i915_request_await_execution(struct i915_request *rq,
 
 	do {
 		fence = *child++;
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-			i915_sw_fence_set_error_once(&rq->submit, fence->error);
+		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 			continue;
-		}
 
 		if (fence->context == rq->fence.context)
 			continue;
@@ -1527,10 +1525,8 @@  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
 
 	do {
 		fence = *child++;
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-			i915_sw_fence_set_error_once(&rq->submit, fence->error);
+		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 			continue;
-		}
 
 		/*
 		 * Requests on the same timeline are explicitly ordered, along