[PATCH v2 0/3] mem2mem: Cleanup .job_abort

[PATCH v2 0/3] mem2mem: Cleanup .job_abort

[Ezequiel Garcia]

Here's the second version of this cleanup series.
Only those patches that haven't been picked are
being submitted. Therefore, just the .job_abort
change is here.

As requested by Hans, I am sending the changes
to rcar_jpu and s5p-g2d drivers independently
so they can be Acked by their maintainers.

The idea in this series is to make .job_abort optional.
Some drivers cannot implement anything rational in
.job_abort, so it does not make much sense for it to be
mandatory.

Regarding rcar and s5p drivers, job_abort is not expected
to wait for a job to complete, so the waits in these
drivers is not needed, and actually suboptimal.

Ezequiel Garcia (3):
  rcar_jpu: Remove unrequired wait in .job_abort
  s5p-g2d: Remove unrequired wait in .job_abort
  mem2mem: Make .job_abort optional

 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c |  5 -----
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c    |  5 -----
 drivers/media/platform/rcar_jpu.c               | 16 ----------------
 drivers/media/platform/rockchip/rga/rga.c       |  6 ------
 drivers/media/platform/s5p-g2d/g2d.c            | 16 ----------------
 drivers/media/platform/s5p-g2d/g2d.h            |  1 -
 drivers/media/platform/s5p-jpeg/jpeg-core.c     |  7 -------
 drivers/media/v4l2-core/v4l2-mem2mem.c          |  6 +++---
 include/media/v4l2-mem2mem.h                    |  2 +-
 9 files changed, 4 insertions(+), 60 deletions(-)

-- 
2.16.3

[PATCH v2 1/3] rcar_jpu: Remove unrequired wait in .job_abort

[Ezequiel Garcia]

As per the documentation, job_abort is not required
to wait until the current job finishes. It is redundant
to do so, as the core will perform the wait operation.

Remove the wait infrastructure completely.

Cc: Mikhail Ulyanov <mikhail.ulyanov@cogentembedded.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/rcar_jpu.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/media/platform/rcar_jpu.c b/drivers/media/platform/rcar_jpu.c
index 469a326838aa..dec696e6b974 100644
--- a/drivers/media/platform/rcar_jpu.c
+++ b/drivers/media/platform/rcar_jpu.c
@@ -198,7 +198,6 @@
  * @vfd_decoder: video device node for decoder mem2mem mode
  * @m2m_dev: v4l2 mem2mem device data
  * @curr: pointer to current context
- * @irq_queue:	interrupt handler waitqueue
  * @regs: JPEG IP registers mapping
  * @irq: JPEG IP irq
  * @clk: JPEG IP clock
