Re: [PATCH v2 03/23] v4l: Support extending the v4l2_pix_format structure

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

Thank you for the review.

On Friday 27 June 2014 14:36:40 Hans Verkuil wrote:
> Hi Laurent,
> 
> Some comments below...
> 
> On 06/24/2014 01:54 AM, Laurent Pinchart wrote:
> > The v4l2_pix_format structure has no reserved field. It is embedded in
> > the v4l2_framebuffer structure which has no reserved fields either, and
> > in the v4l2_format structure which has reserved fields that were not
> > previously required to be zeroed out by applications.
> > 
> > To allow extending v4l2_pix_format, inline it in the v4l2_framebuffer
> > structure, and use the priv field as a magic value to indicate that the
> > application has set all v4l2_pix_format extended fields and zeroed all
> > reserved fields following the v4l2_pix_format field in the v4l2_format
> > structure.
> > 
> > The availability of this API extension is reported to userspace through
> > the new V4L2_CAP_EXT_PIX_FORMAT capability flag. Just checking that the
> > priv field is still set to the magic value at [GS]_FMT return wouldn't
> > be enough, as older kernels don't zero the priv field on return.
> > 
> > To simplify the internal API towards drivers zero the extended fields
> > and set the priv field to the magic value for applications not aware of
> > the extensions.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >   Documentation/DocBook/media/Makefile               |  2 +-
> >   Documentation/DocBook/media/v4l/pixfmt.xml         | 25 +++++++--
> >   Documentation/DocBook/media/v4l/v4l2.xml           |  8 +++
> >   .../DocBook/media/v4l/vidioc-querycap.xml          |  6 +++
> >   drivers/media/parport/bw-qcam.c                    |  2 -
> >   drivers/media/pci/cx18/cx18-ioctl.c                |  1 -
> >   drivers/media/pci/cx25821/cx25821-video.c          |  3 --
> >   drivers/media/pci/ivtv/ivtv-ioctl.c                |  3 --
> >   drivers/media/pci/meye/meye.c                      |  2 -
> >   drivers/media/pci/saa7134/saa7134-empress.c        |  3 --
> >   drivers/media/pci/saa7134/saa7134-video.c          |  2 -
> >   drivers/media/pci/sta2x11/sta2x11_vip.c            |  1 -
> >   drivers/media/platform/coda.c                      |  2 -
> >   drivers/media/platform/davinci/vpif_display.c      |  1 -
> >   drivers/media/platform/mem2mem_testdev.c           |  1 -
> >   drivers/media/platform/omap/omap_vout.c            |  2 -
> >   drivers/media/platform/sh_veu.c                    |  2 -
> >   drivers/media/platform/vino.c                      |  5 --
> >   drivers/media/platform/vivi.c                      |  1 -
> >   drivers/media/usb/cx231xx/cx231xx-417.c            |  2 -
> >   drivers/media/usb/cx231xx/cx231xx-video.c          |  2 -
> >   drivers/media/usb/gspca/gspca.c                    |  8 +--
> >   drivers/media/usb/hdpvr/hdpvr-video.c              |  1 -
> >   drivers/media/usb/stkwebcam/stk-webcam.c           |  2 -
> >   drivers/media/usb/tlg2300/pd-video.c               |  1 -
> >   drivers/media/usb/tm6000/tm6000-video.c            |  2 -
> >   drivers/media/usb/zr364xx/zr364xx.c                |  3 --
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c      | 19 +++++--
> >   drivers/media/v4l2-core/v4l2-ioctl.c               | 61 ++++++++++++++--
> >   include/uapi/linux/videodev2.h                     | 15 +++++-
> >   30 files changed, 126 insertions(+), 62 deletions(-)
> > 

[snip]

> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c index 16bffd8..01b4588 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c

[snip]

> > @@ -1089,12 +1124,17 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops
> > *ops,
> >   	bool is_sdr = vfd->vfl_type = VFL_TYPE_SDR;
> >   	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
> >   	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
> > +	int ret;
> > +
> > +	v4l_sanitize_format(p);
> 
> Note: before g_fmt is called all fields after 'type' are zeroed.
> So it is enough to just set priv to the magic value here.

Agreed, I can change that.

