Hi,
I've got one limitation for v4l2-mem2mem devices: a v4l2-mem2mem device driver which can't support *both* V4L2_BUF_TYPE_VIDEO_CAPTURE and V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE capture buffers.
AFAIK/U, this limitation is unjustified.
So, with v4l2-mem2mem :
1. It's not possible to alternatively queue these capture buffers in the same v4l2-mem2mem instance (fd). 2. It's possible to have one v4l2-mem2mem instance for V4L2_BUF_TYPE_VIDEO_CAPTURE and another one (separate fd) for V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; but .. 3. In order to get 2., the driver has to call v4l2_m2m_ctx_init() in v4l2_ioctl_ops::vidioc_reqbufs(), not in v4l2_file_operations::open() - which is kind of 'OKay' 4. v4l2_m2m_ctx_init() take a callback function as an argument, which will be called to setup *both* the output and capture vb2 queue. 5. Still in order to get 2., the driver has to call vidioc_reqbufs() on the capture buffers first, then on the output buffers. If not, v4l2_m2m_ctx_init() will attempt to setup both output and capture queues, but still w/ incomplete information on the type of the capture queue, the userspace really wants.
A solution would be to decouple the output and capture queues initialization in v4l2_m2m_ctx_init().
-Ilyes
On Mon, Aug 13, 2012 at 2:13 PM, Hans de Goede hdegoede@redhat.com wrote:
Hi,
<snip>
Easy:
- Split off the control part from videodev2.h. Controls are almost 30% of videodev2.h. I think maintaining controls would be easier if they are
moved to e.g. linux/v4l2-controls.h which is included by videodev2.h.
Ack.
- Currently there are three types of controls: standard controls,
controls that are specific to a chipset (e.g. cx2341x, mfc51) and driver-specific controls. The controls of the first two types have well defined and unique IDs. For driver-specific controls however there are no clear rules.
It all depends on one question: should driver-specific controls have a unique control ID as well, or can they overlap with other drivers? If the answer is that they should be unique as well, then alldriver-specific controls will have to be defined in a single header so you can be certain that there is no overlap.
If the answer is that they may overlap, then each driver can eitherdefine their controls inside their own driver, or in a driver-specific public header.
In both cases a control ID range has to be defined for such controls,to ensure that they never clash with standard or chipset-specific control IDs. E.g. >= V4L2_CTRL_CLASS_XXX + 0x8000 (no overlap) or + 0xf000 (overlap allowed).
My preference is to allow overlap.+1 for allowing overlap, and only when it is expected that some special app will actually use the device specific controls (versus the user changing them in a generic v4l2 control panel app), then the private controls should be defined in a public header. If no such special app is expected, the private controls should be defined inside a private header of the driver.
- What should VIDIOC_STREAMON/OFF do if the stream is already
started/stopped? I believe they should do nothing and just return 0. The main reason for that is it can be useful if an application can just call VIDIOC_STREAMOFF without having to check whether streaming is in progress.
+1 for just returning 0
- What should a driver return in TRY_FMT/S_FMT if the requested format is
not supported (possible behaviours include returning the currently selected format or a default format).
The spec says this: "Drivers should not return an error code unlessthe input is ambiguous", but it does not explain what constitutes an ambiguous input. Frankly, I can't think of any and in my opinion TRY/S_FMT should never return an error other than EINVAL (if the buffer type is unsupported) or EBUSY (for S_FMT if streaming is in progress).
Returning an error for any other reason doesn't help the applicationsince the app will have no way of knowing what to do next.
Ack on not returning an error for requesting an unavailable format. As for what the driver should do (default versus current format) I've no preference, I vote for letting this be decided by the driver implementation.
- VIDIOC_QUERYCAP allows bus_info to be empty. Since the purpose of
bus_info is to distinguish multiple identical devices this makes no sense. I propose to make the spec more strict and require that bus_info is always filled in with a unique string.
Ack.
- Deprecate V4L2_BUF_TYPE_PRIVATE. None of the kernel drivers use it, and
I cannot see any good use-case for this. If some new type of buffer is needed, then that should be added instead of allowing someone to abuse this buffer type.
Ack.
- A driver that has both a video node and a vbi node (for example) that
uses the same struct v4l2_ioctl_ops for both nodes will have to check in e.g. vidioc_g_fmt_vid_cap or vidioc_g_fmt_vbi_cap whether it is called from the correct node (video or vbi) and return an error if it isn't.
That's an annoying test that can be done in the V4L2 core as well.Especially since few drivers actually test for that.
Should such checks be added to the V4L2 core? And if so, should we addsome additional VFL types? Currently we have GRABBER (video nodes), VBI, RADIO and SUBDEV. But if we want to do proper core checks, then we would also need OUTPUT, VBI_OUT and M2M.
I'm in favor of adding checks to the core.
Remove the experimental tag from the following old drivers:
VIDEO_TLV320AIC23B USB_STKWEBCAM VIDEO_CX18 VIDEO_CX18_ALSA VIDEO_ZORAN_AVS6EYES DVB_USB_AF9005 MEDIA_TUNER_TEA5761ACK.
Removing this tag from these drivers might be too soon, though: VIDEO_NOON010PC30 VIDEO_OMAP3I've no opinion on these.
- What should VIDIOC_G_STD/DV_PRESET/DV_TIMINGS return if the current
input or output does not support that particular timing approach? EINVAL? ENODATA? This is relevant for the case where a driver has multiple inputs/outputs where some are SDTV (and support the STD API) and others are HDTV (and support the DV_TIMINGS API).
I propose ENODATA.+1 for ENODATA, EINVAL makes no sense if all the input parameters are correct.
- Proposal: add these defines:
#define V4L2_IN_CAP_TIMINGS V4L2_IN_CAP_CUSTOM_TIMINGS #define V4L2_OUT_CAP_TIMINGS V4L2_OUT_CAP_CUSTOM_TIMINGS
Since DV_TIMINGS is now used for any HDTV timings and no longer just for custom, non-standard timings, the word "CUSTOM" is no longer appropriate.
No opinion.
What should video output drivers do with the sequence and timestamp fields when they return a v4l2_buffer from VIDIOC_DQBUF?
I think the spec is clear with respect to the timestamp:
"The driver stores the time at which the first data byte was actually sent out in the timestamp field."
For sequence the spec just says:
"Set by the driver, counting the frames in the sequence."
So I think that output drivers should indeed set both sequence and timestemp.
Ack.
- Make the argument of write-only ioctls const in v4l2-ioctls.h. This
makes it obvious to drivers that they shouldn't change the contents of the input struct since it won't make it back to userspace. It also simplifies v4l2-ioctl.c since it can rely on the fact that after the ioctl call the contents of the struct hasn't changed. Right now the struct contents is logged (if debugging is on) before the ioctl call for write-only ioctls.
Ack (although this will break compilation of some drivers, but that can be fixed).
Hard(er):
- What is the right/best way to set the timestamp? The spec says
gettimeofday, but is it my understanding that ktime_get_ts is much more efficient.
Some drivers are already using ktime_get_ts. Options: a) all drivers must comply to the spec and use gettimeofday b) we change the spec and all drivers must use the more efficientktime_get_ts c) we add a buffer flag V4L2_BUF_FLAG_MONOTONIC to tell userspace that a monotonic clock like ktime_get_ts is used and all drivers that use ktime_get_ts should set that flag.
If we go for c, then we should add a recommendation to use one or theother as the preferred timestamp for new drivers.
Wouldn't b/c break the API?
- If a driver supports only formats with more than one plane, should V4L2_CAP_VIDEO_CAPTURE still be defined?
No
And if a driver also supports single-plane formats in addition to >1 plane formats, should V4L2_CAP_VIDEO_CAPTURE be compulsary?Yes, so that non multi-plane aware apps keep working.
- VIDIOC_CROPCAP: the spec says that CROPCAP must be implemented by all capture and output devices (Section "Image Cropping, Inserting and
Scaling"). In reality only a subset of the drivers support cropcap.
Should cropcap really be compulsory? Or only for drivers that canscale? And in that case, should we make a default implementation for those drivers that do not support it? (E.g.: call g_fmt and use the width/height as the default and bounds rectangles, and set the pixel aspect to 1/1)
I vote for making it non compulsory, and simply returning -ENOTTY for drivers which don't support it.
- Pixel aspect: currently this is only available through VIDIOC_CROPCAP.
It never really belonged to VIDIOC_CROPCAP IMHO. It's just not a property of cropping/composing. It really belongs to the input/output timings (STD or DV_TIMINGS). That's where the pixel aspect ratio is determined.
While it is possible to add it to the dv_timings struct, I see no wayof cleanly adding it to struct v4l2_standard (mostly because VIDIOC_ENUMSTD is now handled inside the V4L2 core and doesn't call the drivers anymore).
An alternative is to add it to struct v4l2_input/output, but I don'tknow if it is possible to defined a pixelaspect for inputs that are not the current input.
What I am thinking of is just to add a new ioctl for thisVIDIOC_G_PIXELASPECT. The argument is then:
struct v4l2_pixelaspect { __u32 type; struct v4l2_fract pixelaspect; __u32 reserved[5]; }; This combines well with the selection API.We will want to be able to enumerate this too, so I vote for extending v4l2_frmsize_discrete with a struct v4l2_fract pixelaspect, and likewise for v4l2_frmsize_stepwise.
Likewise we also want to get the pixelaspect on a TRY_FMT, which is a bit tricky, since we cannot extend v4l2_pix_format (*) instead we could add a v4l2_pix_format_w_aspect, and add that to the v4l2_format union. Then apps who want to pixelratio can look inside v4l2_pix_format_w_aspect instead, with the note that the aspect may be reported as 0/0 by drivers which don't support reporting it. This avoids adding a new ioctl, and gives us a way to get the pixelratio without actually having to set the fmt.
(*) no reserved space inside it, and if we would allow it to grow, we still would have an issue because that would also grow v4l2_framebuffer, which we certainly cannot do.
- How to handle tuner ownership if both a video and radio node share the
same tuner?
Obvious rules: - Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will changeowner or return EBUSY if streaming is in progress.
That won't work, as there is no such thing as streaming from a radio node, I suggest we go with the simple approach we discussed at our last meeting in your Dutch House: Calling S_FREQ, S_TUNER, S_MODULATOR or S_HW_FREQ_SEEK will make an app the tuner-owner, and *closing* the device handle makes an app release its tuner ownership. If an other app already is the tuner owner -EBUSY is returned.
- Ditto for STREAMON, read/write and polling for read/write.No, streaming and tuning are 2 different things, if an app does both, it will likely tune before streaming, but in some cases a user may use a streaming only app together with say v4l2-ctl to do the actual tuning. I think keeping things simple here is key. Lets just treat the "tuner" and "stream" as 2 separate entities with a separate ownership.
- Ditto for ioctls that expect a valid tuner configuration likeQUERYSTD.
QUERY is a read only ioctl, so it should not be influenced by any ownership, nor imply ownership.
- Just opening a device node should *not* switch ownership.Ack!
But it is not clear what to do when any of these ioctls are called: - G_FREQUENCY: could just return the last set frequency for radio orTV: requires that that is remembered when switching ownership. This is what happens today, so G_FREQUENCY does not have to switch ownership.
Ack.
- G_TUNER: the rxsubchans, signal and afc fields all require ownershipof the tuner. So in principle you would want to switch ownership when G_TUNER is called. On the other hand, that would mean that calling v4l2-ctl --all -d /dev/radio0 would change tuner ownership to radio for /dev/video0. That's rather unexpected.
It is possible to just set rxsubchans, signal and afc to 0 if thedevice node doesn't own the tuner. I'm inclined to do that.
Right, G_TUNER should not change ownership, if the tuner is currently in radio mode and a G_TUNER is done on the video node just 0 out the fields which we cannot fill with useful info.
- Should closing a device node switch ownership? E.g. if nobody has aradio device open, should the tuner switch back to TV mode automatically? I don't think it should.
+1 on delaying the mode switch until it is actually necessary to switch mode.
- How about hybrid tuners?No opinion.
Regards,
Hans
-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html