Re: [PATCH RFC 1/2] [media] v4l2: add V4L2_INPUT_TYPE_DEFAULT

[Helen Koike <helen.koike@collabora.com>]

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