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

[Ceraolo Spurio, Daniele]

On 5/22/2023 5:13 PM, Teres Alexis, Alan Previn wrote:
> 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.

I usually always memset first to start from a clean slate, but you're 
right I could flip this.

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

No, PIN_MAPPABLE is only for legacy platform that used the aperture BAR 
for stolen mem access via GGTT. MTL doesn't have it and stolen is 
directly accessible via the LMEM BAR (which is actually the same BAR 2, 
but now behaves differently).

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

IIRC I used local because one of the GSC docs referred to it that way. I 
don't mind the renaming, but I don't think it should be done as part of 
this patch.

Daniele

>