From: Jani Nikula <jani.nikula@linux.intel.com>
To: Gustavo Sousa <gustavo.sousa@intel.com>,
intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/i915/display: Wrap IRQ-specific uncore functions
Date: Tue, 07 Jan 2025 11:16:08 +0200 [thread overview]
Message-ID: <878qrnm1bb.fsf@intel.com> (raw)
In-Reply-To: <20250103174223.58140-3-gustavo.sousa@intel.com>
On Fri, 03 Jan 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> The current display IRQ code calls some IRQ-specific helpers that use
> intel_uncore_*() MMIO functions instead of the display-specific ones.
> Wrap those helpers in intel_de.h and use them to ensure that the proper
> display-specific hooks (currently only DMC wakelock handling) are
> called.
>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_de.h | 43 ++++++++++
> .../gpu/drm/i915/display/intel_display_irq.c | 85 +++++++++----------
> 2 files changed, 83 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index b7399e9d11cc..151126ab6dad 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -6,6 +6,7 @@
> #ifndef __INTEL_DE_H__
> #define __INTEL_DE_H__
>
> +#include "i915_irq.h"
> #include "intel_display_conversion.h"
> #include "intel_display_core.h"
> #include "intel_dmc_wl.h"
> @@ -246,4 +247,46 @@ intel_de_write_dsb(struct intel_display *display, struct intel_dsb *dsb,
> intel_de_write_fw(display, reg, val);
> }
>
> +/*
> + * Functions to handle IRQ registers (intel_de_irq_*).
> + */
> +static inline void
> +intel_de_irq_init(struct intel_display *display, struct i915_irq_regs regs,
> + u32 imr_val, u32 ier_val)
> +{
> + intel_dmc_wl_get(display, regs.imr);
> + intel_dmc_wl_get(display, regs.ier);
> + intel_dmc_wl_get(display, regs.iir);
> +
> + gen2_irq_init(__to_uncore(display), regs, imr_val, ier_val);
> +
> + intel_dmc_wl_put(display, regs.iir);
> + intel_dmc_wl_put(display, regs.ier);
> + intel_dmc_wl_put(display, regs.imr);
> +}
> +
> +static inline void
> +intel_de_irq_reset(struct intel_display *display, struct i915_irq_regs regs)
> +{
> + intel_dmc_wl_get(display, regs.imr);
> + intel_dmc_wl_get(display, regs.ier);
> + intel_dmc_wl_get(display, regs.iir);
> +
> + gen2_irq_reset(__to_uncore(display), regs);
> +
> + intel_dmc_wl_put(display, regs.iir);
> + intel_dmc_wl_put(display, regs.ier);
> + intel_dmc_wl_put(display, regs.imr);
> +}
> +
> +static inline void
> +intel_de_irq_assert_irr_is_zero(struct intel_display *display, i915_reg_t reg)
> +{
> + intel_dmc_wl_get(display, reg);
> +
> + gen2_assert_iir_is_zero(__to_uncore(display), reg);
> +
> + intel_dmc_wl_put(display, reg);
> +}
> +
I don't think intel_de_irq_* belong in this file. They're more about
*irq* than DE register access. I think intel_display_irq.c is the better
location, and I guess they can be static there.
Moreover, I don't like everyone including intel_de.h also including
i915_irq.h by proxy.
BR,
Jani.
> #endif /* __INTEL_DE_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 9662368a651d..ec7af00739ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -1498,7 +1498,6 @@ void bdw_disable_vblank(struct drm_crtc *_crtc)
> static void _vlv_display_irq_reset(struct drm_i915_private *dev_priv)
> {
> struct intel_display *display = &dev_priv->display;
> - struct intel_uncore *uncore = &dev_priv->uncore;
>
> if (IS_CHERRYVIEW(dev_priv))
> intel_de_write(display, DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
> @@ -1510,7 +1509,7 @@ static void _vlv_display_irq_reset(struct drm_i915_private *dev_priv)
>
> i9xx_pipestat_irq_reset(dev_priv);
>
> - gen2_irq_reset(uncore, VLV_IRQ_REGS);
> + intel_de_irq_reset(display, VLV_IRQ_REGS);
> dev_priv->irq_mask = ~0u;
> }
>
> @@ -1534,8 +1533,7 @@ void i9xx_display_irq_reset(struct drm_i915_private *i915)
>
> void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
> {
> - struct intel_uncore *uncore = &dev_priv->uncore;
> -
> + struct intel_display *display = &dev_priv->display;
> u32 pipestat_mask;
> u32 enable_mask;
> enum pipe pipe;
> @@ -1563,13 +1561,12 @@ void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
>
> dev_priv->irq_mask = ~enable_mask;
>
> - gen2_irq_init(uncore, VLV_IRQ_REGS, dev_priv->irq_mask, enable_mask);
> + intel_de_irq_init(display, VLV_IRQ_REGS, dev_priv->irq_mask, enable_mask);
> }
>
> void gen8_display_irq_reset(struct drm_i915_private *dev_priv)
> {
> struct intel_display *display = &dev_priv->display;
> - struct intel_uncore *uncore = &dev_priv->uncore;
> enum pipe pipe;
>
> if (!HAS_DISPLAY(dev_priv))
> @@ -1581,16 +1578,15 @@ void gen8_display_irq_reset(struct drm_i915_private *dev_priv)
> for_each_pipe(dev_priv, pipe)
> if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_PIPE(pipe)))
> - gen2_irq_reset(uncore, GEN8_DE_PIPE_IRQ_REGS(pipe));
> + intel_de_irq_reset(display, GEN8_DE_PIPE_IRQ_REGS(pipe));
>
> - gen2_irq_reset(uncore, GEN8_DE_PORT_IRQ_REGS);
> - gen2_irq_reset(uncore, GEN8_DE_MISC_IRQ_REGS);
> + intel_de_irq_reset(display, GEN8_DE_PORT_IRQ_REGS);
> + intel_de_irq_reset(display, GEN8_DE_MISC_IRQ_REGS);
> }
>
> void gen11_display_irq_reset(struct drm_i915_private *dev_priv)
> {
> struct intel_display *display = &dev_priv->display;
> - struct intel_uncore *uncore = &dev_priv->uncore;
> enum pipe pipe;
> u32 trans_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
> BIT(TRANSCODER_C) | BIT(TRANSCODER_D);
> @@ -1625,24 +1621,24 @@ void gen11_display_irq_reset(struct drm_i915_private *dev_priv)
> for_each_pipe(dev_priv, pipe)
> if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_PIPE(pipe)))
> - gen2_irq_reset(uncore, GEN8_DE_PIPE_IRQ_REGS(pipe));
> + intel_de_irq_reset(display, GEN8_DE_PIPE_IRQ_REGS(pipe));
>
> - gen2_irq_reset(uncore, GEN8_DE_PORT_IRQ_REGS);
> - gen2_irq_reset(uncore, GEN8_DE_MISC_IRQ_REGS);
> + intel_de_irq_reset(display, GEN8_DE_PORT_IRQ_REGS);
> + intel_de_irq_reset(display, GEN8_DE_MISC_IRQ_REGS);
>
> if (DISPLAY_VER(dev_priv) >= 14)
> - gen2_irq_reset(uncore, PICAINTERRUPT_IRQ_REGS);
> + intel_de_irq_reset(display, PICAINTERRUPT_IRQ_REGS);
> else
> - gen2_irq_reset(uncore, GEN11_DE_HPD_IRQ_REGS);
> + intel_de_irq_reset(display, GEN11_DE_HPD_IRQ_REGS);
>
> if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> - gen2_irq_reset(uncore, SDE_IRQ_REGS);
> + intel_de_irq_reset(display, SDE_IRQ_REGS);
> }
>
> void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> u8 pipe_mask)
> {
> - struct intel_uncore *uncore = &dev_priv->uncore;
> + struct intel_display *display = &dev_priv->display;
> u32 extra_ier = GEN8_PIPE_VBLANK | GEN8_PIPE_FIFO_UNDERRUN |
> gen8_de_pipe_flip_done_mask(dev_priv);
> enum pipe pipe;
> @@ -1655,9 +1651,9 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> }
>
> for_each_pipe_masked(dev_priv, pipe, pipe_mask)
> - gen2_irq_init(uncore, GEN8_DE_PIPE_IRQ_REGS(pipe),
> - dev_priv->display.irq.de_irq_mask[pipe],
> - ~dev_priv->display.irq.de_irq_mask[pipe] | extra_ier);
> + intel_de_irq_init(display, GEN8_DE_PIPE_IRQ_REGS(pipe),
> + dev_priv->display.irq.de_irq_mask[pipe],
> + ~dev_priv->display.irq.de_irq_mask[pipe] | extra_ier);
>
> spin_unlock_irq(&dev_priv->irq_lock);
> }
> @@ -1665,7 +1661,7 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> u8 pipe_mask)
> {
> - struct intel_uncore *uncore = &dev_priv->uncore;
> + struct intel_display *display = &dev_priv->display;
> enum pipe pipe;
>
> spin_lock_irq(&dev_priv->irq_lock);
> @@ -1676,7 +1672,7 @@ void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> }
>
> for_each_pipe_masked(dev_priv, pipe, pipe_mask)
> - gen2_irq_reset(uncore, GEN8_DE_PIPE_IRQ_REGS(pipe));
> + intel_de_irq_reset(display, GEN8_DE_PIPE_IRQ_REGS(pipe));
>
> spin_unlock_irq(&dev_priv->irq_lock);
>
> @@ -1697,7 +1693,7 @@ void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> */
> static void ibx_irq_postinstall(struct drm_i915_private *dev_priv)
> {
> - struct intel_uncore *uncore = &dev_priv->uncore;
> + struct intel_display *display = &dev_priv->display;
> u32 mask;
>
> if (HAS_PCH_NOP(dev_priv))
> @@ -1710,7 +1706,7 @@ static void ibx_irq_postinstall(struct drm_i915_private *dev_priv)
> else
> mask = SDE_GMBUS_CPT;
>
> - gen2_irq_init(uncore, SDE_IRQ_REGS, ~mask, 0xffffffff);
> + intel_de_irq_init(display, SDE_IRQ_REGS, ~mask, 0xffffffff);
> }
>
> void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv)
> @@ -1743,7 +1739,7 @@ void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv)
>
> void ilk_de_irq_postinstall(struct drm_i915_private *i915)
> {
> - struct intel_uncore *uncore = &i915->uncore;
> + struct intel_display *display = &i915->display;
> u32 display_mask, extra_mask;
>
> if (DISPLAY_VER(i915) >= 7) {
> @@ -1767,7 +1763,7 @@ void ilk_de_irq_postinstall(struct drm_i915_private *i915)
> }
>
> if (IS_HASWELL(i915)) {
> - gen2_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> + intel_de_irq_assert_irr_is_zero(display, EDP_PSR_IIR);
> display_mask |= DE_EDP_PSR_INT_HSW;
> }
>
> @@ -1778,8 +1774,8 @@ void ilk_de_irq_postinstall(struct drm_i915_private *i915)
>
> ibx_irq_postinstall(i915);
>
> - gen2_irq_init(uncore, DE_IRQ_REGS, i915->irq_mask,
> - display_mask | extra_mask);
> + intel_de_irq_init(display, DE_IRQ_REGS, i915->irq_mask,
> + display_mask | extra_mask);
> }
>
> static void mtp_irq_postinstall(struct drm_i915_private *i915);
> @@ -1788,7 +1784,6 @@ static void icp_irq_postinstall(struct drm_i915_private *i915);
> void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> {
> struct intel_display *display = &dev_priv->display;
> - struct intel_uncore *uncore = &dev_priv->uncore;
>
> u32 de_pipe_masked = gen8_de_pipe_fault_mask(dev_priv) |
> GEN8_PIPE_CDCLK_CRC_DONE;
> @@ -1854,11 +1849,11 @@ void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> if (!intel_display_power_is_enabled(dev_priv, domain))
> continue;
>
> - gen2_assert_iir_is_zero(uncore,
> - TRANS_PSR_IIR(dev_priv, trans));
> + intel_de_irq_assert_irr_is_zero(display,
> + TRANS_PSR_IIR(dev_priv, trans));
> }
> } else {
> - gen2_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> + intel_de_irq_assert_irr_is_zero(display, EDP_PSR_IIR);
> }
>
> for_each_pipe(dev_priv, pipe) {
> @@ -1866,44 +1861,44 @@ void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>
> if (intel_display_power_is_enabled(dev_priv,
> POWER_DOMAIN_PIPE(pipe)))
> - gen2_irq_init(uncore, GEN8_DE_PIPE_IRQ_REGS(pipe),
> - dev_priv->display.irq.de_irq_mask[pipe],
> - de_pipe_enables);
> + intel_de_irq_init(display, GEN8_DE_PIPE_IRQ_REGS(pipe),
> + dev_priv->display.irq.de_irq_mask[pipe],
> + de_pipe_enables);
> }
>
> - gen2_irq_init(uncore, GEN8_DE_PORT_IRQ_REGS, ~de_port_masked, de_port_enables);
> - gen2_irq_init(uncore, GEN8_DE_MISC_IRQ_REGS, ~de_misc_masked, de_misc_masked);
> + intel_de_irq_init(display, GEN8_DE_PORT_IRQ_REGS, ~de_port_masked, de_port_enables);
> + intel_de_irq_init(display, GEN8_DE_MISC_IRQ_REGS, ~de_misc_masked, de_misc_masked);
>
> if (IS_DISPLAY_VER(dev_priv, 11, 13)) {
> u32 de_hpd_masked = 0;
> u32 de_hpd_enables = GEN11_DE_TC_HOTPLUG_MASK |
> GEN11_DE_TBT_HOTPLUG_MASK;
>
> - gen2_irq_init(uncore, GEN11_DE_HPD_IRQ_REGS, ~de_hpd_masked,
> - de_hpd_enables);
> + intel_de_irq_init(display, GEN11_DE_HPD_IRQ_REGS, ~de_hpd_masked,
> + de_hpd_enables);
> }
> }
>
> static void mtp_irq_postinstall(struct drm_i915_private *i915)
> {
> - struct intel_uncore *uncore = &i915->uncore;
> + struct intel_display *display = &i915->display;
> u32 sde_mask = SDE_GMBUS_ICP | SDE_PICAINTERRUPT;
> u32 de_hpd_mask = XELPDP_AUX_TC_MASK;
> u32 de_hpd_enables = de_hpd_mask | XELPDP_DP_ALT_HOTPLUG_MASK |
> XELPDP_TBT_HOTPLUG_MASK;
>
> - gen2_irq_init(uncore, PICAINTERRUPT_IRQ_REGS, ~de_hpd_mask,
> - de_hpd_enables);
> + intel_de_irq_init(display, PICAINTERRUPT_IRQ_REGS, ~de_hpd_mask,
> + de_hpd_enables);
>
> - gen2_irq_init(uncore, SDE_IRQ_REGS, ~sde_mask, 0xffffffff);
> + intel_de_irq_init(display, SDE_IRQ_REGS, ~sde_mask, 0xffffffff);
> }
>
> static void icp_irq_postinstall(struct drm_i915_private *dev_priv)
> {
> - struct intel_uncore *uncore = &dev_priv->uncore;
> + struct intel_display *display = &dev_priv->display;
> u32 mask = SDE_GMBUS_ICP;
>
> - gen2_irq_init(uncore, SDE_IRQ_REGS, ~mask, 0xffffffff);
> + intel_de_irq_init(display, SDE_IRQ_REGS, ~mask, 0xffffffff);
> }
>
> void gen11_de_irq_postinstall(struct drm_i915_private *dev_priv)
--
Jani Nikula, Intel
next prev parent reply other threads:[~2025-01-07 9:16 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-03 17:41 [PATCH 0/3] drm/i915/dmc_wl: Track pipe interrupt registers Gustavo Sousa
2025-01-03 17:41 ` [PATCH 1/3] drm/i915/display: Use display MMIO functions in intel_display_irq.c Gustavo Sousa
2025-01-09 7:03 ` Hogander, Jouni
2025-01-03 17:41 ` [PATCH 2/3] drm/i915/display: Wrap IRQ-specific uncore functions Gustavo Sousa
2025-01-07 9:16 ` Jani Nikula [this message]
2025-01-13 20:51 ` Gustavo Sousa
2025-01-03 17:41 ` [PATCH 3/3] drm/i915/dmc_wl: Track pipe interrupt registers Gustavo Sousa
2025-01-09 7:07 ` Hogander, Jouni
2025-01-03 18:30 ` ✓ CI.Patch_applied: success for " Patchwork
2025-01-03 18:31 ` ✓ CI.checkpatch: " Patchwork
2025-01-03 18:32 ` ✓ CI.KUnit: " Patchwork
2025-01-03 18:46 ` ✗ Fi.CI.SPARSE: warning " Patchwork
2025-01-03 18:50 ` ✓ CI.Build: success " Patchwork
2025-01-03 18:52 ` ✓ CI.Hooks: " Patchwork
2025-01-03 18:54 ` ✗ CI.checksparse: warning " Patchwork
2025-01-03 19:07 ` ✓ i915.CI.BAT: success " Patchwork
2025-01-03 19:19 ` ✓ Xe.CI.BAT: " Patchwork
2025-01-03 22:22 ` ✓ i915.CI.Full: " Patchwork
2025-01-04 2:30 ` ✗ Xe.CI.Full: 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=878qrnm1bb.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=gustavo.sousa@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
/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: link
Be 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.