[PATCH 5/5] accel/ivpu: Use threaded IRQ to handle JOB done messages

[Jeffrey Hugo]

On 11/7/2023 5:35 AM, Jacek Lawrynowicz wrote:
> Remove job_done thread and replace it with generic callback based
> mechanism.
> 
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz at linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_drv.c     |  30 ++---
>   drivers/accel/ivpu/ivpu_drv.h     |   3 +-
>   drivers/accel/ivpu/ivpu_hw_37xx.c |  29 +++--
>   drivers/accel/ivpu/ivpu_ipc.c     | 195 +++++++++++++++++-------------
>   drivers/accel/ivpu/ivpu_ipc.h     |  22 +++-
>   drivers/accel/ivpu/ivpu_job.c     |  84 +++----------
>   drivers/accel/ivpu/ivpu_job.h     |   6 +-
>   7 files changed, 185 insertions(+), 184 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index c4456abe228c..48cbcb254237 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -318,13 +318,15 @@ static int ivpu_wait_for_ready(struct ivpu_device *vdev)
>   	if (ivpu_test_mode & IVPU_TEST_MODE_FW_TEST)
>   		return 0;
>   
> -	ivpu_ipc_consumer_add(vdev, &cons, IVPU_IPC_CHAN_BOOT_MSG);
> +	ivpu_ipc_consumer_add(vdev, &cons, IVPU_IPC_CHAN_BOOT_MSG, NULL);
>   
>   	timeout = jiffies + msecs_to_jiffies(vdev->timeout.boot);
>   	while (1) {
>   		ret = ivpu_ipc_irq_handler(vdev);
> -		if (ret)
> +		if (ret != IRQ_HANDLED) {

What about the IRQ_WAKE_THREAD case?

> +			ret = -EIO;
>   			break;
> +		}
>   		ret = ivpu_ipc_receive(vdev, &cons, &ipc_hdr, NULL, 0);
>   		if (ret != -ETIMEDOUT || time_after_eq(jiffies, timeout))
>   			break;

> diff --git a/drivers/accel/ivpu/ivpu_hw_37xx.c b/drivers/accel/ivpu/ivpu_hw_37xx.c
> index a172cfb1c31f..d1795cd6cc09 100644
> --- a/drivers/accel/ivpu/ivpu_hw_37xx.c
> +++ b/drivers/accel/ivpu/ivpu_hw_37xx.c
> @@ -891,9 +891,13 @@ static void ivpu_hw_37xx_irq_noc_firewall_handler(struct ivpu_device *vdev)
>   }
>   
>   /* Handler for IRQs from VPU core (irqV) */
> -static u32 ivpu_hw_37xx_irqv_handler(struct ivpu_device *vdev, int irq)
> +static irqreturn_t ivpu_hw_37xx_irqv_handler(struct ivpu_device *vdev, int irq)
>   {
>   	u32 status = REGV_RD32(VPU_37XX_HOST_SS_ICB_STATUS_0) & ICB_0_IRQ_MASK;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	if (!status)
> +		return IRQ_NONE;
>   
>   	REGV_WR32(VPU_37XX_HOST_SS_ICB_CLEAR_0, status);
>   
> @@ -901,7 +905,7 @@ static u32 ivpu_hw_37xx_irqv_handler(struct ivpu_device *vdev, int irq)
>   		ivpu_mmu_irq_evtq_handler(vdev);
>   
>   	if (REG_TEST_FLD(VPU_37XX_HOST_SS_ICB_STATUS_0, HOST_IPC_FIFO_INT, status))
> -		ivpu_ipc_irq_handler(vdev);
> +		ret |= ivpu_ipc_irq_handler(vdev);

Why the bitwise operation?  handler() returns a irqreturn_t, so it seems 
like this should just be ret = handler();

>   
>   	if (REG_TEST_FLD(VPU_37XX_HOST_SS_ICB_STATUS_0, MMU_IRQ_1_INT, status))
>   		ivpu_dbg(vdev, IRQ, "MMU sync complete\n");
> @@ -918,17 +922,17 @@ static u32 ivpu_hw_37xx_irqv_handler(struct ivpu_device *vdev, int irq)
>   	if (REG_TEST_FLD(VPU_37XX_HOST_SS_ICB_STATUS_0, NOC_FIREWALL_INT, status))
>   		ivpu_hw_37xx_irq_noc_firewall_handler(vdev);
>   
> -	return status;
> +	return ret;
>   }
>   
>   /* Handler for IRQs from Buttress core (irqB) */
> -static u32 ivpu_hw_37xx_irqb_handler(struct ivpu_device *vdev, int irq)
> +static irqreturn_t ivpu_hw_37xx_irqb_handler(struct ivpu_device *vdev, int irq)
>   {
>   	u32 status = REGB_RD32(VPU_37XX_BUTTRESS_INTERRUPT_STAT) & BUTTRESS_IRQ_MASK;
>   	bool schedule_recovery = false;
>   
> -	if (status == 0)
> -		return 0;
> +	if (!status)
> +		return IRQ_NONE;
>   
>   	if (REG_TEST_FLD(VPU_37XX_BUTTRESS_INTERRUPT_STAT, FREQ_CHANGE, status))
>   		ivpu_dbg(vdev, IRQ, "FREQ_CHANGE irq: %08x",
> @@ -964,23 +968,26 @@ static u32 ivpu_hw_37xx_irqb_handler(struct ivpu_device *vdev, int irq)
>   	if (schedule_recovery)
>   		ivpu_pm_schedule_recovery(vdev);
>   
> -	return status;
> +	return IRQ_HANDLED;
>   }
>   
>   static irqreturn_t ivpu_hw_37xx_irq_handler(int irq, void *ptr)
>   {
>   	struct ivpu_device *vdev = ptr;
> -	u32 ret_irqv, ret_irqb;
> +	irqreturn_t ret = IRQ_NONE;
>   
>   	REGB_WR32(VPU_37XX_BUTTRESS_GLOBAL_INT_MASK, 0x1);
>   
> -	ret_irqv = ivpu_hw_37xx_irqv_handler(vdev, irq);
> -	ret_irqb = ivpu_hw_37xx_irqb_handler(vdev, irq);
> +	ret |= ivpu_hw_37xx_irqv_handler(vdev, irq);
> +	ret |= ivpu_hw_37xx_irqb_handler(vdev, irq);

I think this violates coding-style. typedefs are only to be used in 
limited circumstances.  The one I think that applies here is that the 
type is intended to be completely opaque and only accessed through 
proper accessor functions.  You are peering into the type and using the 
information that it is implemented as a bitfield, which is not for you 
to know.

If irqreturn_t changes in structure, this will break, and I don't think 
it will be caught by the compiler, or be obvious.

>   
>   	/* Re-enable global interrupts to re-trigger MSI for pending interrupts */
>   	REGB_WR32(VPU_37XX_BUTTRESS_GLOBAL_INT_MASK, 0x0);
>   
> -	return IRQ_RETVAL(ret_irqb | ret_irqv);
> +	if (ret & IRQ_WAKE_THREAD)
> +		return IRQ_WAKE_THREAD;
> +
> +	return ret;
>   }
>   
>   static void ivpu_hw_37xx_diagnose_failure(struct ivpu_device *vdev)