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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6EE22C43334 for ; Fri, 17 Jun 2022 22:41:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=v3LM2P3A5mrsu4SvIb86W6AH3Hi8/moEGUNdQVknWas=; b=Q2KHqbrFz9+jZw qaAZ87quqgfZljL8Xf/Sq4U4X31cLYj9JDPoU5NgetrbmglIaASlN8TgMJ9jWih/+QfE3oE3nODmN uok0H/7/n+GhpL5VZMx53LVgX0sUJ0V3KPVzUzOjN4gwjc+Zlzwj00du9VkfDYJg0ztv7X1FiYraW TU8YEg7S3j8nZ06HRpirKZ7Pp7I9s8WlS4tPoWfHIDjWk10agq/aJBYbxe/DGA4M+DF2Cc2k22Bdh zrIYPW6nIxzPJXGnD9y6fJfgI07W7yf5Am85VDN1zuD5+tc6sAfCv6exnJ8DmzegGA72kMB/yPXcc XXqL0Z8bNYUx9OfUniTQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o2KeA-009S1n-Ev; Fri, 17 Jun 2022 22:40:58 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o2Ke7-009S1C-C7 for linux-rockchip@lists.infradead.org; Fri, 17 Jun 2022 22:40:57 +0000 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 48DDA383; Sat, 18 Jun 2022 00:40:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1655505653; bh=0Oi+t3IpYUoLnREESGd08oGiwt3Yjtagh5Tp6AnhQn4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tHJ2hfuD9Mox14KYiIHeOKw9c3DjuvOiAALqCaEN00XbgVArbSWECWtrTm8USur68 eOyR6wcWRSReWJyPDGK2aJEWYacWz8VE0EVHgeZRKUB4I7ImkUZb6oiWq8QTiF9aLz rONssEARhL056JFwWEuUyqmJ79/9dQ25tzD+OQHA= Date: Sat, 18 Jun 2022 01:40:41 +0300 From: Laurent Pinchart To: Daniel Scally Cc: Paul Elder , linux-media@vger.kernel.org, dafna@fastmail.com, heiko@sntech.de, jeanmichel.hautbois@ideasonboard.com, jacopo@jmondi.org, helen.koike@collabora.com, linux-rockchip@lists.infradead.org Subject: Re: [PATCH 37/55] media: mc-entity: Add a new helper function to get a remote pad Message-ID: References: <20220614191127.3420492-1-paul.elder@ideasonboard.com> <20220614191127.3420492-38-paul.elder@ideasonboard.com> <69142509-842a-81e5-2bce-69e2dbb9e4d2@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <69142509-842a-81e5-2bce-69e2dbb9e4d2@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220617_154055_610317_5AB65969 X-CRM114-Status: GOOD ( 43.56 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org Hi Daniel, On Fri, Jun 17, 2022 at 11:33:03PM +0100, Daniel Scally wrote: > On 17/06/2022 22:34, Daniel Scally wrote: > > On 14/06/2022 20:11, Paul Elder wrote: > >> From: Laurent Pinchart > >> > >> The media_entity_remote_pad() helper function returns the first remote > >> pad it find connected to a given pad. Beside being possibly ill-named > >> (as it operates on a pad, not an entity) and non-deterministic (as it > >> stops at the first enabled link), the fact that it returns the first > >> match makes it unsuitable for drivers that need to guarantee that a > >> single link is enabled, for instance when an entity can process data > >> from one of multiple sources at a time. > >> > >> For those use cases, add a new helper function, > >> media_entity_remote_pad_unique(), that operates on an entity and returns > >> a remote pad, with a guarantee that only one link is enabled. To ease > >> its use in drivers, also add an inline wrapper that locates source pads > >> specifically. A wrapper that locates sink pads can easily be added when > >> needed. > >> > >> Signed-off-by: Laurent Pinchart > >> --- > >> Documentation/driver-api/media/mc-core.rst | 4 +- > >> drivers/media/mc/mc-entity.c | 38 ++++++++++++++++++ > >> include/media/media-entity.h | 45 ++++++++++++++++++++++ > >> 3 files changed, 85 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst > >> index 02481a2513b9..a2d1e32e3abb 100644 > >> --- a/Documentation/driver-api/media/mc-core.rst > >> +++ b/Documentation/driver-api/media/mc-core.rst > >> @@ -186,8 +186,8 @@ is required and the graph structure can be freed normally. > >> > >> Helper functions can be used to find a link between two given pads, or a pad > >> connected to another pad through an enabled link > >> -:c:func:`media_entity_find_link()` and > >> -:c:func:`media_entity_remote_pad()`. > >> +(:c:func:`media_entity_find_link()`, :c:func:`media_entity_remote_pad()` and > >> +:c:func:`media_entity_remote_source_pad()`). > >> > >> Use count and power handling > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >> index 11f5207f73aa..1febf5a86be6 100644 > >> --- a/drivers/media/mc/mc-entity.c > >> +++ b/drivers/media/mc/mc-entity.c > >> @@ -9,6 +9,7 @@ > >> */ > >> > >> #include > >> +#include > >> #include > >> #include > >> #include > >> @@ -920,6 +921,43 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad) > >> } > >> EXPORT_SYMBOL_GPL(media_entity_remote_pad); > >> > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type) > >> +{ > >> + struct media_pad *pad = NULL; > >> + struct media_link *link; > >> + > >> + list_for_each_entry(link, &entity->links, list) { > >> + struct media_pad *local_pad; > >> + struct media_pad *remote_pad; > >> + > >> + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) > >> + continue; > > > > Does this need another guard here to make sure the link isn't an > > ancillary link? Only likely to happen at least in the immediate future > > where the entity represents a camera sensor, so possibly not applicable > > here - I couldn't find where the new function is used in the series to > > check. I _think_ it would actually work ok regardless...media_gobj > > type-punning makes my brain ache, but I think the local_pad->entity == > > entity comparison would actually compare the entity->name member of the > > entity at the end of an ancillary link to the entity parameter, not find > > a match and so continue the loop without failing, but that feels a bit > > sub-optimal. > > Or perhaps a better approach would be to provide something like a > "list_for_each_data_link()" iterator - the potential problem here (as > with Quentin's recent issue with the rkisp1 driver) is the assumption > that all links are data links, so maybe it's best to just guarantee that > if we can. I agree with you, without a dedicated iterator, we're bound to repeat the mistake over and over. Would you like to submit a patch to add that iterator, or should I ? I'd name it for_each_media_entity_data_link() or something similar. > >> + > >> + if (type == MEDIA_PAD_FL_SOURCE) { > >> + local_pad = link->sink; > >> + remote_pad = link->source; > >> + } else { > >> + local_pad = link->source; > >> + remote_pad = link->sink; > >> + } > >> + > >> + if (local_pad->entity == entity) { > >> + if (pad) > >> + return ERR_PTR(-ENOTUNIQ); > >> + > >> + pad = remote_pad; > >> + } > >> + } > >> + > >> + if (!pad) > >> + return ERR_PTR(-ENOLINK); > >> + > >> + return pad; > >> +} > >> +EXPORT_SYMBOL_GPL(media_entity_remote_pad_unique); > >> + > >> static void media_interface_init(struct media_device *mdev, > >> struct media_interface *intf, > >> u32 gobj_type, > >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h > >> index a9a1c0ec5d1c..33d5f52719a0 100644 > >> --- a/include/media/media-entity.h > >> +++ b/include/media/media-entity.h > >> @@ -859,6 +859,51 @@ struct media_link *media_entity_find_link(struct media_pad *source, > >> */ > >> struct media_pad *media_entity_remote_pad(const struct media_pad *pad); > >> > >> +/** > >> + * media_entity_remote_pad_unique - Find a remote pad connected to an entity > >> + * @entity: The entity > >> + * @type: The type of pad to find (MEDIA_PAD_FL_SINK or MEDIA_PAD_FL_SOURCE) > >> + * > >> + * Search for and return a remote pad of @type connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source or sink at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +struct media_pad * > >> +media_entity_remote_pad_unique(const struct media_entity *entity, > >> + unsigned int type); > >> + > >> +/** > >> + * media_entity_remote_source_pad - Find a remote source pad connected to an entity > >> + * @entity: The entity > >> + * > >> + * Search for and return a remote source pad connected to @entity through an > >> + * enabled link. If multiple (or no) remote pads match these criteria, an error > >> + * is returned. > >> + * > >> + * The uniqueness constraint makes this helper function suitable for entities > >> + * that support a single active source at a time. > >> + * > >> + * Return: A pointer to the remote pad, or one of the following error pointers > >> + * if an error occurs: > >> + * > >> + * * -ENOTUNIQ - Multiple links are enabled > >> + * * -ENOLINK - No connected pad found > >> + */ > >> +static inline struct media_pad * > >> +media_entity_remote_source_pad(const struct media_entity *entity) > >> +{ > >> + return media_entity_remote_pad_unique(entity, MEDIA_PAD_FL_SOURCE); > >> +} > >> + > >> /** > >> * media_entity_is_streaming - Test if an entity is part of a streaming pipeline > >> * @entity: The entity -- Regards, Laurent Pinchart _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip