| Message ID | 20240912172539.418957-3-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/irq: Remove duplicated irq_enabled variable | expand |
-----Original Message----- From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Rodrigo Vivi Sent: Thursday, September 12, 2024 10:26 AM To: intel-gfx@lists.freedesktop.org Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com> Subject: [PATCH 3/3] drm/i915/irq: Rename suspend/resume funcitons > s/funcitons/functions > Although these functions are used in runtime_pm, they are not > exclusively used there, so remove the misleading prefix. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++-- > drivers/gpu/drm/i915/i915_driver.c | 12 +++++------ > drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++++-------------- > drivers/gpu/drm/i915/i915_irq.h | 4 ++-- > 4 files changed, 24 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c > index 8f1ea95471ef..f42f21632306 100644 > --- a/drivers/gpu/drm/i915/gt/intel_reset.c > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c > @@ -1233,7 +1233,7 @@ void intel_gt_reset(struct intel_gt *gt, > } > > if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) > - intel_runtime_pm_disable_interrupts(gt->i915); > + intel_irq_suspend(gt->i915); > > if (do_reset(gt, stalled_mask)) { > gt_err(gt, "Failed to reset chip\n"); > @@ -1241,7 +1241,7 @@ void intel_gt_reset(struct intel_gt *gt, > } > > if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) > - intel_runtime_pm_enable_interrupts(gt->i915); > + intel_irq_resume(gt->i915); > > intel_overlay_reset(gt->i915); > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c > index 943e938040c0..f82aa313f854 100644 > --- a/drivers/gpu/drm/i915/i915_driver.c > +++ b/drivers/gpu/drm/i915/i915_driver.c > @@ -950,7 +950,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915) > > intel_dp_mst_suspend(i915); > > - intel_runtime_pm_disable_interrupts(i915); > + intel_irq_suspend(i915); > intel_hpd_cancel_work(i915); > > if (HAS_DISPLAY(i915)) > @@ -1035,7 +1035,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > intel_dp_mst_suspend(dev_priv); > > - intel_runtime_pm_disable_interrupts(dev_priv); > + intel_irq_suspend(dev_priv); > intel_hpd_cancel_work(dev_priv); > > if (HAS_DISPLAY(dev_priv)) > @@ -1181,7 +1181,7 @@ static int i915_drm_resume(struct drm_device *dev) > * Modeset enabling in intel_display_driver_init_hw() also needs working > * interrupts. > */ > - intel_runtime_pm_enable_interrupts(dev_priv); > + intel_irq_resume(dev_priv); > > if (HAS_DISPLAY(dev_priv)) > drm_mode_config_reset(dev); > @@ -1481,7 +1481,7 @@ static int intel_runtime_suspend(struct device *kdev) > for_each_gt(gt, dev_priv, i) > intel_gt_runtime_suspend(gt); > > - intel_runtime_pm_disable_interrupts(dev_priv); > + intel_irq_suspend(dev_priv); > > for_each_gt(gt, dev_priv, i) > intel_uncore_suspend(gt->uncore); > @@ -1494,7 +1494,7 @@ static int intel_runtime_suspend(struct device *kdev) > "Runtime suspend failed, disabling it (%d)\n", ret); > intel_uncore_runtime_resume(&dev_priv->uncore); > > - intel_runtime_pm_enable_interrupts(dev_priv); > + intel_irq_resume(dev_priv); > > for_each_gt(gt, dev_priv, i) > intel_gt_runtime_resume(gt); > @@ -1587,7 +1587,7 @@ static int intel_runtime_resume(struct device *kdev) > for_each_gt(gt, dev_priv, i) > intel_uncore_runtime_resume(gt->uncore); > > - intel_runtime_pm_enable_interrupts(dev_priv); > + intel_irq_resume(dev_priv); > > /* > * No point of rolling back things in case of an error, as the best > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2c0fdb5e05a6..d42997fdee65 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1449,31 +1449,29 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv) > } > > /** > - * intel_runtime_pm_disable_interrupts - runtime interrupt disabling > - * @dev_priv: i915 device instance > + * intel_irq_suspend - Suspend interrupts > + * @i915: i915 device instance > * > - * This function is used to disable interrupts at runtime, both in the runtime > - * pm and the system suspend/resume code. > + * This function is used to disable interrupts at runtime. > */ > -void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv) > +void intel_irq_suspend(struct drm_i915_private *i915) > { > - intel_irq_reset(dev_priv); > - dev_priv->irqs_enabled = false; > - intel_synchronize_irq(dev_priv); > + intel_irq_reset(i915); > + i915->irqs_enabled = false; > + intel_synchronize_irq(i915); > } > > /** > - * intel_runtime_pm_enable_interrupts - runtime interrupt enabling > - * @dev_priv: i915 device instance > + * intel_irq_resume - Resume interrupts > + * @i915: i915 device instance > * > - * This function is used to enable interrupts at runtime, both in the runtime > - * pm and the system suspend/resume code. > + * This function is used to enable interrupts at runtime. > */ > -void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv) > +void intel_irq_resume(struct drm_i915_private *i915) > { > - dev_priv->irqs_enabled = true; > - intel_irq_reset(dev_priv); > - intel_irq_postinstall(dev_priv); > + i915->irqs_enabled = true; > + intel_irq_reset(i915); > + intel_irq_postinstall(i915); > } > > bool intel_irqs_enabled(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h > index e665a1b007dc..cde4cac5eca2 100644 > --- a/drivers/gpu/drm/i915/i915_irq.h > +++ b/drivers/gpu/drm/i915/i915_irq.h > @@ -34,8 +34,8 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv); > void gen6_rps_reset_ei(struct drm_i915_private *dev_priv); > u32 gen6_sanitize_rps_pm_mask(const struct drm_i915_private *i915, u32 mask); > > -void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv); > -void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv); > +void intel_irq_suspend(struct drm_i915_private *i915); > +void intel_irq_resume(struct drm_i915_private *i915); Aside: Though part of me would've preferred intel_irq_disable and intel_irq_enable for these function names, I can see why we would want to use suspend and resume instead: irrespective of naming policy, intel_irq_enable is just a bit too close to intel_irqs_enabled in name. LGTM. Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > 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); > -- > 2.46.0 > >
Hi Rodrigo, On Thu, Sep 12, 2024 at 01:25:39PM GMT, Rodrigo Vivi wrote: > Although these functions are used in runtime_pm, they are not > exclusively used there, so remove the misleading prefix. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> A general note, please add a cover letter even if the series looks trivial. It's important to have an overview of what the series does. Thanks, Andi
On Fri, Sep 13, 2024 at 11:07:33AM +0200, Andi Shyti wrote: > Hi Rodrigo, > > On Thu, Sep 12, 2024 at 01:25:39PM GMT, Rodrigo Vivi wrote: > > Although these functions are used in runtime_pm, they are not > > exclusively used there, so remove the misleading prefix. > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > A general note, please add a cover letter even if the series > looks trivial. It's important to have an overview of what the > series does. You are absolutely right. I'm sorry! On a 1 or 2 patch series I don't like seeing cover letters, but on a 3+ patches, specially on the first submission I believe it is important. And it helps maintainers to writing the pull-request notes later. Thanks for the review and for the note, Rodrigo. > > Thanks, > Andi
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index 8f1ea95471ef..f42f21632306 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -1233,7 +1233,7 @@ void intel_gt_reset(struct intel_gt *gt, } if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) - intel_runtime_pm_disable_interrupts(gt->i915); + intel_irq_suspend(gt->i915); if (do_reset(gt, stalled_mask)) { gt_err(gt, "Failed to reset chip\n"); @@ -1241,7 +1241,7 @@ void intel_gt_reset(struct intel_gt *gt, } if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display) - intel_runtime_pm_enable_interrupts(gt->i915); + intel_irq_resume(gt->i915); intel_overlay_reset(gt->i915); diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index 943e938040c0..f82aa313f854 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -950,7 +950,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915) intel_dp_mst_suspend(i915); - intel_runtime_pm_disable_interrupts(i915); + intel_irq_suspend(i915); intel_hpd_cancel_work(i915); if (HAS_DISPLAY(i915)) @@ -1035,7 +1035,7 @@ static int i915_drm_suspend(struct drm_device *dev) intel_dp_mst_suspend(dev_priv); - intel_runtime_pm_disable_interrupts(dev_priv); + intel_irq_suspend(dev_priv); intel_hpd_cancel_work(dev_priv); if (HAS_DISPLAY(dev_priv)) @@ -1181,7 +1181,7 @@ static int i915_drm_resume(struct drm_device *dev) * Modeset enabling in intel_display_driver_init_hw() also needs working * interrupts. */ - intel_runtime_pm_enable_interrupts(dev_priv); + intel_irq_resume(dev_priv); if (HAS_DISPLAY(dev_priv)) drm_mode_config_reset(dev); @@ -1481,7 +1481,7 @@ static int intel_runtime_suspend(struct device *kdev) for_each_gt(gt, dev_priv, i) intel_gt_runtime_suspend(gt); - intel_runtime_pm_disable_interrupts(dev_priv); + intel_irq_suspend(dev_priv); for_each_gt(gt, dev_priv, i) intel_uncore_suspend(gt->uncore); @@ -1494,7 +1494,7 @@ static int intel_runtime_suspend(struct device *kdev) "Runtime suspend failed, disabling it (%d)\n", ret); intel_uncore_runtime_resume(&dev_priv->uncore); - intel_runtime_pm_enable_interrupts(dev_priv); + intel_irq_resume(dev_priv); for_each_gt(gt, dev_priv, i) intel_gt_runtime_resume(gt); @@ -1587,7 +1587,7 @@ static int intel_runtime_resume(struct device *kdev) for_each_gt(gt, dev_priv, i) intel_uncore_runtime_resume(gt->uncore); - intel_runtime_pm_enable_interrupts(dev_priv); + intel_irq_resume(dev_priv); /* * No point of rolling back things in case of an error, as the best diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 2c0fdb5e05a6..d42997fdee65 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1449,31 +1449,29 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv) } /** - * intel_runtime_pm_disable_interrupts - runtime interrupt disabling - * @dev_priv: i915 device instance + * intel_irq_suspend - Suspend interrupts + * @i915: i915 device instance * - * This function is used to disable interrupts at runtime, both in the runtime - * pm and the system suspend/resume code. + * This function is used to disable interrupts at runtime. */ -void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv) +void intel_irq_suspend(struct drm_i915_private *i915) { - intel_irq_reset(dev_priv); - dev_priv->irqs_enabled = false; - intel_synchronize_irq(dev_priv); + intel_irq_reset(i915); + i915->irqs_enabled = false; + intel_synchronize_irq(i915); } /** - * intel_runtime_pm_enable_interrupts - runtime interrupt enabling - * @dev_priv: i915 device instance + * intel_irq_resume - Resume interrupts + * @i915: i915 device instance * - * This function is used to enable interrupts at runtime, both in the runtime - * pm and the system suspend/resume code. + * This function is used to enable interrupts at runtime. */ -void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv) +void intel_irq_resume(struct drm_i915_private *i915) { - dev_priv->irqs_enabled = true; - intel_irq_reset(dev_priv); - intel_irq_postinstall(dev_priv); + i915->irqs_enabled = true; + intel_irq_reset(i915); + intel_irq_postinstall(i915); } bool intel_irqs_enabled(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h index e665a1b007dc..cde4cac5eca2 100644 --- a/drivers/gpu/drm/i915/i915_irq.h +++ b/drivers/gpu/drm/i915/i915_irq.h @@ -34,8 +34,8 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv); void gen6_rps_reset_ei(struct drm_i915_private *dev_priv); u32 gen6_sanitize_rps_pm_mask(const struct drm_i915_private *i915, u32 mask); -void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv); -void intel_runtime_pm_enable_interrupts(struct drm_i915_private *dev_priv); +void intel_irq_suspend(struct drm_i915_private *i915); +void intel_irq_resume(struct drm_i915_private *i915); 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);
Although these functions are used in runtime_pm, they are not exclusively used there, so remove the misleading prefix. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/gt/intel_reset.c | 4 ++-- drivers/gpu/drm/i915/i915_driver.c | 12 +++++------ drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++++-------------- drivers/gpu/drm/i915/i915_irq.h | 4 ++-- 4 files changed, 24 insertions(+), 26 deletions(-)