@@ -213,7 +212,6 @@ struct jpu {
 	struct video_device	vfd_decoder;
 	struct v4l2_m2m_dev	*m2m_dev;
 	struct jpu_ctx		*curr;
-	wait_queue_head_t	irq_queue;
 
 	void __iomem		*regs;
 	unsigned int		irq;
@@ -1494,11 +1492,6 @@ static void jpu_device_run(void *priv)
 
 static void jpu_job_abort(void *priv)
 {
-	struct jpu_ctx *ctx = priv;
-
-	if (!wait_event_timeout(ctx->jpu->irq_queue, !ctx->jpu->curr,
-				msecs_to_jiffies(JPU_JOB_TIMEOUT)))
-		jpu_cleanup(ctx, true);
 }
 
 static const struct v4l2_m2m_ops jpu_m2m_ops = {
@@ -1584,9 +1577,6 @@ static irqreturn_t jpu_irq_handler(int irq, void *dev_id)
 
 	v4l2_m2m_job_finish(jpu->m2m_dev, curr_ctx->fh.m2m_ctx);
 
-	/* ...wakeup abort routine if needed */
-	wake_up(&jpu->irq_queue);
-
 	return IRQ_HANDLED;
 
 handled:
@@ -1620,7 +1610,6 @@ static int jpu_probe(struct platform_device *pdev)
 	if (!jpu)
 		return -ENOMEM;
 
-	init_waitqueue_head(&jpu->irq_queue);
 	mutex_init(&jpu->mutex);
 	spin_lock_init(&jpu->lock);
 	jpu->dev = &pdev->dev;
-- 
2.16.3

[PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Ezequiel Garcia]

As per the documentation, job_abort is not required
to wait until the current job finishes. It is redundant
to do so, as the core will perform the wait operation.

Remove the wait infrastructure completely.

Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Kamil Debski <kamil@wypas.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/s5p-g2d/g2d.c | 11 -----------
 drivers/media/platform/s5p-g2d/g2d.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 66aa8cf1d048..e98708883413 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop *c
 
 static void job_abort(void *prv)
 {
-	struct g2d_ctx *ctx = prv;
-	struct g2d_dev *dev = ctx->dev;
-
-	if (dev->curr == NULL) /* No job currently running */
-		return;
-
-	wait_event_timeout(dev->irq_queue,
-			   dev->curr == NULL,
-			   msecs_to_jiffies(G2D_TIMEOUT));
 }
 
 static void device_run(void *prv)
@@ -563,7 +554,6 @@ static irqreturn_t g2d_isr(int irq, void *prv)
 	v4l2_m2m_job_finish(dev->m2m_dev, ctx->fh.m2m_ctx);
 
 	dev->curr = NULL;
-	wake_up(&dev->irq_queue);
 	return IRQ_HANDLED;
 }
 
@@ -633,7 +623,6 @@ static int g2d_probe(struct platform_device *pdev)
 	spin_lock_init(&dev->ctrl_lock);
 	mutex_init(&dev->mutex);
 	atomic_set(&dev->num_inst, 0);
-	init_waitqueue_head(&dev->irq_queue);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
diff --git a/drivers/media/platform/s5p-g2d/g2d.h b/drivers/media/platform/s5p-g2d/g2d.h
index dd812b557e87..9ffb458a1b93 100644
--- a/drivers/media/platform/s5p-g2d/g2d.h
+++ b/drivers/media/platform/s5p-g2d/g2d.h
@@ -31,7 +31,6 @@ struct g2d_dev {
 	struct g2d_ctx		*curr;
 	struct g2d_variant	*variant;
 	int irq;
-	wait_queue_head_t	irq_queue;
 };
 
 struct g2d_frame {
-- 
2.16.3

[PATCH v2 3/3] mem2mem: Make .job_abort optional

[Ezequiel Garcia]

Implementing job_abort() does not make sense on some drivers.
This is not a problem, as the abort is not required to
wait for the job to finish. Quite the opposite, drivers
are encouraged not to wait.

Demote v4l2_m2m_ops.job_abort from required to optional, and
clean all drivers with dummy implementations.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 5 -----
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c    | 5 -----
 drivers/media/platform/rcar_jpu.c               | 5 -----
 drivers/media/platform/rockchip/rga/rga.c       | 6 ------
 drivers/media/platform/s5p-g2d/g2d.c            | 5 -----
 drivers/media/platform/s5p-jpeg/jpeg-core.c     | 7 -------
 drivers/media/v4l2-core/v4l2-mem2mem.c          | 6 +++---
 include/media/v4l2-mem2mem.h                    | 2 +-
 8 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index af17aaa21f58..b0c24e0426db 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -861,14 +861,9 @@ static int mtk_jpeg_job_ready(void *priv)
 	return (ctx->state == MTK_JPEG_RUNNING) ? 1 : 0;
 }
 
-static void mtk_jpeg_job_abort(void *priv)
-{
-}
-
 static const struct v4l2_m2m_ops mtk_jpeg_m2m_ops = {
 	.device_run = mtk_jpeg_device_run,
 	.job_ready  = mtk_jpeg_job_ready,
-	.job_abort  = mtk_jpeg_job_abort,
 };
 
 static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq,
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index 583d47724ee8..1198e19060a2 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -455,10 +455,6 @@ static void mtk_mdp_m2m_stop_streaming(struct vb2_queue *q)
 	pm_runtime_put(&ctx->mdp_dev->pdev->dev);
 }
 
