| Message ID | 20201016104444.1492028-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove obj->mm.lock! | expand |
On Fri, Oct 16, 2020 at 12:43:45PM +0200, Maarten Lankhorst wrote: > i915_vma_pin may fail with -EDEADLK when we start locking page tables, > so ensure we handle this correctly. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 23 +++++++++++++++---- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index a199336792fb..0f5efced0b87 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -419,13 +419,14 @@ static u64 eb_pin_flags(const struct drm_i915_gem_exec_object2 *entry, > return pin_flags; > } > > -static inline bool > +static inline int > eb_pin_vma(struct i915_execbuffer *eb, > const struct drm_i915_gem_exec_object2 *entry, > struct eb_vma *ev) > { > struct i915_vma *vma = ev->vma; > u64 pin_flags; > + int err; > > if (vma->node.size) > pin_flags = vma->node.start; > @@ -438,16 +439,24 @@ eb_pin_vma(struct i915_execbuffer *eb, > > /* Attempt to reuse the current location if available */ > /* TODO: Add -EDEADLK handling here */ Drop the TODO? > - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags))) { > + err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags); > + if (err == -EDEADLK) > + return err; > + > + if (unlikely(err)) { > if (entry->flags & EXEC_OBJECT_PINNED) > return false; > > /* Failing that pick any _free_ space if suitable */ > - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, > + err = i915_vma_pin_ww(vma, &eb->ww, > entry->pad_to_size, > entry->alignment, > eb_pin_flags(entry, ev->flags) | > - PIN_USER | PIN_NOEVICT))) > + PIN_USER | PIN_NOEVICT); > + if (err == -EDEADLK) > + return err; > + > + if (unlikely(err)) > return false; Confusing to return a boolean 'false' while the return value of this function is an int. Return '0' if that is the intent, which I believe it based on how the caller handles the return. > } > > @@ -900,7 +909,11 @@ static int eb_validate_vmas(struct i915_execbuffer *eb) > if (err) > return err; > Can't say I love the triple comparison of the return values, but if you need to do this I'd put all of comparison in the same clause. Just my opinion. Matt > - if (eb_pin_vma(eb, entry, ev)) { > + err = eb_pin_vma(eb, entry, ev); > + if (err < 0) > + return err; > + > + if (err > 0) { > if (entry->offset != vma->node.start) { > entry->offset = vma->node.start | UPDATE; > eb->args->flags |= __EXEC_HAS_RELOC; > -- > 2.28.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 20-10-2020 om 22:18 schreef Matthew Brost: > On Fri, Oct 16, 2020 at 12:43:45PM +0200, Maarten Lankhorst wrote: >> i915_vma_pin may fail with -EDEADLK when we start locking page tables, >> so ensure we handle this correctly. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 23 +++++++++++++++---- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> index a199336792fb..0f5efced0b87 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> @@ -419,13 +419,14 @@ static u64 eb_pin_flags(const struct drm_i915_gem_exec_object2 *entry, >> return pin_flags; >> } >> >> -static inline bool >> +static inline int >> eb_pin_vma(struct i915_execbuffer *eb, >> const struct drm_i915_gem_exec_object2 *entry, >> struct eb_vma *ev) >> { >> struct i915_vma *vma = ev->vma; >> u64 pin_flags; >> + int err; >> >> if (vma->node.size) >> pin_flags = vma->node.start; >> @@ -438,16 +439,24 @@ eb_pin_vma(struct i915_execbuffer *eb, >> >> /* Attempt to reuse the current location if available */ >> /* TODO: Add -EDEADLK handling here */ > Drop the TODO? > >> - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags))) { >> + err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags); >> + if (err == -EDEADLK) >> + return err; >> + >> + if (unlikely(err)) { >> if (entry->flags & EXEC_OBJECT_PINNED) >> return false; >> >> /* Failing that pick any _free_ space if suitable */ >> - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, >> + err = i915_vma_pin_ww(vma, &eb->ww, >> entry->pad_to_size, >> entry->alignment, >> eb_pin_flags(entry, ev->flags) | >> - PIN_USER | PIN_NOEVICT))) >> + PIN_USER | PIN_NOEVICT); >> + if (err == -EDEADLK) >> + return err; >> + >> + if (unlikely(err)) >> return false; > Confusing to return a boolean 'false' while the return value of this > function is an int. Return '0' if that is the intent, which I believe it > based on how the caller handles the return. Yeah, I think it makes more sense to change eb_pin_vma to a proper int, instead of a special one. In most places we can just propagate the error. I will respin this patch. :) >> } >> >> @@ -900,7 +909,11 @@ static int eb_validate_vmas(struct i915_execbuffer *eb) >> if (err) >> return err; >> > Can't say I love the triple comparison of the return values, but if you > need to do this I'd put all of comparison in the same clause. Just my > opinion. Neither. I think I will just special case -EDEADLK, which should be easy with the fix to eb_pin_vma I suggested above. > > Matt > >> - if (eb_pin_vma(eb, entry, ev)) { >> + err = eb_pin_vma(eb, entry, ev); >> + if (err < 0) >> + return err; >> + >> + if (err > 0) { >> if (entry->offset != vma->node.start) { >> entry->offset = vma->node.start | UPDATE; >> eb->args->flags |= __EXEC_HAS_RELOC; >> -- >> 2.28.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/16/20 12:43 PM, Maarten Lankhorst wrote: > i915_vma_pin may fail with -EDEADLK when we start locking page tables, > so ensure we handle this correctly. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> When previous review comments addressed, Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index a199336792fb..0f5efced0b87 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -419,13 +419,14 @@ static u64 eb_pin_flags(const struct drm_i915_gem_exec_object2 *entry, return pin_flags; } -static inline bool +static inline int eb_pin_vma(struct i915_execbuffer *eb, const struct drm_i915_gem_exec_object2 *entry, struct eb_vma *ev) { struct i915_vma *vma = ev->vma; u64 pin_flags; + int err; if (vma->node.size) pin_flags = vma->node.start; @@ -438,16 +439,24 @@ eb_pin_vma(struct i915_execbuffer *eb, /* Attempt to reuse the current location if available */ /* TODO: Add -EDEADLK handling here */ - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags))) { + err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, pin_flags); + if (err == -EDEADLK) + return err; + + if (unlikely(err)) { if (entry->flags & EXEC_OBJECT_PINNED) return false; /* Failing that pick any _free_ space if suitable */ - if (unlikely(i915_vma_pin_ww(vma, &eb->ww, + err = i915_vma_pin_ww(vma, &eb->ww, entry->pad_to_size, entry->alignment, eb_pin_flags(entry, ev->flags) | - PIN_USER | PIN_NOEVICT))) + PIN_USER | PIN_NOEVICT); + if (err == -EDEADLK) + return err; + + if (unlikely(err)) return false; } @@ -900,7 +909,11 @@ static int eb_validate_vmas(struct i915_execbuffer *eb) if (err) return err; - if (eb_pin_vma(eb, entry, ev)) { + err = eb_pin_vma(eb, entry, ev); + if (err < 0) + return err; + + if (err > 0) { if (entry->offset != vma->node.start) { entry->offset = vma->node.start | UPDATE; eb->args->flags |= __EXEC_HAS_RELOC;
i915_vma_pin may fail with -EDEADLK when we start locking page tables, so ensure we handle this correctly. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-)