[PATCH] amdkfd: Drop interrupt SW ring buffer

[Oded Gabbay]

Hi Michel,
So your patch is quite, hmm, *drastic* :)

Instead, could I suggest to only remove the calls to kfd_interrupt_init()
and kfd_interrupt_exit() ? It will also require a minor modification to the
logic in kgd2kfd_interrupt() but it is much less intrusive than what you are
suggesting.

Alternatively, we could take just this hunk:

> @@ -296,13 +286,5 @@ int kgd2kfd_resume(struct kfd_dev *kfd)
>  /* This is called directly from KGD at ISR. */
>  void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry)
>  {
> -	if (kfd->init_complete) {
> -		spin_lock(&kfd->interrupt_lock);
> -
> -		if (kfd->interrupts_active
> -		    && enqueue_ih_ring_entry(kfd, ih_ring_entry))
> -			schedule_work(&kfd->interrupt_work);
> -
> -		spin_unlock(&kfd->interrupt_lock);
> -	}
> +	/* Process interrupts / schedule work as necessary */
>  }


After all, we do need this feature eventually and most of it is fine, so I
don't want to take it all out. As I said, it is *drastic*.

	Oded

On 01/07/2015 05:59 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
> 
> The SW ring buffer was smaller than the corresponding hardware ring, so
> dmesg could get spammed by
> 
>  kfd kfd: Interrupt ring overflow, dropping interrupt.
> 
> messages when running graphics apps.
> 
> Since the SW ring buffer doesn't actually do anything at this point, just
> remove it for now. When actual interrupt processing code is added to
> amdkfd, it should try to do things immediately and only defer to work
> queues when necessary.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/Makefile        |   3 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c    |  20 +---
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 176 -----------------------------
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h      |  15 ---
>  4 files changed, 2 insertions(+), 212 deletions(-)
>  delete mode 100644 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/Makefile b/drivers/gpu/drm/amd/amdkfd/Makefile
> index be6246d..307a309 100644
> --- a/drivers/gpu/drm/amd/amdkfd/Makefile
> +++ b/drivers/gpu/drm/amd/amdkfd/Makefile
> @@ -8,7 +8,6 @@ amdkfd-y	:= kfd_module.o kfd_device.o kfd_chardev.o kfd_topology.o \
>  		kfd_pasid.o kfd_doorbell.o kfd_flat_memory.o \
>  		kfd_process.o kfd_queue.o kfd_mqd_manager.o \
>  		kfd_kernel_queue.o kfd_packet_manager.o \
> -		kfd_process_queue_manager.o kfd_device_queue_manager.o \
> -		kfd_interrupt.o
> +		kfd_process_queue_manager.o kfd_device_queue_manager.o
>  
>  obj-$(CONFIG_HSA_AMD)	+= amdkfd.o
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 43884eb..633532a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -192,13 +192,6 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>  		goto kfd_topology_add_device_error;
>  	}
>  
> -	if (kfd_interrupt_init(kfd)) {
> -		dev_err(kfd_device,
> -			"Error initializing interrupts for device (%x:%x)\n",
> -			kfd->pdev->vendor, kfd->pdev->device);
> -		goto kfd_interrupt_error;
> -	}
> -
>  	if (!device_iommu_pasid_init(kfd)) {
>  		dev_err(kfd_device,
>  			"Error initializing iommuv2 for device (%x:%x)\n",
> @@ -237,8 +230,6 @@ dqm_start_error:
>  device_queue_manager_error:
>  	amd_iommu_free_device(kfd->pdev);
>  device_iommu_pasid_error:
> -	kfd_interrupt_exit(kfd);
> -kfd_interrupt_error:
>  	kfd_topology_remove_device(kfd);
>  kfd_topology_add_device_error:
>  	kfd2kgd->fini_sa_manager(kfd->kgd);
> @@ -254,7 +245,6 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>  	if (kfd->init_complete) {
>  		device_queue_manager_uninit(kfd->dqm);
>  		amd_iommu_free_device(kfd->pdev);
> -		kfd_interrupt_exit(kfd);
>  		kfd_topology_remove_device(kfd);
>  	}
>  
> @@ -296,13 +286,5 @@ int kgd2kfd_resume(struct kfd_dev *kfd)
>  /* This is called directly from KGD at ISR. */
>  void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry)
>  {
> -	if (kfd->init_complete) {
> -		spin_lock(&kfd->interrupt_lock);
> -
> -		if (kfd->interrupts_active
> -		    && enqueue_ih_ring_entry(kfd, ih_ring_entry))
> -			schedule_work(&kfd->interrupt_work);
> -
> -		spin_unlock(&kfd->interrupt_lock);
> -	}
> +	/* Process interrupts / schedule work as necessary */
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> deleted file mode 100644
> index 5b99909..0000000
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ /dev/null
> @@ -1,176 +0,0 @@
> -/*
> - * Copyright 2014 Advanced Micro Devices, Inc.
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> - * copy of this software and associated documentation files (the "Software"),
> - * to deal in the Software without restriction, including without limitation
> - * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> - * and/or sell copies of the Software, and to permit persons to whom the
> - * Software is furnished to do so, subject to the following conditions:
> - *
> - * The above copyright notice and this permission notice shall be included in
> - * all copies or substantial portions of the Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> - * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> - * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> - * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> - * OTHER DEALINGS IN THE SOFTWARE.
> - */
> -
> -/*
> - * KFD Interrupts.
> - *
> - * AMD GPUs deliver interrupts by pushing an interrupt description onto the
> - * interrupt ring and then sending an interrupt. KGD receives the interrupt
> - * in ISR and sends us a pointer to each new entry on the interrupt ring.
> - *
> - * We generally can't process interrupt-signaled events from ISR, so we call
> - * out to each interrupt client module (currently only the scheduler) to ask if
> - * each interrupt is interesting. If they return true, then it requires further
> - * processing so we copy it to an internal interrupt ring and call each
> - * interrupt client again from a work-queue.
> - *
> - * There's no acknowledgment for the interrupts we use. The hardware simply
> - * queues a new interrupt each time without waiting.
> - *
> - * The fixed-size internal queue means that it's possible for us to lose
> - * interrupts because we have no back-pressure to the hardware.
> - */
> -
> -#include <linux/slab.h>
> -#include <linux/device.h>
> -#include "kfd_priv.h"
> -
> -#define KFD_INTERRUPT_RING_SIZE 256
> -
> -static void interrupt_wq(struct work_struct *);
> -
> -int kfd_interrupt_init(struct kfd_dev *kfd)
> -{
> -	void *interrupt_ring = kmalloc_array(KFD_INTERRUPT_RING_SIZE,
> -					kfd->device_info->ih_ring_entry_size,
> -					GFP_KERNEL);
> -	if (!interrupt_ring)
> -		return -ENOMEM;
> -
> -	kfd->interrupt_ring = interrupt_ring;
> -	kfd->interrupt_ring_size =
> -		KFD_INTERRUPT_RING_SIZE * kfd->device_info->ih_ring_entry_size;
> -	atomic_set(&kfd->interrupt_ring_wptr, 0);
> -	atomic_set(&kfd->interrupt_ring_rptr, 0);
> -
> -	spin_lock_init(&kfd->interrupt_lock);
> -
> -	INIT_WORK(&kfd->interrupt_work, interrupt_wq);
> -
> -	kfd->interrupts_active = true;
> -
> -	/*
> -	 * After this function returns, the interrupt will be enabled. This
> -	 * barrier ensures that the interrupt running on a different processor
> -	 * sees all the above writes.
> -	 */
> -	smp_wmb();
> -
> -	return 0;
> -}
> -
> -void kfd_interrupt_exit(struct kfd_dev *kfd)
> -{
> -	/*
> -	 * Stop the interrupt handler from writing to the ring and scheduling
> -	 * workqueue items. The spinlock ensures that any interrupt running
> -	 * after we have unlocked sees interrupts_active = false.
> -	 */
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&kfd->interrupt_lock, flags);
> -	kfd->interrupts_active = false;
> -	spin_unlock_irqrestore(&kfd->interrupt_lock, flags);
> -
> -	/*
> -	 * Flush_scheduled_work ensures that there are no outstanding
> -	 * work-queue items that will access interrupt_ring. New work items
> -	 * can't be created because we stopped interrupt handling above.
> -	 */
> -	flush_scheduled_work();
> -
> -	kfree(kfd->interrupt_ring);
> -}
> -
> -/*
> - * This assumes that it can't be called concurrently with itself
> - * but only with dequeue_ih_ring_entry.
> - */
> -bool enqueue_ih_ring_entry(struct kfd_dev *kfd,	const void *ih_ring_entry)
> -{
> -	unsigned int rptr = atomic_read(&kfd->interrupt_ring_rptr);
> -	unsigned int wptr = atomic_read(&kfd->interrupt_ring_wptr);
> -
> -	if ((rptr - wptr) % kfd->interrupt_ring_size ==
> -					kfd->device_info->ih_ring_entry_size) {
> -		/* This is very bad, the system is likely to hang. */
> -		dev_err_ratelimited(kfd_chardev(),
> -			"Interrupt ring overflow, dropping interrupt.\n");
> -		return false;
> -	}
> -
> -	memcpy(kfd->interrupt_ring + wptr, ih_ring_entry,
> -			kfd->device_info->ih_ring_entry_size);
> -
> -	wptr = (wptr + kfd->device_info->ih_ring_entry_size) %
> -			kfd->interrupt_ring_size;
> -	smp_wmb(); /* Ensure memcpy'd data is visible before wptr update. */
> -	atomic_set(&kfd->interrupt_ring_wptr, wptr);
> -
> -	return true;
> -}
> -
> -/*
> - * This assumes that it can't be called concurrently with itself
> - * but only with enqueue_ih_ring_entry.
> - */
> -static bool dequeue_ih_ring_entry(struct kfd_dev *kfd, void *ih_ring_entry)
> -{
> -	/*
> -	 * Assume that wait queues have an implicit barrier, i.e. anything that
> -	 * happened in the ISR before it queued work is visible.
> -	 */
> -
> -	unsigned int wptr = atomic_read(&kfd->interrupt_ring_wptr);
> -	unsigned int rptr = atomic_read(&kfd->interrupt_ring_rptr);
> -
> -	if (rptr == wptr)
> -		return false;
> -
> -	memcpy(ih_ring_entry, kfd->interrupt_ring + rptr,
> -			kfd->device_info->ih_ring_entry_size);
> -
> -	rptr = (rptr + kfd->device_info->ih_ring_entry_size) %
> -			kfd->interrupt_ring_size;
> -
> -	/*
> -	 * Ensure the rptr write update is not visible until
> -	 * memcpy has finished reading.
> -	 */
> -	smp_mb();
> -	atomic_set(&kfd->interrupt_ring_rptr, rptr);
> -
> -	return true;
> -}
> -
> -static void interrupt_wq(struct work_struct *work)
> -{
> -	struct kfd_dev *dev = container_of(work, struct kfd_dev,
> -						interrupt_work);
> -
> -	uint32_t ih_ring_entry[DIV_ROUND_UP(
> -				dev->device_info->ih_ring_entry_size,
> -				sizeof(uint32_t))];
> -
> -	while (dequeue_ih_ring_entry(dev, ih_ring_entry))
> -		;
> -}
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index f9fb81e..3a6ac90 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -135,22 +135,10 @@ struct kfd_dev {
>  
>  	struct kgd2kfd_shared_resources shared_resources;
>  
> -	void *interrupt_ring;
> -	size_t interrupt_ring_size;
> -	atomic_t interrupt_ring_rptr;
> -	atomic_t interrupt_ring_wptr;
> -	struct work_struct interrupt_work;
> -	spinlock_t interrupt_lock;
> -
>  	/* QCM Device instance */
>  	struct device_queue_manager *dqm;
>  
>  	bool init_complete;
> -	/*
> -	 * Interrupts of interest to KFD are copied
> -	 * from the HW ring into a SW ring.
> -	 */
> -	bool interrupts_active;
>  };
>  
>  /* KGD2KFD callbacks */
> @@ -513,10 +501,7 @@ struct kfd_dev *kfd_device_by_pci_dev(const struct pci_dev *pdev);
>  struct kfd_dev *kfd_topology_enum_kfd_devices(uint8_t idx);
>  
>  /* Interrupts */
> -int kfd_interrupt_init(struct kfd_dev *dev);
> -void kfd_interrupt_exit(struct kfd_dev *dev);
>  void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry);
> -bool enqueue_ih_ring_entry(struct kfd_dev *kfd,	const void *ih_ring_entry);
>  
>  /* Power Management */
>  void kgd2kfd_suspend(struct kfd_dev *kfd);
>