Re: [PATCH v2 18/22] drm/xe/irq: Split irq mask per engine class

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Oct 15, 2025 at 03:06:33PM -0700, Lucas De Marchi wrote:
> Each engine class has a different bitfield structure in the hw. We've
> been just using a common mask for all of them, but this means that we
> could inadvertently set a wrong bit in one class while enabling
> something in another. Split them to make it more future proof.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_irq.c | 73 +++++++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 9c3a85c4585ed..103804546b280 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -139,25 +139,28 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
>  	struct xe_mmio *mmio = &gt->mmio;
> -	u32 irqs, dmask, smask;
> -	u32 gsc_mask = 0;
> -	u32 heci_mask = 0;
> +	u32 common_mask, val, gsc_mask = 0, heci_mask = 0,
> +	    rcs_mask = 0, bcs_mask = 0, vcs_mask = 0, vecs_mask = 0,
> +	    ccs_mask = 0;
>  
>  	if (xe_device_uses_memirq(xe))
>  		return;
>  
>  	if (xe_device_uc_enabled(xe)) {
> -		irqs = GT_RENDER_USER_INTERRUPT |
> -			GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
> +		common_mask = GT_RENDER_USER_INTERRUPT |

As a follow-up patch, this should probably be renamed to
GT_MI_USER_INTERRUPT since it doesn't have anything to do with render
these days (I think once upon a time this interrupts was only supported
on the RCS, but it's supported everywhere on all the platforms Xe
supports).

> +			      GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;

This one will probably need a rename too.  The purpose is the same on
all engines (indicate that a flush command has finished), but the actual
flush command differs between engines...PIPECTL is only on rcs/ccs, and
MI_FLUSH_DW is used on the other engines.  So maybe
GT_FLUSH_COMPLETE_INTERRUPT or similar would be more accurate.

>  	} else {
> -		irqs = GT_RENDER_USER_INTERRUPT |
> -		       GT_CS_MASTER_ERROR_INTERRUPT |
> -		       GT_CONTEXT_SWITCH_INTERRUPT |
> -		       GT_WAIT_SEMAPHORE_INTERRUPT;
> +		common_mask = GT_RENDER_USER_INTERRUPT |
> +			      GT_CS_MASTER_ERROR_INTERRUPT |
> +			      GT_CONTEXT_SWITCH_INTERRUPT |
> +			      GT_WAIT_SEMAPHORE_INTERRUPT;
>  	}
>  
> -	dmask = irqs << 16 | irqs;
> -	smask = irqs << 16;
> +	rcs_mask |= common_mask;
> +	bcs_mask |= common_mask;
> +	vcs_mask |= common_mask;
> +	vecs_mask |= common_mask;
> +	ccs_mask |= common_mask;
>  
>  	if (xe_gt_is_main_type(gt)) {
>  		/*
> @@ -169,44 +172,62 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>  		u32 bcs_fuse_mask = xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_COPY);
>  
>  		/* Enable interrupts for each engine class */
> -		xe_mmio_write32(mmio, RENDER_COPY_INTR_ENABLE, dmask);
> +		xe_mmio_write32(mmio, RENDER_COPY_INTR_ENABLE,
> +				REG_FIELD_PREP(ENGINE1_MASK, rcs_mask) |
> +				REG_FIELD_PREP(ENGINE0_MASK, bcs_mask));
>  		if (ccs_fuse_mask)
> -			xe_mmio_write32(mmio, CCS_RSVD_INTR_ENABLE, smask);
> +			xe_mmio_write32(mmio, CCS_RSVD_INTR_ENABLE,
> +					REG_FIELD_PREP(ENGINE1_MASK, ccs_mask));
>  
>  		/* Unmask interrupts for each engine instance */
> -		xe_mmio_write32(mmio, RCS0_RSVD_INTR_MASK, ~smask);
> -		xe_mmio_write32(mmio, BCS_RSVD_INTR_MASK, ~smask);
> +		val = ~REG_FIELD_PREP(ENGINE1_MASK, rcs_mask);
> +		xe_mmio_write32(mmio, RCS0_RSVD_INTR_MASK, val);
> +		val = ~REG_FIELD_PREP(ENGINE1_MASK, bcs_mask);
> +		xe_mmio_write32(mmio, BCS_RSVD_INTR_MASK, val);
> +
> +		val = ~(REG_FIELD_PREP(ENGINE1_MASK, bcs_mask) |
> +			REG_FIELD_PREP(ENGINE0_MASK, bcs_mask));
>  		if (bcs_fuse_mask & (BIT(1)|BIT(2)))
> -			xe_mmio_write32(mmio, XEHPC_BCS1_BCS2_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, XEHPC_BCS1_BCS2_INTR_MASK, val);
>  		if (bcs_fuse_mask & (BIT(3)|BIT(4)))
> -			xe_mmio_write32(mmio, XEHPC_BCS3_BCS4_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, XEHPC_BCS3_BCS4_INTR_MASK, val);
>  		if (bcs_fuse_mask & (BIT(5)|BIT(6)))
> -			xe_mmio_write32(mmio, XEHPC_BCS5_BCS6_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, XEHPC_BCS5_BCS6_INTR_MASK, val);
>  		if (bcs_fuse_mask & (BIT(7)|BIT(8)))
> -			xe_mmio_write32(mmio, XEHPC_BCS7_BCS8_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, XEHPC_BCS7_BCS8_INTR_MASK, val);
> +
> +		val = ~(REG_FIELD_PREP(ENGINE1_MASK, ccs_mask) |
> +			REG_FIELD_PREP(ENGINE0_MASK, ccs_mask));
>  		if (ccs_fuse_mask & (BIT(0)|BIT(1)))
> -			xe_mmio_write32(mmio, CCS0_CCS1_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, CCS0_CCS1_INTR_MASK, val);
>  		if (ccs_fuse_mask & (BIT(2)|BIT(3)))
> -			xe_mmio_write32(mmio, CCS2_CCS3_INTR_MASK, ~dmask);
> +			xe_mmio_write32(mmio, CCS2_CCS3_INTR_MASK, val);
>  	}
>  
>  	if (xe_gt_is_media_type(gt) || MEDIA_VER(xe) < 13) {
>  		u32 other_fuse_mask = xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_OTHER);
>  
>  		/* Enable interrupts for each engine class */
> -		xe_mmio_write32(mmio, VCS_VECS_INTR_ENABLE, dmask);
> +		xe_mmio_write32(mmio, VCS_VECS_INTR_ENABLE,
> +				REG_FIELD_PREP(ENGINE1_MASK, vcs_mask) |
> +				REG_FIELD_PREP(ENGINE1_MASK, vecs_mask));
>  
>  		/* Unmask interrupts for each engine instance */