-static void mtk_mdp_m2m_job_abort(void *priv)
-{
-}
-
 /* The color format (num_planes) must be already configured. */
 static void mtk_mdp_prepare_addr(struct mtk_mdp_ctx *ctx,
 				 struct vb2_buffer *vb,
@@ -1227,7 +1223,6 @@ static const struct v4l2_file_operations mtk_mdp_m2m_fops = {
 
 static const struct v4l2_m2m_ops mtk_mdp_m2m_ops = {
 	.device_run	= mtk_mdp_m2m_device_run,
-	.job_abort	= mtk_mdp_m2m_job_abort,
 };
 
 int mtk_mdp_register_m2m_device(struct mtk_mdp_dev *mdp)
diff --git a/drivers/media/platform/rcar_jpu.c b/drivers/media/platform/rcar_jpu.c
index dec696e6b974..c2230d24eb11 100644
--- a/drivers/media/platform/rcar_jpu.c
+++ b/drivers/media/platform/rcar_jpu.c
@@ -1490,13 +1490,8 @@ static void jpu_device_run(void *priv)
 	spin_unlock_irqrestore(&ctx->jpu->lock, flags);
 }
 
-static void jpu_job_abort(void *priv)
-{
-}
-
 static const struct v4l2_m2m_ops jpu_m2m_ops = {
 	.device_run	= jpu_device_run,
-	.job_abort	= jpu_job_abort,
 };
 
 /*
diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 5a5a6139e18a..d833bb32a538 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -39,11 +39,6 @@
 static int debug;
 module_param(debug, int, 0644);
 
-static void job_abort(void *prv)
-{
-	/* Can't do anything rational here */
-}
-
 static void device_run(void *prv)
 {
 	struct rga_ctx *ctx = prv;
@@ -104,7 +99,6 @@ static irqreturn_t rga_isr(int irq, void *prv)
 
 static struct v4l2_m2m_ops rga_m2m_ops = {
 	.device_run = device_run,
-	.job_abort = job_abort,
 };
 
 static int
diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index e98708883413..680a1b94a19a 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -481,10 +481,6 @@ static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop *c
 	return 0;
 }
 
