Re: [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
> A few fixes/updates are required around the GSC memory allocation and it
> is easier to do them all at the same time. The changes are as follows:


alan:snip

> @@ -109,38 +110,21 @@ static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc)
>  {
>  	struct intel_gt *gt = gsc_uc_to_gt(gsc);
>  	struct drm_i915_private *i915 = gt->i915;
> -	struct drm_i915_gem_object *obj;
> -	void *src, *dst;
> +	void *src;


alan:snip

>  
> -	memset(dst, 0, obj->base.size);
> -	memcpy(dst, src, gsc->fw.size);
> +	memset_io(gsc->local_vaddr, 0, gsc->local->size);
> +	memcpy_toio(gsc->local_vaddr, src, gsc->fw.size);
alan: i wonder if it there is benefit to do the memcpy_toio first
and then do the memset_io but only for the balance of area from
offset 'gsc->fw.size' for (gsc->local->size - gsc->fw.size) bytes.

alan:snip

> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -130,26 +130,85 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
>  	}
>  }
>  
> +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size)
alan:snip

> +	obj = i915_gem_object_create_stolen(gt->i915, size);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +
> +	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
alan: should we be passing in the PIN_MAPPABLE flag into the last param?


alan:snip

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> index a2a0813b8a76..c01286dddbdb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> @@ -18,6 +18,7 @@ struct intel_gsc_uc {
>  
>  	/* GSC-specific additions */
>  	struct i915_vma *local; /* private memory for GSC usage */
> +	void __iomem *local_vaddr; /* pointer to access the private memory */
alan:nit: relooking at the these variable names that originate from
last year's patch you worked on introducing gsc_uc, i am wondering now
if we should rename "local" to "privmem" and local_vaddr becomes privmem_vaddr.
(no significant reason other than improving readibility of the code)