hverkuil: ping
hverkuil: I have a few questions regarding the request API, will you be online later on today ?
pinchartl: ping
pinchartl: pong
hverkuil: pong too
(just finished dinner, hopefully you're done with lunch too)
pinchartl: you had questions for me?
yes
regarding the request API
I have a few questions
to start with, you've tied the request API implementation with the control framework
drivers are only notified of V4L2_REQ_CMD_QUEUE
the rest is handled internally
wouldn't it be better to add more v4l2_device operations
and provide helper functions to handle them through the control framework ?
There should not be any need for that. CMD_QUEUE is the only one that needs special driver attention since it may have to sync things up internally.
first of all, drivers that don't use the control framework (for whatever reason) can't use the request API now
I'm thinking about uvcvideo for instance
although it might not be useful there
forcing a midlayer upon drivers doesn't seem a too good idea
it's usually better to let drivers control how to implement the operation
and of course we should encourage them to use the control framework
I see no need for the request API in uvc. I really wish uvc would switch over, but I realize that that's a heck of a lot of work.
I don't trust driver developers :-)
it would be a heck of work in both uvcvideo and the control framework :-)
that's why we have code review ;-)
larsc: JIC, have you done (or going to do) anything with adv8170?
I'd like to allocate a driver private request object for each request as I have information to store there
yeah, but we only review code that is submitted to the mailinglist. Often drivers start as out-of-tree and by forcing them to use a standard framework it should make upstreaming easier (once someone does that work).
and currently I can't be notified that a request has been created
are you talking uvc?
or did you switch to a new topic?
I'm talking about the vsp1 driver
ah.
(I don't plan to implement the request API in the uvcvideo driver)
so you want a callback when V4L2_REQ_CMD_END is called? (and probably also at a similar point when VIDIOC_S_EXT_CTRLS is called for a request)
I was more thinking about V4L2_REQ_CMD_BEGIN
speaking of that
I'm wondering whether it would make sense to separate the request creation from V4L2_REQ_CMD_BEGIN
what sort of info are we talking about?
and make the kernel assign request IDs
No, there is a reason why userspace specifies request IDs:
each request has a hardware execution context I need to allocate
I could do that in the V4L2_REQ_CMD_QUEUE operation callback
if you have multiple device nodes that need to be setup for a specific request, then you want to use the same request ID for all those devices.
but it would be much more convenient to have the context when formats are set already
sure
when you request the hardware to apply a request, then that request ID applies to all devices involved.
that's why I talked about decoupling the request ID allocation from V4L2_REQ_CMD_BEGIN
you would have a V4L2_REQ_CMD_ALLOC that allocates the request ID
and V4L2_REQ_CMD_BEGIN would then use it normally
operations would be more symetric then
V4L2_REQ_CMD_ALLOC/V4L2_REQ_CMD_DELETE
and V4L2_REQ_CMD_BEGIN/V4L2_REQ_CMD_END
alloc/delete could be called on any device node
while begin/end would apply to a single device node
That CMD_DELETE is global (i.e. deletes the request from all device nodes) makes a lot of sense
yes it does
requests should also be refcounted
otherwise you'll leak memory if applications don't clean up
I think that should also be done for APPLY (i.e. global)
making APPLY global seems to make sense too
I am not sure about refcounting: do we want requests to be deleted when the last filehandle is closed? Controls are persistent, and I think it is not unreasonable to do the same for requests.
I typically tested the request API by calling v4l2-ctl to setup a request, then apply it later.
That wouldn't work if it is refcounted.
if we don't refcount requests how do we protect about memory leaks ?
the driver sets the max number of requests for each control (max_reqs field in struct v4l2_ctrl).
it shouldn't be per control
it should be global
so the requests are not deleted (unless CMD_DELETE is called or the driver is unloaded).
no, for some controls it makes no sense being part of a request.
we have pretty different views on the concept then :-)
I think requests are objects that should be created on demand
filled with all kind of information (controls, formats, links, ...)
In fact, depending on the hardware and driver, only a subset of the controls can be used as part of a request.
and queued
Sure, but it makes no sense to add V4L2_CID_POWER_LINE_FREQUENCY to a request.
why not ?
why would you forbid that ?
headless: no
larsc: OK
why would you? The point is that CMD_QUEUE expects precise timing (i.e. the request takes effect for a specific frame). If the driver cannot do that, then it may choose to forbid a control to be part of a request.
I'd like to leave that up to the driver.
exactly, I'd leave it up to the driver
I wouldn't forbid drivers from doing anything in the first place
I'm not.
The driver sets the max_reqs field.
they should allow storing whatever they want in a request
having different values of max_reqs per control doesn't make sense
I disagree.
I expect its either 0 (not supported in requests) or some multiple of VIDEO_MAX_FRAME.
0 is a special case
but different != 0 values per control doesn't seem to make sense
max_reqs is a purely software limit
It can easily be different: for a 'interface-centric' v4l2 device some of the controls are typically inherited from different subdev drivers, and there is no guarantee that those all support the same number of requests for controls. I am not sure how this will work out in practice, so for the time being I prefer to keep it as generic as possible.
The request API can for example also be used to program shadow registers, in which case max_reqs would be the max number of shadow configurations the hardware allows.
why should it ?
you can queue requests in software too
and you can certainly allocate requests way before you queue them
so there's no reason to always hardcode a limit equal to the hardware limit (if any)
it's because I want to keep things as open as possible that I don't want to set this kind of limits :-)
I'd like, in the first version, to have a userspace API that can be implemented by drivers
(with more callbacks than in your version)
and with helper functions for the control framework
this is really new territory
we need to explore
so I'd like to keep it open for drivers to experiment
we'll then find out which patterns emerge
designing a midlayer based on one use case and forcing it upon drivers won't be a great success :-)
would you be fine with such an approach ?
I don't really understand what you want to do. It is tied closely to the control framework, you really can't use it without it (unless you basically duplicate what the framework does).
my main use case doesn't involve controls
it involves formats and links instead
other drivers will be more control-focussed
If you need an additional callback, then that is of course possible, but separating it from the control framework is a bad idea.
some drivers will only need a req_queue callback
others will want to be notified of more request commands
I'd like to have a self-contained request API
and impleemnt support for it in the control framework
this will need several iterations
and implementations in several drivers
before it stabilizes on the kernel side
we should obviously try to make the userspace API stable as soon as possible
for instance
if I can allocate request objects separately from the control framework
I would easily use them to store link configuration
the media controller code should not depend on the control framework
I'm not sure exactly how this will be implemented
but I believe it makes sense to
1. allocate a request
(CMD_ALLOC)
2. fill it with various controls, formats, links, ...
(CMD_BEING, s_ctrl/s_format/..., CMD_END)
3. queue the request
(CMD_QUEUE)
1. would be performed once
CMD_BEGIN/CMD_END would be called on all devnodes that need it
CMD_QUEUE would be called once
oh, 2. also includes PREPARE_BUF
I'd use refcounting to avoid memory leaks
I have to admit I haven't (yet) had time to spend on the matter of request API, but I do share pinchartl's concern of leaking memory.
I am not opposed to CMD_ALLOC, although I would let userspace set the request ID instead of letting the kernel generate an ID. When I played with requests I quickly realized that it made things easier if userspace sets the ID.
Often you want to keep the request ID the same as e.g. the buffer index, or some other index.
CMD_ALLOC can return EBUSY if the requested ID is already in use.
hverkuil: Btw. you can also open your file handle in a shell, and pass that consequently to whatever test program you want to use it, and once you no longer need it, just close it.
the problem with allocating IDs in userspace is that it makes it very hard to control different parts of the devices with different processes (or even threads)
Could we allow both user and kernel allocated IDs?
Say, if the user specifies ID zero, let kernel allocate one?
I am not convinced about memory leaking. It's not really about leaking it is about whether the request data is persistent or not. Controls are persistent, and I think requests should be as well.
sailus_: that would work for me.
hverkuil: Controls are always there since they are an interface to control the hardware.
sailus_: might be the best solution, actually.
Requests are hardly such.
The requests should include video buffers as well, AFAIR the original set only dealed with controls.
You can have however many requests you like, and then just leave them lying around.
buffers can also be part of a request.
Oh, good, good!
Say, if a program crashes, who will clean up its mess?
buffers are always cleaned up, requests stick around. Actually, it is a bit more complex:
why should a request stick around ?
shouldn't requests be ephemeral ?
you allocate one, fill it with data, queue it
By default the request data is discarded by default once it is used. If the V4L2_REQ_CMD_BEGIN_FL_KEEP is set when calling CMD_BEGIN, then the request data is kept to be reused.
I wouldn't discard the request data, I would discard the request
Typical use case is when apps set up 'preset' configurations and want to switch from one to another.
Since these are presets, you don't want to have to set them up every time you need them as the presets themselves don't change.
hverkuil: I'd include the Android camera API v3 implementations to one of the typical use cases.
it was designed with that in mind.
At least that's the way we want it, don't we? :-)
I can see potential use cases for keeping request objects around
That's quite unlike a set of "preset" configurations.
sailus_: it was one of the use cases, the 'preset' use-case is a different one from Android's use-case.
hverkuil: Ack.
I am a bit concerned about continually allocating/freeing requests from a performance perspective.
hverkuil: I'll have to check the patches, where can I find them?
a thid use case is to avoid the costly streamoff-configure-reqbufs-streamon cycle every time you want to change a pipeline configuration
Sometimes it might be useful to be able to keep the request around, but most of the time I'd suppose it'll be an entirely new request every time.
hverkuil: in the android use case there's no way around it, each frame comes with a new request
http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=requests
hverkuil: Thanks!
The way it works now is that the request memory stays around until CMD_DELETE is called. Once the data of a request is used it is just marked as 'used' or something like that, but when new values are set for that request there is no need to allocate the memory again. It just clears the used bit.
(ah, the bit is called 'applied')
I like that behavior.
So: delete and apply should be global for all device nodes.
I wish we could have settled on the MCv2 front before starting discussing the request API, I don't like having to fight multiple battles at the same time :-)
cmd_alloc is fine by me, as long as userspace can set the ID (and 0 to let the kernel generate one). This can also call a callback.
regarding refcounting: I'm not sure about that.
we need a way to free memory if applications don't clean up, don't we ?
If we decide to do that, then the userspace API should have a way to override that in the future. Should be easy enough: a flags field in CMD_ALLOC can be used for that should we need it.
no, the whole point is that applications must not have control over it
you can't allow an application to allocate memory in the kernel and keep it forever after the process goes away
on a side note
in the request queue callback
I need the list of video nodes that are part of the request
I was thinking of getting that from the buffers prepared for the request ID
I need to think more about this. It was really handy when testing with v4l2-ctl. I'd like to have some way of keeping that behavior. Feel free to add refcounting though.
v4l2_device now has the list of video_device structs.
it has a list of all video_device
but I need a list of the ones that arer part of the request
(sorry for the typos, my internet connection is really bad, I type without any visual feedback)
the idea was to walk the list of vdevs and for each vdev see if it has data for the given request.
But it is probably easier to keep track of it in the driver (especially with a request struct).
hmmm... I could do that if I can add a helper function in vb2 to test whether a buffer has been prepared for a given request ID
would that be fine with you ?
If we make global request structs, then it is a lot easier to let the core administrate which vdevs are part of the request.
there's no concept of a video node being part of a request with the current userspace API
all we have is buffer prepared for request IDs
s/buffer/buffers/
there doesn't have to be a buffer involved, depending on the use case.
can you have a video node involved without a buffer ?
Sure. You can make a request, set values (controls), then apply it at any time. CMD_QUEUE however expects that there are buffers involved.
for apply, sure
you need to have either controls or buffers
(or both)
in my case there's no control
so I need to rely on buffers
CMD_QUEUE without buffers doesn't make sense.
I agree
CMD_APPLY doesn't touch buffers, right ?
right
ok, I'll see how I can implement a helper in vb2-v4l2 to get the video device nodes (or rather the queues, and from there the video device nodes) from a request ID
maybe iterating over the video_device using the list from v4l2_device
and for each of them getting the buffer prepared with the request ID
if no buffer is found then the devnode isn't involved in the request
You would also have to check if the vdev has controls set for the request ID. You don't have controls, but other drivers will have that.
sure
but vb2 won't be involved there
I'll add a helper function in vb2 for buffers
oh, sorry, you're right.
that's all I need to start with
and we can then add a helper function in the control framework
sounds good ?
to start with, yes. Although I wonder if it wouldn't be easier to have a global request struct (allocated by CMD_ALLOC) that just keeps track of the vdevs involved in the request.
That is more efficient.
but that can always be done later.
I was thinking about that too
would you store that in a struct video_device *[] then ?
we can't really use a linked list easily as video devices could be part of more than one request
Have you considered using the media device for the request API?
as long as no new video device is added after the request has been alllocatee it should work
worst case we could realloc the array
or just return an error
sailus_: I have
but the idea isn't universally appreciated :-)
pinchartl: I agree.
How universally? :-)
Well, it could always be extended to the media device, it'd be more difficult to make it V4L2 specific again.
In the meantime we could "experiment" on V4L2 a bit. :-)
sailus_: the problem is that each subsystem has its own way of dealing with controls/properties/etc. Moving that into the MC would put subsystem-specific code in the MC framework. I really don't think that is a good idea at all.
sailus_: I like the concept of being able to setup the whole pipeline, including links, pad formats and controls, using a single ioctl call on the media device node
hverkuil: I think we could have subsystem-specific encodings for the parameters, and then having drivers delegate the call to subsystems as appropriate
we're far from that though
pinchartl: That'd be cleaner as well. This way you don't have the ambiguity of accessing the same request over multiple nodes.
It could be an idea though to let the MC allocate a request (i.e. create an abstract request cookie) that other subsystems can use.
I think application should be included as well.
There are cases where multiple subsystems would be part of the same request.
I don't have that kind of hardware but I think someone else did.
I don't have such a use case either but we'll eventually get there
pinchartl: Yes. :-)
all you really need is a request ID that can be used by different subsystems. The MC can do that.
ok, I'll start with the vb2 helper
So CMD_ALLOC/DELETE/APPLY/QUEUE would all be implemented by the MC, CMD_BEGIN/END would be v4l2 specific.
thank you both for your comments
speaking of CMD_ALLOC/DELETE/APPLY/QUEUE
should I go ahead and implement them on the media devnode already ?
or keep them v4l2-specific for now ?
I actually think that this is a very nice idea to move that to the MC.
I never liked the fact that CMD_QUEUE could be called from any video node.
ok, I'll do that
By letting the MC allocate (own) the request this makes a lot more sense.
regarding formats
I'd like not to store them in the control handlers to start with
it also makes it much easier to add a request ID to the topology struct in order to atomically change links for a request.
my first goal is to implement the userspace API
and validate it
would you be fine with that ?
I'd open-code it in the driver first
the g/s pad fmt operations would look at the current request ID in the vfh
we could then check how to implement that in the control framework
but I'd like to do so in two steps to have a testable solution sooner than later
sure, but I suspect you'll have to use the control framework eventually, and I may not ack it if it doesn't use it.
I understand that
I suspect it will depend on how clean or dirty the implementation is
I might not ack it myself either if it's too dirty :-)
but it might turn out to be lean and clean, I don't know yet
You don't need to use controls in your requests, so it may not be too bad for your driver, but for drivers that use controls in requests as well it may become messy.
that's also part of the reason I want to implement it that way, to be able to compare both implementations
s/need/have/
let's discuss it based on the code then
the fact thay you said "I may not ack it" and not "I will not ack it" is enough for me for now :-)
I agree, let's decide based on the code.
I only made a simple implementation for the selection API once, so I can't say that I know what's best here.
pinchartl: The plan sounds good to me.
The main reason for using the control framework for this is that that makes it easy to implement in the v4l2 core.
I.e. if there is a 'subdev_format" control, then the core can automatically set that control when subdev_s_fmt is called. No need for drivers to do anything.
other than creating that control.
I expect that drivers will still want to validate the format
so they should receive the call
try_ctrl op
and possibly delegate it to a helper function
automatically called when a control is set for a request.
yes, but then you'd end up duplicating the format validation code
as g/s_fmt would still need to validate formats
when no request is used
I'd rather forward the call to the drivers and let them decide whether to use helpers directly or perform some processing
If the control is there, then the non-request subdev_s_fmt ioctl can also do a set control (with request ID 0), in which case everything will end up in the same control callbacks. But this is typically something that you need to try and see how well it works.
this was always the approach I had in mind.
if we go that way
I expect some redesign of the control framework will be needed
to allow accessing controls in non-sleepable contexts
That's an unrelated issue.
yes and no
it's unrelated in a way
but formats are quite commonly accessed in such contexts
so there would be a pressing use case
I know you're not looking forward to that :-)
there's also the issue of complexity that we need to take into account
I expect to have real performance numbers in a couple of weeks
But I agree, getting and setting controls in interrupt context will require work.
I've been postponing this, hoping it wouldn't be needed.
I can't blame you :-)
I think it can be done, but likely with some restrictions.
do you also have a feeling that the control framework code complexity is growing out of control ?
(pun intended)
I.e. setting a control over an i2c bus is a no go.
Actually, the core of the framework has been mostly unchanged since the beginning. Support for arrays/compound controls was the only major change.
And I much prefer to have the complexity in a single framework, rather than in each and every driver :-)