| Message ID | 20210902142057.929669-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] drm/i915: Release i915_gem_context from a worker | expand |
On 02/09/2021 15:20, Daniel Vetter wrote: > And use it anywhere we have open-coded checks for ctx->vm that really > only check for full ppgtt. > > Plus for paranoia add a GEM_BUG_ON that checks it's really only set > when we have full ppgtt, just in case. gem_context->vm is different > since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, > which is always set. > > v2: 0day found a testcase that I missed. > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- > drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- > 4 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 7a566fb7cca4..1eec85944c1f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv, > int err; > u32 id; > > - if (!rcu_access_pointer(ctx->vm)) > + if (!i915_gem_context_is_full_ppgtt(ctx)) It reads a bit wrong because GEM context cannot *be* full ppggt. It can be associated with a VM which is or isn't full ppgtt. So a test on a VM retrieved from a context is semnntically more correct. Perhaps you want to consider adding a helper to that effect instead? It could mean splitting into two helpers (getter + test) or maybe just renaming would work. Like i915_gem_context_has_full_ppgtt_vm(ctx)? Regards, Tvrtko > return -ENODEV; > > rcu_read_lock(); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h > index da6e8b506d96..37536a260e6e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h > @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx) > return rcu_dereference_protected(ctx->vm, lockdep_is_held(&ctx->mutex)); > } > > +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx) > +{ > + GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915)); > + > + return !!rcu_access_pointer(ctx->vm); > +} > + > static inline struct i915_address_space * > i915_gem_context_get_eb_vm(struct i915_gem_context *ctx) > { > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 905b1cbd22d5..40f08948f0b2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb) > return PTR_ERR(ctx); > > eb->gem_context = ctx; > - if (rcu_access_pointer(ctx->vm)) > + if (i915_gem_context_is_full_ppgtt(ctx)) > eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT; > > return 0; > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > index fc7fb33a3a52..947154e445a7 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > engine->name, > - yesno(!!rcu_access_pointer(ctx->vm)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > intel_context_put(ce); > kernel_context_close(ctx); > @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > engine->name, > - yesno(!!rcu_access_pointer(ctx->vm)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > intel_context_put(ce); > kernel_context_close(ctx); > @@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > ce->engine->name, > - yesno(!!ctx_vm(ctx)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > i915_gem_context_unlock_engines(ctx); > goto out_file; >
On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote: > On 02/09/2021 15:20, Daniel Vetter wrote: > > And use it anywhere we have open-coded checks for ctx->vm that really > > only check for full ppgtt. > > > > Plus for paranoia add a GEM_BUG_ON that checks it's really only set > > when we have full ppgtt, just in case. gem_context->vm is different > > since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, > > which is always set. > > > > v2: 0day found a testcase that I missed. > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > > drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- > > drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- > > 4 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > index 7a566fb7cca4..1eec85944c1f 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv, > > int err; > > u32 id; > > - if (!rcu_access_pointer(ctx->vm)) > > + if (!i915_gem_context_is_full_ppgtt(ctx)) > > It reads a bit wrong because GEM context cannot *be* full ppggt. It can be > associated with a VM which is or isn't full ppgtt. So a test on a VM > retrieved from a context is semnntically more correct. Perhaps you want to > consider adding a helper to that effect instead? It could mean splitting > into two helpers (getter + test) or maybe just renaming would work. Like > i915_gem_context_has_full_ppgtt_vm(ctx)? The pointer isn't set when the driver/context isn't running in full ppgtt mode. This is why I've added the GEM_BUG_ON to check we're not breaking any invariants. So yeah it is a full ppgtt context or it's not, that is indeed the question here. I'm happy to bikeshed the naming, but I don't see how your suggestion is an improvement. -Daniel > > Regards, > > Tvrtko > > > return -ENODEV; > > rcu_read_lock(); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h > > index da6e8b506d96..37536a260e6e 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h > > @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx) > > return rcu_dereference_protected(ctx->vm, lockdep_is_held(&ctx->mutex)); > > } > > +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx) > > +{ > > + GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915)); > > + > > + return !!rcu_access_pointer(ctx->vm); > > +} > > + > > static inline struct i915_address_space * > > i915_gem_context_get_eb_vm(struct i915_gem_context *ctx) > > { > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 905b1cbd22d5..40f08948f0b2 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb) > > return PTR_ERR(ctx); > > eb->gem_context = ctx; > > - if (rcu_access_pointer(ctx->vm)) > > + if (i915_gem_context_is_full_ppgtt(ctx)) > > eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT; > > return 0; > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > > index fc7fb33a3a52..947154e445a7 100644 > > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > > @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg) > > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > > ndwords, dw, max_dwords(obj), > > engine->name, > > - yesno(!!rcu_access_pointer(ctx->vm)), > > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > > err); > > intel_context_put(ce); > > kernel_context_close(ctx); > > @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg) > > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > > ndwords, dw, max_dwords(obj), > > engine->name, > > - yesno(!!rcu_access_pointer(ctx->vm)), > > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > > err); > > intel_context_put(ce); > > kernel_context_close(ctx); > > @@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg) > > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > > ndwords, dw, max_dwords(obj), > > ce->engine->name, > > - yesno(!!ctx_vm(ctx)), > > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > > err); > > i915_gem_context_unlock_engines(ctx); > > goto out_file; > >
On 02/09/2021 16:22, Daniel Vetter wrote: > On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote: >> On 02/09/2021 15:20, Daniel Vetter wrote: >>> And use it anywhere we have open-coded checks for ctx->vm that really >>> only check for full ppgtt. >>> >>> Plus for paranoia add a GEM_BUG_ON that checks it's really only set >>> when we have full ppgtt, just in case. gem_context->vm is different >>> since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, >>> which is always set. >>> >>> v2: 0day found a testcase that I missed. >>> >>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> Cc: Jon Bloomfield <jon.bloomfield@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> >>> Cc: Matthew Auld <matthew.auld@intel.com> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- >>> drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ >>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- >>> drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- >>> 4 files changed, 12 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c >>> index 7a566fb7cca4..1eec85944c1f 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c >>> @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv, >>> int err; >>> u32 id; >>> - if (!rcu_access_pointer(ctx->vm)) >>> + if (!i915_gem_context_is_full_ppgtt(ctx)) >> >> It reads a bit wrong because GEM context cannot *be* full ppggt. It can be >> associated with a VM which is or isn't full ppgtt. So a test on a VM >> retrieved from a context is semnntically more correct. Perhaps you want to >> consider adding a helper to that effect instead? It could mean splitting >> into two helpers (getter + test) or maybe just renaming would work. Like >> i915_gem_context_has_full_ppgtt_vm(ctx)? > > The pointer isn't set when the driver/context isn't running in full ppgtt > mode. This is why I've added the GEM_BUG_ON to check we're not breaking > any invariants. So yeah it is a full ppgtt context or it's not, that is > indeed the question here. > > I'm happy to bikeshed the naming, but I don't see how your suggestion is > an improvement. I think the pointer being set or not is implementation detail, for instance we could have it always set just like it is in intel_context. I simply think GEM context *isn't* full ppgtt, but the VM is. And since GEM context *points* to a VM, *has* is the right verb in my mind. You did not write why do you not see has as more correct than is so I don't want to be guessing too much. Regards, Tvrtko >> >> Regards, >> >> Tvrtko >> >>> return -ENODEV; >>> rcu_read_lock(); >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h >>> index da6e8b506d96..37536a260e6e 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h >>> @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx) >>> return rcu_dereference_protected(ctx->vm, lockdep_is_held(&ctx->mutex)); >>> } >>> +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx) >>> +{ >>> + GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915)); >>> + >>> + return !!rcu_access_pointer(ctx->vm); >>> +} >>> + >>> static inline struct i915_address_space * >>> i915_gem_context_get_eb_vm(struct i915_gem_context *ctx) >>> { >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> index 905b1cbd22d5..40f08948f0b2 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >>> @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb) >>> return PTR_ERR(ctx); >>> eb->gem_context = ctx; >>> - if (rcu_access_pointer(ctx->vm)) >>> + if (i915_gem_context_is_full_ppgtt(ctx)) >>> eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT; >>> return 0; >>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >>> index fc7fb33a3a52..947154e445a7 100644 >>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >>> @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg) >>> pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", >>> ndwords, dw, max_dwords(obj), >>> engine->name, >>> - yesno(!!rcu_access_pointer(ctx->vm)), >>> + yesno(i915_gem_context_is_full_ppgtt(ctx)), >>> err); >>> intel_context_put(ce); >>> kernel_context_close(ctx); >>> @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg) >>> pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", >>> ndwords, dw, max_dwords(obj), >>> engine->name, >>> - yesno(!!rcu_access_pointer(ctx->vm)), >>> + yesno(i915_gem_context_is_full_ppgtt(ctx)), >>> err); >>> intel_context_put(ce); >>> kernel_context_close(ctx); >>> @@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg) >>> pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", >>> ndwords, dw, max_dwords(obj), >>> ce->engine->name, >>> - yesno(!!ctx_vm(ctx)), >>> + yesno(i915_gem_context_is_full_ppgtt(ctx)), >>> err); >>> i915_gem_context_unlock_engines(ctx); >>> goto out_file; >>> >
> -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Thursday, September 2, 2021 9:42 AM > To: Daniel Vetter <daniel@ffwll.ch> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; DRI Development <dri- > devel@lists.freedesktop.org>; Intel Graphics Development <intel- > gfx@lists.freedesktop.org>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Vetter, Daniel > <daniel.vetter@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; > Chris Wilson <chris@chris-wilson.co.uk>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Thomas Hellström > <thomas.hellstrom@linux.intel.com>; Auld, Matthew > <matthew.auld@intel.com>; Landwerlin, Lionel G > <lionel.g.landwerlin@intel.com>; Dave Airlie <airlied@redhat.com>; Jason > Ekstrand <jason@jlekstrand.net> > Subject: Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add > i915_gem_context_is_full_ppgtt > > > On 02/09/2021 16:22, Daniel Vetter wrote: > > On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote: > >> On 02/09/2021 15:20, Daniel Vetter wrote: > >>> And use it anywhere we have open-coded checks for ctx->vm that really > >>> only check for full ppgtt. > >>> > >>> Plus for paranoia add a GEM_BUG_ON that checks it's really only set > >>> when we have full ppgtt, just in case. gem_context->vm is different > >>> since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, > >>> which is always set. > >>> > >>> v2: 0day found a testcase that I missed. > >>> > >>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>> Cc: Jon Bloomfield <jon.bloomfield@intel.com> > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > >>> Cc: Matthew Auld <matthew.auld@intel.com> > >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >>> Cc: Dave Airlie <airlied@redhat.com> > >>> Cc: Jason Ekstrand <jason@jlekstrand.net> > >>> --- > >>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > >>> drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ > >>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- > >>> drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- > >>> 4 files changed, 12 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c > b/drivers/gpu/drm/i915/gem/i915_gem_context.c > >>> index 7a566fb7cca4..1eec85944c1f 100644 > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > >>> @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct > drm_i915_file_private *file_priv, > >>> int err; > >>> u32 id; > >>> - if (!rcu_access_pointer(ctx->vm)) > >>> + if (!i915_gem_context_is_full_ppgtt(ctx)) > >> > >> It reads a bit wrong because GEM context cannot *be* full ppggt. It can > be > >> associated with a VM which is or isn't full ppgtt. So a test on a VM > >> retrieved from a context is semnntically more correct. Perhaps you want > to > >> consider adding a helper to that effect instead? It could mean splitting > >> into two helpers (getter + test) or maybe just renaming would work. Like > >> i915_gem_context_has_full_ppgtt_vm(ctx)? > > > > The pointer isn't set when the driver/context isn't running in full ppgtt > > mode. This is why I've added the GEM_BUG_ON to check we're not > breaking > > any invariants. So yeah it is a full ppgtt context or it's not, that is > > indeed the question here. > > > > I'm happy to bikeshed the naming, but I don't see how your suggestion is > > an improvement. > > I think the pointer being set or not is implementation detail, for > instance we could have it always set just like it is in intel_context. > > I simply think GEM context *isn't* full ppgtt, but the VM is. And since > GEM context *points* to a VM, *has* is the right verb in my mind. You > did not write why do you not see has as more correct than is so I don't > want to be guessing too much. FWIW, I agree with Tvrtko. i915_gem_context_is_full_ppgtt is incorrect grammar. It IS a bike shed, but, hey it'll live for a while.
On Thu, Sep 02, 2021 at 05:05:05PM +0000, Bloomfield, Jon wrote: > > -----Original Message----- > > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Sent: Thursday, September 2, 2021 9:42 AM > > To: Daniel Vetter <daniel@ffwll.ch> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; DRI Development <dri- > > devel@lists.freedesktop.org>; Intel Graphics Development <intel- > > gfx@lists.freedesktop.org>; Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com>; Vetter, Daniel > > <daniel.vetter@intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>; > > Chris Wilson <chris@chris-wilson.co.uk>; Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com>; Thomas Hellström > > <thomas.hellstrom@linux.intel.com>; Auld, Matthew > > <matthew.auld@intel.com>; Landwerlin, Lionel G > > <lionel.g.landwerlin@intel.com>; Dave Airlie <airlied@redhat.com>; Jason > > Ekstrand <jason@jlekstrand.net> > > Subject: Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add > > i915_gem_context_is_full_ppgtt > > > > > > On 02/09/2021 16:22, Daniel Vetter wrote: > > > On Thu, Sep 02, 2021 at 03:54:36PM +0100, Tvrtko Ursulin wrote: > > >> On 02/09/2021 15:20, Daniel Vetter wrote: > > >>> And use it anywhere we have open-coded checks for ctx->vm that really > > >>> only check for full ppgtt. > > >>> > > >>> Plus for paranoia add a GEM_BUG_ON that checks it's really only set > > >>> when we have full ppgtt, just in case. gem_context->vm is different > > >>> since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, > > >>> which is always set. > > >>> > > >>> v2: 0day found a testcase that I missed. > > >>> > > >>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > >>> Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > >>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > > >>> Cc: Matthew Auld <matthew.auld@intel.com> > > >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > >>> Cc: Dave Airlie <airlied@redhat.com> > > >>> Cc: Jason Ekstrand <jason@jlekstrand.net> > > >>> --- > > >>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > > >>> drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ > > >>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- > > >>> drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- > > >>> 4 files changed, 12 insertions(+), 5 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > >>> index 7a566fb7cca4..1eec85944c1f 100644 > > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > >>> @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct > > drm_i915_file_private *file_priv, > > >>> int err; > > >>> u32 id; > > >>> - if (!rcu_access_pointer(ctx->vm)) > > >>> + if (!i915_gem_context_is_full_ppgtt(ctx)) > > >> > > >> It reads a bit wrong because GEM context cannot *be* full ppggt. It can > > be > > >> associated with a VM which is or isn't full ppgtt. So a test on a VM > > >> retrieved from a context is semnntically more correct. Perhaps you want > > to > > >> consider adding a helper to that effect instead? It could mean splitting > > >> into two helpers (getter + test) or maybe just renaming would work. Like > > >> i915_gem_context_has_full_ppgtt_vm(ctx)? > > > > > > The pointer isn't set when the driver/context isn't running in full ppgtt > > > mode. This is why I've added the GEM_BUG_ON to check we're not > > breaking > > > any invariants. So yeah it is a full ppgtt context or it's not, that is > > > indeed the question here. > > > > > > I'm happy to bikeshed the naming, but I don't see how your suggestion is > > > an improvement. > > > > I think the pointer being set or not is implementation detail, for > > instance we could have it always set just like it is in intel_context. > > > > I simply think GEM context *isn't* full ppgtt, but the VM is. And since > > GEM context *points* to a VM, *has* is the right verb in my mind. You > > did not write why do you not see has as more correct than is so I don't > > want to be guessing too much. > > FWIW, I agree with Tvrtko. i915_gem_context_is_full_ppgtt is incorrect > grammar. It IS a bike shed, but, hey it'll live for a while. Generally all our feature checks are of the various is_foo variety. HAS_FULL_PPGTT is one of the very rare exceptions. So it's a question of "is gem_ctx foo", not "has gem_ctx foo". The fact that we implement the check by looking at a pointer doesn't matter. And yes if it is full ppgtt, it also has it's own private vm, but we don't care about that part at all. It's just a distraction. Anyway I'll repaint, least because the HAS_FULL_PPGTT thing is almost a decade old by now :-) -Daniel
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 7a566fb7cca4..1eec85944c1f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv, int err; u32 id; - if (!rcu_access_pointer(ctx->vm)) + if (!i915_gem_context_is_full_ppgtt(ctx)) return -ENODEV; rcu_read_lock(); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h index da6e8b506d96..37536a260e6e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx) return rcu_dereference_protected(ctx->vm, lockdep_is_held(&ctx->mutex)); } +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx) +{ + GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915)); + + return !!rcu_access_pointer(ctx->vm); +} + static inline struct i915_address_space * i915_gem_context_get_eb_vm(struct i915_gem_context *ctx) { diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 905b1cbd22d5..40f08948f0b2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb) return PTR_ERR(ctx); eb->gem_context = ctx; - if (rcu_access_pointer(ctx->vm)) + if (i915_gem_context_is_full_ppgtt(ctx)) eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT; return 0; diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c index fc7fb33a3a52..947154e445a7 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg) pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", ndwords, dw, max_dwords(obj), engine->name, - yesno(!!rcu_access_pointer(ctx->vm)), + yesno(i915_gem_context_is_full_ppgtt(ctx)), err); intel_context_put(ce); kernel_context_close(ctx); @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg) pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", ndwords, dw, max_dwords(obj), engine->name, - yesno(!!rcu_access_pointer(ctx->vm)), + yesno(i915_gem_context_is_full_ppgtt(ctx)), err); intel_context_put(ce); kernel_context_close(ctx); @@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg) pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", ndwords, dw, max_dwords(obj), ce->engine->name, - yesno(!!ctx_vm(ctx)), + yesno(i915_gem_context_is_full_ppgtt(ctx)), err); i915_gem_context_unlock_engines(ctx); goto out_file;