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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 07937C83003 for ; Fri, 9 Jun 2023 12:57:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240659AbjFIM5n (ORCPT ); Fri, 9 Jun 2023 08:57:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58546 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240952AbjFIM5i (ORCPT ); Fri, 9 Jun 2023 08:57:38 -0400 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CAFD31AB for ; Fri, 9 Jun 2023 05:57:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1686315454; x=1717851454; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=Qhrb+uM2II4IdFYv9/IvOgMZJHsyg+BD+3iBP4I7ftA=; b=G+XdT5mF8BP4umkozOOA3QvgxCCRqyBJLbNEvWYxpSFQ59MGCOhHSBIl zpA+wAcu5GP7zeDYy6LVi48Jk3MRnxQ4N+5EiSxn/a0oKOoDtkXU3+Vea eDrf7GXXisaEnSEiXqwsYaiFMDP1J0a5+8OY1S/xWwHy+JscHvj4F1Tnp B2nxHOkKW/wVxliAOfSFG1yv9yHACHagrtLi5nrijcWNmQBdJ6onN/RgN r8hKjQMFs6AJBo9HNQyWRz2xeytNKZ+dNl/Opq32+W+KKUHVecYe+qZUE Mr6RGJDo17ytkvrM9u+cCWPuFjixVdxI5ETUgNzoH26TikW4tfeE42ynQ w==; X-IronPort-AV: E=McAfee;i="6600,9927,10736"; a="360947224" X-IronPort-AV: E=Sophos;i="6.00,229,1681196400"; d="scan'208";a="360947224" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jun 2023 05:55:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10736"; a="800208465" X-IronPort-AV: E=Sophos;i="6.00,229,1681196400"; d="scan'208";a="800208465" Received: from turnipsi.fi.intel.com (HELO kekkonen.fi.intel.com) ([10.237.72.44]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jun 2023 05:55:38 -0700 Received: from kekkonen.localdomain (localhost [IPv6:::1]) by kekkonen.fi.intel.com (Postfix) with SMTP id A478811F9D2; Fri, 9 Jun 2023 15:55:35 +0300 (EEST) Date: Fri, 9 Jun 2023 12:55:35 +0000 From: Sakari Ailus To: Laurent Pinchart Cc: Tomi Valkeinen , linux-media@vger.kernel.org, bingbu.cao@intel.com, hongju.wang@intel.com Subject: Re: [RFC 3/7] media: uapi: v4l: Document source routes Message-ID: References: <20230505215257.60704-1-sakari.ailus@linux.intel.com> <20230505215257.60704-4-sakari.ailus@linux.intel.com> <5b7038ce-d897-931f-2c6e-30bdd1a30342@ideasonboard.com> <20230602095600.GH19463@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230602095600.GH19463@pendragon.ideasonboard.com> Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Laurent, On Fri, Jun 02, 2023 at 12:56:00PM +0300, Laurent Pinchart wrote: > Hello, > > On Mon, May 08, 2023 at 07:35:04PM +0300, Tomi Valkeinen wrote: > > On 08/05/2023 19:26, Sakari Ailus wrote: > > > On Mon, May 08, 2023 at 01:33:24PM +0300, Tomi Valkeinen wrote: > > >> On 06/05/2023 00:52, Sakari Ailus wrote: > > >>> Document how internal pads are used on source routes. > > >>> > > >>> Signed-off-by: Sakari Ailus > > >>> --- > > >>> .../userspace-api/media/v4l/dev-subdev.rst | 18 ++++++++++++++++++ > > >>> .../media/v4l/vidioc-subdev-g-routing.rst | 5 +++++ > > >>> include/uapi/linux/v4l2-subdev.h | 6 +++++- > > >>> 3 files changed, 28 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > >>> index a4f1df7093e8..395e06d2f0f2 100644 > > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > >>> @@ -551,6 +551,24 @@ A stream at a specific point in the media pipeline is identified by the > > >>> sub-device and a (pad, stream) pair. For sub-devices that do not support > > >>> multiplexed streams the 'stream' field is always 0. > > >>> +.. _v4l2-subdev-source-routes: > > >>> + > > >>> +Source routes > > >>> +^^^^^^^^^^^^^ > > >>> + > > >>> +Cases where a single sub-device pad is a source of multiple streams are special > > >>> +as there is no sink pad for such a route. In those cases, an internal pad is > > >>> +used as the sink pad. Such pads have the :ref:`MEDIA_PAD_FL_INTERNAL_SOURCE > > >>> +` flag set. > > >>> + > > >>> +Internal pads have all the properties of a sink pad in such case, including > > >>> +formats and selections. The format in this case is the source format of the > > >>> +stream. An internal pad always has a single stream only (0). > > >>> + > > >>> +Generally source routes are not modifiable but they can be activated and > > >>> +inactivated using the :ref:`V4L2_SUBDEV_ROUTE_FL_ACTIVE > > >>> +` flag, depending on driver capabilities. > > >>> + > > >> > > >> I find the above chapter a bit difficult to read, but I think we need to > > >> conclude on the internal-pad vs internal-source-pad discussion first. > > >> However, one point/question: > > Agreed, it's far from being clear :-( Even the first sentence, "Cases > where a single sub-device pad is a source of multiple streams are > special" is confusing, having a subdev source pad carrying multiple > streams isn't special, it is found in, for instance, FPD-Link or GMSL > receivers that transmit multiple streams from different cameras over a > single CSI-2 output. This may not be what Sakari meant here, but it can > be understood that way. The sentence continues on the next line. I'll rework this part of the documentation in any case while moving to a plain INTERNAL flag. > > We need more than 3 paragraphs here, and we need a very clear example > with a diagram. I'd recommend using a camera sensor that produces image > data and embedded data for the example, as that's a common real-life use > case. The text should present the example, explain what the problem is, > and then explain how internal pads fix it. Then it can expand on the > features and usage of internal pads in a more generic way. It's fine to have examples but I'd add them to where the other examples are located. > > > >> You write that there's only one stream in an internal pad. I wonder if > > >> that's a good or a necessary limitation... Do you see that allowing multiple > > >> streams brings extra complexity? It's still up to each driver to decide how > > >> many streams they support (most would just support a single one), so each > > >> driver can easily enforce that limit. > > > > > > Good question. As we don't seem to have a tangible reason to allow it, I'd > > > deny it until there's a use case. > > > > > > Or did you have a use case in mind? > > > > > > I thought indicating which streams will be bound together (i.e. either are > > > all disabled or enabled) could be one, but I'm not sure we need that at the > > > moment at least. > > > > I don't have nothing concrete in mind. Maybe a TPG which also generates > > some kind of metadata. But that could be implemented as two internal pads. > > > > >> I'm fine with starting with only single-stream support, but if we later need > > >> to loosen that restriction, I wonder if it'll be difficult if we have > > >> specified that there can be only one stream. I.e. will the drivers and the > > >> userspace depend on that limit. > > > > > > We can always allow what wasn't allowed previously so that's a non-issue, > > > really. But it needs to bring new functionality, otherwise there's no > > > reason to do that. > > > > It's not always that easy. If the drivers and the userspace expect that > > there's a single route with ID 0, and then with a new kernel there are > > more streams or the single stream is ID 1, things could go wrong. > > I agree with Tomi here. On the kernel side it should be fine (with an > unknown amount of pain), but I'd consider this as a userspace API > breakage. If the specifications indicates that only a single stream can > be used, applications may rely on that, and things could go wrong if new > streams are added. > > We can restrict the kernel implementation to a single stream, but the > userspace API has to indicate that multiple streams can co-exist if we > want to allow that in the future. I doubt this ever could be an actual problem. Allowing multiple streams to be merged on a source pad requires the user to first configure them. I find it hard to believe there would be an application that tried to configure routes this way (which would have always failed) and then proceeded anyway, and somehow returning success from route setup leads to routing configuration that the application couldn't handle anymore. -- Kind regards, Sakari Ailus