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

[Teres Alexis, Alan Previn]

On Tue, 2023-05-23 at 08:21 -0700, Ceraolo Spurio, Daniele wrote:
> 
> > 
> > > +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size)
> > alan:snip
> > > +	obj = i915_gem_object_create_stolen(gt->i915, s0ize);
> > > +	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: thanks - sounds good  - i forgot about those differentiations

> 
> > > +++ 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.
alan: sure - sounds good.