On Wed, Jul 20, 2022 at 10:03:42AM -0700, Srivatsa, Anusha wrote:
>
>
> > -----Original Message-----
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, July 20, 2022 2:38 AM
> > To: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Srivatsa, Anusha
> > <anusha.srivatsa@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] drm/i915: Pass drm_i915_private struct
> > instead of gt for gen11_gu_misc_irq_handler()
> >
> >
> > On 18/07/2022 19:54, Matt Roper wrote:
> > > On Mon, Jul 18, 2022 at 11:34:24AM -0700, Anusha Srivatsa wrote:
> > >> gen11_gu_misc_irq_handler() does not do anything tile specific.
> > >>
> > >> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > >
> > > Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > >
> > >> ---
> > >> drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
> > >> 1 file changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > >> b/drivers/gpu/drm/i915/i915_irq.c index 73cebc6aa650..c304af777d58
> > >> 100644
> > >> --- a/drivers/gpu/drm/i915/i915_irq.c
> > >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> > >> @@ -2669,10 +2669,10 @@ gen11_gu_misc_irq_ack(struct intel_gt *gt,
> > const u32 master_ctl)
> > >> }
> > >>
> > >> static void
> > >> -gen11_gu_misc_irq_handler(struct intel_gt *gt, const u32 iir)
> > >> +gen11_gu_misc_irq_handler(struct drm_i915_private *i915, const u32
> > >> +iir)
> > >> {
> > >> if (iir & GEN11_GU_MISC_GSE)
> > >> - intel_opregion_asle_intr(gt->i915);
> > >> + intel_opregion_asle_intr(i915);
> > >> }
> > >>
> > >> static inline u32 gen11_master_intr_disable(void __iomem * const
> > >> regs) @@ -2740,7 +2740,7 @@ static irqreturn_t gen11_irq_handler(int
> > >> irq, void *arg)
> > >>
> >
> > Maybe this is correct but it leaves this, round about here:
> >
> > gu_misc_iir = gen11_gu_misc_irq_ack(gt, master_ctl);
> >
> > So _if_ these registers are truly not per GT, or don't live in the GT block,
> > change this one as well?
> Tvrtko,
> gen11_gu_misc_irq_ack() does not fall in the same category. We do
> operations where we grab register per gt and do some writes that are
> not gt agnostic.
I don't understand what you're saying here. The ack and the handler
functions go hand in hand; they should either both be GT-centric or both
be device centric.
Since the MMIO space is duplicated for each tile, it's always _possible_
to read and process an interrupt register for each tile. The question
is whether this is a type of interrupt where it can only be delivered on
tile0 (similar to display, if we had a multi-tile platform that
supported display) or can each tile raise its own interrupt
independently, requiring that we both ack and handle those interrupts in
a per-tile manner?
Right now the only kind of interrupt we handle on GU_MISC is the "GSE"
bit which is apparently related to panel backlight (display), so it
wouldn't make sense to handle that in a per-GT manner (and we really
shouldn't be receiving the interrupt at all on any of our multi-tile
platforms right now). So it sounds to me like our ack routine should
also be switched over to drm_i915_private.
Matt
>
> Anusha
> > Regards,
> >
> > Tvrtko
> >
> > >> gen11_master_intr_enable(regs);
> > >>
> > >> - gen11_gu_misc_irq_handler(gt, gu_misc_iir);
> > >> + gen11_gu_misc_irq_handler(i915, gu_misc_iir);
> > >>
> > >> pmu_irq_stats(i915, IRQ_HANDLED);
> > >>
> > >> @@ -2805,7 +2805,7 @@ static irqreturn_t dg1_irq_handler(int irq,
> > >> void *arg)
> > >>
> > >> dg1_master_intr_enable(regs);
> > >>
> > >> - gen11_gu_misc_irq_handler(gt, gu_misc_iir);
> > >> + gen11_gu_misc_irq_handler(i915, gu_misc_iir);
> > >>
> > >> pmu_irq_stats(i915, IRQ_HANDLED);
> > >>
> > >> --
> > >> 2.25.1
> > >>
> > >
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
