From: Jani Nikula <jani.nikula@intel.com>
To: Gustavo Sousa <gustavo.sousa@intel.com>,
intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org
Cc: "Jouni Högander" <jouni.hogander@intel.com>
Subject: Re: [PATCH v2 2/3] drm/i915/display: Wrap IRQ-specific uncore functions
Date: Tue, 14 Jan 2025 11:55:20 +0200 [thread overview]
Message-ID: <87v7uhhg8n.fsf@intel.com> (raw)
In-Reply-To: <20250113204306.112266-3-gustavo.sousa@intel.com>
On Mon, 13 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 to ensure that the proper display-specific hooks
> (currently only DMC wakelock handling) are called.
>
> v2:
> - Move functions to intel_display_irq.c instead of having them in
> intel_de.h. (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> .../gpu/drm/i915/display/intel_display_irq.c | 128 ++++++++++++------
> 1 file changed, 83 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 9662368a651d..d9734fcd0d45 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -15,6 +15,7 @@
> #include "intel_display_irq.h"
> #include "intel_display_trace.h"
> #include "intel_display_types.h"
> +#include "intel_dmc_wl.h"
> #include "intel_dp_aux.h"
> #include "intel_dsb.h"
> #include "intel_fdi_regs.h"
> @@ -25,6 +26,46 @@
> #include "intel_pmdemand.h"
> #include "intel_psr.h"
> #include "intel_psr_regs.h"
> +#include "intel_uncore.h"
> +
> +static void
> +intel_display_irq_regs_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_intel_uncore(display->drm), 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 void
> +intel_display_irq_regs_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_intel_uncore(display->drm), regs);
> +
> + intel_dmc_wl_put(display, regs.iir);
> + intel_dmc_wl_put(display, regs.ier);
> + intel_dmc_wl_put(display, regs.imr);
> +}
> +
> +static void
> +intel_display_irq_regs_assert_irr_is_zero(struct intel_display *display, i915_reg_t reg)
> +{
> + intel_dmc_wl_get(display, reg);
> +
> + gen2_assert_iir_is_zero(to_intel_uncore(display->drm), reg);
> +
> + intel_dmc_wl_put(display, reg);
> +}
I think the _regs_ in the function names are all superfluous.
Other than that, seems fine.
BR,
Jani.
>
> static void
> intel_handle_vblank(struct drm_i915_private *dev_priv, enum pipe pipe)
> @@ -1498,7 +1539,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 +1550,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_display_irq_regs_reset(display, VLV_IRQ_REGS);
> dev_priv->irq_mask = ~0u;
> }
>
> @@ -1534,8 +1574,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 +1602,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_display_irq_regs_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 +1619,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_display_irq_regs_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_display_irq_regs_reset(display, GEN8_DE_PORT_IRQ_REGS);
> + intel_display_irq_regs_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 +1662,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_display_irq_regs_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_display_irq_regs_reset(display, GEN8_DE_PORT_IRQ_REGS);
> + intel_display_irq_regs_reset(display, GEN8_DE_MISC_IRQ_REGS);
>
> if (DISPLAY_VER(dev_priv) >= 14)
> - gen2_irq_reset(uncore, PICAINTERRUPT_IRQ_REGS);
> + intel_display_irq_regs_reset(display, PICAINTERRUPT_IRQ_REGS);
> else
> - gen2_irq_reset(uncore, GEN11_DE_HPD_IRQ_REGS);
> + intel_display_irq_regs_reset(display, GEN11_DE_HPD_IRQ_REGS);
>
> if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> - gen2_irq_reset(uncore, SDE_IRQ_REGS);
> + intel_display_irq_regs_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 +1692,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_display_irq_regs_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 +1702,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 +1713,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_display_irq_regs_reset(display, GEN8_DE_PIPE_IRQ_REGS(pipe));
>
> spin_unlock_irq(&dev_priv->irq_lock);
>
> @@ -1697,7 +1734,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 +1747,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_display_irq_regs_init(display, SDE_IRQ_REGS, ~mask, 0xffffffff);
> }
>
> void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv)
> @@ -1743,7 +1780,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 +1804,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_display_irq_regs_assert_irr_is_zero(display, EDP_PSR_IIR);
> display_mask |= DE_EDP_PSR_INT_HSW;
> }
>
> @@ -1778,8 +1815,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_display_irq_regs_init(display, DE_IRQ_REGS, i915->irq_mask,
> + display_mask | extra_mask);
> }
>
> static void mtp_irq_postinstall(struct drm_i915_private *i915);
> @@ -1788,7 +1825,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 +1890,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_display_irq_regs_assert_irr_is_zero(display,
> + TRANS_PSR_IIR(dev_priv, trans));
> }
> } else {
> - gen2_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> + intel_display_irq_regs_assert_irr_is_zero(display, EDP_PSR_IIR);
> }
>
> for_each_pipe(dev_priv, pipe) {
> @@ -1866,44 +1902,46 @@ 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_display_irq_regs_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_display_irq_regs_init(display, GEN8_DE_PORT_IRQ_REGS, ~de_port_masked,
> + de_port_enables);
> + intel_display_irq_regs_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_display_irq_regs_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_display_irq_regs_init(display, PICAINTERRUPT_IRQ_REGS, ~de_hpd_mask,
> + de_hpd_enables);
>
> - gen2_irq_init(uncore, SDE_IRQ_REGS, ~sde_mask, 0xffffffff);
> + intel_display_irq_regs_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_display_irq_regs_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-14 9:55 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-13 20:38 [PATCH v2 0/3] drm/i915/dmc_wl: Track pipe interrupt registers Gustavo Sousa
2025-01-13 20:38 ` [PATCH v2 1/3] drm/i915/display: Use display MMIO functions in intel_display_irq.c Gustavo Sousa
2025-01-13 20:38 ` [PATCH v2 2/3] drm/i915/display: Wrap IRQ-specific uncore functions Gustavo Sousa
2025-01-14 8:35 ` Luca Coelho
2025-01-14 9:55 ` Jani Nikula [this message]
2025-01-14 10:56 ` Gustavo Sousa
2025-01-14 11:40 ` Jani Nikula
2025-01-13 20:38 ` [PATCH v2 3/3] drm/i915/dmc_wl: Track pipe interrupt registers Gustavo Sousa
2025-01-13 21:15 ` ✗ Fi.CI.SPARSE: warning for drm/i915/dmc_wl: Track pipe interrupt registers (rev2) Patchwork
2025-01-13 21:27 ` ✓ CI.Patch_applied: success " Patchwork
2025-01-13 21:27 ` ✓ CI.checkpatch: " Patchwork
2025-01-13 21:28 ` ✓ CI.KUnit: " Patchwork
2025-01-13 21:42 ` ✗ i915.CI.BAT: failure " Patchwork
2025-01-13 21:46 ` ✓ CI.Build: success " Patchwork
2025-01-13 21:48 ` ✓ CI.Hooks: " Patchwork
2025-01-13 21:50 ` ✓ CI.checksparse: " Patchwork
2025-01-13 22:18 ` ✓ Xe.CI.BAT: " Patchwork
2025-01-14 12:50 ` ✗ Xe.CI.Full: failure " Patchwork
2025-01-16 20:55 ` Gustavo Sousa
2025-01-14 13:52 ` ✗ Fi.CI.SPARSE: warning for drm/i915/dmc_wl: Track pipe interrupt registers (rev3) Patchwork
2025-01-14 14:12 ` ✗ i915.CI.BAT: failure " Patchwork
2025-01-14 16:40 ` Gustavo Sousa
2025-01-15 7:20 ` Ravali, JupallyX
2025-01-15 7:07 ` ✓ i915.CI.BAT: success " Patchwork
2025-01-15 20:20 ` ✗ i915.CI.Full: failure " Patchwork
2025-01-16 19:48 ` Gustavo Sousa
2025-01-17 11:46 ` [PATCH v2 0/3] drm/i915/dmc_wl: Track pipe interrupt registers Gustavo Sousa
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=87v7uhhg8n.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=gustavo.sousa@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jouni.hogander@intel.com \
/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.