| Message ID | 1507712056-25030-4-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > GuC interrupts are currently enabled by Logging and disabled in different > scenarios. Make disabling check whether interrupts were already disabled > and similar for enable path. This will remove the state tracking for the > callers of these functions based on kernel parameters. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > b/drivers/gpu/drm/i915/i915_irq.c > index a3de408..6cf417c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct > drm_i915_private *dev_priv) > void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) > { > + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) Hmm, I don't like that functions from irq.c read and modify guc internal members directly. I would expect that functions here just do their job and any state is maintained by the helper function(s) in guc.c. Also note that this change will not help scenario where one client will try to disable irqs while other client still depends on them. Michal > + return; > + > spin_lock_irq(&dev_priv->irq_lock); > - if (!dev_priv->guc.interrupts_enabled) { > - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & > - dev_priv->pm_guc_events); > - dev_priv->guc.interrupts_enabled = true; > - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); > - } > + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & > + dev_priv->pm_guc_events); > + dev_priv->guc.interrupts_enabled = true; > + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); > spin_unlock_irq(&dev_priv->irq_lock); > } > void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) > { > + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) > + return; > + > spin_lock_irq(&dev_priv->irq_lock); > dev_priv->guc.interrupts_enabled = false;
On 10/11/2017 8:50 PM, Michal Wajdeczko wrote: > On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> GuC interrupts are currently enabled by Logging and disabled in >> different >> scenarios. Make disabling check whether interrupts were already disabled >> and similar for enable path. This will remove the state tracking for the >> callers of these functions based on kernel parameters. >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index a3de408..6cf417c 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct >> drm_i915_private *dev_priv) >> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) >> { >> + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) > > Hmm, I don't like that functions from irq.c read and modify guc internal > members directly. I would expect that functions here just do their job > and any state is maintained by the helper function(s) in guc.c. Sure will move to guc.c. > > Also note that this change will not help scenario where one client will > try to disable irqs while other client still depends on them. > Will add refcounting then. > Michal > >> + return; >> + >> spin_lock_irq(&dev_priv->irq_lock); >> - if (!dev_priv->guc.interrupts_enabled) { >> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >> - dev_priv->pm_guc_events); >> - dev_priv->guc.interrupts_enabled = true; >> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >> - } >> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >> + dev_priv->pm_guc_events); >> + dev_priv->guc.interrupts_enabled = true; >> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >> spin_unlock_irq(&dev_priv->irq_lock); >> } >> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) >> { >> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) >> + return; >> + >> spin_lock_irq(&dev_priv->irq_lock); >> dev_priv->guc.interrupts_enabled = false;
On 10/12/2017 11:20 AM, Sagar Arun Kamble wrote: > > > On 10/11/2017 8:50 PM, Michal Wajdeczko wrote: >> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble >> <sagar.a.kamble@intel.com> wrote: >> >>> GuC interrupts are currently enabled by Logging and disabled in >>> different >>> scenarios. Make disabling check whether interrupts were already >>> disabled >>> and similar for enable path. This will remove the state tracking for >>> the >>> callers of these functions based on kernel parameters. >>> >>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c >>> b/drivers/gpu/drm/i915/i915_irq.c >>> index a3de408..6cf417c 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct >>> drm_i915_private *dev_priv) >>> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) >>> { >>> + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) >> >> Hmm, I don't like that functions from irq.c read and modify guc internal >> members directly. I would expect that functions here just do their job >> and any state is maintained by the helper function(s) in guc.c. > Sure will move to guc.c. >> >> Also note that this change will not help scenario where one client will >> try to disable irqs while other client still depends on them. >> > Will add refcounting then. realized that disable_guc_interrupts is currently happening twice during unload and that can make refcounting asymmetrical. So will stay with bool state for now and will revisit during interrupts related changes may be as precursor to GuC CT series. >> Michal >> >>> + return; >>> + >>> spin_lock_irq(&dev_priv->irq_lock); >>> - if (!dev_priv->guc.interrupts_enabled) { >>> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >>> - dev_priv->pm_guc_events); >>> - dev_priv->guc.interrupts_enabled = true; >>> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >>> - } >>> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >>> + dev_priv->pm_guc_events); >>> + dev_priv->guc.interrupts_enabled = true; >>> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >>> spin_unlock_irq(&dev_priv->irq_lock); >>> } >>> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) >>> { >>> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) >>> + return; >>> + >>> spin_lock_irq(&dev_priv->irq_lock); >>> dev_priv->guc.interrupts_enabled = false; > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 10/12/2017 11:47 AM, Sagar Arun Kamble wrote: > > > On 10/12/2017 11:20 AM, Sagar Arun Kamble wrote: >> >> >> On 10/11/2017 8:50 PM, Michal Wajdeczko wrote: >>> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble >>> <sagar.a.kamble@intel.com> wrote: >>> >>>> GuC interrupts are currently enabled by Logging and disabled in >>>> different >>>> scenarios. Make disabling check whether interrupts were already >>>> disabled >>>> and similar for enable path. This will remove the state tracking >>>> for the >>>> callers of these functions based on kernel parameters. >>>> >>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------ >>>> 1 file changed, 10 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c >>>> b/drivers/gpu/drm/i915/i915_irq.c >>>> index a3de408..6cf417c 100644 >>>> --- a/drivers/gpu/drm/i915/i915_irq.c >>>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>>> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct >>>> drm_i915_private *dev_priv) >>>> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) >>>> { >>>> + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) >>> >>> Hmm, I don't like that functions from irq.c read and modify guc >>> internal >>> members directly. I would expect that functions here just do their job >>> and any state is maintained by the helper function(s) in guc.c. >> Sure will move to guc.c. >>> >>> Also note that this change will not help scenario where one client will >>> try to disable irqs while other client still depends on them. >>> >> Will add refcounting then. > realized that disable_guc_interrupts is currently happening twice > during unload and that can make > refcounting asymmetrical. So will stay with bool state for now and > will revisit during interrupts related > changes may be as precursor to GuC CT series. Based on current interrupt mgmt - pm_ier/pm_imr are already handled by intel_runtime_pm_*_interrupts. And these are being done across suspend/reset properly. Just need to order w.r.t GuC suspend/resume. So gen9_*_guc_interrupts should not be called during fini. They will be taken care of based on interrupt clients like Logging, CT Buffer. >>> Michal >>> >>>> + return; >>>> + >>>> spin_lock_irq(&dev_priv->irq_lock); >>>> - if (!dev_priv->guc.interrupts_enabled) { >>>> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >>>> - dev_priv->pm_guc_events); >>>> - dev_priv->guc.interrupts_enabled = true; >>>> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >>>> - } >>>> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >>>> + dev_priv->pm_guc_events); >>>> + dev_priv->guc.interrupts_enabled = true; >>>> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >>>> spin_unlock_irq(&dev_priv->irq_lock); >>>> } >>>> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) >>>> { >>>> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) >>>> + return; >>>> + >>>> spin_lock_irq(&dev_priv->irq_lock); >>>> dev_priv->guc.interrupts_enabled = false; >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a3de408..6cf417c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv) void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) { + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) + return; + spin_lock_irq(&dev_priv->irq_lock); - if (!dev_priv->guc.interrupts_enabled) { - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & - dev_priv->pm_guc_events); - dev_priv->guc.interrupts_enabled = true; - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); - } + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & + dev_priv->pm_guc_events); + dev_priv->guc.interrupts_enabled = true; + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); spin_unlock_irq(&dev_priv->irq_lock); } void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) { + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) + return; + spin_lock_irq(&dev_priv->irq_lock); dev_priv->guc.interrupts_enabled = false;
GuC interrupts are currently enabled by Logging and disabled in different scenarios. Make disabling check whether interrupts were already disabled and similar for enable path. This will remove the state tracking for the callers of these functions based on kernel parameters. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)