* [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
@ 2017-03-30 16:02 Helen Koike
2017-03-30 16:02 ` [PATCH 2/2] [media] docs-rst: " Helen Koike
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Helen Koike @ 2017-03-30 16:02 UTC (permalink / raw)
To: Sakari Ailus, Hans Verkuil
Cc: Mauro Carvalho Chehab, linux-media, jgebben, Laurent Pinchart
Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
used when no inputs are available in the device
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
include/uapi/linux/videodev2.h | 1 +
3 files changed, 54 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 0c3f238..ccaf04b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
return vdev->lock;
}
+int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
+ struct v4l2_input *i)
+{
+ if (i->index > 0)
+ return -EINVAL;
+
+ memset(i, 0, sizeof(*i));
+ i->type = V4L2_INPUT_TYPE_DEFAULT;
+ strlcpy(i->name, "Default", sizeof(i->name));
+
+ return 0;
+}
+EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
+
+int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
+{
+ *i = 0;
+ return 0;
+}
+EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
+
+int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
+{
+ return i ? -EINVAL : 0;
+}
+EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
+
/* Common ioctl debug function. This function can be used by
external ioctl messages as well as internal V4L ioctl */
void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 6cd94e5..accc470 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -652,6 +652,32 @@ struct video_device;
*/
struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
+
+/**
+ * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
+ *
+ * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
+ * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
+ */
+int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
+ struct v4l2_input *i);
+
+/**
+ * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
+ *
+ * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
+ * when using v4l2_ioctl_enum_input_default
+ */
+int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
+
+/**
+ * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
+ *
+ * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
+ * when using v4l2_ioctl_enum_input_default
+ */
+int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
+
/* names for fancy debug output */
extern const char *v4l2_field_names[];
extern const char *v4l2_type_names[];
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 316be62..c10bbde 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1477,6 +1477,7 @@ struct v4l2_input {
};
/* Values for the 'type' field */
+#define V4L2_INPUT_TYPE_DEFAULT 0
#define V4L2_INPUT_TYPE_TUNER 1
#define V4L2_INPUT_TYPE_CAMERA 2
#define V4L2_INPUT_TYPE_TOUCH 3
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] [media] docs-rst: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 16:02 [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT Helen Koike
@ 2017-03-30 16:02 ` Helen Koike
2017-03-30 20:26 ` Sakari Ailus
2017-03-30 19:56 ` [PATCH RFC 1/2] [media] v4l2: " Laurent Pinchart
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Helen Koike @ 2017-03-30 16:02 UTC (permalink / raw)
To: Sakari Ailus, Hans Verkuil
Cc: Mauro Carvalho Chehab, linux-media, jgebben, Laurent Pinchart
add documentation for V4L2_INPUT_TYPE_DEFAULT
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
Documentation/media/uapi/v4l/vidioc-enuminput.rst | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/media/uapi/v4l/vidioc-enuminput.rst b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
index 17aaaf9..0237e10 100644
--- a/Documentation/media/uapi/v4l/vidioc-enuminput.rst
+++ b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
@@ -112,6 +112,9 @@ at index zero, incrementing by one until the driver returns ``EINVAL``.
:stub-columns: 0
:widths: 3 1 4
+ * - ``V4L2_INPUT_TYPE_DEFAULT``
+ - 0
+ - This is the default value returned when no input is supported.
* - ``V4L2_INPUT_TYPE_TUNER``
- 1
- This input uses a tuner (RF demodulator).
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 16:02 [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT Helen Koike
2017-03-30 16:02 ` [PATCH 2/2] [media] docs-rst: " Helen Koike
@ 2017-03-30 19:56 ` Laurent Pinchart
2017-03-31 2:39 ` Helen Koike
2017-03-31 8:29 ` Hans Verkuil
2017-06-14 4:50 ` [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC Helen Koike
3 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2017-03-30 19:56 UTC (permalink / raw)
To: Helen Koike
Cc: Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab, linux-media, jgebben
Hi Helen,
Thank you for the patch.
On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> used when no inputs are available in the device
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 54 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device
> *vdev, unsigned cmd) return vdev->lock;
> }
>
> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> + struct v4l2_input *i)
> +{
> + if (i->index > 0)
> + return -EINVAL;
> +
> + memset(i, 0, sizeof(*i));
> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> + strlcpy(i->name, "Default", sizeof(i->name));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
V4L2 tends to use EXPORT_SYMBOL_GPL.
What would you think about calling those default functions directly from the
core when the input ioctl handlers are not set ? You wouldn't need to modify
drivers.
> +
> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int
> *i) +{
> + *i = 0;
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> +
> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int
> i) +{
> + return i ? -EINVAL : 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> +
> /* Common ioctl debug function. This function can be used by
> external ioctl messages as well as internal V4L ioctl */
> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 6cd94e5..accc470 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -652,6 +652,32 @@ struct video_device;
> */
> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int
> cmd);
>
> +
> +/**
> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT
> ioctl + *
> + * Plug this function in vidioc_enum_input field of the struct
> v4l2_ioctl_ops to + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> + */
> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> + struct v4l2_input *i);
> +
> +/**
> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
> + *
> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
> + * when using v4l2_ioctl_enum_input_default
> + */
> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int
> *i); +
> +/**
> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
> + *
> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
> + * when using v4l2_ioctl_enum_input_default
> + */
> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int
> i); +
> /* names for fancy debug output */
> extern const char *v4l2_field_names[];
> extern const char *v4l2_type_names[];
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 316be62..c10bbde 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1477,6 +1477,7 @@ struct v4l2_input {
> };
>
> /* Values for the 'type' field */
> +#define V4L2_INPUT_TYPE_DEFAULT 0
> #define V4L2_INPUT_TYPE_TUNER 1
> #define V4L2_INPUT_TYPE_CAMERA 2
> #define V4L2_INPUT_TYPE_TOUCH 3
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] [media] docs-rst: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 16:02 ` [PATCH 2/2] [media] docs-rst: " Helen Koike
@ 2017-03-30 20:26 ` Sakari Ailus
2017-03-31 9:47 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2017-03-30 20:26 UTC (permalink / raw)
To: Helen Koike
Cc: Hans Verkuil, Mauro Carvalho Chehab, linux-media, jgebben,
Laurent Pinchart
Hi Helen and others,
On Thu, Mar 30, 2017 at 01:02:18PM -0300, Helen Koike wrote:
> add documentation for V4L2_INPUT_TYPE_DEFAULT
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> Documentation/media/uapi/v4l/vidioc-enuminput.rst | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-enuminput.rst b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> index 17aaaf9..0237e10 100644
> --- a/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> @@ -112,6 +112,9 @@ at index zero, incrementing by one until the driver returns ``EINVAL``.
> :stub-columns: 0
> :widths: 3 1 4
>
> + * - ``V4L2_INPUT_TYPE_DEFAULT``
> + - 0
> + - This is the default value returned when no input is supported.
> * - ``V4L2_INPUT_TYPE_TUNER``
> - 1
> - This input uses a tuner (RF demodulator).
What would you think of calling this input as "unknown" instead of
"default"? That's what an input which isn't really specified actually is.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 19:56 ` [PATCH RFC 1/2] [media] v4l2: " Laurent Pinchart
@ 2017-03-31 2:39 ` Helen Koike
2017-03-31 3:55 ` Helen Koike
2017-03-31 8:41 ` Laurent Pinchart
0 siblings, 2 replies; 28+ messages in thread
From: Helen Koike @ 2017-03-31 2:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab, linux-media, jgebben
Hi Laurent,
Thanks for reviewing
On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
> Hi Helen,
>
> Thank you for the patch.
>
> On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>> used when no inputs are available in the device
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> ---
>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>> include/uapi/linux/videodev2.h | 1 +
>> 3 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device
>> *vdev, unsigned cmd) return vdev->lock;
>> }
>>
>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>> + struct v4l2_input *i)
>> +{
>> + if (i->index > 0)
>> + return -EINVAL;
>> +
>> + memset(i, 0, sizeof(*i));
>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>> + strlcpy(i->name, "Default", sizeof(i->name));
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>
> V4L2 tends to use EXPORT_SYMBOL_GPL.
The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
another patch) ?
>
> What would you think about calling those default functions directly from the
> core when the input ioctl handlers are not set ? You wouldn't need to modify
> drivers.
Sure, I'll add them in ops inside __video_register_device when it
validates the ioctls
>
>> +
>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int
>> *i) +{
>> + *i = 0;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>> +
>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int
>> i) +{
>> + return i ? -EINVAL : 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>> +
>> /* Common ioctl debug function. This function can be used by
>> external ioctl messages as well as internal V4L ioctl */
>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index 6cd94e5..accc470 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -652,6 +652,32 @@ struct video_device;
>> */
>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int
>> cmd);
>>
>> +
>> +/**
>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT
>> ioctl + *
>> + * Plug this function in vidioc_enum_input field of the struct
>> v4l2_ioctl_ops to + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>> + */
>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>> + struct v4l2_input *i);
>> +
>> +/**
>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
>> + *
>> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
>> + * when using v4l2_ioctl_enum_input_default
>> + */
>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int
>> *i); +
>> +/**
>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
>> + *
>> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
>> + * when using v4l2_ioctl_enum_input_default
>> + */
>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int
>> i); +
>> /* names for fancy debug output */
>> extern const char *v4l2_field_names[];
>> extern const char *v4l2_type_names[];
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 316be62..c10bbde 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>> };
>>
>> /* Values for the 'type' field */
>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>> #define V4L2_INPUT_TYPE_TUNER 1
>> #define V4L2_INPUT_TYPE_CAMERA 2
>> #define V4L2_INPUT_TYPE_TOUCH 3
>
Helen
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 2:39 ` Helen Koike
@ 2017-03-31 3:55 ` Helen Koike
2017-03-31 9:42 ` Mauro Carvalho Chehab
2017-03-31 8:41 ` Laurent Pinchart
1 sibling, 1 reply; 28+ messages in thread
From: Helen Koike @ 2017-03-31 3:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab, linux-media, jgebben
On 2017-03-30 11:39 PM, Helen Koike wrote:
> Hi Laurent,
>
> Thanks for reviewing
>
> On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
>> Hi Helen,
>>
>> Thank you for the patch.
>>
>> On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>> used when no inputs are available in the device
>>>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>> include/uapi/linux/videodev2.h | 1 +
>>> 3 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>> video_device
>>> *vdev, unsigned cmd) return vdev->lock;
>>> }
>>>
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i)
>>> +{
>>> + if (i->index > 0)
>>> + return -EINVAL;
>>> +
>>> + memset(i, 0, sizeof(*i));
>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>
>> V4L2 tends to use EXPORT_SYMBOL_GPL.
>
> The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
> EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
> another patch) ?
>
>>
>> What would you think about calling those default functions directly
>> from the
>> core when the input ioctl handlers are not set ? You wouldn't need to
>> modify
>> drivers.
>
> Sure, I'll add them in ops inside __video_register_device when it
> validates the ioctls
I just realize I can not simply override struct v4l2_ioctl_ops as it is
declared as a const inside strut video_device. I'll call those default
functions only when the ioctls are handled to not modify vdev->ops.
>
>>
>>> +
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>> unsigned int
>>> *i) +{
>>> + *i = 0;
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>> +
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>> unsigned int
>>> i) +{
>>> + return i ? -EINVAL : 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>> +
>>> /* Common ioctl debug function. This function can be used by
>>> external ioctl messages as well as internal V4L ioctl */
>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index 6cd94e5..accc470 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -652,6 +652,32 @@ struct video_device;
>>> */
>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
>>> unsigned int
>>> cmd);
>>>
>>> +
>>> +/**
>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
>>> VIDIOC_ENUM_INPUT
>>> ioctl + *
>>> + * Plug this function in vidioc_enum_input field of the struct
>>> v4l2_ioctl_ops to + * enumerate a single input as
>>> V4L2_INPUT_TYPE_DEFAULT
>>> + */
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
>>> ioctl
>>> + *
>>> + * Plug this function in vidioc_g_input field of the struct
>>> v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>> unsigned int
>>> *i); +
>>> +/**
>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
>>> ioctl
>>> + *
>>> + * Plug this function in vidioc_s_input field of the struct
>>> v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>> unsigned int
>>> i); +
>>> /* names for fancy debug output */
>>> extern const char *v4l2_field_names[];
>>> extern const char *v4l2_type_names[];
>>> diff --git a/include/uapi/linux/videodev2.h
>>> b/include/uapi/linux/videodev2.h
>>> index 316be62..c10bbde 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>> };
>>>
>>> /* Values for the 'type' field */
>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>> #define V4L2_INPUT_TYPE_TUNER 1
>>> #define V4L2_INPUT_TYPE_CAMERA 2
>>> #define V4L2_INPUT_TYPE_TOUCH 3
>>
>
> Helen
Helen
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 16:02 [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT Helen Koike
2017-03-30 16:02 ` [PATCH 2/2] [media] docs-rst: " Helen Koike
2017-03-30 19:56 ` [PATCH RFC 1/2] [media] v4l2: " Laurent Pinchart
@ 2017-03-31 8:29 ` Hans Verkuil
2017-03-31 8:41 ` Laurent Pinchart
2017-03-31 9:57 ` Mauro Carvalho Chehab
2017-06-14 4:50 ` [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC Helen Koike
3 siblings, 2 replies; 28+ messages in thread
From: Hans Verkuil @ 2017-03-31 8:29 UTC (permalink / raw)
To: Helen Koike, Sakari Ailus
Cc: Mauro Carvalho Chehab, linux-media, jgebben, Laurent Pinchart
On 30/03/17 18:02, Helen Koike wrote:
> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> used when no inputs are available in the device
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 54 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 0c3f238..ccaf04b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
> return vdev->lock;
> }
>
> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> + struct v4l2_input *i)
> +{
> + if (i->index > 0)
> + return -EINVAL;
> +
> + memset(i, 0, sizeof(*i));
> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> + strlcpy(i->name, "Default", sizeof(i->name));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> +
> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> +
> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
> +{
> + return i ? -EINVAL : 0;
> +}
> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> +
> /* Common ioctl debug function. This function can be used by
> external ioctl messages as well as internal V4L ioctl */
> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 6cd94e5..accc470 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -652,6 +652,32 @@ struct video_device;
> */
> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
>
> +
> +/**
> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
> + *
> + * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> + */
> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> + struct v4l2_input *i);
> +
> +/**
> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
> + *
> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
> + * when using v4l2_ioctl_enum_input_default
> + */
> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
> +
> +/**
> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
> + *
> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
> + * when using v4l2_ioctl_enum_input_default
> + */
> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
> +
> /* names for fancy debug output */
> extern const char *v4l2_field_names[];
> extern const char *v4l2_type_names[];
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 316be62..c10bbde 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1477,6 +1477,7 @@ struct v4l2_input {
> };
>
> /* Values for the 'type' field */
> +#define V4L2_INPUT_TYPE_DEFAULT 0
I don't think we should add a new type here. The whole point of this exercise is to
allow existing apps to work, and existing apps expect a TYPE_CAMERA.
BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
stream, whether it is from a sensor, composite input, HDMI, etc.
The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
indicates that this input is controlled via the media controller. That makes much
more sense and it wouldn't potentially break applications.
Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
spec is again out of date).
Regarding the name: should we use the name stored in struct video_device instead?
That might be more descriptive. Alternatively use something like "Media Controller Input".
More helpful (perhaps) than just "Default" or "Unknown".
I'll make a patch to update the input/output type description in the spec.
Regards,
Hans
> #define V4L2_INPUT_TYPE_TUNER 1
> #define V4L2_INPUT_TYPE_CAMERA 2
> #define V4L2_INPUT_TYPE_TOUCH 3
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 8:29 ` Hans Verkuil
@ 2017-03-31 8:41 ` Laurent Pinchart
2017-03-31 9:08 ` Hans Verkuil
2017-03-31 9:57 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2017-03-31 8:41 UTC (permalink / raw)
To: Hans Verkuil
Cc: Helen Koike, Sakari Ailus, Mauro Carvalho Chehab, linux-media, jgebben
Hi Hans,
On Friday 31 Mar 2017 10:29:04 Hans Verkuil wrote:
> On 30/03/17 18:02, Helen Koike wrote:
> > Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> > used when no inputs are available in the device
> >
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > ---
> >
> > drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> > include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> > include/uapi/linux/videodev2.h | 1 +
> > 3 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> > video_device *vdev, unsigned cmd)>
> > return vdev->lock;
> >
> > }
> >
> > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > + struct v4l2_input *i)
> > +{
> > + if (i->index > 0)
> > + return -EINVAL;
> > +
> > + memset(i, 0, sizeof(*i));
> > + i->type = V4L2_INPUT_TYPE_DEFAULT;
> > + strlcpy(i->name, "Default", sizeof(i->name));
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> > +
> > +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
> > int *i)
> > +{
> > + *i = 0;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> > +
> > +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
> > int i)
> > +{
> > + return i ? -EINVAL : 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> > +
> > /* Common ioctl debug function. This function can be used by
> > external ioctl messages as well as internal V4L ioctl */
> > void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index 6cd94e5..accc470 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -652,6 +652,32 @@ struct video_device;
> > */
> >
> > struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int
> > cmd);
> > +
> > +/**
> > + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
> > VIDIOC_ENUM_INPUT ioctl + *
> > + * Plug this function in vidioc_enum_input field of the struct
> > v4l2_ioctl_ops to + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> > + */
> > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > + struct v4l2_input *i);
> > +
> > +/**
> > + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
> > ioctl
> > + *
> > + * Plug this function in vidioc_g_input field of the struct
> > v4l2_ioctl_ops
> > + * when using v4l2_ioctl_enum_input_default
> > + */
> > +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
> > int *i); +
> > +/**
> > + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
> > ioctl
> > + *
> > + * Plug this function in vidioc_s_input field of the struct
> > v4l2_ioctl_ops
> > + * when using v4l2_ioctl_enum_input_default
> > + */
> > +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
> > int i); +
> >
> > /* names for fancy debug output */
> > extern const char *v4l2_field_names[];
> > extern const char *v4l2_type_names[];
> >
> > diff --git a/include/uapi/linux/videodev2.h
> > b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1477,6 +1477,7 @@ struct v4l2_input {
> >
> > };
> >
> > /* Values for the 'type' field */
> >
> > +#define V4L2_INPUT_TYPE_DEFAULT 0
>
> I don't think we should add a new type here. The whole point of this
> exercise is to allow existing apps to work, and existing apps expect a
> TYPE_CAMERA.
>
> BTW, don't read to much in the term 'CAMERA': it's really a catch all for
> any video stream, whether it is from a sensor, composite input, HDMI, etc.
>
> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of
> date :-(
>
> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability
> that indicates that this input is controlled via the media controller. That
> makes much more sense and it wouldn't potentially break applications.
>
> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and
> use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated
> name and the spec is again out of date).
What would those capabilities be used for ? Applications can already know
whether the MC API is used by a driver. Furthermore, if we really need such a
flag, I wouldn't add it at the input/output level but as a video node
capability flag.
> Regarding the name: should we use the name stored in struct video_device
> instead? That might be more descriptive. Alternatively use something like
> "Media Controller Input".
>
> More helpful (perhaps) than just "Default" or "Unknown".
If the purpose of the name field is to be displayed as-is to the end-user,
then "Media Controller Input" is as useless as "Unknown". "Default" would be
slightly better.
> I'll make a patch to update the input/output type description in the spec.
>
> > #define V4L2_INPUT_TYPE_TUNER 1
> > #define V4L2_INPUT_TYPE_CAMERA 2
> > #define V4L2_INPUT_TYPE_TOUCH 3
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 2:39 ` Helen Koike
2017-03-31 3:55 ` Helen Koike
@ 2017-03-31 8:41 ` Laurent Pinchart
2017-03-31 9:33 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2017-03-31 8:41 UTC (permalink / raw)
To: Helen Koike
Cc: Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab, linux-media, jgebben
Hi Helen,
On Thursday 30 Mar 2017 23:39:01 Helen Koike wrote:
> On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
> > On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
> >> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> >> used when no inputs are available in the device
> >>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >> ---
> >>
> >> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> >> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> >> include/uapi/linux/videodev2.h | 1 +
> >> 3 files changed, 54 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> >> video_device *vdev, unsigned cmd) return vdev->lock;
> >>
> >> }
> >>
> >> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >> + struct v4l2_input *i)
> >> +{
> >> + if (i->index > 0)
> >> + return -EINVAL;
> >> +
> >> + memset(i, 0, sizeof(*i));
> >> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> >> + strlcpy(i->name, "Default", sizeof(i->name));
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> >
> > V4L2 tends to use EXPORT_SYMBOL_GPL.
>
> The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
> EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
> another patch) ?
You're right, let's leave it as-is then.
> > What would you think about calling those default functions directly from
> > the core when the input ioctl handlers are not set ? You wouldn't need to
> > modify drivers.
>
> Sure, I'll add them in ops inside __video_register_device when it
> validates the ioctls
>
> >> +
> >> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
> >> int
> >> *i) +{
> >> + *i = 0;
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> >> +
> >> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
> >> int
> >> i) +{
> >> + return i ? -EINVAL : 0;
> >> +}
> >> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> >> +
> >>
> >> /* Common ioctl debug function. This function can be used by
> >>
> >> external ioctl messages as well as internal V4L ioctl */
> >>
> >> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> >>
> >> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >> index 6cd94e5..accc470 100644
> >> --- a/include/media/v4l2-ioctl.h
> >> +++ b/include/media/v4l2-ioctl.h
> >> @@ -652,6 +652,32 @@ struct video_device;
> >>
> >> */
> >>
> >> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
> >> int
> >>
> >> cmd);
> >>
> >> +
> >> +/**
> >> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
> >> VIDIOC_ENUM_INPUT
> >> ioctl + *
> >> + * Plug this function in vidioc_enum_input field of the struct
> >> v4l2_ioctl_ops to + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> >> + */
> >> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >> + struct v4l2_input *i);
> >> +
> >> +/**
> >> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
> >> ioctl
> >> + *
> >> + * Plug this function in vidioc_g_input field of the struct
> >> v4l2_ioctl_ops
> >> + * when using v4l2_ioctl_enum_input_default
> >> + */
> >> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
> >> int
> >> *i); +
> >> +/**
> >> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
> >> ioctl
> >> + *
> >> + * Plug this function in vidioc_s_input field of the struct
> >> v4l2_ioctl_ops
> >> + * when using v4l2_ioctl_enum_input_default
> >> + */
> >> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
> >> int
> >> i); +
> >>
> >> /* names for fancy debug output */
> >> extern const char *v4l2_field_names[];
> >> extern const char *v4l2_type_names[];
> >>
> >> diff --git a/include/uapi/linux/videodev2.h
> >> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -1477,6 +1477,7 @@ struct v4l2_input {
> >>
> >> };
> >>
> >> /* Values for the 'type' field */
> >>
> >> +#define V4L2_INPUT_TYPE_DEFAULT 0
> >>
> >> #define V4L2_INPUT_TYPE_TUNER 1
> >> #define V4L2_INPUT_TYPE_CAMERA 2
> >> #define V4L2_INPUT_TYPE_TOUCH 3
>
> Helen
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 8:41 ` Laurent Pinchart
@ 2017-03-31 9:08 ` Hans Verkuil
0 siblings, 0 replies; 28+ messages in thread
From: Hans Verkuil @ 2017-03-31 9:08 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Helen Koike, Sakari Ailus, Mauro Carvalho Chehab, linux-media, jgebben
On 31/03/17 10:41, Laurent Pinchart wrote:
> Hi Hans,
>
> On Friday 31 Mar 2017 10:29:04 Hans Verkuil wrote:
>> On 30/03/17 18:02, Helen Koike wrote:
>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>> used when no inputs are available in the device
>>>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>> include/uapi/linux/videodev2.h | 1 +
>>> 3 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>> video_device *vdev, unsigned cmd)>
>>> return vdev->lock;
>>>
>>> }
>>>
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i)
>>> +{
>>> + if (i->index > 0)
>>> + return -EINVAL;
>>> +
>>> + memset(i, 0, sizeof(*i));
>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>> +
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
>>> int *i)
>>> +{
>>> + *i = 0;
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>> +
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
>>> int i)
>>> +{
>>> + return i ? -EINVAL : 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>> +
>>> /* Common ioctl debug function. This function can be used by
>>> external ioctl messages as well as internal V4L ioctl */
>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index 6cd94e5..accc470 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -652,6 +652,32 @@ struct video_device;
>>> */
>>>
>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int
>>> cmd);
>>> +
>>> +/**
>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
>>> VIDIOC_ENUM_INPUT ioctl + *
>>> + * Plug this function in vidioc_enum_input field of the struct
>>> v4l2_ioctl_ops to + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>> + */
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
>>> ioctl
>>> + *
>>> + * Plug this function in vidioc_g_input field of the struct
>>> v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned
>>> int *i); +
>>> +/**
>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
>>> ioctl
>>> + *
>>> + * Plug this function in vidioc_s_input field of the struct
>>> v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned
>>> int i); +
>>>
>>> /* names for fancy debug output */
>>> extern const char *v4l2_field_names[];
>>> extern const char *v4l2_type_names[];
>>>
>>> diff --git a/include/uapi/linux/videodev2.h
>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>>
>>> };
>>>
>>> /* Values for the 'type' field */
>>>
>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>
>> I don't think we should add a new type here. The whole point of this
>> exercise is to allow existing apps to work, and existing apps expect a
>> TYPE_CAMERA.
>>
>> BTW, don't read to much in the term 'CAMERA': it's really a catch all for
>> any video stream, whether it is from a sensor, composite input, HDMI, etc.
>>
>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of
>> date :-(
>>
>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability
>> that indicates that this input is controlled via the media controller. That
>> makes much more sense and it wouldn't potentially break applications.
>>
>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and
>> use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated
>> name and the spec is again out of date).
>
> What would those capabilities be used for ? Applications can already know
> whether the MC API is used by a driver. Furthermore, if we really need such a
> flag, I wouldn't add it at the input/output level but as a video node
> capability flag.
I have no objection to adding a V4L2_CAP_MC capability instead. In fact, I
think that would be a good idea and better than adding IN/OUT_CAP_MC.
>> Regarding the name: should we use the name stored in struct video_device
>> instead? That might be more descriptive. Alternatively use something like
>> "Media Controller Input".
>>
>> More helpful (perhaps) than just "Default" or "Unknown".
>
> If the purpose of the name field is to be displayed as-is to the end-user,
> then "Media Controller Input" is as useless as "Unknown". "Default" would be
> slightly better.
I would like to have something that indicates to the user that this input is
controlled via the MC. "Default" doesn't do that. I'm honestly not certain
what would be a good description. Frankly, I think that copying the video_device
name might be the better solution here. At least that way the name can actually
map to something sensible.
Regards,
Hans
>
>> I'll make a patch to update the input/output type description in the spec.
>>
>>> #define V4L2_INPUT_TYPE_TUNER 1
>>> #define V4L2_INPUT_TYPE_CAMERA 2
>>> #define V4L2_INPUT_TYPE_TOUCH 3
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 8:41 ` Laurent Pinchart
@ 2017-03-31 9:33 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 28+ messages in thread
From: Mauro Carvalho Chehab @ 2017-03-31 9:33 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Helen Koike, Sakari Ailus, Hans Verkuil, linux-media, jgebben
Em Fri, 31 Mar 2017 11:41:51 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> Hi Helen,
>
> On Thursday 30 Mar 2017 23:39:01 Helen Koike wrote:
> > On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
> > > On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
> > >> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> > >> used when no inputs are available in the device
> > >>
> > >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > >> ---
> > >>
> > >> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> > >> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> > >> include/uapi/linux/videodev2.h | 1 +
> > >> 3 files changed, 54 insertions(+)
> > >>
> > >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > >> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > >> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> > >> video_device *vdev, unsigned cmd) return vdev->lock;
> > >>
> > >> }
> > >>
> > >> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > >> + struct v4l2_input *i)
> > >> +{
> > >> + if (i->index > 0)
> > >> + return -EINVAL;
> > >> +
> > >> + memset(i, 0, sizeof(*i));
> > >> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> > >> + strlcpy(i->name, "Default", sizeof(i->name));
> > >> +
> > >> + return 0;
> > >> +}
> > >> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> > >
> > > V4L2 tends to use EXPORT_SYMBOL_GPL.
> >
> > The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
> > EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
> > another patch) ?
>
> You're right, let's leave it as-is then.
At the time V4L2 was written, there was no EXPORT_SYMBOL_GPL(). That's
why there are some parts that aren't explicit about the symbol usage
license.
For newer symbols, we're using EXPORT_SYMBOL_GPL(), in order to let
clear about the licensing for the code.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 3:55 ` Helen Koike
@ 2017-03-31 9:42 ` Mauro Carvalho Chehab
2017-03-31 9:51 ` Hans Verkuil
0 siblings, 1 reply; 28+ messages in thread
From: Mauro Carvalho Chehab @ 2017-03-31 9:42 UTC (permalink / raw)
To: Helen Koike
Cc: Laurent Pinchart, Sakari Ailus, Hans Verkuil, linux-media, jgebben
Em Fri, 31 Mar 2017 00:55:12 -0300
Helen Koike <helen.koike@collabora.com> escreveu:
> On 2017-03-30 11:39 PM, Helen Koike wrote:
> > Hi Laurent,
> >
> > Thanks for reviewing
> >
> > On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
> >> Hi Helen,
> >>
> >> Thank you for the patch.
> >>
> >> On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
> >>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> >>> used when no inputs are available in the device
> >>>
> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> >>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> >>> include/uapi/linux/videodev2.h | 1 +
> >>> 3 files changed, 54 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> >>> video_device
> >>> *vdev, unsigned cmd) return vdev->lock;
> >>> }
> >>>
> >>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>> + struct v4l2_input *i)
> >>> +{
> >>> + if (i->index > 0)
> >>> + return -EINVAL;
> >>> +
> >>> + memset(i, 0, sizeof(*i));
> >>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> >>> + strlcpy(i->name, "Default", sizeof(i->name));
> >>> +
> >>> + return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> >>
> >> V4L2 tends to use EXPORT_SYMBOL_GPL.
> >
> > The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
> > EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
> > another patch) ?
> >
> >>
> >> What would you think about calling those default functions directly
> >> from the
> >> core when the input ioctl handlers are not set ? You wouldn't need to
> >> modify
> >> drivers.
> >
> > Sure, I'll add them in ops inside __video_register_device when it
> > validates the ioctls
>
> I just realize I can not simply override struct v4l2_ioctl_ops as it is
> declared as a const inside strut video_device. I'll call those default
> functions only when the ioctls are handled to not modify vdev->ops.
You should not override it, anyway. What you should do, instead, is
something like:
static int v4l_ginput(const struct v4l2_ioctl_ops *ops, ...)
{
if (ops->vidioc_ginput)
return ops->vidioc_ginput(...);
/* default code */
}
You should also make sure that the ioctls are alway valid, e. g. by
calling SET_VALID_IOCTL()
Thanks,
Mauro
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] [media] docs-rst: add V4L2_INPUT_TYPE_DEFAULT
2017-03-30 20:26 ` Sakari Ailus
@ 2017-03-31 9:47 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 28+ messages in thread
From: Mauro Carvalho Chehab @ 2017-03-31 9:47 UTC (permalink / raw)
To: Sakari Ailus
Cc: Helen Koike, Hans Verkuil, linux-media, jgebben, Laurent Pinchart
Em Thu, 30 Mar 2017 23:26:26 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> Hi Helen and others,
>
> On Thu, Mar 30, 2017 at 01:02:18PM -0300, Helen Koike wrote:
> > add documentation for V4L2_INPUT_TYPE_DEFAULT
> >
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > ---
> > Documentation/media/uapi/v4l/vidioc-enuminput.rst | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/vidioc-enuminput.rst b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> > index 17aaaf9..0237e10 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-enuminput.rst
> > @@ -112,6 +112,9 @@ at index zero, incrementing by one until the driver returns ``EINVAL``.
> > :stub-columns: 0
> > :widths: 3 1 4
> >
> > + * - ``V4L2_INPUT_TYPE_DEFAULT``
> > + - 0
> > + - This is the default value returned when no input is supported.
Input *IS* supported. The device has one input. So, the description is wrong ;)
> > * - ``V4L2_INPUT_TYPE_TUNER``
> > - 1
> > - This input uses a tuner (RF demodulator).
>
> What would you think of calling this input as "unknown" instead of
> "default"? That's what an input which isn't really specified actually is.
Yeah, default seems a bad name to me, too.
Actually, I think that the best here would be to not create a new type.
just use V4L2_INPUT_TYPE_CAMERA. That's actually the default for the
embedded drivers. If a driver is providing input for something else,
then it should implement vidioc_enuminput method.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 9:42 ` Mauro Carvalho Chehab
@ 2017-03-31 9:51 ` Hans Verkuil
0 siblings, 0 replies; 28+ messages in thread
From: Hans Verkuil @ 2017-03-31 9:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Helen Koike
Cc: Laurent Pinchart, Sakari Ailus, linux-media, jgebben
On 31/03/17 11:42, Mauro Carvalho Chehab wrote:
> Em Fri, 31 Mar 2017 00:55:12 -0300
> Helen Koike <helen.koike@collabora.com> escreveu:
>
>> On 2017-03-30 11:39 PM, Helen Koike wrote:
>>> Hi Laurent,
>>>
>>> Thanks for reviewing
>>>
>>> On 2017-03-30 04:56 PM, Laurent Pinchart wrote:
>>>> Hi Helen,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Thursday 30 Mar 2017 13:02:17 Helen Koike wrote:
>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>>>> used when no inputs are available in the device
>>>>>
>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>> ---
>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>>>> include/uapi/linux/videodev2.h | 1 +
>>>>> 3 files changed, 54 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>>>> video_device
>>>>> *vdev, unsigned cmd) return vdev->lock;
>>>>> }
>>>>>
>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>> + struct v4l2_input *i)
>>>>> +{
>>>>> + if (i->index > 0)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + memset(i, 0, sizeof(*i));
>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>>>
>>>> V4L2 tends to use EXPORT_SYMBOL_GPL.
>>>
>>> The whole v4l2-ioctl.c file is using EXPORT_SYMBOL instead of
>>> EXPORT_SYMBOL_GPL, should we change it all to EXPORT_SYMBOL_GPL then (in
>>> another patch) ?
>>>
>>>>
>>>> What would you think about calling those default functions directly
>>>> from the
>>>> core when the input ioctl handlers are not set ? You wouldn't need to
>>>> modify
>>>> drivers.
>>>
>>> Sure, I'll add them in ops inside __video_register_device when it
>>> validates the ioctls
>>
>> I just realize I can not simply override struct v4l2_ioctl_ops as it is
>> declared as a const inside strut video_device. I'll call those default
>> functions only when the ioctls are handled to not modify vdev->ops.
>
> You should not override it, anyway. What you should do, instead, is
> something like:
>
> static int v4l_ginput(const struct v4l2_ioctl_ops *ops, ...)
> {
> if (ops->vidioc_ginput)
> return ops->vidioc_ginput(...);
>
> /* default code */
> }
>
> You should also make sure that the ioctls are alway valid, e. g. by
> calling SET_VALID_IOCTL()
Helen, FYI:
The input ioctls are compulsory for:
V4L2_CAP_VIDEO_CAPTURE
V4L2_CAP_VBI_CAPTURE
V4L2_CAP_VIDEO_CAPTURE_MPLANE
V4L2_CAP_SLICED_VBI_CAPTURE
The output ioctls are compulsory for:
V4L2_CAP_VIDEO_OUTPUT
V4L2_CAP_VBI_OUTPUT
V4L2_CAP_VIDEO_OUTPUT_MPLANE
V4L2_CAP_SLICED_VBI_OUTPUT
If none of these caps are set, then we shouldn't provide these stubs.
Regards,
Hans
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 8:29 ` Hans Verkuil
2017-03-31 8:41 ` Laurent Pinchart
@ 2017-03-31 9:57 ` Mauro Carvalho Chehab
2017-04-03 15:11 ` Helen Koike
2017-04-07 9:36 ` Hans Verkuil
1 sibling, 2 replies; 28+ messages in thread
From: Mauro Carvalho Chehab @ 2017-03-31 9:57 UTC (permalink / raw)
To: Hans Verkuil
Cc: Helen Koike, Sakari Ailus, linux-media, jgebben, Laurent Pinchart
Em Fri, 31 Mar 2017 10:29:04 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 30/03/17 18:02, Helen Koike wrote:
> > Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> > used when no inputs are available in the device
> >
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > ---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> > include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> > include/uapi/linux/videodev2.h | 1 +
> > 3 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 0c3f238..ccaf04b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
> > return vdev->lock;
> > }
> >
> > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > + struct v4l2_input *i)
> > +{
> > + if (i->index > 0)
> > + return -EINVAL;
> > +
> > + memset(i, 0, sizeof(*i));
> > + i->type = V4L2_INPUT_TYPE_DEFAULT;
> > + strlcpy(i->name, "Default", sizeof(i->name));
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> > +
> > +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
> > +{
> > + *i = 0;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> > +
> > +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
> > +{
> > + return i ? -EINVAL : 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> > +
> > /* Common ioctl debug function. This function can be used by
> > external ioctl messages as well as internal V4L ioctl */
> > void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index 6cd94e5..accc470 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -652,6 +652,32 @@ struct video_device;
> > */
> > struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
> >
> > +
> > +/**
> > + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
> > + *
> > + * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
> > + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> > + */
> > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > + struct v4l2_input *i);
> > +
> > +/**
> > + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
> > + *
> > + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
> > + * when using v4l2_ioctl_enum_input_default
> > + */
> > +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
> > +
> > +/**
> > + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
> > + *
> > + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
> > + * when using v4l2_ioctl_enum_input_default
> > + */
> > +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
> > +
> > /* names for fancy debug output */
> > extern const char *v4l2_field_names[];
> > extern const char *v4l2_type_names[];
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 316be62..c10bbde 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1477,6 +1477,7 @@ struct v4l2_input {
> > };
> >
> > /* Values for the 'type' field */
> > +#define V4L2_INPUT_TYPE_DEFAULT 0
>
> I don't think we should add a new type here.
I second that. Just replied the same thing on a comment from Sakari to
patch 2/2.
> The whole point of this exercise is to
> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>
> BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
> stream, whether it is from a sensor, composite input, HDMI, etc.
>
> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
Yeah, we always used "CAMERA" to mean NOT_TUNER.
> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
> indicates that this input is controlled via the media controller. That makes much
> more sense and it wouldn't potentially break applications.
>
> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
> V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
> spec is again out of date).
I don't see any sense on distinguishing IN and OUT for MC. I mean: should
we ever allow that any driver to have their inputs controlled via V4L2 API,
and their outputs controlled via MC (or vice-versa)? I don't think so.
Either all device inputs/outputs are controlled via V4L2 or via MC. So,
let's call it just V4L2_CAP_MC.
> Regarding the name: should we use the name stored in struct video_device instead?
> That might be more descriptive.
Makes sense to me.
> Alternatively use something like "Media Controller Input".
Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
stored at video_device.
> More helpful (perhaps) than just "Default" or "Unknown".
>
> I'll make a patch to update the input/output type description in the spec.
Thanks!
>
> Regards,
>
> Hans
>
> > #define V4L2_INPUT_TYPE_TUNER 1
> > #define V4L2_INPUT_TYPE_CAMERA 2
> > #define V4L2_INPUT_TYPE_TOUCH 3
> >
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 9:57 ` Mauro Carvalho Chehab
@ 2017-04-03 15:11 ` Helen Koike
2017-04-04 13:22 ` Sakari Ailus
2017-04-07 9:36 ` Hans Verkuil
1 sibling, 1 reply; 28+ messages in thread
From: Helen Koike @ 2017-04-03 15:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans Verkuil
Cc: Sakari Ailus, linux-media, jgebben, Laurent Pinchart
Hi,
On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
> Em Fri, 31 Mar 2017 10:29:04 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On 30/03/17 18:02, Helen Koike wrote:
>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>> used when no inputs are available in the device
>>>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>> include/uapi/linux/videodev2.h | 1 +
>>> 3 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 0c3f238..ccaf04b 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
>>> return vdev->lock;
>>> }
>>>
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i)
>>> +{
>>> + if (i->index > 0)
>>> + return -EINVAL;
>>> +
>>> + memset(i, 0, sizeof(*i));
>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>> +
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
>>> +{
>>> + *i = 0;
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>> +
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
>>> +{
>>> + return i ? -EINVAL : 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>> +
>>> /* Common ioctl debug function. This function can be used by
>>> external ioctl messages as well as internal V4L ioctl */
>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index 6cd94e5..accc470 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -652,6 +652,32 @@ struct video_device;
>>> */
>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
>>>
>>> +
>>> +/**
>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>> + */
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
>>> +
>>> /* names for fancy debug output */
>>> extern const char *v4l2_field_names[];
>>> extern const char *v4l2_type_names[];
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 316be62..c10bbde 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>> };
>>>
>>> /* Values for the 'type' field */
>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>
>> I don't think we should add a new type here.
>
> I second that. Just replied the same thing on a comment from Sakari to
> patch 2/2.
>
>> The whole point of this exercise is to
>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>
>> BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
>> stream, whether it is from a sensor, composite input, HDMI, etc.
>>
>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
>
> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>
>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
>> indicates that this input is controlled via the media controller. That makes much
>> more sense and it wouldn't potentially break applications.
>>
>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
>> V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
>> spec is again out of date).
>
> I don't see any sense on distinguishing IN and OUT for MC. I mean: should
> we ever allow that any driver to have their inputs controlled via V4L2 API,
> and their outputs controlled via MC (or vice-versa)? I don't think so.
>
> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> let's call it just V4L2_CAP_MC.
>
>> Regarding the name: should we use the name stored in struct video_device instead?
>> That might be more descriptive.
>
> Makes sense to me.
>
>> Alternatively use something like "Media Controller Input".
>
> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> stored at video_device.
Just to clarify: the V4L2_CAP_MC would indicated that the media
controller is enabled in general? Or just for inputs and outputs?
If it is the first case, not necessarily the inputs/outputs are
controlled via MC (we can still have a MC capable driver, but
inputs/outputs controlled via V4L2 no? When the driver doesn't offer the
necessary link controls via MC), then checking if V4L2_CAP_MC then use
the name "Media Controller Input" is not enough.
If it is the second case, then wouldn't it be better to name it
V4L2_CAP_MC_IO ?
Thanks
Helen
>
>> More helpful (perhaps) than just "Default" or "Unknown".
>>
>> I'll make a patch to update the input/output type description in the spec.
>
> Thanks!
>
>>
>> Regards,
>>
>> Hans
>>
>>> #define V4L2_INPUT_TYPE_TUNER 1
>>> #define V4L2_INPUT_TYPE_CAMERA 2
>>> #define V4L2_INPUT_TYPE_TOUCH 3
>>>
>>
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-04-03 15:11 ` Helen Koike
@ 2017-04-04 13:22 ` Sakari Ailus
2017-04-07 9:46 ` Hans Verkuil
0 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2017-04-04 13:22 UTC (permalink / raw)
To: Helen Koike
Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-media, jgebben,
Laurent Pinchart
Hi Helen,
On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
> Hi,
>
> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
> >Em Fri, 31 Mar 2017 10:29:04 +0200
> >Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >>On 30/03/17 18:02, Helen Koike wrote:
> >>>Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
> >>>used when no inputs are available in the device
> >>>
> >>>Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>---
> >>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> >>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> >>> include/uapi/linux/videodev2.h | 1 +
> >>> 3 files changed, 54 insertions(+)
> >>>
> >>>diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>index 0c3f238..ccaf04b 100644
> >>>--- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>@@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
> >>> return vdev->lock;
> >>> }
> >>>
> >>>+int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>+ struct v4l2_input *i)
> >>>+{
> >>>+ if (i->index > 0)
> >>>+ return -EINVAL;
> >>>+
> >>>+ memset(i, 0, sizeof(*i));
> >>>+ i->type = V4L2_INPUT_TYPE_DEFAULT;
> >>>+ strlcpy(i->name, "Default", sizeof(i->name));
> >>>+
> >>>+ return 0;
> >>>+}
> >>>+EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> >>>+
> >>>+int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
> >>>+{
> >>>+ *i = 0;
> >>>+ return 0;
> >>>+}
> >>>+EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> >>>+
> >>>+int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
> >>>+{
> >>>+ return i ? -EINVAL : 0;
> >>>+}
> >>>+EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> >>>+
> >>> /* Common ioctl debug function. This function can be used by
> >>> external ioctl messages as well as internal V4L ioctl */
> >>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> >>>diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >>>index 6cd94e5..accc470 100644
> >>>--- a/include/media/v4l2-ioctl.h
> >>>+++ b/include/media/v4l2-ioctl.h
> >>>@@ -652,6 +652,32 @@ struct video_device;
> >>> */
> >>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
> >>>
> >>>+
> >>>+/**
> >>>+ * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
> >>>+ *
> >>>+ * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
> >>>+ * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> >>>+ */
> >>>+int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>+ struct v4l2_input *i);
> >>>+
> >>>+/**
> >>>+ * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
> >>>+ *
> >>>+ * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
> >>>+ * when using v4l2_ioctl_enum_input_default
> >>>+ */
> >>>+int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
> >>>+
> >>>+/**
> >>>+ * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
> >>>+ *
> >>>+ * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
> >>>+ * when using v4l2_ioctl_enum_input_default
> >>>+ */
> >>>+int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
> >>>+
> >>> /* names for fancy debug output */
> >>> extern const char *v4l2_field_names[];
> >>> extern const char *v4l2_type_names[];
> >>>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>>index 316be62..c10bbde 100644
> >>>--- a/include/uapi/linux/videodev2.h
> >>>+++ b/include/uapi/linux/videodev2.h
> >>>@@ -1477,6 +1477,7 @@ struct v4l2_input {
> >>> };
> >>>
> >>> /* Values for the 'type' field */
> >>>+#define V4L2_INPUT_TYPE_DEFAULT 0
> >>
> >>I don't think we should add a new type here.
> >
> >I second that. Just replied the same thing on a comment from Sakari to
> >patch 2/2.
> >
> >>The whole point of this exercise is to
> >>allow existing apps to work, and existing apps expect a TYPE_CAMERA.
> >>
> >>BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
> >>stream, whether it is from a sensor, composite input, HDMI, etc.
> >>
> >>The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
> >
> >Yeah, we always used "CAMERA" to mean NOT_TUNER.
> >
> >>Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
> >>indicates that this input is controlled via the media controller. That makes much
> >>more sense and it wouldn't potentially break applications.
> >>
> >>Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
> >>V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
> >>spec is again out of date).
> >
> >I don't see any sense on distinguishing IN and OUT for MC. I mean: should
> >we ever allow that any driver to have their inputs controlled via V4L2 API,
> >and their outputs controlled via MC (or vice-versa)? I don't think so.
> >
> >Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> >let's call it just V4L2_CAP_MC.
> >
> >>Regarding the name: should we use the name stored in struct video_device instead?
> >>That might be more descriptive.
> >
> >Makes sense to me.
> >
> >>Alternatively use something like "Media Controller Input".
> >
> >Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> >stored at video_device.
>
> Just to clarify: the V4L2_CAP_MC would indicated that the media controller
> is enabled in general? Or just for inputs and outputs?
I let Mauro and Hans to comment on their own behalf, but I think whatever is
communicated through the input IOCTLs should be applicable to inputs only.
The fact that the video device is a part of an MC graph could be conveyed
using a capability flag. Or by providing information on the media device
node, something that has been proposed earlier on. Either is out of the
scope of this patchset IMO, but should be addressed separately.
> If it is the first case, not necessarily the inputs/outputs are controlled
> via MC (we can still have a MC capable driver, but inputs/outputs controlled
> via V4L2 no? When the driver doesn't offer the necessary link controls via
> MC), then checking if V4L2_CAP_MC then use the name "Media Controller Input"
> is not enough.
> If it is the second case, then wouldn't it be better to name it
> V4L2_CAP_MC_IO ?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-03-31 9:57 ` Mauro Carvalho Chehab
2017-04-03 15:11 ` Helen Koike
@ 2017-04-07 9:36 ` Hans Verkuil
1 sibling, 0 replies; 28+ messages in thread
From: Hans Verkuil @ 2017-04-07 9:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Helen Koike, Sakari Ailus, linux-media, jgebben, Laurent Pinchart
On 03/31/2017 11:57 AM, Mauro Carvalho Chehab wrote:
> Em Fri, 31 Mar 2017 10:29:04 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On 30/03/17 18:02, Helen Koike wrote:
>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>> used when no inputs are available in the device
>>>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>> include/uapi/linux/videodev2.h | 1 +
>>> 3 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 0c3f238..ccaf04b 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
>>> return vdev->lock;
>>> }
>>>
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i)
>>> +{
>>> + if (i->index > 0)
>>> + return -EINVAL;
>>> +
>>> + memset(i, 0, sizeof(*i));
>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>> +
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
>>> +{
>>> + *i = 0;
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>> +
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
>>> +{
>>> + return i ? -EINVAL : 0;
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>> +
>>> /* Common ioctl debug function. This function can be used by
>>> external ioctl messages as well as internal V4L ioctl */
>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index 6cd94e5..accc470 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -652,6 +652,32 @@ struct video_device;
>>> */
>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
>>>
>>> +
>>> +/**
>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>> + */
>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>> + struct v4l2_input *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
>>> +
>>> +/**
>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
>>> + *
>>> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
>>> + * when using v4l2_ioctl_enum_input_default
>>> + */
>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
>>> +
>>> /* names for fancy debug output */
>>> extern const char *v4l2_field_names[];
>>> extern const char *v4l2_type_names[];
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 316be62..c10bbde 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>> };
>>>
>>> /* Values for the 'type' field */
>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>
>> I don't think we should add a new type here.
>
> I second that. Just replied the same thing on a comment from Sakari to
> patch 2/2.
>
>> The whole point of this exercise is to
>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>
>> BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
>> stream, whether it is from a sensor, composite input, HDMI, etc.
>>
>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
>
> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>
>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
>> indicates that this input is controlled via the media controller. That makes much
>> more sense and it wouldn't potentially break applications.
>>
>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
>> V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
>> spec is again out of date).
>
> I don't see any sense on distinguishing IN and OUT for MC. I mean: should
> we ever allow that any driver to have their inputs controlled via V4L2 API,
> and their outputs controlled via MC (or vice-versa)? I don't think so.
It's historical: the V4L2_OUT_CAP_ defines are aliases of the V4L2_IN_CAP_ defines.
Regards,
Hans
>
> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> let's call it just V4L2_CAP_MC.
>
>> Regarding the name: should we use the name stored in struct video_device instead?
>> That might be more descriptive.
>
> Makes sense to me.
>
>> Alternatively use something like "Media Controller Input".
>
> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> stored at video_device.
>
>> More helpful (perhaps) than just "Default" or "Unknown".
>>
>> I'll make a patch to update the input/output type description in the spec.
>
> Thanks!
>
>>
>> Regards,
>>
>> Hans
>>
>>> #define V4L2_INPUT_TYPE_TUNER 1
>>> #define V4L2_INPUT_TYPE_CAMERA 2
>>> #define V4L2_INPUT_TYPE_TOUCH 3
>>>
>>
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-04-04 13:22 ` Sakari Ailus
@ 2017-04-07 9:46 ` Hans Verkuil
2017-04-07 9:53 ` Laurent Pinchart
0 siblings, 1 reply; 28+ messages in thread
From: Hans Verkuil @ 2017-04-07 9:46 UTC (permalink / raw)
To: Sakari Ailus, Helen Koike
Cc: Mauro Carvalho Chehab, linux-media, jgebben, Laurent Pinchart
On 04/04/2017 03:22 PM, Sakari Ailus wrote:
> Hi Helen,
>
> On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
>> Hi,
>>
>> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 31 Mar 2017 10:29:04 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> On 30/03/17 18:02, Helen Koike wrote:
>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to be
>>>>> used when no inputs are available in the device
>>>>>
>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>> ---
>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>>>> include/uapi/linux/videodev2.h | 1 +
>>>>> 3 files changed, 54 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> index 0c3f238..ccaf04b 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
>>>>> return vdev->lock;
>>>>> }
>>>>>
>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>> + struct v4l2_input *i)
>>>>> +{
>>>>> + if (i->index > 0)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + memset(i, 0, sizeof(*i));
>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>>>> +
>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i)
>>>>> +{
>>>>> + *i = 0;
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>>>> +
>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i)
>>>>> +{
>>>>> + return i ? -EINVAL : 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>>>> +
>>>>> /* Common ioctl debug function. This function can be used by
>>>>> external ioctl messages as well as internal V4L ioctl */
>>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>>>> index 6cd94e5..accc470 100644
>>>>> --- a/include/media/v4l2-ioctl.h
>>>>> +++ b/include/media/v4l2-ioctl.h
>>>>> @@ -652,6 +652,32 @@ struct video_device;
>>>>> */
>>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
>>>>>
>>>>> +
>>>>> +/**
>>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for VIDIOC_ENUM_INPUT ioctl
>>>>> + *
>>>>> + * Plug this function in vidioc_enum_input field of the struct v4l2_ioctl_ops to
>>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>>>> + */
>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>> + struct v4l2_input *i);
>>>>> +
>>>>> +/**
>>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT ioctl
>>>>> + *
>>>>> + * Plug this function in vidioc_g_input field of the struct v4l2_ioctl_ops
>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>> + */
>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv, unsigned int *i);
>>>>> +
>>>>> +/**
>>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT ioctl
>>>>> + *
>>>>> + * Plug this function in vidioc_s_input field of the struct v4l2_ioctl_ops
>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>> + */
>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv, unsigned int i);
>>>>> +
>>>>> /* names for fancy debug output */
>>>>> extern const char *v4l2_field_names[];
>>>>> extern const char *v4l2_type_names[];
>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>> index 316be62..c10bbde 100644
>>>>> --- a/include/uapi/linux/videodev2.h
>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>>>> };
>>>>>
>>>>> /* Values for the 'type' field */
>>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>>>
>>>> I don't think we should add a new type here.
>>>
>>> I second that. Just replied the same thing on a comment from Sakari to
>>> patch 2/2.
>>>
>>>> The whole point of this exercise is to
>>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>>>
>>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all for any video
>>>> stream, whether it is from a sensor, composite input, HDMI, etc.
>>>>
>>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly out of date :-(
>>>
>>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>>>
>>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC capability that
>>>> indicates that this input is controlled via the media controller. That makes much
>>>> more sense and it wouldn't potentially break applications.
>>>>
>>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC and use
>>>> V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible outdated name and the
>>>> spec is again out of date).
>>>
>>> I don't see any sense on distinguishing IN and OUT for MC. I mean: should
>>> we ever allow that any driver to have their inputs controlled via V4L2 API,
>>> and their outputs controlled via MC (or vice-versa)? I don't think so.
>>>
>>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
>>> let's call it just V4L2_CAP_MC.
>>>
>>>> Regarding the name: should we use the name stored in struct video_device instead?
>>>> That might be more descriptive.
>>>
>>> Makes sense to me.
>>>
>>>> Alternatively use something like "Media Controller Input".
>>>
>>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
>>> stored at video_device.
>>
>> Just to clarify: the V4L2_CAP_MC would indicated that the media controller
>> is enabled in general? Or just for inputs and outputs?
>
> I let Mauro and Hans to comment on their own behalf, but I think whatever is
> communicated through the input IOCTLs should be applicable to inputs only.
>
> The fact that the video device is a part of an MC graph could be conveyed
> using a capability flag. Or by providing information on the media device
> node, something that has been proposed earlier on. Either is out of the
> scope of this patchset IMO, but should be addressed separately.
>
>> If it is the first case, not necessarily the inputs/outputs are controlled
>> via MC (we can still have a MC capable driver, but inputs/outputs controlled
>> via V4L2 no? When the driver doesn't offer the necessary link controls via
>> MC), then checking if V4L2_CAP_MC then use the name "Media Controller Input"
>> is not enough.
>> If it is the second case, then wouldn't it be better to name it
>> V4L2_CAP_MC_IO ?
>
It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also feel
that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing V4L2_IN/OUT_CAP
flags make no sense in this case.
In v4l2-ioctl.c we can just check V4L2_CAP_IO_MC in video_device->device_caps,
and, if present, implement dummy s/g/enum_in/output ioctls. The enum ioctl would
set V4L2_IN/OUT_CAP_MC and use video_device->name as the description.
Regards,
Hans
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-04-07 9:46 ` Hans Verkuil
@ 2017-04-07 9:53 ` Laurent Pinchart
2017-06-06 16:22 ` Helen Koike
0 siblings, 1 reply; 28+ messages in thread
From: Laurent Pinchart @ 2017-04-07 9:53 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, Helen Koike, Mauro Carvalho Chehab, linux-media, jgebben
Hi Hans,
On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
> On 04/04/2017 03:22 PM, Sakari Ailus wrote:
> > On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
> >> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
> >>> Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
> >>>> On 30/03/17 18:02, Helen Koike wrote:
> >>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
> >>>>> be used when no inputs are available in the device
> >>>>>
> >>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>> ---
> >>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> >>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> >>>>> include/uapi/linux/videodev2.h | 1 +
> >>>>> 3 files changed, 54 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> >>>>> video_device *vdev, unsigned cmd)
> >>>>> return vdev->lock;
> >>>>> }
> >>>>>
> >>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>>> + struct v4l2_input *i)
> >>>>> +{
> >>>>> + if (i->index > 0)
> >>>>> + return -EINVAL;
> >>>>> +
> >>>>> + memset(i, 0, sizeof(*i));
> >>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> >>>>> + strlcpy(i->name, "Default", sizeof(i->name));
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> >>>>> +
> >>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> >>>>> unsigned int *i)
> >>>>> +{
> >>>>> + *i = 0;
> >>>>> + return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> >>>>> +
> >>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> >>>>> unsigned int i)
> >>>>> +{
> >>>>> + return i ? -EINVAL : 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> >>>>> +
> >>>>> /* Common ioctl debug function. This function can be used by
> >>>>> external ioctl messages as well as internal V4L ioctl */
> >>>>>
> >>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> >>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >>>>> index 6cd94e5..accc470 100644
> >>>>> --- a/include/media/v4l2-ioctl.h
> >>>>> +++ b/include/media/v4l2-ioctl.h
> >>>>> @@ -652,6 +652,32 @@ struct video_device;
> >>>>> */
> >>>>>
> >>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
> >>>>> int cmd);
> >>>>> +
> >>>>> +/**
> >>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
> >>>>> VIDIOC_ENUM_INPUT ioctl
> >>>>> + *
> >>>>> + * Plug this function in vidioc_enum_input field of the struct
> >>>>> v4l2_ioctl_ops to
> >>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> >>>>> + */
> >>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>>> + struct v4l2_input *i);
> >>>>> +
> >>>>> +/**
> >>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
> >>>>> ioctl
> >>>>> + *
> >>>>> + * Plug this function in vidioc_g_input field of the struct
> >>>>> v4l2_ioctl_ops
> >>>>> + * when using v4l2_ioctl_enum_input_default
> >>>>> + */
> >>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> >>>>> unsigned int *i);
> >>>>> +
> >>>>> +/**
> >>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
> >>>>> ioctl
> >>>>> + *
> >>>>> + * Plug this function in vidioc_s_input field of the struct
> >>>>> v4l2_ioctl_ops
> >>>>> + * when using v4l2_ioctl_enum_input_default
> >>>>> + */
> >>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> >>>>> unsigned int i);
> >>>>> +
> >>>>> /* names for fancy debug output */
> >>>>> extern const char *v4l2_field_names[];
> >>>>> extern const char *v4l2_type_names[];
> >>>>> diff --git a/include/uapi/linux/videodev2.h
> >>>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
> >>>>> --- a/include/uapi/linux/videodev2.h
> >>>>> +++ b/include/uapi/linux/videodev2.h
> >>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
> >>>>> };
> >>>>>
> >>>>> /* Values for the 'type' field */
> >>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
> >>>>
> >>>> I don't think we should add a new type here.
> >>>
> >>> I second that. Just replied the same thing on a comment from Sakari to
> >>> patch 2/2.
> >>>
> >>>> The whole point of this exercise is to
> >>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
> >>>>
> >>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all
> >>>> for any video stream, whether it is from a sensor, composite input,
> >>>> HDMI, etc.
> >>>>
> >>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
> >>>> out of date :-(
> >>>
> >>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
> >>>
> >>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC
> >>>> capability that indicates that this input is controlled via the media
> >>>> controller. That makes much more sense and it wouldn't potentially
> >>>> break applications.
> >>>>
> >>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
> >>>> and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
> >>>> outdated name and the spec is again out of date).
> >>>
> >>> I don't see any sense on distinguishing IN and OUT for MC. I mean:
> >>> should
> >>> we ever allow that any driver to have their inputs controlled via V4L2
> >>> API,
> >>> and their outputs controlled via MC (or vice-versa)? I don't think so.
> >>>
> >>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> >>> let's call it just V4L2_CAP_MC.
> >>>
> >>>> Regarding the name: should we use the name stored in struct
> >>>> video_device instead? That might be more descriptive.
> >>>
> >>> Makes sense to me.
> >>>
> >>>> Alternatively use something like "Media Controller Input".
> >>>
> >>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> >>> stored at video_device.
> >>
> >> Just to clarify: the V4L2_CAP_MC would indicated that the media
> >> controller
> >> is enabled in general? Or just for inputs and outputs?
> >
> > I let Mauro and Hans to comment on their own behalf, but I think whatever
> > is communicated through the input IOCTLs should be applicable to inputs
> > only.
> >
> > The fact that the video device is a part of an MC graph could be conveyed
> > using a capability flag. Or by providing information on the media device
> > node, something that has been proposed earlier on. Either is out of the
> > scope of this patchset IMO, but should be addressed separately.
> >
> >> If it is the first case, not necessarily the inputs/outputs are
> >> controlled
> >> via MC (we can still have a MC capable driver, but inputs/outputs
> >> controlled via V4L2 no? When the driver doesn't offer the necessary link
> >> controls via MC), then checking if V4L2_CAP_MC then use the name "Media
> >> Controller Input" is not enough.
> >> If it is the second case, then wouldn't it be better to name it
> >> V4L2_CAP_MC_IO ?
>
> It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
> feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
> V4L2_IN/OUT_CAP flags make no sense in this case.
I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
of view. I'd rather avoid adding flags unless there's a real use for them.
> In v4l2-ioctl.c we can just check V4L2_CAP_IO_MC in
> video_device->device_caps, and, if present, implement dummy
> s/g/enum_in/output ioctls. The enum ioctl would set V4L2_IN/OUT_CAP_MC and
> use video_device->name as the description.
We still haven't agreed on what the description should be used for. If it is
to be presented to a user through an input/output selection UI, video_device-
>name doesn't seem very useful.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-04-07 9:53 ` Laurent Pinchart
@ 2017-06-06 16:22 ` Helen Koike
2017-06-12 9:26 ` Hans Verkuil
0 siblings, 1 reply; 28+ messages in thread
From: Helen Koike @ 2017-06-06 16:22 UTC (permalink / raw)
To: Laurent Pinchart, Hans Verkuil
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, jgebben
Hi All,
Just reviving this discussion
On 2017-04-07 06:53 AM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
>> On 04/04/2017 03:22 PM, Sakari Ailus wrote:
>>> On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
>>>> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
>>>>> Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
>>>>>> On 30/03/17 18:02, Helen Koike wrote:
>>>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
>>>>>>> be used when no inputs are available in the device
>>>>>>>
>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>> ---
>>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>>>>>> include/uapi/linux/videodev2.h | 1 +
>>>>>>> 3 files changed, 54 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>>>>>> video_device *vdev, unsigned cmd)
>>>>>>> return vdev->lock;
>>>>>>> }
>>>>>>>
>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>> + struct v4l2_input *i)
>>>>>>> +{
>>>>>>> + if (i->index > 0)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + memset(i, 0, sizeof(*i));
>>>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>>>>>> +
>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>> unsigned int *i)
>>>>>>> +{
>>>>>>> + *i = 0;
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>>>>>> +
>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>> unsigned int i)
>>>>>>> +{
>>>>>>> + return i ? -EINVAL : 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>>>>>> +
>>>>>>> /* Common ioctl debug function. This function can be used by
>>>>>>> external ioctl messages as well as internal V4L ioctl */
>>>>>>>
>>>>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>>>>>> index 6cd94e5..accc470 100644
>>>>>>> --- a/include/media/v4l2-ioctl.h
>>>>>>> +++ b/include/media/v4l2-ioctl.h
>>>>>>> @@ -652,6 +652,32 @@ struct video_device;
>>>>>>> */
>>>>>>>
>>>>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
>>>>>>> int cmd);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
>>>>>>> VIDIOC_ENUM_INPUT ioctl
>>>>>>> + *
>>>>>>> + * Plug this function in vidioc_enum_input field of the struct
>>>>>>> v4l2_ioctl_ops to
>>>>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>>>>>> + */
>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>> + struct v4l2_input *i);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
>>>>>>> ioctl
>>>>>>> + *
>>>>>>> + * Plug this function in vidioc_g_input field of the struct
>>>>>>> v4l2_ioctl_ops
>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>> + */
>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>> unsigned int *i);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
>>>>>>> ioctl
>>>>>>> + *
>>>>>>> + * Plug this function in vidioc_s_input field of the struct
>>>>>>> v4l2_ioctl_ops
>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>> + */
>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>> unsigned int i);
>>>>>>> +
>>>>>>> /* names for fancy debug output */
>>>>>>> extern const char *v4l2_field_names[];
>>>>>>> extern const char *v4l2_type_names[];
>>>>>>> diff --git a/include/uapi/linux/videodev2.h
>>>>>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>>>>>> };
>>>>>>>
>>>>>>> /* Values for the 'type' field */
>>>>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>>>>>
>>>>>> I don't think we should add a new type here.
>>>>>
>>>>> I second that. Just replied the same thing on a comment from Sakari to
>>>>> patch 2/2.
>>>>>
>>>>>> The whole point of this exercise is to
>>>>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>>>>>
>>>>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all
>>>>>> for any video stream, whether it is from a sensor, composite input,
>>>>>> HDMI, etc.
>>>>>>
>>>>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
>>>>>> out of date :-(
>>>>>
>>>>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>>>>>
>>>>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC
>>>>>> capability that indicates that this input is controlled via the media
>>>>>> controller. That makes much more sense and it wouldn't potentially
>>>>>> break applications.
>>>>>>
>>>>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
>>>>>> and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
>>>>>> outdated name and the spec is again out of date).
>>>>>
>>>>> I don't see any sense on distinguishing IN and OUT for MC. I mean:
>>>>> should
>>>>> we ever allow that any driver to have their inputs controlled via V4L2
>>>>> API,
>>>>> and their outputs controlled via MC (or vice-versa)? I don't think so.
>>>>>
>>>>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
>>>>> let's call it just V4L2_CAP_MC.
>>>>>
>>>>>> Regarding the name: should we use the name stored in struct
>>>>>> video_device instead? That might be more descriptive.
>>>>>
>>>>> Makes sense to me.
>>>>>
>>>>>> Alternatively use something like "Media Controller Input".
>>>>>
>>>>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
>>>>> stored at video_device.
>>>>
>>>> Just to clarify: the V4L2_CAP_MC would indicated that the media
>>>> controller
>>>> is enabled in general? Or just for inputs and outputs?
>>>
>>> I let Mauro and Hans to comment on their own behalf, but I think whatever
>>> is communicated through the input IOCTLs should be applicable to inputs
>>> only.
>>>
>>> The fact that the video device is a part of an MC graph could be conveyed
>>> using a capability flag. Or by providing information on the media device
>>> node, something that has been proposed earlier on. Either is out of the
>>> scope of this patchset IMO, but should be addressed separately.
>>>
>>>> If it is the first case, not necessarily the inputs/outputs are
>>>> controlled
>>>> via MC (we can still have a MC capable driver, but inputs/outputs
>>>> controlled via V4L2 no? When the driver doesn't offer the necessary link
>>>> controls via MC), then checking if V4L2_CAP_MC then use the name "Media
>>>> Controller Input" is not enough.
>>>> If it is the second case, then wouldn't it be better to name it
>>>> V4L2_CAP_MC_IO ?
>>
>> It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
>> feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
>> V4L2_IN/OUT_CAP flags make no sense in this case.
>
> I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
> of view. I'd rather avoid adding flags unless there's a real use for them.
Just to clarify, should this capability flag be set in struct
v4l2_input/struct v4l2_output through VIDIOC_ENUMINPUT/,
VIDIOC_ENUMOUTPUT? Or should it be set in struct v4l2_capability
through VIDIOC_QUERYCAP ?
Because if it is the first case, the I feel we should have two flags
V4L2_IN/OUT_CAP_MC in the API to follow the current convention, but this
kinda implies that we could have a driver that allows both flags to be
set differently.
Setting a V4L2_IO_CAP_MC at struct v4l2_capability would avoid this
interpretation.
>
>> In v4l2-ioctl.c we can just check V4L2_CAP_IO_MC in
>> video_device->device_caps, and, if present, implement dummy
>> s/g/enum_in/output ioctls. The enum ioctl would set V4L2_IN/OUT_CAP_MC and
>> use video_device->name as the description.
>
> We still haven't agreed on what the description should be used for. If it is
> to be presented to a user through an input/output selection UI, video_device-
>> name doesn't seem very useful.
>
video_device->name doesn't seems to let it clear that this input is
controlled by the Media framework as it will be a different name for
each driver, shouldn't this be a fixed name?
LN Koike
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-06-06 16:22 ` Helen Koike
@ 2017-06-12 9:26 ` Hans Verkuil
2017-06-30 11:03 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Hans Verkuil @ 2017-06-12 9:26 UTC (permalink / raw)
To: Helen Koike, Laurent Pinchart
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, jgebben
On 06/06/2017 06:22 PM, Helen Koike wrote:
> Hi All,
>
> Just reviving this discussion
>
> On 2017-04-07 06:53 AM, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
>>> On 04/04/2017 03:22 PM, Sakari Ailus wrote:
>>>> On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
>>>>> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
>>>>>> Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
>>>>>>> On 30/03/17 18:02, Helen Koike wrote:
>>>>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
>>>>>>>> be used when no inputs are available in the device
>>>>>>>>
>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>>> ---
>>>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>>>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>>>>>>> include/uapi/linux/videodev2.h | 1 +
>>>>>>>> 3 files changed, 54 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>>>>>>> video_device *vdev, unsigned cmd)
>>>>>>>> return vdev->lock;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>>> + struct v4l2_input *i)
>>>>>>>> +{
>>>>>>>> + if (i->index > 0)
>>>>>>>> + return -EINVAL;
>>>>>>>> +
>>>>>>>> + memset(i, 0, sizeof(*i));
>>>>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>>>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>>>>>>> +
>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>>> unsigned int *i)
>>>>>>>> +{
>>>>>>>> + *i = 0;
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>>>>>>> +
>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>>> unsigned int i)
>>>>>>>> +{
>>>>>>>> + return i ? -EINVAL : 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>>>>>>> +
>>>>>>>> /* Common ioctl debug function. This function can be used by
>>>>>>>> external ioctl messages as well as internal V4L ioctl */
>>>>>>>>
>>>>>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>>>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>>>>>>> index 6cd94e5..accc470 100644
>>>>>>>> --- a/include/media/v4l2-ioctl.h
>>>>>>>> +++ b/include/media/v4l2-ioctl.h
>>>>>>>> @@ -652,6 +652,32 @@ struct video_device;
>>>>>>>> */
>>>>>>>>
>>>>>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
>>>>>>>> int cmd);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
>>>>>>>> VIDIOC_ENUM_INPUT ioctl
>>>>>>>> + *
>>>>>>>> + * Plug this function in vidioc_enum_input field of the struct
>>>>>>>> v4l2_ioctl_ops to
>>>>>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>>>>>>> + */
>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>>> + struct v4l2_input *i);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
>>>>>>>> ioctl
>>>>>>>> + *
>>>>>>>> + * Plug this function in vidioc_g_input field of the struct
>>>>>>>> v4l2_ioctl_ops
>>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>>> + */
>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>>> unsigned int *i);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
>>>>>>>> ioctl
>>>>>>>> + *
>>>>>>>> + * Plug this function in vidioc_s_input field of the struct
>>>>>>>> v4l2_ioctl_ops
>>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>>> + */
>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>>> unsigned int i);
>>>>>>>> +
>>>>>>>> /* names for fancy debug output */
>>>>>>>> extern const char *v4l2_field_names[];
>>>>>>>> extern const char *v4l2_type_names[];
>>>>>>>> diff --git a/include/uapi/linux/videodev2.h
>>>>>>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>>>>>>> };
>>>>>>>>
>>>>>>>> /* Values for the 'type' field */
>>>>>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>>>>>>
>>>>>>> I don't think we should add a new type here.
>>>>>>
>>>>>> I second that. Just replied the same thing on a comment from Sakari to
>>>>>> patch 2/2.
>>>>>>
>>>>>>> The whole point of this exercise is to
>>>>>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>>>>>>
>>>>>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all
>>>>>>> for any video stream, whether it is from a sensor, composite input,
>>>>>>> HDMI, etc.
>>>>>>>
>>>>>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
>>>>>>> out of date :-(
>>>>>>
>>>>>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>>>>>>
>>>>>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC
>>>>>>> capability that indicates that this input is controlled via the media
>>>>>>> controller. That makes much more sense and it wouldn't potentially
>>>>>>> break applications.
>>>>>>>
>>>>>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
>>>>>>> and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
>>>>>>> outdated name and the spec is again out of date).
>>>>>>
>>>>>> I don't see any sense on distinguishing IN and OUT for MC. I mean:
>>>>>> should
>>>>>> we ever allow that any driver to have their inputs controlled via V4L2
>>>>>> API,
>>>>>> and their outputs controlled via MC (or vice-versa)? I don't think so.
>>>>>>
>>>>>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
>>>>>> let's call it just V4L2_CAP_MC.
>>>>>>
>>>>>>> Regarding the name: should we use the name stored in struct
>>>>>>> video_device instead? That might be more descriptive.
>>>>>>
>>>>>> Makes sense to me.
>>>>>>
>>>>>>> Alternatively use something like "Media Controller Input".
>>>>>>
>>>>>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
>>>>>> stored at video_device.
>>>>>
>>>>> Just to clarify: the V4L2_CAP_MC would indicated that the media
>>>>> controller
>>>>> is enabled in general? Or just for inputs and outputs?
>>>>
>>>> I let Mauro and Hans to comment on their own behalf, but I think whatever
>>>> is communicated through the input IOCTLs should be applicable to inputs
>>>> only.
>>>>
>>>> The fact that the video device is a part of an MC graph could be conveyed
>>>> using a capability flag. Or by providing information on the media device
>>>> node, something that has been proposed earlier on. Either is out of the
>>>> scope of this patchset IMO, but should be addressed separately.
>>>>
>>>>> If it is the first case, not necessarily the inputs/outputs are
>>>>> controlled
>>>>> via MC (we can still have a MC capable driver, but inputs/outputs
>>>>> controlled via V4L2 no? When the driver doesn't offer the necessary link
>>>>> controls via MC), then checking if V4L2_CAP_MC then use the name "Media
>>>>> Controller Input" is not enough.
>>>>> If it is the second case, then wouldn't it be better to name it
>>>>> V4L2_CAP_MC_IO ?
>>>
>>> It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
>>> feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
>>> V4L2_IN/OUT_CAP flags make no sense in this case.
>>
>> I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
>> of view. I'd rather avoid adding flags unless there's a real use for them.
>
>
> Just to clarify, should this capability flag be set in struct
> v4l2_input/struct v4l2_output through VIDIOC_ENUMINPUT/,
> VIDIOC_ENUMOUTPUT? Or should it be set in struct v4l2_capability
> through VIDIOC_QUERYCAP ?
> Because if it is the first case, the I feel we should have two flags
> V4L2_IN/OUT_CAP_MC in the API to follow the current convention, but this
> kinda implies that we could have a driver that allows both flags to be
> set differently.
> Setting a V4L2_IO_CAP_MC at struct v4l2_capability would avoid this
> interpretation.
There are two different capability flags being discussed here.
The first is V4L2_CAP_IO_MC for struct v4l2_capability: if set, then the
inputs and outputs are controlled by the media controller and not through
the video device.
I think everyone agrees with that capability flag.
The second capability flag was for struct v4l2_input/output, but not everyone
agreed with that, so let's just drop that.
>
>
>>
>>> In v4l2-ioctl.c we can just check V4L2_CAP_IO_MC in
>>> video_device->device_caps, and, if present, implement dummy
>>> s/g/enum_in/output ioctls. The enum ioctl would set V4L2_IN/OUT_CAP_MC and
>>> use video_device->name as the description.
>>
>> We still haven't agreed on what the description should be used for. If it is
>> to be presented to a user through an input/output selection UI, video_device-
>>> name doesn't seem very useful.
>>
>
> video_device->name doesn't seems to let it clear that this input is
> controlled by the Media framework as it will be a different name for
> each driver, shouldn't this be a fixed name?
If you have multiple video input nodes, then they would all have an input called
"Media Controller Input", which to me seems very uninformative. The new V4L2_CAP_IO_MC
already communicates that you need the MC, and applications can use that to put up
some proper explanation. Let's be honest: for the average user "Media Controller Input"
is gibberish anyway. And since all video input devices use the same input description
it is even more confusing.
If everyone prefers this to using the video_device name, then I won't block it. But
I think it is a bad idea :-)
Regards,
Hans
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC
2017-03-30 16:02 [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT Helen Koike
` (2 preceding siblings ...)
2017-03-31 8:29 ` Hans Verkuil
@ 2017-06-14 4:50 ` Helen Koike
2017-06-19 11:15 ` Hans Verkuil
3 siblings, 1 reply; 28+ messages in thread
From: Helen Koike @ 2017-06-14 4:50 UTC (permalink / raw)
To: Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, linux-media, linux-kernel
Cc: Mauro Carvalho Chehab, jgebben, Laurent Pinchart
Add V4L2_CAP_IO_MC to be used in struct v4l2_capability to indicate that
input and output are controlled by the Media Controller instead of V4L2
API.
When this flag is set, ioctls for get, set and enum input and outputs
are automatically enabled and programmed to call helper function.
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
Changes in v2::
- replace the type by capability
- erase V4L2_INPUT_TYPE_DEFAULT
- also consider output
- plug helpers in the ops automatically so drivers doesn't need
to set it by hand
- update docs
- commit message and title
---
Documentation/media/uapi/v4l/vidioc-querycap.rst | 3 +
Documentation/media/videodev2.h.rst.exceptions | 1 +
drivers/media/v4l2-core/v4l2-dev.c | 35 +++++++--
drivers/media/v4l2-core/v4l2-ioctl.c | 91 ++++++++++++++++++++++--
include/uapi/linux/videodev2.h | 2 +
5 files changed, 120 insertions(+), 12 deletions(-)
diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
index 12e0d9a..2bd1223 100644
--- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
@@ -252,6 +252,9 @@ specification the ioctl returns an ``EINVAL`` error code.
* - ``V4L2_CAP_TOUCH``
- 0x10000000
- This is a touch device.
+ * - ``V4L2_CAP_IO_MC``
+ - 0x20000000
+ - This device has its inputs and outputs controller by the Media Controller
* - ``V4L2_CAP_DEVICE_CAPS``
- 0x80000000
- The driver fills the ``device_caps`` field. This capability can
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index a5cb0a8..0b48cd0 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -159,6 +159,7 @@ replace define V4L2_CAP_ASYNCIO device-capabilities
replace define V4L2_CAP_STREAMING device-capabilities
replace define V4L2_CAP_DEVICE_CAPS device-capabilities
replace define V4L2_CAP_TOUCH device-capabilities
+replace define V4L2_CAP_IO_MC device-capabilities
# V4L2 pix flags
replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c647ba6..0f272fe 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -688,22 +688,34 @@ static void determine_valid_ioctls(struct video_device *vdev)
SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
if (is_rx) {
SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
- SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
- SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
- SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
+ if (vdev->device_caps & V4L2_CAP_IO_MC) {
+ set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
+ set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
+ set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
+ } else {
+ SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+ SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+ SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+ }
}
if (is_tx) {
- SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
- SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
- SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
+ if (vdev->device_caps & V4L2_CAP_IO_MC) {
+ set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
+ set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
+ set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
+ } else {
+ SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
+ SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
+ SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+ }
}
if (ops->vidioc_g_parm || (vdev->vfl_type == VFL_TYPE_GRABBER &&
ops->vidioc_g_std))
@@ -945,6 +957,17 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
video_device[vdev->minor] = vdev;
mutex_unlock(&videodev_lock);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ if (vdev->ioctl_ops
+ && !vdev->ioctl_ops->vidioc_enum_input
+ && !vdev->ioctl_ops->vidioc_s_input
+ && !vdev->ioctl_ops->vidioc_g_input
+ && !vdev->ioctl_ops->vidioc_enum_output
+ && !vdev->ioctl_ops->vidioc_s_output
+ && !vdev->ioctl_ops->vidioc_g_output)
+ vdev->device_caps |= V4L2_CAP_IO_MC;
+#endif
+
if (vdev->ioctl_ops)
determine_valid_ioctls(vdev);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index e5a2187..9d8c645 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1019,6 +1019,70 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
return ret;
}
+static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
+ struct v4l2_input *i)
+{
+ struct video_device *vfd = video_devdata(file);
+
+ if (i->index > 0)
+ return -EINVAL;
+
+ memset(i, 0, sizeof(*i));
+ strlcpy(i->name, vfd->name, sizeof(i->name));
+
+ return 0;
+}
+
+static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
+ struct v4l2_output *o)
+{
+ struct video_device *vfd = video_devdata(file);
+
+ if (o->index > 0)
+ return -EINVAL;
+
+ memset(o, 0, sizeof(*o));
+ strlcpy(o->name, vfd->name, sizeof(o->name));
+
+ return 0;
+}
+
+static int v4l2_ioctl_g_input_mc(struct file *file, void *priv, unsigned int *i)
+{
+ *i = 0;
+ return 0;
+}
+#define v4l2_ioctl_g_output_mc v4l2_ioctl_g_input_mc
+
+static int v4l2_ioctl_s_input_mc(struct file *file, void *priv, unsigned int i)
+{
+ return i ? -EINVAL : 0;
+}
+#define v4l2_ioctl_s_output_mc v4l2_ioctl_s_input_mc
+
+
+static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct video_device *vfd = video_devdata(file);
+
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_g_input_mc(file, fh, arg);
+ else
+ return ops->vidioc_g_input(file, fh, arg);
+}
+
+static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct video_device *vfd = video_devdata(file);
+
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_g_output_mc(file, fh, arg);
+ else
+ return ops->vidioc_g_output(file, fh, arg);
+}
+
static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -1028,13 +1092,22 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
ret = v4l_enable_media_source(vfd);
if (ret)
return ret;
- return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
+
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_s_input_mc(file, fh, *(unsigned int *)arg);
+ else
+ return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
}
static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
- return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
+ struct video_device *vfd = video_devdata(file);
+
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_s_output_mc(file, fh, *(unsigned int *)arg);
+ else
+ return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
}
static int v4l_g_priority(const struct v4l2_ioctl_ops *ops,
@@ -1077,7 +1150,10 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
if (is_valid_ioctl(vfd, VIDIOC_S_STD))
p->capabilities |= V4L2_IN_CAP_STD;
- return ops->vidioc_enum_input(file, fh, p);
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_enum_input_mc(file, fh, p);
+ else
+ return ops->vidioc_enum_input(file, fh, p);
}
static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
@@ -1095,7 +1171,10 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
if (is_valid_ioctl(vfd, VIDIOC_S_STD))
p->capabilities |= V4L2_OUT_CAP_STD;
- return ops->vidioc_enum_output(file, fh, p);
+ if (vfd->device_caps & V4L2_CAP_IO_MC)
+ return v4l2_ioctl_enum_output_mc(file, fh, p);
+ else
+ return ops->vidioc_enum_output(file, fh, p);
}
static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
@@ -2534,11 +2613,11 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO_STD(VIDIOC_S_AUDIO, vidioc_s_audio, v4l_print_audio, INFO_FL_PRIO),
IOCTL_INFO_FNC(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
- IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
+ IOCTL_INFO_FNC(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, 0),
IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, INFO_FL_PRIO),
- IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
+ IOCTL_INFO_FNC(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout, v4l_print_audioout, 0),
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2b8feb8..94cb196 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -460,6 +460,8 @@ struct v4l2_capability {
#define V4L2_CAP_TOUCH 0x10000000 /* Is a touch device */
+#define V4L2_CAP_IO_MC 0x20000000 /* Is input/output controlled by the media controler */
+
#define V4L2_CAP_DEVICE_CAPS 0x80000000 /* sets device capabilities field */
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC
2017-06-14 4:50 ` [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC Helen Koike
@ 2017-06-19 11:15 ` Hans Verkuil
2017-06-19 17:49 ` Helen Koike
0 siblings, 1 reply; 28+ messages in thread
From: Hans Verkuil @ 2017-06-19 11:15 UTC (permalink / raw)
To: Helen Koike, Sakari Ailus, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, linux-media, linux-kernel
Cc: Mauro Carvalho Chehab, jgebben, Laurent Pinchart
On 06/14/2017 06:50 AM, Helen Koike wrote:
> Add V4L2_CAP_IO_MC to be used in struct v4l2_capability to indicate that
> input and output are controlled by the Media Controller instead of V4L2
> API.
> When this flag is set, ioctls for get, set and enum input and outputs
> are automatically enabled and programmed to call helper function.
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
>
> Changes in v2::
> - replace the type by capability
> - erase V4L2_INPUT_TYPE_DEFAULT
> - also consider output
> - plug helpers in the ops automatically so drivers doesn't need
> to set it by hand
> - update docs
> - commit message and title
> ---
> Documentation/media/uapi/v4l/vidioc-querycap.rst | 3 +
> Documentation/media/videodev2.h.rst.exceptions | 1 +
> drivers/media/v4l2-core/v4l2-dev.c | 35 +++++++--
> drivers/media/v4l2-core/v4l2-ioctl.c | 91 ++++++++++++++++++++++--
> include/uapi/linux/videodev2.h | 2 +
> 5 files changed, 120 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 12e0d9a..2bd1223 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -252,6 +252,9 @@ specification the ioctl returns an ``EINVAL`` error code.
> * - ``V4L2_CAP_TOUCH``
> - 0x10000000
> - This is a touch device.
> + * - ``V4L2_CAP_IO_MC``
> + - 0x20000000
> + - This device has its inputs and outputs controller by the Media Controller
controller -> controlled
But I would rephrase this a bit:
- The inputs and/or outputs of this device are controlled by the Media Controller
(see: <add link to Part IV of the media documentation>).
> * - ``V4L2_CAP_DEVICE_CAPS``
> - 0x80000000
> - The driver fills the ``device_caps`` field. This capability can
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index a5cb0a8..0b48cd0 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -159,6 +159,7 @@ replace define V4L2_CAP_ASYNCIO device-capabilities
> replace define V4L2_CAP_STREAMING device-capabilities
> replace define V4L2_CAP_DEVICE_CAPS device-capabilities
> replace define V4L2_CAP_TOUCH device-capabilities
> +replace define V4L2_CAP_IO_MC device-capabilities
>
> # V4L2 pix flags
> replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c647ba6..0f272fe 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -688,22 +688,34 @@ static void determine_valid_ioctls(struct video_device *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
> if (is_rx) {
> SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> - SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> - SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> - SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
> SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
> SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
> SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
> SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> + if (vdev->device_caps & V4L2_CAP_IO_MC) {
> + set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> + set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> + set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> + } else {
> + SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> + SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> + SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> + }
> }
> if (is_tx) {
> - SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> - SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> - SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
> SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
> SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> + if (vdev->device_caps & V4L2_CAP_IO_MC) {
> + set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> + set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> + set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
> + } else {
> + SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> + SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> + SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> + }
> }
> if (ops->vidioc_g_parm || (vdev->vfl_type == VFL_TYPE_GRABBER &&
> ops->vidioc_g_std))
> @@ -945,6 +957,17 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
> video_device[vdev->minor] = vdev;
> mutex_unlock(&videodev_lock);
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (vdev->ioctl_ops
> + && !vdev->ioctl_ops->vidioc_enum_input
> + && !vdev->ioctl_ops->vidioc_s_input
> + && !vdev->ioctl_ops->vidioc_g_input
> + && !vdev->ioctl_ops->vidioc_enum_output
> + && !vdev->ioctl_ops->vidioc_s_output
> + && !vdev->ioctl_ops->vidioc_g_output)
> + vdev->device_caps |= V4L2_CAP_IO_MC;
No, this part should be dropped.
Let the driver set this capability explicitly. This code for example would set the IO_MC
bit as well for radio devices, and that's not what you want.
The MC can also be used by regular drivers (there is nothing preventing drivers from
doing that). So just leave this up to the driver.
> +#endif
> +
> if (vdev->ioctl_ops)
> determine_valid_ioctls(vdev);
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index e5a2187..9d8c645 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1019,6 +1019,70 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
> return ret;
> }
>
> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
> + struct v4l2_input *i)
> +{
> + struct video_device *vfd = video_devdata(file);
> +
> + if (i->index > 0)
> + return -EINVAL;
> +
> + memset(i, 0, sizeof(*i));
> + strlcpy(i->name, vfd->name, sizeof(i->name));
i->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
> + struct v4l2_output *o)
> +{
> + struct video_device *vfd = video_devdata(file);
> +
> + if (o->index > 0)
> + return -EINVAL;
> +
> + memset(o, 0, sizeof(*o));
> + strlcpy(o->name, vfd->name, sizeof(o->name));
o->type = V4L2_OUTPUT_TYPE_ANALOG;
> +
> + return 0;
> +}
> +
> +static int v4l2_ioctl_g_input_mc(struct file *file, void *priv, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +#define v4l2_ioctl_g_output_mc v4l2_ioctl_g_input_mc
> +
> +static int v4l2_ioctl_s_input_mc(struct file *file, void *priv, unsigned int i)
> +{
> + return i ? -EINVAL : 0;
> +}
> +#define v4l2_ioctl_s_output_mc v4l2_ioctl_s_input_mc
> +
> +
> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct video_device *vfd = video_devdata(file);
> +
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_g_input_mc(file, fh, arg);
> + else
'else' is not needed.
> + return ops->vidioc_g_input(file, fh, arg);
> +}
> +
> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct video_device *vfd = video_devdata(file);
> +
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_g_output_mc(file, fh, arg);
> + else
'else' is not needed. Same for the others below.
> + return ops->vidioc_g_output(file, fh, arg);
> +}
> +
> static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> @@ -1028,13 +1092,22 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> ret = v4l_enable_media_source(vfd);
> if (ret)
> return ret;
> - return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
> +
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_s_input_mc(file, fh, *(unsigned int *)arg);
> + else
> + return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
> }
>
> static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> - return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
> + struct video_device *vfd = video_devdata(file);
> +
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_s_output_mc(file, fh, *(unsigned int *)arg);
> + else
> + return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
> }
>
> static int v4l_g_priority(const struct v4l2_ioctl_ops *ops,
> @@ -1077,7 +1150,10 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
> if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> p->capabilities |= V4L2_IN_CAP_STD;
>
> - return ops->vidioc_enum_input(file, fh, p);
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_enum_input_mc(file, fh, p);
> + else
> + return ops->vidioc_enum_input(file, fh, p);
> }
>
> static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
> @@ -1095,7 +1171,10 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
> if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> p->capabilities |= V4L2_OUT_CAP_STD;
>
> - return ops->vidioc_enum_output(file, fh, p);
> + if (vfd->device_caps & V4L2_CAP_IO_MC)
> + return v4l2_ioctl_enum_output_mc(file, fh, p);
> + else
> + return ops->vidioc_enum_output(file, fh, p);
> }
>
> static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> @@ -2534,11 +2613,11 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
> IOCTL_INFO_STD(VIDIOC_S_AUDIO, vidioc_s_audio, v4l_print_audio, INFO_FL_PRIO),
> IOCTL_INFO_FNC(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
> IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
> - IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
> + IOCTL_INFO_FNC(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
> IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
> IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, 0),
> IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid, INFO_FL_PRIO),
> - IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
> + IOCTL_INFO_FNC(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
> IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
> IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
> IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout, v4l_print_audioout, 0),
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2b8feb8..94cb196 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -460,6 +460,8 @@ struct v4l2_capability {
>
> #define V4L2_CAP_TOUCH 0x10000000 /* Is a touch device */
>
> +#define V4L2_CAP_IO_MC 0x20000000 /* Is input/output controlled by the media controler */
controler -> controller
> +
> #define V4L2_CAP_DEVICE_CAPS 0x80000000 /* sets device capabilities field */
>
> /*
>
Regards,
Hans
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC
2017-06-19 11:15 ` Hans Verkuil
@ 2017-06-19 17:49 ` Helen Koike
0 siblings, 0 replies; 28+ messages in thread
From: Helen Koike @ 2017-06-19 17:49 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, linux-media, linux-kernel
Cc: Mauro Carvalho Chehab, jgebben, Laurent Pinchart
Hi Hans,
Thanks for reviewing this
On 2017-06-19 08:15 AM, Hans Verkuil wrote:
> On 06/14/2017 06:50 AM, Helen Koike wrote:
>> Add V4L2_CAP_IO_MC to be used in struct v4l2_capability to indicate that
>> input and output are controlled by the Media Controller instead of V4L2
>> API.
>> When this flag is set, ioctls for get, set and enum input and outputs
>> are automatically enabled and programmed to call helper function.
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> Changes in v2::
>> - replace the type by capability
>> - erase V4L2_INPUT_TYPE_DEFAULT
>> - also consider output
>> - plug helpers in the ops automatically so drivers doesn't need
>> to set it by hand
>> - update docs
>> - commit message and title
>> ---
>> Documentation/media/uapi/v4l/vidioc-querycap.rst | 3 +
>> Documentation/media/videodev2.h.rst.exceptions | 1 +
>> drivers/media/v4l2-core/v4l2-dev.c | 35 +++++++--
>> drivers/media/v4l2-core/v4l2-ioctl.c | 91
>> ++++++++++++++++++++++--
>> include/uapi/linux/videodev2.h | 2 +
>> 5 files changed, 120 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> index 12e0d9a..2bd1223 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> @@ -252,6 +252,9 @@ specification the ioctl returns an ``EINVAL``
>> error code.
>> * - ``V4L2_CAP_TOUCH``
>> - 0x10000000
>> - This is a touch device.
>> + * - ``V4L2_CAP_IO_MC``
>> + - 0x20000000
>> + - This device has its inputs and outputs controller by the
>> Media Controller
>
> controller -> controlled
Sorry, I'll remember to use a spell checker next time
>
> But I would rephrase this a bit:
>
> - The inputs and/or outputs of this device are controlled by the
> Media Controller
> (see: <add link to Part IV of the media documentation>).
>
Sure, much better.
In this document, almost all the flags start with "The device
supports..." or "The device has...", I was thinking to do something similar.
>> * - ``V4L2_CAP_DEVICE_CAPS``
>> - 0x80000000
>> - The driver fills the ``device_caps`` field. This capability can
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions
>> b/Documentation/media/videodev2.h.rst.exceptions
>> index a5cb0a8..0b48cd0 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -159,6 +159,7 @@ replace define V4L2_CAP_ASYNCIO device-capabilities
>> replace define V4L2_CAP_STREAMING device-capabilities
>> replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>> replace define V4L2_CAP_TOUCH device-capabilities
>> +replace define V4L2_CAP_IO_MC device-capabilities
>> # V4L2 pix flags
>> replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
>> b/drivers/media/v4l2-core/v4l2-dev.c
>> index c647ba6..0f272fe 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -688,22 +688,34 @@ static void determine_valid_ioctls(struct
>> video_device *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>> if (is_rx) {
>> SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
>> - SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>> - SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>> - SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>> SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>> SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>> SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
>> SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS,
>> vidioc_query_dv_timings);
>> SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>> + if (vdev->device_caps & V4L2_CAP_IO_MC) {
>> + set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>> + set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>> + set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>> + } else {
>> + SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT,
>> vidioc_enum_input);
>> + SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>> + SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>> + }
>> }
>> if (is_tx) {
>> - SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>> - SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>> - SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>> SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>> SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>> SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
>> + if (vdev->device_caps & V4L2_CAP_IO_MC) {
>> + set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>> + set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>> + set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>> + } else {
>> + SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT,
>> vidioc_enum_output);
>> + SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>> + SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>> + }
>> }
>> if (ops->vidioc_g_parm || (vdev->vfl_type ==
>> VFL_TYPE_GRABBER &&
>> ops->vidioc_g_std))
>> @@ -945,6 +957,17 @@ int __video_register_device(struct video_device
>> *vdev, int type, int nr,
>> video_device[vdev->minor] = vdev;
>> mutex_unlock(&videodev_lock);
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> + if (vdev->ioctl_ops
>> + && !vdev->ioctl_ops->vidioc_enum_input
>> + && !vdev->ioctl_ops->vidioc_s_input
>> + && !vdev->ioctl_ops->vidioc_g_input
>> + && !vdev->ioctl_ops->vidioc_enum_output
>> + && !vdev->ioctl_ops->vidioc_s_output
>> + && !vdev->ioctl_ops->vidioc_g_output)
>> + vdev->device_caps |= V4L2_CAP_IO_MC;
>
> No, this part should be dropped.
>
> Let the driver set this capability explicitly. This code for example
> would set the IO_MC
> bit as well for radio devices, and that's not what you want.
hmm, right, so we will need to update the drivers one by one in the end.
I think I still don't fully understand the importance of the
V4L2_CAP_IO_MC flag (sorry to bring this up again, I just want to make
sure this is right), we don't have a flag to say that an output is an
HDMI, user space uses the VIDIOC_ENUMOUTPUT ioctl to figure that this is
an HDMI by its output's name, why this can't be the same for MC
controlled IOs?
>
> The MC can also be used by regular drivers (there is nothing preventing
> drivers from
> doing that). So just leave this up to the driver.
>
>> +#endif
>> +
>> if (vdev->ioctl_ops)
>> determine_valid_ioctls(vdev);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index e5a2187..9d8c645 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1019,6 +1019,70 @@ static int v4l_querycap(const struct
>> v4l2_ioctl_ops *ops,
>> return ret;
>> }
>> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
>> + struct v4l2_input *i)
>> +{
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (i->index > 0)
>> + return -EINVAL;
>> +
>> + memset(i, 0, sizeof(*i));
>> + strlcpy(i->name, vfd->name, sizeof(i->name));
>
> i->type = V4L2_INPUT_TYPE_CAMERA;
>
>> +
>> + return 0;
>> +}
>> +
>> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
>> + struct v4l2_output *o)
>> +{
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (o->index > 0)
>> + return -EINVAL;
>> +
>> + memset(o, 0, sizeof(*o));
>> + strlcpy(o->name, vfd->name, sizeof(o->name));
>
> o->type = V4L2_OUTPUT_TYPE_ANALOG;
>
>> +
>> + return 0;
>> +}
>> +
>> +static int v4l2_ioctl_g_input_mc(struct file *file, void *priv,
>> unsigned int *i)
>> +{
>> + *i = 0;
>> + return 0;
>> +}
>> +#define v4l2_ioctl_g_output_mc v4l2_ioctl_g_input_mc
>> +
>> +static int v4l2_ioctl_s_input_mc(struct file *file, void *priv,
>> unsigned int i)
>> +{
>> + return i ? -EINVAL : 0;
>> +}
>> +#define v4l2_ioctl_s_output_mc v4l2_ioctl_s_input_mc
>> +
>> +
>> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_g_input_mc(file, fh, arg);
>> + else
>
> 'else' is not needed.
'else' is not needed but I thought it was easier to read the code this
way (and I believe the compiler optimizes this), anyway, I'll remove the
'else' then, no problem.
>
>> + return ops->vidioc_g_input(file, fh, arg);
>> +}
>> +
>> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_g_output_mc(file, fh, arg);
>> + else
>
> 'else' is not needed. Same for the others below.
>
>> + return ops->vidioc_g_output(file, fh, arg);
>> +}
>> +
>> static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> @@ -1028,13 +1092,22 @@ static int v4l_s_input(const struct
>> v4l2_ioctl_ops *ops,
>> ret = v4l_enable_media_source(vfd);
>> if (ret)
>> return ret;
>> - return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>> +
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_s_input_mc(file, fh, *(unsigned int *)arg);
>> + else
>> + return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>> }
>> static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> - return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>> + struct video_device *vfd = video_devdata(file);
>> +
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_s_output_mc(file, fh, *(unsigned int *)arg);
>> + else
>> + return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>> }
>> static int v4l_g_priority(const struct v4l2_ioctl_ops *ops,
>> @@ -1077,7 +1150,10 @@ static int v4l_enuminput(const struct
>> v4l2_ioctl_ops *ops,
>> if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>> p->capabilities |= V4L2_IN_CAP_STD;
>> - return ops->vidioc_enum_input(file, fh, p);
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_enum_input_mc(file, fh, p);
>> + else
>> + return ops->vidioc_enum_input(file, fh, p);
>> }
>> static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>> @@ -1095,7 +1171,10 @@ static int v4l_enumoutput(const struct
>> v4l2_ioctl_ops *ops,
>> if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>> p->capabilities |= V4L2_OUT_CAP_STD;
>> - return ops->vidioc_enum_output(file, fh, p);
>> + if (vfd->device_caps & V4L2_CAP_IO_MC)
>> + return v4l2_ioctl_enum_output_mc(file, fh, p);
>> + else
>> + return ops->vidioc_enum_output(file, fh, p);
>> }
>> static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>> @@ -2534,11 +2613,11 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>> IOCTL_INFO_STD(VIDIOC_S_AUDIO, vidioc_s_audio, v4l_print_audio,
>> INFO_FL_PRIO),
>> IOCTL_INFO_FNC(VIDIOC_QUERYCTRL, v4l_queryctrl,
>> v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
>> IOCTL_INFO_FNC(VIDIOC_QUERYMENU, v4l_querymenu,
>> v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu,
>> index)),
>> - IOCTL_INFO_STD(VIDIOC_G_INPUT, vidioc_g_input, v4l_print_u32, 0),
>> + IOCTL_INFO_FNC(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
>> IOCTL_INFO_FNC(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32,
>> INFO_FL_PRIO),
>> IOCTL_INFO_STD(VIDIOC_G_EDID, vidioc_g_edid, v4l_print_edid, 0),
>> IOCTL_INFO_STD(VIDIOC_S_EDID, vidioc_s_edid, v4l_print_edid,
>> INFO_FL_PRIO),
>> - IOCTL_INFO_STD(VIDIOC_G_OUTPUT, vidioc_g_output, v4l_print_u32, 0),
>> + IOCTL_INFO_FNC(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
>> IOCTL_INFO_FNC(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32,
>> INFO_FL_PRIO),
>> IOCTL_INFO_FNC(VIDIOC_ENUMOUTPUT, v4l_enumoutput,
>> v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
>> IOCTL_INFO_STD(VIDIOC_G_AUDOUT, vidioc_g_audout,
>> v4l_print_audioout, 0),
>> diff --git a/include/uapi/linux/videodev2.h
>> b/include/uapi/linux/videodev2.h
>> index 2b8feb8..94cb196 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -460,6 +460,8 @@ struct v4l2_capability {
>> #define V4L2_CAP_TOUCH 0x10000000 /* Is a touch
>> device */
>> +#define V4L2_CAP_IO_MC 0x20000000 /* Is input/output
>> controlled by the media controler */
>
> controler -> controller
>
>> +
>> #define V4L2_CAP_DEVICE_CAPS 0x80000000 /* sets device
>> capabilities field */
>> /*
>>
>
> Regards,
>
> Hans
Thanks,
Helen
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-06-12 9:26 ` Hans Verkuil
@ 2017-06-30 11:03 ` Sakari Ailus
2017-06-30 11:13 ` Hans Verkuil
0 siblings, 1 reply; 28+ messages in thread
From: Sakari Ailus @ 2017-06-30 11:03 UTC (permalink / raw)
To: Hans Verkuil
Cc: Helen Koike, Laurent Pinchart, Mauro Carvalho Chehab,
linux-media, jgebben
Hi Hans and others,
On Mon, Jun 12, 2017 at 11:26:12AM +0200, Hans Verkuil wrote:
> On 06/06/2017 06:22 PM, Helen Koike wrote:
> > Hi All,
> >
> > Just reviving this discussion
> >
> > On 2017-04-07 06:53 AM, Laurent Pinchart wrote:
> > > Hi Hans,
> > >
> > > On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
> > > > On 04/04/2017 03:22 PM, Sakari Ailus wrote:
> > > > > On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
> > > > > > On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
> > > > > > > Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
> > > > > > > > On 30/03/17 18:02, Helen Koike wrote:
> > > > > > > > > Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
> > > > > > > > > be used when no inputs are available in the device
> > > > > > > > >
> > > > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > > > ---
> > > > > > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> > > > > > > > > include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> > > > > > > > > include/uapi/linux/videodev2.h | 1 +
> > > > > > > > > 3 files changed, 54 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > > > > > > b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> > > > > > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > > > > > > @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> > > > > > > > > video_device *vdev, unsigned cmd)
> > > > > > > > > return vdev->lock;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > > > > > > > > + struct v4l2_input *i)
> > > > > > > > > +{
> > > > > > > > > + if (i->index > 0)
> > > > > > > > > + return -EINVAL;
> > > > > > > > > +
> > > > > > > > > + memset(i, 0, sizeof(*i));
> > > > > > > > > + i->type = V4L2_INPUT_TYPE_DEFAULT;
> > > > > > > > > + strlcpy(i->name, "Default", sizeof(i->name));
> > > > > > > > > +
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> > > > > > > > > +
> > > > > > > > > +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> > > > > > > > > unsigned int *i)
> > > > > > > > > +{
> > > > > > > > > + *i = 0;
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> > > > > > > > > +
> > > > > > > > > +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> > > > > > > > > unsigned int i)
> > > > > > > > > +{
> > > > > > > > > + return i ? -EINVAL : 0;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> > > > > > > > > +
> > > > > > > > > /* Common ioctl debug function. This function can be used by
> > > > > > > > > external ioctl messages as well as internal V4L ioctl */
> > > > > > > > >
> > > > > > > > > void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> > > > > > > > > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > > > > > > > > index 6cd94e5..accc470 100644
> > > > > > > > > --- a/include/media/v4l2-ioctl.h
> > > > > > > > > +++ b/include/media/v4l2-ioctl.h
> > > > > > > > > @@ -652,6 +652,32 @@ struct video_device;
> > > > > > > > > */
> > > > > > > > >
> > > > > > > > > struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
> > > > > > > > > int cmd);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
> > > > > > > > > VIDIOC_ENUM_INPUT ioctl
> > > > > > > > > + *
> > > > > > > > > + * Plug this function in vidioc_enum_input field of the struct
> > > > > > > > > v4l2_ioctl_ops to
> > > > > > > > > + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> > > > > > > > > + */
> > > > > > > > > +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> > > > > > > > > + struct v4l2_input *i);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
> > > > > > > > > ioctl
> > > > > > > > > + *
> > > > > > > > > + * Plug this function in vidioc_g_input field of the struct
> > > > > > > > > v4l2_ioctl_ops
> > > > > > > > > + * when using v4l2_ioctl_enum_input_default
> > > > > > > > > + */
> > > > > > > > > +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> > > > > > > > > unsigned int *i);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
> > > > > > > > > ioctl
> > > > > > > > > + *
> > > > > > > > > + * Plug this function in vidioc_s_input field of the struct
> > > > > > > > > v4l2_ioctl_ops
> > > > > > > > > + * when using v4l2_ioctl_enum_input_default
> > > > > > > > > + */
> > > > > > > > > +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> > > > > > > > > unsigned int i);
> > > > > > > > > +
> > > > > > > > > /* names for fancy debug output */
> > > > > > > > > extern const char *v4l2_field_names[];
> > > > > > > > > extern const char *v4l2_type_names[];
> > > > > > > > > diff --git a/include/uapi/linux/videodev2.h
> > > > > > > > > b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
> > > > > > > > > --- a/include/uapi/linux/videodev2.h
> > > > > > > > > +++ b/include/uapi/linux/videodev2.h
> > > > > > > > > @@ -1477,6 +1477,7 @@ struct v4l2_input {
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > /* Values for the 'type' field */
> > > > > > > > > +#define V4L2_INPUT_TYPE_DEFAULT 0
> > > > > > > >
> > > > > > > > I don't think we should add a new type here.
> > > > > > >
> > > > > > > I second that. Just replied the same thing on a comment from Sakari to
> > > > > > > patch 2/2.
> > > > > > >
> > > > > > > > The whole point of this exercise is to
> > > > > > > > allow existing apps to work, and existing apps expect a TYPE_CAMERA.
> > > > > > > >
> > > > > > > > BTW, don't read to much in the term 'CAMERA': it's really a catch all
> > > > > > > > for any video stream, whether it is from a sensor, composite input,
> > > > > > > > HDMI, etc.
> > > > > > > >
> > > > > > > > The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
> > > > > > > > out of date :-(
> > > > > > >
> > > > > > > Yeah, we always used "CAMERA" to mean NOT_TUNER.
> > > > > > >
> > > > > > > > Rather than creating a new type I would add a new V4L2_IN_CAP_MC
> > > > > > > > capability that indicates that this input is controlled via the media
> > > > > > > > controller. That makes much more sense and it wouldn't potentially
> > > > > > > > break applications.
> > > > > > > >
> > > > > > > > Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
> > > > > > > > and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
> > > > > > > > outdated name and the spec is again out of date).
> > > > > > >
> > > > > > > I don't see any sense on distinguishing IN and OUT for MC. I mean:
> > > > > > > should
> > > > > > > we ever allow that any driver to have their inputs controlled via V4L2
> > > > > > > API,
> > > > > > > and their outputs controlled via MC (or vice-versa)? I don't think so.
> > > > > > >
> > > > > > > Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> > > > > > > let's call it just V4L2_CAP_MC.
> > > > > > >
> > > > > > > > Regarding the name: should we use the name stored in struct
> > > > > > > > video_device instead? That might be more descriptive.
> > > > > > >
> > > > > > > Makes sense to me.
> > > > > > >
> > > > > > > > Alternatively use something like "Media Controller Input".
> > > > > > >
> > > > > > > Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> > > > > > > stored at video_device.
> > > > > >
> > > > > > Just to clarify: the V4L2_CAP_MC would indicated that the media
> > > > > > controller
> > > > > > is enabled in general? Or just for inputs and outputs?
> > > > >
> > > > > I let Mauro and Hans to comment on their own behalf, but I think whatever
> > > > > is communicated through the input IOCTLs should be applicable to inputs
> > > > > only.
> > > > >
> > > > > The fact that the video device is a part of an MC graph could be conveyed
> > > > > using a capability flag. Or by providing information on the media device
> > > > > node, something that has been proposed earlier on. Either is out of the
> > > > > scope of this patchset IMO, but should be addressed separately.
> > > > >
> > > > > > If it is the first case, not necessarily the inputs/outputs are
> > > > > > controlled
> > > > > > via MC (we can still have a MC capable driver, but inputs/outputs
> > > > > > controlled via V4L2 no? When the driver doesn't offer the necessary link
> > > > > > controls via MC), then checking if V4L2_CAP_MC then use the name "Media
> > > > > > Controller Input" is not enough.
> > > > > > If it is the second case, then wouldn't it be better to name it
> > > > > > V4L2_CAP_MC_IO ?
> > > >
> > > > It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
> > > > feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
> > > > V4L2_IN/OUT_CAP flags make no sense in this case.
> > >
> > > I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
> > > of view. I'd rather avoid adding flags unless there's a real use for them.
> >
> >
> > Just to clarify, should this capability flag be set in struct
> > v4l2_input/struct v4l2_output through VIDIOC_ENUMINPUT/,
> > VIDIOC_ENUMOUTPUT? Or should it be set in struct v4l2_capability
> > through VIDIOC_QUERYCAP ?
> > Because if it is the first case, the I feel we should have two flags
> > V4L2_IN/OUT_CAP_MC in the API to follow the current convention, but this
> > kinda implies that we could have a driver that allows both flags to be
> > set differently.
> > Setting a V4L2_IO_CAP_MC at struct v4l2_capability would avoid this
> > interpretation.
>
> There are two different capability flags being discussed here.
>
> The first is V4L2_CAP_IO_MC for struct v4l2_capability: if set, then the
> inputs and outputs are controlled by the media controller and not through
> the video device.
>
> I think everyone agrees with that capability flag.
What exactly would that capability flag tell?
That the driver implements Media controller support? Or that any pipeline
configuration works through Media controller?
We've discussed device profiles in the past. Should this capability flag
tell about a device profile? We haven't documented the profiles, but in
practice the V4L2 video nodes are a data interface only on MC-enabled
drivers. That suggests that if there is input selection to be done, that
would take place on a control interface, i.e. on a V4L2 sub-device node.
I think it'd be good to have a single flag for this rather than small hints
here and there of what kind of an interface the user is dealing with.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-06-30 11:03 ` Sakari Ailus
@ 2017-06-30 11:13 ` Hans Verkuil
2017-06-30 13:49 ` Sakari Ailus
0 siblings, 1 reply; 28+ messages in thread
From: Hans Verkuil @ 2017-06-30 11:13 UTC (permalink / raw)
To: Sakari Ailus
Cc: Helen Koike, Laurent Pinchart, Mauro Carvalho Chehab,
linux-media, jgebben
On 30/06/17 13:03, Sakari Ailus wrote:
> Hi Hans and others,
>
> On Mon, Jun 12, 2017 at 11:26:12AM +0200, Hans Verkuil wrote:
>> On 06/06/2017 06:22 PM, Helen Koike wrote:
>>> Hi All,
>>>
>>> Just reviving this discussion
>>>
>>> On 2017-04-07 06:53 AM, Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
>>>>> On 04/04/2017 03:22 PM, Sakari Ailus wrote:
>>>>>> On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
>>>>>>> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
>>>>>>>> Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
>>>>>>>>> On 30/03/17 18:02, Helen Koike wrote:
>>>>>>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
>>>>>>>>>> be used when no inputs are available in the device
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
>>>>>>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
>>>>>>>>>> include/uapi/linux/videodev2.h | 1 +
>>>>>>>>>> 3 files changed, 54 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
>>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
>>>>>>>>>> video_device *vdev, unsigned cmd)
>>>>>>>>>> return vdev->lock;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>>>>> + struct v4l2_input *i)
>>>>>>>>>> +{
>>>>>>>>>> + if (i->index > 0)
>>>>>>>>>> + return -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> + memset(i, 0, sizeof(*i));
>>>>>>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
>>>>>>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
>>>>>>>>>> +
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
>>>>>>>>>> +
>>>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>>>>> unsigned int *i)
>>>>>>>>>> +{
>>>>>>>>>> + *i = 0;
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
>>>>>>>>>> +
>>>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>>>>> unsigned int i)
>>>>>>>>>> +{
>>>>>>>>>> + return i ? -EINVAL : 0;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
>>>>>>>>>> +
>>>>>>>>>> /* Common ioctl debug function. This function can be used by
>>>>>>>>>> external ioctl messages as well as internal V4L ioctl */
>>>>>>>>>>
>>>>>>>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
>>>>>>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>>>>>>>>> index 6cd94e5..accc470 100644
>>>>>>>>>> --- a/include/media/v4l2-ioctl.h
>>>>>>>>>> +++ b/include/media/v4l2-ioctl.h
>>>>>>>>>> @@ -652,6 +652,32 @@ struct video_device;
>>>>>>>>>> */
>>>>>>>>>>
>>>>>>>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
>>>>>>>>>> int cmd);
>>>>>>>>>> +
>>>>>>>>>> +/**
>>>>>>>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
>>>>>>>>>> VIDIOC_ENUM_INPUT ioctl
>>>>>>>>>> + *
>>>>>>>>>> + * Plug this function in vidioc_enum_input field of the struct
>>>>>>>>>> v4l2_ioctl_ops to
>>>>>>>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
>>>>>>>>>> + */
>>>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
>>>>>>>>>> + struct v4l2_input *i);
>>>>>>>>>> +
>>>>>>>>>> +/**
>>>>>>>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
>>>>>>>>>> ioctl
>>>>>>>>>> + *
>>>>>>>>>> + * Plug this function in vidioc_g_input field of the struct
>>>>>>>>>> v4l2_ioctl_ops
>>>>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>>>>> + */
>>>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
>>>>>>>>>> unsigned int *i);
>>>>>>>>>> +
>>>>>>>>>> +/**
>>>>>>>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
>>>>>>>>>> ioctl
>>>>>>>>>> + *
>>>>>>>>>> + * Plug this function in vidioc_s_input field of the struct
>>>>>>>>>> v4l2_ioctl_ops
>>>>>>>>>> + * when using v4l2_ioctl_enum_input_default
>>>>>>>>>> + */
>>>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
>>>>>>>>>> unsigned int i);
>>>>>>>>>> +
>>>>>>>>>> /* names for fancy debug output */
>>>>>>>>>> extern const char *v4l2_field_names[];
>>>>>>>>>> extern const char *v4l2_type_names[];
>>>>>>>>>> diff --git a/include/uapi/linux/videodev2.h
>>>>>>>>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
>>>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> /* Values for the 'type' field */
>>>>>>>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
>>>>>>>>>
>>>>>>>>> I don't think we should add a new type here.
>>>>>>>>
>>>>>>>> I second that. Just replied the same thing on a comment from Sakari to
>>>>>>>> patch 2/2.
>>>>>>>>
>>>>>>>>> The whole point of this exercise is to
>>>>>>>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
>>>>>>>>>
>>>>>>>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all
>>>>>>>>> for any video stream, whether it is from a sensor, composite input,
>>>>>>>>> HDMI, etc.
>>>>>>>>>
>>>>>>>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
>>>>>>>>> out of date :-(
>>>>>>>>
>>>>>>>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
>>>>>>>>
>>>>>>>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC
>>>>>>>>> capability that indicates that this input is controlled via the media
>>>>>>>>> controller. That makes much more sense and it wouldn't potentially
>>>>>>>>> break applications.
>>>>>>>>>
>>>>>>>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
>>>>>>>>> and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
>>>>>>>>> outdated name and the spec is again out of date).
>>>>>>>>
>>>>>>>> I don't see any sense on distinguishing IN and OUT for MC. I mean:
>>>>>>>> should
>>>>>>>> we ever allow that any driver to have their inputs controlled via V4L2
>>>>>>>> API,
>>>>>>>> and their outputs controlled via MC (or vice-versa)? I don't think so.
>>>>>>>>
>>>>>>>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
>>>>>>>> let's call it just V4L2_CAP_MC.
>>>>>>>>
>>>>>>>>> Regarding the name: should we use the name stored in struct
>>>>>>>>> video_device instead? That might be more descriptive.
>>>>>>>>
>>>>>>>> Makes sense to me.
>>>>>>>>
>>>>>>>>> Alternatively use something like "Media Controller Input".
>>>>>>>>
>>>>>>>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
>>>>>>>> stored at video_device.
>>>>>>>
>>>>>>> Just to clarify: the V4L2_CAP_MC would indicated that the media
>>>>>>> controller
>>>>>>> is enabled in general? Or just for inputs and outputs?
>>>>>>
>>>>>> I let Mauro and Hans to comment on their own behalf, but I think whatever
>>>>>> is communicated through the input IOCTLs should be applicable to inputs
>>>>>> only.
>>>>>>
>>>>>> The fact that the video device is a part of an MC graph could be conveyed
>>>>>> using a capability flag. Or by providing information on the media device
>>>>>> node, something that has been proposed earlier on. Either is out of the
>>>>>> scope of this patchset IMO, but should be addressed separately.
>>>>>>
>>>>>>> If it is the first case, not necessarily the inputs/outputs are
>>>>>>> controlled
>>>>>>> via MC (we can still have a MC capable driver, but inputs/outputs
>>>>>>> controlled via V4L2 no? When the driver doesn't offer the necessary link
>>>>>>> controls via MC), then checking if V4L2_CAP_MC then use the name "Media
>>>>>>> Controller Input" is not enough.
>>>>>>> If it is the second case, then wouldn't it be better to name it
>>>>>>> V4L2_CAP_MC_IO ?
>>>>>
>>>>> It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
>>>>> feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
>>>>> V4L2_IN/OUT_CAP flags make no sense in this case.
>>>>
>>>> I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
>>>> of view. I'd rather avoid adding flags unless there's a real use for them.
>>>
>>>
>>> Just to clarify, should this capability flag be set in struct
>>> v4l2_input/struct v4l2_output through VIDIOC_ENUMINPUT/,
>>> VIDIOC_ENUMOUTPUT? Or should it be set in struct v4l2_capability
>>> through VIDIOC_QUERYCAP ?
>>> Because if it is the first case, the I feel we should have two flags
>>> V4L2_IN/OUT_CAP_MC in the API to follow the current convention, but this
>>> kinda implies that we could have a driver that allows both flags to be
>>> set differently.
>>> Setting a V4L2_IO_CAP_MC at struct v4l2_capability would avoid this
>>> interpretation.
>>
>> There are two different capability flags being discussed here.
>>
>> The first is V4L2_CAP_IO_MC for struct v4l2_capability: if set, then the
>> inputs and outputs are controlled by the media controller and not through
>> the video device.
>>
>> I think everyone agrees with that capability flag.
>
> What exactly would that capability flag tell?
>
> That the driver implements Media controller support? Or that any pipeline
> configuration works through Media controller?
The second. Otherwise the capability flag would be V4L2_CAP_MC :-)
Note that a driver can implement the MC, but not need it to setup the
pipeline. I.e. adding the MC to bttv would make no change to how you select
an input.
> We've discussed device profiles in the past. Should this capability flag
> tell about a device profile? We haven't documented the profiles, but in
> practice the V4L2 video nodes are a data interface only on MC-enabled
> drivers. That suggests that if there is input selection to be done, that
> would take place on a control interface, i.e. on a V4L2 sub-device node.
>
> I think it'd be good to have a single flag for this rather than small hints
> here and there of what kind of an interface the user is dealing with.
>
Well, the device profile here is that you need to use the MC to configure
the pipeline for this input as opposed to being able to just use S_INPUT.
Regards,
Hans
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT
2017-06-30 11:13 ` Hans Verkuil
@ 2017-06-30 13:49 ` Sakari Ailus
0 siblings, 0 replies; 28+ messages in thread
From: Sakari Ailus @ 2017-06-30 13:49 UTC (permalink / raw)
To: Hans Verkuil
Cc: Helen Koike, Laurent Pinchart, Mauro Carvalho Chehab,
linux-media, jgebben
On Fri, Jun 30, 2017 at 01:13:40PM +0200, Hans Verkuil wrote:
> On 30/06/17 13:03, Sakari Ailus wrote:
> > Hi Hans and others,
> >
> > On Mon, Jun 12, 2017 at 11:26:12AM +0200, Hans Verkuil wrote:
> >> On 06/06/2017 06:22 PM, Helen Koike wrote:
> >>> Hi All,
> >>>
> >>> Just reviving this discussion
> >>>
> >>> On 2017-04-07 06:53 AM, Laurent Pinchart wrote:
> >>>> Hi Hans,
> >>>>
> >>>> On Friday 07 Apr 2017 11:46:48 Hans Verkuil wrote:
> >>>>> On 04/04/2017 03:22 PM, Sakari Ailus wrote:
> >>>>>> On Mon, Apr 03, 2017 at 12:11:54PM -0300, Helen Koike wrote:
> >>>>>>> On 2017-03-31 06:57 AM, Mauro Carvalho Chehab wrote:
> >>>>>>>> Em Fri, 31 Mar 2017 10:29:04 +0200 Hans Verkuil escreveu:
> >>>>>>>>> On 30/03/17 18:02, Helen Koike wrote:
> >>>>>>>>>> Add V4L2_INPUT_TYPE_DEFAULT and helpers functions for input ioctls to
> >>>>>>>>>> be used when no inputs are available in the device
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 27 +++++++++++++++++++++++++++
> >>>>>>>>>> include/media/v4l2-ioctl.h | 26 ++++++++++++++++++++++++++
> >>>>>>>>>> include/uapi/linux/videodev2.h | 1 +
> >>>>>>>>>> 3 files changed, 54 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>>>>>>> b/drivers/media/v4l2-core/v4l2-ioctl.c index 0c3f238..ccaf04b 100644
> >>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>>>>>>> @@ -2573,6 +2573,33 @@ struct mutex *v4l2_ioctl_get_lock(struct
> >>>>>>>>>> video_device *vdev, unsigned cmd)
> >>>>>>>>>> return vdev->lock;
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>>>>>>>> + struct v4l2_input *i)
> >>>>>>>>>> +{
> >>>>>>>>>> + if (i->index > 0)
> >>>>>>>>>> + return -EINVAL;
> >>>>>>>>>> +
> >>>>>>>>>> + memset(i, 0, sizeof(*i));
> >>>>>>>>>> + i->type = V4L2_INPUT_TYPE_DEFAULT;
> >>>>>>>>>> + strlcpy(i->name, "Default", sizeof(i->name));
> >>>>>>>>>> +
> >>>>>>>>>> + return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_enum_input_default);
> >>>>>>>>>> +
> >>>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> >>>>>>>>>> unsigned int *i)
> >>>>>>>>>> +{
> >>>>>>>>>> + *i = 0;
> >>>>>>>>>> + return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_g_input_default);
> >>>>>>>>>> +
> >>>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> >>>>>>>>>> unsigned int i)
> >>>>>>>>>> +{
> >>>>>>>>>> + return i ? -EINVAL : 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(v4l2_ioctl_s_input_default);
> >>>>>>>>>> +
> >>>>>>>>>> /* Common ioctl debug function. This function can be used by
> >>>>>>>>>> external ioctl messages as well as internal V4L ioctl */
> >>>>>>>>>>
> >>>>>>>>>> void v4l_printk_ioctl(const char *prefix, unsigned int cmd)
> >>>>>>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >>>>>>>>>> index 6cd94e5..accc470 100644
> >>>>>>>>>> --- a/include/media/v4l2-ioctl.h
> >>>>>>>>>> +++ b/include/media/v4l2-ioctl.h
> >>>>>>>>>> @@ -652,6 +652,32 @@ struct video_device;
> >>>>>>>>>> */
> >>>>>>>>>>
> >>>>>>>>>> struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned
> >>>>>>>>>> int cmd);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * v4l2_ioctl_enum_input_default - v4l2 ioctl helper for
> >>>>>>>>>> VIDIOC_ENUM_INPUT ioctl
> >>>>>>>>>> + *
> >>>>>>>>>> + * Plug this function in vidioc_enum_input field of the struct
> >>>>>>>>>> v4l2_ioctl_ops to
> >>>>>>>>>> + * enumerate a single input as V4L2_INPUT_TYPE_DEFAULT
> >>>>>>>>>> + */
> >>>>>>>>>> +int v4l2_ioctl_enum_input_default(struct file *file, void *priv,
> >>>>>>>>>> + struct v4l2_input *i);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * v4l2_ioctl_g_input_default - v4l2 ioctl helper for VIDIOC_G_INPUT
> >>>>>>>>>> ioctl
> >>>>>>>>>> + *
> >>>>>>>>>> + * Plug this function in vidioc_g_input field of the struct
> >>>>>>>>>> v4l2_ioctl_ops
> >>>>>>>>>> + * when using v4l2_ioctl_enum_input_default
> >>>>>>>>>> + */
> >>>>>>>>>> +int v4l2_ioctl_g_input_default(struct file *file, void *priv,
> >>>>>>>>>> unsigned int *i);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * v4l2_ioctl_s_input_default - v4l2 ioctl helper for VIDIOC_S_INPUT
> >>>>>>>>>> ioctl
> >>>>>>>>>> + *
> >>>>>>>>>> + * Plug this function in vidioc_s_input field of the struct
> >>>>>>>>>> v4l2_ioctl_ops
> >>>>>>>>>> + * when using v4l2_ioctl_enum_input_default
> >>>>>>>>>> + */
> >>>>>>>>>> +int v4l2_ioctl_s_input_default(struct file *file, void *priv,
> >>>>>>>>>> unsigned int i);
> >>>>>>>>>> +
> >>>>>>>>>> /* names for fancy debug output */
> >>>>>>>>>> extern const char *v4l2_field_names[];
> >>>>>>>>>> extern const char *v4l2_type_names[];
> >>>>>>>>>> diff --git a/include/uapi/linux/videodev2.h
> >>>>>>>>>> b/include/uapi/linux/videodev2.h index 316be62..c10bbde 100644
> >>>>>>>>>> --- a/include/uapi/linux/videodev2.h
> >>>>>>>>>> +++ b/include/uapi/linux/videodev2.h
> >>>>>>>>>> @@ -1477,6 +1477,7 @@ struct v4l2_input {
> >>>>>>>>>> };
> >>>>>>>>>>
> >>>>>>>>>> /* Values for the 'type' field */
> >>>>>>>>>> +#define V4L2_INPUT_TYPE_DEFAULT 0
> >>>>>>>>>
> >>>>>>>>> I don't think we should add a new type here.
> >>>>>>>>
> >>>>>>>> I second that. Just replied the same thing on a comment from Sakari to
> >>>>>>>> patch 2/2.
> >>>>>>>>
> >>>>>>>>> The whole point of this exercise is to
> >>>>>>>>> allow existing apps to work, and existing apps expect a TYPE_CAMERA.
> >>>>>>>>>
> >>>>>>>>> BTW, don't read to much in the term 'CAMERA': it's really a catch all
> >>>>>>>>> for any video stream, whether it is from a sensor, composite input,
> >>>>>>>>> HDMI, etc.
> >>>>>>>>>
> >>>>>>>>> The description for V4L2_INPUT_TYPE_CAMERA in the spec is hopelessly
> >>>>>>>>> out of date :-(
> >>>>>>>>
> >>>>>>>> Yeah, we always used "CAMERA" to mean NOT_TUNER.
> >>>>>>>>
> >>>>>>>>> Rather than creating a new type I would add a new V4L2_IN_CAP_MC
> >>>>>>>>> capability that indicates that this input is controlled via the media
> >>>>>>>>> controller. That makes much more sense and it wouldn't potentially
> >>>>>>>>> break applications.
> >>>>>>>>>
> >>>>>>>>> Exactly the same can be done for outputs as well: add V4L2_OUT_CAP_MC
> >>>>>>>>> and use V4L2_OUTPUT_TYPE_ANALOG as the output type (again, a horrible
> >>>>>>>>> outdated name and the spec is again out of date).
> >>>>>>>>
> >>>>>>>> I don't see any sense on distinguishing IN and OUT for MC. I mean:
> >>>>>>>> should
> >>>>>>>> we ever allow that any driver to have their inputs controlled via V4L2
> >>>>>>>> API,
> >>>>>>>> and their outputs controlled via MC (or vice-versa)? I don't think so.
> >>>>>>>>
> >>>>>>>> Either all device inputs/outputs are controlled via V4L2 or via MC. So,
> >>>>>>>> let's call it just V4L2_CAP_MC.
> >>>>>>>>
> >>>>>>>>> Regarding the name: should we use the name stored in struct
> >>>>>>>>> video_device instead? That might be more descriptive.
> >>>>>>>>
> >>>>>>>> Makes sense to me.
> >>>>>>>>
> >>>>>>>>> Alternatively use something like "Media Controller Input".
> >>>>>>>>
> >>>>>>>> Yeah, we could do that, if V4L2_CAP_MC. if not, we can use the name
> >>>>>>>> stored at video_device.
> >>>>>>>
> >>>>>>> Just to clarify: the V4L2_CAP_MC would indicated that the media
> >>>>>>> controller
> >>>>>>> is enabled in general? Or just for inputs and outputs?
> >>>>>>
> >>>>>> I let Mauro and Hans to comment on their own behalf, but I think whatever
> >>>>>> is communicated through the input IOCTLs should be applicable to inputs
> >>>>>> only.
> >>>>>>
> >>>>>> The fact that the video device is a part of an MC graph could be conveyed
> >>>>>> using a capability flag. Or by providing information on the media device
> >>>>>> node, something that has been proposed earlier on. Either is out of the
> >>>>>> scope of this patchset IMO, but should be addressed separately.
> >>>>>>
> >>>>>>> If it is the first case, not necessarily the inputs/outputs are
> >>>>>>> controlled
> >>>>>>> via MC (we can still have a MC capable driver, but inputs/outputs
> >>>>>>> controlled via V4L2 no? When the driver doesn't offer the necessary link
> >>>>>>> controls via MC), then checking if V4L2_CAP_MC then use the name "Media
> >>>>>>> Controller Input" is not enough.
> >>>>>>> If it is the second case, then wouldn't it be better to name it
> >>>>>>> V4L2_CAP_MC_IO ?
> >>>>>
> >>>>> It's the second case. I would probably name it V4L2_CAP_IO_MC. But I also
> >>>>> feel that we need a V4L2_IN/OUT_CAP_MC as well. Because the existing
> >>>>> V4L2_IN/OUT_CAP flags make no sense in this case.
> >>>>
> >>>> I'm not sure to see any use for V4L2_IN/OUT_CAP_MC from an application point
> >>>> of view. I'd rather avoid adding flags unless there's a real use for them.
> >>>
> >>>
> >>> Just to clarify, should this capability flag be set in struct
> >>> v4l2_input/struct v4l2_output through VIDIOC_ENUMINPUT/,
> >>> VIDIOC_ENUMOUTPUT? Or should it be set in struct v4l2_capability
> >>> through VIDIOC_QUERYCAP ?
> >>> Because if it is the first case, the I feel we should have two flags
> >>> V4L2_IN/OUT_CAP_MC in the API to follow the current convention, but this
> >>> kinda implies that we could have a driver that allows both flags to be
> >>> set differently.
> >>> Setting a V4L2_IO_CAP_MC at struct v4l2_capability would avoid this
> >>> interpretation.
> >>
> >> There are two different capability flags being discussed here.
> >>
> >> The first is V4L2_CAP_IO_MC for struct v4l2_capability: if set, then the
> >> inputs and outputs are controlled by the media controller and not through
> >> the video device.
> >>
> >> I think everyone agrees with that capability flag.
> >
> > What exactly would that capability flag tell?
> >
> > That the driver implements Media controller support? Or that any pipeline
> > configuration works through Media controller?
>
> The second. Otherwise the capability flag would be V4L2_CAP_MC :-)
>
> Note that a driver can implement the MC, but not need it to setup the
> pipeline. I.e. adding the MC to bttv would make no change to how you select
> an input.
>
> > We've discussed device profiles in the past. Should this capability flag
> > tell about a device profile? We haven't documented the profiles, but in
> > practice the V4L2 video nodes are a data interface only on MC-enabled
> > drivers. That suggests that if there is input selection to be done, that
> > would take place on a control interface, i.e. on a V4L2 sub-device node.
> >
> > I think it'd be good to have a single flag for this rather than small hints
> > here and there of what kind of an interface the user is dealing with.
> >
>
> Well, the device profile here is that you need to use the MC to configure
> the pipeline for this input as opposed to being able to just use S_INPUT.
Ack. Sounds fine to me.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-06-30 13:50 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 16:02 [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT Helen Koike
2017-03-30 16:02 ` [PATCH 2/2] [media] docs-rst: " Helen Koike
2017-03-30 20:26 ` Sakari Ailus
2017-03-31 9:47 ` Mauro Carvalho Chehab
2017-03-30 19:56 ` [PATCH RFC 1/2] [media] v4l2: " Laurent Pinchart
2017-03-31 2:39 ` Helen Koike
2017-03-31 3:55 ` Helen Koike
2017-03-31 9:42 ` Mauro Carvalho Chehab
2017-03-31 9:51 ` Hans Verkuil
2017-03-31 8:41 ` Laurent Pinchart
2017-03-31 9:33 ` Mauro Carvalho Chehab
2017-03-31 8:29 ` Hans Verkuil
2017-03-31 8:41 ` Laurent Pinchart
2017-03-31 9:08 ` Hans Verkuil
2017-03-31 9:57 ` Mauro Carvalho Chehab
2017-04-03 15:11 ` Helen Koike
2017-04-04 13:22 ` Sakari Ailus
2017-04-07 9:46 ` Hans Verkuil
2017-04-07 9:53 ` Laurent Pinchart
2017-06-06 16:22 ` Helen Koike
2017-06-12 9:26 ` Hans Verkuil
2017-06-30 11:03 ` Sakari Ailus
2017-06-30 11:13 ` Hans Verkuil
2017-06-30 13:49 ` Sakari Ailus
2017-04-07 9:36 ` Hans Verkuil
2017-06-14 4:50 ` [PATCH v2] [media] v4l2: add V4L2_CAP_IO_MC Helen Koike
2017-06-19 11:15 ` Hans Verkuil
2017-06-19 17:49 ` Helen Koike
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.