Re: [RFC PATCH 1/6] media: mc: Implement shared media graph

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 17111 bytes --]

Hi,

Le vendredi 19 juin 2026 à 14:26 +0900, Paul Elder a écrit :
> Currently, a media graph contains a main device whose driver is
> responsible for creating the media device. We have however recently run
> into devices that have multiple devices that can quality as a main
> device. Examples are the RK3588 which has a VICAP and two ISP
> instances, and another example is the i.MX8MP which has an ISI and two
> ISP instances. As there is currently no way to reconcile who the main
> device is in the media device, these setups simple cannot be used
> simultaneously.
> 
> This patch extends the media controller API with a "shared media graph"
> framework. This allows drivers to share a media device, thus enabling
> the setups mentioned above. Instead of owning and creating a media
> device, drivers can join-or-create a shared media device via the shared
> media graph API. The matching is done automatically based on the
> detected endpoints in the device tree.

Dropping a note, we should check if it makes sense combine this with
include/linux/component.h

> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  drivers/media/mc/Makefile          |   2 +-
>  drivers/media/mc/mc-shared-graph.c | 335 +++++++++++++++++++++++++++++
>  include/media/mc-shared-graph.h    |  92 ++++++++
>  3 files changed, 428 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/mc/mc-shared-graph.c
>  create mode 100644 include/media/mc-shared-graph.h
> 
> diff --git a/drivers/media/mc/Makefile b/drivers/media/mc/Makefile
> index 2b7af42ba59c..1d502fdc52ad 100644
> --- a/drivers/media/mc/Makefile
> +++ b/drivers/media/mc/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  mc-objs	:= mc-device.o mc-devnode.o mc-entity.o \
> -	   mc-request.o
> +	   mc-request.o mc-shared-graph.o
>  
>  ifneq ($(CONFIG_USB),)
>  	mc-objs += mc-dev-allocator.o
> diff --git a/drivers/media/mc/mc-shared-graph.c b/drivers/media/mc/mc-shared-graph.c
> new file mode 100644
> index 000000000000..c4067e5b861d
> --- /dev/null
> +++ b/drivers/media/mc/mc-shared-graph.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mc-shared-graph.c - Media Controller Shared Graph API
> + *
> + * Copyright (c) 2026 Paul Elder <paul.elder@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds the Media Controller Shared Graph API. This allows drivers
> + * to create shared media graphs or join existing media graphs from other
> + * drivers, so that they can all be in the same media graph. This allows us to
> + * have more complex media graphs chaining more complex hardware together,
> + * instead of simple async subdevs.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/fwnode.h>
> +#include <linux/kref.h>
> +#include <linux/property.h>
> +
> +#include <media/media-device.h>
> +
> +#include <media/mc-shared-graph.h>
> +
> +static LIST_HEAD(media_device_shared_list);
> +static DEFINE_MUTEX(media_device_shared_lock);
> +
> +struct media_device_shared_member {
> +	struct device *dev;
> +	struct fwnode_handle *fwnode;
> +	struct list_head list;
> +};
> +
> +struct media_device_shared_link {
> +	struct media_entity *source;
> +	u16 source_pad;
> +	struct media_entity *sink;
> +	u16 sink_pad;
> +	u32 flags;
> +	struct list_head list;
> +};
> +
> +// TODO figure out locking for when multiple drivers touch the media graph;
> +// maybe macros for shared versions?
> +struct media_device_shared {
> +	struct media_device mdev;
> +	struct list_head members;
> +	struct list_head links;
> +
> +	struct list_head list;
> +	struct kref refcount;
> +
> +	struct device *removed_device;
> +};
> +
> +static inline struct media_device_shared *
> +to_media_device_shared(struct media_device *mdev)
> +{
> +	return container_of(mdev, struct media_device_shared, mdev);
> +}
> +
> +static void media_device_shared_release(struct kref *kref)
> +{
> +	struct media_device_shared *mds =
> +		container_of(kref, struct media_device_shared, refcount);
> +
> +	dev_dbg(mds->removed_device, "%s: releasing Media Device\n", __func__);
> +
> +	mutex_lock(&media_device_shared_lock);
> +
> +	media_device_unregister(&mds->mdev);
> +	media_device_cleanup(&mds->mdev);
> +
> +	list_del(&mds->list);
> +	mutex_unlock(&media_device_shared_lock);
> +
> +	kfree(mds);
> +}
> +
> +/* Callers should hold media_device_shared_lock when calling this function */
> +static bool __media_device_shared_find_match(struct media_device_shared *mds,
> +					     struct fwnode_handle *fwnode)
> +{
> +	struct media_device_shared_member *member;
> +	struct fwnode_handle *ep;
> +	struct fwnode_handle *remote_ep;
> +	bool match = false;
> +
> +	// TODO: parse the device tree endpoints graph instead of finding just the
> +	// first-level neighbours
> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
> +		list_for_each_entry(member, &mds->members, list) {
> +			remote_ep = fwnode_graph_get_remote_port_parent(ep);
> +			match = (member->fwnode == remote_ep);
> +			fwnode_handle_put(remote_ep);
> +
> +			if (!match)
> +				continue;
> +
> +			goto match_complete;
> +		}
> +	}
> +
> +match_complete:
> +	fwnode_handle_put(ep);
> +	return match;
> +}
> +
> +/* Callers should hold media_device_shared_lock when calling this function */
> +static struct media_device *__media_device_shared_get(struct device *dev)
> +{
> +	struct media_device_shared *mds;
> +	struct media_device_shared_member *member;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	bool ret;
> +
> +	dev_dbg(dev, "%s: searching for media device for %pfwf", __func__, fwnode);
> +
> +	list_for_each_entry(mds, &media_device_shared_list, list) {
> +		ret = __media_device_shared_find_match(mds, fwnode);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (!ret)
> +		return NULL;
> +
> +	member = kzalloc_obj(*member);
> +	if (!member)
> +		return NULL;
> +
> +	member->dev = dev;
> +	member->fwnode = fwnode;
> +	list_add_tail(&member->list, &mds->members);
> +	kref_get(&mds->refcount);
> +
> +	dev_dbg(dev, "%s: %pfwf joined media device of %pfwf",
> +		__func__, fwnode,
> +		list_first_entry(&mds->members, struct media_device_shared_member, list)->fwnode);
> +
> +	return &mds->mdev;
> +}
> +
> +/* Callers should hold media_device_shared_lock when calling this function */
> +static struct media_device *__media_device_shared_create(struct device *dev)
> +{
> +	struct media_device_shared *mds;
> +	struct media_device_shared_member *member;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	int ret;
> +
> +	mds = kzalloc_obj(*mds);
> +	if (!mds)
> +		return NULL;
> +
> +	member = kzalloc_obj(*member);
> +	if (!member)
> +		goto err_free_mds;
> +
> +	media_device_init(&mds->mdev);
> +
> +	ret = media_device_register(&mds->mdev);
> +	if (ret)
> +		goto err_free_member;
> +
> +	INIT_LIST_HEAD(&mds->members);
> +	member->dev = dev;
> +	member->fwnode = fwnode;
> +	list_add_tail(&member->list, &mds->members);
> +
> +	INIT_LIST_HEAD(&mds->links);
> +
> +	kref_init(&mds->refcount);
> +	list_add_tail(&mds->list, &media_device_shared_list);
> +
> +	// TODO figure out how to reconcile this with multiple members
> +	mds->mdev.dev = dev;
> +
> +	devv_dbg(dev, "%s: Allocated media device with %pfwf at %p\n",
> +		 __func__, fwnode, &mds->mdev);
> +	return &mds->mdev;
> +
> +err_free_member:
> +	kfree(member);
> +err_free_mds:
> +	kfree(mds);
> +	return NULL;
> +}
> +
> +// TODO figure out how to resolve the identifiers (model, driver name, etc);
> +// atm it's racy and whoever gets it last wins
> +struct media_device *media_device_shared_join(struct device *dev)
> +{
> +	struct media_device *mdev;
> +
> +	mutex_lock(&media_device_shared_lock);
> +
> +	mdev = __media_device_shared_get(dev);
> +	if (!!mdev) {

Looks fun / geeky, but please don't, just do if (mdev). We really don't care
here if the value passed to the if is exactly 1 or some other non-nul value.

> +		dev_dbg(dev, "%s: found media device for %pfwf", __func__, dev_fwnode(dev));
> +		mutex_unlock(&media_device_shared_lock);
> +		return mdev;
> +	}
> +
> +	mdev = __media_device_shared_create(dev);
> +	if (!mdev) {
> +		dev_warn(dev, "%s: failed to create media device for %pfwf", __func__, dev_fwnode(dev));
> +		mutex_unlock(&media_device_shared_lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	dev_dbg(dev, "%s: created media device for %pfwf", __func__, dev_fwnode(dev));
> +	mutex_unlock(&media_device_shared_lock);
> +	return mdev;
> +}
> +EXPORT_SYMBOL_GPL(media_device_shared_join);
> +
> +void media_device_shared_leave(struct media_device *mdev, struct device *dev)
> +{
> +	struct media_device_shared *mds = to_media_device_shared(mdev);
> +	struct media_device_shared_member *member;
> +	struct media_device_shared_member *member_tmp;
> +	bool removed = false;
> +
> +	mutex_lock(&media_device_shared_lock);
> +
> +	list_for_each_entry_safe(member, member_tmp, &mds->members, list) {
> +		if (member->dev == dev) {
> +			list_del(&member->list);
> +			kfree(member);
> +			removed = true;
> +		}
> +	}
> +
> +	if (!removed)
> +		dev_err(dev, "%s: %pfwf trying to leave from graph in which not a member",
> +			__func__, dev_fwnode(dev));
> +
> +	mds->removed_device = dev;
> +	mutex_unlock(&media_device_shared_lock);
> +	kref_put(&mds->refcount, media_device_shared_release);
> +}
> +EXPORT_SYMBOL_GPL(media_device_shared_leave);
> +
> +int media_device_shared_join_link_source(struct media_device *mdev,
> +					 struct device *dev,
> +					 struct media_entity *source,
> +					 u16 source_pad, u32 flags)
> +{
> +	struct media_device_shared *mds = to_media_device_shared(mdev);
> +	struct media_device_shared_link *link;
> +	struct media_device_shared_link *link_tmp;
> +	int ret = 0;
> +
> +	mutex_lock(&media_device_shared_lock);
> +
> +	/*
> +	 * TODO Figure out flags. Should we use greatest common denominator? Or
> +	 * prioritize sink? Or whoever wins the race? For now we just take the flags
> +	 * from the sink.
> +	 *
> +	 * TODO Figure out how to actually do the matching. For now we just match
> +	 * whoever comes in first. This works with the simple example we're running
> +	 * with now (rkcif + one rkisp2) but with setups with multiple copies of
> +	 * hardware this will cause problems, like with rkcif + two rkisp2 and
> +	 * imx8-isi + two rkisp1.
> +	 */
> +	list_for_each_entry_safe(link, link_tmp, &mds->links, list) {
> +		if (link->sink) {
> +			ret = media_create_pad_link(source, source_pad,
> +						    link->sink, link->sink_pad,
> +						    link->flags);
> +			list_del(&link->list);
> +			kfree(link);
> +			goto exit_join_link_source;
> +		}
> +	}
> +
> +	link = kzalloc_obj(*link);
> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto exit_join_link_source;
> +	}
> +
> +	link->source = source;
> +	link->source_pad = source_pad;
> +	link->flags = flags;
> +	list_add_tail(&link->list, &mds->links);
> +
> +exit_join_link_source:
> +	mutex_unlock(&media_device_shared_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(media_device_shared_join_link_source);
> +
> +// TODO deduplicate from above
> +int media_device_shared_join_link_sink(struct media_device *mdev,
> +				       struct device *dev,
> +				       struct media_entity *sink,
> +				       u16 sink_pad, u32 flags)
> +{
> +	struct media_device_shared *mds = to_media_device_shared(mdev);
> +	struct media_device_shared_link *link;
> +	struct media_device_shared_link *link_tmp;
> +	int ret = 0;
> +
> +	mutex_lock(&media_device_shared_lock);
> +
> +	list_for_each_entry_safe(link, link_tmp, &mds->links, list) {
> +		if (link->source) {
> +			ret = media_create_pad_link(link->source, link->source_pad,
> +						    sink, sink_pad,
> +						    flags);
> +			list_del(&link->list);
> +			kfree(link);
> +			goto exit_join_link_sink;
> +		}
> +	}
> +
> +	link = kzalloc_obj(*link);
> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto exit_join_link_sink;
> +	}
> +
> +	link->sink = sink;
> +	link->sink_pad = sink_pad;
> +	link->flags = flags;
> +	list_add_tail(&link->list, &mds->links);
> +
> +exit_join_link_sink:
> +	mutex_unlock(&media_device_shared_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(media_device_shared_join_link_sink);
> diff --git a/include/media/mc-shared-graph.h b/include/media/mc-shared-graph.h
> new file mode 100644
> index 000000000000..487325163f84
> --- /dev/null
> +++ b/include/media/mc-shared-graph.h
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mc-shared-graph.h - Media Controller Shared Graph API
> + *
> + * Copyright (c) 2026 Paul Elder <paul.elder@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds the Media Controller Shared Graph API. This allows drivers
> + * to create shared media graphs or join existing media graphs from other
> + * drivers, so that they can all be in the same media graph. This allows us to
> + * have more complex media graphs chaining more complex hardware together,
> + * instead of simple async subdevs.
> + */
> +
> +#include <linux/types.h>
> +
> +#ifndef _MEDIA_SHARED_GRAPH_H
> +#define _MEDIA_SHARED_GRAPH_H
> +
> +struct device;
> +struct media_device;
> +struct media_entity;
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +/**
> + * media_device_shared_join() - Join or create a new shared media device
> + *
> + * @dev:		struct &device pointer
> + *
> + * This is the entrance function for a device to join or create a new shared
> + * media device. It searches for an existing shared media device based on the
> + * neighbours in the device's device tree ports node. If found, then this
> + * functions returns the existing shared media device and joins it. If one is
> + * not found then one is created and initialized and returned.
> + */
> +struct media_device *media_device_shared_join(struct device *dev);
> +
> +/**
> + * media_device_shared_leave() - Leave the shared media device.
> + *
> + * @mdev:		struct &media_device pointer
> + * @dev:		struct &device pointer
> + *
> + * This function makes the device leave the shared media device. When all
> + * members have left the media device it will be freed.
> + */
> +void media_device_shared_leave(struct media_device *mdev, struct device *dev);
> +
> +/**
> + * media_device_shared_join_link_source() - Register a link source in the shared media device
> + *
> + * @mdev: The struct &media_device pointer that is part of a shared media device
> + * @dev: struct &device pointer
> + * @source: The link source
> + * @source_pad: The pad
> + * @flags: The flags
> + *
> + * This function registers with the shared media device the source part of a
> + * link. When the shared media device receives the matching sink part of a link
> + * via media_device_shared_join_link_sink() then the link will be fully created.
> + */
> +int media_device_shared_join_link_source(struct media_device *mdev,
> +					 struct device *dev,
> +					 struct media_entity *source,
> +					 u16 source_pad, u32 flags);
> +
> +/**
> + * media_device_shared_join_link_sink() - Register a link sink in the shared media device
> + *
> + * Same as media_device_shared_join_link_source() but for sink instead of
> + * source.
> + */
> +int media_device_shared_join_link_sink(struct media_device *mdev,
> +				       struct device *dev,
> +				       struct media_entity *sink,
> +				       u16 sink_pad, u32 flags);
> +#else
> +static inline struct media_device *media_device_shared_join(struct device *dev)
> +{ return NULL; }
> +static inline void media_device_shared_leave(struct media_device *mdev,
> +					     struct device *dev) { }
> +static inline int media_device_shared_join_link_source(struct media_device *mdev,
> +						       struct device *dev,
> +						       struct media_entity *source,
> +						       u16 source_pad, u32 flags) { }
> +static inline int media_device_shared_join_link_sink(struct media_device *mdev,
> +						     struct device *dev,
> +						     struct media_entity *sink,
> +						     u16 sink_pad, u32 flags) { }
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +#endif /* _MEDIA_DEV_SHARED_GRAPH_H */

I've checked the request media ops, and tried to project how request could be
placed back into this. The main use of the ops is to extend request structure.
And any kind of scheduling will require that. Though, a scheduler based on
request inside a shared media graph should probably be media graph specific.

All in all, it makes me thing that we should plan for something less
generic/storage like, and perhaps allow for for ops by a non-device driver, that
could then further customize scheduling and implement request ?

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]