[03/14] drm/i915/dsb: Hook up DSB error interrupts

Message ID	20240624191032.27333-4-ville.syrjala@linux.intel.com (mailing list archive)
State	New
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Ville Syrjala <ville.syrjala@linux.intel.com> To: intel-gfx@lists.freedesktop.org Subject: [PATCH 03/14] drm/i915/dsb: Hook up DSB error interrupts Date: Mon, 24 Jun 2024 22:10:21 +0300 Message-ID: <20240624191032.27333-4-ville.syrjala@linux.intel.com> In-Reply-To: <20240624191032.27333-1-ville.syrjala@linux.intel.com> References: <20240624191032.27333-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	drm/i915/dsb: Use chained DSBs for LUT programming \| expand [00/14] drm/i915/dsb: Use chained DSBs for LUT programming [01/14] drm/i915: Calculate vblank delay more accurately [02/14] drm/i915: Make vrr_{enabling, disabling}() usable outside intel_display.c [03/14] drm/i915/dsb: Hook up DSB error interrupts [04/14] drm/i915/dsb: Convert dewake_scanline to a hw scanline number earlier [05/14] drm/i915/dsb: Shuffle code around [06/14] drm/i915/dsb: Fix dewake scanline [07/14] drm/i915/dsb: Account for VRR properly in DSB scanline stuff [08/14] drm/i915/dsb: Precompute DSB_CHICKEN [09/14] drm/i915/dsb: Introduce intel_dsb_wait_scanline_{in, out}() [10/14] drm/i915/dsb: Introduce intel_dsb_chain() [11/14] drm/i915/dsb: Allow intel_dsb_chain() to use DSB_WAIT_FOR_VBLANK [12/14] drm/i915/dsb: Clear DSB_ENABLE_DEWAKE once the DSB is done [13/14] drm/i915/dsb: s/dsb/dsb_color_vblank/ [14/14] drm/i915/dsb: Use chained DSBs for LUT programming

Message ID

20240624191032.27333-4-ville.syrjala@linux.intel.com (mailing list archive)

State

New

Headers

From: Ville Syrjala <ville.syrjala@linux.intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: [PATCH 03/14] drm/i915/dsb: Hook up DSB error interrupts
Date: Mon, 24 Jun 2024 22:10:21 +0300
Message-ID: <20240624191032.27333-4-ville.syrjala@linux.intel.com>
In-Reply-To: <20240624191032.27333-1-ville.syrjala@linux.intel.com>
References: <20240624191032.27333-1-ville.syrjala@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: list
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Series

drm/i915/dsb: Use chained DSBs for LUT programming | expand

Commit Message

Ville Syrjälä June 24, 2024, 7:10 p.m. UTC

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Enable all DSB error/fault interrupts so that we can see if
anything goes terribly wrong.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_display_irq.c  | 17 ++++++
 drivers/gpu/drm/i915/display/intel_dsb.c      | 58 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h      |  6 ++
 drivers/gpu/drm/i915/i915_reg.h               |  4 ++
 4 files changed, 85 insertions(+)

Comments

Jani Nikula June 25, 2024, 7:58 a.m. UTC | #1

