| Message ID | 20221027221554.2638087-4-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the GSC CS | expand |
On Thu, Oct 27, 2022 at 03:15:52PM -0700, Daniele Ceraolo Spurio wrote: > The GSC CS re-uses the same interrupt bits that the GSC used in older > platforms. This means that we can now have an engine interrupt coming > out of OTHER_CLASS, so we need to handle that appropriately. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------ > 1 file changed, 43 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > index f26882fdc24c..34ff1ee7e931 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance, > instance, iir); > } > > -static void > -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, > - const u8 instance, const u16 iir) > +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance) > { > - struct intel_engine_cs *engine; > - > - /* > - * Platforms with standalone media have their media engines in another > - * GT. > - */ > - if (MEDIA_VER(gt->i915) >= 13 && > - (class == VIDEO_DECODE_CLASS || class == VIDEO_ENHANCEMENT_CLASS)) { > - if (!gt->i915->media_gt) > - goto err; > + struct intel_gt *media_gt = gt->i915->media_gt; > > - gt = gt->i915->media_gt; > + /* we expect the non-media gt to be passed in */ > + GEM_BUG_ON(gt == media_gt); > + > + if (!media_gt) > + return gt; > + > + switch (class) { > + case VIDEO_DECODE_CLASS: > + case VIDEO_ENHANCEMENT_CLASS: > + return media_gt; > + case OTHER_CLASS: > + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0)) > + return media_gt; > + fallthrough; > + default: > + return gt; > } > - > - if (instance <= MAX_ENGINE_INSTANCE) > - engine = gt->engine_class[class][instance]; > - else > - engine = NULL; > - > - if (likely(engine)) > - return intel_engine_cs_irq(engine, iir); > - > -err: > - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, instance=0x%x\n", > - class, instance); > } > > static void > @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, const u32 identity) > const u8 class = GEN11_INTR_ENGINE_CLASS(identity); > const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); > const u16 intr = GEN11_INTR_ENGINE_INTR(identity); > + struct intel_engine_cs *engine; > > if (unlikely(!intr)) > return; > > - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) > - return gen11_engine_irq_handler(gt, class, instance, intr); > + /* > + * Platforms with standalone media have the media and GSC engines in > + * another GT. > + */ > + gt = pick_gt(gt, class, instance); > + > + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) > + engine = gt->engine_class[class][instance]; > + else > + engine = NULL; > + > + if (engine) > + return intel_engine_cs_irq(engine, intr); > > if (class == OTHER_CLASS) > return gen11_other_irq_handler(gt, instance, intr); > @@ -206,7 +210,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0); > if (CCS_MASK(gt)) > intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, 0); > - if (HAS_HECI_GSC(gt->i915)) > + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, 0); > > /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */ > @@ -233,7 +237,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~0); > if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) > intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~0); > - if (HAS_HECI_GSC(gt->i915)) > + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~0); > > intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0); > @@ -249,7 +253,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > { > struct intel_uncore *uncore = gt->uncore; > u32 irqs = GT_RENDER_USER_INTERRUPT; > - const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); > + u32 gsc_mask = 0; > u32 dmask; > u32 smask; > > @@ -261,6 +265,11 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > dmask = irqs << 16 | irqs; > smask = irqs << 16; > > + if (HAS_ENGINE(gt, GSC0)) > + gsc_mask = irqs; > + else if (HAS_HECI_GSC(gt->i915)) > + gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); > + > BUILD_BUG_ON(irqs & 0xffff0000); > > /* Enable RCS, BCS, VCS and VECS class interrupts. */ > @@ -268,9 +277,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask); > if (CCS_MASK(gt)) > intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask); > - if (HAS_HECI_GSC(gt->i915)) > - intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, > - gsc_mask); > + if (gsc_mask) > + intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask); > > /* Unmask irqs on RCS, BCS, VCS and VECS engines. */ > intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask); > @@ -296,7 +304,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~dmask); > if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) > intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask); > - if (HAS_HECI_GSC(gt->i915)) > + if (gsc_mask) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask); > > /* > -- > 2.37.3 >
On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote: > The GSC CS re-uses the same interrupt bits that the GSC used in older > platforms. This means that we can now have an engine interrupt coming > out of OTHER_CLASS, so we need to handle that appropriately. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------ > 1 file changed, 43 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > index f26882fdc24c..34ff1ee7e931 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c > @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance, > instance, iir); > } > > -static void > -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, > - const u8 instance, const u16 iir) > +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance) > { > - struct intel_engine_cs *engine; > - > - /* > - * Platforms with standalone media have their media engines in another > - * GT. > - */ > - if (MEDIA_VER(gt->i915) >= 13 && > - (class == VIDEO_DECODE_CLASS || class == VIDEO_ENHANCEMENT_CLASS)) { > - if (!gt->i915->media_gt) > - goto err; > + struct intel_gt *media_gt = gt->i915->media_gt; > > - gt = gt->i915->media_gt; > + /* we expect the non-media gt to be passed in */ > + GEM_BUG_ON(gt == media_gt); > + > + if (!media_gt) > + return gt; > + > + switch (class) { > + case VIDEO_DECODE_CLASS: > + case VIDEO_ENHANCEMENT_CLASS: > + return media_gt; > + case OTHER_CLASS: > + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0)) > + return media_gt; > + fallthrough; > + default: > + return gt; > } > - > - if (instance <= MAX_ENGINE_INSTANCE) > - engine = gt->engine_class[class][instance]; > - else > - engine = NULL; > - > - if (likely(engine)) > - return intel_engine_cs_irq(engine, iir); > - > -err: > - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, instance=0x%x\n", > - class, instance); > } > > static void > @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, const u32 identity) > const u8 class = GEN11_INTR_ENGINE_CLASS(identity); > const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); > const u16 intr = GEN11_INTR_ENGINE_INTR(identity); > + struct intel_engine_cs *engine; > > if (unlikely(!intr)) > return; > > - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) > - return gen11_engine_irq_handler(gt, class, instance, intr); > + /* > + * Platforms with standalone media have the media and GSC engines in > + * another GT. > + */ > + gt = pick_gt(gt, class, instance); > + > + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) > + engine = gt->engine_class[class][instance]; > + else > + engine = NULL; > + > + if (engine) > + return intel_engine_cs_irq(engine, intr); Drive by observation - you could fold the above two ifs into one since engine appears unused afterwards. Regards, Tvrtko > > if (class == OTHER_CLASS) > return gen11_other_irq_handler(gt, instance, intr); > @@ -206,7 +210,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0); > if (CCS_MASK(gt)) > intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, 0); > - if (HAS_HECI_GSC(gt->i915)) > + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, 0); > > /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */ > @@ -233,7 +237,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) > intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~0); > if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) > intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~0); > - if (HAS_HECI_GSC(gt->i915)) > + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~0); > > intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0); > @@ -249,7 +253,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > { > struct intel_uncore *uncore = gt->uncore; > u32 irqs = GT_RENDER_USER_INTERRUPT; > - const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); > + u32 gsc_mask = 0; > u32 dmask; > u32 smask; > > @@ -261,6 +265,11 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > dmask = irqs << 16 | irqs; > smask = irqs << 16; > > + if (HAS_ENGINE(gt, GSC0)) > + gsc_mask = irqs; > + else if (HAS_HECI_GSC(gt->i915)) > + gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); > + > BUILD_BUG_ON(irqs & 0xffff0000); > > /* Enable RCS, BCS, VCS and VECS class interrupts. */ > @@ -268,9 +277,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask); > if (CCS_MASK(gt)) > intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask); > - if (HAS_HECI_GSC(gt->i915)) > - intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, > - gsc_mask); > + if (gsc_mask) > + intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask); > > /* Unmask irqs on RCS, BCS, VCS and VECS engines. */ > intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask); > @@ -296,7 +304,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) > intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~dmask); > if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) > intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask); > - if (HAS_HECI_GSC(gt->i915)) > + if (gsc_mask) > intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask); > > /*
On 10/28/2022 1:38 AM, Tvrtko Ursulin wrote: > > On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote: >> The GSC CS re-uses the same interrupt bits that the GSC used in older >> platforms. This means that we can now have an engine interrupt coming >> out of OTHER_CLASS, so we need to handle that appropriately. >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------ >> 1 file changed, 43 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> index f26882fdc24c..34ff1ee7e931 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >> @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, >> const u8 instance, >> instance, iir); >> } >> -static void >> -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, >> - const u8 instance, const u16 iir) >> +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 >> instance) >> { >> - struct intel_engine_cs *engine; >> - >> - /* >> - * Platforms with standalone media have their media engines in >> another >> - * GT. >> - */ >> - if (MEDIA_VER(gt->i915) >= 13 && >> - (class == VIDEO_DECODE_CLASS || class == >> VIDEO_ENHANCEMENT_CLASS)) { >> - if (!gt->i915->media_gt) >> - goto err; >> + struct intel_gt *media_gt = gt->i915->media_gt; >> - gt = gt->i915->media_gt; >> + /* we expect the non-media gt to be passed in */ >> + GEM_BUG_ON(gt == media_gt); >> + >> + if (!media_gt) >> + return gt; >> + >> + switch (class) { >> + case VIDEO_DECODE_CLASS: >> + case VIDEO_ENHANCEMENT_CLASS: >> + return media_gt; >> + case OTHER_CLASS: >> + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, >> GSC0)) >> + return media_gt; >> + fallthrough; >> + default: >> + return gt; >> } >> - >> - if (instance <= MAX_ENGINE_INSTANCE) >> - engine = gt->engine_class[class][instance]; >> - else >> - engine = NULL; >> - >> - if (likely(engine)) >> - return intel_engine_cs_irq(engine, iir); >> - >> -err: >> - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, >> instance=0x%x\n", >> - class, instance); >> } >> static void >> @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, >> const u32 identity) >> const u8 class = GEN11_INTR_ENGINE_CLASS(identity); >> const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); >> const u16 intr = GEN11_INTR_ENGINE_INTR(identity); >> + struct intel_engine_cs *engine; >> if (unlikely(!intr)) >> return; >> - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) >> - return gen11_engine_irq_handler(gt, class, instance, intr); >> + /* >> + * Platforms with standalone media have the media and GSC >> engines in >> + * another GT. >> + */ >> + gt = pick_gt(gt, class, instance); >> + >> + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) >> + engine = gt->engine_class[class][instance]; >> + else >> + engine = NULL; >> + >> + if (engine) >> + return intel_engine_cs_irq(engine, intr); > > Drive by observation - you could fold the above two ifs into one since > engine appears unused afterwards. engine can be NULL in both branches of the if statement, so to get a unified if we'd have to do something like: if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) { struct intel_engine_cs *engine = gt->engine_class[class][instance]; if (engine) return intel_engine_cs_irq(engine, intr); } Is this what you are suggesting? Daniele > > Regards, > > Tvrtko > >> if (class == OTHER_CLASS) >> return gen11_other_irq_handler(gt, instance, intr); >> @@ -206,7 +210,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) >> intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0); >> if (CCS_MASK(gt)) >> intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, 0); >> - if (HAS_HECI_GSC(gt->i915)) >> + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) >> intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, 0); >> /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */ >> @@ -233,7 +237,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) >> intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~0); >> if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) >> intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~0); >> - if (HAS_HECI_GSC(gt->i915)) >> + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) >> intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~0); >> intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0); >> @@ -249,7 +253,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) >> { >> struct intel_uncore *uncore = gt->uncore; >> u32 irqs = GT_RENDER_USER_INTERRUPT; >> - const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); >> + u32 gsc_mask = 0; >> u32 dmask; >> u32 smask; >> @@ -261,6 +265,11 @@ void gen11_gt_irq_postinstall(struct intel_gt >> *gt) >> dmask = irqs << 16 | irqs; >> smask = irqs << 16; >> + if (HAS_ENGINE(gt, GSC0)) >> + gsc_mask = irqs; >> + else if (HAS_HECI_GSC(gt->i915)) >> + gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); >> + >> BUILD_BUG_ON(irqs & 0xffff0000); >> /* Enable RCS, BCS, VCS and VECS class interrupts. */ >> @@ -268,9 +277,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) >> intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask); >> if (CCS_MASK(gt)) >> intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask); >> - if (HAS_HECI_GSC(gt->i915)) >> - intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, >> - gsc_mask); >> + if (gsc_mask) >> + intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, >> gsc_mask); >> /* Unmask irqs on RCS, BCS, VCS and VECS engines. */ >> intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask); >> @@ -296,7 +304,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) >> intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~dmask); >> if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) >> intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask); >> - if (HAS_HECI_GSC(gt->i915)) >> + if (gsc_mask) >> intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, >> ~gsc_mask); >> /*
On 28/10/2022 18:00, Ceraolo Spurio, Daniele wrote: > On 10/28/2022 1:38 AM, Tvrtko Ursulin wrote: >> >> On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote: >>> The GSC CS re-uses the same interrupt bits that the GSC used in older >>> platforms. This means that we can now have an engine interrupt coming >>> out of OTHER_CLASS, so we need to handle that appropriately. >>> >>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Matt Roper <matthew.d.roper@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------ >>> 1 file changed, 43 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>> index f26882fdc24c..34ff1ee7e931 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>> @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, >>> const u8 instance, >>> instance, iir); >>> } >>> -static void >>> -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, >>> - const u8 instance, const u16 iir) >>> +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 >>> instance) >>> { >>> - struct intel_engine_cs *engine; >>> - >>> - /* >>> - * Platforms with standalone media have their media engines in >>> another >>> - * GT. >>> - */ >>> - if (MEDIA_VER(gt->i915) >= 13 && >>> - (class == VIDEO_DECODE_CLASS || class == >>> VIDEO_ENHANCEMENT_CLASS)) { >>> - if (!gt->i915->media_gt) >>> - goto err; >>> + struct intel_gt *media_gt = gt->i915->media_gt; >>> - gt = gt->i915->media_gt; >>> + /* we expect the non-media gt to be passed in */ >>> + GEM_BUG_ON(gt == media_gt); >>> + >>> + if (!media_gt) >>> + return gt; >>> + >>> + switch (class) { >>> + case VIDEO_DECODE_CLASS: >>> + case VIDEO_ENHANCEMENT_CLASS: >>> + return media_gt; >>> + case OTHER_CLASS: >>> + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, >>> GSC0)) >>> + return media_gt; >>> + fallthrough; >>> + default: >>> + return gt; >>> } >>> - >>> - if (instance <= MAX_ENGINE_INSTANCE) >>> - engine = gt->engine_class[class][instance]; >>> - else >>> - engine = NULL; >>> - >>> - if (likely(engine)) >>> - return intel_engine_cs_irq(engine, iir); >>> - >>> -err: >>> - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, >>> instance=0x%x\n", >>> - class, instance); >>> } >>> static void >>> @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, >>> const u32 identity) >>> const u8 class = GEN11_INTR_ENGINE_CLASS(identity); >>> const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); >>> const u16 intr = GEN11_INTR_ENGINE_INTR(identity); >>> + struct intel_engine_cs *engine; >>> if (unlikely(!intr)) >>> return; >>> - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) >>> - return gen11_engine_irq_handler(gt, class, instance, intr); >>> + /* >>> + * Platforms with standalone media have the media and GSC >>> engines in >>> + * another GT. >>> + */ >>> + gt = pick_gt(gt, class, instance); >>> + >>> + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) >>> + engine = gt->engine_class[class][instance]; >>> + else >>> + engine = NULL; >>> + >>> + if (engine) >>> + return intel_engine_cs_irq(engine, intr); >> >> Drive by observation - you could fold the above two ifs into one since >> engine appears unused afterwards. > > engine can be NULL in both branches of the if statement, so to get a > unified if we'd have to do something like: > > if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) { > struct intel_engine_cs *engine = > gt->engine_class[class][instance]; > if (engine) > return intel_engine_cs_irq(engine, intr); > } > > Is this what you are suggesting? Right, two ifs are needed after all. Well at least it would avoid the pointless engine = NULL assignment. Up to you. Absence of any out-of-range class/instance logging is intentional? Regards, Tvrtko
On 10/31/2022 5:55 AM, Tvrtko Ursulin wrote: > > On 28/10/2022 18:00, Ceraolo Spurio, Daniele wrote: >> On 10/28/2022 1:38 AM, Tvrtko Ursulin wrote: >>> >>> On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote: >>>> The GSC CS re-uses the same interrupt bits that the GSC used in older >>>> platforms. This means that we can now have an engine interrupt coming >>>> out of OTHER_CLASS, so we need to handle that appropriately. >>>> >>>> Signed-off-by: Daniele Ceraolo Spurio >>>> <daniele.ceraolospurio@intel.com> >>>> Cc: Matt Roper <matthew.d.roper@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 >>>> ++++++++++++++------------ >>>> 1 file changed, 43 insertions(+), 35 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> index f26882fdc24c..34ff1ee7e931 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c >>>> @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, >>>> const u8 instance, >>>> instance, iir); >>>> } >>>> -static void >>>> -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, >>>> - const u8 instance, const u16 iir) >>>> +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 >>>> instance) >>>> { >>>> - struct intel_engine_cs *engine; >>>> - >>>> - /* >>>> - * Platforms with standalone media have their media engines in >>>> another >>>> - * GT. >>>> - */ >>>> - if (MEDIA_VER(gt->i915) >= 13 && >>>> - (class == VIDEO_DECODE_CLASS || class == >>>> VIDEO_ENHANCEMENT_CLASS)) { >>>> - if (!gt->i915->media_gt) >>>> - goto err; >>>> + struct intel_gt *media_gt = gt->i915->media_gt; >>>> - gt = gt->i915->media_gt; >>>> + /* we expect the non-media gt to be passed in */ >>>> + GEM_BUG_ON(gt == media_gt); >>>> + >>>> + if (!media_gt) >>>> + return gt; >>>> + >>>> + switch (class) { >>>> + case VIDEO_DECODE_CLASS: >>>> + case VIDEO_ENHANCEMENT_CLASS: >>>> + return media_gt; >>>> + case OTHER_CLASS: >>>> + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, >>>> GSC0)) >>>> + return media_gt; >>>> + fallthrough; >>>> + default: >>>> + return gt; >>>> } >>>> - >>>> - if (instance <= MAX_ENGINE_INSTANCE) >>>> - engine = gt->engine_class[class][instance]; >>>> - else >>>> - engine = NULL; >>>> - >>>> - if (likely(engine)) >>>> - return intel_engine_cs_irq(engine, iir); >>>> - >>>> -err: >>>> - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, >>>> instance=0x%x\n", >>>> - class, instance); >>>> } >>>> static void >>>> @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt >>>> *gt, const u32 identity) >>>> const u8 class = GEN11_INTR_ENGINE_CLASS(identity); >>>> const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); >>>> const u16 intr = GEN11_INTR_ENGINE_INTR(identity); >>>> + struct intel_engine_cs *engine; >>>> if (unlikely(!intr)) >>>> return; >>>> - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) >>>> - return gen11_engine_irq_handler(gt, class, instance, intr); >>>> + /* >>>> + * Platforms with standalone media have the media and GSC >>>> engines in >>>> + * another GT. >>>> + */ >>>> + gt = pick_gt(gt, class, instance); >>>> + >>>> + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) >>>> + engine = gt->engine_class[class][instance]; >>>> + else >>>> + engine = NULL; >>>> + >>>> + if (engine) >>>> + return intel_engine_cs_irq(engine, intr); >>> >>> Drive by observation - you could fold the above two ifs into one >>> since engine appears unused afterwards. >> >> engine can be NULL in both branches of the if statement, so to get a >> unified if we'd have to do something like: >> >> if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) { >> struct intel_engine_cs *engine = >> gt->engine_class[class][instance]; >> if (engine) >> return intel_engine_cs_irq(engine, intr); >> } >> >> Is this what you are suggesting? > > Right, two ifs are needed after all. Well at least it would avoid the > pointless engine = NULL assignment. Up to you. > > Absence of any out-of-range class/instance logging is intentional? There is already a log further down in this function. Daniele > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c index f26882fdc24c..34ff1ee7e931 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance, instance, iir); } -static void -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class, - const u8 instance, const u16 iir) +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance) { - struct intel_engine_cs *engine; - - /* - * Platforms with standalone media have their media engines in another - * GT. - */ - if (MEDIA_VER(gt->i915) >= 13 && - (class == VIDEO_DECODE_CLASS || class == VIDEO_ENHANCEMENT_CLASS)) { - if (!gt->i915->media_gt) - goto err; + struct intel_gt *media_gt = gt->i915->media_gt; - gt = gt->i915->media_gt; + /* we expect the non-media gt to be passed in */ + GEM_BUG_ON(gt == media_gt); + + if (!media_gt) + return gt; + + switch (class) { + case VIDEO_DECODE_CLASS: + case VIDEO_ENHANCEMENT_CLASS: + return media_gt; + case OTHER_CLASS: + if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0)) + return media_gt; + fallthrough; + default: + return gt; } - - if (instance <= MAX_ENGINE_INSTANCE) - engine = gt->engine_class[class][instance]; - else - engine = NULL; - - if (likely(engine)) - return intel_engine_cs_irq(engine, iir); - -err: - WARN_ONCE(1, "unhandled engine interrupt class=0x%x, instance=0x%x\n", - class, instance); } static void @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, const u32 identity) const u8 class = GEN11_INTR_ENGINE_CLASS(identity); const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity); const u16 intr = GEN11_INTR_ENGINE_INTR(identity); + struct intel_engine_cs *engine; if (unlikely(!intr)) return; - if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS) - return gen11_engine_irq_handler(gt, class, instance, intr); + /* + * Platforms with standalone media have the media and GSC engines in + * another GT. + */ + gt = pick_gt(gt, class, instance); + + if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) + engine = gt->engine_class[class][instance]; + else + engine = NULL; + + if (engine) + return intel_engine_cs_irq(engine, intr); if (class == OTHER_CLASS) return gen11_other_irq_handler(gt, instance, intr); @@ -206,7 +210,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0); if (CCS_MASK(gt)) intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, 0); - if (HAS_HECI_GSC(gt->i915)) + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, 0); /* Restore masks irqs on RCS, BCS, VCS and VECS engines. */ @@ -233,7 +237,7 @@ void gen11_gt_irq_reset(struct intel_gt *gt) intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~0); if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~0); - if (HAS_HECI_GSC(gt->i915)) + if (HAS_HECI_GSC(gt->i915) || HAS_ENGINE(gt, GSC0)) intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~0); intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0); @@ -249,7 +253,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) { struct intel_uncore *uncore = gt->uncore; u32 irqs = GT_RENDER_USER_INTERRUPT; - const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); + u32 gsc_mask = 0; u32 dmask; u32 smask; @@ -261,6 +265,11 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) dmask = irqs << 16 | irqs; smask = irqs << 16; + if (HAS_ENGINE(gt, GSC0)) + gsc_mask = irqs; + else if (HAS_HECI_GSC(gt->i915)) + gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1); + BUILD_BUG_ON(irqs & 0xffff0000); /* Enable RCS, BCS, VCS and VECS class interrupts. */ @@ -268,9 +277,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask); if (CCS_MASK(gt)) intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask); - if (HAS_HECI_GSC(gt->i915)) - intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, - gsc_mask); + if (gsc_mask) + intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask); /* Unmask irqs on RCS, BCS, VCS and VECS engines. */ intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask); @@ -296,7 +304,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt) intel_uncore_write(uncore, GEN12_CCS0_CCS1_INTR_MASK, ~dmask); if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3)) intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask); - if (HAS_HECI_GSC(gt->i915)) + if (gsc_mask) intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask); /*
The GSC CS re-uses the same interrupt bits that the GSC used in older platforms. This means that we can now have an engine interrupt coming out of OTHER_CLASS, so we need to handle that appropriately. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------ 1 file changed, 43 insertions(+), 35 deletions(-)