Re: [PATCH 13/15] mediactl: Add media device ioctl API

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Sakari,

On 02/18/2016 01:09 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Mon, Feb 15, 2016 at 02:06:06PM +0100, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> Thanks for the review.
>>
>> On 02/15/2016 01:41 PM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> Jacek Anaszewski wrote:
>>>> Ioctls executed on complex media devices need special handling.
>>>> For instance some ioctls need to be targeted for specific sub-devices,
>>>> depending on the media device configuration. The APIs being introduced
>>>> address such requirements.
>>>>
>>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>   utils/media-ctl/Makefile.am          |    2 +-
>>>>   utils/media-ctl/libv4l2media_ioctl.c |  404 ++++++++++++++++++++++++++++++++++
>>>>   utils/media-ctl/libv4l2media_ioctl.h |   48 ++++
>>>>   3 files changed, 453 insertions(+), 1 deletion(-)
>>>>   create mode 100644 utils/media-ctl/libv4l2media_ioctl.c
>>>>   create mode 100644 utils/media-ctl/libv4l2media_ioctl.h
>>>>
>>>> diff --git a/utils/media-ctl/Makefile.am b/utils/media-ctl/Makefile.am
>>>> index 3e883e0..7f18624 100644
>>>> --- a/utils/media-ctl/Makefile.am
>>>> +++ b/utils/media-ctl/Makefile.am
>>>> @@ -1,6 +1,6 @@
>>>>   noinst_LTLIBRARIES = libmediactl.la libv4l2subdev.la libmediatext.la
>>>>
>>>> -libmediactl_la_SOURCES = libmediactl.c mediactl-priv.h
>>>> +libmediactl_la_SOURCES = libmediactl.c mediactl-priv.h libv4l2media_ioctl.c libv4l2media_ioctl.h
>>>>   libmediactl_la_CFLAGS = -static $(LIBUDEV_CFLAGS)
>>>>   libmediactl_la_LDFLAGS = -static $(LIBUDEV_LIBS)
>>>>
>>>> diff --git a/utils/media-ctl/libv4l2media_ioctl.c b/utils/media-ctl/libv4l2media_ioctl.c
>>>> new file mode 100644
>>>> index 0000000..b186121
>>>> --- /dev/null
>>>> +++ b/utils/media-ctl/libv4l2media_ioctl.c
>>>> @@ -0,0 +1,404 @@
>>>> +/*
>>>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
>>>> + *              http://www.samsung.com
>>>> + *
>>>> + * Author: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU Lesser General Public License as published by
>>>> + * the Free Software Foundation; either version 2.1 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <errno.h>
>>>> +#include <stdlib.h>
>>>> +#include <sys/syscall.h>
>>>> +#include <unistd.h>
>>>> +
>>>> +#include <linux/videodev2.h>
>>>> +
>>>> +#include "libv4l2media_ioctl.h"
>>>> +#include "mediactl-priv.h"
>>>> +#include "mediactl.h"
>>>> +#include "v4l2subdev.h"
>>>> +
>>>> +#define VIDIOC_CTRL(type)					\
>>>> +	((type) == VIDIOC_S_CTRL ? "VIDIOC_S_CTRL" :		\
>>>> +				   "VIDIOC_G_CTRL")
>>>> +
>>>> +#define VIDIOC_EXT_CTRL(type)					\
>>>> +	((type) == VIDIOC_S_EXT_CTRLS ? 			\
>>>> +		"VIDIOC_S_EXT_CTRLS"	:			\
>>>> +		 ((type) == VIDIOC_G_EXT_CTRLS ? 		\
>>>> +				    "VIDIOC_G_EXT_CTRLS" :	\
>>>> +				    "VIDIOC_TRY_EXT_CTRLS"))
>>>> +
>>>> +#define SYS_IOCTL(fd, cmd, arg) \
>>>> +	syscall(SYS_ioctl, (int)(fd), (unsigned long)(cmd), (void *)(arg))
>>>> +
>>>> +
>>>> +int media_ioctl_ctrl(struct media_device *media, int request,
>>>
>>> unsigned int request
>>
>> OK.
>>
>>>
>>>> +		     struct v4l2_control *arg)
>>>
>>> I wonder if it'd make sense to always use v4l2_ext_control instead. You
>>> can't access 64-bit integer controls with VIDIOC_S_CTRL for instance.
>>
>> This function is meant to handle VIDIOC_S_CTRL/VIDIOC_G_CTRL ioctls.
>> For ext ctrls there is media_ioctl_ext_ctrl().
>
> Is there any reason not to use extended control always?
>
> In other words, do we have a driver that does support Media controller but
> does not support extended controls?

Shouldn't we support non-extended controls for backward compatibility
reasons? I am not aware of the policy in this matter.

>>> As this is a user space library, I'd probably add a function to handle
>>> S/G/TRY control each.
>>
>> There is media_ioctl_ext_ctrl() that handles VIDIOC_S_EXT_CTRLS,
>> VIDIOC_G_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS.
>>
>>> Have you considered binding the control to a video node rather than a
>>> media device? We have many sensors on current media devices already, and
>>> e.g. exposure time control can be found in multiple sub-devices.
>>
>> Doesn't v4l2-ctrl-redir config entry address that?
>
> How does it work if you have, say, two video nodes where you can capture
> images from a different sensor? I.e. your media graph could look like this:
>
> 	sensor0 -> CSI-2 0 -> video0
>
> 	sensor1 -> CSI-2 1 -> video1

Exemplary config settings for this case:

v4l2-ctrl-redir 0x0098091f -> "sensor0"
v4l2-ctrl-redir 0x0098091f -> "sensor1"

In media_ioctl_ctrl the v4l2_subdev_get_pipeline_entity_by_cid(media,
ctrl.id) is called which walks through the pipeline and checks if there
has been a v4l2 control redirection defined for given entity.

If no redirection is defined then the control is set on the first
entity in the pipeline that supports it. Effectively, for this
arrangement no redirection would be required if the control
is to be set on sensors. It would be required if we wanted
to bind the control to the videoN entity. Now I am wondering
if I should change the entry name to v4l2-ctrl-binding, or maybe
someone has better idea?

BTW, are there some unique identifiers added to the entity names if
more than one entity of a name is to be registered? E.g. what would
happen if I had two S5C73M3 sensors in a media device? I assumed that
entity names are unique.

-- 
Best regards,
Jacek Anaszewski