[PATCH 0/5] Assorted CODA fixes

[PATCH 0/5] 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 to 4 address unneeded usage of threaded interrupt and deferred
worker. Careful code inspection shows the interrupt can be serviced
on a normal handler.

In this regard, the driver is still waiting for job completion in
the .device_run hook, which the framework already does. Therefore,
there are two waits: one in the driver, one in the mem2mem core.

Please note that I'm not addressing this for now.

Finally, patch 5 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.

Ezequiel Garcia (5):
  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: Remove pic_run_work worker
  media: coda: Clear the interrupt reason

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

-- 
2.20.1

[PATCH 1/5] 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..1f53ca4effd2 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 /dev/video%d\n",
+			  type == CODA_INST_ENCODER ? "encoder" : "decoder",
+			  vfd->num);
+	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 2/5] 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.

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 3/5] 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.

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 1f53ca4effd2..617d4547ec82 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 4/5] media: coda: Remove pic_run_work worker

[Ezequiel Garcia]

There isn't any reason to run the mem2mem job on a separate worker,
because the mem2mem framework guarantees that device_run will never
run in interrupt context.

Therefore, get rid of the extra pic_run_work worker, and instead
run the job in the context of .device_run itself.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-common.c | 9 ---------
 drivers/media/platform/coda/coda.h        | 1 -
 2 files changed, 10 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 617d4547ec82..e0227593a649 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1306,14 +1306,6 @@ static void coda_device_run(void *m2m_priv)
 {
 	struct coda_ctx *ctx = m2m_priv;
 	struct coda_dev *dev = ctx->dev;
-
-	queue_work(dev->workqueue, &ctx->pic_run_work);
-}
-
-static void coda_pic_run_work(struct work_struct *work)
-{
-	struct coda_ctx *ctx = container_of(work, struct coda_ctx, pic_run_work);
-	struct coda_dev *dev = ctx->dev;
 	int ret;
 
 	mutex_lock(&ctx->buffer_mutex);
@@ -2233,7 +2225,6 @@ static int coda_open(struct file *file)
 	ctx->ops = ctx->cvd->ops;
 	ctx->use_bit = !ctx->cvd->direct;
 	init_completion(&ctx->completion);
-	INIT_WORK(&ctx->pic_run_work, coda_pic_run_work);
 	if (ctx->ops->seq_end_work)
 		INIT_WORK(&ctx->seq_end_work, ctx->ops->seq_end_work);
 	v4l2_fh_init(&ctx->fh, video_devdata(file));
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 31c80bda2c0b..6870edeb6324 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -194,7 +194,6 @@ struct coda_context_ops {
 struct coda_ctx {
 	struct coda_dev			*dev;
 	struct mutex			buffer_mutex;
-	struct work_struct		pic_run_work;
 	struct work_struct		seq_end_work;
 	struct completion		completion;
 	const struct coda_video_device	*cvd;
-- 
2.20.1

[PATCH 5/5] 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.

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 1/5] media: coda: Print a nicer device registered message

[Hans Verkuil]

On 4/25/19 8:35 PM, 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>
> ---
>  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..1f53ca4effd2 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 /dev/video%d\n",
> +			  type == CODA_INST_ENCODER ? "encoder" : "decoder",
> +			  vfd->num);

You can use video_device_node_name() to print the video node name.

Regards,

	Hans

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

Re: [PATCH 2/5] media: coda: Remove unbalanced and unneeded mutex unlock

[Philipp Zabel]

On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> 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.
> 
> 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;
>  	}

I think this is right. I've never seen this message, and I'm not sure if
it is even possible to get a PIC_RUN completion interrupt with the m2m
context already released, since we reset the hardware on timeouts in
pic_run_work. Either way, the mutex is unlocked in pic_run_work after a
timeout if the completion is not triggered, so this one has to go.

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

regards
Philipp

Re: [PATCH 3/5] media: coda: Replace the threaded interrupt with a hard interrupt

[Philipp Zabel]

On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> 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.
> 
> 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 1f53ca4effd2..617d4547ec82 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;

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

regards
Philipp

Re: [PATCH 4/5] media: coda: Remove pic_run_work worker

[Philipp Zabel]

On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> There isn't any reason to run the mem2mem job on a separate worker,
> because the mem2mem framework guarantees that device_run will never
> run in interrupt context.

