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@xxxxxxxxxxx>
> > > Cc: Kamil Debski <kamil@xxxxxxxxx>
> > > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > > ---
> > > 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
