From: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> To: John Harrison <john.c.harrison@intel.com>, <intel-gfx@lists.freedesktop.org> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/7] drm/i915/uc: use different ggtt pin offsets for uc loads Date: Fri, 30 Sep 2022 16:42:16 -0700 [thread overview] Message-ID: <31387d98-9e17-3998-6d38-ad3b5115f2f3@intel.com> (raw) In-Reply-To: <28a2896a-f240-e7b7-a127-b2782164ff58@intel.com> On 9/30/2022 4:24 PM, John Harrison wrote: > > > On 9/22/2022 15:11, Daniele Ceraolo Spurio wrote: >> Our current FW loading process is the same for all FWs: >> >> - Pin FW to GGTT at the start of the ggtt->uc_fw node >> - Load the FW >> - Unpin >> >> This worked because we didn't have a case where 2 FWs would be loaded on >> the same GGTT at the same time. On MTL, however, this can happend if >> both > The point being that the mapping is done using a single 'dummy' vma > that can't be mapped to two different places at the same time? But > isn't there a separate dummy object per uc instance. So there would be > one for the GuC on the render GT and another for the GuC on the media > GT. So each would be mapped separately to it's own unique address and > there is no conflict? Or what am I missing? The issue is that without this patch all the dummy vmas are inserted at the same address (start of the reserved node), which only works if they can't be mapped at the same time. Note that we don't use the generic vm functions to insert the dummy vma and we instead specify the exact offset we want it mapped at. This patch makes it so that each dummy vma has its own private offset. > >> GTs are reset at the same time, so we can't pin everything in the same >> spot and we need to use separate offset. For simplicity, instead of >> calculating the exact required size, we reserve a 2MB slot for each fw. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: John Harrison <john.c.harrison@intel.com> >> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> index b91ad4aede1f..d6ca772e9f4b 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -666,16 +666,33 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) >> return err; >> } >> +/* >> + * The reserved GGTT space is ~18 MBs. All our blobs are well below >> 1MB, so for >> + * safety we reserve 2MB each. >> + */ >> +#define INTEL_UC_RSVD_GGTT_PER_FW SZ_2M >> static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw) >> { >> - struct i915_ggtt *ggtt = __uc_fw_to_gt(uc_fw)->ggtt; >> + struct intel_gt *gt = __uc_fw_to_gt(uc_fw); >> + struct i915_ggtt *ggtt = gt->ggtt; >> struct drm_mm_node *node = &ggtt->uc_fw; >> + u32 offset = uc_fw->type * INTEL_UC_RSVD_GGTT_PER_FW; >> + >> + /* >> + * To keep the math simple, we use 8MB for the root tile and 8MB >> for >> + * the media one. >> + */ >> + BUILD_BUG_ON(INTEL_UC_FW_NUM_TYPES * INTEL_UC_RSVD_GGTT_PER_FW > >> SZ_8M); > Doesn't this need to be >= ? No, this is a size check and 8MB is fine for a size. > >> + if (gt->type == GT_MEDIA) >> + offset += SZ_8M; >> GEM_BUG_ON(!drm_mm_node_allocated(node)); >> GEM_BUG_ON(upper_32_bits(node->start)); >> GEM_BUG_ON(upper_32_bits(node->start + node->size - 1)); >> + GEM_BUG_ON(offset + uc_fw->obj->base.size > node->size); >> + GEM_BUG_ON(uc_fw->obj->base.size > INTEL_UC_RSVD_GGTT_PER_FW); > Given that the firmware blob is loaded from the disk and therefore > under user control, is a BUG_ON appropriate? Although there doesn't > look to be any obvious way to abort at this point. Could the size > check be moved to where we actually load the firmware rather than > where it is being mapped? My idea was that we wouldn't release a blob that violates this without updating the kernel first, so the only way a user can violate this is if they try to load a bogus file. But I can still move this check to fetch time and fail the fetch if the size is too big, so we can fall-back to something sensible. > >> - return lower_32_bits(node->start); >> + return lower_32_bits(node->start + offset); >> } >> static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw) >> @@ -690,7 +707,6 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw >> *uc_fw) >> dummy->bi.pages = obj->mm.pages; >> GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj)); >> - GEM_BUG_ON(dummy->node_size > ggtt->uc_fw.size); > Why remove this? The new GEM_BUG_ONs in the function above perform a more strict test, so this is redundant. Daniele > > John. > >> /* uc_fw->obj cache domains were not controlled across >> suspend */ >> if (i915_gem_object_has_struct_page(obj)) >
WARNING: multiple messages have this Message-ID (diff)
From: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> To: John Harrison <john.c.harrison@intel.com>, <intel-gfx@lists.freedesktop.org> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 3/7] drm/i915/uc: use different ggtt pin offsets for uc loads Date: Fri, 30 Sep 2022 16:42:16 -0700 [thread overview] Message-ID: <31387d98-9e17-3998-6d38-ad3b5115f2f3@intel.com> (raw) In-Reply-To: <28a2896a-f240-e7b7-a127-b2782164ff58@intel.com> On 9/30/2022 4:24 PM, John Harrison wrote: > > > On 9/22/2022 15:11, Daniele Ceraolo Spurio wrote: >> Our current FW loading process is the same for all FWs: >> >> - Pin FW to GGTT at the start of the ggtt->uc_fw node >> - Load the FW >> - Unpin >> >> This worked because we didn't have a case where 2 FWs would be loaded on >> the same GGTT at the same time. On MTL, however, this can happend if >> both > The point being that the mapping is done using a single 'dummy' vma > that can't be mapped to two different places at the same time? But > isn't there a separate dummy object per uc instance. So there would be > one for the GuC on the render GT and another for the GuC on the media > GT. So each would be mapped separately to it's own unique address and > there is no conflict? Or what am I missing? The issue is that without this patch all the dummy vmas are inserted at the same address (start of the reserved node), which only works if they can't be mapped at the same time. Note that we don't use the generic vm functions to insert the dummy vma and we instead specify the exact offset we want it mapped at. This patch makes it so that each dummy vma has its own private offset. > >> GTs are reset at the same time, so we can't pin everything in the same >> spot and we need to use separate offset. For simplicity, instead of >> calculating the exact required size, we reserve a 2MB slot for each fw. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: John Harrison <john.c.harrison@intel.com> >> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> index b91ad4aede1f..d6ca772e9f4b 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -666,16 +666,33 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) >> return err; >> } >> +/* >> + * The reserved GGTT space is ~18 MBs. All our blobs are well below >> 1MB, so for >> + * safety we reserve 2MB each. >> + */ >> +#define INTEL_UC_RSVD_GGTT_PER_FW SZ_2M >> static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw) >> { >> - struct i915_ggtt *ggtt = __uc_fw_to_gt(uc_fw)->ggtt; >> + struct intel_gt *gt = __uc_fw_to_gt(uc_fw); >> + struct i915_ggtt *ggtt = gt->ggtt; >> struct drm_mm_node *node = &ggtt->uc_fw; >> + u32 offset = uc_fw->type * INTEL_UC_RSVD_GGTT_PER_FW; >> + >> + /* >> + * To keep the math simple, we use 8MB for the root tile and 8MB >> for >> + * the media one. >> + */ >> + BUILD_BUG_ON(INTEL_UC_FW_NUM_TYPES * INTEL_UC_RSVD_GGTT_PER_FW > >> SZ_8M); > Doesn't this need to be >= ? No, this is a size check and 8MB is fine for a size. > >> + if (gt->type == GT_MEDIA) >> + offset += SZ_8M; >> GEM_BUG_ON(!drm_mm_node_allocated(node)); >> GEM_BUG_ON(upper_32_bits(node->start)); >> GEM_BUG_ON(upper_32_bits(node->start + node->size - 1)); >> + GEM_BUG_ON(offset + uc_fw->obj->base.size > node->size); >> + GEM_BUG_ON(uc_fw->obj->base.size > INTEL_UC_RSVD_GGTT_PER_FW); > Given that the firmware blob is loaded from the disk and therefore > under user control, is a BUG_ON appropriate? Although there doesn't > look to be any obvious way to abort at this point. Could the size > check be moved to where we actually load the firmware rather than > where it is being mapped? My idea was that we wouldn't release a blob that violates this without updating the kernel first, so the only way a user can violate this is if they try to load a bogus file. But I can still move this check to fetch time and fail the fetch if the size is too big, so we can fall-back to something sensible. > >> - return lower_32_bits(node->start); >> + return lower_32_bits(node->start + offset); >> } >> static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw) >> @@ -690,7 +707,6 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw >> *uc_fw) >> dummy->bi.pages = obj->mm.pages; >> GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj)); >> - GEM_BUG_ON(dummy->node_size > ggtt->uc_fw.size); > Why remove this? The new GEM_BUG_ONs in the function above perform a more strict test, so this is redundant. Daniele > > John. > >> /* uc_fw->obj cache domains were not controlled across >> suspend */ >> if (i915_gem_object_has_struct_page(obj)) >
next prev parent reply other threads:[~2022-09-30 23:42 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-22 22:11 [Intel-gfx] [PATCH 0/7] drm/i915: prepare for uC loading on MTL Daniele Ceraolo Spurio 2022-09-22 22:11 ` Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] [PATCH 1/7] drm/i915/huc: only load HuC on GTs that have VCS engines Daniele Ceraolo Spurio 2022-09-22 22:11 ` Daniele Ceraolo Spurio 2022-09-23 10:53 ` [Intel-gfx] " Tvrtko Ursulin 2022-09-23 15:41 ` Ceraolo Spurio, Daniele 2022-09-26 16:15 ` Tvrtko Ursulin 2022-09-26 16:28 ` Ceraolo Spurio, Daniele 2022-09-27 9:58 ` Iddamsetty, Aravind 2022-09-27 9:58 ` [Intel-gfx] " Iddamsetty, Aravind 2022-09-22 22:11 ` [PATCH 2/7] drm/i915/uc: fetch uc firmwares for each GT Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-09-30 22:49 ` John Harrison 2022-09-30 22:49 ` [Intel-gfx] " John Harrison 2022-09-22 22:11 ` [PATCH 3/7] drm/i915/uc: use different ggtt pin offsets for uc loads Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-09-30 23:24 ` John Harrison 2022-09-30 23:24 ` [Intel-gfx] " John Harrison 2022-09-30 23:42 ` Ceraolo Spurio, Daniele [this message] 2022-09-30 23:42 ` Ceraolo Spurio, Daniele 2022-10-10 19:18 ` John Harrison 2022-10-10 19:18 ` [Intel-gfx] " John Harrison 2022-09-22 22:11 ` [PATCH 4/7] drm/i915/guc: Add GuC deprivilege feature to MTL Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-09-30 23:33 ` John Harrison 2022-09-30 23:33 ` [Intel-gfx] " John Harrison 2022-09-22 22:11 ` [PATCH 5/7] drm/i915/mtl: Handle wopcm per-GT and limit calculations Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-09-23 9:24 ` Jani Nikula 2022-09-23 15:34 ` Ceraolo Spurio, Daniele 2022-09-22 22:11 ` [PATCH 6/7] drm/i915/guc: define media GT GuC send regs Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-10-01 0:26 ` John Harrison 2022-10-01 0:26 ` [Intel-gfx] " John Harrison 2022-09-22 22:11 ` [PATCH 7/7] drm/i915/guc: handle interrupts from media GuC Daniele Ceraolo Spurio 2022-09-22 22:11 ` [Intel-gfx] " Daniele Ceraolo Spurio 2022-09-28 0:10 ` Matt Roper 2022-09-28 0:10 ` [Intel-gfx] " Matt Roper 2022-09-28 0:22 ` Ceraolo Spurio, Daniele 2022-09-28 0:22 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2022-09-28 18:07 ` Matt Roper 2022-09-28 18:07 ` [Intel-gfx] " Matt Roper 2022-09-28 18:25 ` Ceraolo Spurio, Daniele 2022-09-28 18:25 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2022-09-23 1:12 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: prepare for uC loading on MTL Patchwork 2022-09-23 1:12 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2022-09-23 1:31 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-09-23 9:24 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=31387d98-9e17-3998-6d38-ad3b5115f2f3@intel.com \ --to=daniele.ceraolospurio@intel.com \ --cc=alan.previn.teres.alexis@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=john.c.harrison@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.