| Message ID | 20180109232336.11029-9-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Paulo Zanoni (2018-01-09 23:23:17) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Since it is not possible to mask individual engine instances > and they are all permanently unmasked we do not need to do > anything for engine interrupt management. This scares me as we will more than double our interrupt generation rate as we have a breadcrumb interrupt after every request, just in case we need to synchronize with the request. We've been relying on the ability to mask those interrupts off, as historically we have been able to saturate cpus with the amount of interrupts we could generate. -Chris
On 10/01/18 02:12, Chris Wilson wrote: > Quoting Paulo Zanoni (2018-01-09 23:23:17) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Since it is not possible to mask individual engine instances >> and they are all permanently unmasked we do not need to do >> anything for engine interrupt management. > > This scares me as we will more than double our interrupt generation rate > as we have a breadcrumb interrupt after every request, just in case we > need to synchronize with the request. We've been relying on the ability > to mask those interrupts off, as historically we have been able to > saturate cpus with the amount of interrupts we could generate. > -Chris We do have masks per instance, but they're in gunit and not in the CS. they're defined in patch 4 of this series: +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) Each instance gets half of a register for a 16 bits vector. I think there was some gotcha with the masks being in gunit but can't remember exactly what. Tvrtko, do you remember anything on this? Thanks, Daniele > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 11/01/2018 19:17, Daniele Ceraolo Spurio wrote: > > > On 10/01/18 02:12, Chris Wilson wrote: >> Quoting Paulo Zanoni (2018-01-09 23:23:17) >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Since it is not possible to mask individual engine instances >>> and they are all permanently unmasked we do not need to do >>> anything for engine interrupt management. >> >> This scares me as we will more than double our interrupt generation rate >> as we have a breadcrumb interrupt after every request, just in case we >> need to synchronize with the request. We've been relying on the ability >> to mask those interrupts off, as historically we have been able to >> saturate cpus with the amount of interrupts we could generate. >> -Chris > > We do have masks per instance, but they're in gunit and not in the CS. > they're defined in patch 4 of this series: > > +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) > +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) > +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) > +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) > +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) > > Each instance gets half of a register for a 16 bits vector. I think > there was some gotcha with the masks being in gunit but can't remember > exactly what. Tvrtko, do you remember anything on this? I remember the message was to not use them since they were pencilled in for removal from the HW. There was some issue with them in general which made that decision attractive, coupled with the fact other OS did not have a need for them. If that removal did not materialize we could perhaps revisit the plans. It would require a locked RMW cycle as minimum since there are two instances in each reg but that probably isn't such a big deal. So yeah, I think summary would be someone needs to pull on sleeves of a HW designer, or someone close, to check what happened with the removal plans, and what exactly was the problematic bit with frequent (un)masking. Regards, Tvrtko
On 1/15/2018 2:38 AM, Tvrtko Ursulin wrote: > > On 11/01/2018 19:17, Daniele Ceraolo Spurio wrote: >> >> >> On 10/01/18 02:12, Chris Wilson wrote: >>> Quoting Paulo Zanoni (2018-01-09 23:23:17) >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> Since it is not possible to mask individual engine instances >>>> and they are all permanently unmasked we do not need to do >>>> anything for engine interrupt management. >>> >>> This scares me as we will more than double our interrupt generation rate >>> as we have a breadcrumb interrupt after every request, just in case we >>> need to synchronize with the request. We've been relying on the ability >>> to mask those interrupts off, as historically we have been able to >>> saturate cpus with the amount of interrupts we could generate. >>> -Chris >> >> We do have masks per instance, but they're in gunit and not in the CS. >> they're defined in patch 4 of this series: >> >> +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) >> +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) >> +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) >> +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) >> +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) >> >> Each instance gets half of a register for a 16 bits vector. I think >> there was some gotcha with the masks being in gunit but can't remember >> exactly what. Tvrtko, do you remember anything on this? > > I remember the message was to not use them since they were pencilled in > for removal from the HW. There was some issue with them in general which > made that decision attractive, coupled with the fact other OS did not > have a need for them. > > If that removal did not materialize we could perhaps revisit the plans. > It would require a locked RMW cycle as minimum since there are two > instances in each reg but that probably isn't such a big deal. > > So yeah, I think summary would be someone needs to pull on sleeves of a > HW designer, or someone close, to check what happened with the removal > plans, and what exactly was the problematic bit with frequent (un)masking. I asked the hardware team about the removal, and that does not seem to be the case. I have added you to the email if you want more clarification. Thanks, Vinay. > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2/1/2018 3:58 PM, Belgaumkar, Vinay wrote: > > > On 1/15/2018 2:38 AM, Tvrtko Ursulin wrote: >> >> On 11/01/2018 19:17, Daniele Ceraolo Spurio wrote: >>> >>> >>> On 10/01/18 02:12, Chris Wilson wrote: >>>> Quoting Paulo Zanoni (2018-01-09 23:23:17) >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> Since it is not possible to mask individual engine instances >>>>> and they are all permanently unmasked we do not need to do >>>>> anything for engine interrupt management. >>>> >>>> This scares me as we will more than double our interrupt generation >>>> rate >>>> as we have a breadcrumb interrupt after every request, just in case we >>>> need to synchronize with the request. We've been relying on the ability >>>> to mask those interrupts off, as historically we have been able to >>>> saturate cpus with the amount of interrupts we could generate. >>>> -Chris >>> >>> We do have masks per instance, but they're in gunit and not in the >>> CS. they're defined in patch 4 of this series: >>> >>> +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) >>> +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) >>> +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) >>> +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) >>> +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) >>> >>> Each instance gets half of a register for a 16 bits vector. I think >>> there was some gotcha with the masks being in gunit but can't >>> remember exactly what. Tvrtko, do you remember anything on this? >> >> I remember the message was to not use them since they were pencilled >> in for removal from the HW. There was some issue with them in general >> which made that decision attractive, coupled with the fact other OS >> did not have a need for them. >> >> If that removal did not materialize we could perhaps revisit the >> plans. It would require a locked RMW cycle as minimum since there are >> two instances in each reg but that probably isn't such a big deal. >> >> So yeah, I think summary would be someone needs to pull on sleeves of >> a HW designer, or someone close, to check what happened with the >> removal plans, and what exactly was the problematic bit with frequent >> (un)masking. > > I asked the hardware team about the removal, and that does not seem to > be the case. I have added you to the email if you want more clarification. Please disregard that comment, looks like there was already a separate discussion on this. Thanks, Vinay. > > Thanks, > Vinay. > >> >> Regards, >> >> Tvrtko >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c index 86acac010bb8..3326b4307c29 100644 --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c @@ -178,18 +178,22 @@ static void irq_enable(struct intel_engine_cs *engine) */ set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted); - /* Caller disables interrupts */ - spin_lock(&engine->i915->irq_lock); - engine->irq_enable(engine); - spin_unlock(&engine->i915->irq_lock); + if (engine->irq_enable) { + /* Caller disables interrupts */ + spin_lock(&engine->i915->irq_lock); + engine->irq_enable(engine); + spin_unlock(&engine->i915->irq_lock); + } } static void irq_disable(struct intel_engine_cs *engine) { - /* Caller disables interrupts */ - spin_lock(&engine->i915->irq_lock); - engine->irq_disable(engine); - spin_unlock(&engine->i915->irq_lock); + if (engine->irq_disable) { + /* Caller disables interrupts */ + spin_lock(&engine->i915->irq_lock); + engine->irq_disable(engine); + spin_unlock(&engine->i915->irq_lock); + } } void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index ff25f209d0a5..de41ad2d5fbc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1963,8 +1963,15 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) engine->set_default_submission = execlists_set_default_submission; - engine->irq_enable = gen8_logical_ring_enable_irq; - engine->irq_disable = gen8_logical_ring_disable_irq; + if (INTEL_GEN(engine->i915) < 11) { + engine->irq_enable = gen8_logical_ring_enable_irq; + engine->irq_disable = gen8_logical_ring_disable_irq; + } else { + /* + * On Gen11 interrupts are permanently unmasked and there + * are no per-engine instance mask bits. + */ + } engine->emit_bb_start = gen8_emit_bb_start; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index d0b22753d26e..2a8823166a0b 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -402,6 +402,11 @@ struct intel_engine_cs { u32 irq_keep_mask; /* always keep these interrupts */ u32 irq_enable_mask; /* bitmask to enable ring interrupt */ + + /* + * irq_enable and irq_disable do not have to be provided for + * an engine. In other words they can be NULL. + */ void (*irq_enable)(struct intel_engine_cs *engine); void (*irq_disable)(struct intel_engine_cs *engine);