[PATCH v2 0/4] Assorted CODA fixes

[PATCH v2 0/4] Assorted CODA fixes

[Ezequiel Garcia]

While working on a SoC with CODA980 (currently not supported by the
driver), I came across some low-hanging fruit that looked worth
addressing.

Patch 1 is just a cosmetic change to print a different "more standard"
device registered message, if such thing exists.

Patches 2 and 3 address unneeded usage of a threaded interrupt.
Careful code inspection shows the interrupt can be serviced
on a normal handler.

Finally, patch 4 clears a hardware register called INT_REASON.
Without this clearing the firmware has been found to get stuck
on our CODA980. I believe this fix might be useful to carry upstream,
because it's so small and we have some  indications that it's actually
the right thing to do.

Not much has changed in this series, compared to v1.

v2:

 * Add R-B by Philip.
 * Drop the worker removal patch, which is a controversial change.
 * Use video_device_node_name as suggested by Hans.

Ezequiel Garcia (4):
  media: coda: Print a nicer device registered message
  media: coda: Remove unbalanced and unneeded mutex unlock
  media: coda: Replace the threaded interrupt with a hard interrupt
  media: coda: Clear the interrupt reason

 drivers/media/platform/coda/coda-bit.c    |  2 +-
 drivers/media/platform/coda/coda-common.c | 17 +++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] media: coda: Print a nicer device registered message

[Ezequiel Garcia]

This is just a cosmetic change to print a more descriptive
message, to distinguish decoder from encoder:

So, instead of printing

  coda 2040000.vpu: codec registered as /dev/video[4-5]

With this change, the driver now prints

  coda 2040000.vpu: encoder registered as /dev/video4
  coda 2040000.vpu: decoder registered as /dev/video5

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-common.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 3ce58dee4422..39421ded5c63 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2480,9 +2480,12 @@ static int coda_hw_init(struct coda_dev *dev)
 static int coda_register_device(struct coda_dev *dev, int i)
 {
 	struct video_device *vfd = &dev->vfd[i];
+	enum coda_inst_type type;
+	int ret;
 
 	if (i >= dev->devtype->num_vdevs)
 		return -EINVAL;
+	type = dev->devtype->vdevs[i]->type;
 
 	strscpy(vfd->name, dev->devtype->vdevs[i]->name, sizeof(vfd->name));
 	vfd->fops	= &coda_fops;
@@ -2498,7 +2501,12 @@ static int coda_register_device(struct coda_dev *dev, int i)
 	v4l2_disable_ioctl(vfd, VIDIOC_G_CROP);
 	v4l2_disable_ioctl(vfd, VIDIOC_S_CROP);
 
-	return video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+	if (!ret)
+		v4l2_info(&dev->v4l2_dev, "%s registered as %s\n",
+			  type == CODA_INST_ENCODER ? "encoder" : "decoder",
+			  video_device_node_name(vfd));
+	return ret;
 }
 
 static void coda_copy_firmware(struct coda_dev *dev, const u8 * const buf,
@@ -2612,9 +2620,6 @@ static void coda_fw_callback(const struct firmware *fw, void *context)
 		}
 	}
 
-	v4l2_info(&dev->v4l2_dev, "codec registered as /dev/video[%d-%d]\n",
-		  dev->vfd[0].num, dev->vfd[i - 1].num);
-
 	pm_runtime_put_sync(&pdev->dev);
 	return;
 
-- 
2.20.1

[PATCH v2 2/4] media: coda: Remove unbalanced and unneeded mutex unlock

[Ezequiel Garcia]

The mutex unlock in the threaded interrupt handler is not paired
with any mutex lock. Remove it.

This bug has been here for a really long time, so it applies
to any stable repo.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-bit.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index eaa86737fa04..bddd2f5c8c2b 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2308,7 +2308,6 @@ irqreturn_t coda_irq_handler(int irq, void *data)
 	if (ctx == NULL) {
 		v4l2_err(&dev->v4l2_dev,
 			 "Instance released before the end of transaction\n");
-		mutex_unlock(&dev->coda_mutex);
 		return IRQ_HANDLED;
 	}
 
-- 
2.20.1