-static void job_abort(void *prv)
-{
-}
-
 static void device_run(void *prv)
 {
 	struct g2d_ctx *ctx = prv;
@@ -603,7 +599,6 @@ static const struct video_device g2d_videodev = {
 
 static const struct v4l2_m2m_ops g2d_m2m_ops = {
 	.device_run	= device_run,
-	.job_abort	= job_abort,
 };
 
 static const struct of_device_id exynos_g2d_match[];
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 79b63da27f53..04fd2e0493c0 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2467,26 +2467,19 @@ static int s5p_jpeg_job_ready(void *priv)
 	return 1;
 }
 
-static void s5p_jpeg_job_abort(void *priv)
-{
-}
-
 static struct v4l2_m2m_ops s5p_jpeg_m2m_ops = {
 	.device_run	= s5p_jpeg_device_run,
 	.job_ready	= s5p_jpeg_job_ready,
-	.job_abort	= s5p_jpeg_job_abort,
 };
 
 static struct v4l2_m2m_ops exynos3250_jpeg_m2m_ops = {
 	.device_run	= exynos3250_jpeg_device_run,
 	.job_ready	= s5p_jpeg_job_ready,
-	.job_abort	= s5p_jpeg_job_abort,
 };
 
 static struct v4l2_m2m_ops exynos4_jpeg_m2m_ops = {
 	.device_run	= exynos4_jpeg_device_run,
 	.job_ready	= s5p_jpeg_job_ready,
-	.job_abort	= s5p_jpeg_job_abort,
 };
 
 /*
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c4f963d96a79..7bae19ca611e 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -300,7 +300,8 @@ static void v4l2_m2m_cancel_job(struct v4l2_m2m_ctx *m2m_ctx)
 	m2m_ctx->job_flags |= TRANS_ABORT;
 	if (m2m_ctx->job_flags & TRANS_RUNNING) {
 		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
-		m2m_dev->m2m_ops->job_abort(m2m_ctx->priv);
+		if (m2m_dev->m2m_ops->job_abort)
+			m2m_dev->m2m_ops->job_abort(m2m_ctx->priv);
 		dprintk("m2m_ctx %p running, will wait to complete", m2m_ctx);
 		wait_event(m2m_ctx->finished,
 				!(m2m_ctx->job_flags & TRANS_RUNNING));
@@ -599,8 +600,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
 {
 	struct v4l2_m2m_dev *m2m_dev;
 
-	if (!m2m_ops || WARN_ON(!m2m_ops->device_run) ||
-			WARN_ON(!m2m_ops->job_abort))
+	if (!m2m_ops || WARN_ON(!m2m_ops->device_run))
 		return ERR_PTR(-EINVAL);
 
 	m2m_dev = kzalloc(sizeof *m2m_dev, GFP_KERNEL);
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 8f4b208cfee7..c2d817d3ff42 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -32,7 +32,7 @@
  *		assumed that one source and one destination buffer are all
  *		that is required for the driver to perform one full transaction.
  *		This method may not sleep.
- * @job_abort:	required. Informs the driver that it has to abort the currently
+ * @job_abort:	optional. Informs the driver that it has to abort the currently
  *		running transaction as soon as possible (i.e. as soon as it can
  *		stop the device safely; e.g. in the next interrupt handler),
  *		even if the transaction would not have been finished by then.
-- 
2.16.3

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Hans Verkuil]

On 18/06/18 06:38, Ezequiel Garcia wrote:
> As per the documentation, job_abort is not required
> to wait until the current job finishes. It is redundant
> to do so, as the core will perform the wait operation.
> 
> Remove the wait infrastructure completely.

Sylwester, can you review this?

Thanks!

	Hans

> 
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Kamil Debski <kamil@wypas.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/s5p-g2d/g2d.c | 11 -----------
>  drivers/media/platform/s5p-g2d/g2d.h |  1 -
>  2 files changed, 12 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
> index 66aa8cf1d048..e98708883413 100644
> --- a/drivers/media/platform/s5p-g2d/g2d.c
> +++ b/drivers/media/platform/s5p-g2d/g2d.c
> @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop *c
>  
>  static void job_abort(void *prv)
>  {
> -	struct g2d_ctx *ctx = prv;
> -	struct g2d_dev *dev = ctx->dev;
> -
> -	if (dev->curr == NULL) /* No job currently running */
> -		return;
> -
> -	wait_event_timeout(dev->irq_queue,
> -			   dev->curr == NULL,
> -			   msecs_to_jiffies(G2D_TIMEOUT));
>  }
>  
>  static void device_run(void *prv)
> @@ -563,7 +554,6 @@ static irqreturn_t g2d_isr(int irq, void *prv)
>  	v4l2_m2m_job_finish(dev->m2m_dev, ctx->fh.m2m_ctx);
>  
>  	dev->curr = NULL;
> -	wake_up(&dev->irq_queue);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -633,7 +623,6 @@ static int g2d_probe(struct platform_device *pdev)
>  	spin_lock_init(&dev->ctrl_lock);
>  	mutex_init(&dev->mutex);
>  	atomic_set(&dev->num_inst, 0);
> -	init_waitqueue_head(&dev->irq_queue);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> diff --git a/drivers/media/platform/s5p-g2d/g2d.h b/drivers/media/platform/s5p-g2d/g2d.h
> index dd812b557e87..9ffb458a1b93 100644
> --- a/drivers/media/platform/s5p-g2d/g2d.h
> +++ b/drivers/media/platform/s5p-g2d/g2d.h
> @@ -31,7 +31,6 @@ struct g2d_dev {
>  	struct g2d_ctx		*curr;
>  	struct g2d_variant	*variant;
>  	int irq;
> -	wait_queue_head_t	irq_queue;
>  };
>  
>  struct g2d_frame {
> 

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Sylwester Nawrocki]

Hi,

On 07/04/2018 10:04 AM, Hans Verkuil wrote:
> On 18/06/18 06:38, Ezequiel Garcia wrote:
>> As per the documentation, job_abort is not required
>> to wait until the current job finishes. It is redundant
>> to do so, as the core will perform the wait operation.

