[RFCv1 PATCH 0/3] Per-device-node capabilities

[RFCv1 PATCH 0/3] Per-device-node capabilities

[Hans Verkuil]

During the recent V4L-DVB workshop we discussed the meaning of the capabilities
field as returned by QUERYCAP. Historically these capabilities refer to the
capabilities of the video device as a whole, and not to the capabilities of the
filehandle through which QUERYCAP was called.

So QUERYCAP would give the same capabilities for video device nodes as it would
for vbi or radio device nodes.

For more complex devices it is very desirable to give the caps for just the
current device node.

This patch series adds this functionality to V4L2 by adding a new global
capability bit and a new device_caps field:

#define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */

struct v4l2_capability {
	...
	__u32   device_caps;    /* Device node capabilities */
	...
}

It implements it for vivi and ivtv as well.

I haven't updated the documentation yet as I want to get a round of
feedback first. Especially with regards to the naming: I decided to call it
'device_caps' since for applications the term 'device' is fairly unambiguous
IMHO.

Comments?

	Hans

[RFCv1 PATCH 1/3] V4L2: Add per-device-node capabilities

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If V4L2_CAP_DEVICE_CAPS is set, then the new device_caps field is filled with
the capabilities of the opened device node.

The capabilities field traditionally contains the capabilities of the whole
device. E.g., if you open video0, then if it contains VBI caps then that means
that there is a corresponding vbi node as well. And the capabilities field of
both the video and vbi node should contain identical caps.

However, it would be very useful to also have a capabilities field that contains
just the caps for the currently open device, hence the new CAP bit and field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/linux/videodev2.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 4b752d5..2b6338b 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -243,8 +243,9 @@ struct v4l2_capability {
 	__u8	card[32];	/* i.e. "Hauppauge WinTV" */
 	__u8	bus_info[32];	/* "PCI:" + pci_name(pci_dev) */
 	__u32   version;        /* should use KERNEL_VERSION() */
-	__u32	capabilities;	/* Device capabilities */
-	__u32	reserved[4];
+	__u32	capabilities;	/* Global device capabilities */
+	__u32	device_caps;	/* Device node capabilities */
+	__u32	reserved[3];
 };
 
 /* Values for 'capabilities' field */
@@ -274,6 +275,8 @@ struct v4l2_capability {
 #define V4L2_CAP_ASYNCIO                0x02000000  /* async I/O */
 #define V4L2_CAP_STREAMING              0x04000000  /* streaming I/O ioctls */
 
+#define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
+
 /*
  *	V I D E O   I M A G E   F O R M A T
  */
-- 
1.7.6.3

[RFCv1 PATCH 2/3] vivi: set device_caps.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/vivi.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 7d754fb..84ea88d 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -819,8 +819,9 @@ static int vidioc_querycap(struct file *file, void  *priv,
 	strcpy(cap->driver, "vivi");
 	strcpy(cap->card, "vivi");
 	strlcpy(cap->bus_info, dev->v4l2_dev.name, sizeof(cap->bus_info));
-	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | \
-			    V4L2_CAP_READWRITE;
+	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
+			    V4L2_CAP_READWRITE | V4L2_CAP_DEVICE_CAPS;
+	cap->device_caps = cap->capabilities;
 	return 0;
 }
 
-- 
1.7.6.3

[RFCv1 PATCH 3/3] ivtv: setup per-device caps.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/ivtv/ivtv-driver.h  |    1 +
 drivers/media/video/ivtv/ivtv-ioctl.c   |    7 +++++--
 drivers/media/video/ivtv/ivtv-streams.c |   14 ++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtv-driver.h b/drivers/media/video/ivtv/ivtv-driver.h
index 8f9cc17..06b9efd 100644
--- a/drivers/media/video/ivtv/ivtv-driver.h
+++ b/drivers/media/video/ivtv/ivtv-driver.h
@@ -331,6 +331,7 @@ struct ivtv_stream {
 	struct ivtv *itv; 		/* for ease of use */
 	const char *name;		/* name of the stream */
 	int type;			/* stream type */
+	u32 caps;			/* V4L2 capabilities */
 
 	u32 id;
 	spinlock_t qlock; 		/* locks access to the queues */
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index ecafa69..6be63e9 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -752,12 +752,15 @@ static int ivtv_s_register(struct file *file, void *fh, struct v4l2_dbg_register
 
 static int ivtv_querycap(struct file *file, void *fh, struct v4l2_capability *vcap)
 {
-	struct ivtv *itv = fh2id(fh)->itv;
+	struct ivtv_open_id *id = fh2id(file->private_data);
+	struct ivtv *itv = id->itv;
+	struct ivtv_stream *s = &itv->streams[id->type];
 
 	strlcpy(vcap->driver, IVTV_DRIVER_NAME, sizeof(vcap->driver));
 	strlcpy(vcap->card, itv->card_name, sizeof(vcap->card));
 	snprintf(vcap->bus_info, sizeof(vcap->bus_info), "PCI:%s", pci_name(itv->pdev));
-	vcap->capabilities = itv->v4l2_cap; 	    /* capabilities */
+	vcap->capabilities = itv->v4l2_cap | V4L2_CAP_DEVICE_CAPS;
+	vcap->device_caps = s->caps;
 	return 0;
 }
 
diff --git a/drivers/media/video/ivtv/ivtv-streams.c b/drivers/media/video/ivtv/ivtv-streams.c
index e7794dc..4d4ae6e 100644
--- a/drivers/media/video/ivtv/ivtv-streams.c
+++ b/drivers/media/video/ivtv/ivtv-streams.c
@@ -78,60 +78,73 @@ static struct {
 	int num_offset;
 	int dma, pio;
 	enum v4l2_buf_type buf_type;
+	u32 v4l2_caps;
 	const struct v4l2_file_operations *fops;
 } ivtv_stream_info[] = {
 	{	/* IVTV_ENC_STREAM_TYPE_MPG */
 		"encoder MPG",
 		VFL_TYPE_GRABBER, 0,
 		PCI_DMA_FROMDEVICE, 0, V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_TUNER |
+			V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_ENC_STREAM_TYPE_YUV */
 		"encoder YUV",
 		VFL_TYPE_GRABBER, IVTV_V4L2_ENC_YUV_OFFSET,
 		PCI_DMA_FROMDEVICE, 0, V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_TUNER |
+			V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_ENC_STREAM_TYPE_VBI */
 		"encoder VBI",
 		VFL_TYPE_VBI, 0,
 		PCI_DMA_FROMDEVICE, 0, V4L2_BUF_TYPE_VBI_CAPTURE,
+		V4L2_CAP_VBI_CAPTURE | V4L2_CAP_SLICED_VBI_CAPTURE | V4L2_CAP_TUNER |
+			V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_ENC_STREAM_TYPE_PCM */
 		"encoder PCM",
 		VFL_TYPE_GRABBER, IVTV_V4L2_ENC_PCM_OFFSET,
 		PCI_DMA_FROMDEVICE, 0, V4L2_BUF_TYPE_PRIVATE,
+		V4L2_CAP_TUNER | V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_ENC_STREAM_TYPE_RAD */
 		"encoder radio",
 		VFL_TYPE_RADIO, 0,
 		PCI_DMA_NONE, 1, V4L2_BUF_TYPE_PRIVATE,
+		V4L2_CAP_RADIO | V4L2_CAP_TUNER,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_DEC_STREAM_TYPE_MPG */
 		"decoder MPG",
 		VFL_TYPE_GRABBER, IVTV_V4L2_DEC_MPG_OFFSET,
 		PCI_DMA_TODEVICE, 0, V4L2_BUF_TYPE_VIDEO_OUTPUT,
+		V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_dec_fops
 	},
 	{	/* IVTV_DEC_STREAM_TYPE_VBI */
 		"decoder VBI",
 		VFL_TYPE_VBI, IVTV_V4L2_DEC_VBI_OFFSET,
 		PCI_DMA_NONE, 1, V4L2_BUF_TYPE_VBI_CAPTURE,
+		V4L2_CAP_SLICED_VBI_CAPTURE | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_enc_fops
 	},
 	{	/* IVTV_DEC_STREAM_TYPE_VOUT */
 		"decoder VOUT",
 		VFL_TYPE_VBI, IVTV_V4L2_DEC_VOUT_OFFSET,
 		PCI_DMA_NONE, 1, V4L2_BUF_TYPE_VBI_OUTPUT,
+		V4L2_CAP_SLICED_VBI_OUTPUT | V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_dec_fops
 	},
 	{	/* IVTV_DEC_STREAM_TYPE_YUV */
 		"decoder YUV",
 		VFL_TYPE_GRABBER, IVTV_V4L2_DEC_YUV_OFFSET,
 		PCI_DMA_TODEVICE, 0, V4L2_BUF_TYPE_VIDEO_OUTPUT,
+		V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_AUDIO | V4L2_CAP_READWRITE,
 		&ivtv_v4l2_dec_fops
 	}
 };
@@ -149,6 +162,7 @@ static void ivtv_stream_init(struct ivtv *itv, int type)
 	s->itv = itv;
 	s->type = type;
 	s->name = ivtv_stream_info[type].name;
+	s->caps = ivtv_stream_info[type].v4l2_caps;
 
 	if (ivtv_stream_info[type].pio)
 		s->dma = PCI_DMA_NONE;
-- 
1.7.6.3

Re: [RFCv1 PATCH 1/3] V4L2: Add per-device-node capabilities

[Sylwester Nawrocki]

Hello Hans,

On 11/07/2011 11:37 AM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
> 
> If V4L2_CAP_DEVICE_CAPS is set, then the new device_caps field is filled with
> the capabilities of the opened device node.
> 
> The capabilities field traditionally contains the capabilities of the whole
> device. E.g., if you open video0, then if it contains VBI caps then that means
> that there is a corresponding vbi node as well. And the capabilities field of
> both the video and vbi node should contain identical caps.
> 
> However, it would be very useful to also have a capabilities field that contains
> just the caps for the currently open device, hence the new CAP bit and field.
> 
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   include/linux/videodev2.h |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 4b752d5..2b6338b 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -243,8 +243,9 @@ struct v4l2_capability {
>   	__u8	card[32];	/* i.e. "Hauppauge WinTV" */
>   	__u8	bus_info[32];	/* "PCI:" + pci_name(pci_dev) */
>   	__u32   version;        /* should use KERNEL_VERSION() */
> -	__u32	capabilities;	/* Device capabilities */
> -	__u32	reserved[4];
> +	__u32	capabilities;	/* Global device capabilities */
> +	__u32	device_caps;	/* Device node capabilities */

How about changing this to

	__u32	devnode_caps;	/* Device node capabilities */

> +	__u32	reserved[3];
>   };
> 
>   /* Values for 'capabilities' field */
> @@ -274,6 +275,8 @@ struct v4l2_capability {
>   #define V4L2_CAP_ASYNCIO                0x02000000  /* async I/O */
>   #define V4L2_CAP_STREAMING              0x04000000  /* streaming I/O ioctls */
> 
> +#define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */

..and

#define V4L2_CAP_DEVNODE_CAPS            0x80000000  /* sets device node capabilities field */

?

'device' might suggest a whole physical device/system at first sight.
Maybe devnode_caps is not be the best name but it seems more explicit and
less confusing :) 

It's just my personal opinion though.

--
Regards,
Sylwester

Re: [RFCv1 PATCH 1/3] V4L2: Add per-device-node capabilities

[Hans Verkuil]

On Tuesday 15 November 2011 21:18:12 Sylwester Nawrocki wrote:
> Hello Hans,
> 
> On 11/07/2011 11:37 AM, Hans Verkuil wrote:
> > From: Hans Verkuil<hans.verkuil@cisco.com>
> > 
> > If V4L2_CAP_DEVICE_CAPS is set, then the new device_caps field is filled
> > with the capabilities of the opened device node.
> > 
> > The capabilities field traditionally contains the capabilities of the
> > whole device. E.g., if you open video0, then if it contains VBI caps
> > then that means that there is a corresponding vbi node as well. And the
> > capabilities field of both the video and vbi node should contain
> > identical caps.
> > 
> > However, it would be very useful to also have a capabilities field that
> > contains just the caps for the currently open device, hence the new CAP
> > bit and field.
> > 
> > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> > ---
> > 
> >   include/linux/videodev2.h |    7 +++++--
> >   1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 4b752d5..2b6338b 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -243,8 +243,9 @@ struct v4l2_capability {
> > 
> >   	__u8	card[32];	/* i.e. "Hauppauge WinTV" */
> >   	__u8	bus_info[32];	/* "PCI:" + pci_name(pci_dev) */
> >   	__u32   version;        /* should use KERNEL_VERSION() */
> > 
> > -	__u32	capabilities;	/* Device capabilities */
> > -	__u32	reserved[4];
> > +	__u32	capabilities;	/* Global device capabilities */
> > +	__u32	device_caps;	/* Device node capabilities */
> 
> How about changing this to
> 
> 	__u32	devnode_caps;	/* Device node capabilities */
> 
> > +	__u32	reserved[3];
> > 
> >   };
> >   
> >   /* Values for 'capabilities' field */
> > 
> > @@ -274,6 +275,8 @@ struct v4l2_capability {
> > 
> >   #define V4L2_CAP_ASYNCIO                0x02000000  /* async I/O */
> >   #define V4L2_CAP_STREAMING              0x04000000  /* streaming I/O
> >   ioctls */
> > 
> > +#define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device
> > capabilities field */
> 
> ..and
> 
> #define V4L2_CAP_DEVNODE_CAPS            0x80000000  /* sets device node
> capabilities field */
> 
> ?
> 
> 'device' might suggest a whole physical device/system at first sight.
> Maybe devnode_caps is not be the best name but it seems more explicit and
> less confusing :)
> 
> It's just my personal opinion though.

I also have a preference for devnode, but it is my understanding that Mauro 
prefers 'device' over 'devnode'. Is that correct, Mauro?

I am OK with either.

Regards,

	Hans

> 
> --
> Regards,
> Sylwester
> --
> 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

Re: [RFCv1 PATCH 1/3] V4L2: Add per-device-node capabilities

[Mauro Carvalho Chehab]

Em 16-11-2011 12:41, Hans Verkuil escreveu:
> On Tuesday 15 November 2011 21:18:12 Sylwester Nawrocki wrote:
>> Hello Hans,
>>
>> On 11/07/2011 11:37 AM, Hans Verkuil wrote:
>>> From: Hans Verkuil<hans.verkuil@cisco.com>
>>>
>>> If V4L2_CAP_DEVICE_CAPS is set, then the new device_caps field is filled
>>> with the capabilities of the opened device node.
>>>
>>> The capabilities field traditionally contains the capabilities of the
>>> whole device. E.g., if you open video0, then if it contains VBI caps
>>> then that means that there is a corresponding vbi node as well. And the
>>> capabilities field of both the video and vbi node should contain
>>> identical caps.
>>>
>>> However, it would be very useful to also have a capabilities field that
>>> contains just the caps for the currently open device, hence the new CAP
>>> bit and field.
>>>
>>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
>>> ---
>>>
>>>   include/linux/videodev2.h |    7 +++++--
>>>   1 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>> index 4b752d5..2b6338b 100644
>>> --- a/include/linux/videodev2.h
>>> +++ b/include/linux/videodev2.h
>>> @@ -243,8 +243,9 @@ struct v4l2_capability {
>>>
>>>   	__u8	card[32];	/* i.e. "Hauppauge WinTV" */
>>>   	__u8	bus_info[32];	/* "PCI:" + pci_name(pci_dev) */
>>>   	__u32   version;        /* should use KERNEL_VERSION() */
>>>
>>> -	__u32	capabilities;	/* Device capabilities */
>>> -	__u32	reserved[4];
>>> +	__u32	capabilities;	/* Global device capabilities */
>>> +	__u32	device_caps;	/* Device node capabilities */
>>
>> How about changing this to
>>
>> 	__u32	devnode_caps;	/* Device node capabilities */
>>
>>> +	__u32	reserved[3];
>>>
>>>   };
>>>   
>>>   /* Values for 'capabilities' field */
>>>
>>> @@ -274,6 +275,8 @@ struct v4l2_capability {
>>>
>>>   #define V4L2_CAP_ASYNCIO                0x02000000  /* async I/O */
>>>   #define V4L2_CAP_STREAMING              0x04000000  /* streaming I/O
>>>   ioctls */
>>>
>>> +#define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device
>>> capabilities field */
>>
>> ..and
>>
>> #define V4L2_CAP_DEVNODE_CAPS            0x80000000  /* sets device node
>> capabilities field */
>>
>> ?
>>
>> 'device' might suggest a whole physical device/system at first sight.
>> Maybe devnode_caps is not be the best name but it seems more explicit and
>> less confusing :)

On kernel, "device" doesn't represent a physical device/system.
See, for example:
	Documentation/devices.txt

It is clear there that a device is directly associated with a devnode.

The thing is that the kernel struct that represents a device is "struct device".
And the userspace view for "struct device" is a device node.

As I told several times, what we call "subdevice" is, in fact a device, as it is
(on most cases) exported via devnodes to userspace. For example, all I2C subdevices
are devices, as they can be seen via devnodes at the I2C bus support (if i2c-dev
module is loaded). Worse than that, if MC/subdev API is used, the same sub-device
will be, in fact, 2 devices (one I2C device, and one V4L2 subdev device), each
device with its own device node.

A "physical device" can have more than one device. For example, serial devices, in
general, have two devices for each physical one (cua0-191 and TTYS0-191). This is
there since the beginning of Linux.

So, calling/interpreting the term "device" as the physical device is _wrong_.
It might be used as-is only if there's only one device is associated to the physical
device. I don't think there's any such case currently at V4L2 (as, at least, one
bus device and one V4L2 device will be created at the simplest case).

>> It's just my personal opinion though.
> 
> I also have a preference for devnode, but it is my understanding that Mauro 
> prefers 'device' over 'devnode'. Is that correct, Mauro?

This is a recurrent discussion. Do a "git grep devnode|wc" and compare it with
a "git grep device|wc".

So, calling anything as "devnode" is confusing, as there's no obvious way to
distinguish it from "device".

>>> +	__u32	capabilities;	/* Global device capabilities */
>>> +	__u32	device_caps;	/* Device node capabilities */

I would change the comments to:

	__u32	capabilities;	/* capabilities present at the physical device as a hole */
	__u32	device_caps;	/* capabilities accessed via this particular device (node) */

Btw, the better would be to use the standard way to comment it:

/**
 * struct v4l2_capability - Describes V4L2 device caps on VIDIOC_QUERYCAP
...
 @capabilities: capabilities present at the physical device as a hole
 @device_caps:	capabilities accessed via this particular device
...
 */


> I am OK with either.
> 
> Regards,
> 
> 	Hans
> 
>>
>> --
>> Regards,
>> Sylwester
>> --
>> 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