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

[Ceraolo Spurio, Daniele]

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

> 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.

> ---
>   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
  */

> +#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>

Daniele

>   	if (ret)
>   		drm_dbg(&ggtt->vm.i915->drm,
>   			"Failed to reserve top of GGTT for GuC\n");