From: Daniel Vetter <daniel@ffwll.ch> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: Matthew Brost <matthew.brost@intel.com>, Dave Airlie <airlied@linux.ie>, Mika Kuoppala <mika.kuoppala@linux.intel.com>, intel-gfx <intel-gfx@lists.freedesktop.org>, Lucas De Marchi <lucas.demarchi@intel.com>, dri-devel <dri-devel@lists.freedesktop.org>, "Wilson, Chris" <chris@chris-wilson.co.uk>, Rodrigo Vivi <rodrigo.vivi@intel.com> Subject: Re: [PATCH v3 1/2] drm/i915: Use the correct IRQ during resume Date: Wed, 30 Jun 2021 19:02:01 +0200 [thread overview] Message-ID: <CAKMK7uFFW1Nxch1s+5CjXhRs4hU12H7C9zRSurO54c+ePzys5w@mail.gmail.com> (raw) In-Reply-To: <ce44caf4-1823-121b-5db4-61eaa9827327@suse.de> On Wed, Jun 30, 2021 at 4:18 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 30.06.21 um 12:49 schrieb Daniel Vetter: > > On Wed, Jun 30, 2021 at 11:52:27AM +0200, Thomas Zimmermann wrote: > >> The code in xcs_resume() probably didn't work as intended. It uses > >> struct drm_device.irq, which is allocated to 0, but never initialized > >> by i915 to the device's interrupt number. > >> > >> v3: > >> * also use intel_synchronize_hardirq() at another callsite > >> v2: > >> * wrap irq code in intel_synchronize_hardirq() (Ville) > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Fixes: 536f77b1caa0 ("drm/i915/gt: Call stop_ring() from ring resume, again") > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- > >> drivers/gpu/drm/i915/gt/intel_ring_submission.c | 2 +- > >> drivers/gpu/drm/i915/i915_irq.c | 5 +++++ > >> drivers/gpu/drm/i915/i915_irq.h | 1 + > >> 4 files changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> index 88694822716a..5ca3d1664335 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> @@ -1229,7 +1229,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) > >> return true; > >> > >> /* Waiting to drain ELSP? */ > >> - synchronize_hardirq(to_pci_dev(engine->i915->drm.dev)->irq); > >> + intel_synchronize_hardirq(engine->i915); > >> intel_engine_flush_submission(engine); > >> > >> /* ELSP is empty, but there are ready requests? E.g. after reset */ > >> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> index 5d42a12ef3d6..1b5a22a83db6 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> @@ -185,7 +185,7 @@ static int xcs_resume(struct intel_engine_cs *engine) > >> ring->head, ring->tail); > >> > >> /* Double check the ring is empty & disabled before we resume */ > >> - synchronize_hardirq(engine->i915->drm.irq); > >> + intel_synchronize_hardirq(engine->i915); > >> if (!stop_ring(engine)) > >> goto err; > >> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > >> index 7d0ce8b9f8ed..2203dca19895 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.c > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > >> @@ -4575,3 +4575,8 @@ void intel_synchronize_irq(struct drm_i915_private *i915) > >> { > >> synchronize_irq(to_pci_dev(i915->drm.dev)->irq); > >> } > >> + > >> +void intel_synchronize_hardirq(struct drm_i915_private *i915) > >> +{ > >> + synchronize_hardirq(to_pci_dev(i915->drm.dev)->irq); > > > > I honestly think the hardirq here is about as much cargo-culted as using > > the wrong irq number. > > > > I'd just use intel_synchronize_irq in both places and see whether CI > > complains, then go with that. > > Well, ok. I don't think I have Sandybridge HW available. Would the Intel > CI infrastructure catch any problems with such a change? If there's anything obvious busted with it it should catch it (like if we end up calling synchronize_irq from softirq context, where only synchronize_hardirq is allowed, but I don't think that's the case). And if I'm wrong we know what kind of comment to put there to explain why things are different. -Daniel > Best regards > Thomas > > > -Daniel > > > >> +} > >> diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h > >> index db34d5dbe402..e43b6734f21b 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.h > >> +++ b/drivers/gpu/drm/i915/i915_irq.h > >> @@ -94,6 +94,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv); > >> void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv); > >> bool intel_irqs_enabled(struct drm_i915_private *dev_priv); > >> void intel_synchronize_irq(struct drm_i915_private *i915); > >> +void intel_synchronize_hardirq(struct drm_i915_private *i915); > >> > >> int intel_get_crtc_scanline(struct intel_crtc *crtc); > >> void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv, > >> -- > >> 2.32.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: Dave Airlie <airlied@linux.ie>, intel-gfx <intel-gfx@lists.freedesktop.org>, Lucas De Marchi <lucas.demarchi@intel.com>, dri-devel <dri-devel@lists.freedesktop.org>, "Wilson, Chris" <chris@chris-wilson.co.uk> Subject: Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Use the correct IRQ during resume Date: Wed, 30 Jun 2021 19:02:01 +0200 [thread overview] Message-ID: <CAKMK7uFFW1Nxch1s+5CjXhRs4hU12H7C9zRSurO54c+ePzys5w@mail.gmail.com> (raw) In-Reply-To: <ce44caf4-1823-121b-5db4-61eaa9827327@suse.de> On Wed, Jun 30, 2021 at 4:18 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 30.06.21 um 12:49 schrieb Daniel Vetter: > > On Wed, Jun 30, 2021 at 11:52:27AM +0200, Thomas Zimmermann wrote: > >> The code in xcs_resume() probably didn't work as intended. It uses > >> struct drm_device.irq, which is allocated to 0, but never initialized > >> by i915 to the device's interrupt number. > >> > >> v3: > >> * also use intel_synchronize_hardirq() at another callsite > >> v2: > >> * wrap irq code in intel_synchronize_hardirq() (Ville) > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Fixes: 536f77b1caa0 ("drm/i915/gt: Call stop_ring() from ring resume, again") > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +- > >> drivers/gpu/drm/i915/gt/intel_ring_submission.c | 2 +- > >> drivers/gpu/drm/i915/i915_irq.c | 5 +++++ > >> drivers/gpu/drm/i915/i915_irq.h | 1 + > >> 4 files changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> index 88694822716a..5ca3d1664335 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > >> @@ -1229,7 +1229,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine) > >> return true; > >> > >> /* Waiting to drain ELSP? */ > >> - synchronize_hardirq(to_pci_dev(engine->i915->drm.dev)->irq); > >> + intel_synchronize_hardirq(engine->i915); > >> intel_engine_flush_submission(engine); > >> > >> /* ELSP is empty, but there are ready requests? E.g. after reset */ > >> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> index 5d42a12ef3d6..1b5a22a83db6 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c > >> @@ -185,7 +185,7 @@ static int xcs_resume(struct intel_engine_cs *engine) > >> ring->head, ring->tail); > >> > >> /* Double check the ring is empty & disabled before we resume */ > >> - synchronize_hardirq(engine->i915->drm.irq); > >> + intel_synchronize_hardirq(engine->i915); > >> if (!stop_ring(engine)) > >> goto err; > >> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > >> index 7d0ce8b9f8ed..2203dca19895 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.c > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > >> @@ -4575,3 +4575,8 @@ void intel_synchronize_irq(struct drm_i915_private *i915) > >> { > >> synchronize_irq(to_pci_dev(i915->drm.dev)->irq); > >> } > >> + > >> +void intel_synchronize_hardirq(struct drm_i915_private *i915) > >> +{ > >> + synchronize_hardirq(to_pci_dev(i915->drm.dev)->irq); > > > > I honestly think the hardirq here is about as much cargo-culted as using > > the wrong irq number. > > > > I'd just use intel_synchronize_irq in both places and see whether CI > > complains, then go with that. > > Well, ok. I don't think I have Sandybridge HW available. Would the Intel > CI infrastructure catch any problems with such a change? If there's anything obvious busted with it it should catch it (like if we end up calling synchronize_irq from softirq context, where only synchronize_hardirq is allowed, but I don't think that's the case). And if I'm wrong we know what kind of comment to put there to explain why things are different. -Daniel > Best regards > Thomas > > > -Daniel > > > >> +} > >> diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h > >> index db34d5dbe402..e43b6734f21b 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.h > >> +++ b/drivers/gpu/drm/i915/i915_irq.h > >> @@ -94,6 +94,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv); > >> void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv); > >> bool intel_irqs_enabled(struct drm_i915_private *dev_priv); > >> void intel_synchronize_irq(struct drm_i915_private *i915); > >> +void intel_synchronize_hardirq(struct drm_i915_private *i915); > >> > >> int intel_get_crtc_scanline(struct intel_crtc *crtc); > >> void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv, > >> -- > >> 2.32.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-06-30 17:02 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-30 9:52 [PATCH v3 0/2] drm/i915: IRQ fixes Thomas Zimmermann 2021-06-30 9:52 ` [Intel-gfx] " Thomas Zimmermann 2021-06-30 9:52 ` [PATCH v3 1/2] drm/i915: Use the correct IRQ during resume Thomas Zimmermann 2021-06-30 9:52 ` [Intel-gfx] " Thomas Zimmermann 2021-06-30 10:49 ` Daniel Vetter 2021-06-30 10:49 ` [Intel-gfx] " Daniel Vetter 2021-06-30 14:18 ` Thomas Zimmermann 2021-06-30 14:18 ` [Intel-gfx] " Thomas Zimmermann 2021-06-30 17:02 ` Daniel Vetter [this message] 2021-06-30 17:02 ` Daniel Vetter 2021-06-30 9:52 ` [PATCH v3 2/2] drm/i915: Drop all references to DRM IRQ midlayer Thomas Zimmermann 2021-06-30 9:52 ` [Intel-gfx] " Thomas Zimmermann 2021-06-30 9:52 ` Thomas Zimmermann 2021-06-30 10:06 ` Greg KH 2021-06-30 10:06 ` [Intel-gfx] " Greg KH 2021-06-30 10:06 ` Greg KH 2021-06-30 11:46 ` Thomas Zimmermann 2021-06-30 11:46 ` [Intel-gfx] " Thomas Zimmermann 2021-06-30 11:46 ` Thomas Zimmermann 2021-06-30 16:41 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: IRQ fixes (rev2) Patchwork 2021-06-30 17:06 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-07-01 0:07 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " 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=CAKMK7uFFW1Nxch1s+5CjXhRs4hU12H7C9zRSurO54c+ePzys5w@mail.gmail.com \ --to=daniel@ffwll.ch \ --cc=airlied@linux.ie \ --cc=chris@chris-wilson.co.uk \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=lucas.demarchi@intel.com \ --cc=matthew.brost@intel.com \ --cc=mika.kuoppala@linux.intel.com \ --cc=rodrigo.vivi@intel.com \ --cc=tzimmermann@suse.de \ /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.