Re: [PATCH 1/8] drm/xe/irq: refactor irq flows to support also msix

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jun 26, 2024 at 01:33:38PM +0300, Dani Liberman wrote:
> When enabling MSIX, there is no need for HW to aggregate interrupts.
> Added a separate MSIX flow that skips unnecessary code in irq init,
> reset, and fini.
> 
> Signed-off-by: Dani Liberman <dliberman@habana.ai>

just a heads up that I just pushed this patch:
https://patchwork.freedesktop.org/patch/597392/

so I believe you might need to rebase.

Cc: Ilia Levi <illevi@habana.ai>

> ---
>  drivers/gpu/drm/xe/xe_device_types.h |  2 +
>  drivers/gpu/drm/xe/xe_irq.c          | 99 +++++++++++++++++++++-------
>  2 files changed, 77 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index c37be471d11c..09ce7ba1b4ed 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -306,6 +306,8 @@ struct xe_device {
>  
>  		/** @irq.enabled: interrupts enabled on this device */
>  		bool enabled;
> +		/** @irq.msix_enabled: msix interrupts enabled on this device */
> +		bool msix_enabled;
>  	} irq;
>  
>  	/** @ttm: ttm device */
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index ab3d5b7a1e8c..663a86edecc0 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -666,28 +666,46 @@ static irq_handler_t xe_irq_handler(struct xe_device *xe)
>  		return xelp_irq_handler;
>  }
>  
> -static void irq_uninstall(void *arg)
> +static void xe_irq_msi_free(struct xe_device *xe)
>  {
> -	struct xe_device *xe = arg;
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	int irq;
>  
> +	irq = pci_irq_vector(pdev, 0);
> +	free_irq(irq, xe);
> +}
> +
> +static void xe_irq_msix_free(struct xe_device *xe)
> +{
> +
> +}
> +
> +static void xe_irq_free(struct xe_device *xe)
> +{
> +	if (xe->irq.msix_enabled)
> +		xe_irq_msix_free(xe);
> +	else
> +		xe_irq_msi_free(xe);
> +}
> +
> +static void irq_uninstall(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
>  	if (!xe->irq.enabled)
>  		return;
>  
>  	xe->irq.enabled = false;
>  	xe_irq_reset(xe);
>  
> -	irq = pci_irq_vector(pdev, 0);
> -	free_irq(irq, xe);
> +	xe_irq_free(xe);
>  }
>  
> -int xe_irq_install(struct xe_device *xe)
> +static int xe_irq_msi_request(struct xe_device *xe)
>  {
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> -	unsigned int irq_flags = PCI_IRQ_MSIX;
>  	irq_handler_t irq_handler;
> -	int err, irq, nvec;
> +	int irq, err;
>  
>  	irq_handler = xe_irq_handler(xe);
>  	if (!irq_handler) {
> @@ -695,32 +713,55 @@ int xe_irq_install(struct xe_device *xe)
>  		return -EINVAL;
>  	}
>  
> -	xe_irq_reset(xe);
> +	irq = pci_irq_vector(pdev, 0);
> +	err = request_irq(irq, irq_handler, IRQF_SHARED, DRIVER_NAME, xe);
> +	if (err < 0) {
> +		drm_err(&xe->drm, "Failed to request MSI IRQ %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xe_irq_msix_request(struct xe_device *xe)
> +{
> +	return 0;
> +}
> +
> +int xe_irq_install(struct xe_device *xe)
> +{
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	unsigned int irq_flags = PCI_IRQ_MSIX;
> +
> +	int err, nvec;
>  
>  	nvec = pci_msix_vec_count(pdev);
> -	if (nvec <= 0) {
> -		if (nvec == -EINVAL) {
> -			/* MSIX capability is not supported in the device, using MSI */
> -			irq_flags = PCI_IRQ_MSI;
> -			nvec = 1;
> -		} else {
> -			drm_err(&xe->drm, "MSIX: Failed getting count\n");
> -			return nvec;
> -		}
> +	if (nvec > 0) {
> +		xe->irq.msix_enabled = true;
> +	} else if (nvec == -EINVAL) {
> +		/* MSIX capability is not supported in the device, using MSI */
> +		irq_flags = PCI_IRQ_MSI;
> +		nvec = 1;
> +	} else {
> +		drm_err(&xe->drm, "MSIX: Failed getting count\n");
> +		return nvec;
>  	}
>  
> +	xe_irq_reset(xe);
> +
>  	err = pci_alloc_irq_vectors(pdev, nvec, nvec, irq_flags);
>  	if (err < 0) {
>  		drm_err(&xe->drm, "MSI/MSIX: Failed to enable support %d\n", err);
>  		return err;
>  	}
>  
> -	irq = pci_irq_vector(pdev, 0);
> -	err = request_irq(irq, irq_handler, IRQF_SHARED, DRIVER_NAME, xe);
> -	if (err < 0) {
> -		drm_err(&xe->drm, "Failed to request MSI/MSIX IRQ %d\n", err);
> +	if (xe->irq.msix_enabled)
> +		err = xe_irq_msix_request(xe);
> +	else
> +		err = xe_irq_msi_request(xe);
> +
> +	if (err)
>  		return err;
> -	}
>  
>  	xe->irq.enabled = true;
>  
> @@ -733,7 +774,7 @@ int xe_irq_install(struct xe_device *xe)
>  	return 0;
>  
>  free_irq_handler:
> -	free_irq(irq, xe);
> +	xe_irq_free(xe);
>  
>  	return err;
>  }
> @@ -743,6 +784,11 @@ void xe_irq_shutdown(struct xe_device *xe)
>  	irq_uninstall(xe);
>  }
>  
> +static void xe_irq_msix_synchronize_irq(struct xe_device *xe)
> +{
> +
> +}
> +
>  void xe_irq_suspend(struct xe_device *xe)
>  {
>  	int irq = to_pci_dev(xe->drm.dev)->irq;
> @@ -751,7 +797,12 @@ void xe_irq_suspend(struct xe_device *xe)
>  	xe->irq.enabled = false; /* no new irqs */
>  	spin_unlock_irq(&xe->irq.lock);
>  
> -	synchronize_irq(irq); /* flush irqs */
> +	/* flush irqs */
> +	if (xe->irq.msix_enabled)
> +		xe_irq_msix_synchronize_irq(xe);
> +	else
> +		synchronize_irq(irq);
> +
>  	xe_irq_reset(xe); /* turn irqs off */
>  }
>  
> -- 
> 2.34.1
>