| Message ID | 20180618043852.13293-3-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 { >
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
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
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
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 {
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(-)