From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C062C433E0 for ; Thu, 25 Jun 2020 10:29:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4048220679 for ; Thu, 25 Jun 2020 10:29:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="SNK4VxQ4" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404067AbgFYK3k (ORCPT ); Thu, 25 Jun 2020 06:29:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37884 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404026AbgFYK3V (ORCPT ); Thu, 25 Jun 2020 06:29:21 -0400 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B22D8C061573; Thu, 25 Jun 2020 03:29:20 -0700 (PDT) Received: from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E13F9521; Thu, 25 Jun 2020 12:29:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1593080959; bh=fzhwVwT7blmI0ykjwNQs323H5w5gdXRqNujjIggQOA4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SNK4VxQ4RVMFiE15hTal6+mfRo2z7GqjOQa22J++z/ckXh+C5mdrobHS3H7YyaNAt 8IDbitPUAKPz1m3MkZmOjnpMCzHdyRZFF2XZ2r+jMWPTeoiLcruaVWRVArU5Vmzm68 Fo2ZPzlKJMXZ4uYUWMPD2iJMXY9xW7fVfNgwUMCg= Date: Thu, 25 Jun 2020 13:29:17 +0300 From: Laurent Pinchart To: Ramzi Ben Meftah Cc: Jacopo Mondi , niklas soderlund , Kieran Bingham , Mauro Carvalho Chehab , Hans Verkuil , Sakari Ailus , Janusz Krzysztofik , Steve Longerbeam , Ezequiel Garcia , Arnd Bergmann , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Michael Rodin , efriedrich@de.adit-jv.com, erosca@de.adit-jv.com Subject: Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT Message-ID: <20200625102701.GG5865@pendragon.ideasonboard.com> References: <1592301619-17631-1-git-send-email-rbmeftah@de.adit-jv.com> <20200624075307.hl6wew7vr5ue225t@uno.localdomain> <20200625020138.GW5980@pendragon.ideasonboard.com> <20200625093046.GA91893@vmlxhi-110.adit-jv.com> <20200625094724.GE5865@pendragon.ideasonboard.com> <20200625101835.GA5081@vmlxhi-110.adit-jv.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200625101835.GA5081@vmlxhi-110.adit-jv.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ramzi, On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote: > On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote: > > On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote: > >> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote: > >>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote: > >>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote: > >>>>> From: Steve Longerbeam > >>>> > >>>> +Niklas, +Laurent > >>>> > >>>> Niklas, Laurent, how does this play with CAP_IO_MC ? > >>> > >>> I don't think it's related to CAP_IO_MC, but I don't think it's a good > >>> idea either :-) Routing doesn't go through the subdev [gs]_input > >>> operations in MC-based drivers. It should be configured through link > >>> setup instead. This patch goes in the wrong direction, sorry Steve. > >> > >> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative > >> with Media Controller? > > > > No there isn't at the moment. I'm not opposed to adding such a feature, > > but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a > > subdev pad operation (v4l2_subdev_pad_ops), not a video operation > > (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input, > > as it would retrieve properties of the input corresponding to the pad, > > not enumerate inputs. > > Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill > this need. But, seems this is not expose to user space although many drivers > do implememt it. > Should I add VIDIOC_SUBDEV_G_INPUT_STATUS? Isn't g_input_status a video operation ? I would propose adding a g_input_status pad operation, and expose that to userspace. We should take that as an opportunity to consider designing that new operation from scratch (possibly naming it differently) and make sure it could address both analog and digital systems (for instance being able to report the status of an SDI input). > >>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT > >>>>> ioctls for use via v4l2 subdevice node. > >>>>> > >>>>> This commit should probably not be pushed upstream, because the (old) > >>>>> idea of video inputs conflicts with the newer concept of establishing > >>>>> media links between src->sink pads. > >>>>> > >>>>> However it might make sense for some subdevices to support enum/get/set > >>>>> inputs. One example would be the analog front end subdevice for the > >>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs > >>>>> can be done without requiring the implementation of media entities that > >>>>> would define the analog source for which to establish a media link. > >>>>> > >>>>> Signed-off-by: Steve Longerbeam > >>>>> --- > >>>>> drivers/media/v4l2-core/v4l2-subdev.c | 9 +++++++++ > >>>>> include/media/v4l2-subdev.h | 11 +++++++++++ > >>>>> 2 files changed, 20 insertions(+) > >>>>> > >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> index 6b989fe..73fbfe9 100644 > >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > >>>>> return -ENOTTY; > >>>>> return v4l2_querymenu(vfh->ctrl_handler, arg); > >>>>> > >>>>> + case VIDIOC_ENUMINPUT: > >>>>> + return v4l2_subdev_call(sd, video, enuminput, arg); > >>>>> + > >>>>> + case VIDIOC_G_INPUT: > >>>>> + return v4l2_subdev_call(sd, video, g_input, arg); > >>>>> + > >>>>> + case VIDIOC_S_INPUT: > >>>>> + return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg); > >>>>> + > >>>>> case VIDIOC_G_CTRL: > >>>>> if (!vfh->ctrl_handler) > >>>>> return -ENOTTY; > >>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >>>>> index f7fe78a..6e1a9cd 100644 > >>>>> --- a/include/media/v4l2-subdev.h > >>>>> +++ b/include/media/v4l2-subdev.h > >>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc { > >>>>> * @g_input_status: get input status. Same as the status field in the > >>>>> * &struct &v4l2_input > >>>>> * > >>>>> + * @enuminput: enumerate inputs. Should return the same input status as > >>>>> + * @g_input_status if the passed input index is the currently active > >>>>> + * input. > >>>>> + * > >>>>> + * @g_input: returns the currently active input index. > >>>>> + * > >>>>> + * @s_input: set the active input. > >>>>> + * > >>>>> * @s_stream: used to notify the driver that a video stream will start or has > >>>>> * stopped. > >>>>> * > >>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops { > >>>>> int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std); > >>>>> int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std); > >>>>> int (*g_input_status)(struct v4l2_subdev *sd, u32 *status); > >>>>> + int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input); > >>>>> + int (*g_input)(struct v4l2_subdev *sd, u32 *index); > >>>>> + int (*s_input)(struct v4l2_subdev *sd, u32 index); > >>>>> int (*s_stream)(struct v4l2_subdev *sd, int enable); > >>>>> int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect); > >>>>> int (*g_frame_interval)(struct v4l2_subdev *sd, -- Regards, Laurent Pinchart