Re: [RFC PATCH 0/8] [media] Request API, take three

[Hans Verkuil]

On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>
>>> Again, this is a simple version that only implements the flow of requests,
>>> without applying controls. The intent is to get an agreement on a base to work
>>> on, since the previous versions went straight back to the redesign board.
>>>
>>> Highlights of this version:
>>>
>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>> since the driver would not know the state of the request, and thus cannot do
>>> anything with the buffer. Drivers are now responsible for applying request
>>> parameters themselves.
>>>
>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>> flow of buffers decides the order in which requests are processed. Individual
>>> devices of the same pipeline can implement synchronization if needed, but this
>>> is beyond this first version.
>>>
>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>> series, so I am waiting for the latter to land to do it properly.
>>>
>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>> they own, resulting in the buffer queue being blocked until the request is
>>> submitted. An alternate design would be to only block the
>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>> buffer order is becoming increasingly important I have decided to just block the
>>> queue. This is open to discussion though.
>>
>> I don't think we should mess with the order.
> 
> Agreed, let's keep it that way then.
> 
>>
>>>
>>> * Documentation! Also described usecases for codec and simple (i.e. not part of
>>> a complex pipeline) capture device.
>>
>> I'll concentrate on reviewing that.
>>
>>>
>>> Still remaining to do:
>>>
>>> * As pointed out by Hans on the previous revision, do not assume that drivers
>>> using v4l2_fh have a per-handle state. I have not yet found a good way to
>>> differenciate both usages.
>>
>> I suspect we need to add a flag or something for this.
> 
> I hope we don't need to, let's see if I can find a pattern...
> 
>>
>>> * Integrate Hans' patchset for control handling: as said above, this is futile
>>> unless we can agree on the basic design, which I hope we can do this time.
>>> Chrome OS needs this really badly now and will have to move forward no matter
>>> what, so I hope this will be considered good enough for a common base of work.
>>
>> I am not sure there is any reason to not move forward with the control handling.
>> You need this anyway IMHO, regardless of any public API considerations.
> 
> Only reasons are my lazyness and because I wanted to focus on the
> request flow first. But you're right. I have a version with your
> control framework changes integrated (they worked on the first attempt
> btw! :)), let me create a clean patchset from this and send another
> RFC.

Scary that it worked on the first attempt :-)

> 
>>
>>> A few thoughts/questions that emerged when writing this patchset:
>>>
>>> * Since requests are exposed as file descriptors, we could easily move the
>>> MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the
>>> requests themselves, instead of having to perform them on the media device that
>>> provided the request in the first place. That would add a bit more flexibility
>>> if/when passing requests around and means the media device only needs to handle
>>> MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me.
>>> Any comment for/against that?
>>
>> Makes sense IMHO.
> 
> Glad to hear it, that was my preferred design. :)
> 
>>
>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>> the request FD that produced them (vim2m acts that way). This seems to work
>>> well, however FDs are process-local and could be closed before the CAPTURE
>>> buffer is dequeued, rendering that information less meaningful, if not
>>> dangerous.
>>
>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>> increased so the data it represents won't be released if userspace closes the fd.
> 
> The refcount of the request is increased. The refcount of the FD is
> not, since it is only a userspace reference to the request.

I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
through the fd. That's probably the model you want to follow.

> 
>>
>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>> it shouldn't be 'dangerous' AFAICT.
> 
> It userspace later gets that closed FD back from a DQBUF call, and
> decides to use it again, then we would have a problem. I agree that it
> is userspace responsibility to be careful here, but making things
> foolproof never hurts.

I think all the issues will go away if you refcount the fd instead of the
request. It worked well for dma-buf for years.

> 
>>
>>> Wouldn't it be better/safer to use a global request ID for
>>> such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so
>>> user-space knows which request ID a FD refers to.
>>
>> I think it is not a good idea to have both an fd and an ID to refer to the
>> same object. That's going to be very confusing I think.
> 
> IDs would not refer to the object, they would just be a way to
> identify it. FDs would be the actual reference.
> 
> If we drop the idea of returning request FDs to userspace after the
> initial allocation (which is the only time we can be sure that a
> returned FD is valid), then this is not a problem anymore, but I think
> it may be useful to mark CAPTURE buffers with the request that
> generated them.
> 
>>
>>> * Using the media node to provide requests makes absolute sense for complex
>>> camera devices, but is tedious for codec devices which work on one node and
>>> require to protect request/media related code with #ifdef
>>> CONFIG_MEDIA_CONTROLLER.
>>
>> Why? They would now depend on MEDIA_CONTROLLER (i.e. they won't appear in the
>> menuconfig unless MEDIA_CONTROLLER is set). No need for an #ifdef.
> 
> Ah, if we make them depend on MEDIA_CONTROLLER, then indeed. But do we
> want to do this for e.g. vim2m and vivid?

If they support requests, then yes.

> 
>>
>>  For these devices, the sole role of the media node is
>>> to produce the request, and that's it (since request submission could be moved
>>> to the request FD as explained above). That's a modest use that hardly justifies
>>> bringing in the whole media framework IMHO. With a bit of extra abstraction, it
>>> should be possible to decouple the base requests from the media controller
>>> altogether, and propose two kinds of requests implementations: one simpler
>>> implementation that works directly with a single V4L2 node (useful for codecs),
>>> and another one that works on a media node and can control all its entities
>>> (good for camera). This would allow codecs to use the request API without
>>> forcing the media controller, and would considerably simplify the use-case. Any
>>> objection to that? IIRC the earlier design documents mentioned this possibility.
>>
>> I think this is an interesting idea, but I would postpone making a decision on
>> this until later. We need this MC support regardless, so let's start off with that.
>>
>> Once that's working we can discuss if we should or should not create a shortcut
>> for codecs. Trying to decide this now would only confuse the process.
> 
> Sounds good, as long as we make a decision before merging.

Of course.

Regards,

	Hans