On Mon, 24 Jun 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Enable all DSB error/fault interrupts so that we can see if
> anything goes terribly wrong.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 17 ++++++
>  drivers/gpu/drm/i915/display/intel_dsb.c      | 58 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsb.h      |  6 ++
>  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
>  4 files changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 5219ba295c74..7169db984651 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -14,6 +14,7 @@
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_dp_aux.h"
> +#include "intel_dsb.h"
>  #include "intel_fdi_regs.h"
>  #include "intel_fifo_underrun.h"
>  #include "intel_gmbus.h"
> @@ -1143,6 +1144,17 @@ void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  
>  		intel_uncore_write(&dev_priv->uncore, GEN8_DE_PIPE_IIR(pipe), iir);
>  
> +		if (HAS_DSB(dev_priv)) {
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_0))
> +				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_0);
> +
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_1))
> +				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_1);
> +
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_2))
> +				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_2);
> +		}
> +
>  		if (iir & GEN8_PIPE_VBLANK)
>  			intel_handle_vblank(dev_priv, pipe);
>  
> @@ -1718,6 +1730,11 @@ void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  			de_port_masked |= DSI0_TE | DSI1_TE;
>  	}
>  
> +	if (HAS_DSB(dev_priv))
> +		de_pipe_masked |= GEN12_DSB_INT(INTEL_DSB_0) |
> +			GEN12_DSB_INT(INTEL_DSB_1) |
> +			GEN12_DSB_INT(INTEL_DSB_2);
> +
>  	de_pipe_enables = de_pipe_masked |
>  		GEN8_PIPE_VBLANK |
>  		gen8_de_pipe_underrun_mask(dev_priv) |
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2ab3765f6c06..ded696363258 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -339,6 +339,42 @@ static u32 dsb_chicken(struct intel_crtc *crtc)
>  		return DSB_SKIP_WAITS_EN;
>  }
>  
> +static u32 dsb_error_int_status(struct intel_display *display)
> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);

I think this is redundant, you can just pass display to DISPLAY_VER().

> +	u32 errors;
> +
> +	errors = DSB_GTT_FAULT_INT_STATUS |
> +		DSB_RSPTIMEOUT_INT_STATUS |
> +		DSB_POLL_ERR_INT_STATUS;
> +
> +	/*
> +	 * All the non-existing status bits operate as
> +	 * normal r/w bits, so any attempt to clear them
> +	 * will just end up setting them. Never do that so
> +	 * we won't mistake them for actual error interrupts.
> +	 */
> +	if (DISPLAY_VER(i915) >= 14)
> +		errors |= DSB_ATS_FAULT_INT_STATUS;
> +
> +	return errors;
> +}
> +
> +static u32 dsb_error_int_en(struct intel_display *display)
> +{
> +	struct drm_i915_private *i915 = to_i915(display->drm);

Ditto.

> +	u32 errors;
> +
> +	errors = DSB_GTT_FAULT_INT_EN |
> +		DSB_RSPTIMEOUT_INT_EN |
> +		DSB_POLL_ERR_INT_EN;
> +
> +	if (DISPLAY_VER(i915) >= 14)
> +		errors |= DSB_ATS_FAULT_INT_EN;
> +
> +	return errors;
> +}
> +
>  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
>  			      int dewake_scanline)
>  {
> @@ -363,6 +399,10 @@ static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
>  	intel_de_write_fw(display, DSB_CHICKEN(pipe, dsb->id),
>  			  dsb_chicken(crtc));
>  
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> +			  dsb_error_int_status(display) | DSB_PROG_INT_STATUS |
> +			  dsb_error_int_en(display));
> +
>  	intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
>  			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
>  
> @@ -430,6 +470,9 @@ void intel_dsb_wait(struct intel_dsb *dsb)
>  	dsb->free_pos = 0;
>  	dsb->ins_start_offset = 0;
>  	intel_de_write_fw(display, DSB_CTRL(pipe, dsb->id), 0);
> +
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> +			  dsb_error_int_status(display) | DSB_PROG_INT_STATUS);
>  }
>  
>  /**
> @@ -513,3 +556,18 @@ void intel_dsb_cleanup(struct intel_dsb *dsb)
>  	intel_dsb_buffer_cleanup(&dsb->dsb_buf);
>  	kfree(dsb);
>  }
> +
> +void intel_dsb_irq_handler(struct intel_display *display,
> +			   enum pipe pipe, enum intel_dsb_id dsb_id)
> +{
> +	struct intel_crtc *crtc = intel_crtc_for_pipe(to_i915(display->drm), pipe);
> +	u32 tmp, errors;
> +
> +	tmp = intel_de_read_fw(display, DSB_INTERRUPT(pipe, dsb_id));
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb_id), tmp);
> +
> +	errors = tmp & dsb_error_int_status(display);
> +	if (errors)
> +		drm_err(display->drm, "[CRTC:%d:%s] / DSB %d error interrupt: 0x%x\n",
> +			crtc->base.base.id, crtc->base.name, dsb_id, errors);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index bb42749f2ea4..84fc2f8434d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -13,8 +13,11 @@
>  struct intel_atomic_state;
>  struct intel_crtc;
>  struct intel_crtc_state;
> +struct intel_display;
>  struct intel_dsb;
>  
> +enum pipe;
> +
>  enum intel_dsb_id {
>  	INTEL_DSB_0,
>  	INTEL_DSB_1,
> @@ -41,4 +44,7 @@ void intel_dsb_commit(struct intel_dsb *dsb,
>  		      bool wait_for_vblank);
>  void intel_dsb_wait(struct intel_dsb *dsb);
>  
> +void intel_dsb_irq_handler(struct intel_display *display,
> +			   enum pipe pipe, enum intel_dsb_id dsb_id);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0e3d79227e3c..49a9761ca313 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2515,6 +2515,10 @@
>  #define  GEN11_PIPE_PLANE7_FLIP_DONE	REG_BIT(18) /* icl/tgl */
>  #define  GEN11_PIPE_PLANE6_FLIP_DONE	REG_BIT(17) /* icl/tgl */
>  #define  GEN11_PIPE_PLANE5_FLIP_DONE	REG_BIT(16) /* icl+ */
> +#define  GEN12_DSB_2_INT		REG_BIT(15) /* tgl+ */
> +#define  GEN12_DSB_1_INT		REG_BIT(14) /* tgl+ */
> +#define  GEN12_DSB_0_INT		REG_BIT(13) /* tgl+ */
> +#define  GEN12_DSB_INT(dsb_id)		REG_BIT(13 + (dsb_id))
>  #define  GEN9_PIPE_CURSOR_FAULT		REG_BIT(11) /* skl+ */
>  #define  GEN9_PIPE_PLANE4_FAULT		REG_BIT(10) /* skl+ */
>  #define  GEN8_PIPE_CURSOR_FAULT		REG_BIT(10) /* bdw */

