On Thu, 31 Aug 2023 15:49:28 -0700
"Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx> 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 0000:00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT for GuC
> > i915 0000:00:02.0: Device initialization failed (-28)
> > i915 0000: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 0000: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 <devel@xxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx # 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(&ggtt->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(&ggtt->vm, NULL, &ggtt->uc_fw, size,
> > - GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
> > - PIN_NOEVICT);
> > + ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->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 <daniele.ceraolospurio@xxxxxxxxx>
Thank you.
Javier