> >   	switch (p->type) {
> >   	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >   		if (unlikely(!is_rx || !is_vid || !ops->vidioc_g_fmt_vid_cap))
> >   			break;
> > -		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> > +		ret = ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> > +		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> 
> v4l_sanitize_format already sets priv to PRIV_MAGIC, so there is no need to
> do it again.

Except that the vidioc_g_fmt_vid_cap() implementation might have zeroed it. 
I've removed the priv assignments to zero in drivers wherever I found them, 
but I might have missed one, hence the explicit assignment here.

> > +		return ret;
> >   	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >   		if (unlikely(!is_rx || !is_vid || !ops-
>vidioc_g_fmt_vid_cap_mplane))
> >   			break;
> > @@ -1114,7 +1154,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops
> > *ops,> 
> >   	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> >   		if (unlikely(!is_tx || !is_vid || !ops->vidioc_g_fmt_vid_out))
> >   			break;
> > -		return ops->vidioc_g_fmt_vid_out(file, fh, arg);
> > +		ret = ops->vidioc_g_fmt_vid_out(file, fh, arg);
> > +		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> 
> Ditto.

Same explanation as above.

> > +		return ret;
> >   	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >   		if (unlikely(!is_tx || !is_vid || !ops-
>vidioc_g_fmt_vid_out_mplane))
> >   			break;
> > @@ -1502,7 +1544,18 @@ static int v4l_create_bufs(const struct
> > v4l2_ioctl_ops *ops,> 
> >   	struct v4l2_create_buffers *create = arg;
> >   	int ret = check_fmt(file, create->format.type);
> > -	return ret ? ret : ops->vidioc_create_bufs(file, fh, create);
> > +	if (ret)
> > +		return ret;
> > +
> > +	v4l_sanitize_format(&create->format);
> > +
> > +	ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +	if (create->format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> > +	    create->format.type = V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +		create->format.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> 
> Ditto.

Ditto :-)

> > +
> > +	return ret;
> >   }
> 
> Shouldn't v4l_sanitize_format also be called for s/try_fmt? It makes much
> more sense there.

I can't remember why I haven't added a call to v4l_sanitize_format there, so 
I'll assume it was just an oversight, I'll fix that.

> >   static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
> > 
> > diff --git a/include/uapi/linux/videodev2.h
> > b/include/uapi/linux/videodev2.h index 0125f4d..2656a94 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -268,6 +268,7 @@ struct v4l2_capability {
> > 
> >  #define V4L2_CAP_MODULATOR		0x00080000  /* has a modulator */
> >   
> >  #define V4L2_CAP_SDR_CAPTURE	0x00100000  /* Is a SDR capture device */
> > +#define V4L2_CAP_EXT_PIX_FORMAT		0x00200000  /* Supports the extended
> > pixel format */ 
> >  #define V4L2_CAP_READWRITE      0x01000000  /* read/write systemcalls */
> > #define V4L2_CAP_ASYNCIO         0x02000000  /* async I/O */
> > @@ -448,6 +449,9 @@ struct v4l2_pix_format {
> > 
> >   #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ
> >   u8 */ #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6')
> >   /* IQ u16le */> 
> > +/* priv field value to indicates that subsequent fields are valid. */
> > +#define V4L2_PIX_FMT_PRIV_MAGIC		0xdeadbeef
> 
> Hmm, 'deadbeef' is used quite a lot (git grep deadbeef), perhaps we should
> use another magic number. E.g. 'feedcafe' or something like that.

Sure.

> > +
> >   /*
> >    *	F O R M A T   E N U M E R A T I O N
> >    */
> > @@ -752,7 +756,16 @@ struct v4l2_framebuffer {
> >   /* FIXME: in theory we should pass something like PCI device + memory
> >    * region + offset instead of some physical address */
> >   	void                    *base;
> > -	struct v4l2_pix_format	fmt;
> > +	struct {
> > +		__u32		width;
> > +		__u32		height;
> > +		__u32		pixelformat;
> > +		__u32		field;		/* enum v4l2_field */
> > +		__u32		bytesperline;	/* for padding, zero if unused */
> > +		__u32		sizeimage;
> > +		__u32		colorspace;	/* enum v4l2_colorspace */
> > +		__u32		priv;		/* private data, depends on pixelformat 
*/
> 
> Let's change the comment to '/* Reserved field, set to 0. */.
> Ditto in the documentation. 'priv' has never been used for anything
> framebuffer related, so that whole 'private data' concept that priv was
> once used for should be eradicated.
> 
> Note that DocBook should be updated as well w.r.t. the v4l2_framebuffer
> struct.

Indeed. I'll fix that.

> > +	} fmt;
> > 
> >   };
> >   /*  Flags for the 'capability' field. Read only */
> >   #define V4L2_FBUF_CAP_EXTERNOVERLAY	0x0001
> 
> I would like to see this patch split in two: first adapt try/s_fmt in
> v4l2-ioctl.c to set priv to 0 and all drivers to drop the zeroing of priv
> (since that's now no longer necessary to do explicitly).

How about g_fmt ?

> The next patch would introduce support for the extended pix format.
> 
> That way all the driver 'priv = 0' patches would be in their own trivial
> patch.

-- 
Regards,

Laurent Pinchart