Manna, Animesh June 28, 2024, 9:21 a.m. UTC | #2

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Tuesday, June 25, 2024 12:40 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [PATCH 03/14] drm/i915/dsb: Hook up DSB error interrupts
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Enable all DSB error/fault interrupts so that we can see if anything goes
> terribly wrong.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 17 ++++++
>  drivers/gpu/drm/i915/display/intel_dsb.c      | 58 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsb.h      |  6 ++
>  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
>  4 files changed, 85 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 5219ba295c74..7169db984651 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -14,6 +14,7 @@
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_dp_aux.h"
> +#include "intel_dsb.h"
>  #include "intel_fdi_regs.h"
>  #include "intel_fifo_underrun.h"
>  #include "intel_gmbus.h"
> @@ -1143,6 +1144,17 @@ void gen8_de_irq_handler(struct
> drm_i915_private *dev_priv, u32 master_ctl)
> 
>  		intel_uncore_write(&dev_priv->uncore,
> GEN8_DE_PIPE_IIR(pipe), iir);
> 
> +		if (HAS_DSB(dev_priv)) {
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_0))
> +				intel_dsb_irq_handler(&dev_priv->display,
> pipe, INTEL_DSB_0);
> +
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_1))
> +				intel_dsb_irq_handler(&dev_priv->display,
> pipe, INTEL_DSB_1);
> +
> +			if (iir & GEN12_DSB_INT(INTEL_DSB_2))
> +				intel_dsb_irq_handler(&dev_priv->display,
> pipe, INTEL_DSB_2);
> +		}
> +
>  		if (iir & GEN8_PIPE_VBLANK)
>  			intel_handle_vblank(dev_priv, pipe);
> 
> @@ -1718,6 +1730,11 @@ void gen8_de_irq_postinstall(struct
> drm_i915_private *dev_priv)
>  			de_port_masked |= DSI0_TE | DSI1_TE;
>  	}
> 
> +	if (HAS_DSB(dev_priv))
> +		de_pipe_masked |= GEN12_DSB_INT(INTEL_DSB_0) |
> +			GEN12_DSB_INT(INTEL_DSB_1) |
> +			GEN12_DSB_INT(INTEL_DSB_2);
> +
>  	de_pipe_enables = de_pipe_masked |
>  		GEN8_PIPE_VBLANK |
>  		gen8_de_pipe_underrun_mask(dev_priv) | diff --git
> a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2ab3765f6c06..ded696363258 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -339,6 +339,42 @@ static u32 dsb_chicken(struct intel_crtc *crtc)
>  		return DSB_SKIP_WAITS_EN;
>  }
> 
> +static u32 dsb_error_int_status(struct intel_display *display) {
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	u32 errors;
> +
> +	errors = DSB_GTT_FAULT_INT_STATUS |
> +		DSB_RSPTIMEOUT_INT_STATUS |
> +		DSB_POLL_ERR_INT_STATUS;
> +
> +	/*
> +	 * All the non-existing status bits operate as
> +	 * normal r/w bits, so any attempt to clear them
> +	 * will just end up setting them. Never do that so
> +	 * we won't mistake them for actual error interrupts.
> +	 */
> +	if (DISPLAY_VER(i915) >= 14)
> +		errors |= DSB_ATS_FAULT_INT_STATUS;
> +
> +	return errors;
> +}
> +
> +static u32 dsb_error_int_en(struct intel_display *display) {
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +	u32 errors;
> +
> +	errors = DSB_GTT_FAULT_INT_EN |
> +		DSB_RSPTIMEOUT_INT_EN |
> +		DSB_POLL_ERR_INT_EN;
> +
> +	if (DISPLAY_VER(i915) >= 14)
> +		errors |= DSB_ATS_FAULT_INT_EN;
> +
> +	return errors;
> +}
> +
>  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
>  			      int dewake_scanline)
>  {
> @@ -363,6 +399,10 @@ static void _intel_dsb_commit(struct intel_dsb *dsb,
> u32 ctrl,
>  	intel_de_write_fw(display, DSB_CHICKEN(pipe, dsb->id),
>  			  dsb_chicken(crtc));
> 
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> +			  dsb_error_int_status(display) |
> DSB_PROG_INT_STATUS |
> +			  dsb_error_int_en(display));
> +
>  	intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
>  			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
> 
> @@ -430,6 +470,9 @@ void intel_dsb_wait(struct intel_dsb *dsb)
>  	dsb->free_pos = 0;
>  	dsb->ins_start_offset = 0;
>  	intel_de_write_fw(display, DSB_CTRL(pipe, dsb->id), 0);
> +
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> +			  dsb_error_int_status(display) |
> DSB_PROG_INT_STATUS);
>  }
> 
>  /**
> @@ -513,3 +556,18 @@ void intel_dsb_cleanup(struct intel_dsb *dsb)
>  	intel_dsb_buffer_cleanup(&dsb->dsb_buf);
>  	kfree(dsb);
>  }
> +
> +void intel_dsb_irq_handler(struct intel_display *display,
> +			   enum pipe pipe, enum intel_dsb_id dsb_id) {
> +	struct intel_crtc *crtc = intel_crtc_for_pipe(to_i915(display->drm),
> pipe);
> +	u32 tmp, errors;
> +
> +	tmp = intel_de_read_fw(display, DSB_INTERRUPT(pipe, dsb_id));
> +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb_id), tmp);
> +
> +	errors = tmp & dsb_error_int_status(display);
> +	if (errors)
> +		drm_err(display->drm, "[CRTC:%d:%s] / DSB %d error
> interrupt: 0x%x\n",
> +			crtc->base.base.id, crtc->base.name, dsb_id, errors);

Just a nitpick: maybe good to print the errors in a string format instead of hex value.

Regards,
Animesh

> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h
> b/drivers/gpu/drm/i915/display/intel_dsb.h
> index bb42749f2ea4..84fc2f8434d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -13,8 +13,11 @@
>  struct intel_atomic_state;
>  struct intel_crtc;
>  struct intel_crtc_state;
> +struct intel_display;
>  struct intel_dsb;
> 
> +enum pipe;
> +
>  enum intel_dsb_id {
>  	INTEL_DSB_0,
>  	INTEL_DSB_1,
> @@ -41,4 +44,7 @@ void intel_dsb_commit(struct intel_dsb *dsb,
>  		      bool wait_for_vblank);
>  void intel_dsb_wait(struct intel_dsb *dsb);
> 
> +void intel_dsb_irq_handler(struct intel_display *display,
> +			   enum pipe pipe, enum intel_dsb_id dsb_id);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 0e3d79227e3c..49a9761ca313
> 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2515,6 +2515,10 @@
>  #define  GEN11_PIPE_PLANE7_FLIP_DONE	REG_BIT(18) /* icl/tgl */
>  #define  GEN11_PIPE_PLANE6_FLIP_DONE	REG_BIT(17) /* icl/tgl */
>  #define  GEN11_PIPE_PLANE5_FLIP_DONE	REG_BIT(16) /* icl+ */
> +#define  GEN12_DSB_2_INT		REG_BIT(15) /* tgl+ */
> +#define  GEN12_DSB_1_INT		REG_BIT(14) /* tgl+ */
> +#define  GEN12_DSB_0_INT		REG_BIT(13) /* tgl+ */
> +#define  GEN12_DSB_INT(dsb_id)		REG_BIT(13 + (dsb_id))
>  #define  GEN9_PIPE_CURSOR_FAULT		REG_BIT(11) /* skl+ */
>  #define  GEN9_PIPE_PLANE4_FAULT		REG_BIT(10) /* skl+ */
>  #define  GEN8_PIPE_CURSOR_FAULT		REG_BIT(10) /* bdw */
> --
> 2.44.2

