Re: [RFC] Request API and V4L2 capabilities
On 4 August 2018 at 10:50, Hans Verkuil wrote: > Hi all, > > While the Request API patch series addresses all the core API issues, there > are some high-level considerations as well: > > 1) How can the application tell that the Request API is supported and for >which buffer types (capture/output) and pixel formats? > > 2) How can the application tell if the Request API is required as opposed to > being >optional? > > 3) Some controls may be required in each request, how to let userspace know > this? >Is it even necessary to inform userspace? > > 4) (For bonus points): How to let the application know which streaming I/O > modes >are available? That's never been possible before, but it would be very nice >indeed if that's made explicit. > > Since the Request API associates data with frame buffers it makes sense to > expose > this as a new capability field in struct v4l2_requestbuffers and struct > v4l2_create_buffers. > > The first struct has 2 reserved fields, the second has 8, so it's not a > problem to > take one for a capability field. Both structs also have a buffer type, so we > know > if this is requested for a capture or output buffer type. The pixel format is > known > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt > we'll have > drivers where the request caps would actually depend on the pixel format, but > it > theoretically possible. Actually, I think that for stateless JPEG encoders and decoders, an application could work without the Request API. In that case, the same encoding/decoding parameters would be used for all the encoded/decoded buffers. So, it seems that having per-pixelformat capabilities sounds correct. > For both ioctls you can call them with count=0 at the start > of the application. REQBUFS has of course the side-effect of deleting all > buffers, > but at the start of your application you don't have any yet. CREATE_BUFS has > no > side-effects. > > I propose adding these capabilities: > > #define V4L2_BUF_CAP_HAS_REQUESTS 0x0001 > #define V4L2_BUF_CAP_REQUIRES_REQUESTS 0x0002 > #define V4L2_BUF_CAP_HAS_MMAP 0x0100 > #define V4L2_BUF_CAP_HAS_USERPTR0x0200 > #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 > > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set. > > At this time I think that REQUIRES_REQUESTS would only need to be set for the > output queue of stateless codecs. > > If capabilities is 0, then it's from an old kernel and all you know is that > requests are certainly not supported, and that MMAP is supported. Whether > USERPTR > or DMABUF are supported isn't known in that case (just try it :-) ). > > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it > is very > easy to add if we create a new capability field anyway, and it has always > annoyed > the hell out of me that we didn't have a good way to let userspace know what > streaming I/O modes we support. And with vb2 it's easy to implement. > > Regarding point 3: I think this should be documented next to the pixel > format. I.e. > the MPEG-2 Slice format used by the stateless cedrus codec requires the > request API > and that two MPEG-2 controls (slice params and quantization matrices) must be > present > in each request. > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed > here. > It's really implied by the fact that you use a stateless codec. It doesn't > help > generic applications like v4l2-ctl or qv4l2 either since in order to support > stateless codecs they will have to know about the details of these controls > anyway. > This makes a lot of sense to me. > So I am inclined to say that it is not necessary to expose this information in > the API, but it has to be documented together with the pixel format > documentation. > > Comments? Ideas? > -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Tue Aug 7 05:00:10 CEST 2018 media-tree git hash:12f336c88090fb8004736fd4329184326a49673b media_build git hash: a0cd9105aaeb5af5af36117af923a3d1de4b76d7 v4l-utils git hash: 90905c2e4b17d7595256f3824e2d30d19b0df1a1 edid-decode git hash: ab18befbcacd6cd4dff63faa82e32700369d6f25 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.16.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.115-i686: OK linux-3.18.115-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.140-i686: OK linux-4.4.140-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.112-i686: OK linux-4.9.112-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.55-i686: OK linux-4.14.55-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.6-i686: OK linux-4.17.6-x86_64: OK linux-4.18-rc4-i686: OK linux-4.18-rc4-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
do the editing
Want to follow up the email sent last week. Do you have needs for photo editing? We can edit 400 images within 24 hours. We are working on all kinds of ecommerce photos, jewelry photos, and the portrait images. We do cutting out and clipping path and others, and also we provide retouching for your photos, You can throw us a photo and we will do testing for you to check our quality. Thanks, Jason
for editing the photos
Want to follow up the email sent last week. Do you have needs for photo editing? We can edit 400 images within 24 hours. We are working on all kinds of ecommerce photos, jewelry photos, and the portrait images. We do cutting out and clipping path and others, and also we provide retouching for your photos, You can throw us a photo and we will do testing for you to check our quality. Thanks, Jason
for editing the photos
Want to follow up the email sent last week. Do you have needs for photo editing? We can edit 400 images within 24 hours. We are working on all kinds of ecommerce photos, jewelry photos, and the portrait images. We do cutting out and clipping path and others, and also we provide retouching for your photos, You can throw us a photo and we will do testing for you to check our quality. Thanks, Jason
Re: [PATCH] uvcvideo: add quirk to force Phytec CAM 004H to GBRG
Hi Christoph,
On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:
> On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:
> > Hi Laurent
> >
> >> Could you please send me the output of 'lsusb -d 199e:8302 -v' (if
> >> possible running as root) ?
> >
> > please see bottom of this mail
> >
> >>> Signed-off-by: Christoph Fritz
> >>> Tested-by: Norbert Wesp
> >>> ---
> >>>
> >>> drivers/media/usb/uvc/uvc_driver.c | 16
> >>> drivers/media/usb/uvc/uvcvideo.h | 1 +
> >>> 2 files changed, 17 insertions(+)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> >>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
> >>> *dev,
> >>> width_multiplier = 2;
> >>> }
> >>> }
> >>> + if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
> >>> + if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
> >>> + strlcpy(format->name, "GBRG Bayer (GBRG)",
> >>> + sizeof(format->name));
> >>> + format->fcc = V4L2_PIX_FMT_SGBRG8;
> >>> + }
> >>> + }
> >>>
> >>> if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
> >>> ftype = UVC_VS_FRAME_UNCOMPRESSED;
> >>> @@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
> >>> .bInterfaceClass = USB_CLASS_VENDOR_SPEC,
> >>> .bInterfaceSubClass = 1,
> >>> .bInterfaceProtocol = 0 },
> >>> + /* PHYTEC CAM 004H cameras */
> >>> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> >>> + | USB_DEVICE_ID_MATCH_INT_INFO,
> >>> + .idVendor = 0x199e,
> >>> + .idProduct= 0x8302,
> >>> + .bInterfaceClass = USB_CLASS_VIDEO,
> >>> + .bInterfaceSubClass = 1,
> >>> + .bInterfaceProtocol = 0,
> >>> + .driver_info = UVC_QUIRK_FORCE_GBRG },
> >>> /* Bodelin ProScopeHR */
> >>> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> >>> | USB_DEVICE_ID_MATCH_DEV_HI
> >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> >>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644
> >>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>> @@ -164,6 +164,7 @@
> >>> #define UVC_QUIRK_RESTRICT_FRAME_RATE0x0200
> >>> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x0400
> >>> #define UVC_QUIRK_FORCE_Y8 0x0800
> >>> +#define UVC_QUIRK_FORCE_GBRG 0x1000
> >>
> >> I don't think we should add a quirk flag for every format that needs to
> >> be forced. Instead, now that we have a new way to store per-device
> >> parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible
> >> device information"), how about making use of it and adding a field to
> >> the uvc_device_info structure to store the forced format ?
> >
> > I'm currently stuck on kernel 4.9, but for mainline I'll use extensible
> > device information and send a v2 in the next weeks.
> >
> > output of 'lsusb -d 199e:8302 -v':
> >
> > Bus 001 Device 010: ID 199e:8302 The Imaging Source Europe GmbH
> >
> > Device Descriptor:
> > bLength18
> > bDescriptorType 1
> > bcdUSB 2.00
> > bDeviceClass 239 Miscellaneous Device
> > bDeviceSubClass 2 ?
> > bDeviceProtocol 1 Interface Association
> > bMaxPacketSize064
> > idVendor 0x199e The Imaging Source Europe GmbH
> > idProduct 0x8302
> > bcdDevice8.13
> > iManufacturer 2 ?
> > iProduct1 ?
> > iSerial 3 ?
> > bNumConfigurations 1
[snip]
> > VideoStreaming Interface Descriptor:
> > bLength28
> > bDescriptorType36
> > bDescriptorSubtype 16 (FORMAT_FRAME_BASED)
> > bFormatIndex1
> > bNumFrameDescriptors5
> > guidFormat
> > {47524247--1000-8000-00aa00389b71}
>
> I only know of two devices supporting this format, this one and the DFx
> 72BUC02. The only two devices I'm aware of that use other Bayer formats are
> the DFK 23UV024 and DFK 23UX249, and they both report a GBRG format.
>
> I wonder if the DFx 72BUC02 is affected too. In any case, as your device
> exposes a single format, you can force the format to V4L2_PIX_FMT_SGBRG8
> without first checking whether it is equal to V4L2_PIX_FMT_SGRBG8. Feel free
> to patch the existing FORCE_Y8 code to also remove the format check, I've
> discussed it today with Philipp Zabel who implemented the FORCE_Y8 quirk
> and he's fine with that.
Any update on a v2 for this patch ?
--
[PATCH] media: uvcvideo: Make uvc_control_mapping menu_info field const
The menu_info field of the uvc_control_mapping structure points to an
array of menu info data that are never changed by the driver. Make the
pointer const and constify the related static arrays in the driver.
Signed-off-by: Laurent Pinchart
---
drivers/media/usb/uvc/uvc_ctrl.c | 10 +-
drivers/media/usb/uvc/uvcvideo.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 146e0ac09ba2..79fdb7ced565 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -354,13 +354,13 @@ static const struct uvc_control_info uvc_ctrls[] = {
},
};
-static struct uvc_menu_info power_line_frequency_controls[] = {
+static const struct uvc_menu_info power_line_frequency_controls[] = {
{ 0, "Disabled" },
{ 1, "50 Hz" },
{ 2, "60 Hz" },
};
-static struct uvc_menu_info exposure_auto_controls[] = {
+static const struct uvc_menu_info exposure_auto_controls[] = {
{ 2, "Auto Mode" },
{ 1, "Manual Mode" },
{ 4, "Shutter Priority Mode" },
@@ -978,7 +978,7 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping
*mapping,
s32 value = mapping->get(mapping, UVC_GET_CUR, data);
if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
- struct uvc_menu_info *menu = mapping->menu_info;
+ const struct uvc_menu_info *menu = mapping->menu_info;
unsigned int i;
for (i = 0; i < mapping->menu_count; ++i, ++menu) {
@@ -1025,7 +1025,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain
*chain,
{
struct uvc_control_mapping *master_map = NULL;
struct uvc_control *master_ctrl = NULL;
- struct uvc_menu_info *menu;
+ const struct uvc_menu_info *menu;
unsigned int i;
memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
@@ -1145,7 +1145,7 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
struct v4l2_querymenu *query_menu)
{
- struct uvc_menu_info *menu_info;
+ const struct uvc_menu_info *menu_info;
struct uvc_control_mapping *mapping;
struct uvc_control *ctrl;
u32 index = query_menu->index;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index f7dcd3295a99..067cd5bb5a8a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -234,7 +234,7 @@ struct uvc_control_mapping {
enum v4l2_ctrl_type v4l2_type;
u32 data_type;
- struct uvc_menu_info *menu_info;
+ const struct uvc_menu_info *menu_info;
u32 menu_count;
u32 master_id;
--
Regards,
Laurent Pinchart
Re: [PATCH] uvcvideo: extend UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams
Hi Pavel,
On Tuesday, 5 September 2017 20:27:39 EEST Pavel Rojtberg wrote:
> 2017-09-04 11:56 GMT+02:00 Laurent Pinchart:
> > On Monday, 4 September 2017 11:14:17 EEST Pavel Rojtberg wrote:
> >> From: Pavel Rojtberg
> >>
> >> attaching two Logitech C615 webcams currently results in
> >>
> >> VIDIOC_STREAMON: No space left on device
> >>
> >> as the required bandwidth is not estimated correctly by the device.
> >> In fact it always requests 3060 bytes - no matter the format or
> >> resolution.
> >>
> >> setting UVC_QUIRK_FIX_BANDWIDTH does not help either as it is only
> >> implemented for uncompressed streams.
> >>
> >> This patch extends UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams by making a
> >> (conservative) assumption of 4bpp for MJPEG streams.
> >> As the actual compression ration is often closer to 1bpp this can be
> >> overridden via the new mjpeg_bpp parameter.
> >>
> >> Based on:
> >> https://www.mail-archive.com/linux-uvc-devel@lists.berlios.de/msg05724.ht
> >> ml
> >> ---
> >>
> >> drivers/media/usb/uvc/uvc_driver.c | 14 +-
> >> drivers/media/usb/uvc/uvc_video.c | 3 ++-
> >> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> >> b/drivers/media/usb/uvc/uvc_driver.c index 70842c5..f7b759e 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -37,6 +37,7 @@ unsigned int uvc_no_drop_param;
> >> static unsigned int uvc_quirks_param = -1;
> >> unsigned int uvc_trace_param;
> >> unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;
> >> +static unsigned int uvc_mjpeg_bpp_param;
> >>
> >> /* -
> >> * Video formats
> >> @@ -463,7 +464,16 @@ static int uvc_parse_format(struct uvc_device *dev,
> >> strlcpy(format->name, "MJPEG", sizeof format->name);
> >> format->fcc = V4L2_PIX_FMT_MJPEG;
> >> format->flags = UVC_FMT_FLAG_COMPRESSED;
> >> - format->bpp = 0;
> >> + if ((uvc_mjpeg_bpp_param >= 1) && (uvc_mjpeg_bpp_param <=
> >> 16)) {
> >> + format->bpp = uvc_mjpeg_bpp_param;
> >> + } else {
> >> + /* conservative estimate. Actual values are around
> >> 1bpp.
> >> + * see e.g.
> >> + *
> >> https://developers.google.com/speed/webp/docs/webp_study
> >> + */
> >> + format->bpp = 4;
> >> + }
> >> +
> >>
> >> ftype = UVC_VS_FRAME_MJPEG;
> >> break;
> >>
> >> @@ -2274,6 +2284,8 @@ module_param_named(trace, uvc_trace_param, uint,
> >> S_IRUGO|S_IWUSR); MODULE_PARM_DESC(trace, "Trace level bitmask");
> >>
> >> module_param_named(timeout, uvc_timeout_param, uint, S_IRUGO|S_IWUSR);
> >> MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> >>
> >> +module_param_named(mjpeg_bpp, uvc_mjpeg_bpp_param, uint,
> >> S_IRUGO|S_IWUSR);
> >> +MODULE_PARM_DESC(mjpeg_bpp, "MJPEG bits per pixel for bandwidth quirk");
> >
> > I'd rather avoid adding a new module parameter for this, it would be
> > confusing for users. What is your use case to make the MJPEG average BPP
> > configurable ? Can't we come up with a heuristic that would calculate it
> > automatically ?
>
> this is the minimal expected JPEG compression ratio. It depends on the
> JPEG quality chosen by the hardware (which we do not know) as well as the
> actual image (which we can not control either).
> We could hardcode it to the average bpp which 1 - but this will break
> when somebody films a high-frequency checkerborad. We could leave it at 4,
> but this would again artificially limit the number of possible cameras when
> not filming a checkerboard.
> 4 would already be a noticable improvement over the current state though.
>
> If you can come up with a working heuristic - I am open for that too.
So it's basically an estimate of the MJPEG compression ratio, which depends on
multiple parameters, some of which we could possibly control (such as the
compression quality), and some of which we have no influence on (such as the
scene in front of the camera).
What really bothers me is that if even we can't estimate the value, exposing
it as a module parameter will only confuse most users. How about starting with
a fixed conservative estimate (such as 4bpp) and see where that leads us ?
I would also not try to fake a bpp value in uvc_parse_format() for MJPEG
formats, but instead adapt uvc_fixup_video_ctrl() with MJPEG-specific
calculation. This would allow expressing a compression ratio in the code
instead of an artificial bpp value, allowing more precision in the future if
needed.
> >> /* -
> >> * Driver initialization and cleanup
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >>
Re: [PATCHv17 31/34] vim2m: support requests
Hey Hans, Just a small nit. On Sat, 2018-08-04 at 14:45 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Add support for requests to vim2m. > > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 26 ++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > index 6f87ef025ff1..3b8df2c9d24a 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -379,8 +379,18 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + /* Apply request controls if needed */ > + if (src_buf->vb2_buf.req_obj.req) Seems v4l2_ctrl_request_setup checks for null parameters, so the check is not needed. > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + >hdl); > + > device_process(ctx, src_buf, dst_buf); > > + /* Complete request controls if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, > + >hdl); > + > Ditto.
Re: [RFC PATCH 0/3] Media Controller Properties
On 08/03/2018 05:23 PM, Mauro Carvalho Chehab wrote: > Em Fri, 3 Aug 2018 17:03:20 +0200 >>> I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest >>> option by renaming the old ioctl and adding a new one with property support. > > Why? No need for that at the public header. Just add the needed fields at the > end of the code and check for struct size at the ioctl handler. > > It could make sense to have the old struct inside media-device.c, just > to allow using sizeof() there. Sorry, you need the old struct. The application may be newer than the kernel, so if the new topology struct (with props support) doesn't work, then it has to fall back to the old ioctl. So applications will need to know the old size. That said, it would be sufficient in this case to just export the old ioctl define since the old struct layout is identical to the new (except of course for the new fields added to the end). E.g. add just this to media.h: /* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */ #define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04 This brings me to another related question: I can easily support both the old and new G_TOPOLOGY ioctls in media-device.c. But what should I do if we add still more fields to the topology struct in the future and so we might be called by a newer application with a G_TOPOLOGY ioctl that has a size larger than we have now. We can either reject this (that's what we do today in fact, hence the need for the TOPOLOGY_OLD), or we can just accept it and zero the unknown fields at the end of the larger struct. I think we need to do the latter, otherwise we will have to keep adding new ioctl variants whenever we add a field. Alternatively, we can just add a pile of reserved fields to struct media_v2_topology which should last us for many years based on past experience with reserved fields. Regards, Hans
Re: [RFC] Request API and V4L2 capabilities
On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote: > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote: > > Hi, > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > > > > > Regarding point 3: I think this should be documented next to the > > > > > pixel format. I.e. > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires > > > > > the request API > > > > > and that two MPEG-2 controls (slice params and quantization matrices) > > > > > must be present > > > > > in each request. > > > > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is > > > > > needed here. > > > > > It's really implied by the fact that you use a stateless codec. It > > > > > doesn't help > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to > > > > > support > > > > > stateless codecs they will have to know about the details of these > > > > > controls anyway. > > > > > > > > > > So I am inclined to say that it is not necessary to expose this > > > > > information in > > > > > the API, but it has to be documented together with the pixel format > > > > > documentation. > > > > > > > > I think this is affected by considerations about codec profile/level > > > > support. More specifically, some controls will only be required for > > > > supporting advanced codec profiles/levels, so they can only be > > > > explicitly marked with appropriate flags by the driver when the target > > > > profile/level is known. And I don't think it would be sane for userspace > > > > to explicitly set what profile/level it's aiming at. As a result, I > > > > don't think we can explicitly mark controls as required or optional. > > > > > > > > I also like the idea that it should instead be implicit and that the > > > > documentation should detail which specific stateless metadata controls > > > > are required for a given profile/level. > > > > > > > > As for controls validation, the approach followed in the Cedrus driver > > > > is to check that the most basic controls are filled and allow having > > > > missing controls for those that match advanced profiles. > > > > > > > > Since this approach feels somewhat generic enough to be applied to all > > > > stateless VPU drivers, maybe this should be made a helper in the > > > > framework? > > > > > > Sounds reasonable. Not sure if it will be in the first version, but it is > > > easy to add later. > > > > Definitely, I don't think this is such a high priority for now either. > > > > > > In addition, I see a need for exposing the maximum profile/level that > > > > the driver supports for decoding. I would suggest reusing the already- > > > > existing dedicated controls used for encoding for this purpose. For > > > > decoders, they would be used to expose the (read-only) maximum > > > > profile/level that is supported by the hardware and keep using them as a > > > > settable value in a range (matching the level of support) for encoders. > > > > > > > > This is necessary for userspace to determine whether a given video can > > > > be decoded in hardware or not. Instead of half-way decoding the video > > > > (ending up in funky results), this would easily allow skipping hardware > > > > decoding and e.g. falling back on software decoding. > > > > > > I think it might be better to expose this through new read-only bitmask > > > controls: i.e. a bitmask containing the supported profiles and levels. > > > > It seems that this is more or less what the coda driver is doing for > > decoding actually, although it uses a menu control between min/max > > supported profile/levels, with a mask to "blacklist" the unsupported > > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the > > control read-only. > > > > > Reusing the existing controls for a decoder is odd since there is not > > > really a concept of a 'current' value since you just want to report what > > > is supported. And I am not sure if all decoders can report the profile > > > or level that they detect. > > > > Is that really a problem when the READ_ONLY flag is set? I thought it > > was designed to fit this specific case, when the driver reports a value > > that userspace cannot affect. > > Well, for read-only menu controls the current value of the control would > have to indicate what the current profile/level is that is being decoded. > > That's not really relevant since what you want is just to query the > supported profiles/levels. A read-only bitmask control is the fastest > method (if only because using a menu control requires the application to > enumerate all possibilities with QUERYMENU). Ah yes, I finally understand the issue with what the current control value represents here. Since I don't think the driver should have to bother with figuring out the profile in use (as expressed earlier, I think it should be
Re: [RFC] Request API and V4L2 capabilities
On 08/06/2018 11:13 AM, Paul Kocialkowski wrote: > Hi, > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: >> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: >>> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: Regarding point 3: I think this should be documented next to the pixel format. I.e. the MPEG-2 Slice format used by the stateless cedrus codec requires the request API and that two MPEG-2 controls (slice params and quantization matrices) must be present in each request. I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here. It's really implied by the fact that you use a stateless codec. It doesn't help generic applications like v4l2-ctl or qv4l2 either since in order to support stateless codecs they will have to know about the details of these controls anyway. So I am inclined to say that it is not necessary to expose this information in the API, but it has to be documented together with the pixel format documentation. >>> >>> I think this is affected by considerations about codec profile/level >>> support. More specifically, some controls will only be required for >>> supporting advanced codec profiles/levels, so they can only be >>> explicitly marked with appropriate flags by the driver when the target >>> profile/level is known. And I don't think it would be sane for userspace >>> to explicitly set what profile/level it's aiming at. As a result, I >>> don't think we can explicitly mark controls as required or optional. >>> >>> I also like the idea that it should instead be implicit and that the >>> documentation should detail which specific stateless metadata controls >>> are required for a given profile/level. >>> >>> As for controls validation, the approach followed in the Cedrus driver >>> is to check that the most basic controls are filled and allow having >>> missing controls for those that match advanced profiles. >>> >>> Since this approach feels somewhat generic enough to be applied to all >>> stateless VPU drivers, maybe this should be made a helper in the >>> framework? >> >> Sounds reasonable. Not sure if it will be in the first version, but it is >> easy to add later. > > Definitely, I don't think this is such a high priority for now either. > >>> In addition, I see a need for exposing the maximum profile/level that >>> the driver supports for decoding. I would suggest reusing the already- >>> existing dedicated controls used for encoding for this purpose. For >>> decoders, they would be used to expose the (read-only) maximum >>> profile/level that is supported by the hardware and keep using them as a >>> settable value in a range (matching the level of support) for encoders. >>> >>> This is necessary for userspace to determine whether a given video can >>> be decoded in hardware or not. Instead of half-way decoding the video >>> (ending up in funky results), this would easily allow skipping hardware >>> decoding and e.g. falling back on software decoding. >> >> I think it might be better to expose this through new read-only bitmask >> controls: i.e. a bitmask containing the supported profiles and levels. > > It seems that this is more or less what the coda driver is doing for > decoding actually, although it uses a menu control between min/max > supported profile/levels, with a mask to "blacklist" the unsupported > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the > control read-only. > >> Reusing the existing controls for a decoder is odd since there is not >> really a concept of a 'current' value since you just want to report what >> is supported. And I am not sure if all decoders can report the profile >> or level that they detect. > > Is that really a problem when the READ_ONLY flag is set? I thought it > was designed to fit this specific case, when the driver reports a value > that userspace cannot affect. Well, for read-only menu controls the current value of the control would have to indicate what the current profile/level is that is being decoded. That's not really relevant since what you want is just to query the supported profiles/levels. A read-only bitmask control is the fastest method (if only because using a menu control requires the application to enumerate all possibilities with QUERYMENU). > > Otherwise, I agree that having a bitmask type would be a better fit, but > I think it would be beneficial to keep the already-defined control and > associated values, which implies using the menu control type for both > encoders and decoders. > > If this is not an option, I would be in favour of adding per-codec read- > only bitmask controls (e.g. for H264 something like > V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already- > existing profile/level definitions as bit identifiers (a bit like coda > is using them to craft a mask for the menu items to blacklist) for > decoding only.
Re: [RFC] Request API and V4L2 capabilities
Hi, On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote: > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > > > Regarding point 3: I think this should be documented next to the pixel > > > format. I.e. > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the > > > request API > > > and that two MPEG-2 controls (slice params and quantization matrices) > > > must be present > > > in each request. > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is > > > needed here. > > > It's really implied by the fact that you use a stateless codec. It > > > doesn't help > > > generic applications like v4l2-ctl or qv4l2 either since in order to > > > support > > > stateless codecs they will have to know about the details of these > > > controls anyway. > > > > > > So I am inclined to say that it is not necessary to expose this > > > information in > > > the API, but it has to be documented together with the pixel format > > > documentation. > > > > I think this is affected by considerations about codec profile/level > > support. More specifically, some controls will only be required for > > supporting advanced codec profiles/levels, so they can only be > > explicitly marked with appropriate flags by the driver when the target > > profile/level is known. And I don't think it would be sane for userspace > > to explicitly set what profile/level it's aiming at. As a result, I > > don't think we can explicitly mark controls as required or optional. > > > > I also like the idea that it should instead be implicit and that the > > documentation should detail which specific stateless metadata controls > > are required for a given profile/level. > > > > As for controls validation, the approach followed in the Cedrus driver > > is to check that the most basic controls are filled and allow having > > missing controls for those that match advanced profiles. > > > > Since this approach feels somewhat generic enough to be applied to all > > stateless VPU drivers, maybe this should be made a helper in the > > framework? > > Sounds reasonable. Not sure if it will be in the first version, but it is > easy to add later. Definitely, I don't think this is such a high priority for now either. > > In addition, I see a need for exposing the maximum profile/level that > > the driver supports for decoding. I would suggest reusing the already- > > existing dedicated controls used for encoding for this purpose. For > > decoders, they would be used to expose the (read-only) maximum > > profile/level that is supported by the hardware and keep using them as a > > settable value in a range (matching the level of support) for encoders. > > > > This is necessary for userspace to determine whether a given video can > > be decoded in hardware or not. Instead of half-way decoding the video > > (ending up in funky results), this would easily allow skipping hardware > > decoding and e.g. falling back on software decoding. > > I think it might be better to expose this through new read-only bitmask > controls: i.e. a bitmask containing the supported profiles and levels. It seems that this is more or less what the coda driver is doing for decoding actually, although it uses a menu control between min/max supported profile/levels, with a mask to "blacklist" the unsupported values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the control read-only. > Reusing the existing controls for a decoder is odd since there is not > really a concept of a 'current' value since you just want to report what > is supported. And I am not sure if all decoders can report the profile > or level that they detect. Is that really a problem when the READ_ONLY flag is set? I thought it was designed to fit this specific case, when the driver reports a value that userspace cannot affect. Otherwise, I agree that having a bitmask type would be a better fit, but I think it would be beneficial to keep the already-defined control and associated values, which implies using the menu control type for both encoders and decoders. If this is not an option, I would be in favour of adding per-codec read- only bitmask controls (e.g. for H264 something like V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already- existing profile/level definitions as bit identifiers (a bit like coda is using them to craft a mask for the menu items to blacklist) for decoding only. What do you think? Cheers, Paul -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [RFC] Request API and V4L2 capabilities
On 08/06/2018 10:16 AM, Paul Kocialkowski wrote: > Hi Hans and all, > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: >> Hi all, >> >> While the Request API patch series addresses all the core API issues, there >> are some high-level considerations as well: >> >> 1) How can the application tell that the Request API is supported and for >>which buffer types (capture/output) and pixel formats? >> >> 2) How can the application tell if the Request API is required as opposed to >> being >>optional? >> >> 3) Some controls may be required in each request, how to let userspace know >> this? >>Is it even necessary to inform userspace? >> >> 4) (For bonus points): How to let the application know which streaming I/O >> modes >>are available? That's never been possible before, but it would be very >> nice >>indeed if that's made explicit. > > Thanks for bringing up these considerations and questions, which perhaps > cover the last missing bits for streamlined use of the request API by > userspace. I would suggest another item, related to 3): > > 5) How can applications tell whether the driver supports a specific > codec profile/level, not only for encoding but also for decoding? It's > common for low-end embedded hardware to not support the most advanced > profiles (e.g. H264 high profile). > >> Since the Request API associates data with frame buffers it makes sense to >> expose >> this as a new capability field in struct v4l2_requestbuffers and struct >> v4l2_create_buffers. >> >> The first struct has 2 reserved fields, the second has 8, so it's not a >> problem to >> take one for a capability field. Both structs also have a buffer type, so we >> know >> if this is requested for a capture or output buffer type. The pixel format >> is known >> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt >> we'll have >> drivers where the request caps would actually depend on the pixel format, >> but it >> theoretically possible. For both ioctls you can call them with count=0 at >> the start >> of the application. REQBUFS has of course the side-effect of deleting all >> buffers, >> but at the start of your application you don't have any yet. CREATE_BUFS has >> no >> side-effects. > > My initial thoughts on this point were to have flags exposed in > v4l2_capability, but now that you're saying it, it does make sense for > the flag to be associated with a buffer rather than the global device. > > In addition, I've heard of cases (IIRC it was some Rockchip platforms) > where the platform has both stateless and stateful VPUs (I think it was > stateless up to H264 and stateful for H265). This would allow supporting > these two hardware blocks under the same video device (if that makes > sense anyway). And even if there's no immediate need, it's always good > to have this level of granularity (with little drawbacks). > >> I propose adding these capabilities: >> >> #define V4L2_BUF_CAP_HAS_REQUESTS0x0001 >> #define V4L2_BUF_CAP_REQUIRES_REQUESTS 0x0002 >> #define V4L2_BUF_CAP_HAS_MMAP0x0100 >> #define V4L2_BUF_CAP_HAS_USERPTR 0x0200 >> #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 >> >> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set. >> >> At this time I think that REQUIRES_REQUESTS would only need to be set for the >> output queue of stateless codecs. >> >> If capabilities is 0, then it's from an old kernel and all you know is that >> requests are certainly not supported, and that MMAP is supported. Whether >> USERPTR >> or DMABUF are supported isn't known in that case (just try it :-) ). > > Sounds good to me! > >> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it >> is very >> easy to add if we create a new capability field anyway, and it has always >> annoyed >> the hell out of me that we didn't have a good way to let userspace know what >> streaming I/O modes we support. And with vb2 it's easy to implement. > > I totally agree here, it would be very nice to take the occasion to > expose to userspace what I/O modes are available. The current try-and- > see approach works, but this feels much better indeed. > >> Regarding point 3: I think this should be documented next to the pixel >> format. I.e. >> the MPEG-2 Slice format used by the stateless cedrus codec requires the >> request API >> and that two MPEG-2 controls (slice params and quantization matrices) must >> be present >> in each request. >> >> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed >> here. >> It's really implied by the fact that you use a stateless codec. It doesn't >> help >> generic applications like v4l2-ctl or qv4l2 either since in order to support >> stateless codecs they will have to know about the details of these controls >> anyway. >> >> So I am inclined to say that it is not necessary to expose this information >> in >> the API, but it has to be
Re: [RFC] Request API and V4L2 capabilities
Hi Hans and all, On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote: > Hi all, > > While the Request API patch series addresses all the core API issues, there > are some high-level considerations as well: > > 1) How can the application tell that the Request API is supported and for >which buffer types (capture/output) and pixel formats? > > 2) How can the application tell if the Request API is required as opposed to > being >optional? > > 3) Some controls may be required in each request, how to let userspace know > this? >Is it even necessary to inform userspace? > > 4) (For bonus points): How to let the application know which streaming I/O > modes >are available? That's never been possible before, but it would be very nice >indeed if that's made explicit. Thanks for bringing up these considerations and questions, which perhaps cover the last missing bits for streamlined use of the request API by userspace. I would suggest another item, related to 3): 5) How can applications tell whether the driver supports a specific codec profile/level, not only for encoding but also for decoding? It's common for low-end embedded hardware to not support the most advanced profiles (e.g. H264 high profile). > Since the Request API associates data with frame buffers it makes sense to > expose > this as a new capability field in struct v4l2_requestbuffers and struct > v4l2_create_buffers. > > The first struct has 2 reserved fields, the second has 8, so it's not a > problem to > take one for a capability field. Both structs also have a buffer type, so we > know > if this is requested for a capture or output buffer type. The pixel format is > known > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt > we'll have > drivers where the request caps would actually depend on the pixel format, but > it > theoretically possible. For both ioctls you can call them with count=0 at the > start > of the application. REQBUFS has of course the side-effect of deleting all > buffers, > but at the start of your application you don't have any yet. CREATE_BUFS has > no > side-effects. My initial thoughts on this point were to have flags exposed in v4l2_capability, but now that you're saying it, it does make sense for the flag to be associated with a buffer rather than the global device. In addition, I've heard of cases (IIRC it was some Rockchip platforms) where the platform has both stateless and stateful VPUs (I think it was stateless up to H264 and stateful for H265). This would allow supporting these two hardware blocks under the same video device (if that makes sense anyway). And even if there's no immediate need, it's always good to have this level of granularity (with little drawbacks). > I propose adding these capabilities: > > #define V4L2_BUF_CAP_HAS_REQUESTS 0x0001 > #define V4L2_BUF_CAP_REQUIRES_REQUESTS0x0002 > #define V4L2_BUF_CAP_HAS_MMAP 0x0100 > #define V4L2_BUF_CAP_HAS_USERPTR 0x0200 > #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 > > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set. > > At this time I think that REQUIRES_REQUESTS would only need to be set for the > output queue of stateless codecs. > > If capabilities is 0, then it's from an old kernel and all you know is that > requests are certainly not supported, and that MMAP is supported. Whether > USERPTR > or DMABUF are supported isn't known in that case (just try it :-) ). Sounds good to me! > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it > is very > easy to add if we create a new capability field anyway, and it has always > annoyed > the hell out of me that we didn't have a good way to let userspace know what > streaming I/O modes we support. And with vb2 it's easy to implement. I totally agree here, it would be very nice to take the occasion to expose to userspace what I/O modes are available. The current try-and- see approach works, but this feels much better indeed. > Regarding point 3: I think this should be documented next to the pixel > format. I.e. > the MPEG-2 Slice format used by the stateless cedrus codec requires the > request API > and that two MPEG-2 controls (slice params and quantization matrices) must be > present > in each request. > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed > here. > It's really implied by the fact that you use a stateless codec. It doesn't > help > generic applications like v4l2-ctl or qv4l2 either since in order to support > stateless codecs they will have to know about the details of these controls > anyway. > > So I am inclined to say that it is not necessary to expose this information in > the API, but it has to be documented together with the pixel format > documentation. I think this is affected by considerations about codec profile/level support. More specifically, some controls will only be required for