As a follow-up, we should probably add vcs_fuse_mask / vecs_fuse_mask
checks for these, and also handle the rest of the engines up to VCS7 and
VECS3 (since we have all of those in media_xelpmp's descriptor).

> -		xe_mmio_write32(mmio, VCS0_VCS1_INTR_MASK, ~dmask);
> -		xe_mmio_write32(mmio, VCS2_VCS3_INTR_MASK, ~dmask);
> -		xe_mmio_write32(mmio, VECS0_VECS1_INTR_MASK, ~dmask);
> +		val = ~(REG_FIELD_PREP(ENGINE1_MASK, vcs_mask) |
> +			REG_FIELD_PREP(ENGINE0_MASK, vcs_mask));
> +		xe_mmio_write32(mmio, VCS0_VCS1_INTR_MASK, val);
> +		xe_mmio_write32(mmio, VCS2_VCS3_INTR_MASK, val);
> +
> +		val = ~(REG_FIELD_PREP(ENGINE1_MASK, vcs_mask) |
> +			REG_FIELD_PREP(ENGINE0_MASK, vcs_mask));

Was this one supposed to be using vecs_mask?


Matt

> +		xe_mmio_write32(mmio, VECS0_VECS1_INTR_MASK, val);
>  
>  		/*
>  		 * the heci2 interrupt is enabled via the same register as the
>  		 * GSCCS interrupts, but it has its own mask register.
>  		 */
>  		if (other_fuse_mask) {
> -			gsc_mask = irqs | GSC_ER_COMPLETE;
> +			gsc_mask = common_mask | GSC_ER_COMPLETE;
>  			heci_mask = GSC_IRQ_INTF(1);
>  		} else if (xe->info.has_heci_gscfi) {
>  			gsc_mask = GSC_IRQ_INTF(1);
> 
> -- 
> 2.51.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation