From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Matthew Auld <matthew.william.auld@gmail.com> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>, ML dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member Date: Mon, 16 Aug 2021 15:49:29 +0200 [thread overview] Message-ID: <85d1661e-d9bb-f64a-70d5-ffe98e2acf12@linux.intel.com> (raw) In-Reply-To: <00736b43-8abf-ff15-e9dd-5ed54da77379@linux.intel.com> On 8/16/21 3:34 PM, Maarten Lankhorst wrote: > Op 16-08-2021 om 15:30 schreef Thomas Hellström: >> 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 > I believe not setting _err is a bug anyway. Why would you do such a loop without at least one err = ww_mutex_lock(&ww); ? > > Infinite loop would catch that at first test. OK, I'll skip the clearing then. /Thomas > > ~Maarten >
WARNING: multiple messages have this Message-ID (diff)
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Matthew Auld <matthew.william.auld@gmail.com> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>, ML dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member Date: Mon, 16 Aug 2021 15:49:29 +0200 [thread overview] Message-ID: <85d1661e-d9bb-f64a-70d5-ffe98e2acf12@linux.intel.com> (raw) In-Reply-To: <00736b43-8abf-ff15-e9dd-5ed54da77379@linux.intel.com> On 8/16/21 3:34 PM, Maarten Lankhorst wrote: > Op 16-08-2021 om 15:30 schreef Thomas Hellström: >> 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 > I believe not setting _err is a bug anyway. Why would you do such a loop without at least one err = ww_mutex_lock(&ww); ? > > Infinite loop would catch that at first test. OK, I'll skip the clearing then. /Thomas > > ~Maarten >
next prev parent reply other threads:[~2021-08-16 13:49 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 2021-08-16 13:30 ` [Intel-gfx] " 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 [this message] 2021-08-16 13:49 ` 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=85d1661e-d9bb-f64a-70d5-ffe98e2acf12@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: linkBe 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.