Received-SPF: pass (google.com: domain of linux-kernel+bounces-76862-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <0d30d2ad-e0af-44ca-a6e7-5dafafe1b82d@ti.com>
Date: Thu, 22 Feb 2024 10:11:36 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] media: chips-media: wave5: Add hrtimer based polling
 support
Content-Language: en-US
To: Sebastian Fricke <sebastian.fricke@collabora.com>,
        Devarsh Thakkar
	<devarsht@ti.com>
CC: <nas.chung@chipsnmedia.com>, <jackson.lee@chipsnmedia.com>,
        <mchehab@kernel.org>, <linux-media@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <nm@ti.com>, <praneeth@ti.com>,
        <vigneshr@ti.com>, <a-bhatia1@ti.com>, <j-luthra@ti.com>,
        <b-brnich@ti.com>, <detheridge@ti.com>, <p-mantena@ti.com>,
        <vijayp@ti.com>
References: <20240125130833.1953617-1-devarsht@ti.com>
 <20240216094514.lm4naivfrbiexotl@basti-XPS-13-9310>
From: Andrew Davis <afd@ti.com>
In-Reply-To: <20240216094514.lm4naivfrbiexotl@basti-XPS-13-9310>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

On 2/16/24 3:45 AM, Sebastian Fricke wrote:
> Hey Devarsh,
> 
> @nas.chung@chipsnmedia.com or @jackson.lee@chipsnmedia.com
> Could you please run tests on the hardware with this patch and review
> this patch to make sure that this doesn't break the other hardware with
> Wave5 chips. Thanks!
> 
> On 25.01.2024 18:38, Devarsh Thakkar wrote:
>> Add support for starting a polling timer in case interrupt is not
> 
> s/interrupt/an interrupt/
> 
>> available. This helps keep the VPU functional in SoC's such as AM62A, where
> 
> s/helps keep/helps to keep/
> s/SoC's/SoCs/
> 
>> the hardware interrupt hookup may not be present due to an SoC errata [1].
> 
> I would prefer if we do not hack the driver to adjust to SoC errata,
> instead of adding polling functionality you could also implement a fake
> IRQ chip, then the driver can still assume that an interrupt is present
> and you create a virtual interrupt source that does polling in the
> background.
> You could start by adding the fake irq-chip within the wave5 folder and
> later on that could be moved to the `drivers/irqchip/` directory (by you
> or someone else), which would be a nice addition to the Linux kernel to
> provide machines without IRQ support a way to register an interrupt.

+1

I'm really surprised that isn't already a thing. A lot of drivers do
hacky irq+polling to work around missing status update interrupts.
It seems to lead to race conditions and generally messy to follow code
in most cases. A simple generic "fire an inteterrupt every X time units"
virtual interrupt source would cleanup a lot of these.

Andrew