Could you elaborate how the core ensures DMA operation is not in progress
after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied?

>> Remove the wait infrastructure completely.
> 
> Sylwester, can you review this?

Thanks for forwarding Hans!

>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Kamil Debski <kamil@wypas.org>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>>   drivers/media/platform/s5p-g2d/g2d.c | 11 -----------
>>   drivers/media/platform/s5p-g2d/g2d.h |  1 -
>>   2 files changed, 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
>> index 66aa8cf1d048..e98708883413 100644
>> --- a/drivers/media/platform/s5p-g2d/g2d.c
>> +++ b/drivers/media/platform/s5p-g2d/g2d.c
>> @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop *c
>>   
>>   static void job_abort(void *prv)
>>   {
>> -	struct g2d_ctx *ctx = prv;
>> -	struct g2d_dev *dev = ctx->dev;
>> -
>> -	if (dev->curr == NULL) /* No job currently running */
>> -		return;
>> -
>> -	wait_event_timeout(dev->irq_queue,
>> -			   dev->curr == NULL,
>> -			   msecs_to_jiffies(G2D_TIMEOUT));

I think after this patch there will be a potential race condition possible,
we could have the hardware DMA and CPU writing to same buffer with sequence like:
...
QBUF
STREAMON
STREAMOFF
DQBUF 
CPU accessing the buffer  <--  at this point G2D DMA could still be writing
to an already dequeued buffer. Image processing can take few miliseconds, it should
be fairly easy to trigger such a condition.

Not saying about DMA being still in progress after device file handle is closed,
but that's something that deserves a separate patch. It seems there is a bug in 
the driver as there is no call to v4l2_m2m_ctx_release()in the device fops release() 
callback.

I think we could remove the s5p-g2d driver altogether as the functionality is covered
by the exynos DRM IPP driver (drivers/gpu/drm/exynos).

>>   }
>>   
>>   static void device_run(void *prv)
>> @@ -563,7 +554,6 @@ static irqreturn_t g2d_isr(int irq, void *prv)
>>   	v4l2_m2m_job_finish(dev->m2m_dev, ctx->fh.m2m_ctx);
>>   
>>   	dev->curr = NULL;
>> -	wake_up(&dev->irq_queue);
>>   	return IRQ_HANDLED;
>>   }

--
Regards,
Sylwester

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Ezequiel Garcia]

On Fri, 2018-07-06 at 13:09 +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 07/04/2018 10:04 AM, Hans Verkuil wrote:
> > On 18/06/18 06:38, Ezequiel Garcia wrote:
> > > As per the documentation, job_abort is not required
> > > to wait until the current job finishes. It is redundant
> > > to do so, as the core will perform the wait operation.
> 
> Could you elaborate how the core ensures DMA operation is not in
> progress
> after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied?
> 

Well, .streamoff is handled by v4l2_m2m_streamoff, which
guarantees that no job is running by calling v4l2_m2m_cancel_job.

The call chain goes like this:

vidioc_streamoff
  v4l2_m2m_ioctl_streamoff
    v4l2_m2m_streamoff
      v4l2_m2m_cancel_job
        wait_event(m2m_ctx->finished, ...);

The wait_event() wakes up by v4l2_m2m_job_finish(),
which is called by g2d_isr after marking the buffers
as done.

The reason why I haven't elaborated this in the commit log
is because it's documented in .job_abort declaration.
 
