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

[Paul Elder]

Hi Michael,

Thanks for the review!

Quoting Michael Riesch (2026-07-11 05:23:50)
> 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@xxxxxxxxxxxxxxxx>
> 
> Frenetically-cheered-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxx>
> 
> 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.

\o/

Yes the overview is right.

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

Yeah I guess traversing the graph is kind of a critical feature.

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

Hm yeah that's true; we'd just have a sea of media entities and we can link
whoever we want...

We didn't really discuss that part very hard last time.

> 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",
>      }

The problem is where would this information go? We can't put it in the driver;
it *must* come from the device tree. And I don't think we can just add a new
property that defines a media subsystem. So I think our only option is to
traverse the endpoint graph in the device tree.

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

Yes.

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

(Just in general or with respect to this function...?)

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

Yes; given that both rkcif and rkisp2 could potentially exist on devices where
the other does not, this is not realistically doable. (I mean if we really want
to we could but it would be a high-cost mess)

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

Similar problem as above (can't choose main driver), the one above that (no
place to put group member list).

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

I always thought that media device and media graph were the
same/interchangable...

> 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

afaict that's the case.

> actually something with devm_ then this requires more thought.
> 
> IMHO the third option is the way to go.

I'm glad we agree :)

> 
> > +
> > +// 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)?

Well I think driver_name and (maybe) hw_revision are still useful to have. The
only problem is I have no clue where to put/get that information.

The only good-ish idea I could come up with was something like
"shared_media_graph_{board compatible}_{smallest base address of media graph
member}". So like "shared_media_graph_rockchip,rk3588_fdcb0000" for the rk3588
since between the csi receivers and vicap and isp, the isp has the smallest
address. That way:
- the information comes from the device tree
- the name is persistent
- it can identify separate shared media graphs on the same platform

idk about the hardware revision though. I think probably only driver match data
can hold that information. But then we'd probably have duplicate information
since otherwise if one of the member drivers doesn't probe then we'd be missing
the hw_revision. Or maybe we don't care about hardware revision since it
doesn't make sense for the shared media graph part and it only makes sense for
the individual IP cores. In which case that information is lost unless we move
it somewhere else. Or maybe it's not used anyway so we can indeed just drop it.

> 
> 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,
>         ...
>      }

Same problem as above: there's no reasonable place to put this information.

> 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

Hehe :)

> IIUC it's typically the sink that passes some flags to
> v4l2_create_fwnode_links_to_pad. So prioritizing the sink seems reasonable.

Ok good.

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

Oh, good idea!

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

Yeah that sounds like a good idea.


Thanks,

Paul

> 
> > [...]
> 
> Thanks and best regards,
> Michael