| Message ID | 20230329165658.2686549-5-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add support for MTL GSC SW Proxy | expand |
On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote: > The GSC notifies us of a proxy request via the HECI2 interrupt. The alan:snip > @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > u32 irqs = GT_RENDER_USER_INTERRUPT; > u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0; > u32 gsc_mask = 0; > + u32 heci_mask = 0; alan: nit: should we call this heci2_mask - uniquely identifiable from legacy. alan:snip > - else if (HAS_HECI_GSC(gt->i915)) > + heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/ alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2? i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14? alan:snip > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index 4aecb5a7b631..da11ce5ca99e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1577,6 +1577,7 @@ > > #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) > #define GEN11_CSME (31) > +#define GEN12_HECI_2 (30) alan: I dont see this being used anywhere - should remove this. > +#define GEN11_HECI2_RSVD_INTR_MASK _MMIO(0x1900e4) alan: GEN11? don't you mean GEN12? > +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir) > +{ > + struct intel_gt *gt = gsc_uc_to_gt(gsc); > + > + if (unlikely(!iir)) > + return; > + > + lockdep_assert_held(gt->irq_lock); > + > + if (!gsc->proxy.component) { > + gt_err(gt, "GSC proxy irq received without the component being bound!\n"); alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we know that proxy runtime irqs could happen anytime (depending on other gpu-security-related system interactions). However, would the component driver be bound/unbound through a suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request came too early after a resume cycle. If yes, then should we not do somethign here to ensure that when the component gets bound later, we know there is something waiting to be processed? (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger the worker from there, i.e. the bind func?) alan:snip > static int i915_gsc_proxy_component_bind(struct device *i915_kdev, > struct device *tee_kdev, void *data) > { > struct drm_i915_private *i915 = kdev_to_i915(i915_kdev); > - struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc; > + struct intel_gt *gt = i915->media_gt; > + struct intel_gsc_uc *gsc = >->uc.gsc; > + intel_wakeref_t wakeref; > + > + /* enable HECI2 IRQs */ > + with_intel_runtime_pm(&i915->runtime_pm, wakeref) > + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE), > + 0, HECI_H_CSR_IE); alan: i notice we don't seem to care about potentially re-writing a '1' into reset if it was midst reset when we read. Shouldn't we also protect against that here? 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 023bded10dde..a2a0813b8a76 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > @@ -23,6 +23,9 @@ struct intel_gsc_uc { > /* for delayed load and proxy handling */ > struct workqueue_struct *wq; > struct work_struct work; > + u32 gsc_work_actions; /* protected by gt->irq_lock */ > +#define GSC_ACTION_FW_LOAD BIT(0) > +#define GSC_ACTION_SW_PROXY BIT(1 > alan: perhaps its time to have a substruct for "proxy_worker" and include 'wq' and 'work' in additional to actions? > ) > > struct { > struct i915_gsc_proxy_component *component;
On 4/20/2023 11:49 AM, Teres Alexis, Alan Previn wrote: > On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote: >> The GSC notifies us of a proxy request via the HECI2 interrupt. The > alan:snip > >> @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) >> u32 irqs = GT_RENDER_USER_INTERRUPT; >> u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0; >> u32 gsc_mask = 0; >> + u32 heci_mask = 0; > alan: nit: should we call this heci2_mask - uniquely identifiable from legacy. The mask is theoretically not just for HECI2, the bit we set in it is. If future platforms add back the HECI1 interrupt then it'd go in the same mask. > > > alan:snip > >> - else if (HAS_HECI_GSC(gt->i915)) >> + heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/ > alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2? > i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14? GSC_IRQ_INTF(1) is the HECI2 interrupt on DG2 and the bit has remained the same exactly to allow SW to re-use the same code/defines, so IMO we should do so. > > alan:snip > > > >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> index 4aecb5a7b631..da11ce5ca99e 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> @@ -1577,6 +1577,7 @@ >> >> #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) >> #define GEN11_CSME (31) >> +#define GEN12_HECI_2 (30) > alan: I dont see this being used anywhere - should remove this. A few of the defines for this register here are not used. I've added this one in as a way of documenting where the bit was, but I can remove it if you think it's unneeded. > >> +#define GEN11_HECI2_RSVD_INTR_MASK _MMIO(0x1900e4) > alan: GEN11? don't you mean GEN12? > Yup, this should be GEN12 > > > >> +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir) >> +{ >> + struct intel_gt *gt = gsc_uc_to_gt(gsc); >> + >> + if (unlikely(!iir)) >> + return; >> + >> + lockdep_assert_held(gt->irq_lock); >> + >> + if (!gsc->proxy.component) { >> + gt_err(gt, "GSC proxy irq received without the component being bound!\n"); > alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we > know that proxy runtime irqs could happen anytime (depending on other gpu-security-related > system interactions). However, would the component driver be bound/unbound through a > suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request > came too early after a resume cycle. If yes, then should we not do somethign here to ensure that > when the component gets bound later, we know there is something waiting to be processed? > (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger > the worker from there, i.e. the bind func?) A proxy request can only be triggered in response to a userspace ask, which in turn can only happen once we've completed the resume flow and the component is re-bound. Therefore, we should never have a situation where we get a proxy interrupt without the component being bound. > > alan:snip > >> static int i915_gsc_proxy_component_bind(struct device *i915_kdev, >> struct device *tee_kdev, void *data) >> { >> struct drm_i915_private *i915 = kdev_to_i915(i915_kdev); >> - struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc; >> + struct intel_gt *gt = i915->media_gt; >> + struct intel_gsc_uc *gsc = >->uc.gsc; >> + intel_wakeref_t wakeref; >> + >> + /* enable HECI2 IRQs */ >> + with_intel_runtime_pm(&i915->runtime_pm, wakeref) >> + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE), >> + 0, HECI_H_CSR_IE); > alan: i notice we don't seem to care about potentially re-writing a '1' into reset > if it was midst reset when we read. Shouldn't we also protect against that here? Yeah, I'll add that in > > 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 023bded10dde..a2a0813b8a76 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> @@ -23,6 +23,9 @@ struct intel_gsc_uc { >> /* for delayed load and proxy handling */ >> struct workqueue_struct *wq; >> struct work_struct work; >> + u32 gsc_work_actions; /* protected by gt->irq_lock */ >> +#define GSC_ACTION_FW_LOAD BIT(0) >> +#define GSC_ACTION_SW_PROXY BIT(1 >> > alan: perhaps its time to have a substruct for "proxy_worker" and include > 'wq' and 'work' in additional to actions? The worker is not just for proxy, we use it for a GSC and HuC loading as well. It's the main way we handle the gsc_uc, so IMO it's better off staying in the main struct. However, if you still think a substructure will make things cleaner I can add it in, but please suggest a name because I have no idea what to call it (something like handler?). Daniele >> ) >> >> struct { >> struct i915_gsc_proxy_component *component;
I think we are also bottom-ing on the opens fo this patch too: On Thu, 2023-04-20 at 13:21 -0700, Ceraolo Spurio, Daniele wrote: > On 4/20/2023 11:49 AM, Teres Alexis, Alan Previn wrote: > > On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote: > > > The GSC notifies us of a proxy request via the HECI2 interrupt. The > > alan:snip > > > > > @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > > > u32 irqs = GT_RENDER_USER_INTERRUPT; > > > u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0; > > > u32 gsc_mask = 0; > > > + u32 heci_mask = 0; > > alan: nit: should we call this heci2_mask - uniquely identifiable from legacy. > > The mask is theoretically not just for HECI2, the bit we set in it is. > If future platforms add back the HECI1 interrupt then it'd go in the > same mask. alan: yeah - im good with that - no change needed then - was a nit anyways. > > alan:snip > > > > > - else if (HAS_HECI_GSC(gt->i915)) > > > + heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/ > > alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2? > > i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14? > > GSC_IRQ_INTF(1) is the HECI2 interrupt on DG2 and the bit has remained > the same exactly to allow SW to re-use the same code/defines, so IMO we > should do so. alan: okay sure - sounds good. > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > > index 4aecb5a7b631..da11ce5ca99e 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > > > @@ -1577,6 +1577,7 @@ > > > > > > #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) > > > #define GEN11_CSME (31) > > > +#define GEN12_HECI_2 (30) > > alan: I dont see this being used anywhere - should remove this. > > A few of the defines for this register here are not used. I've added > this one in as a way of documenting where the bit was, but I can remove > it if you think it's unneeded. alan: Oh i actually didnt notice that earlier - in that case, please keep it would be nice to add a comment for all of those such bits (consider this a nit). > > > > > +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir) > > > +{ > > > + struct intel_gt *gt = gsc_uc_to_gt(gsc); > > > + > > > + if (unlikely(!iir)) > > > + return; > > > + > > > + lockdep_assert_held(gt->irq_lock); > > > + > > > + if (!gsc->proxy.component) { > > > + gt_err(gt, "GSC proxy irq received without the component being bound!\n"); > > alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we > > know that proxy runtime irqs could happen anytime (depending on other gpu-security-related > > system interactions). However, would the component driver be bound/unbound through a > > suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request > > came too early after a resume cycle. If yes, then should we not do somethign here to ensure that > > when the component gets bound later, we know there is something waiting to be processed? > > (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger > > the worker from there, i.e. the bind func?) > > A proxy request can only be triggered in response to a userspace ask, > which in turn can only happen once we've completed the resume flow and > the component is re-bound. Therefore, we should never have a situation > where we get a proxy interrupt without the component being bound. alan: thanks -my bad. > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > > > @@ -23,6 +23,9 @@ struct intel_gsc_uc { > > > /* for delayed load and proxy handling */ > > > struct workqueue_struct *wq; > > > struct work_struct work; > > > + u32 gsc_work_actions; /* protected by gt->irq_lock */ > > > +#define GSC_ACTION_FW_LOAD BIT(0) > > > +#define GSC_ACTION_SW_PROXY BIT(1 > > > > > alan: perhaps its time to have a substruct for "proxy_worker" and include > > 'wq' and 'work' in additional to actions? > > The worker is not just for proxy, we use it for a GSC and HuC loading as > well. It's the main way we handle the gsc_uc, so IMO it's better off > staying in the main struct. However, if you still think a substructure > will make things cleaner I can add it in, but please suggest a name > because I have no idea what to call it (something like handler?). alan: i was thinking "task_handler" - but since its not specific to proxy, then let's not change it.
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c index 1b25a6039152..c433ffdb3380 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c @@ -15,6 +15,7 @@ #include "intel_uncore.h" #include "intel_rps.h" #include "pxp/intel_pxp_irq.h" +#include "uc/intel_gsc_proxy.h" static void guc_irq_handler(struct intel_guc *guc, u16 iir) { @@ -81,6 +82,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance, if (instance == OTHER_GSC_INSTANCE) return intel_gsc_irq_handler(gt, iir); + if (instance == OTHER_GSC_HECI_2_INSTANCE) + return intel_gsc_proxy_irq_handler(>->uc.gsc, iir); + WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n", instance, iir); } @@ -100,6 +104,8 @@ static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance) case VIDEO_ENHANCEMENT_CLASS: return media_gt; case OTHER_CLASS: + if (instance == OTHER_GSC_HECI_2_INSTANCE) + return media_gt; if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0)) return media_gt; fallthrough; @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) u32 irqs = GT_RENDER_USER_INTERRUPT; u32 guc_mask = intel_uc_wants_guc(>->uc) ? GUC_INTR_GUC2HOST : 0; u32 gsc_mask = 0; + u32 heci_mask = 0; u32 dmask; u32 smask; @@ -267,10 +274,16 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) dmask = irqs << 16 | irqs; smask = irqs << 16; - if (HAS_ENGINE(gt, GSC0)) + if (HAS_ENGINE(gt, GSC0)) { + /* + * the heci2 interrupt is enabled via the same register as the + * GSC interrupt, but it has its own mask register. + */ gsc_mask = irqs; - else if (HAS_HECI_GSC(gt->i915)) + heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/ + } else if (HAS_HECI_GSC(gt->i915)) { gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); + } BUILD_BUG_ON(irqs & 0xffff0000); @@ -280,7 +293,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) if (CCS_MASK(gt)) intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask); if (gsc_mask) - intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask); + intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask | heci_mask); /* Unmask irqs on RCS, BCS, VCS and VECS engines. */ intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask); @@ -308,6 +321,9 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask); if (gsc_mask) intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask); + if (heci_mask) + intel_uncore_write(uncore, GEN11_HECI2_RSVD_INTR_MASK, + ~REG_FIELD_PREP(ENGINE1_MASK, heci_mask)); if (guc_mask) { /* the enable bit is common for both GTs but the masks are separate */ diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 4aecb5a7b631..da11ce5ca99e 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1577,6 +1577,7 @@ #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) #define GEN11_CSME (31) +#define GEN12_HECI_2 (30) #define GEN11_GUNIT (28) #define GEN11_GUC (25) #define MTL_MGUC (24) @@ -1618,6 +1619,7 @@ /* irq instances for OTHER_CLASS */ #define OTHER_GUC_INSTANCE 0 #define OTHER_GTPM_INSTANCE 1 +#define OTHER_GSC_HECI_2_INSTANCE 3 #define OTHER_KCR_INSTANCE 4 #define OTHER_GSC_INSTANCE 6 #define OTHER_MEDIA_GUC_INSTANCE 16 @@ -1633,6 +1635,7 @@ #define GEN12_VCS6_VCS7_INTR_MASK _MMIO(0x1900b4) #define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) #define GEN12_VECS2_VECS3_INTR_MASK _MMIO(0x1900d4) +#define GEN11_HECI2_RSVD_INTR_MASK _MMIO(0x1900e4) #define GEN11_GUC_SG_INTR_MASK _MMIO(0x1900e8) #define MTL_GUC_MGUC_INTR_MASK _MMIO(0x1900e8) /* MTL+ */ #define GEN11_GPM_WGBOXPERF_INTR_MASK _MMIO(0x1900ec) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c index ed8f68e78c26..2889e0d39ff3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c @@ -14,6 +14,7 @@ #include "intel_gsc_uc.h" #include "intel_gsc_uc_heci_cmd_submit.h" #include "i915_drv.h" +#include "i915_reg.h" /* * GSC proxy: @@ -273,17 +274,49 @@ int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc) gt_err(gt, "GSC proxy worker called without the component being bound!\n"); err = -EIO; } else { + /* + * write the status bit to clear it and allow new proxy + * interrupts to be generated while we handle the current + * request, but be sure not to write the reset bit + */ + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE), + HECI_H_CSR_RST, HECI_H_CSR_IS); err = proxy_query(gsc); } mutex_unlock(&gsc->proxy.mutex); return err; } +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir) +{ + struct intel_gt *gt = gsc_uc_to_gt(gsc); + + if (unlikely(!iir)) + return; + + lockdep_assert_held(gt->irq_lock); + + if (!gsc->proxy.component) { + gt_err(gt, "GSC proxy irq received without the component being bound!\n"); + return; + } + + gsc->gsc_work_actions |= GSC_ACTION_SW_PROXY; + queue_work(gsc->wq, &gsc->work); +} + static int i915_gsc_proxy_component_bind(struct device *i915_kdev, struct device *tee_kdev, void *data) { struct drm_i915_private *i915 = kdev_to_i915(i915_kdev); - struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc; + struct intel_gt *gt = i915->media_gt; + struct intel_gsc_uc *gsc = >->uc.gsc; + intel_wakeref_t wakeref; + + /* enable HECI2 IRQs */ + with_intel_runtime_pm(&i915->runtime_pm, wakeref) + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE), + 0, HECI_H_CSR_IE); mutex_lock(&gsc->proxy.mutex); gsc->proxy.component = data; @@ -297,11 +330,18 @@ static void i915_gsc_proxy_component_unbind(struct device *i915_kdev, struct device *tee_kdev, void *data) { struct drm_i915_private *i915 = kdev_to_i915(i915_kdev); - struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc; + struct intel_gt *gt = i915->media_gt; + struct intel_gsc_uc *gsc = >->uc.gsc; + intel_wakeref_t wakeref; mutex_lock(&gsc->proxy.mutex); gsc->proxy.component = NULL; mutex_unlock(&gsc->proxy.mutex); + + /* disable HECI2 IRQs */ + with_intel_runtime_pm(&i915->runtime_pm, wakeref) + intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE), + HECI_H_CSR_IE, 0); } static const struct component_ops i915_gsc_proxy_component_ops = { diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h index da3e9dd5d820..c55bafcbaec4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h @@ -13,5 +13,6 @@ struct intel_gsc_uc; int intel_gsc_proxy_init(struct intel_gsc_uc *gsc); void intel_gsc_proxy_fini(struct intel_gsc_uc *gsc); int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc); +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir); #endif 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 b505b208f04b..64bff01026e8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c @@ -17,20 +17,40 @@ static void gsc_work(struct work_struct *work) struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work); struct intel_gt *gt = gsc_uc_to_gt(gsc); intel_wakeref_t wakeref; + u32 actions; int ret; wakeref = intel_runtime_pm_get(gt->uncore->rpm); - ret = intel_gsc_uc_fw_upload(gsc); - if (ret) - goto out_put; - - ret = intel_gsc_proxy_request_handler(gsc); - if (ret) - goto out_put; + spin_lock_irq(gt->irq_lock); + actions = gsc->gsc_work_actions; + gsc->gsc_work_actions = 0; + spin_unlock_irq(gt->irq_lock); + + if (actions & GSC_ACTION_FW_LOAD) { + ret = intel_gsc_uc_fw_upload(gsc); + if (ret == -EEXIST) /* skip proxy if not a new load */ + actions &= ~GSC_ACTION_FW_LOAD; + else if (ret) + goto out_put; + } - gt_dbg(gt, "GSC Proxy initialized\n"); - intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING); + if (actions & (GSC_ACTION_FW_LOAD | GSC_ACTION_SW_PROXY)) { + if (!intel_gsc_uc_fw_init_done(gsc)) { + gt_err(gt, "Proxy request received with GSC not loaded!\n"); + goto out_put; + } + + ret = intel_gsc_proxy_request_handler(gsc); + if (ret) + goto out_put; + + /* mark the GSC FW init as done the first time we run this */ + if (actions & GSC_ACTION_FW_LOAD) { + gt_dbg(gt, "GSC Proxy initialized\n"); + intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING); + } + } out_put: intel_runtime_pm_put(gt->uncore->rpm, wakeref); @@ -177,11 +197,17 @@ void intel_gsc_uc_resume(struct intel_gsc_uc *gsc) void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc) { + struct intel_gt *gt = gsc_uc_to_gt(gsc); + if (!intel_uc_fw_is_loadable(&gsc->fw)) return; if (intel_gsc_uc_fw_init_done(gsc)) return; + spin_lock_irq(gt->irq_lock); + gsc->gsc_work_actions |= GSC_ACTION_FW_LOAD; + spin_unlock_irq(gt->irq_lock); + queue_work(gsc->wq, &gsc->work); } 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 023bded10dde..a2a0813b8a76 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h @@ -23,6 +23,9 @@ struct intel_gsc_uc { /* for delayed load and proxy handling */ struct workqueue_struct *wq; struct work_struct work; + u32 gsc_work_actions; /* protected by gt->irq_lock */ +#define GSC_ACTION_FW_LOAD BIT(0) +#define GSC_ACTION_SW_PROXY BIT(1) struct { struct i915_gsc_proxy_component *component;
The GSC notifies us of a proxy request via the HECI2 interrupt. The interrupt must be enabled both in the HECI layer and in our usual gt irq programming; for the latter, the interrupt is enabled via the same enable register as the GSC CS, but it does have its own mask register. When the interrupt is received, we also need to de-assert it in both layers. The handling of the proxy request is deferred to the same worker that we use for GSC load. New flags have been added to distinguish between the init case and the proxy interrupt. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_irq.c | 22 ++++++++-- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 3 ++ drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c | 44 +++++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 44 ++++++++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 3 ++ 6 files changed, 103 insertions(+), 14 deletions(-)