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

[Jacek Lawrynowicz]

Hi,

On 10.11.2023 17:44, Jeffrey Hugo wrote:
> 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?

IRQ_WAKE_THREAD is only used when consumer has a callback (last arg to ivpu_ipc_consumer_add()).
We are here waiting for the response synchronously before IRQs are enabled.

>> +            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();

I could use simple assignment here as currently no other handlers propagate return values but there is still this:
>> +    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.

I agree, this is shady :) I will define my own bitfield in v2.

> 
> 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)

Regards,
Jacek