Re: [PATCH v3 22/26] drm/i915/writeback: Enable writeback interrupts

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 25, 2026 at 04:37:40PM +0530, Suraj Kandpal wrote:
> Enable writeback interrupts while enabling writeback
> and define the isr handler and schedule work for later
> to signal completion job.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 10 ++++
>  .../gpu/drm/i915/display/intel_display_regs.h |  1 +
>  .../gpu/drm/i915/display/intel_writeback.c    | 50 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_writeback.h    |  1 +
>  4 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 70c1bba7c0a8..656fb314b985 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -29,6 +29,8 @@
>  #include "intel_pmdemand.h"
>  #include "intel_psr.h"
>  #include "intel_psr_regs.h"
> +#include "intel_writeback.h"
> +#include "intel_writeback_reg.h"
>  
>  static void irq_reset(struct intel_display *display, struct i915_irq_regs regs)
>  {
> @@ -1281,6 +1283,11 @@ gen8_de_misc_irq_handler(struct intel_display *display, u32 iir)
>  		found = true;
>  	}
>  
> +	if (iir & (GEN8_DE_MISC_WD0)) {
> +		intel_writeback_isr_handler(display);
> +		found = true;
> +	}
> +
>  	if (iir & GEN8_DE_EDP_PSR) {
>  		struct intel_encoder *encoder;
>  		u32 psr_iir;
> @@ -2337,6 +2344,9 @@ void gen8_de_irq_postinstall(struct intel_display *display)
>  	if (DISPLAY_VER(display) < 11)
>  		de_misc_masked |= GEN8_DE_MISC_GSE;
>  
> +	if (DISPLAY_VER(display) >= 13)
> +		de_misc_masked |= GEN8_DE_MISC_WD0;
> +
>  	if (display->platform.geminilake || display->platform.broxton)
>  		de_port_masked |= BXT_DE_PORT_GMBUS;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
> index 4746e9ebd920..e637b10597c2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> @@ -1495,6 +1495,7 @@
>  #define  XELPDP_RM_TIMEOUT		REG_BIT(29)
>  #define  XELPDP_PMDEMAND_RSPTOUT_ERR	REG_BIT(27)
>  #define  GEN8_DE_MISC_GSE		REG_BIT(27)
> +#define  GEN8_DE_MISC_WD0		REG_BIT(23)
>  #define  GEN8_DE_EDP_PSR		REG_BIT(19)
>  #define  XELPDP_PMDEMAND_RSP		REG_BIT(3)
>  #define  XE2LPD_DBUF_OVERLAP_DETECTED	REG_BIT(1)
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.c b/drivers/gpu/drm/i915/display/intel_writeback.c
> index 54e74450e080..864d4a28de10 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.c
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.c
> @@ -14,6 +14,7 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_vblank.h>
>  
>  #include "intel_atomic.h"
>  #include "intel_connector.h"
> @@ -323,6 +324,20 @@ void intel_writeback_atomic_commit(struct intel_atomic_state *state)
>  	}
>  }
>  
> +static void
> +intel_writeback_enable_interrupts(struct intel_display *display,
> +				  enum transcoder trans)
> +{
> +	u32 tmp;
> +
> +	tmp = intel_de_read(display, WD_IIR(trans));
> +	intel_de_write_fw(display, WD_IIR(trans), tmp);
> +
> +	tmp = ~(WD_GTT_FAULT_INT | WD_WRITE_COMPLETE_INT |
> +		WD_VBLANK_INT | WD_CAPTURING_INT);
> +	intel_de_write(display, WD_IMR(trans), tmp);

If this is a double buffered IIR register then we really need to use the
i915_irq_regs stuff to do this properly.

> +}
> +
>  static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
>  					   struct intel_encoder *encoder,
>  					   const struct intel_crtc_state *crtc_state,
> @@ -348,6 +363,7 @@ static void intel_writeback_enable_encoder(struct intel_atomic_state *state,
>  	fb = job->fb;
>  	hactive = adjusted_mode->hdisplay;
>  	vactive = adjusted_mode->vdisplay;
> +	intel_writeback_enable_interrupts(display, trans);
>  
>  	/* Configure WD_STRIDE, WD_SURF and WD_TAIL_CFG */
>  	/* Enable Planes, Pipes and Transcoder */
> @@ -509,6 +525,40 @@ intel_writeback_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +void intel_writeback_isr_handler(struct intel_display *display)

"isr_handler" is not a term we use anywhere else.

> +{
> +	struct intel_encoder *encoder;
> +	struct intel_writeback_connector *wb_conn;
> +	struct intel_crtc *crtc;
> +	u32 iir;
> +
> +	for_each_intel_encoder(display->drm, encoder) {

We should already know which WD transcoder generated the interrupt.
Iterating all of them blindly doesn't seem right.

> +		if (encoder->type != INTEL_OUTPUT_WRITEBACK)
> +			continue;
> +
> +		wb_conn = enc_to_intel_writeback_connector(encoder);
> +		if (!wb_conn->job) {

The interrupt code shouldn't care about that.

> +			drm_err(display->drm, "No writeback job for the connector\n");
> +			continue;
> +		}
> +
> +		crtc = intel_crtc_for_pipe(display, wb_conn->pipe);

Hmm. The pipe assignment will be dynamic. So looks like this stuff
will need some actual thought...

> +		iir = intel_de_read(display, WD_IIR(wb_conn->trans));
> +		if (iir & WD_GTT_FAULT_INT)
> +			drm_err(display->drm, " GTT fault during writeback\n");

Missing at least the ATS fault.

> +		if (iir & WD_WRITE_COMPLETE_INT)
> +			drm_dbg_kms(display->drm, "Writeback job write completed\n");
> +		if (iir & WD_VBLANK_INT) {
> +			drm_crtc_handle_vblank(&crtc->base);

I suspect the commit completion needs to happen from
WD_WRITE_COMPLETE_INT. So either we put the vblank handling there, or
we use manual commit completion for WD (ala. async flip/DSB/flip queue).

I guess one option would be something like 
  /* armed event for flip queue based updates */
  struct drm_pending_vblank_event *flipq_event;\
+ /* armed event for each WD transcoder */
  struct drm_pending_vblank_event *wd_event[2];

And then we iterate the crtcs, looking for one with an event for the
appropriate WD transcoder.


Another idea that just came to me would be something like this:

struct intel_crtc {
	...
	struct drm_pending_vblank_event *event;
	enum {
		EVENT_NONE,
		EVENT_FLIP_DONE
		EVENT_DSB,
		EVENT_FLIP_QUEUE,
		EVENT_WD0,
		EVENT_WD1,
	} event_type;
	...
};

And then we keep the event and its type in sync while arming/sending.
Would avoid having to keep so many mutually exclusive event pointers
around.

> +			drm_dbg_kms(display->drm, "Writeback vblank raised\n");
> +		}
> +		if (iir & WD_CAPTURING_INT)
> +			drm_dbg_kms(display->drm, "Writeback job capture has started\n");
> +
> +		intel_de_write(display, WD_IIR(wb_conn->trans), iir);
> +	}
> +}
> +
>  int intel_writeback_init(struct intel_display *display)
>  {
>  	struct intel_encoder *encoder;
> diff --git a/drivers/gpu/drm/i915/display/intel_writeback.h b/drivers/gpu/drm/i915/display/intel_writeback.h
> index 3c145cf73e20..83a986753c4c 100644
> --- a/drivers/gpu/drm/i915/display/intel_writeback.h
> +++ b/drivers/gpu/drm/i915/display/intel_writeback.h
> @@ -16,6 +16,7 @@ struct intel_writeback_connector;
>  
>  int intel_writeback_init(struct intel_display *display);
>  void intel_writeback_atomic_commit(struct intel_atomic_state *state);
> +void intel_writeback_isr_handler(struct intel_display *display);
>  
>  #endif /* __INTEL_WRITEBACK_H__ */
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel