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@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.
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