Ville Syrjälä June 28, 2024, 11:04 a.m. UTC | #3

On Fri, Jun 28, 2024 at 09:21:24AM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Tuesday, June 25, 2024 12:40 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH 03/14] drm/i915/dsb: Hook up DSB error interrupts
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Enable all DSB error/fault interrupts so that we can see if anything goes
> > terribly wrong.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_irq.c  | 17 ++++++
> >  drivers/gpu/drm/i915/display/intel_dsb.c      | 58 +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dsb.h      |  6 ++
> >  drivers/gpu/drm/i915/i915_reg.h               |  4 ++
> >  4 files changed, 85 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > index 5219ba295c74..7169db984651 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > @@ -14,6 +14,7 @@
> >  #include "intel_display_trace.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_aux.h"
> > +#include "intel_dsb.h"
> >  #include "intel_fdi_regs.h"
> >  #include "intel_fifo_underrun.h"
> >  #include "intel_gmbus.h"
> > @@ -1143,6 +1144,17 @@ void gen8_de_irq_handler(struct
> > drm_i915_private *dev_priv, u32 master_ctl)
> > 
> >  		intel_uncore_write(&dev_priv->uncore,
> > GEN8_DE_PIPE_IIR(pipe), iir);
> > 
> > +		if (HAS_DSB(dev_priv)) {
> > +			if (iir & GEN12_DSB_INT(INTEL_DSB_0))
> > +				intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_0);
> > +
> > +			if (iir & GEN12_DSB_INT(INTEL_DSB_1))
> > +				intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_1);
> > +
> > +			if (iir & GEN12_DSB_INT(INTEL_DSB_2))
> > +				intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_2);
> > +		}
> > +
> >  		if (iir & GEN8_PIPE_VBLANK)
> >  			intel_handle_vblank(dev_priv, pipe);
> > 
> > @@ -1718,6 +1730,11 @@ void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  			de_port_masked |= DSI0_TE | DSI1_TE;
> >  	}
> > 
> > +	if (HAS_DSB(dev_priv))
> > +		de_pipe_masked |= GEN12_DSB_INT(INTEL_DSB_0) |
> > +			GEN12_DSB_INT(INTEL_DSB_1) |
> > +			GEN12_DSB_INT(INTEL_DSB_2);
> > +
> >  	de_pipe_enables = de_pipe_masked |
> >  		GEN8_PIPE_VBLANK |
> >  		gen8_de_pipe_underrun_mask(dev_priv) | diff --git
> > a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index 2ab3765f6c06..ded696363258 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -339,6 +339,42 @@ static u32 dsb_chicken(struct intel_crtc *crtc)
> >  		return DSB_SKIP_WAITS_EN;
> >  }
> > 
> > +static u32 dsb_error_int_status(struct intel_display *display) {
> > +	struct drm_i915_private *i915 = to_i915(display->drm);
> > +	u32 errors;
> > +
> > +	errors = DSB_GTT_FAULT_INT_STATUS |
> > +		DSB_RSPTIMEOUT_INT_STATUS |
> > +		DSB_POLL_ERR_INT_STATUS;
> > +
> > +	/*
> > +	 * All the non-existing status bits operate as
> > +	 * normal r/w bits, so any attempt to clear them
> > +	 * will just end up setting them. Never do that so
> > +	 * we won't mistake them for actual error interrupts.
> > +	 */
> > +	if (DISPLAY_VER(i915) >= 14)
> > +		errors |= DSB_ATS_FAULT_INT_STATUS;
> > +
> > +	return errors;
> > +}
> > +
> > +static u32 dsb_error_int_en(struct intel_display *display) {
> > +	struct drm_i915_private *i915 = to_i915(display->drm);
> > +	u32 errors;
> > +
> > +	errors = DSB_GTT_FAULT_INT_EN |
> > +		DSB_RSPTIMEOUT_INT_EN |
> > +		DSB_POLL_ERR_INT_EN;
> > +
> > +	if (DISPLAY_VER(i915) >= 14)
> > +		errors |= DSB_ATS_FAULT_INT_EN;
> > +
> > +	return errors;
> > +}
> > +
> >  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
> >  			      int dewake_scanline)
> >  {
> > @@ -363,6 +399,10 @@ static void _intel_dsb_commit(struct intel_dsb *dsb,
> > u32 ctrl,
> >  	intel_de_write_fw(display, DSB_CHICKEN(pipe, dsb->id),
> >  			  dsb_chicken(crtc));
> > 
> > +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> > +			  dsb_error_int_status(display) |
> > DSB_PROG_INT_STATUS |
> > +			  dsb_error_int_en(display));
> > +
> >  	intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
> >  			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
> > 
> > @@ -430,6 +470,9 @@ void intel_dsb_wait(struct intel_dsb *dsb)
> >  	dsb->free_pos = 0;
> >  	dsb->ins_start_offset = 0;
> >  	intel_de_write_fw(display, DSB_CTRL(pipe, dsb->id), 0);
> > +
> > +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> > +			  dsb_error_int_status(display) |
> > DSB_PROG_INT_STATUS);
> >  }
> > 
> >  /**
> > @@ -513,3 +556,18 @@ void intel_dsb_cleanup(struct intel_dsb *dsb)
> >  	intel_dsb_buffer_cleanup(&dsb->dsb_buf);
> >  	kfree(dsb);
> >  }
> > +
> > +void intel_dsb_irq_handler(struct intel_display *display,
> > +			   enum pipe pipe, enum intel_dsb_id dsb_id) {
> > +	struct intel_crtc *crtc = intel_crtc_for_pipe(to_i915(display->drm),
> > pipe);
> > +	u32 tmp, errors;
> > +
> > +	tmp = intel_de_read_fw(display, DSB_INTERRUPT(pipe, dsb_id));
> > +	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb_id), tmp);
> > +
> > +	errors = tmp & dsb_error_int_status(display);
> > +	if (errors)
> > +		drm_err(display->drm, "[CRTC:%d:%s] / DSB %d error
> > interrupt: 0x%x\n",
> > +			crtc->base.base.id, crtc->base.name, dsb_id, errors);
> 
> Just a nitpick: maybe good to print the errors in a string format instead of hex value.

Yeah that might be nice. But we don't do that for eg. pipe fault
errors either. Perhaps a nice little task for someone to go over
all of these and make them a bit more human friendly...

diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
index 5219ba295c74..7169db984651 100644
--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
@@ -14,6 +14,7 @@ 
 #include "intel_display_trace.h"
 #include "intel_display_types.h"
 #include "intel_dp_aux.h"
+#include "intel_dsb.h"
 #include "intel_fdi_regs.h"
 #include "intel_fifo_underrun.h"
 #include "intel_gmbus.h"
@@ -1143,6 +1144,17 @@  void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 
 		intel_uncore_write(&dev_priv->uncore, GEN8_DE_PIPE_IIR(pipe), iir);
 
+		if (HAS_DSB(dev_priv)) {
+			if (iir & GEN12_DSB_INT(INTEL_DSB_0))
+				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_0);
+
+			if (iir & GEN12_DSB_INT(INTEL_DSB_1))
+				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_1);
+
+			if (iir & GEN12_DSB_INT(INTEL_DSB_2))
+				intel_dsb_irq_handler(&dev_priv->display, pipe, INTEL_DSB_2);
+		}
+
 		if (iir & GEN8_PIPE_VBLANK)
 			intel_handle_vblank(dev_priv, pipe);
 
@@ -1718,6 +1730,11 @@  void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 			de_port_masked |= DSI0_TE | DSI1_TE;
 	}
 
+	if (HAS_DSB(dev_priv))
+		de_pipe_masked |= GEN12_DSB_INT(INTEL_DSB_0) |
+			GEN12_DSB_INT(INTEL_DSB_1) |
+			GEN12_DSB_INT(INTEL_DSB_2);
+
 	de_pipe_enables = de_pipe_masked |
 		GEN8_PIPE_VBLANK |
 		gen8_de_pipe_underrun_mask(dev_priv) |
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 2ab3765f6c06..ded696363258 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -339,6 +339,42 @@  static u32 dsb_chicken(struct intel_crtc *crtc)
 		return DSB_SKIP_WAITS_EN;
 }
 
+static u32 dsb_error_int_status(struct intel_display *display)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	u32 errors;
+
+	errors = DSB_GTT_FAULT_INT_STATUS |
+		DSB_RSPTIMEOUT_INT_STATUS |
+		DSB_POLL_ERR_INT_STATUS;
+
+	/*
+	 * All the non-existing status bits operate as
+	 * normal r/w bits, so any attempt to clear them
+	 * will just end up setting them. Never do that so
+	 * we won't mistake them for actual error interrupts.
+	 */
+	if (DISPLAY_VER(i915) >= 14)
+		errors |= DSB_ATS_FAULT_INT_STATUS;
+
+	return errors;
+}
+
+static u32 dsb_error_int_en(struct intel_display *display)
+{
+	struct drm_i915_private *i915 = to_i915(display->drm);
+	u32 errors;
+
+	errors = DSB_GTT_FAULT_INT_EN |
+		DSB_RSPTIMEOUT_INT_EN |
+		DSB_POLL_ERR_INT_EN;
+
+	if (DISPLAY_VER(i915) >= 14)
+		errors |= DSB_ATS_FAULT_INT_EN;
+
+	return errors;
+}
+
 static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
 			      int dewake_scanline)
 {
@@ -363,6 +399,10 @@  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
 	intel_de_write_fw(display, DSB_CHICKEN(pipe, dsb->id),
 			  dsb_chicken(crtc));
 
+	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
+			  dsb_error_int_status(display) | DSB_PROG_INT_STATUS |
+			  dsb_error_int_en(display));
+
 	intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
 			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
 
