| Message ID | 20220302220304.1327896-1-djrscally@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce ancillary links | expand |
Hi ! Thanks for the patch ! On 02/03/2022 23:03, Daniel Scally wrote: > When iterating over the media graph, don't follow links that are not > data links. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > > Changes since v2: > > - None > > Changes since v1: > > - Moved to the head of the series > - s/pad-to-pad/data (Sakari) > - Dropped the debug message (Laurent) > > Changes since the rfc: > > - new patch > > drivers/media/mc/mc-entity.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index b411f9796191..d0563ee4b28b 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -295,6 +295,12 @@ static void media_graph_walk_iter(struct media_graph *graph) > > link = list_entry(link_top(graph), typeof(*link), list); > > + /* If the link is not a data link, don't follow it */ > + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) { > + link_top(graph) = link_top(graph)->next; > + return; > + } > + > /* The link is not enabled so we do not follow. */ > if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { > link_top(graph) = link_top(graph)->next;
Hi ! Thanks for the patch ! On 02/03/2022 23:03, Daniel Scally wrote: > Now we have three types of media link, printing the right name during > debug output is slightly more complicated. Add a helper function to > make it easier. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > > Changes since v2: > > - None > > Changes since v1: > > - renamed function to link_type_name() (Laurent) > > Changes since the rfc: > > - new patch > > drivers/media/mc/mc-entity.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index d0563ee4b28b..1a7d0a4fb9e8 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -44,6 +44,20 @@ static inline const char *intf_type(struct media_interface *intf) > } > }; > > +static inline const char *link_type_name(struct media_link *link) > +{ > + switch (link->flags & MEDIA_LNK_FL_LINK_TYPE) { > + case MEDIA_LNK_FL_DATA_LINK: > + return "data"; > + case MEDIA_LNK_FL_INTERFACE_LINK: > + return "interface"; > + case MEDIA_LNK_FL_ANCILLARY_LINK: > + return "ancillary"; > + default: > + return "unknown"; > + } > +} > + > __must_check int __media_entity_enum_init(struct media_entity_enum *ent_enum, > int idx_max) > { > @@ -89,9 +103,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj) > > dev_dbg(gobj->mdev->dev, > "%s id %u: %s link id %u ==> id %u\n", > - event_name, media_id(gobj), > - media_type(link->gobj0) == MEDIA_GRAPH_PAD ? > - "data" : "interface", > + event_name, media_id(gobj), link_type_name(link), > media_id(link->gobj0), > media_id(link->gobj1)); > break;
Hi Dan, Thank you for the patch. On Wed, Mar 02, 2022 at 10:03:04PM +0000, Daniel Scally wrote: > Upon an async fwnode match, there's some typical behaviour that the > notifier and matching subdev will want to do. For example, a notifier > representing a sensor matching to an async subdev representing its > VCM will want to create an ancillary link to expose that relationship > to userspace. > > To avoid lots of code in individual drivers, try to build these links > within v4l2 core. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > Changes since v2: > > - Stopped checking the notifier entity's function when creating the new > links, and just create them whenever the subdev entity's function is either > a lens controller or a flash. (Sakari) > > Changes since v1: > > - Added #ifdef guards for CONFIG_MEDIA_CONTROLLER > - Some spelling and nomenclature cleanup (Laurent) > > Changes since the rfc: > > - None > > drivers/media/v4l2-core/v4l2-async.c | 31 ++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 0404267f1ae4..436bd6900fd8 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -275,6 +275,24 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier) > static int > v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier); > > +static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n, > + struct v4l2_subdev *sd) > +{ > + struct media_link *link = NULL; > + > +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER) > + > + if (sd->entity.function != MEDIA_ENT_F_LENS && > + sd->entity.function != MEDIA_ENT_F_FLASH) > + return 0; > + > + link = media_create_ancillary_link(&n->sd->entity, &sd->entity); > + > +#endif > + > + return IS_ERR(link) ? PTR_ERR(link) : 0; > +} > + > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > struct v4l2_device *v4l2_dev, > struct v4l2_subdev *sd, > @@ -293,6 +311,19 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > return ret; > } > > + /* > + * Depending of the function of the entities involved, we may want to > + * create links between them (for example between a sensor and its lens > + * or between a sensor's source pad and the connected device's sink > + * pad). > + */ > + ret = v4l2_async_create_ancillary_links(notifier, sd); > + if (ret) { > + v4l2_async_nf_call_unbind(notifier, sd, asd); > + v4l2_device_unregister_subdev(sd); > + return ret; > + } > + > /* Remove from the waiting list */ > list_del(&asd->list); > sd->asd = asd;
Hello everyone Any more comments on this series? Thanks Dan On 02/03/2022 22:02, Daniel Scally wrote: > Hello all > > At present there's no means in the kernel of describing the supporting > relationship between subdevices that work together to form an effective single > unit - the type example in this case being a camera sensor and its > corresponding vcm. To attempt to solve that, this series adds a new type of > media link called MEDIA_LNK_FL_ANCILLARY_LINK, which connects two instances of > struct media_entity. > > The mechanism of connection I have modelled as a notifier and async subdev, > which seemed the best route since sensor drivers already typically will call > v4l2_async_register_subdev_sensor() on probe, and that function already looks > for a reference to a firmware node with the reference named "lens-focus". To > avoid boilerplate in the sensor drivers, I added some new functions in > v4l2-async that are called in v4l2_async_match_notify() to create the ancillary > links. I haven't gone further than that yet, but I suspect we could cut down on > code elsewhere by, for example, also creating pad-to-pad links in the same place > > Series level changes since v2: > > - Squashed #2 and #3 > > Series-level changes since v1: > > - New patch adding some documentation to the uAPI pages. > > Dan > > > Daniel Scally (5): > media: entity: Skip non-data links in graph iteration > media: media.h: Add new media link type > media: entity: Add link_type_name() helper > media: entity: Add support for ancillary links > media: v4l2-async: Create links during v4l2_async_match_notify() > > .../media/mediactl/media-controller-model.rst | 6 +++ > .../media/mediactl/media-types.rst | 17 +++++-- > drivers/media/mc/mc-entity.c | 46 +++++++++++++++++-- > drivers/media/v4l2-core/v4l2-async.c | 31 +++++++++++++ > include/media/media-entity.h | 19 ++++++++ > include/uapi/linux/media.h | 1 + > 6 files changed, 112 insertions(+), 8 deletions(-) >