From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Subject: Re: [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member
Date: Mon, 16 Aug 2021 15:30:49 +0200 [thread overview]
Message-ID: <ffc6a66a-555f-9c39-4131-90dc33395f23@linux.intel.com> (raw)
In-Reply-To: <CAM0jSHPLAtyYofaLzMsrPsyLX=13UAfw3tzbAQoW+F5+XJgrfQ@mail.gmail.com>
On 8/16/21 3:25 PM, Matthew Auld wrote:
> On Mon, 16 Aug 2021 at 09:49, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> It's only used by the for_i915_gem_ww() macro and we can use
>> the (typically) on-stack _err variable in its place.
>>
>> While initially setting the _err variable to -EDEADLK to enter the
>> loop, we clear it before actually entering using fetch_and_zero() to
>> avoid empty loops or code not setting the _err variable running forever.
>>
>> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++---------------
>> 1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h
>> index f6b1a796667b..98348b1e6182 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_ww.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h
>> @@ -7,12 +7,13 @@
>>
>> #include <drm/drm_drv.h>
>>
>> +#include "i915_utils.h"
>> +
>> struct i915_gem_ww_ctx {
>> struct ww_acquire_ctx ctx;
>> struct list_head obj_list;
>> struct drm_i915_gem_object *contended;
>> - unsigned short intr;
>> - unsigned short loop;
>> + bool intr;
>> };
>>
>> void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
>> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
>> /* Internal functions used by the inlines! Don't use. */
>> static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
>> {
>> - ww->loop = 0;
>> if (err == -EDEADLK) {
>> err = i915_gem_ww_ctx_backoff(ww);
>> if (!err)
>> - ww->loop = 1;
>> + err = -EDEADLK;
>> }
>>
>> - if (!ww->loop)
>> + if (err != -EDEADLK)
>> i915_gem_ww_ctx_fini(ww);
>>
>> return err;
>> }
>>
>> -static inline void
>> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr)
>> -{
>> - i915_gem_ww_ctx_init(ww, intr);
>> - ww->loop = 1;
>> -}
>> -
>> -#define for_i915_gem_ww(_ww, _err, _intr) \
>> - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \
>> +#define for_i915_gem_ww(_ww, _err, _intr) \
>> + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
>> + fetch_and_zero(&_err) == -EDEADLK; \
> Doesn't this now hide "normal" errors, like say get_pages() returning
> -ENOSPC or so?
Yes, good catch. We should either just clear the -EDEADLK case, or not
clear the error at all..
/Thomas
WARNING: multiple messages have this Message-ID (diff)
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member
Date: Mon, 16 Aug 2021 15:30:49 +0200 [thread overview]
Message-ID: <ffc6a66a-555f-9c39-4131-90dc33395f23@linux.intel.com> (raw)
In-Reply-To: <CAM0jSHPLAtyYofaLzMsrPsyLX=13UAfw3tzbAQoW+F5+XJgrfQ@mail.gmail.com>
On 8/16/21 3:25 PM, Matthew Auld wrote:
> On Mon, 16 Aug 2021 at 09:49, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> It's only used by the for_i915_gem_ww() macro and we can use
>> the (typically) on-stack _err variable in its place.
>>
>> While initially setting the _err variable to -EDEADLK to enter the
>> loop, we clear it before actually entering using fetch_and_zero() to
>> avoid empty loops or code not setting the _err variable running forever.
>>
>> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++---------------
>> 1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h
>> index f6b1a796667b..98348b1e6182 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_ww.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h
>> @@ -7,12 +7,13 @@
>>
>> #include <drm/drm_drv.h>
>>
>> +#include "i915_utils.h"
>> +
>> struct i915_gem_ww_ctx {
>> struct ww_acquire_ctx ctx;
>> struct list_head obj_list;
>> struct drm_i915_gem_object *contended;
>> - unsigned short intr;
>> - unsigned short loop;
>> + bool intr;
>> };
>>
>> void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
>> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
>> /* Internal functions used by the inlines! Don't use. */
>> static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
>> {
>> - ww->loop = 0;
>> if (err == -EDEADLK) {
>> err = i915_gem_ww_ctx_backoff(ww);
>> if (!err)
>> - ww->loop = 1;
>> + err = -EDEADLK;
>> }
>>
>> - if (!ww->loop)
>> + if (err != -EDEADLK)
>> i915_gem_ww_ctx_fini(ww);
>>
>> return err;
>> }
>>
>> -static inline void
>> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr)
>> -{
>> - i915_gem_ww_ctx_init(ww, intr);
>> - ww->loop = 1;
>> -}
>> -
>> -#define for_i915_gem_ww(_ww, _err, _intr) \
>> - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \
>> +#define for_i915_gem_ww(_ww, _err, _intr) \
>> + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
>> + fetch_and_zero(&_err) == -EDEADLK; \
> Doesn't this now hide "normal" errors, like say get_pages() returning
> -ENOSPC or so?
Yes, good catch. We should either just clear the -EDEADLK case, or not
clear the error at all..
/Thomas
next prev parent reply other threads:[~2021-08-16 13:30 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-16 8:48 [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member Thomas Hellström
2021-08-16 8:48 ` [Intel-gfx] " Thomas Hellström
2021-08-16 9:41 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2021-08-16 10:12 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-08-16 12:05 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-08-16 13:25 ` [PATCH] " Matthew Auld
2021-08-16 13:25 ` [Intel-gfx] " Matthew Auld
2021-08-16 13:30 ` Thomas Hellström [this message]
2021-08-16 13:30 ` Thomas Hellström
2021-08-16 13:34 ` Maarten Lankhorst
2021-08-16 13:34 ` [Intel-gfx] " Maarten Lankhorst
2021-08-16 13:49 ` Thomas Hellström
2021-08-16 13:49 ` [Intel-gfx] " Thomas Hellström
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ffc6a66a-555f-9c39-4131-90dc33395f23@linux.intel.com \
--to=thomas.hellstrom@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=matthew.william.auld@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.