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