[PATCH v2 3/4] media: coda: Replace the threaded interrupt with a hard interrupt

[Ezequiel Garcia]

The current interrupt handler is doing very little, and not doing
any non-atomic operations. Pretty much all it does is accessing
a couple registers, taking a couple spinlocks and then signalling
a completion.

There is no reason this should be a threaded interrupt handler,
so move the handler to regular hard interrupt context.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 39421ded5c63..a8848a4e927a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2789,8 +2789,8 @@ static int coda_probe(struct platform_device *pdev)
 		return irq;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, coda_irq_handler,
-			IRQF_ONESHOT, dev_name(&pdev->dev), dev);
+	ret = devm_request_irq(&pdev->dev, irq, coda_irq_handler, 0,
+			       dev_name(&pdev->dev), dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to request irq: %d\n", ret);
 		return ret;
-- 
2.20.1

[PATCH v2 4/4] media: coda: Clear the interrupt reason

[Ezequiel Garcia]

This commit clears the interrupt reason (INT_REASON) register
on the interrupt handler. Without this clearing, the CODA hardware
has been observed to get completely stalled on CODA980 variants,
requiring a pretty deep hardware reset.

The datasheet specifies that the INT_REASON register is set
by the CODA hardware, and should be cleared by the host.

While the CODA versions that are currently supported by this driver
don't seem to need this change, it's a really small change,
so it seems a wise thing to do to avoid hitting some
rare race-condition in the hardware.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-bit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index bddd2f5c8c2b..a653aaa3ca15 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2301,6 +2301,7 @@ irqreturn_t coda_irq_handler(int irq, void *data)
 
 	/* read status register to attend the IRQ */
 	coda_read(dev, CODA_REG_BIT_INT_STATUS);
+	coda_write(dev, 0, CODA_REG_BIT_INT_REASON);
 	coda_write(dev, CODA_REG_BIT_INT_CLEAR_SET,
 		      CODA_REG_BIT_INT_CLEAR);
 
-- 
2.20.1

Re: [PATCH v2 1/4] media: coda: Print a nicer device registered message

[Philipp Zabel]

Hi Ezequiel,

On Thu, 2019-05-02 at 19:00 -0300, Ezequiel Garcia wrote:
> This is just a cosmetic change to print a more descriptive
> message, to distinguish decoder from encoder:
> 
> So, instead of printing
> 
>   coda 2040000.vpu: codec registered as /dev/video[4-5]
> 
> With this change, the driver now prints
> 
>   coda 2040000.vpu: encoder registered as /dev/video4
>   coda 2040000.vpu: decoder registered as /dev/video5
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 0/4] Assorted CODA fixes

[Ezequiel Garcia]

On Thu, 2019-05-02 at 19:00 -0300, Ezequiel Garcia wrote:
> While working on a SoC with CODA980 (currently not supported by the
> driver), I came across some low-hanging fruit that looked worth
> addressing.
> 
> Patch 1 is just a cosmetic change to print a different "more standard"
> device registered message, if such thing exists.
> 
> Patches 2 and 3 address unneeded usage of a threaded interrupt.
> Careful code inspection shows the interrupt can be serviced
> on a normal handler.
> 
> Finally, patch 4 clears a hardware register called INT_REASON.
> Without this clearing the firmware has been found to get stuck
> on our CODA980. I believe this fix might be useful to carry upstream,
> because it's so small and we have some  indications that it's actually
> the right thing to do.
> 
> Not much has changed in this series, compared to v1.
> 
> v2:
> 
>  * Add R-B by Philip.
>  * Drop the worker removal patch, which is a controversial change.
>  * Use video_device_node_name as suggested by Hans.
> 
> Ezequiel Garcia (4):
>   media: coda: Print a nicer device registered message
>   media: coda: Remove unbalanced and unneeded mutex unlock
>   media: coda: Replace the threaded interrupt with a hard interrupt
>   media: coda: Clear the interrupt reason
> 
>  drivers/media/platform/coda/coda-bit.c    |  2 +-
>  drivers/media/platform/coda/coda-common.c | 17 +++++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 

Hi Hans,

Any other feedback on these?

Thanks,
Ezequiel