@@ -430,6 +470,9 @@  void intel_dsb_wait(struct intel_dsb *dsb)
 	dsb->free_pos = 0;
 	dsb->ins_start_offset = 0;
 	intel_de_write_fw(display, DSB_CTRL(pipe, dsb->id), 0);
+
+	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
+			  dsb_error_int_status(display) | DSB_PROG_INT_STATUS);
 }
 
 /**
@@ -513,3 +556,18 @@  void intel_dsb_cleanup(struct intel_dsb *dsb)
 	intel_dsb_buffer_cleanup(&dsb->dsb_buf);
 	kfree(dsb);
 }
+
+void intel_dsb_irq_handler(struct intel_display *display,
+			   enum pipe pipe, enum intel_dsb_id dsb_id)
+{
+	struct intel_crtc *crtc = intel_crtc_for_pipe(to_i915(display->drm), pipe);
+	u32 tmp, errors;
+
+	tmp = intel_de_read_fw(display, DSB_INTERRUPT(pipe, dsb_id));
+	intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb_id), tmp);
+
+	errors = tmp & dsb_error_int_status(display);
+	if (errors)
+		drm_err(display->drm, "[CRTC:%d:%s] / DSB %d error interrupt: 0x%x\n",
+			crtc->base.base.id, crtc->base.name, dsb_id, errors);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index bb42749f2ea4..84fc2f8434d1 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -13,8 +13,11 @@ 
 struct intel_atomic_state;
 struct intel_crtc;
 struct intel_crtc_state;
+struct intel_display;
 struct intel_dsb;
 
+enum pipe;
+
 enum intel_dsb_id {
 	INTEL_DSB_0,
 	INTEL_DSB_1,
@@ -41,4 +44,7 @@  void intel_dsb_commit(struct intel_dsb *dsb,
 		      bool wait_for_vblank);
 void intel_dsb_wait(struct intel_dsb *dsb);
 
+void intel_dsb_irq_handler(struct intel_display *display,
+			   enum pipe pipe, enum intel_dsb_id dsb_id);
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0e3d79227e3c..49a9761ca313 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2515,6 +2515,10 @@ 
 #define  GEN11_PIPE_PLANE7_FLIP_DONE	REG_BIT(18) /* icl/tgl */
 #define  GEN11_PIPE_PLANE6_FLIP_DONE	REG_BIT(17) /* icl/tgl */
 #define  GEN11_PIPE_PLANE5_FLIP_DONE	REG_BIT(16) /* icl+ */
+#define  GEN12_DSB_2_INT		REG_BIT(15) /* tgl+ */
+#define  GEN12_DSB_1_INT		REG_BIT(14) /* tgl+ */
+#define  GEN12_DSB_0_INT		REG_BIT(13) /* tgl+ */
+#define  GEN12_DSB_INT(dsb_id)		REG_BIT(13 + (dsb_id))
 #define  GEN9_PIPE_CURSOR_FAULT		REG_BIT(11) /* skl+ */
 #define  GEN9_PIPE_PLANE4_FAULT		REG_BIT(10) /* skl+ */
 #define  GEN8_PIPE_CURSOR_FAULT		REG_BIT(10) /* bdw */

[03/14] drm/i915/dsb: Hook up DSB error interrupts

Commit Message

Comments

Patch