[Intel-gfx] [PATCH v2] drm/i915/gt: Fix reservation address in ggtt_reserve_guc_top
There is an assertion in ggtt_reserve_guc_top that the global GTT
is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit
platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc
("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON
is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the
subsequent reservation fails and the driver fails to initialise
the device:
i915 :00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT
for GuC
i915 :00:02.0: Device initialization failed (-28)
i915 :00:02.0: Please file a bug on drm/i915; see
https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for
details.
i915: probe of :00:02.0 failed with error -28
Make the reservation at the top of the available space, whatever
that is, instead of assuming that the top will be GUC_GGTT_TOP.
Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT")
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9080
Signed-off-by: Javier Pello
Reviewed-by: Daniele Ceraolo Spurio
Cc: Fernando Pacheco
Cc: Chris Wilson
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: intel-gfx@lists.freedesktop.org
Cc: sta...@vger.kernel.org # v5.3+
---
v2: style change
drivers/gpu/drm/i915/gt/intel_ggtt.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index dd0ed941..da21f278 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -511,20 +511,31 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm,
vm->clear_range(vm, vma_res->start, vma_res->vma_size);
}
+/*
+ * Reserve the top of the GuC address space for firmware images. Addresses
+ * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC,
+ * which makes for a suitable range to hold GuC/HuC firmware images if the
+ * size of the GGTT is 4G. However, on a 32-bit platform the size of the GGTT
+ * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a chunk
+ * of the same size anyway, which is far more than needed, to keep the logic
+ * in uc_fw_ggtt_offset() simple.
+ */
+#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP)
+
static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
{
- u64 size;
+ u64 offset;
int ret;
if (!intel_uc_uses_guc(>vm.gt->uc))
return 0;
- GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
- size = ggtt->vm.total - GUC_GGTT_TOP;
+ GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE);
+ offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE;
- ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw, size,
- GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
- PIN_NOEVICT);
+ ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw,
+ GUC_TOP_RESERVE_SIZE, offset,
+ I915_COLOR_UNEVICTABLE, PIN_NOEVICT);
if (ret)
drm_dbg(>vm.i915->drm,
"Failed to reserve top of GGTT for GuC\n");
--
2.41.0
Re: [Intel-gfx] [PATCH] drm/i915/gt: Fix reservation address in ggtt_reserve_guc_top
On Thu, 31 Aug 2023 15:49:28 -0700
"Ceraolo Spurio, Daniele" wrote:
> On 8/25/2023 7:33 AM, Javier Pello wrote:
> > There is an assertion in ggtt_reserve_guc_top that the global GTT
> > is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit
> > platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc
> > ("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON
> > is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the
> > subsequent reservation fails and the driver fails to initialise
> > the device:
> >
> > i915 :00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of
> > GGTT for GuC
> > i915 :00:02.0: Device initialization failed (-28)
> > i915 :00:02.0: Please file a bug on drm/i915; see
> > https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for
> > details.
> > i915: probe of :00:02.0 failed with error -28
> >
> > Make the reservation at the top of the available space, whatever
> > that is, instead of assuming that the top will be GUC_GGTT_TOP.
> >
> > Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT")
>
> For tracking, it might be good to also add:
>
> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9080
Sure.
> > Signed-off-by: Javier Pello
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: sta...@vger.kernel.org # v5.3+
>
> Need the full CC list here, so that when the patch gets back-ported the
> relevant developers get automatically added.
I was unsure if I should add them from the start or wait for a review.
> > ---
> > drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 +++--
> > 1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > index e9328e1a..0157bebb 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > @@ -511,20 +511,29 @@ void intel_ggtt_unbind_vma(struct i915_address_space
> > *vm,
> > vm->clear_range(vm, vma_res->start, vma_res->vma_size);
> > }
> >
> > +/* Reserve the top of the GuC address space for firmware images. Addresses
> > + * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC,
> > + * which makes for a suitable range to hold GuC/HuC firmware images if the
> > + * size of the GGTT is 4G. However, on a 32-bit platform the size of the
> > GGTT
> > + * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a
> > chunk
> > + * of the same size anyway, which is far more than needed, to keep the
> > logic
> > + * in uc_fw_ggtt_offset() simple. */
>
> Style: multi-line comment should be formatted as:
>
> /*
> * Text
> * more text
> */
Yes, I realised that after I sent the patch, but I thought I would
wait for replies rather than resending with only a style change.
> > +#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP)
> > +
> > static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
> > {
> > - u64 size;
> > + u64 offset;
> > int ret;
> >
> > if (!intel_uc_uses_guc(>vm.gt->uc))
> > return 0;
> >
> > - GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
> > - size = ggtt->vm.total - GUC_GGTT_TOP;
> > + GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE);
> > + offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE;
> >
> > - ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw, size,
> > - GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
> > - PIN_NOEVICT);
> > + ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw,
> > + GUC_TOP_RESERVE_SIZE, offset,
> > + I915_COLOR_UNEVICTABLE, PIN_NOEVICT);
>
> The code change looks good to me, so with the style fix and the
> additions to the commit message this is:
>
> Reviewed-by: Daniele Ceraolo Spurio
Thank you.
Javier
[Intel-gfx] [PATCH] drm/i915/gt: Fix reservation address in ggtt_reserve_guc_top
There is an assertion in ggtt_reserve_guc_top that the global GTT
is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit
platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc
("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON
is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the
subsequent reservation fails and the driver fails to initialise
the device:
i915 :00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT
for GuC
i915 :00:02.0: Device initialization failed (-28)
i915 :00:02.0: Please file a bug on drm/i915; see
https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for
details.
i915: probe of :00:02.0 failed with error -28
Make the reservation at the top of the available space, whatever
that is, instead of assuming that the top will be GUC_GGTT_TOP.
Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT")
Signed-off-by: Javier Pello
Cc: intel-gfx@lists.freedesktop.org
Cc: sta...@vger.kernel.org # v5.3+
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index e9328e1a..0157bebb 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -511,20 +511,29 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm,
vm->clear_range(vm, vma_res->start, vma_res->vma_size);
}
+/* Reserve the top of the GuC address space for firmware images. Addresses
+ * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC,
+ * which makes for a suitable range to hold GuC/HuC firmware images if the
+ * size of the GGTT is 4G. However, on a 32-bit platform the size of the GGTT
+ * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a chunk
+ * of the same size anyway, which is far more than needed, to keep the logic
+ * in uc_fw_ggtt_offset() simple. */
+#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP)
+
static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
{
- u64 size;
+ u64 offset;
int ret;
if (!intel_uc_uses_guc(>vm.gt->uc))
return 0;
- GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
- size = ggtt->vm.total - GUC_GGTT_TOP;
+ GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE);
+ offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE;
- ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw, size,
- GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
- PIN_NOEVICT);
+ ret = i915_gem_gtt_reserve(>vm, NULL, >uc_fw,
+ GUC_TOP_RESERVE_SIZE, offset,
+ I915_COLOR_UNEVICTABLE, PIN_NOEVICT);
if (ret)
drm_dbg(>vm.i915->drm,
"Failed to reserve top of GGTT for GuC\n");
--
2.41.0
