From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 96315CCFA1A for ; Tue, 11 Nov 2025 15:48:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 417D710E343; Tue, 11 Nov 2025 15:48:38 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="R/AJVd6x"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by gabe.freedesktop.org (Postfix) with ESMTPS id CE79B10E343; Tue, 11 Nov 2025 15:48:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1762876117; x=1794412117; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=CbpqNAVHnWxOyT0+VFB+OZAKKcTz1My2FECAzp66hks=; b=R/AJVd6xxbBXkwBLiqclv3rJS2a6bwkpz/rnav+3I0PhNSwxxka4s2kW ReZIwkngXvawfor7tx0U8PnRhrX0Hh722ynIA/GrphbXGlGFyAPt8yuma gAHPDMi/+mr0RqsPOZHiZicH0tQuATDhD4vzUXRV1dIuF3d1btg2EbXP+ vnvHnq6IhnLAx/iC1PkZYZG71Oj2GBqiDlvq6hg67upzQtluWVJlO9hkH ss+aaRCt4v2thvWgOWXdUBoZy37rWUtcLgEtKpBNv1hu475QQlOtSNU4a iPCkXupiR8mJvD2BTSpuGxP9p4M0CDOTiHlQj89rITeMINK7lKyIwFReU A==; X-CSE-ConnectionGUID: WmY1PrjoSke95WuU2ddomA== X-CSE-MsgGUID: gcFxP4eMRk6OhfD54IGhtQ== X-IronPort-AV: E=McAfee;i="6800,10657,11610"; a="64978533" X-IronPort-AV: E=Sophos;i="6.19,296,1754982000"; d="scan'208";a="64978533" Received: from fmviesa007.fm.intel.com ([10.60.135.147]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2025 07:48:35 -0800 X-CSE-ConnectionGUID: KfowMALoS4m5ADJbeNM98g== X-CSE-MsgGUID: 9vMKywFgRQuWsaQorq+3BQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.19,296,1754982000"; d="scan'208";a="188636662" Received: from bjrankin-mobl3.amr.corp.intel.com (HELO localhost) ([10.124.223.47]) by fmviesa007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2025 07:48:34 -0800 Date: Tue, 11 Nov 2025 17:48:31 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org Subject: Re: [PATCH v2 1/3] drm/{i915,xe}/display: duplicate gen2 irq/error init/reset in display irq Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Tue, Nov 11, 2025 at 09:34:02AM +0200, Jani Nikula wrote: > Duplicate gen2_irq_reset(), gen2_assert_iir_is_zero(), gen2_irq_init(), > gen2_error_reset(), and gen2_error_init() in intel_display_irq.c. > > This allows us to drop the duplicates from xe, and prepares for future > cleanups. Although duplication is undesirable in general, in this case > the local duplicates lead to a cleaner end result. > > There's a slight wrinkle in gen2_assert_iir_is_zero(). We need to use > non-device based logging until we pass in struct intel_display in a > separate change. > > v2: > - Keep xe compat stuff due to series reorder and rebase > - Keep the WARN as regular WARN > - Rename the functions in the same go > > Suggested-by: Ville Syrjala > Signed-off-by: Jani Nikula Reviewed-by: Ville Syrjälä > > --- > > Note: 'git show --color-moved' helps review > --- > .../gpu/drm/i915/display/intel_display_irq.c | 82 +++++++++++++++++-- > drivers/gpu/drm/xe/display/ext/i915_irq.c | 67 --------------- > 2 files changed, 73 insertions(+), 76 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c > index 43b27deb4a26..acfaff13c3ba 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c > @@ -33,6 +33,72 @@ > #include "intel_psr_regs.h" > #include "intel_uncore.h" > > +static void irq_reset(struct intel_uncore *uncore, struct i915_irq_regs regs) > +{ > + intel_uncore_write(uncore, regs.imr, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.imr); > + > + intel_uncore_write(uncore, regs.ier, 0); > + > + /* IIR can theoretically queue up two events. Be paranoid. */ > + intel_uncore_write(uncore, regs.iir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.iir); > + intel_uncore_write(uncore, regs.iir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.iir); > +} > + > +/* > + * We should clear IMR at preinstall/uninstall, and just check at postinstall. > + */ > +static void assert_iir_is_zero(struct intel_uncore *uncore, i915_reg_t reg) > +{ > + u32 val = intel_uncore_read(uncore, reg); > + > + if (val == 0) > + return; > + > + WARN(1, > + "Interrupt register 0x%x is not zero: 0x%08x\n", > + i915_mmio_reg_offset(reg), val); > + intel_uncore_write(uncore, reg, 0xffffffff); > + intel_uncore_posting_read(uncore, reg); > + intel_uncore_write(uncore, reg, 0xffffffff); > + intel_uncore_posting_read(uncore, reg); > +} > + > +static void irq_init(struct intel_uncore *uncore, struct i915_irq_regs regs, > + u32 imr_val, u32 ier_val) > +{ > + assert_iir_is_zero(uncore, regs.iir); > + > + intel_uncore_write(uncore, regs.ier, ier_val); > + intel_uncore_write(uncore, regs.imr, imr_val); > + intel_uncore_posting_read(uncore, regs.imr); > +} > + > +static void error_reset(struct intel_uncore *uncore, struct i915_error_regs regs) > +{ > + intel_uncore_write(uncore, regs.emr, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.emr); > + > + intel_uncore_write(uncore, regs.eir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.eir); > + intel_uncore_write(uncore, regs.eir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.eir); > +} > + > +static void error_init(struct intel_uncore *uncore, struct i915_error_regs regs, > + u32 emr_val) > +{ > + intel_uncore_write(uncore, regs.eir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.eir); > + intel_uncore_write(uncore, regs.eir, 0xffffffff); > + intel_uncore_posting_read(uncore, regs.eir); > + > + intel_uncore_write(uncore, regs.emr, emr_val); > + intel_uncore_posting_read(uncore, regs.emr); > +} > + > static void > intel_display_irq_regs_init(struct intel_display *display, struct i915_irq_regs regs, > u32 imr_val, u32 ier_val) > @@ -41,7 +107,7 @@ intel_display_irq_regs_init(struct intel_display *display, struct i915_irq_regs > 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); > + 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); > @@ -55,7 +121,7 @@ intel_display_irq_regs_reset(struct intel_display *display, struct i915_irq_regs > intel_dmc_wl_get(display, regs.ier); > intel_dmc_wl_get(display, regs.iir); > > - gen2_irq_reset(to_intel_uncore(display->drm), regs); > + irq_reset(to_intel_uncore(display->drm), regs); > > intel_dmc_wl_put(display, regs.iir); > intel_dmc_wl_put(display, regs.ier); > @@ -67,7 +133,7 @@ intel_display_irq_regs_assert_irr_is_zero(struct intel_display *display, i915_re > { > intel_dmc_wl_get(display, reg); > > - gen2_assert_iir_is_zero(to_intel_uncore(display->drm), reg); > + assert_iir_is_zero(to_intel_uncore(display->drm), reg); > > intel_dmc_wl_put(display, reg); > } > @@ -1918,8 +1984,7 @@ static void _vlv_display_irq_reset(struct intel_display *display) > else > intel_de_write(display, DPINVGTT, DPINVGTT_STATUS_MASK_VLV); > > - gen2_error_reset(to_intel_uncore(display->drm), > - VLV_ERROR_REGS); > + error_reset(to_intel_uncore(display->drm), VLV_ERROR_REGS); > > i915_hotplug_interrupt_update_locked(display, 0xffffffff, 0); > intel_de_rmw(display, PORT_HOTPLUG_STAT(display), 0, 0); > @@ -2014,8 +2079,7 @@ static void _vlv_display_irq_postinstall(struct intel_display *display) > DPINVGTT_STATUS_MASK_VLV | > DPINVGTT_EN_MASK_VLV); > > - gen2_error_init(to_intel_uncore(display->drm), > - VLV_ERROR_REGS, ~vlv_error_mask()); > + error_init(to_intel_uncore(display->drm), VLV_ERROR_REGS, ~vlv_error_mask()); > > pipestat_mask = PIPE_CRC_DONE_INTERRUPT_STATUS; > > @@ -2054,7 +2118,7 @@ static void ibx_display_irq_reset(struct intel_display *display) > if (HAS_PCH_NOP(display)) > return; > > - gen2_irq_reset(to_intel_uncore(display->drm), SDE_IRQ_REGS); > + irq_reset(to_intel_uncore(display->drm), SDE_IRQ_REGS); > > if (HAS_PCH_CPT(display) || HAS_PCH_LPT(display)) > intel_de_write(display, SERR_INT, 0xffffffff); > @@ -2064,7 +2128,7 @@ void ilk_display_irq_reset(struct intel_display *display) > { > struct intel_uncore *uncore = to_intel_uncore(display->drm); > > - gen2_irq_reset(uncore, DE_IRQ_REGS); > + irq_reset(uncore, DE_IRQ_REGS); > display->irq.ilk_de_imr_mask = ~0u; > > if (DISPLAY_VER(display) == 7) > diff --git a/drivers/gpu/drm/xe/display/ext/i915_irq.c b/drivers/gpu/drm/xe/display/ext/i915_irq.c > index 3c6bca66ddab..1011c1c754d0 100644 > --- a/drivers/gpu/drm/xe/display/ext/i915_irq.c > +++ b/drivers/gpu/drm/xe/display/ext/i915_irq.c > @@ -7,73 +7,6 @@ > #include "i915_reg.h" > #include "intel_uncore.h" > > -void gen2_irq_reset(struct intel_uncore *uncore, struct i915_irq_regs regs) > -{ > - intel_uncore_write(uncore, regs.imr, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.imr); > - > - intel_uncore_write(uncore, regs.ier, 0); > - > - /* IIR can theoretically queue up two events. Be paranoid. */ > - intel_uncore_write(uncore, regs.iir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.iir); > - intel_uncore_write(uncore, regs.iir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.iir); > -} > - > -/* > - * We should clear IMR at preinstall/uninstall, and just check at postinstall. > - */ > -void gen2_assert_iir_is_zero(struct intel_uncore *uncore, i915_reg_t reg) > -{ > - struct xe_device *xe = container_of(uncore, struct xe_device, uncore); > - u32 val = intel_uncore_read(uncore, reg); > - > - if (val == 0) > - return; > - > - drm_WARN(&xe->drm, 1, > - "Interrupt register 0x%x is not zero: 0x%08x\n", > - i915_mmio_reg_offset(reg), val); > - intel_uncore_write(uncore, reg, 0xffffffff); > - intel_uncore_posting_read(uncore, reg); > - intel_uncore_write(uncore, reg, 0xffffffff); > - intel_uncore_posting_read(uncore, reg); > -} > - > -void gen2_irq_init(struct intel_uncore *uncore, struct i915_irq_regs regs, > - u32 imr_val, u32 ier_val) > -{ > - gen2_assert_iir_is_zero(uncore, regs.iir); > - > - intel_uncore_write(uncore, regs.ier, ier_val); > - intel_uncore_write(uncore, regs.imr, imr_val); > - intel_uncore_posting_read(uncore, regs.imr); > -} > - > -void gen2_error_reset(struct intel_uncore *uncore, struct i915_error_regs regs) > -{ > - intel_uncore_write(uncore, regs.emr, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.emr); > - > - intel_uncore_write(uncore, regs.eir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.eir); > - intel_uncore_write(uncore, regs.eir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.eir); > -} > - > -void gen2_error_init(struct intel_uncore *uncore, struct i915_error_regs regs, > - u32 emr_val) > -{ > - intel_uncore_write(uncore, regs.eir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.eir); > - intel_uncore_write(uncore, regs.eir, 0xffffffff); > - intel_uncore_posting_read(uncore, regs.eir); > - > - intel_uncore_write(uncore, regs.emr, emr_val); > - intel_uncore_posting_read(uncore, regs.emr); > -} > - > bool intel_irqs_enabled(struct xe_device *xe) > { > return atomic_read(&xe->irq.enabled); > -- > 2.47.3 -- Ville Syrjälä Intel