From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:50209 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753349AbcCVJgJ (ORCPT ); Tue, 22 Mar 2016 05:36:09 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O4F000QTPC6W450@mailout3.w1.samsung.com> for linux-media@vger.kernel.org; Tue, 22 Mar 2016 09:36:07 +0000 (GMT) Message-id: <56F11205.8000903@samsung.com> Date: Tue, 22 Mar 2016 10:36:05 +0100 From: Jacek Anaszewski MIME-version: 1.0 To: Sakari Ailus Cc: Sakari Ailus , linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com, gjasny@googlemail.com, hdegoede@redhat.com, hverkuil@xs4all.nl Subject: Re: [PATCH 13/15] mediactl: Add media device ioctl API References: <1453133860-21571-1-git-send-email-j.anaszewski@samsung.com> <1453133860-21571-14-git-send-email-j.anaszewski@samsung.com> <56C1C775.2090002@linux.intel.com> <56C1CD3E.6090108@samsung.com> <20160218120951.GO32612@valkosipuli.retiisi.org.uk> <56C5C3C0.7000808@samsung.com> <20160321000714.GE11084@valkosipuli.retiisi.org.uk> In-reply-to: <20160321000714.GE11084@valkosipuli.retiisi.org.uk> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Sakari, On 03/21/2016 01:07 AM, Sakari Ailus wrote: > Hi Jacek, > > On Thu, Feb 18, 2016 at 02:14:40PM +0100, Jacek Anaszewski wrote: >> 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 >>>>>> Acked-by: Kyungmin Park >>>>>> --- >>>>>> 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 >>>>>> + * >>>>>> + * 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 >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +#include >>>>>> + >>>>>> +#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. > > To put it bluntly, supporting the non-extended controls in this use is waste > of time IMHO. OK, I'll drop the non-ext controls related API then. >>>>> 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. > > That's still based on media device, not video device. Two video devices may > be part of different pipelines, and a different sensor as well. > > Redirecting the controls should be based on a video node, not media device. Why do you consider it as based on a media device? I'd rather say that it is based on media entity, so indirectly based on media device. Is it what you have on mind? >> >> 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. > > Yes. Currently we've got away with the problem by adding the i2c address of > i2c devices to the entity name. The proper solution (there was a lengthy > discussion on it ~ a year ago, too late to try to find out exactly when) > would be to provide all the available information on the entity to the user > space using the property API (which we don't have yet). The entity name > remains unique in most situations but it's not necessarily stable. > I assume that the fact that they're not stable mean that we cannot rely on the entity name. Using sub-dev names and video device names seems reasonable then. -- Best regards, Jacek Anaszewski