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

[Michael Riesch <michael.riesch@collabora.com>]

Hi Paul,

Thanks for your work! And sorry for the long silence.

On 6/19/26 07:26, Paul Elder wrote:
> 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.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Frenetically-cheered-by: Michael Riesch <michael.riesch@collabora.com>

OK, what follows is pretty much a rubberducking session in which I try
to explain myself the concept you came up with. Possibly there are one
or two thoughts and/or questions you can use to develop the concept further.

IIUC this is a new API with four calls. There is _join() and _leave()
that more or less replace the creation and deletion of a media device in
a V4L2 driver. And there is join_link_source() and join_link_sink() that
are called for each cross-device connection on the source and sink side,
respectively.

There is one shared media device per disjoint graph in the device tree.
If a new device calls _join(), the function iterates over all shared
media devices, iterates over all DT endpoints of the new device, and
iterates over all members of each shared media device. If the fwnode of
the parent of the remote endpoint of a certain endpoint of the new
device equals the fwnode of a certain member of a certain shared media
device, we found a match. This certain shared media device is then used
in the driver of the new device. Before that, a member for the new
device is added to the shared media device.

In case no shared media device is found, a new one is created and used.
A member for the new device is added to the shared device in this case, too.

The actual linking takes place in the _join_link_{sink,source}
functions, which iterate over the links of the shared media device and
check whether a link has a pointer to a source (or sink) respectively.
If a source (or sink) is found, a link is created. Otherwise, a link (in
the form of a newly introduced data structure, not a media_link) is
added to the links of a shared media device. The latter somewhat
corresponds to the v4l2 async notifier mechanism.

So far, so good. Even if the genitive construction chain that describes
the matching above is a bit daunting. All in all I think we are on a
reasonable path here.

> [...]
> +// 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;
> +};

[discussed in other thread]

> [...]
> +/* 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;
> +}

<rubberducking>
IIUC there could be devices A -> B -> C that share a DT graph (and thus
should share a media device), but suppose the probe order is driver A,
C, B then A would rightfully create a new shared media device, but C
would not see it and create yet another shared media device. Thus, the
the find_match() function needs to traverse C -> B -> A to find the
correct shared media device.
</rubberducking>

Just saying that with one and only one media device to rule them all we
wouldn't have to deal with this. Another option would be to assume that
each driver knows to which shared media device it belongs. For example,
the rkcif driver could be aware that it belongs to the
    {
        .id = MEDIA_SHARED_ROCKCHIP_CAMERA,
        .name = "Rockchip Camera Subsystem",
     }
shared media device. This would reduce the magic above to a lookup
operation. All that said, it should be feasible of course to traverse
and find.

> [...]
> +/* 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);

This can be refactored as this is carried out either way, right?

> +
> +	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;
> +}

OK, what options do we have?!

We could pick one device of many. But based on what criteria? If there
was a good way to pick one, we could equally pick a driver that
registers the media device and move on. We conclude that apparently
that's not the case, otherwise we wouldn't be doing this exercise. Next.

We could introduce a pseudo device that provides the umbrella. But
again, that would be close to creating an umbrella driver that registers
the media device. And at some point we decided not to do that. Next.

We move from the notion of a media **device** to a media **graph**
(which is in alignment with the subject of this mail). The media graph
is something owned by the subsystem, not by any device/driver. The media
entities therein, however, could be owned by devices/drivers. Maybe
struct media_entity should feature a struct device *dev; member.

I could imagine that the dev member is used mostly for
dev_{info,err,...}, we would need to get rid of them. If there is
actually something with devm_ then this requires more thought.

IMHO the third option is the way to go.

> +
> +// 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) {
> +		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);

Similarly, we have to bid farewell to the notion of a media **device**.
Can we agree on making
    struct device *dev;
    char driver_name[32];
    char serial[40];
    char bus_info[32];
    u32 hw_revision;
optional for shared media devices (or, better, graphs)?

I could imagine, though, that we provide some minimally meaningful info
in a struct
    {
        .id = MEDIA_SHARED_ROCKCHIP_CAMERA,
        .name = "Rockchip Camera Subsystem",
        .driver_name = "rkcss",
        .hw_revision = 42,
        ...
     }
and apply that info here.

> [...]
> +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.

The phrase "whoever wins the race" is surely gold in the documentation
;-) I guess it should be same as with any _notifier_bound setup, and
IIUC it's typically the sink that passes some flags to
v4l2_create_fwnode_links_to_pad. So prioritizing the sink seems reasonable.

> +	 *
> +	 * 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.
> +	 */

In the end, shouldn't this follow whatever v4l2_async does? Not exactly
sure, but I think storing a struct media_entity * and a struct
fwnode_endpoint * for side A should do the trick. Side B would appear
later with the second struct fwnode_endpoint. The check would then be if
fwnode_graph_get_remote_endpoint(fwnode_A) == fwnode_B (+ maybe vice
versa). The link would then be filled with media_entity_{A,B} and
media_entity_{A,B}->ops.get_fwnode_pad(media_entity_{A,B},
fwnode_{A,B}). Obviously we need some selector that tells us whether
A=source and B=sink or vice versa for that.

> +	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

Should be possible to have some helper with (..., bool is_source) and
then two nice functions that are exposed to the public.
To start the bike shedding: Maybe
media_device_shared_register_{source,sink}?

> [...]

Thanks and best regards,
Michael