Re: [RFC 00/10] V4L2 explicit synchronization support

[Shuah Khan]

Hi Gustavo,

On Mon, Apr 3, 2017 at 1:46 PM, Javier Martinez Canillas
<javier@xxxxxxxxxxxxxxx> wrote:
> Hello Mauro and Gustavo,
>
> On 04/03/2017 07:16 AM, Mauro Carvalho Chehab wrote:
>> Hi Gustavo,
>>
>> Em Mon, 13 Mar 2017 16:20:25 -0300
>> Gustavo Padovan <gustavo@xxxxxxxxxxx> escreveu:
>>
>>> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
>>>
>>> Hi,
>>>
>>> This RFC adds support for Explicit Synchronization of shared buffers in V4L2.
>>> It uses the Sync File Framework[1] as vector to communicate the fences
>>> between kernel and userspace.
>>
>> Thanks for your work!
>>
>> I looked on your patchset, and I didn't notice anything really weird
>> there. So, instead on reviewing patch per patch, I prefer to discuss
>> about the requirements and API, as depending on it, the code base will
>> change a lot.
>>
>
> Agree that's better to first set on an uAPI and then implement based on that.

Yes. Agreeing on uAPI first would work well.

>
>> I'd like to do some tests with it on devices with mem2mem drivers.
>> My plan is to use an Exynos board for such thing, but I guess that
>> the DRM driver for it currently doesn't. I'm seeing internally if someone
>> could be sure that Exynos driver upstream will become ready for such
>> tests.
>>
>
> Not sure if you should try to do testing before agreeing on an uAPI and
> implementation.

Running some tests might give you a better feel for m2m - export buf - drm
cases without fences on exynos. This could help understand the performance
gains with fences.

>
>> Javier wrote some patches last year meant to implement implicit
>> fences support. What we noticed is that, while his mechanism worked
>> fine for pure capture and pure output devices, when we added a mem2mem
>> device, on a DMABUF+fences pipeline, e. g.:
>>
>>       sensor -> [m2m] -> DRM
>>
>> End everything using fences/DMABUF, the fences mechanism caused dead
>> locks on existing userspace apps.
>>
>> A m2m device has both capture and output devnodes. Both should be
>> queued/dequeued. The capture queue is synchronized internally at the
>> driver with the output buffer[1].
>>
>> [1] The names here are counter-intuitive: "capture" is a devnode
>> where userspace receives a video stream; "output" is a devnode where
>> userspace feeds a video stream.
>>
>> The problem is that adding implicit fences changed the behavior of
>> the ioctls, causing gstreamer to wait forever for buffers to be ready.
>>
>
> The problem was related to trying to make user-space unaware of the implicit
> fences support, and so it tried to QBUF a buffer that had already a pending
> fence. A workaround was to block the second QBUF ioctl if the buffer had a
> pending fence, but this caused the mentioned deadlock since GStreamer wasn't
> expecting the QBUF ioctl to block.
>
>> I suspect that, even with explicit fences, the behavior of Q/DQ
>> will be incompatible with the current behavior (or will require some
>> dirty hacks to make it identical).

One big difference between implicit and explicit is that use-space is aware
of fences in the case of explicit. Can that knowledge be helpful in avoiding
the the problems we have seen with explicit?

>>
>> So, IMHO, the best would be to use a new set of ioctls, when fences are
>> used (like VIDIOC_QFENCE/DQFENCE).
>>
>
> For explicit you can check if there's an input-fence so is different than
> implicit, but still I agree that it would be better to have specific ioctls.
>

It would be nice if we can avoid adding more ioctls. As I mentioned earlier,
user-space is aware that fences are in use. Can we key off of that and make
it a queue property to be used to change the user-space action for fence vs.
no fence case?

<snip.>

thanks,
-- Shuah