[Intel-gfx] [PATCH] drm/i915/gt: Fix reservation address in ggtt_reserve_guc_top

[Javier Pello]

On Thu, 31 Aug 2023 15:49:28 -0700
"Ceraolo Spurio, Daniele" <daniele.ceraolospurio at intel.com> 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 at otheo.eu>
> > Cc: intel-gfx at lists.freedesktop.org
> > Cc: stable at 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(&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 at intel.com>

Thank you.

Javier