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.