> Please have a look at that potential solution, you potentially need to
> share the registers between the codec and the fake irq-chip in the DTS.
> 
>>
>> The timer is shared across all instances of encoder and decoder and is
> 
> s/encoder and decoder/encoders and decoders/
> 
>> started when first instance of encoder or decoder is opened and stopped
> 
> s/first/the first/
> s/encoder or decoder/an encoder or a decoder/
> 
>> when last instance is closed, thus avoiding per instance polling and saving
> 
> s/last/the last/
> 
>> CPU bandwidth.
>>
>> hrtimer callback is called with 5ms polling interval while any of the
> 
> s/hrtimer/The hrtimer/
> 
>> encoder/decoder instances are running to check the interrupt status as
>> being done in irq handler.
> 
> s/irq/the irq/
> 
> Could you ellaborate why you choose the HRtimer for this job and why
> does it need to be a 5ms polling interval, is that a requirement of this
> specific hardware or the best constant you figured out during testing?
> 
>>
>> Based on above interrupt status, use a worker thread to iterate over the
> 
> s/on above interrupt status/on the interrupt status above/
> 
>> interrupt status for each instance and send completion event as being done
> 
> s/completion/a completion/
> 
>> in irq thread function.
> 
> s/irq/the irq/
> 
>>
>> Parse for irq number before v4l2 device registration and if not available
>> only then, initialize hrtimer and worker thread.
> 
> Thanks for the effort on the description but you do not have to write
> out word for word what the code is doing, the code should be descriptive
> enough on its own.
> More important is that you clearly describe, for example:
> - why did you choose a certain design?
> - why did you choose for the HRtimer?
> - why is the 5ms interval is important (are other intervals also valid?)
> - why did you choose to add a polling mechanism instead of possible alternatives (like for example an fake irq-chip),
> -
>>
>> Move the core functionality of irq thread function to a separate function
> 
> s/irq/the irq/
> 
>> wave5_vpu_handle_irq so that it can be used by both the worker thread when
>> using polling mode and irq thread when using interrupt mode.
> 
> s/irq/the irq/
> 
>>
>> Protect hrtimer access and instance list with device specific mutex locks
> 
> s/hrtimer/the hrtimer/
> 
>> to avoid race conditions while different instances of encoder and decoder
> 
> s/encoder and decoder/encoders and decoders/
> 
>> are started together.
>>
>> Add module param to change polling interval for debug purpose.
> 
> s/module/a module/
> s/polling/the polling/
> s/debug purpose/debugging purposes/
> 
> Here again is the question, what can the user expect with different
> polling intervals? Is there a valid range of intervals?
> 
> The actual implementation seems good, but I would like to first make
> sure that we take the best possible approach to this issue.
> 
> Greetings,
> Sebastian
> 
>>
>> [1] https://www.ti.com/lit/pdf/spruj16
>> (Ref: Section 4.2.3.3 Resets, Interrupts, and Clocks)
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> Test logs:
>> https://gist.github.com/devarsht/613bc8aa66e65814c8374ffb6a4f50fb
>> ---
>> .../platform/chips-media/wave5/wave5-helper.c |  16 ++-
>> .../chips-media/wave5/wave5-vpu-dec.c         |  13 +-
>> .../chips-media/wave5/wave5-vpu-enc.c         |  13 +-
>> .../platform/chips-media/wave5/wave5-vpu.c    | 117 ++++++++++++------
>> .../platform/chips-media/wave5/wave5-vpuapi.h |   4 +
>> 5 files changed, 122 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> index 8433ecab230c..475b7628964f 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> @@ -52,6 +52,8 @@ int wave5_vpu_release_device(struct file *filp,
>>                  char *name)
>> {
>>     struct vpu_instance *inst = wave5_to_vpu_inst(filp->private_data);
>> +    struct vpu_device *dev = inst->dev;
>> +    int ret = 0;
>>
>>     v4l2_m2m_ctx_release(inst->v4l2_fh.m2m_ctx);
>>     if (inst->state != VPU_INST_STATE_NONE) {
>> @@ -71,8 +73,20 @@ int wave5_vpu_release_device(struct file *filp,
>>     }
>>
>>     wave5_cleanup_instance(inst);
>> +    if (dev->irq < 0) {
>> +        ret = mutex_lock_interruptible(&dev->dev_lock);
>> +        if (ret)
>> +            return ret;
>>
>> -    return 0;
>> +        if (list_empty(&dev->instances)) {
>> +            dev_dbg(dev->dev, "Disabling the hrtimer\n");
>> +            hrtimer_cancel(&dev->hrtimer);
>> +        }
>> +
>> +        mutex_unlock(&dev->dev_lock);
>> +    }
>> +
>> +    return ret;
>> }
>>
>> int wave5_vpu_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq,
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> index ef227af72348..c8624c681fa6 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> @@ -1810,7 +1810,6 @@ static int wave5_vpu_open_dec(struct file *filp)
>>     v4l2_fh_add(&inst->v4l2_fh);
>>
>>     INIT_LIST_HEAD(&inst->list);
>> -    list_add_tail(&inst->list, &dev->instances);
>>
>>     inst->v4l2_m2m_dev = inst->dev->v4l2_m2m_dec_dev;
>>     inst->v4l2_fh.m2m_ctx =
>> @@ -1867,6 +1866,18 @@ static int wave5_vpu_open_dec(struct file *filp)
>>
>>     wave5_vdi_allocate_sram(inst->dev);
>>
>> +    ret = mutex_lock_interruptible(&dev->dev_lock);
>> +    if (ret)
>> +        goto cleanup_inst;
>> +
>> +    if (dev->irq < 0 && !hrtimer_active(&dev->hrtimer) && list_empty(&dev->instances))
>> +        hrtimer_start(&dev->hrtimer, ns_to_ktime(dev->vpu_poll_interval * NSEC_PER_MSEC),
>> +                  HRTIMER_MODE_REL_PINNED);
>> +
>> +    list_add_tail(&inst->list, &dev->instances);
>> +
>> +    mutex_unlock(&dev->dev_lock);
>> +
>>     return 0;
>>
>> cleanup_inst:
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> index f29cfa3af94a..9e88424761b6 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> @@ -1554,7 +1554,6 @@ static int wave5_vpu_open_enc(struct file *filp)
>>     v4l2_fh_add(&inst->v4l2_fh);
>>
>>     INIT_LIST_HEAD(&inst->list);
>> -    list_add_tail(&inst->list, &dev->instances);
>>
>>     inst->v4l2_m2m_dev = inst->dev->v4l2_m2m_enc_dev;
>>     inst->v4l2_fh.m2m_ctx =
>> @@ -1729,6 +1728,18 @@ static int wave5_vpu_open_enc(struct file *filp)
>>
>>     wave5_vdi_allocate_sram(inst->dev);
>>
>> +    ret = mutex_lock_interruptible(&dev->dev_lock);
>> +    if (ret)
>> +        goto cleanup_inst;
>> +
>> +    if (dev->irq < 0 && !hrtimer_active(&dev->hrtimer) && list_empty(&dev->instances))
>> +        hrtimer_start(&dev->hrtimer, ns_to_ktime(dev->vpu_poll_interval * NSEC_PER_MSEC),
>> +                  HRTIMER_MODE_REL_PINNED);
>> +
>> +    list_add_tail(&inst->list, &dev->instances);
>> +
>> +    mutex_unlock(&dev->dev_lock);
>> +
>>     return 0;
>>
>> cleanup_inst:
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> index bfe4caa79cc9..968ec9baf1ef 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> @@ -26,6 +26,9 @@ struct wave5_match_data {
>>     const char *fw_name;
>> };
>>
>> +static int vpu_poll_interval = 5;
>> +module_param(vpu_poll_interval, int, 0644);
>> +
>> int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned int timeout)
>> {
>>     int ret;
>> @@ -40,7 +43,7 @@ int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned int timeout)
>>     return 0;
>> }
>>
>> -static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
>> +static void wave5_vpu_handle_irq(void *dev_id)
>> {
>>     u32 seq_done;
>>     u32 cmd_done;
>> @@ -48,42 +51,67 @@ static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
>>     struct vpu_instance *inst;
>>     struct vpu_device *dev = dev_id;
>>
>> -    if (wave5_vdi_read_register(dev, W5_VPU_VPU_INT_STS)) {
>> -        irq_reason = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
>> -        wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_reason);
>> -        wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
>> -
>> -        list_for_each_entry(inst, &dev->instances, list) {
>> -            seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
>> -            cmd_done = wave5_vdi_read_register(dev, W5_RET_QUEUE_CMD_DONE_INST);
>> -
>> -            if (irq_reason & BIT(INT_WAVE5_INIT_SEQ) ||
>> -                irq_reason & BIT(INT_WAVE5_ENC_SET_PARAM)) {
>> -                if (seq_done & BIT(inst->id)) {
>> -                    seq_done &= ~BIT(inst->id);
>> -                    wave5_vdi_write_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO,
>> -                                 seq_done);
>> -                    complete(&inst->irq_done);
>> -                }
>> +    irq_reason = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
>> +    wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_reason);
>> +    wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
>> +
>> +    list_for_each_entry(inst, &dev->instances, list) {
>> +        seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
>> +        cmd_done = wave5_vdi_read_register(dev, W5_RET_QUEUE_CMD_DONE_INST);
>> +
>> +        if (irq_reason & BIT(INT_WAVE5_INIT_SEQ) ||
>> +            irq_reason & BIT(INT_WAVE5_ENC_SET_PARAM)) {
>> +            if (seq_done & BIT(inst->id)) {
>> +                seq_done &= ~BIT(inst->id);
>> +                wave5_vdi_write_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO,
>> +                             seq_done);
>> +                complete(&inst->irq_done);
>>             }
>> +        }
>>
>> -            if (irq_reason & BIT(INT_WAVE5_DEC_PIC) ||
>> -                irq_reason & BIT(INT_WAVE5_ENC_PIC)) {
>> -                if (cmd_done & BIT(inst->id)) {
>> -                    cmd_done &= ~BIT(inst->id);
>> -                    wave5_vdi_write_register(dev, W5_RET_QUEUE_CMD_DONE_INST,
>> -                                 cmd_done);
>> -                    inst->ops->finish_process(inst);
>> -                }
>> +        if (irq_reason & BIT(INT_WAVE5_DEC_PIC) ||
>> +            irq_reason & BIT(INT_WAVE5_ENC_PIC)) {
>> +            if (cmd_done & BIT(inst->id)) {
>> +                cmd_done &= ~BIT(inst->id);
>> +                wave5_vdi_write_register(dev, W5_RET_QUEUE_CMD_DONE_INST,
>> +                             cmd_done);
>> +                inst->ops->finish_process(inst);
>>             }
>> -
>> -            wave5_vpu_clear_interrupt(inst, irq_reason);
>>         }
>> +
>> +        wave5_vpu_clear_interrupt(inst, irq_reason);
>>     }
>> +}
>> +
>> +static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
>> +{
>> +    struct vpu_device *dev = dev_id;
>> +
>> +    if (wave5_vdi_read_register(dev, W5_VPU_VPU_INT_STS))
>> +        wave5_vpu_handle_irq(dev);
>>
>>     return IRQ_HANDLED;
>> }
>>
>> +static void wave5_vpu_irq_work_fn(struct kthread_work *work)
>> +{
>> +    struct vpu_device *dev = container_of(work, struct vpu_device, work);
>> +
>> +    if (wave5_vdi_read_register(dev, W5_VPU_VPU_INT_STS))
>> +        wave5_vpu_handle_irq(dev);
>> +}
>> +
>> +static enum hrtimer_restart wave5_vpu_timer_callback(struct hrtimer *timer)
>> +{
>> +    struct vpu_device *dev =
>> +            container_of(timer, struct vpu_device, hrtimer);
>> +
>> +    kthread_queue_work(dev->worker, &dev->work);
>> +    hrtimer_forward_now(timer, ns_to_ktime(vpu_poll_interval * NSEC_PER_MSEC));
>> +
>> +    return HRTIMER_RESTART;
>> +}
>> +
>> static int wave5_vpu_load_firmware(struct device *dev, const char *fw_name,
>>                    u32 *revision)
>> {
>> @@ -209,16 +237,24 @@ static int wave5_vpu_probe(struct platform_device *pdev)
>>
>>     dev->irq = platform_get_irq(pdev, 0);
>>     if (dev->irq < 0) {
>> -        dev_err(&pdev->dev, "failed to get irq resource\n");
>> -        ret = -ENXIO;
>> -        goto err_enc_unreg;
>> -    }
>> -
>> -    ret = devm_request_threaded_irq(&pdev->dev, dev->irq, NULL,
>> -                    wave5_vpu_irq_thread, IRQF_ONESHOT, "vpu_irq", dev);
>> -    if (ret) {
>> -        dev_err(&pdev->dev, "Register interrupt handler, fail: %d\n", ret);
>> -        goto err_enc_unreg;
>> +        dev_err(&pdev->dev, "failed to get irq resource, falling back to polling\n");
>> +        hrtimer_init(&dev->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
>> +        dev->hrtimer.function = &wave5_vpu_timer_callback;
>> +        dev->worker = kthread_create_worker(0, "vpu_irq_thread");
>> +        if (IS_ERR(dev->worker)) {
>> +            dev_err(&pdev->dev, "failed to create vpu irq worker\n");
>> +            ret = PTR_ERR(dev->worker);
>> +            goto err_vdi_release;
>> +        }
>> +        dev->vpu_poll_interval = vpu_poll_interval;
>> +        kthread_init_work(&dev->work, wave5_vpu_irq_work_fn);
>> +    } else {
>> +        ret = devm_request_threaded_irq(&pdev->dev, dev->irq, NULL,
>> +                        wave5_vpu_irq_thread, IRQF_ONESHOT, "vpu_irq", dev);
>> +        if (ret) {
>> +            dev_err(&pdev->dev, "Register interrupt handler, fail: %d\n", ret);
>> +            goto err_enc_unreg;
>> +        }
>>     }
>>
>>     ret = wave5_vpu_load_firmware(&pdev->dev, match_data->fw_name, &fw_revision);
>> @@ -254,6 +290,11 @@ static int wave5_vpu_remove(struct platform_device *pdev)
>> {
>>     struct vpu_device *dev = dev_get_drvdata(&pdev->dev);
>>
>> +    if (dev->irq < 0) {
>> +        kthread_destroy_worker(dev->worker);
>> +        hrtimer_cancel(&dev->hrtimer);
>> +    }
>> +
>>     mutex_destroy(&dev->dev_lock);
>>     mutex_destroy(&dev->hw_lock);
>>     clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> index 352f6e904e50..edc50450ddb8 100644
>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> @@ -756,6 +756,10 @@ struct vpu_device {
>>     u32 product_code;
>>     struct ida inst_ida;
>>     struct clk_bulk_data *clks;
>> +    struct hrtimer hrtimer;
>> +    struct kthread_work work;
>> +    struct kthread_worker *worker;
>> +    int vpu_poll_interval;
>>     int num_clks;
>> };
>>
>> -- 
>> 2.34.1
>>
>>
>