| Message ID | 1453465172-28125-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote: > In the error-handling paths of i915_gem_do_execbuffer() and > intel_crtc_page_flip(), the local pointer-to-request variables > were expected to be either valid pointers or NULL. Since > > 2682708 drm/i915: simplify allocation of driver-internal requests > > they could also be ERR_PTR() values, so the tests need to be > updated to accommodate this case. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Quick quiz, name at least testcase that exercised this code? -Chris
On 22/01/16 12:19, Dave Gordon wrote: > In the error-handling paths of i915_gem_do_execbuffer() and > intel_crtc_page_flip(), the local pointer-to-request variables > were expected to be either valid pointers or NULL. Since > > 2682708 drm/i915: simplify allocation of driver-internal requests > > they could also be ERR_PTR() values, so the tests need to be > updated to accommodate this case. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Not sure if CI will pick up a new patch in an old series. Anyway: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 2dc08ce..a7bd555 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags) > * must be freed again. If it was submitted then it is being tracked > * on the active request list and no clean up is required here. > */ > - if (ret && req) > + if (ret && !IS_ERR_OR_NULL(req)) > i915_gem_request_cancel(req); > > mutex_unlock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8104511..b88cdac 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > cleanup_unpin: > intel_unpin_fb_obj(fb, crtc->primary->state); > cleanup_pending: > - if (request) > + if (!IS_ERR_OR_NULL(request)) > i915_gem_request_cancel(request); > atomic_dec(&intel_crtc->unpin_work_count); > mutex_unlock(&dev->struct_mutex); >
On 22/01/16 13:01, Chris Wilson wrote: > On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote: >> In the error-handling paths of i915_gem_do_execbuffer() and >> intel_crtc_page_flip(), the local pointer-to-request variables >> were expected to be either valid pointers or NULL. Since >> >> 2682708 drm/i915: simplify allocation of driver-internal requests >> >> they could also be ERR_PTR() values, so the tests need to be >> updated to accommodate this case. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Quick quiz, name at least testcase that exercised this code? gem_close_race did it for me, but can you explain the weird ERR_PTR of *ffca ? :) Regards, Tvrtko
On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote: > > On 22/01/16 13:01, Chris Wilson wrote: > >On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote: > >>In the error-handling paths of i915_gem_do_execbuffer() and > >>intel_crtc_page_flip(), the local pointer-to-request variables > >>were expected to be either valid pointers or NULL. Since > >> > >> 2682708 drm/i915: simplify allocation of driver-internal requests > >> > >>they could also be ERR_PTR() values, so the tests need to be > >>updated to accommodate this case. > >> > >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > >>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > >Quick quiz, name at least testcase that exercised this code? > > gem_close_race did it for me, but can you explain the weird ERR_PTR > of *ffca ? :) -54 That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc. -Chris
On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote: > > On 22/01/16 12:19, Dave Gordon wrote: > >In the error-handling paths of i915_gem_do_execbuffer() and > >intel_crtc_page_flip(), the local pointer-to-request variables > >were expected to be either valid pointers or NULL. Since > > > > 2682708 drm/i915: simplify allocation of driver-internal requests > > > >they could also be ERR_PTR() values, so the tests need to be > >updated to accommodate this case. > > > >Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > Not sure if CI will pick up a new patch in an old series. I think it'll treat this one as a replacement for patch 1/4 and then ofc totally fall over. So would need a resend of the entire pile. -Daniel > > Anyway: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Regards, > > Tvrtko > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >index 2dc08ce..a7bd555 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags) > > * must be freed again. If it was submitted then it is being tracked > > * on the active request list and no clean up is required here. > > */ > >- if (ret && req) > >+ if (ret && !IS_ERR_OR_NULL(req)) > > i915_gem_request_cancel(req); > > > > mutex_unlock(&dev->struct_mutex); > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index 8104511..b88cdac 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > > cleanup_unpin: > > intel_unpin_fb_obj(fb, crtc->primary->state); > > cleanup_pending: > >- if (request) > >+ if (!IS_ERR_OR_NULL(request)) > > i915_gem_request_cancel(request); > > atomic_dec(&intel_crtc->unpin_work_count); > > mutex_unlock(&dev->struct_mutex); > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 25/01/16 16:28, Daniel Vetter wrote: > On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote: >> >> On 22/01/16 12:19, Dave Gordon wrote: >>> In the error-handling paths of i915_gem_do_execbuffer() and >>> intel_crtc_page_flip(), the local pointer-to-request variables >>> were expected to be either valid pointers or NULL. Since >>> >>> 2682708 drm/i915: simplify allocation of driver-internal requests >>> >>> they could also be ERR_PTR() values, so the tests need to be >>> updated to accommodate this case. >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- >>> drivers/gpu/drm/i915/intel_display.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> Not sure if CI will pick up a new patch in an old series. > > I think it'll treat this one as a replacement for patch 1/4 and then ofc > totally fall over. So would need a resend of the entire pile. The rest of the pile has been merged already so I think just this patch on its own then (not as in-reply-to anything). Regards, Tvrtko
On 22/01/16 13:34, Chris Wilson wrote: > On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote: >> >> On 22/01/16 13:01, Chris Wilson wrote: >>> On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote: >>>> In the error-handling paths of i915_gem_do_execbuffer() and >>>> intel_crtc_page_flip(), the local pointer-to-request variables >>>> were expected to be either valid pointers or NULL. Since >>>> >>>> 2682708 drm/i915: simplify allocation of driver-internal requests >>>> >>>> they could also be ERR_PTR() values, so the tests need to be >>>> updated to accommodate this case. >>>> >>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Quick quiz, name at least testcase that exercised this code? >> >> gem_close_race did it for me, but can you explain the weird ERR_PTR >> of *ffca ? :) > > -54 > > That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc. > -Chris Assuming that's the fault address, it will actually be the sum of the errno mistakenly passed to i915_gem_request_cancel(), plus the offset of 'ringbuf' in the drm_i915_gem_request structure: void i915_gem_request_cancel(struct drm_i915_gem_request *req) { intel_ring_reserved_space_cancel(req->ringbuf); i915_gem_request_unreference(req); } As it happens, 'ringbuf' is at offset 0x38 (56), so the errno was -2, which is the expected error -ENOENT :) .Dave.
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 2dc08ce..a7bd555 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags) * must be freed again. If it was submitted then it is being tracked * on the active request list and no clean up is required here. */ - if (ret && req) + if (ret && !IS_ERR_OR_NULL(req)) i915_gem_request_cancel(req); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8104511..b88cdac 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, cleanup_unpin: intel_unpin_fb_obj(fb, crtc->primary->state); cleanup_pending: - if (request) + if (!IS_ERR_OR_NULL(request)) i915_gem_request_cancel(request); atomic_dec(&intel_crtc->unpin_work_count); mutex_unlock(&dev->struct_mutex);
In the error-handling paths of i915_gem_do_execbuffer() and intel_crtc_page_flip(), the local pointer-to-request variables were expected to be either valid pointers or NULL. Since 2682708 drm/i915: simplify allocation of driver-internal requests they could also be ERR_PTR() values, so the tests need to be updated to accommodate this case. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)