The purpose of the workqueue is to serialize BIT processor commands,
currently the PIC_RUN commands issued by the mem2mem framework (as well
as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END
command issued directly from the STREAMOFF ioctl.
Further, to fully support the stateful decoder API we'll have to move
SEQ_INIT out of the mem2mem device_run as well, since that should be
called on queued OUTPUT buffers before the CAPTURE side is even
streaming.

regards
Philipp

Re: [PATCH 5/5] media: coda: Clear the interrupt reason

[Philipp Zabel]

On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> 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.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

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

regards
Philipp

Re: [PATCH 3/5] media: coda: Replace the threaded interrupt with a hard interrupt

[Philipp Zabel]

On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> 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.
> 
> 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 1f53ca4effd2..617d4547ec82 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;

There is one thing that this would complicate. For at least H.264 I
know that we can pad the bitstream in reaction to a BIT_BUF_EMPTY
interrupt, to make a picture run finish successfully when it is stuck in
  buffer underrun condition. This would need to be done under the
bitstream_mutex in a threaded handler.

regards
Philipp

Re: [PATCH 3/5] media: coda: Replace the threaded interrupt with a hard interrupt

[Ezequiel Garcia]

Hi Philipp,

Thanks for the quick review of the series.

On Fri, 2019-04-26 at 11:16 +0200, Philipp Zabel wrote:
> On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> > 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.
> > 
> > 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 1f53ca4effd2..617d4547ec82 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;
> 
> There is one thing that this would complicate. For at least H.264 I
> know that we can pad the bitstream in reaction to a BIT_BUF_EMPTY
> interrupt, to make a picture run finish successfully when it is stuck in
>   buffer underrun condition. This would need to be done under the
> bitstream_mutex in a threaded handler.
> 

In this case, a top-half handler would still stand in good stead,
as it would check the interrupt status, and fire the bottom-half
threaded interrupt to handle the buffer underrun.

Thanks,
Ezequiel

Re: [PATCH 4/5] media: coda: Remove pic_run_work worker

[Ezequiel Garcia]

On Fri, 2019-04-26 at 10:11 +0200, Philipp Zabel wrote:
> On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> > There isn't any reason to run the mem2mem job on a separate worker,
> > because the mem2mem framework guarantees that device_run will never
> > run in interrupt context.
> 
> The purpose of the workqueue is to serialize BIT processor commands,
> currently the PIC_RUN commands issued by the mem2mem framework (as well
> as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END
> command issued directly from the STREAMOFF ioctl.

Right, but that's serialized by the coda_mutex, not by the worker, right?

> Further, to fully support the stateful decoder API we'll have to move
> SEQ_INIT out of the mem2mem device_run as well, since that should be
> called on queued OUTPUT buffers before the CAPTURE side is even
> streaming.
> 

Isn't that already done? I see SEQ_INIT being issued in start_streaming.
In what sense is this driver currently violating the decoder spec?

So, returning to the serialization. I believe commands are still
serialized after this change.

The pic_run_work worker is only queued from .device_run.
Only one job can run on a context at any given time,
but multiple contexts can run in parallel.

Inside the worker, the coda_mutex serializes the commands.
The worker waits until the command has finished execution.

So, with this commit, there is no longer a worker, but commands
are still serialized by the mutex and the fact that the command
is completed in .device_run.

That being said, the worker does makes the serialization
more clear, so I think dropping this patch is better.

Perhaps adding a small comment so the purpose of pic_run_work
is clear.

Thanks,
Eze

Re: [PATCH 3/5] media: coda: Replace the threaded interrupt with a hard interrupt

[Philipp Zabel]

On Fri, 2019-04-26 at 14:56 -0300, Ezequiel Garcia wrote:
> Hi Philipp,
> 
> Thanks for the quick review of the series.
> 
> On Fri, 2019-04-26 at 11:16 +0200, Philipp Zabel wrote:
> > On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> > > 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.
> > > 
> > > 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 1f53ca4effd2..617d4547ec82 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;
> > 
> > There is one thing that this would complicate. For at least H.264 I
> > know that we can pad the bitstream in reaction to a BIT_BUF_EMPTY
> > interrupt, to make a picture run finish successfully when it is stuck in
> >   buffer underrun condition. This would need to be done under the
> > bitstream_mutex in a threaded handler.
> > 
> 
> In this case, a top-half handler would still stand in good stead,
> as it would check the interrupt status, and fire the bottom-half
> threaded interrupt to handle the buffer underrun.

I agree, and the R-b stands.

regards
Philipp

Re: [PATCH 4/5] media: coda: Remove pic_run_work worker

[Philipp Zabel]

On Fri, 2019-04-26 at 15:19 -0300, Ezequiel Garcia wrote:
> On Fri, 2019-04-26 at 10:11 +0200, Philipp Zabel wrote:
> > On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote:
> > > There isn't any reason to run the mem2mem job on a separate worker,
> > > because the mem2mem framework guarantees that device_run will never
> > > run in interrupt context.
> > 
> > The purpose of the workqueue is to serialize BIT processor commands,
> > currently the PIC_RUN commands issued by the mem2mem framework (as well
> > as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END
> > command issued directly from the STREAMOFF ioctl.
> 
> Right, but that's serialized by the coda_mutex, not by the worker, right?

Hmm, coda_start_decoding as called from .start_streaming is indeed just
serialized by the device coda_mutex. I would prefer this to be scheduled
as a work item as well though, those are way easier to reason about.

As an aside, while SEQ_INIT on its own is fast, technically
.start_streaming (or .buf_queue, see below) wouldn't even have to wait
until the SEQ_INIT command has finished, in case the BIT processor is
busy decoding for another context.

> > Further, to fully support the stateful decoder API we'll have to move
> > SEQ_INIT out of the mem2mem device_run as well, since that should be
> > called on queued OUTPUT buffers before the CAPTURE side is even
> > streaming.
> 
> Isn't that already done? I see SEQ_INIT being issued in start_streaming.
> In what sense is this driver currently violating the decoder spec?

I suppose it should try SEQ_INIT on every OUTPUT .buf_queue as long as
initialization hasn't succeeded, not only on .start_streaming of the
second queue (whichever is last). Right now, the buffers fed into the
output queue before STREAMON on both OUTPUT and CAPTURE must already
contain headers, or stream start will fail.

> So, returning to the serialization. I believe commands are still
> serialized after this change.

While I'm not sure if any odd corner cases could still arise with this
change, I think you are right. I remember having a few deadlock issues
with the locking before adding the workqueue, but that was before we
could just wait for the device to finish in .device_run.

> The pic_run_work worker is only queued from .device_run.
> Only one job can run on a context at any given time,
> but multiple contexts can run in parallel.
> 
> Inside the worker, the coda_mutex serializes the commands.
> The worker waits until the command has finished execution.
> 
> So, with this commit, there is no longer a worker, but commands
> are still serialized by the mutex and the fact that the command
> is completed in .device_run.
> 
> That being said, the worker does makes the serialization
> more clear, so I think dropping this patch is better.

I agree with both analysis and conclusion.

> Perhaps adding a small comment so the purpose of pic_run_work
> is clear.

Yes, that is a good idea.

regards
Philipp