[Intel-xe] [PATCH v2 1/8] drm/xe/irq: Cleanup media GT interrupt flow

[Andi Shyti]

Hi Matt,

On Wed, Apr 12, 2023 at 03:52:41PM -0700, Matt Roper wrote:
> Although platforms like MTL have media exposed as a separate GT, it's
> important to recognize that this second GT is *not* a tile as we see in
> platforms like PVC.  The interrupt registers (including the ones related
> to GT interrupts) reside in the SGunit so there's only a single instance
> on the entire platform; they are not replicated per-GT.  Thus all media
> interrupts are delivered together with the interrupts for the primary
> GT.
> 
> v2:
>  - Also update reset & postinstall routines to skip media GT.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>

Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com> 

just a couple of thougths...

> ---
>  drivers/gpu/drm/xe/xe_irq.c | 79 ++++++++++++++++++++++++++++++-------
>  1 file changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 13f9f220bca0..7e78eecba62a 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -232,7 +232,40 @@ gt_other_irq_handler(struct xe_gt *gt, const u8 instance, const u16 iir)
>  	}
>  }
>  
> -static void gt_irq_handler(struct xe_device *xe, struct xe_gt *gt,
> +static struct xe_gt *find_media_gt(struct xe_device *xe)
> +{
> +	struct xe_gt *gt;
> +	int id;
> +
> +	for_each_gt(gt, xe, id)
> +		if (xe_gt_is_media_type(gt))
> +			return gt;
> +
> +	drm_WARN_ONCE(&xe->drm, 1, "Cannot find media GT for interrupt handling\n");
> +	return xe_device_get_gt(xe, 0);

isn't it easier if:

	return ERR_PTR(-ENODEV);

...

> +}
> +
> +static struct xe_gt *pick_engine_gt(struct xe_gt *full_gt,
> +				    enum xe_engine_class class,
> +				    unsigned int instance)
> +{
> +	struct xe_device *xe = gt_to_xe(full_gt);
> +
> +	if (MEDIA_VER(xe) < 13)
> +		return full_gt;
> +
> +	if (class == XE_ENGINE_CLASS_VIDEO_DECODE ||
> +	    class == XE_ENGINE_CLASS_VIDEO_ENHANCE)
> +		return find_media_gt(xe);
> +
> +	if (class == XE_ENGINE_CLASS_OTHER &&
> +	    instance == OTHER_MEDIA_GUC_INSTANCE)
> +		return find_media_gt(xe);
> +
> +	return full_gt;

...
	xe_gt = find_media_gt(xe)

	return PTR_ERR(xe_gt) == -ENODEV ? full_gt : xe_gt;

> +}
> +
> +static void gt_irq_handler(struct xe_device *xe, struct xe_gt *full_gt,
>  			   u32 master_ctl, long unsigned int *intr_dw,
>  			   u32 *identity)
>  {
> @@ -241,27 +274,33 @@ static void gt_irq_handler(struct xe_device *xe, struct xe_gt *gt,
>  	enum xe_engine_class class;
>  	struct xe_hw_engine *hwe;
>  
> +	/*
> +	 * Media GT interrupts are delivered on the primary GT.  This function
> +	 * should never be called on the media GT itself.
> +	 */
> +	drm_WARN_ON_ONCE(&xe->drm, xe_gt_is_media_type(full_gt));

I think this is too drastic, what if...

> +
>  	spin_lock(&xe->irq.lock);
>  
>  	for (bank = 0; bank < 2; bank++) {

...
	if (xe_gt_is_media_type(full_gt))
		goto media_engine;

...

>  		if (!(master_ctl & GT_DW_IRQ(bank)))
>  			continue;
>  
> -		if (!xe_gt_is_media_type(gt)) {
> -			intr_dw[bank] =
> -				xe_mmio_read32(gt, GT_INTR_DW(bank).reg);
> -			for_each_set_bit(bit, intr_dw + bank, 32)
> -				identity[bit] = gt_engine_identity(xe, gt,
> -								   bank, bit);
> -			xe_mmio_write32(gt, GT_INTR_DW(bank).reg,
> -					intr_dw[bank]);
> -		}
> +		intr_dw[bank] = xe_mmio_read32(full_gt, GT_INTR_DW(bank).reg);
> +		for_each_set_bit(bit, intr_dw + bank, 32)
> +			identity[bit] = gt_engine_identity(xe, full_gt,
> +							   bank, bit);
> +		xe_mmio_write32(full_gt, GT_INTR_DW(bank).reg, intr_dw[bank]);

...
	media_enige:

And we avoid all the above "find_media_gt". We use for_each_gt
for GT's and we consider the media engine like an "incomplete"
GT, instead of jumping from GT to GT.

Besides, looking in the future, how is this working with two
tiles and one media GT?

Nevertheless, the patch is correct and I confirm my r-b. My
suggestions are just cosmetics.

Andi

>  
>  		for_each_set_bit(bit, intr_dw + bank, 32) {
> +			struct xe_gt *gt;
> +
>  			class = INTR_ENGINE_CLASS(identity[bit]);
>  			instance = INTR_ENGINE_INSTANCE(identity[bit]);
>  			intr_vec = INTR_ENGINE_INTR(identity[bit]);
>  
> +			gt = pick_engine_gt(full_gt, class, instance);
> +
>  			if (class == XE_ENGINE_CLASS_OTHER) {
>  				gt_other_irq_handler(gt, instance, intr_vec);
>  				continue;
> @@ -369,11 +408,17 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>  	}
>  
>  	for_each_gt(gt, xe, id) {
> +		/*
> +		 * The media GT isn't a tile, so its interrupts are delivered
> +		 * as part of the primary GT's flow.
> +		 */
> +		if (xe_gt_is_media_type(gt))
> +			continue;
> +
>  		if ((master_tile_ctl & DG1_MSTR_TILE(gt->info.vram_id)) == 0)
>  			continue;
>  
> -		if (!xe_gt_is_media_type(gt))
> -			master_ctl = xe_mmio_read32(gt, GFX_MSTR_IRQ.reg);
> +		master_ctl = xe_mmio_read32(gt, GFX_MSTR_IRQ.reg);
>  
>  		/*
>  		 * We might be in irq handler just when PCIe DPC is initiated
> @@ -386,8 +431,8 @@ static irqreturn_t dg1_irq_handler(int irq, void *arg)
>  			return IRQ_HANDLED;
>  		}
>  
> -		if (!xe_gt_is_media_type(gt))
> -			xe_mmio_write32(gt, GFX_MSTR_IRQ.reg, master_ctl);
> +		xe_mmio_write32(gt, GFX_MSTR_IRQ.reg, master_ctl);
> +
>  		gt_irq_handler(xe, gt, master_ctl, intr_dw, identity);
>  
>  		/*
> @@ -473,6 +518,9 @@ static void xe_irq_reset(struct xe_device *xe)
>  	u8 id;
>  
>  	for_each_gt(gt, xe, id) {
> +		if (xe_gt_is_media_type(gt))
> +			continue;
> +
>  		if (GRAPHICS_VERx100(xe) >= 1210)
>  			dg1_irq_reset(gt);
>  		else
> @@ -486,6 +534,9 @@ void xe_gt_irq_postinstall(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
>  
> +	if (xe_gt_is_media_type(gt))
> +		return;
> +
>  	if (GRAPHICS_VERx100(xe) >= 1210)
>  		dg1_irq_postinstall(xe, gt);
>  	else
> -- 
> 2.39.2