> > > Remove the wait infrastructure completely.
> > 
> > Sylwester, can you review this?
> 
> Thanks for forwarding Hans!
> 
> > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Kamil Debski <kamil@wypas.org>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >   drivers/media/platform/s5p-g2d/g2d.c | 11 -----------
> > >   drivers/media/platform/s5p-g2d/g2d.h |  1 -
> > >   2 files changed, 12 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/s5p-g2d/g2d.c
> > > b/drivers/media/platform/s5p-g2d/g2d.c
> > > index 66aa8cf1d048..e98708883413 100644
> > > --- a/drivers/media/platform/s5p-g2d/g2d.c
> > > +++ b/drivers/media/platform/s5p-g2d/g2d.c
> > > @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file,
> > > void *prv, const struct v4l2_crop *c
> > >   
> > >   static void job_abort(void *prv)
> > >   {
> > > -	struct g2d_ctx *ctx = prv;
> > > -	struct g2d_dev *dev = ctx->dev;
> > > -
> > > -	if (dev->curr == NULL) /* No job currently running */
> > > -		return;
> > > -
> > > -	wait_event_timeout(dev->irq_queue,
> > > -			   dev->curr == NULL,
> > > -			   msecs_to_jiffies(G2D_TIMEOUT));
> 
> I think after this patch there will be a potential race condition
> possible,
> we could have the hardware DMA and CPU writing to same buffer with
> sequence like:
> ...
> QBUF
> STREAMON
> STREAMOFF
> DQBUF 
> CPU accessing the buffer  <--  at this point G2D DMA could still be
> writing
> to an already dequeued buffer. Image processing can take few
> miliseconds, it should
> be fairly easy to trigger such a condition.
> 

I don't think this is the case, as I've explained above. This commit
merely removes a redundant wait, as job_abort simply waits the
interrupt handler to complete, and that is the purpose of
v4l2_m2m_job_finish.

It only makes sense to implement job_abort if you can actually stop
the current DMA. If you can only wait for it to complete, then it's not
needed.

> Not saying about DMA being still in progress after device file handle
> is closed,
> but that's something that deserves a separate patch. It seems there
> is a bug in 
> the driver as there is no call to v4l2_m2m_ctx_release()in the device
> fops release() 
> callback.
> 

Yes, that seems correct. Should be fixed in a separate patch.

> I think we could remove the s5p-g2d driver altogether as the
> functionality is covered
> by the exynos DRM IPP driver (drivers/gpu/drm/exynos).
> 

That I'll leave for you to decide :-)

The intention of this series is simply to make job_abort optional,
and remove those drivers that implement job_abort as a wait-for-
current-job.

Thanks for reviewing!
Eze

Re: [PATCH v2 1/3] rcar_jpu: Remove unrequired wait in .job_abort

[Mikhail Ulyanov]

Acked-by: Mikhail Ulyanov <mikhail.ulyanov@cogentembedded.com>

On Mon, Jun 18, 2018 at 7:38 AM, Ezequiel Garcia <ezequiel@collabora.com> wrote:
> As per the documentation, job_abort is not required
> to wait until the current job finishes. It is redundant
> to do so, as the core will perform the wait operation.
>
> Remove the wait infrastructure completely.
>
> Cc: Mikhail Ulyanov <mikhail.ulyanov@cogentembedded.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/rcar_jpu.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/media/platform/rcar_jpu.c b/drivers/media/platform/rcar_jpu.c
> index 469a326838aa..dec696e6b974 100644
> --- a/drivers/media/platform/rcar_jpu.c
> +++ b/drivers/media/platform/rcar_jpu.c
> @@ -198,7 +198,6 @@
>   * @vfd_decoder: video device node for decoder mem2mem mode
>   * @m2m_dev: v4l2 mem2mem device data
>   * @curr: pointer to current context
> - * @irq_queue: interrupt handler waitqueue
>   * @regs: JPEG IP registers mapping
>   * @irq: JPEG IP irq
>   * @clk: JPEG IP clock
> @@ -213,7 +212,6 @@ struct jpu {
>         struct video_device     vfd_decoder;
>         struct v4l2_m2m_dev     *m2m_dev;
>         struct jpu_ctx          *curr;
> -       wait_queue_head_t       irq_queue;
>
>         void __iomem            *regs;
>         unsigned int            irq;
> @@ -1494,11 +1492,6 @@ static void jpu_device_run(void *priv)
>
>  static void jpu_job_abort(void *priv)
>  {
> -       struct jpu_ctx *ctx = priv;
> -
> -       if (!wait_event_timeout(ctx->jpu->irq_queue, !ctx->jpu->curr,
> -                               msecs_to_jiffies(JPU_JOB_TIMEOUT)))
> -               jpu_cleanup(ctx, true);
>  }
>
>  static const struct v4l2_m2m_ops jpu_m2m_ops = {
> @@ -1584,9 +1577,6 @@ static irqreturn_t jpu_irq_handler(int irq, void *dev_id)
>
>         v4l2_m2m_job_finish(jpu->m2m_dev, curr_ctx->fh.m2m_ctx);
>
> -       /* ...wakeup abort routine if needed */
> -       wake_up(&jpu->irq_queue);
> -
>         return IRQ_HANDLED;
>
>  handled:
> @@ -1620,7 +1610,6 @@ static int jpu_probe(struct platform_device *pdev)
>         if (!jpu)
>                 return -ENOMEM;
>
> -       init_waitqueue_head(&jpu->irq_queue);
>         mutex_init(&jpu->mutex);
>         spin_lock_init(&jpu->lock);
>         jpu->dev = &pdev->dev;
> --
> 2.16.3
>



-- 
W.B.R, Mikhail.

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Sylwester Nawrocki]

On 07/06/2018 03:43 PM, Ezequiel Garcia wrote:

>> Could you elaborate how the core ensures DMA operation is not in
>> progress
>> after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied?
>>
> 
> Well, .streamoff is handled by v4l2_m2m_streamoff, which
> guarantees that no job is running by calling v4l2_m2m_cancel_job.
> 
> The call chain goes like this:
> 
> vidioc_streamoff
>    v4l2_m2m_ioctl_streamoff
>      v4l2_m2m_streamoff
>        v4l2_m2m_cancel_job
>          wait_event(m2m_ctx->finished, ...);
> 
> The wait_event() wakes up by v4l2_m2m_job_finish(),
> which is called by g2d_isr after marking the buffers
> as done.
> 
> The reason why I haven't elaborated this in the commit log
> is because it's documented in .job_abort declaration.

Indeed, you are right, job_abort implementation can be safely removed
in this case. As it is it doesn't help to handle cases when the HW gets
stuck and refuses to generate an interrupt. The rcar_jpu seems to be
addressing such situation properly though.

>>>> diff --git a/drivers/media/platform/s5p-g2d/g2d.c
>>>> b/drivers/media/platform/s5p-g2d/g2d.c
>>>> index 66aa8cf1d048..e98708883413 100644
>>>> --- a/drivers/media/platform/s5p-g2d/g2d.c
>>>> +++ b/drivers/media/platform/s5p-g2d/g2d.c
>>>> @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file,
>>>> void *prv, const struct v4l2_crop *c
>>>>    
>>>>    static void job_abort(void *prv)
>>>>    {
>>>> -	struct g2d_ctx *ctx = prv;
>>>> -	struct g2d_dev *dev = ctx->dev;
>>>> -
>>>> -	if (dev->curr == NULL) /* No job currently running */
>>>> -		return;
>>>> -
>>>> -	wait_event_timeout(dev->irq_queue,
>>>> -			   dev->curr == NULL,
>>>> -			   msecs_to_jiffies(G2D_TIMEOUT));
>>
>> I think after this patch there will be a potential race condition
>> possible,
>> we could have the hardware DMA and CPU writing to same buffer with
>> sequence like:
>> ...
>> QBUF
>> STREAMON
>> STREAMOFF
>> DQBUF
>> CPU accessing the buffer  <--  at this point G2D DMA could still be
>> writing
>> to an already dequeued buffer. Image processing can take few
>> miliseconds, it should
>> be fairly easy to trigger such a condition.
>>
> 
> I don't think this is the case, as I've explained above. This commit
> merely removes a redundant wait, as job_abort simply waits the
> interrupt handler to complete, and that is the purpose of
> v4l2_m2m_job_finish.
> 
> It only makes sense to implement job_abort if you can actually stop
> the current DMA. If you can only wait for it to complete, then it's not
> needed.

Agreed.

> The intention of this series is simply to make job_abort optional,
> and remove those drivers that implement job_abort as a wait-for-
> current-job.

Sure, thanks for your effort.

--
Regards,
Sylwester