Hello Sebastian > @nas.chung@xxxxxxxxxxxxxxx or @jackson.lee@xxxxxxxxxxxxxxx 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. > 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@xxxxxx> For this patch, Tested-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx> > >--- > >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 > > > >
