| Message ID | 1403567669-18539-4-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(-) > > diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile > index 1d27f0a..494da94 100644 > --- a/Documentation/DocBook/media/Makefile > +++ b/Documentation/DocBook/media/Makefile > @@ -174,7 +174,7 @@ FILENAME = \ > DOCUMENTED = \ > -e "s/\(enum *\)v4l2_mpeg_cx2341x_video_\([a-z]*_spatial_filter_type\)/\1<link linkend=\"\2\">v4l2_mpeg_cx2341x_video_\2<\/link>/g" \ > -e "s/\(\(enum\|struct\) *\)\(v4l2_[a-zA-Z0-9_]*\)/\1<link linkend=\"\3\">\3<\/link>/g" \ > - -e "s/\(V4L2_PIX_FMT_[A-Z0-9_]\+\) /<link linkend=\"\1\">\1<\/link> /g" \ > + -e "s/\(V4L2_PIX_FMT_[A-Z0-9_]\+\)\(\s\+v4l2_fourcc\)/<link linkend=\"\1\">\1<\/link>\2/g" \ > -e ":a;s/\(linkend=\".*\)_\(.*\">\)/\1-\2/;ta" \ > -e "s/v4l2\-mpeg\-vbi\-ITV0/v4l2-mpeg-vbi-itv0-1/g" > > diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml > index 91dcbc8..8c56cacd 100644 > --- a/Documentation/DocBook/media/v4l/pixfmt.xml > +++ b/Documentation/DocBook/media/v4l/pixfmt.xml > @@ -112,9 +112,28 @@ see <xref linkend="colorspaces" />.</entry> > <row> > <entry>__u32</entry> > <entry><structfield>priv</structfield></entry> > - <entry>Reserved for custom (driver defined) additional > -information about formats. When not used drivers and applications must > -set this field to zero.</entry> > + <entry><para>This field indicates whether the remaining fields of the > +<structname>v4l2_pix_format</structname> structure, also called the extended > +fields, are valid. When set to <constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, it > +indicates that the extended fields have been correctly initialized. When set to > +any other value it indicates that the extended fields contain undefined values. > +</para> > +<para>Applications that wish to use the pixel format extended fields must first > +ensure that the feature is supported by querying the device for the > +<link linkend="querycap"><constant>V4L2_CAP_EXT_PIX_FORMAT</constant></link> > +capability. If the capability isn't set the pixel format extended fields are not > +supported and using the extended fields will lead to undefined results.</para> > +<para>To use the extended fields, applications must set the > +<structfield>priv</structfield> field to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, initialize all the extended fields > +and zero the unused bytes of the <structname>v4l2_format</structname> > +<structfield>raw_data</structfield> field.</para> > +<para>When the <structfield>priv</structfield> field isn't set to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> drivers must act as if all the > +extended fields were set to zero. On return drivers must set the > +<structfield>priv</structfield> field to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> and all the extended field to s/field/fields/ > +applicable values.</para></entry> > </row> > </tbody> > </tgroup> > diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml > index b445161..d0a48be 100644 > --- a/Documentation/DocBook/media/v4l/v4l2.xml > +++ b/Documentation/DocBook/media/v4l/v4l2.xml > @@ -152,6 +152,14 @@ structs, ioctls) must be noted in more detail in the history chapter > applications. --> > > <revision> > + <revnumber>3.16</revnumber> > + <date>2014-05-27</date> > + <authorinitials>lp</authorinitials> > + <revremark>Extended &v4l2-pix-format;. > + </revremark> > + </revision> > + > + <revision> > <revnumber>3.15</revnumber> > <date>2014-02-03</date> > <authorinitials>hv, ap</authorinitials> > diff --git a/Documentation/DocBook/media/v4l/vidioc-querycap.xml b/Documentation/DocBook/media/v4l/vidioc-querycap.xml > index 370d49d..d0c5e60 100644 > --- a/Documentation/DocBook/media/v4l/vidioc-querycap.xml > +++ b/Documentation/DocBook/media/v4l/vidioc-querycap.xml > @@ -302,6 +302,12 @@ modulator programming see > <link linkend="sdr">SDR Capture</link> interface.</entry> > </row> > <row> > + <entry><constant>V4L2_CAP_EXT_PIX_FORMAT</constant></entry> > + <entry>0x00200000</entry> > + <entry>The device supports the &v4l2-pix-format; extended > +fields.</entry> > + </row> > + <row> > <entry><constant>V4L2_CAP_READWRITE</constant></entry> > <entry>0x01000000</entry> > <entry>The device supports the <link > diff --git a/drivers/media/parport/bw-qcam.c b/drivers/media/parport/bw-qcam.c > index 416507a..67fff38 100644 > --- a/drivers/media/parport/bw-qcam.c > +++ b/drivers/media/parport/bw-qcam.c > @@ -759,7 +759,6 @@ static int qcam_g_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f > pix->sizeimage = pix->width * pix->height; > /* Just a guess */ > pix->colorspace = V4L2_COLORSPACE_SRGB; > - pix->priv = 0; > return 0; > } > > @@ -785,7 +784,6 @@ static int qcam_try_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format > pix->sizeimage = pix->width * pix->height; > /* Just a guess */ > pix->colorspace = V4L2_COLORSPACE_SRGB; > - pix->priv = 0; > return 0; > } > > diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c > index fefb2cd..6f2b590 100644 > --- a/drivers/media/pci/cx18/cx18-ioctl.c > +++ b/drivers/media/pci/cx18/cx18-ioctl.c > @@ -156,7 +156,6 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh, > pixfmt->height = cx->cxhdl.height; > pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; > pixfmt->field = V4L2_FIELD_INTERLACED; > - pixfmt->priv = 0; > if (id->type == CX18_ENC_STREAM_TYPE_YUV) { > pixfmt->pixelformat = s->pixelformat; > pixfmt->sizeimage = s->vb_bytes_per_frame; > diff --git a/drivers/media/pci/cx25821/cx25821-video.c b/drivers/media/pci/cx25821/cx25821-video.c > index d270819..c7ae087 100644 > --- a/drivers/media/pci/cx25821/cx25821-video.c > +++ b/drivers/media/pci/cx25821/cx25821-video.c > @@ -576,7 +576,6 @@ static int cx25821_vidioc_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = (chan->width * chan->fmt->depth) >> 3; > f->fmt.pix.sizeimage = chan->height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -615,7 +614,6 @@ static int cx25821_vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -867,7 +865,6 @@ static int cx25821_vidioc_try_fmt_vid_out(struct file *file, void *priv, > f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > return 0; > } > > diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.c b/drivers/media/pci/ivtv/ivtv-ioctl.c > index b3667a0..3e0cb77 100644 > --- a/drivers/media/pci/ivtv/ivtv-ioctl.c > +++ b/drivers/media/pci/ivtv/ivtv-ioctl.c > @@ -351,7 +351,6 @@ static int ivtv_g_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f > pixfmt->height = itv->cxhdl.height; > pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; > pixfmt->field = V4L2_FIELD_INTERLACED; > - pixfmt->priv = 0; > if (id->type == IVTV_ENC_STREAM_TYPE_YUV) { > pixfmt->pixelformat = V4L2_PIX_FMT_HM12; > /* YUV size is (Y=(h*720) + UV=(h*(720/2))) */ > @@ -418,7 +417,6 @@ static int ivtv_g_fmt_vid_out(struct file *file, void *fh, struct v4l2_format *f > pixfmt->height = itv->main_rect.height; > pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; > pixfmt->field = V4L2_FIELD_INTERLACED; > - pixfmt->priv = 0; > if (id->type == IVTV_DEC_STREAM_TYPE_YUV) { > switch (itv->yuv_info.lace_mode & IVTV_YUV_MODE_MASK) { > case IVTV_YUV_MODE_INTERLACED: > @@ -1384,7 +1382,6 @@ static int ivtv_g_fbuf(struct file *file, void *fh, struct v4l2_framebuffer *fb) > fb->fmt.bytesperline = fb->fmt.width; > fb->fmt.colorspace = V4L2_COLORSPACE_SMPTE170M; > fb->fmt.field = V4L2_FIELD_INTERLACED; > - fb->fmt.priv = 0; > if (fb->fmt.pixelformat != V4L2_PIX_FMT_PAL8) > fb->fmt.bytesperline *= 2; > if (fb->fmt.pixelformat == V4L2_PIX_FMT_RGB32 || > diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c > index 54d5c82..4e7fba0 100644 > --- a/drivers/media/pci/meye/meye.c > +++ b/drivers/media/pci/meye/meye.c > @@ -1166,7 +1166,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *fh, > f->fmt.pix.sizeimage = f->fmt.pix.height * > f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = 0; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -1232,7 +1231,6 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *fh, > f->fmt.pix.sizeimage = f->fmt.pix.height * > f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = 0; > - f->fmt.pix.priv = 0; > > return 0; > } > diff --git a/drivers/media/pci/saa7134/saa7134-empress.c b/drivers/media/pci/saa7134/saa7134-empress.c > index e65c760..ab020fa 100644 > --- a/drivers/media/pci/saa7134/saa7134-empress.c > +++ b/drivers/media/pci/saa7134/saa7134-empress.c > @@ -130,7 +130,6 @@ static int empress_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; > f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; > f->fmt.pix.bytesperline = 0; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -148,7 +147,6 @@ static int empress_s_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; > f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; > f->fmt.pix.bytesperline = 0; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -166,7 +164,6 @@ static int empress_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; > f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; > f->fmt.pix.bytesperline = 0; > - f->fmt.pix.priv = 0; > > return 0; > } > diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c > index d375999..0cfa2ca 100644 > --- a/drivers/media/pci/saa7134/saa7134-video.c > +++ b/drivers/media/pci/saa7134/saa7134-video.c > @@ -1235,7 +1235,6 @@ static int saa7134_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.sizeimage = > f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > return 0; > } > > @@ -1315,7 +1314,6 @@ static int saa7134_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.sizeimage = > f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > > return 0; > } > diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c > index d2abd3b..e264460 100644 > --- a/drivers/media/pci/sta2x11/sta2x11_vip.c > +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c > @@ -640,7 +640,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = f->fmt.pix.width * 2; > f->fmt.pix.sizeimage = f->fmt.pix.width * 2 * f->fmt.pix.height; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > return 0; > } > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > index b178379..5488660 100644 > --- a/drivers/media/platform/coda.c > +++ b/drivers/media/platform/coda.c > @@ -613,8 +613,6 @@ static int coda_try_fmt(struct coda_ctx *ctx, struct coda_codec *codec, > BUG(); > } > > - f->fmt.pix.priv = 0; > - > return 0; > } > > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c > index 5bb085b..042607a 100644 > --- a/drivers/media/platform/davinci/vpif_display.c > +++ b/drivers/media/platform/davinci/vpif_display.c > @@ -648,7 +648,6 @@ static int vpif_try_fmt_vid_out(struct file *file, void *priv, > pixfmt->width = common->fmt.fmt.pix.width; > pixfmt->height = common->fmt.fmt.pix.height; > pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height * 2; > - pixfmt->priv = 0; > > return 0; > } > diff --git a/drivers/media/platform/mem2mem_testdev.c b/drivers/media/platform/mem2mem_testdev.c > index 0714070..c1b03cf 100644 > --- a/drivers/media/platform/mem2mem_testdev.c > +++ b/drivers/media/platform/mem2mem_testdev.c > @@ -532,7 +532,6 @@ static int vidioc_try_fmt(struct v4l2_format *f, struct m2mtest_fmt *fmt) > f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.field = V4L2_FIELD_NONE; > - f->fmt.pix.priv = 0; > > return 0; > } > diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c > index 9a726ea..2d177fa 100644 > --- a/drivers/media/platform/omap/omap_vout.c > +++ b/drivers/media/platform/omap/omap_vout.c > @@ -165,7 +165,6 @@ static int omap_vout_try_format(struct v4l2_pix_format *pix) > > pix->pixelformat = omap_formats[ifmt].pixelformat; > pix->field = V4L2_FIELD_ANY; > - pix->priv = 0; > > switch (pix->pixelformat) { > case V4L2_PIX_FMT_YUYV: > @@ -1896,7 +1895,6 @@ static int __init omap_vout_setup_video_data(struct omap_vout_device *vout) > pix->field = V4L2_FIELD_ANY; > pix->bytesperline = pix->width * 2; > pix->sizeimage = pix->bytesperline * pix->height; > - pix->priv = 0; > pix->colorspace = V4L2_COLORSPACE_JPEG; > > vout->bpp = RGB565_BPP; > diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c > index 744e43b..8dc279d 100644 > --- a/drivers/media/platform/sh_veu.c > +++ b/drivers/media/platform/sh_veu.c > @@ -425,7 +425,6 @@ static int sh_veu_g_fmt(struct sh_veu_file *veu_file, struct v4l2_format *f) > pix->bytesperline = vfmt->bytesperline; > pix->sizeimage = vfmt->bytesperline * pix->height * > vfmt->fmt->depth / vfmt->fmt->ydepth; > - pix->priv = 0; > dev_dbg(veu->dev, "%s(): type: %d, size %u @ %ux%u, fmt %x\n", __func__, > f->type, pix->sizeimage, pix->width, pix->height, pix->pixelformat); > > @@ -473,7 +472,6 @@ static int sh_veu_try_fmt(struct v4l2_format *f, const struct sh_veu_format *fmt > > pix->pixelformat = fmt->fourcc; > pix->colorspace = sh_veu_4cc2cspace(pix->pixelformat); > - pix->priv = 0; > > pr_debug("%s(): type: %d, size %u\n", __func__, f->type, pix->sizeimage); > > diff --git a/drivers/media/platform/vino.c b/drivers/media/platform/vino.c > index 470d353..91d44ea1 100644 > --- a/drivers/media/platform/vino.c > +++ b/drivers/media/platform/vino.c > @@ -3147,7 +3147,6 @@ static int vino_try_fmt_vid_cap(struct file *file, void *__fh, > pf->colorspace = > vino_data_formats[tempvcs.data_format].colorspace; > > - pf->priv = 0; > return 0; > } > > @@ -3175,8 +3174,6 @@ static int vino_g_fmt_vid_cap(struct file *file, void *__fh, > pf->colorspace = > vino_data_formats[vcs->data_format].colorspace; > > - pf->priv = 0; > - > spin_unlock_irqrestore(&vino_drvdata->input_lock, flags); > return 0; > } > @@ -3219,8 +3216,6 @@ static int vino_s_fmt_vid_cap(struct file *file, void *__fh, > pf->colorspace = > vino_data_formats[vcs->data_format].colorspace; > > - pf->priv = 0; > - > spin_unlock_irqrestore(&vino_drvdata->input_lock, flags); > return 0; > } > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c > index d00bf3d..23480e7 100644 > --- a/drivers/media/platform/vivi.c > +++ b/drivers/media/platform/vivi.c > @@ -1014,7 +1014,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > else > f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > - f->fmt.pix.priv = 0; > return 0; > } > > diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c > index 30a0c69..7982440 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-417.c > +++ b/drivers/media/usb/cx231xx/cx231xx-417.c > @@ -1563,7 +1563,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.width = dev->ts1.width; > f->fmt.pix.height = dev->ts1.height; > f->fmt.pix.field = V4L2_FIELD_INTERLACED; > - f->fmt.pix.priv = 0; > dprintk(1, "VIDIOC_G_FMT: w: %d, h: %d\n", > dev->ts1.width, dev->ts1.height); > dprintk(3, "exit vidioc_g_fmt_vid_cap()\n"); > @@ -1582,7 +1581,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.sizeimage = mpeglines * mpeglinesize; > f->fmt.pix.field = V4L2_FIELD_INTERLACED; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > - f->fmt.pix.priv = 0; > dprintk(1, "VIDIOC_TRY_FMT: w: %d, h: %d\n", > dev->ts1.width, dev->ts1.height); > dprintk(3, "exit vidioc_try_fmt_vid_cap()\n"); > diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c > index 1f87513..8f489fa 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-video.c > +++ b/drivers/media/usb/cx231xx/cx231xx-video.c > @@ -886,7 +886,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > > f->fmt.pix.field = V4L2_FIELD_INTERLACED; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -931,7 +930,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * height; > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > f->fmt.pix.field = V4L2_FIELD_INTERLACED; > - f->fmt.pix.priv = 0; > > return 0; > } > diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c > index f3a7ace..1b3d7bf 100644 > --- a/drivers/media/usb/gspca/gspca.c > +++ b/drivers/media/usb/gspca/gspca.c > @@ -1102,8 +1102,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, > struct gspca_dev *gspca_dev = video_drvdata(file); > > fmt->fmt.pix = gspca_dev->pixfmt; > - /* some drivers use priv internally, zero it before giving it to > - userspace */ > + /* some drivers use priv internally, zero it before giving it back to > + the core */ > fmt->fmt.pix.priv = 0; > return 0; > } > @@ -1139,8 +1139,8 @@ static int try_fmt_vid_cap(struct gspca_dev *gspca_dev, > fmt->fmt.pix.height = h; > gspca_dev->sd_desc->try_fmt(gspca_dev, fmt); > } > - /* some drivers use priv internally, zero it before giving it to > - userspace */ > + /* some drivers use priv internally, zero it before giving it back to > + the core */ > fmt->fmt.pix.priv = 0; > return mode; /* used when s_fmt */ > } > diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c > index 0500c417..cf9a21e 100644 > --- a/drivers/media/usb/hdpvr/hdpvr-video.c > +++ b/drivers/media/usb/hdpvr/hdpvr-video.c > @@ -1022,7 +1022,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *_fh, > f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; > f->fmt.pix.sizeimage = dev->bulk_in_size; > f->fmt.pix.bytesperline = 0; > - f->fmt.pix.priv = 0; > if (f->fmt.pix.width == 720) { > /* SDTV formats */ > f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c > index be77482..adfa832 100644 > --- a/drivers/media/usb/stkwebcam/stk-webcam.c > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c > @@ -923,7 +923,6 @@ static int stk_vidioc_g_fmt_vid_cap(struct file *filp, > pix_format->bytesperline = 2 * pix_format->width; > pix_format->sizeimage = pix_format->bytesperline > * pix_format->height; > - pix_format->priv = 0; > return 0; > } > > @@ -967,7 +966,6 @@ static int stk_try_fmt_vid_cap(struct file *filp, > fmtd->fmt.pix.bytesperline = 2 * fmtd->fmt.pix.width; > fmtd->fmt.pix.sizeimage = fmtd->fmt.pix.bytesperline > * fmtd->fmt.pix.height; > - fmtd->fmt.pix.priv = 0; > return 0; > } > > diff --git a/drivers/media/usb/tlg2300/pd-video.c b/drivers/media/usb/tlg2300/pd-video.c > index 8df668d..8cd7f02 100644 > --- a/drivers/media/usb/tlg2300/pd-video.c > +++ b/drivers/media/usb/tlg2300/pd-video.c > @@ -1321,7 +1321,6 @@ static void init_video_context(struct running_context *context) > .bytesperline = 720 * 2, > .sizeimage = 720 * 576 * 2, > .colorspace = V4L2_COLORSPACE_SMPTE170M, > - .priv = 0 > }; > } > > diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c > index e6b3d5d..1626e9b 100644 > --- a/drivers/media/usb/tm6000/tm6000-video.c > +++ b/drivers/media/usb/tm6000/tm6000-video.c > @@ -918,7 +918,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, > (f->fmt.pix.width * fh->fmt->depth) >> 3; > f->fmt.pix.sizeimage = > f->fmt.pix.height * f->fmt.pix.bytesperline; > - f->fmt.pix.priv = 0; > > return 0; > } > @@ -959,7 +958,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.width &= ~0x01; > > f->fmt.pix.field = field; > - f->fmt.pix.priv = 0; > > f->fmt.pix.bytesperline = > (f->fmt.pix.width * fmt->depth) >> 3; > diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c > index 74d56df..0f63954 100644 > --- a/drivers/media/usb/zr364xx/zr364xx.c > +++ b/drivers/media/usb/zr364xx/zr364xx.c > @@ -806,7 +806,6 @@ static int zr364xx_vidioc_try_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = f->fmt.pix.width * 2; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; > - f->fmt.pix.priv = 0; > DBG("%s: V4L2_PIX_FMT_%s (%d) ok!\n", __func__, > decode_fourcc(f->fmt.pix.pixelformat, pixelformat_name), > f->fmt.pix.field); > @@ -829,7 +828,6 @@ static int zr364xx_vidioc_g_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = f->fmt.pix.width * 2; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; > - f->fmt.pix.priv = 0; > return 0; > } > > @@ -866,7 +864,6 @@ static int zr364xx_vidioc_s_fmt_vid_cap(struct file *file, void *priv, > f->fmt.pix.bytesperline = f->fmt.pix.width * 2; > f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; > f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; > - f->fmt.pix.priv = 0; > cam->vb_vidq.field = f->fmt.pix.field; > > if (f->fmt.pix.width == 160 && f->fmt.pix.height == 120) > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 7e2411c..cca6c2f 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -540,7 +540,16 @@ struct v4l2_framebuffer32 { > __u32 capability; > __u32 flags; > compat_caddr_t base; > - struct v4l2_pix_format fmt; > + struct { > + __u32 width; > + __u32 height; > + __u32 pixelformat; > + __u32 field; > + __u32 bytesperline; > + __u32 sizeimage; > + __u32 colorspace; > + __u32 priv; > + } fmt; > }; > > static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_framebuffer32 __user *up) > @@ -550,10 +559,10 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_frame > if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_framebuffer32)) || > get_user(tmp, &up->base) || > get_user(kp->capability, &up->capability) || > - get_user(kp->flags, &up->flags)) > + get_user(kp->flags, &up->flags) || > + copy_from_user(&kp->fmt, &up->fmt, sizeof(up->fmt))) > return -EFAULT; > kp->base = compat_ptr(tmp); > - get_v4l2_pix_format(&kp->fmt, &up->fmt); > return 0; > } > > @@ -564,9 +573,9 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_frame > if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_framebuffer32)) || > put_user(tmp, &up->base) || > put_user(kp->capability, &up->capability) || > - put_user(kp->flags, &up->flags)) > + put_user(kp->flags, &up->flags) || > + copy_to_user(&up->fmt, &kp->fmt, sizeof(up->fmt))) > return -EFAULT; > - put_v4l2_pix_format(&kp->fmt, &up->fmt); > return 0; > } > > 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 > @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) > return -EINVAL; > } > > +static void v4l_sanitize_format(struct v4l2_format *fmt) > +{ > + unsigned int offset; > + > + /* > + * The v4l2_pix_format structure has been extended with fields that were > + * not previously required to be set to zero by applications. The priv > + * field, when set to a magic value, indicates the the extended fields > + * are valid. Otherwise they will contain undefined values. To simplify > + * the API towards drivers zero the extended fields and set the priv > + * field to the magic value when the extended pixel format structure > + * isn't used by applications. > + */ > + > + if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE && > + fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + return; > + > + if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC) > + return; > + > + fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > + > + offset = offsetof(struct v4l2_pix_format, priv) > + + sizeof(fmt->fmt.pix.priv); > + memset(((void *)&fmt->fmt.pix) + offset, 0, > + sizeof(fmt->fmt.pix) - offset); > +} > + > static int v4l_querycap(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > struct v4l2_capability *cap = (struct v4l2_capability *)arg; > + int ret; > > cap->version = LINUX_VERSION_CODE; > - return ops->vidioc_querycap(file, fh, cap); > + > + ret = ops->vidioc_querycap(file, fh, cap); > + > + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; > + > + return ret; > } > > static int v4l_s_input(const struct v4l2_ioctl_ops *ops, > @@ -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. > > 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. > + 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. > + 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. > + > + return ret; > } Shouldn't v4l_sanitize_format also be called for s/try_fmt? It makes much more sense there. > > 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. > + > /* > * 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. > + } 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). 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, 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
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.
Hi Laurent, Something that just caught my eye: 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> > --- > > diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml > index 91dcbc8..8c56cacd 100644 > --- a/Documentation/DocBook/media/v4l/pixfmt.xml > +++ b/Documentation/DocBook/media/v4l/pixfmt.xml > @@ -112,9 +112,28 @@ see <xref linkend="colorspaces" />.</entry> > <row> > <entry>__u32</entry> > <entry><structfield>priv</structfield></entry> > - <entry>Reserved for custom (driver defined) additional > -information about formats. When not used drivers and applications must > -set this field to zero.</entry> > + <entry><para>This field indicates whether the remaining fields of the > +<structname>v4l2_pix_format</structname> structure, also called the extended > +fields, are valid. When set to <constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, it > +indicates that the extended fields have been correctly initialized. When set to > +any other value it indicates that the extended fields contain undefined values. > +</para> > +<para>Applications that wish to use the pixel format extended fields must first > +ensure that the feature is supported by querying the device for the > +<link linkend="querycap"><constant>V4L2_CAP_EXT_PIX_FORMAT</constant></link> > +capability. If the capability isn't set the pixel format extended fields are not > +supported and using the extended fields will lead to undefined results.</para> > +<para>To use the extended fields, applications must set the > +<structfield>priv</structfield> field to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, initialize all the extended fields > +and zero the unused bytes of the <structname>v4l2_format</structname> > +<structfield>raw_data</structfield> field.</para> Easy to write, much harder to implement. You would end up with something like: memset(&fmt.fmt.pix.flags + sizeof(fmt.fmt.pix.flags), 0, sizeof(fmt.fmt.raw_data) - sizeof(fmt.fmt.pix)); Not user-friendly and error-prone. I would suggest adding a reserved array to pix_format instead, of at least size (10 + 2 * 7) / 4 = 6 __u32. So: __u32 reserved[6]. Better would be to go with 10 + 17 = 27 elements (same as the number of reserved elements in v4l2_pix_format_mplane and one struct v4l2_plane_pix_format). That will allow you to just say that the app should zero the reserved array. > +<para>When the <structfield>priv</structfield> field isn't set to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> drivers must act as if all the > +extended fields were set to zero. On return drivers must set the > +<structfield>priv</structfield> field to > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> and all the extended field to > +applicable values.</para></entry> > </row> > </tbody> > </tgroup> 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
And another thing that I found while implementing this in v4l2-ctl: 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> > 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 > @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) > return -EINVAL; > } > > +static void v4l_sanitize_format(struct v4l2_format *fmt) > +{ > + unsigned int offset; > + > + /* > + * The v4l2_pix_format structure has been extended with fields that were > + * not previously required to be set to zero by applications. The priv > + * field, when set to a magic value, indicates the the extended fields > + * are valid. Otherwise they will contain undefined values. To simplify > + * the API towards drivers zero the extended fields and set the priv > + * field to the magic value when the extended pixel format structure > + * isn't used by applications. > + */ > + > + if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE && > + fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > + return; > + > + if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC) > + return; > + > + fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > + > + offset = offsetof(struct v4l2_pix_format, priv) > + + sizeof(fmt->fmt.pix.priv); > + memset(((void *)&fmt->fmt.pix) + offset, 0, > + sizeof(fmt->fmt.pix) - offset); > +} > + > static int v4l_querycap(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > struct v4l2_capability *cap = (struct v4l2_capability *)arg; > + int ret; > > cap->version = LINUX_VERSION_CODE; > - return ops->vidioc_querycap(file, fh, cap); > + > + ret = ops->vidioc_querycap(file, fh, cap); > + > + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; It should be ORed to cap->device_caps as well. > + > + return ret; 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
On 07/17/2014 11:22 PM, Hans Verkuil wrote: > And another thing that I found while implementing this in v4l2-ctl: > > 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> > >> 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 >> @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) >> return -EINVAL; >> } >> >> +static void v4l_sanitize_format(struct v4l2_format *fmt) >> +{ >> + unsigned int offset; >> + >> + /* >> + * The v4l2_pix_format structure has been extended with fields that were >> + * not previously required to be set to zero by applications. The priv >> + * field, when set to a magic value, indicates the the extended fields >> + * are valid. Otherwise they will contain undefined values. To simplify >> + * the API towards drivers zero the extended fields and set the priv >> + * field to the magic value when the extended pixel format structure >> + * isn't used by applications. >> + */ >> + >> + if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE && >> + fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) >> + return; >> + >> + if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC) >> + return; >> + >> + fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; >> + >> + offset = offsetof(struct v4l2_pix_format, priv) >> + + sizeof(fmt->fmt.pix.priv); >> + memset(((void *)&fmt->fmt.pix) + offset, 0, >> + sizeof(fmt->fmt.pix) - offset); >> +} >> + >> static int v4l_querycap(const struct v4l2_ioctl_ops *ops, >> struct file *file, void *fh, void *arg) >> { >> struct v4l2_capability *cap = (struct v4l2_capability *)arg; >> + int ret; >> >> cap->version = LINUX_VERSION_CODE; >> - return ops->vidioc_querycap(file, fh, cap); >> + >> + ret = ops->vidioc_querycap(file, fh, cap); >> + >> + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; > > It should be ORed to cap->device_caps as well. But only if cap->capabilities sets V4L2_CAP_DEVICE_CAPS. Should we unconditionally add this flag or only if CAP_VIDEO_CAPTURE or CAP_VIDEO_OUTPUT is set? So we could do this: if (cap->capabilities & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; if ((cap->capabilities & V4L2_CAP_DEVICE_CAPS) && (cap->device_caps & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) cap->device_caps |= V4L2_CAP_EXT_PIX_FORMAT; I can argue either direction: on the one hand ext_pix_format handling is part of the v4l2 core, so it is valid for all that use it, on the other hand it makes no sense for a non-video device. My preference would be to set it only in combination with video capture/output since it just looks peculiar otherwise. 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
Hi Hans, On Friday 18 July 2014 07:10:48 Hans Verkuil wrote: > On 07/17/2014 11:22 PM, Hans Verkuil wrote: > > And another thing that I found while implementing this in v4l2-ctl: > > > > 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> > >> > >> 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 > >> @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum > >> v4l2_buf_type type) [snip] > >> static int v4l_querycap(const struct v4l2_ioctl_ops *ops, > >> struct file *file, void *fh, void *arg) > >> { > >> struct v4l2_capability *cap = (struct v4l2_capability *)arg; > >> + int ret; > >> > >> cap->version = LINUX_VERSION_CODE; > >> > >> - return ops->vidioc_querycap(file, fh, cap); > >> + > >> + ret = ops->vidioc_querycap(file, fh, cap); > >> + > >> + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; > > > > It should be ORed to cap->device_caps as well. > > But only if cap->capabilities sets V4L2_CAP_DEVICE_CAPS. > > Should we unconditionally add this flag or only if CAP_VIDEO_CAPTURE or > CAP_VIDEO_OUTPUT is set? > > So we could do this: > > if (cap->capabilities & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) > cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; > if ((cap->capabilities & V4L2_CAP_DEVICE_CAPS) && > (cap->device_caps & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) > cap->device_caps |= V4L2_CAP_EXT_PIX_FORMAT; > > > I can argue either direction: on the one hand ext_pix_format handling is > part of the v4l2 core, so it is valid for all that use it, on the other > hand it makes no sense for a non-video device. > > My preference would be to set it only in combination with video > capture/output since it just looks peculiar otherwise. I have mixed feelings here. As the flag indicates whether a particular feature is supported by the V4L2 API, wouldn't it make more sense to set it only in the capabilities field, and unconditionally ? Does setting it conditionally bring any benefit to kernel space or userspace ? Same question for device_caps, I don't think it would help applications in a any way (but please feel free to point me to use cases I might have missed).
On 07/18/2014 02:27 PM, Laurent Pinchart wrote: > Hi Hans, > > On Friday 18 July 2014 07:10:48 Hans Verkuil wrote: >> On 07/17/2014 11:22 PM, Hans Verkuil wrote: >>> And another thing that I found while implementing this in v4l2-ctl: >>> >>> 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> >>>> >>>> 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 >>>> @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum >>>> v4l2_buf_type type) > > [snip] > >>>> static int v4l_querycap(const struct v4l2_ioctl_ops *ops, >>>> struct file *file, void *fh, void *arg) >>>> { >>>> struct v4l2_capability *cap = (struct v4l2_capability *)arg; >>>> + int ret; >>>> >>>> cap->version = LINUX_VERSION_CODE; >>>> >>>> - return ops->vidioc_querycap(file, fh, cap); >>>> + >>>> + ret = ops->vidioc_querycap(file, fh, cap); >>>> + >>>> + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; >>> >>> It should be ORed to cap->device_caps as well. >> >> But only if cap->capabilities sets V4L2_CAP_DEVICE_CAPS. >> >> Should we unconditionally add this flag or only if CAP_VIDEO_CAPTURE or >> CAP_VIDEO_OUTPUT is set? >> >> So we could do this: >> >> if (cap->capabilities & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) >> cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; >> if ((cap->capabilities & V4L2_CAP_DEVICE_CAPS) && >> (cap->device_caps & (V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_VIDEO_OUTPUT)) >> cap->device_caps |= V4L2_CAP_EXT_PIX_FORMAT; >> >> >> I can argue either direction: on the one hand ext_pix_format handling is >> part of the v4l2 core, so it is valid for all that use it, on the other >> hand it makes no sense for a non-video device. >> >> My preference would be to set it only in combination with video >> capture/output since it just looks peculiar otherwise. > > I have mixed feelings here. As the flag indicates whether a particular feature > is supported by the V4L2 API, wouldn't it make more sense to set it only in > the capabilities field, and unconditionally ? Does setting it conditionally > bring any benefit to kernel space or userspace ? Same question for > device_caps, I don't think it would help applications in a any way (but please > feel free to point me to use cases I might have missed). There are two things: should it be set for device_caps as well: absolutely. The device_caps field should contain all caps relevant for that device and EXT_PIX_FORMAT definitely belongs there. Several of the apps in v4l-utils do something like this: __u32 caps = qcap->capabilities; if (caps & V4L2_CAP_DEVICE_CAPS) caps = qcap->device_caps; And after that I just use caps. It makes life very easy. Whether V4L2_CAP_EXT_PIX_FORMAT should be set unconditionally: I've got mixed feelings as well. So let's leave it as is, I'm OK with that. Just add it to device_caps as well... 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
Hi Hans, On Thursday 17 July 2014 23:04:09 Hans Verkuil wrote: > Hi Laurent, > > Something that just caught my eye: > > 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> > > --- > > > > diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml > > b/Documentation/DocBook/media/v4l/pixfmt.xml index 91dcbc8..8c56cacd > > 100644 > > --- a/Documentation/DocBook/media/v4l/pixfmt.xml > > +++ b/Documentation/DocBook/media/v4l/pixfmt.xml [snip] > > +<para>To use the extended fields, applications must set the > > +<structfield>priv</structfield> field to > > +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, initialize all the extended > > fields > > +and zero the unused bytes of the <structname>v4l2_format</structname> > > +<structfield>raw_data</structfield> field.</para> > > Easy to write, much harder to implement. You would end up with something > like: > > memset(&fmt.fmt.pix.flags + sizeof(fmt.fmt.pix.flags), 0, > sizeof(fmt.fmt.raw_data) - sizeof(fmt.fmt.pix)); > > Not user-friendly and error-prone. Or, rather, memset the whole v4l2_format structure to 0 and then fill it. > I would suggest adding a reserved array to pix_format instead, of at least > size (10 + 2 * 7) / 4 = 6 __u32. So: __u32 reserved[6]. Better would be to > go with 10 + 17 = 27 elements (same as the number of reserved elements in > v4l2_pix_format_mplane and one struct v4l2_plane_pix_format). Maybe it's a bit late, but I'm not sure to see where you got those values. If we want to use a reserved array, it would make more sense to make it cover the whole raw_data array, otherwise future extensions could require an API change. On the other hand this would result in the v4l2_pix_format structure suddenly consuming 200 bytes instead of 36 today. That wouldn't be good when allocated on the stack. > That will allow you to just say that the app should zero the reserved array.
On 07/21/2014 10:56 PM, Laurent Pinchart wrote: > Hi Hans, > > On Thursday 17 July 2014 23:04:09 Hans Verkuil wrote: >> Hi Laurent, >> >> Something that just caught my eye: >> >> 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> >>> --- >>> >>> diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml >>> b/Documentation/DocBook/media/v4l/pixfmt.xml index 91dcbc8..8c56cacd >>> 100644 >>> --- a/Documentation/DocBook/media/v4l/pixfmt.xml >>> +++ b/Documentation/DocBook/media/v4l/pixfmt.xml > > [snip] > >>> +<para>To use the extended fields, applications must set the >>> +<structfield>priv</structfield> field to >>> +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, initialize all the extended >>> fields >>> +and zero the unused bytes of the <structname>v4l2_format</structname> >>> +<structfield>raw_data</structfield> field.</para> >> >> Easy to write, much harder to implement. You would end up with something >> like: >> >> memset(&fmt.fmt.pix.flags + sizeof(fmt.fmt.pix.flags), 0, >> sizeof(fmt.fmt.raw_data) - sizeof(fmt.fmt.pix)); >> >> Not user-friendly and error-prone. > > Or, rather, memset the whole v4l2_format structure to 0 and then fill it. > >> I would suggest adding a reserved array to pix_format instead, of at least >> size (10 + 2 * 7) / 4 = 6 __u32. So: __u32 reserved[6]. Better would be to >> go with 10 + 17 = 27 elements (same as the number of reserved elements in >> v4l2_pix_format_mplane and one struct v4l2_plane_pix_format). > > Maybe it's a bit late, but I'm not sure to see where you got those values. I'm making the assumption that anything we might want to add to pix_format, we also want to add to pix_format_mplane. And the latter has 10 reserved bytes in the pix_format_mplane struct and another 7 __u16's in each plane_pix_format. So for a single plane format that means that there are 10 + 2 * 7 bytes reserved space available in the multiplanar case (for the main struct + one plane struct). We could add a __u8 reserved[24] to pix_format. Then the amount of reserved fields in pix_format is identical to that in pix_format_mplane. That makes it easy to keep in sync. The alignment looks to be OK too (no holes in the struct). (BTW, when I wrote '10 + 17' in my earlier email I meant '10 + 14'. Sorry about that confusion.) But perhaps I am just over-analyzing and the real problem is the text in the spec 'initialize all the extended fields and zero the unused bytes of the v4l2_format raw_data field.'. It might be better to add something along the lines of: "It is good practice to either call VIDIOC_G_FMT first, and then modify any fields, or to memset to 0 the whole v4l2_format structure before filling in fields." > If we want to use a reserved array, it would make more sense to make it cover the > whole raw_data array, otherwise future extensions could require an API change. > On the other hand this would result in the v4l2_pix_format structure suddenly > consuming 200 bytes instead of 36 today. That wouldn't be good when allocated > on the stack. I think the amount of available space in the multiplanar structs puts an upper limit to what can be done with pix_format anyway. So reserving more space seems unnecessary. It's not as if we'll see a huge number of new fields appearing. I know of one flag that might be needed to signal alternate quantization ranges to enhance the colorspace information, but that's all I know about. Regards, Hans > >> That will allow you to just say that the app should zero the reserved array. > -- 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
diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile index 1d27f0a..494da94 100644 --- a/Documentation/DocBook/media/Makefile +++ b/Documentation/DocBook/media/Makefile @@ -174,7 +174,7 @@ FILENAME = \ DOCUMENTED = \ -e "s/\(enum *\)v4l2_mpeg_cx2341x_video_\([a-z]*_spatial_filter_type\)/\1<link linkend=\"\2\">v4l2_mpeg_cx2341x_video_\2<\/link>/g" \ -e "s/\(\(enum\|struct\) *\)\(v4l2_[a-zA-Z0-9_]*\)/\1<link linkend=\"\3\">\3<\/link>/g" \ - -e "s/\(V4L2_PIX_FMT_[A-Z0-9_]\+\) /<link linkend=\"\1\">\1<\/link> /g" \ + -e "s/\(V4L2_PIX_FMT_[A-Z0-9_]\+\)\(\s\+v4l2_fourcc\)/<link linkend=\"\1\">\1<\/link>\2/g" \ -e ":a;s/\(linkend=\".*\)_\(.*\">\)/\1-\2/;ta" \ -e "s/v4l2\-mpeg\-vbi\-ITV0/v4l2-mpeg-vbi-itv0-1/g" diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml index 91dcbc8..8c56cacd 100644 --- a/Documentation/DocBook/media/v4l/pixfmt.xml +++ b/Documentation/DocBook/media/v4l/pixfmt.xml @@ -112,9 +112,28 @@ see <xref linkend="colorspaces" />.</entry> <row> <entry>__u32</entry> <entry><structfield>priv</structfield></entry> - <entry>Reserved for custom (driver defined) additional -information about formats. When not used drivers and applications must -set this field to zero.</entry> + <entry><para>This field indicates whether the remaining fields of the +<structname>v4l2_pix_format</structname> structure, also called the extended +fields, are valid. When set to <constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, it +indicates that the extended fields have been correctly initialized. When set to +any other value it indicates that the extended fields contain undefined values. +</para> +<para>Applications that wish to use the pixel format extended fields must first +ensure that the feature is supported by querying the device for the +<link linkend="querycap"><constant>V4L2_CAP_EXT_PIX_FORMAT</constant></link> +capability. If the capability isn't set the pixel format extended fields are not +supported and using the extended fields will lead to undefined results.</para> +<para>To use the extended fields, applications must set the +<structfield>priv</structfield> field to +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant>, initialize all the extended fields +and zero the unused bytes of the <structname>v4l2_format</structname> +<structfield>raw_data</structfield> field.</para> +<para>When the <structfield>priv</structfield> field isn't set to +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> drivers must act as if all the +extended fields were set to zero. On return drivers must set the +<structfield>priv</structfield> field to +<constant>V4L2_PIX_FMT_PRIV_MAGIC</constant> and all the extended field to +applicable values.</para></entry> </row> </tbody> </tgroup> diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml index b445161..d0a48be 100644 --- a/Documentation/DocBook/media/v4l/v4l2.xml +++ b/Documentation/DocBook/media/v4l/v4l2.xml @@ -152,6 +152,14 @@ structs, ioctls) must be noted in more detail in the history chapter applications. --> <revision> + <revnumber>3.16</revnumber> + <date>2014-05-27</date> + <authorinitials>lp</authorinitials> + <revremark>Extended &v4l2-pix-format;. + </revremark> + </revision> + + <revision> <revnumber>3.15</revnumber> <date>2014-02-03</date> <authorinitials>hv, ap</authorinitials> diff --git a/Documentation/DocBook/media/v4l/vidioc-querycap.xml b/Documentation/DocBook/media/v4l/vidioc-querycap.xml index 370d49d..d0c5e60 100644 --- a/Documentation/DocBook/media/v4l/vidioc-querycap.xml +++ b/Documentation/DocBook/media/v4l/vidioc-querycap.xml @@ -302,6 +302,12 @@ modulator programming see <link linkend="sdr">SDR Capture</link> interface.</entry> </row> <row> + <entry><constant>V4L2_CAP_EXT_PIX_FORMAT</constant></entry> + <entry>0x00200000</entry> + <entry>The device supports the &v4l2-pix-format; extended +fields.</entry> + </row> + <row> <entry><constant>V4L2_CAP_READWRITE</constant></entry> <entry>0x01000000</entry> <entry>The device supports the <link diff --git a/drivers/media/parport/bw-qcam.c b/drivers/media/parport/bw-qcam.c index 416507a..67fff38 100644 --- a/drivers/media/parport/bw-qcam.c +++ b/drivers/media/parport/bw-qcam.c @@ -759,7 +759,6 @@ static int qcam_g_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f pix->sizeimage = pix->width * pix->height; /* Just a guess */ pix->colorspace = V4L2_COLORSPACE_SRGB; - pix->priv = 0; return 0; } @@ -785,7 +784,6 @@ static int qcam_try_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format pix->sizeimage = pix->width * pix->height; /* Just a guess */ pix->colorspace = V4L2_COLORSPACE_SRGB; - pix->priv = 0; return 0; } diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c index fefb2cd..6f2b590 100644 --- a/drivers/media/pci/cx18/cx18-ioctl.c +++ b/drivers/media/pci/cx18/cx18-ioctl.c @@ -156,7 +156,6 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh, pixfmt->height = cx->cxhdl.height; pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; pixfmt->field = V4L2_FIELD_INTERLACED; - pixfmt->priv = 0; if (id->type == CX18_ENC_STREAM_TYPE_YUV) { pixfmt->pixelformat = s->pixelformat; pixfmt->sizeimage = s->vb_bytes_per_frame; diff --git a/drivers/media/pci/cx25821/cx25821-video.c b/drivers/media/pci/cx25821/cx25821-video.c index d270819..c7ae087 100644 --- a/drivers/media/pci/cx25821/cx25821-video.c +++ b/drivers/media/pci/cx25821/cx25821-video.c @@ -576,7 +576,6 @@ static int cx25821_vidioc_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = (chan->width * chan->fmt->depth) >> 3; f->fmt.pix.sizeimage = chan->height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } @@ -615,7 +614,6 @@ static int cx25821_vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } @@ -867,7 +865,6 @@ static int cx25821_vidioc_try_fmt_vid_out(struct file *file, void *priv, f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.c b/drivers/media/pci/ivtv/ivtv-ioctl.c index b3667a0..3e0cb77 100644 --- a/drivers/media/pci/ivtv/ivtv-ioctl.c +++ b/drivers/media/pci/ivtv/ivtv-ioctl.c @@ -351,7 +351,6 @@ static int ivtv_g_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f pixfmt->height = itv->cxhdl.height; pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; pixfmt->field = V4L2_FIELD_INTERLACED; - pixfmt->priv = 0; if (id->type == IVTV_ENC_STREAM_TYPE_YUV) { pixfmt->pixelformat = V4L2_PIX_FMT_HM12; /* YUV size is (Y=(h*720) + UV=(h*(720/2))) */ @@ -418,7 +417,6 @@ static int ivtv_g_fmt_vid_out(struct file *file, void *fh, struct v4l2_format *f pixfmt->height = itv->main_rect.height; pixfmt->colorspace = V4L2_COLORSPACE_SMPTE170M; pixfmt->field = V4L2_FIELD_INTERLACED; - pixfmt->priv = 0; if (id->type == IVTV_DEC_STREAM_TYPE_YUV) { switch (itv->yuv_info.lace_mode & IVTV_YUV_MODE_MASK) { case IVTV_YUV_MODE_INTERLACED: @@ -1384,7 +1382,6 @@ static int ivtv_g_fbuf(struct file *file, void *fh, struct v4l2_framebuffer *fb) fb->fmt.bytesperline = fb->fmt.width; fb->fmt.colorspace = V4L2_COLORSPACE_SMPTE170M; fb->fmt.field = V4L2_FIELD_INTERLACED; - fb->fmt.priv = 0; if (fb->fmt.pixelformat != V4L2_PIX_FMT_PAL8) fb->fmt.bytesperline *= 2; if (fb->fmt.pixelformat == V4L2_PIX_FMT_RGB32 || diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c index 54d5c82..4e7fba0 100644 --- a/drivers/media/pci/meye/meye.c +++ b/drivers/media/pci/meye/meye.c @@ -1166,7 +1166,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *fh, f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = 0; - f->fmt.pix.priv = 0; return 0; } @@ -1232,7 +1231,6 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *fh, f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = 0; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/pci/saa7134/saa7134-empress.c b/drivers/media/pci/saa7134/saa7134-empress.c index e65c760..ab020fa 100644 --- a/drivers/media/pci/saa7134/saa7134-empress.c +++ b/drivers/media/pci/saa7134/saa7134-empress.c @@ -130,7 +130,6 @@ static int empress_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; f->fmt.pix.bytesperline = 0; - f->fmt.pix.priv = 0; return 0; } @@ -148,7 +147,6 @@ static int empress_s_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; f->fmt.pix.bytesperline = 0; - f->fmt.pix.priv = 0; return 0; } @@ -166,7 +164,6 @@ static int empress_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; f->fmt.pix.sizeimage = TS_PACKET_SIZE * dev->ts.nr_packets; f->fmt.pix.bytesperline = 0; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c index d375999..0cfa2ca 100644 --- a/drivers/media/pci/saa7134/saa7134-video.c +++ b/drivers/media/pci/saa7134/saa7134-video.c @@ -1235,7 +1235,6 @@ static int saa7134_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } @@ -1315,7 +1314,6 @@ static int saa7134_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c index d2abd3b..e264460 100644 --- a/drivers/media/pci/sta2x11/sta2x11_vip.c +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c @@ -640,7 +640,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = f->fmt.pix.width * 2; f->fmt.pix.sizeimage = f->fmt.pix.width * 2 * f->fmt.pix.height; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c index b178379..5488660 100644 --- a/drivers/media/platform/coda.c +++ b/drivers/media/platform/coda.c @@ -613,8 +613,6 @@ static int coda_try_fmt(struct coda_ctx *ctx, struct coda_codec *codec, BUG(); } - f->fmt.pix.priv = 0; - return 0; } diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c index 5bb085b..042607a 100644 --- a/drivers/media/platform/davinci/vpif_display.c +++ b/drivers/media/platform/davinci/vpif_display.c @@ -648,7 +648,6 @@ static int vpif_try_fmt_vid_out(struct file *file, void *priv, pixfmt->width = common->fmt.fmt.pix.width; pixfmt->height = common->fmt.fmt.pix.height; pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height * 2; - pixfmt->priv = 0; return 0; } diff --git a/drivers/media/platform/mem2mem_testdev.c b/drivers/media/platform/mem2mem_testdev.c index 0714070..c1b03cf 100644 --- a/drivers/media/platform/mem2mem_testdev.c +++ b/drivers/media/platform/mem2mem_testdev.c @@ -532,7 +532,6 @@ static int vidioc_try_fmt(struct v4l2_format *f, struct m2mtest_fmt *fmt) f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.field = V4L2_FIELD_NONE; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c index 9a726ea..2d177fa 100644 --- a/drivers/media/platform/omap/omap_vout.c +++ b/drivers/media/platform/omap/omap_vout.c @@ -165,7 +165,6 @@ static int omap_vout_try_format(struct v4l2_pix_format *pix) pix->pixelformat = omap_formats[ifmt].pixelformat; pix->field = V4L2_FIELD_ANY; - pix->priv = 0; switch (pix->pixelformat) { case V4L2_PIX_FMT_YUYV: @@ -1896,7 +1895,6 @@ static int __init omap_vout_setup_video_data(struct omap_vout_device *vout) pix->field = V4L2_FIELD_ANY; pix->bytesperline = pix->width * 2; pix->sizeimage = pix->bytesperline * pix->height; - pix->priv = 0; pix->colorspace = V4L2_COLORSPACE_JPEG; vout->bpp = RGB565_BPP; diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c index 744e43b..8dc279d 100644 --- a/drivers/media/platform/sh_veu.c +++ b/drivers/media/platform/sh_veu.c @@ -425,7 +425,6 @@ static int sh_veu_g_fmt(struct sh_veu_file *veu_file, struct v4l2_format *f) pix->bytesperline = vfmt->bytesperline; pix->sizeimage = vfmt->bytesperline * pix->height * vfmt->fmt->depth / vfmt->fmt->ydepth; - pix->priv = 0; dev_dbg(veu->dev, "%s(): type: %d, size %u @ %ux%u, fmt %x\n", __func__, f->type, pix->sizeimage, pix->width, pix->height, pix->pixelformat); @@ -473,7 +472,6 @@ static int sh_veu_try_fmt(struct v4l2_format *f, const struct sh_veu_format *fmt pix->pixelformat = fmt->fourcc; pix->colorspace = sh_veu_4cc2cspace(pix->pixelformat); - pix->priv = 0; pr_debug("%s(): type: %d, size %u\n", __func__, f->type, pix->sizeimage); diff --git a/drivers/media/platform/vino.c b/drivers/media/platform/vino.c index 470d353..91d44ea1 100644 --- a/drivers/media/platform/vino.c +++ b/drivers/media/platform/vino.c @@ -3147,7 +3147,6 @@ static int vino_try_fmt_vid_cap(struct file *file, void *__fh, pf->colorspace = vino_data_formats[tempvcs.data_format].colorspace; - pf->priv = 0; return 0; } @@ -3175,8 +3174,6 @@ static int vino_g_fmt_vid_cap(struct file *file, void *__fh, pf->colorspace = vino_data_formats[vcs->data_format].colorspace; - pf->priv = 0; - spin_unlock_irqrestore(&vino_drvdata->input_lock, flags); return 0; } @@ -3219,8 +3216,6 @@ static int vino_s_fmt_vid_cap(struct file *file, void *__fh, pf->colorspace = vino_data_formats[vcs->data_format].colorspace; - pf->priv = 0; - spin_unlock_irqrestore(&vino_drvdata->input_lock, flags); return 0; } diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c index d00bf3d..23480e7 100644 --- a/drivers/media/platform/vivi.c +++ b/drivers/media/platform/vivi.c @@ -1014,7 +1014,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; else f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c index 30a0c69..7982440 100644 --- a/drivers/media/usb/cx231xx/cx231xx-417.c +++ b/drivers/media/usb/cx231xx/cx231xx-417.c @@ -1563,7 +1563,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.width = dev->ts1.width; f->fmt.pix.height = dev->ts1.height; f->fmt.pix.field = V4L2_FIELD_INTERLACED; - f->fmt.pix.priv = 0; dprintk(1, "VIDIOC_G_FMT: w: %d, h: %d\n", dev->ts1.width, dev->ts1.height); dprintk(3, "exit vidioc_g_fmt_vid_cap()\n"); @@ -1582,7 +1581,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.sizeimage = mpeglines * mpeglinesize; f->fmt.pix.field = V4L2_FIELD_INTERLACED; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; - f->fmt.pix.priv = 0; dprintk(1, "VIDIOC_TRY_FMT: w: %d, h: %d\n", dev->ts1.width, dev->ts1.height); dprintk(3, "exit vidioc_try_fmt_vid_cap()\n"); diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c index 1f87513..8f489fa 100644 --- a/drivers/media/usb/cx231xx/cx231xx-video.c +++ b/drivers/media/usb/cx231xx/cx231xx-video.c @@ -886,7 +886,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; f->fmt.pix.field = V4L2_FIELD_INTERLACED; - f->fmt.pix.priv = 0; return 0; } @@ -931,7 +930,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * height; f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; f->fmt.pix.field = V4L2_FIELD_INTERLACED; - f->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index f3a7ace..1b3d7bf 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -1102,8 +1102,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, struct gspca_dev *gspca_dev = video_drvdata(file); fmt->fmt.pix = gspca_dev->pixfmt; - /* some drivers use priv internally, zero it before giving it to - userspace */ + /* some drivers use priv internally, zero it before giving it back to + the core */ fmt->fmt.pix.priv = 0; return 0; } @@ -1139,8 +1139,8 @@ static int try_fmt_vid_cap(struct gspca_dev *gspca_dev, fmt->fmt.pix.height = h; gspca_dev->sd_desc->try_fmt(gspca_dev, fmt); } - /* some drivers use priv internally, zero it before giving it to - userspace */ + /* some drivers use priv internally, zero it before giving it back to + the core */ fmt->fmt.pix.priv = 0; return mode; /* used when s_fmt */ } diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c index 0500c417..cf9a21e 100644 --- a/drivers/media/usb/hdpvr/hdpvr-video.c +++ b/drivers/media/usb/hdpvr/hdpvr-video.c @@ -1022,7 +1022,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *_fh, f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG; f->fmt.pix.sizeimage = dev->bulk_in_size; f->fmt.pix.bytesperline = 0; - f->fmt.pix.priv = 0; if (f->fmt.pix.width == 720) { /* SDTV formats */ f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c index be77482..adfa832 100644 --- a/drivers/media/usb/stkwebcam/stk-webcam.c +++ b/drivers/media/usb/stkwebcam/stk-webcam.c @@ -923,7 +923,6 @@ static int stk_vidioc_g_fmt_vid_cap(struct file *filp, pix_format->bytesperline = 2 * pix_format->width; pix_format->sizeimage = pix_format->bytesperline * pix_format->height; - pix_format->priv = 0; return 0; } @@ -967,7 +966,6 @@ static int stk_try_fmt_vid_cap(struct file *filp, fmtd->fmt.pix.bytesperline = 2 * fmtd->fmt.pix.width; fmtd->fmt.pix.sizeimage = fmtd->fmt.pix.bytesperline * fmtd->fmt.pix.height; - fmtd->fmt.pix.priv = 0; return 0; } diff --git a/drivers/media/usb/tlg2300/pd-video.c b/drivers/media/usb/tlg2300/pd-video.c index 8df668d..8cd7f02 100644 --- a/drivers/media/usb/tlg2300/pd-video.c +++ b/drivers/media/usb/tlg2300/pd-video.c @@ -1321,7 +1321,6 @@ static void init_video_context(struct running_context *context) .bytesperline = 720 * 2, .sizeimage = 720 * 576 * 2, .colorspace = V4L2_COLORSPACE_SMPTE170M, - .priv = 0 }; } diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c index e6b3d5d..1626e9b 100644 --- a/drivers/media/usb/tm6000/tm6000-video.c +++ b/drivers/media/usb/tm6000/tm6000-video.c @@ -918,7 +918,6 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, (f->fmt.pix.width * fh->fmt->depth) >> 3; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; - f->fmt.pix.priv = 0; return 0; } @@ -959,7 +958,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.width &= ~0x01; f->fmt.pix.field = field; - f->fmt.pix.priv = 0; f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3; diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c index 74d56df..0f63954 100644 --- a/drivers/media/usb/zr364xx/zr364xx.c +++ b/drivers/media/usb/zr364xx/zr364xx.c @@ -806,7 +806,6 @@ static int zr364xx_vidioc_try_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = f->fmt.pix.width * 2; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; - f->fmt.pix.priv = 0; DBG("%s: V4L2_PIX_FMT_%s (%d) ok!\n", __func__, decode_fourcc(f->fmt.pix.pixelformat, pixelformat_name), f->fmt.pix.field); @@ -829,7 +828,6 @@ static int zr364xx_vidioc_g_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = f->fmt.pix.width * 2; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; - f->fmt.pix.priv = 0; return 0; } @@ -866,7 +864,6 @@ static int zr364xx_vidioc_s_fmt_vid_cap(struct file *file, void *priv, f->fmt.pix.bytesperline = f->fmt.pix.width * 2; f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline; f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG; - f->fmt.pix.priv = 0; cam->vb_vidq.field = f->fmt.pix.field; if (f->fmt.pix.width == 160 && f->fmt.pix.height == 120) diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 7e2411c..cca6c2f 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -540,7 +540,16 @@ struct v4l2_framebuffer32 { __u32 capability; __u32 flags; compat_caddr_t base; - struct v4l2_pix_format fmt; + struct { + __u32 width; + __u32 height; + __u32 pixelformat; + __u32 field; + __u32 bytesperline; + __u32 sizeimage; + __u32 colorspace; + __u32 priv; + } fmt; }; static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_framebuffer32 __user *up) @@ -550,10 +559,10 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_frame if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_framebuffer32)) || get_user(tmp, &up->base) || get_user(kp->capability, &up->capability) || - get_user(kp->flags, &up->flags)) + get_user(kp->flags, &up->flags) || + copy_from_user(&kp->fmt, &up->fmt, sizeof(up->fmt))) return -EFAULT; kp->base = compat_ptr(tmp); - get_v4l2_pix_format(&kp->fmt, &up->fmt); return 0; } @@ -564,9 +573,9 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer *kp, struct v4l2_frame if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_framebuffer32)) || put_user(tmp, &up->base) || put_user(kp->capability, &up->capability) || - put_user(kp->flags, &up->flags)) + put_user(kp->flags, &up->flags) || + copy_to_user(&up->fmt, &kp->fmt, sizeof(up->fmt))) return -EFAULT; - put_v4l2_pix_format(&kp->fmt, &up->fmt); return 0; } 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 @@ -959,13 +959,48 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type) return -EINVAL; } +static void v4l_sanitize_format(struct v4l2_format *fmt) +{ + unsigned int offset; + + /* + * The v4l2_pix_format structure has been extended with fields that were + * not previously required to be set to zero by applications. The priv + * field, when set to a magic value, indicates the the extended fields + * are valid. Otherwise they will contain undefined values. To simplify + * the API towards drivers zero the extended fields and set the priv + * field to the magic value when the extended pixel format structure + * isn't used by applications. + */ + + if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE && + fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) + return; + + if (fmt->fmt.pix.priv == V4L2_PIX_FMT_PRIV_MAGIC) + return; + + fmt->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; + + offset = offsetof(struct v4l2_pix_format, priv) + + sizeof(fmt->fmt.pix.priv); + memset(((void *)&fmt->fmt.pix) + offset, 0, + sizeof(fmt->fmt.pix) - offset); +} + static int v4l_querycap(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { struct v4l2_capability *cap = (struct v4l2_capability *)arg; + int ret; cap->version = LINUX_VERSION_CODE; - return ops->vidioc_querycap(file, fh, cap); + + ret = ops->vidioc_querycap(file, fh, cap); + + cap->capabilities |= V4L2_CAP_EXT_PIX_FORMAT; + + return ret; } static int v4l_s_input(const struct v4l2_ioctl_ops *ops, @@ -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); 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; + 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; + 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; + + return ret; } 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 + /* * 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 */ + } fmt; }; /* Flags for the 'capability' field. Read only */ #define V4L2_FBUF_CAP_EXTERNOVERLAY 0x0001
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(-)