Message-ID: <367daea8-9412-757a-0ea1-e304249528a0@ti.com>
Date: Wed, 28 Feb 2024 21:41:23 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Andrew Davis <afd@...com>,
        Sebastian Fricke
	<sebastian.fricke@...labora.com>
CC: <nas.chung@...psnmedia.com>, <jackson.lee@...psnmedia.com>,
        <mchehab@...nel.org>, <linux-media@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <nm@...com>, <praneeth@...com>,
        <vigneshr@...com>, <a-bhatia1@...com>, <j-luthra@...com>,
        <b-brnich@...com>, <detheridge@...com>, <p-mantena@...com>,
        <vijayp@...com>
Subject: Re: [PATCH] media: chips-media: wave5: Add hrtimer based polling
 support

Hi Andrew,

Thanks for the review.

On 22/02/24 21:41, Andrew Davis wrote:
> On 2/16/24 3:45 AM, Sebastian Fricke wrote:
>> Hey Devarsh,
>>
>> @nas.chung@...psnmedia.com or @jackson.lee@...psnmedia.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
> 

Well at least for the video codec type of scenarios where multiple instances
of encode & decode can run together and we require to achieve balance between
optimum CPU utilization, real-time performance and latencies we require an
intelligent polling based scheme as implemented in this patchset and I don't
think the fakeirq approach would help to replicate an optimum solution given
all the meta-data related to instance handling and polling interval is present
in video codec driver.

I already shared a good amount of reasoning here [0], but to summarize in
general the scheme we want to follow (and also the scheme which this patchset
implements) is to have a single polling thread for all instances of encoder
and decoder so as to keep the CPU utilization in check while achieving
real-time latencies and performance as seen in the results [1].

For e.g. we only need to start polling when first instance gets launched from
user-space and this information is known only to VPU driver so it would still
need to signal to irq chip (possibly using irq_enable hook) to start polling
on first instance launch.

And again we need to stop polling when last instance gets closed, now here
again driver only knows when the last VPU instance gets closed so the VPU
driver would need to signal to irqchip again to stop polling.

And all these handling requires locking related handling too as multiple
instances can be launched anytime and of any type (either for encoder and
decoder) and this logic would again fall in VPU driver.

Secondly it makes more sense to keep VPU polling interval param inside the VPU
driver rather than fakeirq driver (which would ideally be agnostic to VPU and
handling other peripherals too) as user may want to change it to a different
value than defaults if they have a preference to a subset of fixed use-cases
or want to give more preference to CPU utilizations over Performance/latency
or vice-versa.

Also if we look at it from another angle in some scenario the polling scheme
may offer some advantage over the irq too if system is already under high
interrupt load. For e.g let's say we want to run a 32 instance use-case for a
non real-time processing of video. Now with the polling scheme implemented
here, there would be a single polling thread which would trigger a softirq
every X ms as compare to 32 interrupts being fired per frame for each instance
when irq is being used, and if the system is already running under heavy
interrupt load than it may make more sense to switch to polling if the
use-case doesn't demand aggressive latencies (as with polling VPU latencies
would be slightly higher but that's okay for non real-time video processing
scenarios ). So I think polling feature implemented may not be completely
use-less in general too for SoC's where VPU irq is present.

[0] :
https://lore.kernel.org/all/47fe5d24-4331-a88c-5481-d60a3ae96efa@ti.com/

[1] :
1x 640x480@25 Encoding :
https://gist.github.com/devarsht/ee9664d3403d1212ef477a027b71896c

4x 1080p30 Transcode (File→decode→encode→file).csv irq vs polling comparison :
https://gist.github.com/devarsht/3a58b4f201430dfc61697c7e224e74c2

1x 1080p transcode (File→decode→encode→file) polling_vs_irq_transcode
comparison :
https://gist.github.com/devarsht/a6480f1f2cbdf8dd694d698309d81fb0

1080p60 streaming use-case for irq vs polling comparison
https://gist.github.com/devarsht/44aaa4322454e85e01a8d65ac47c5edb

Regards
Devarsh

>> 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@...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
>>>
>>>
>>

Powered by blists - more mailing lists