| Message ID | 20230505160415.889525-3-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: GSC FW support for MTL | expand |
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)
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 >
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.
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index f46eb17a7a98..4814901fac9e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -3,6 +3,7 @@ * Copyright © 2022 Intel Corporation */ +#include "gem/i915_gem_lmem.h" #include "gt/intel_engine_pm.h" #include "gt/intel_gpu_commands.h" #include "gt/intel_gt.h" @@ -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; if (!gsc->local) return -ENODEV; - obj = gsc->local->obj; - - if (obj->base.size < gsc->fw.size) + if (gsc->local->size < gsc->fw.size) return -ENOSPC; - /* - * Wa_22016122933: For MTL the shared memory needs to be mapped - * as WC on CPU side and UC (PAT index 2) on GPU side - */ - if (IS_METEORLAKE(i915)) - i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE); - - dst = i915_gem_object_pin_map_unlocked(obj, - i915_coherent_map_type(i915, obj, true)); - if (IS_ERR(dst)) - return PTR_ERR(dst); - src = i915_gem_object_pin_map_unlocked(gsc->fw.obj, i915_coherent_map_type(i915, gsc->fw.obj, true)); - if (IS_ERR(src)) { - i915_gem_object_unpin_map(obj); + if (IS_ERR(src)) return PTR_ERR(src); - } - 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); /* * Wa_22016122933: Making sure the data in dst is @@ -149,7 +133,6 @@ static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc) intel_guc_write_barrier(>->uc.guc); i915_gem_object_unpin_map(gsc->fw.obj); - i915_gem_object_unpin_map(obj); return 0; } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c index bbf21b93fc8d..2ae693b01b49 100644 --- 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) +{ + struct intel_gt *gt = gsc_uc_to_gt(gsc); + struct drm_i915_gem_object *obj; + struct i915_vma *vma; + void __iomem *vaddr; + int ret = 0; + + /* + * The GSC FW doesn't immediately suspend after becoming idle, so there + * is a chance that it could still be awake after we successfully + * return from the pci suspend function, even if there are no pending + * operations. + * The FW might therefore try to access memory for its suspend operation + * after the kernel has completed the HW suspend flow; this can cause + * issues if the FW is mapped in normal RAM memory, as some of the + * involved HW units might've already lost power. + * The driver must therefore avoid this situation and the recommended + * way to do so is to use stolen memory for the GSC memory allocation, + * because stolen memory takes a different path in HW and it is + * guaranteed to always work as long as the GPU itself is awake (which + * it must be if the GSC is awake). + */ + 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); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + goto err; + } + + vaddr = i915_vma_pin_iomap(vma); + i915_vma_unpin(vma); + if (IS_ERR(vaddr)) { + ret = PTR_ERR(vaddr); + goto err; + } + + i915_vma_make_unshrinkable(vma); + + gsc->local = vma; + gsc->local_vaddr = vaddr; + + return 0; + +err: + i915_gem_object_put(obj); + return ret; +} + +static void gsc_unmap_and_free_vma(struct intel_gsc_uc *gsc) +{ + struct i915_vma *vma = fetch_and_zero(&gsc->local); + + if (!vma) + return; + + gsc->local_vaddr = NULL; + i915_vma_unpin_iomap(vma); + i915_gem_object_put(vma->obj); +} + int intel_gsc_uc_init(struct intel_gsc_uc *gsc) { static struct lock_class_key gsc_lock; struct intel_gt *gt = gsc_uc_to_gt(gsc); struct intel_engine_cs *engine = gt->engine[GSC0]; struct intel_context *ce; - struct i915_vma *vma; int err; err = intel_uc_fw_init(&gsc->fw); if (err) goto out; - vma = intel_guc_allocate_vma(>->uc.guc, SZ_8M); - if (IS_ERR(vma)) { - err = PTR_ERR(vma); + err = gsc_allocate_and_map_vma(gsc, SZ_4M); + if (err) goto out_fw; - } - - gsc->local = vma; ce = intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_4K, I915_GEM_HWS_GSC_ADDR, @@ -170,7 +229,7 @@ int intel_gsc_uc_init(struct intel_gsc_uc *gsc) return 0; out_vma: - i915_vma_unpin_and_release(&gsc->local, 0); + gsc_unmap_and_free_vma(gsc); out_fw: intel_uc_fw_fini(&gsc->fw); out: @@ -194,7 +253,7 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc) if (gsc->ce) intel_engine_destroy_pinned_context(fetch_and_zero(&gsc->ce)); - i915_vma_unpin_and_release(&gsc->local, 0); + gsc_unmap_and_free_vma(gsc); intel_uc_fw_fini(&gsc->fw); } 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 */ struct intel_context *ce; /* for submission to GSC FW via GSC engine */ /* for delayed load and proxy handling */
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: 1 - Switch the memory allocation to stolen memory. We need to avoid accesses from GSC FW to normal memory after the suspend function has completed and to do so we can either switch to using stolen or make sure the GSC is gone to sleep before the end of the suspend function. Given that the GSC waits for a bit before going idle even if there are no pending operations, it is easier and quicker to just use stolen memory. 2 - Reduce the GSC allocation size to 4MBs, which is the POR requirement. The 8MBs were needed only for early FW and I had misunderstood that as being the expected POR size when I sent the original patch. 3 - Perma-map the GSC allocation. This isn't required immediately, but it will be needed later to be able to quickly extract the GSC logs, which are inside the allocation. Since the mapping code needs to be rewritten due to switching to stolen, it makes sense to do the switch immediately to avoid having to change it again later. Note that the explicit setting of CACHE_NONE for Wa_22016122933 has been dropped because that's the default setting for stolen memory on !LLC platforms. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 29 ++------- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 77 ++++++++++++++++++++--- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 1 + 3 files changed, 75 insertions(+), 32 deletions(-)