| Message ID | 20211213232849.40071-6-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce ancillary links | expand |
Hi Daniel, Thank you for the patch. On Mon, Dec 13, 2021 at 11:28:49PM +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> > --- > Changes since the rfc: > > - None > > drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 0404267f1ae4..6575b1cbe95f 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *sd) > +{ > + struct media_link *link; > + > + if (sd->entity.function != MEDIA_ENT_F_LENS && > + sd->entity.function != MEDIA_ENT_F_FLASH) > + return -EINVAL; > + > + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, Is there a guarantee at this point that notifier->sd->entity has already been registered with the media_device ? That's done by media_device_register_entity() called from v4l2_device_register_subdev(). > + MEDIA_LNK_FL_ENABLED | > + MEDIA_LNK_FL_IMMUTABLE); > + > + return IS_ERR(link) ? PTR_ERR(link) : 0; > +} > + > +/* > + * Setup links on behalf of the notifier and subdev, where it's obvious what s/Setup/Create/ ("link setup" refers to another operation, enabling and disabling links at runtime) > + * should be done. At the moment, we only support cases where the notifier > + * is a sensor and the subdev is a lens. s/sensor/camera sensor/ s/lens/lens controller/ > + * > + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE > + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR > + */ > +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *sd) > +{ > + if (!notifier->sd) > + return 0; > + > + switch (notifier->sd->entity.function) { > + case MEDIA_ENT_F_CAM_SENSOR: > + return __v4l2_async_create_ancillary_link(notifier, sd); > + default: > + return 0; > + } > +} > + > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > struct v4l2_device *v4l2_dev, > struct v4l2_subdev *sd, > @@ -293,6 +332,18 @@ 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) s/)/)./ > + */ > + ret = v4l2_async_try_create_links(notifier, sd); > + if (ret) { > + v4l2_device_unregister_subdev(sd); > + return ret; > + } > + > /* Remove from the waiting list */ > list_del(&asd->list); > sd->asd = asd;
Hi Laurent On 14/12/2021 22:22, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Dec 13, 2021 at 11:28:49PM +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> >> --- >> Changes since the rfc: >> >> - None >> >> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >> index 0404267f1ae4..6575b1cbe95f 100644 >> --- a/drivers/media/v4l2-core/v4l2-async.c >> +++ b/drivers/media/v4l2-core/v4l2-async.c >> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *sd) >> +{ >> + struct media_link *link; >> + >> + if (sd->entity.function != MEDIA_ENT_F_LENS && >> + sd->entity.function != MEDIA_ENT_F_FLASH) >> + return -EINVAL; >> + >> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > > Is there a guarantee at this point that notifier->sd->entity has already > been registered with the media_device ? That's done by > media_device_register_entity() called from > v4l2_device_register_subdev(). v4l2_async_match_notify() calls v4l2_device_register_subdev() before the point that I've added the call to v4l2_async_try_create_links(), so I think that's covered there. >> + MEDIA_LNK_FL_ENABLED | >> + MEDIA_LNK_FL_IMMUTABLE); >> + >> + return IS_ERR(link) ? PTR_ERR(link) : 0; >> +} >> + >> +/* >> + * Setup links on behalf of the notifier and subdev, where it's obvious what > > s/Setup/Create/ ("link setup" refers to another operation, enabling and > disabling links at runtime) Yes, good point; although that too is a piece of terminology I find a bit jarring to be honest; I would have named that function media_entity_configure_link() > >> + * should be done. At the moment, we only support cases where the notifier >> + * is a sensor and the subdev is a lens. > > s/sensor/camera sensor/ > s/lens/lens controller/ > >> + * >> + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE >> + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR >> + */ >> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *sd) >> +{ >> + if (!notifier->sd) >> + return 0; >> + >> + switch (notifier->sd->entity.function) { >> + case MEDIA_ENT_F_CAM_SENSOR: >> + return __v4l2_async_create_ancillary_link(notifier, sd); >> + default: >> + return 0; >> + } >> +} >> + >> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, >> struct v4l2_device *v4l2_dev, >> struct v4l2_subdev *sd, >> @@ -293,6 +332,18 @@ 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) > > s/)/)./ > >> + */ >> + ret = v4l2_async_try_create_links(notifier, sd); >> + if (ret) { >> + v4l2_device_unregister_subdev(sd); >> + return ret; >> + } >> + >> /* Remove from the waiting list */ >> list_del(&asd->list); >> sd->asd = asd; >
Hi Daniel, On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: > On 14/12/2021 22:22, Laurent Pinchart wrote: > > On Mon, Dec 13, 2021 at 11:28:49PM +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> > >> --- > >> Changes since the rfc: > >> > >> - None > >> > >> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > >> 1 file changed, 51 insertions(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >> index 0404267f1ae4..6575b1cbe95f 100644 > >> --- a/drivers/media/v4l2-core/v4l2-async.c > >> +++ b/drivers/media/v4l2-core/v4l2-async.c > >> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *sd) > >> +{ > >> + struct media_link *link; > >> + > >> + if (sd->entity.function != MEDIA_ENT_F_LENS && > >> + sd->entity.function != MEDIA_ENT_F_FLASH) > >> + return -EINVAL; > >> + > >> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > > > > Is there a guarantee at this point that notifier->sd->entity has already > > been registered with the media_device ? That's done by > > media_device_register_entity() called from > > v4l2_device_register_subdev(). > > v4l2_async_match_notify() calls v4l2_device_register_subdev() before the > point that I've added the call to v4l2_async_try_create_links(), so I > think that's covered there. It calls it on sd, not notifier->sd. It's the latter that concerns me. > >> + MEDIA_LNK_FL_ENABLED | > >> + MEDIA_LNK_FL_IMMUTABLE); > >> + > >> + return IS_ERR(link) ? PTR_ERR(link) : 0; > >> +} > >> + > >> +/* > >> + * Setup links on behalf of the notifier and subdev, where it's obvious what > > > > s/Setup/Create/ ("link setup" refers to another operation, enabling and > > disabling links at runtime) > > Yes, good point; although that too is a piece of terminology I find a > bit jarring to be honest; I would have named that function > media_entity_configure_link() > > >> + * should be done. At the moment, we only support cases where the notifier > >> + * is a sensor and the subdev is a lens. > > > > s/sensor/camera sensor/ > > s/lens/lens controller/ > > > >> + * > >> + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE > >> + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR > >> + */ > >> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *sd) > >> +{ > >> + if (!notifier->sd) > >> + return 0; > >> + > >> + switch (notifier->sd->entity.function) { > >> + case MEDIA_ENT_F_CAM_SENSOR: > >> + return __v4l2_async_create_ancillary_link(notifier, sd); > >> + default: > >> + return 0; > >> + } > >> +} > >> + > >> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > >> struct v4l2_device *v4l2_dev, > >> struct v4l2_subdev *sd, > >> @@ -293,6 +332,18 @@ 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) > > > > s/)/)./ > > > >> + */ > >> + ret = v4l2_async_try_create_links(notifier, sd); > >> + if (ret) { > >> + v4l2_device_unregister_subdev(sd); > >> + return ret; > >> + } > >> + > >> /* Remove from the waiting list */ > >> list_del(&asd->list); > >> sd->asd = asd;
Hi Laurent On 14/12/2021 23:01, Laurent Pinchart wrote: > Hi Daniel, > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: >> On 14/12/2021 22:22, Laurent Pinchart wrote: >>> On Mon, Dec 13, 2021 at 11:28:49PM +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> >>>> --- >>>> Changes since the rfc: >>>> >>>> - None >>>> >>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ >>>> 1 file changed, 51 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>>> index 0404267f1ae4..6575b1cbe95f 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-async.c >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, >>>> + struct v4l2_subdev *sd) >>>> +{ >>>> + struct media_link *link; >>>> + >>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && >>>> + sd->entity.function != MEDIA_ENT_F_FLASH) >>>> + return -EINVAL; >>>> + >>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, >>> Is there a guarantee at this point that notifier->sd->entity has already >>> been registered with the media_device ? That's done by >>> media_device_register_entity() called from >>> v4l2_device_register_subdev(). >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the >> point that I've added the call to v4l2_async_try_create_links(), so I >> think that's covered there. > It calls it on sd, not notifier->sd. It's the latter that concerns me. Ah, you're right of course...I guess in that case the notifier->sd would get registered during the v4l2_async_match_notify() where the sensor driver's subdev is sd, but I don't think there's any guarantee that that would happen first...I haven't traced it through but my guess is that it would depend on the order in which the ipu3-cio2, sensor and lens controller drivers probed. I'll check to try and be sure how it works tomorrow > >>>> + MEDIA_LNK_FL_ENABLED | >>>> + MEDIA_LNK_FL_IMMUTABLE); >>>> + >>>> + return IS_ERR(link) ? PTR_ERR(link) : 0; >>>> +} >>>> + >>>> +/* >>>> + * Setup links on behalf of the notifier and subdev, where it's obvious what >>> s/Setup/Create/ ("link setup" refers to another operation, enabling and >>> disabling links at runtime) >> Yes, good point; although that too is a piece of terminology I find a >> bit jarring to be honest; I would have named that function >> media_entity_configure_link() >> >>>> + * should be done. At the moment, we only support cases where the notifier >>>> + * is a sensor and the subdev is a lens. >>> s/sensor/camera sensor/ >>> s/lens/lens controller/ >>> >>>> + * >>>> + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE >>>> + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR >>>> + */ >>>> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, >>>> + struct v4l2_subdev *sd) >>>> +{ >>>> + if (!notifier->sd) >>>> + return 0; >>>> + >>>> + switch (notifier->sd->entity.function) { >>>> + case MEDIA_ENT_F_CAM_SENSOR: >>>> + return __v4l2_async_create_ancillary_link(notifier, sd); >>>> + default: >>>> + return 0; >>>> + } >>>> +} >>>> + >>>> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, >>>> struct v4l2_device *v4l2_dev, >>>> struct v4l2_subdev *sd, >>>> @@ -293,6 +332,18 @@ 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) >>> s/)/)./ >>> >>>> + */ >>>> + ret = v4l2_async_try_create_links(notifier, sd); >>>> + if (ret) { >>>> + v4l2_device_unregister_subdev(sd); >>>> + return ret; >>>> + } >>>> + >>>> /* Remove from the waiting list */ >>>> list_del(&asd->list); >>>> sd->asd = asd;
Hi Dan, On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: > On 14/12/2021 23:01, Laurent Pinchart wrote: > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: > >> On 14/12/2021 22:22, Laurent Pinchart wrote: > >>> On Mon, Dec 13, 2021 at 11:28:49PM +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> > >>>> --- > >>>> Changes since the rfc: > >>>> > >>>> - None > >>>> > >>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > >>>> 1 file changed, 51 insertions(+) > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >>>> index 0404267f1ae4..6575b1cbe95f 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > >>>> + struct v4l2_subdev *sd) > >>>> +{ > >>>> + struct media_link *link; > >>>> + > >>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && > >>>> + sd->entity.function != MEDIA_ENT_F_FLASH) > >>>> + return -EINVAL; > >>>> + > >>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > >>> > >>> Is there a guarantee at this point that notifier->sd->entity has already > >>> been registered with the media_device ? That's done by > >>> media_device_register_entity() called from > >>> v4l2_device_register_subdev(). > >> > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the > >> point that I've added the call to v4l2_async_try_create_links(), so I > >> think that's covered there. > > > > It calls it on sd, not notifier->sd. It's the latter that concerns me. > > Ah, you're right of course...I guess in that case the notifier->sd would > get registered during the v4l2_async_match_notify() where the sensor > driver's subdev is sd, but I don't think there's any guarantee that that > would happen first...I haven't traced it through but my guess is that it > would depend on the order in which the ipu3-cio2, sensor and lens > controller drivers probed. I'll check to try and be sure how it works > tomorrow I was looking at media_device_register_entity(), and it sets INIT_LIST_HEAD(&entity->links); entity->num_links = 0; entity->num_backlinks = 0; If we create links before that, things may go bad. > >>>> + MEDIA_LNK_FL_ENABLED | > >>>> + MEDIA_LNK_FL_IMMUTABLE); > >>>> + > >>>> + return IS_ERR(link) ? PTR_ERR(link) : 0; > >>>> +} > >>>> + > >>>> +/* > >>>> + * Setup links on behalf of the notifier and subdev, where it's obvious what > >>> > >>> s/Setup/Create/ ("link setup" refers to another operation, enabling and > >>> disabling links at runtime) > >> > >> Yes, good point; although that too is a piece of terminology I find a > >> bit jarring to be honest; I would have named that function > >> media_entity_configure_link() > >> > >>>> + * should be done. At the moment, we only support cases where the notifier > >>>> + * is a sensor and the subdev is a lens. > >>> > >>> s/sensor/camera sensor/ > >>> s/lens/lens controller/ > >>> > >>>> + * > >>>> + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE > >>>> + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR > >>>> + */ > >>>> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, > >>>> + struct v4l2_subdev *sd) > >>>> +{ > >>>> + if (!notifier->sd) > >>>> + return 0; > >>>> + > >>>> + switch (notifier->sd->entity.function) { > >>>> + case MEDIA_ENT_F_CAM_SENSOR: > >>>> + return __v4l2_async_create_ancillary_link(notifier, sd); > >>>> + default: > >>>> + return 0; > >>>> + } > >>>> +} > >>>> + > >>>> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > >>>> struct v4l2_device *v4l2_dev, > >>>> struct v4l2_subdev *sd, > >>>> @@ -293,6 +332,18 @@ 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) > >>> > >>> s/)/)./ > >>> > >>>> + */ > >>>> + ret = v4l2_async_try_create_links(notifier, sd); > >>>> + if (ret) { > >>>> + v4l2_device_unregister_subdev(sd); > >>>> + return ret; > >>>> + } > >>>> + > >>>> /* Remove from the waiting list */ > >>>> list_del(&asd->list); > >>>> sd->asd = asd;
On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote: > Hi Dan, > > On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: > > On 14/12/2021 23:01, Laurent Pinchart wrote: > > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: > > >> On 14/12/2021 22:22, Laurent Pinchart wrote: > > >>> On Mon, Dec 13, 2021 at 11:28:49PM +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> > > >>>> --- > > >>>> Changes since the rfc: > > >>>> > > >>>> - None > > >>>> > > >>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > > >>>> 1 file changed, 51 insertions(+) > > >>>> > > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > >>>> index 0404267f1ae4..6575b1cbe95f 100644 > > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c > > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > > >>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > > >>>> + struct v4l2_subdev *sd) > > >>>> +{ > > >>>> + struct media_link *link; > > >>>> + > > >>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && > > >>>> + sd->entity.function != MEDIA_ENT_F_FLASH) > > >>>> + return -EINVAL; > > >>>> + > > >>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > > >>> > > >>> Is there a guarantee at this point that notifier->sd->entity has already > > >>> been registered with the media_device ? That's done by > > >>> media_device_register_entity() called from > > >>> v4l2_device_register_subdev(). > > >> > > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the > > >> point that I've added the call to v4l2_async_try_create_links(), so I > > >> think that's covered there. > > > > > > It calls it on sd, not notifier->sd. It's the latter that concerns me. > > > > Ah, you're right of course...I guess in that case the notifier->sd would > > get registered during the v4l2_async_match_notify() where the sensor > > driver's subdev is sd, but I don't think there's any guarantee that that > > would happen first...I haven't traced it through but my guess is that it > > would depend on the order in which the ipu3-cio2, sensor and lens > > controller drivers probed. I'll check to try and be sure how it works > > tomorrow > > I was looking at media_device_register_entity(), and it sets > > INIT_LIST_HEAD(&entity->links); > entity->num_links = 0; > entity->num_backlinks = 0; > > If we create links before that, things may go bad. Yes. There's a guarantee that the notifier's complete callback is called once the notifier's subdevs as well as sub-notifiers are bound and complete. But there's no guarantee on the initialisation of related entities. Especially for sensors, the async subdev is registered after the sensor's own async notifier. I wonder if the ugly registered callback could be used for this purpose. Better of course would be to avoid that.
Hi Sakari, On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote: > On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote: > > On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: > > > On 14/12/2021 23:01, Laurent Pinchart wrote: > > > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: > > > >> On 14/12/2021 22:22, Laurent Pinchart wrote: > > > >>> On Mon, Dec 13, 2021 at 11:28:49PM +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> > > > >>>> --- > > > >>>> Changes since the rfc: > > > >>>> > > > >>>> - None > > > >>>> > > > >>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > > > >>>> 1 file changed, 51 insertions(+) > > > >>>> > > > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > >>>> index 0404267f1ae4..6575b1cbe95f 100644 > > > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c > > > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > > > >>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > > > >>>> + struct v4l2_subdev *sd) > > > >>>> +{ > > > >>>> + struct media_link *link; > > > >>>> + > > > >>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && > > > >>>> + sd->entity.function != MEDIA_ENT_F_FLASH) > > > >>>> + return -EINVAL; > > > >>>> + > > > >>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > > > >>> > > > >>> Is there a guarantee at this point that notifier->sd->entity has already > > > >>> been registered with the media_device ? That's done by > > > >>> media_device_register_entity() called from > > > >>> v4l2_device_register_subdev(). > > > >> > > > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the > > > >> point that I've added the call to v4l2_async_try_create_links(), so I > > > >> think that's covered there. > > > > > > > > It calls it on sd, not notifier->sd. It's the latter that concerns me. > > > > > > Ah, you're right of course...I guess in that case the notifier->sd would > > > get registered during the v4l2_async_match_notify() where the sensor > > > driver's subdev is sd, but I don't think there's any guarantee that that > > > would happen first...I haven't traced it through but my guess is that it > > > would depend on the order in which the ipu3-cio2, sensor and lens > > > controller drivers probed. I'll check to try and be sure how it works > > > tomorrow > > > > I was looking at media_device_register_entity(), and it sets > > > > INIT_LIST_HEAD(&entity->links); > > entity->num_links = 0; > > entity->num_backlinks = 0; > > > > If we create links before that, things may go bad. > > Yes. > > There's a guarantee that the notifier's complete callback is called once > the notifier's subdevs as well as sub-notifiers are bound and complete. But > there's no guarantee on the initialisation of related entities. > > Especially for sensors, the async subdev is registered after the sensor's > own async notifier. > > I wonder if the ugly registered callback could be used for this purpose. > Better of course would be to avoid that. I'd really like all these links to be created automatically by the code, but given the very loosely defined rules regarding entity initialization, I'm worried this may not be possible without quite a bit of cleanup first :-( It looks like quite a bit of the work done in media_device_register_entity() could (and likely should) be moved to media_entity_init(), but I'm not sure if that would be enough to properly fix the issue.
Hi Laurent, Sakari On 15/12/2021 09:55, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote: >> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote: >>> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: >>>> On 14/12/2021 23:01, Laurent Pinchart wrote: >>>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: >>>>>> On 14/12/2021 22:22, Laurent Pinchart wrote: >>>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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> >>>>>>>> --- >>>>>>>> Changes since the rfc: >>>>>>>> >>>>>>>> - None >>>>>>>> >>>>>>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 51 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>>>>>>> index 0404267f1ae4..6575b1cbe95f 100644 >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>>>>>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, >>>>>>>> + struct v4l2_subdev *sd) >>>>>>>> +{ >>>>>>>> + struct media_link *link; >>>>>>>> + >>>>>>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && >>>>>>>> + sd->entity.function != MEDIA_ENT_F_FLASH) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, >>>>>>> >>>>>>> Is there a guarantee at this point that notifier->sd->entity has already >>>>>>> been registered with the media_device ? That's done by >>>>>>> media_device_register_entity() called from >>>>>>> v4l2_device_register_subdev(). >>>>>> >>>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the >>>>>> point that I've added the call to v4l2_async_try_create_links(), so I >>>>>> think that's covered there. >>>>> >>>>> It calls it on sd, not notifier->sd. It's the latter that concerns me. >>>> >>>> Ah, you're right of course...I guess in that case the notifier->sd would >>>> get registered during the v4l2_async_match_notify() where the sensor >>>> driver's subdev is sd, but I don't think there's any guarantee that that >>>> would happen first...I haven't traced it through but my guess is that it >>>> would depend on the order in which the ipu3-cio2, sensor and lens >>>> controller drivers probed. I'll check to try and be sure how it works >>>> tomorrow >>> >>> I was looking at media_device_register_entity(), and it sets >>> >>> INIT_LIST_HEAD(&entity->links); >>> entity->num_links = 0; >>> entity->num_backlinks = 0; >>> >>> If we create links before that, things may go bad. Yep, that definitely looks like it would make things go badly wrong. I'm building with a delayed ov8865 probe now, let's see what happens... >> >> Yes. >> >> There's a guarantee that the notifier's complete callback is called once >> the notifier's subdevs as well as sub-notifiers are bound and complete. But >> there's no guarantee on the initialisation of related entities. >> >> Especially for sensors, the async subdev is registered after the sensor's >> own async notifier. >> >> I wonder if the ugly registered callback could be used for this purpose. >> Better of course would be to avoid that. > > I'd really like all these links to be created automatically by the code, > but given the very loosely defined rules regarding entity > initialization, I'm worried this may not be possible without quite a bit > of cleanup first :-( Yeah. At present at least the primary entity would need to be linked to the media dev, as it's taking primary->graph_obj.mdev as the pointer to use in media_gobj_create() in media_create_ancillary_link(). That's a pretty big problem actually...but I'd really like to try and solve it as we could cut a lot of code out the other drivers if we do the same thing for the data links. One way around it might be to defer matching in v4l2_async_find_match() if the notifier's subdev hasn't picked up an mdev itself yet...which could guarantee the ordering but sort of breaks the asynchronicity of it all. I'm almost certainly missing some other reason that that's a terrible idea too. I'll try and explore some ways to do this that still keeps the link setup within core - thanks for pointing it out Laurent > It looks like quite a bit of the work done in > media_device_register_entity() could (and likely should) be moved to > media_entity_init(), but I'm not sure if that would be enough to > properly fix the issue. I guess you mean media_entity_pads_init()? Or media_device_init()? >
Hi Dan, On Wed, Dec 15, 2021 at 11:10:09PM +0000, Daniel Scally wrote: > On 15/12/2021 09:55, Laurent Pinchart wrote: > > On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote: > >> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote: > >>> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: > >>>> On 14/12/2021 23:01, Laurent Pinchart wrote: > >>>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: > >>>>>> On 14/12/2021 22:22, Laurent Pinchart wrote: > >>>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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> > >>>>>>>> --- > >>>>>>>> Changes since the rfc: > >>>>>>>> > >>>>>>>> - None > >>>>>>>> > >>>>>>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ > >>>>>>>> 1 file changed, 51 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > >>>>>>>> index 0404267f1ae4..6575b1cbe95f 100644 > >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>>>>>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, > >>>>>>>> + struct v4l2_subdev *sd) > >>>>>>>> +{ > >>>>>>>> + struct media_link *link; > >>>>>>>> + > >>>>>>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && > >>>>>>>> + sd->entity.function != MEDIA_ENT_F_FLASH) > >>>>>>>> + return -EINVAL; > >>>>>>>> + > >>>>>>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > >>>>>>> > >>>>>>> Is there a guarantee at this point that notifier->sd->entity has already > >>>>>>> been registered with the media_device ? That's done by > >>>>>>> media_device_register_entity() called from > >>>>>>> v4l2_device_register_subdev(). > >>>>>> > >>>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the > >>>>>> point that I've added the call to v4l2_async_try_create_links(), so I > >>>>>> think that's covered there. > >>>>> > >>>>> It calls it on sd, not notifier->sd. It's the latter that concerns me. > >>>> > >>>> Ah, you're right of course...I guess in that case the notifier->sd would > >>>> get registered during the v4l2_async_match_notify() where the sensor > >>>> driver's subdev is sd, but I don't think there's any guarantee that that > >>>> would happen first...I haven't traced it through but my guess is that it > >>>> would depend on the order in which the ipu3-cio2, sensor and lens > >>>> controller drivers probed. I'll check to try and be sure how it works > >>>> tomorrow > >>> > >>> I was looking at media_device_register_entity(), and it sets > >>> > >>> INIT_LIST_HEAD(&entity->links); > >>> entity->num_links = 0; > >>> entity->num_backlinks = 0; > >>> > >>> If we create links before that, things may go bad. > > Yep, that definitely looks like it would make things go badly wrong. I'm > building with a delayed ov8865 probe now, let's see what happens... > > >> Yes. > >> > >> There's a guarantee that the notifier's complete callback is called once > >> the notifier's subdevs as well as sub-notifiers are bound and complete. But > >> there's no guarantee on the initialisation of related entities. > >> > >> Especially for sensors, the async subdev is registered after the sensor's > >> own async notifier. > >> > >> I wonder if the ugly registered callback could be used for this purpose. > >> Better of course would be to avoid that. > > > > I'd really like all these links to be created automatically by the code, > > but given the very loosely defined rules regarding entity > > initialization, I'm worried this may not be possible without quite a bit > > of cleanup first :-( > > Yeah. At present at least the primary entity would need to be linked to > the media dev, as it's taking primary->graph_obj.mdev as the pointer to > use in media_gobj_create() in media_create_ancillary_link(). That's a > pretty big problem actually...but I'd really like to try and solve it as > we could cut a lot of code out the other drivers if we do the same thing > for the data links. > > One way around it might be to defer matching in v4l2_async_find_match() > if the notifier's subdev hasn't picked up an mdev itself yet...which > could guarantee the ordering but sort of breaks the asynchronicity of it > all. I'm almost certainly missing some other reason that that's a > terrible idea too. v4l2-async is a mess, that's the main reason why everybody is reluctant to touch it :-) On my long todo list is a task to rewrite it from scratch, with an API that wouldn't be V4L2-specific. > I'll try and explore some ways to do this that still keeps the link > setup within core - thanks for pointing it out Laurent > > > It looks like quite a bit of the work done in > > media_device_register_entity() could (and likely should) be moved to > > media_entity_init(), but I'm not sure if that would be enough to > > properly fix the issue. > > I guess you mean media_entity_pads_init()? Or media_device_init()? The former, sorry.
Hi Daniel,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.16-rc5 next-20211215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211214-073020
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-r015-20211216 (https://download.01.org/0day-ci/archive/20211216/202112161906.gHHRLukN-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project dd245bab9fbb364faa1581e4f92ba3119a872fba)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7e7fcd65e8144f3ffa337760c26fb786f4028466
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211214-073020
git checkout 7e7fcd65e8144f3ffa337760c26fb786f4028466
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/media/v4l2-core/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/media/v4l2-core/v4l2-async.c:284:10: error: no member named 'entity' in 'struct v4l2_subdev'
if (sd->entity.function != MEDIA_ENT_F_LENS &&
~~ ^
drivers/media/v4l2-core/v4l2-async.c:285:10: error: no member named 'entity' in 'struct v4l2_subdev'
sd->entity.function != MEDIA_ENT_F_FLASH)
~~ ^
drivers/media/v4l2-core/v4l2-async.c:288:52: error: no member named 'entity' in 'struct v4l2_subdev'
link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity,
~~~~~~~~~~~~ ^
drivers/media/v4l2-core/v4l2-async.c:288:65: error: no member named 'entity' in 'struct v4l2_subdev'
link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity,
~~ ^
drivers/media/v4l2-core/v4l2-async.c:309:24: error: no member named 'entity' in 'struct v4l2_subdev'
switch (notifier->sd->entity.function) {
~~~~~~~~~~~~ ^
5 errors generated.
vim +284 drivers/media/v4l2-core/v4l2-async.c
277
278 static int
279 __v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
280 struct v4l2_subdev *sd)
281 {
282 struct media_link *link;
283
> 284 if (sd->entity.function != MEDIA_ENT_F_LENS &&
285 sd->entity.function != MEDIA_ENT_F_FLASH)
286 return -EINVAL;
287
288 link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity,
289 MEDIA_LNK_FL_ENABLED |
290 MEDIA_LNK_FL_IMMUTABLE);
291
292 return IS_ERR(link) ? PTR_ERR(link) : 0;
293 }
294
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
I guess this needs to be a no-op if the media controller API isn't configured. On 16/12/2021 11:10, kernel test robot wrote: > Hi Daniel, > > I love your patch! Yet something to improve: > > [auto build test ERROR on media-tree/master] > [also build test ERROR on v5.16-rc5 next-20211215] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211214-073020 > base: git://linuxtv.org/media_tree.git master > config: x86_64-randconfig-r015-20211216 (https://download.01.org/0day-ci/archive/20211216/202112161906.gHHRLukN-lkp@intel.com/config) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project dd245bab9fbb364faa1581e4f92ba3119a872fba) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/7e7fcd65e8144f3ffa337760c26fb786f4028466 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211214-073020 > git checkout 7e7fcd65e8144f3ffa337760c26fb786f4028466 > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/media/v4l2-core/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > >>> drivers/media/v4l2-core/v4l2-async.c:284:10: error: no member named 'entity' in 'struct v4l2_subdev' > if (sd->entity.function != MEDIA_ENT_F_LENS && > ~~ ^ > drivers/media/v4l2-core/v4l2-async.c:285:10: error: no member named 'entity' in 'struct v4l2_subdev' > sd->entity.function != MEDIA_ENT_F_FLASH) > ~~ ^ > drivers/media/v4l2-core/v4l2-async.c:288:52: error: no member named 'entity' in 'struct v4l2_subdev' > link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > ~~~~~~~~~~~~ ^ > drivers/media/v4l2-core/v4l2-async.c:288:65: error: no member named 'entity' in 'struct v4l2_subdev' > link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > ~~ ^ > drivers/media/v4l2-core/v4l2-async.c:309:24: error: no member named 'entity' in 'struct v4l2_subdev' > switch (notifier->sd->entity.function) { > ~~~~~~~~~~~~ ^ > 5 errors generated. > > > vim +284 drivers/media/v4l2-core/v4l2-async.c > > 277 > 278 static int > 279 __v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier, > 280 struct v4l2_subdev *sd) > 281 { > 282 struct media_link *link; > 283 > > 284 if (sd->entity.function != MEDIA_ENT_F_LENS && > 285 sd->entity.function != MEDIA_ENT_F_FLASH) > 286 return -EINVAL; > 287 > 288 link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, > 289 MEDIA_LNK_FL_ENABLED | > 290 MEDIA_LNK_FL_IMMUTABLE); > 291 > 292 return IS_ERR(link) ? PTR_ERR(link) : 0; > 293 } > 294 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Laurent On 15/12/2021 09:04, Laurent Pinchart wrote: A month to the day! Sorry about the delay - more on that below... > Hi Dan, > > On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote: >> On 14/12/2021 23:01, Laurent Pinchart wrote: >>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote: >>>> On 14/12/2021 22:22, Laurent Pinchart wrote: >>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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> >>>>>> --- >>>>>> Changes since the rfc: >>>>>> >>>>>> - None >>>>>> >>>>>> drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ >>>>>> 1 file changed, 51 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>>>>> index 0404267f1ae4..6575b1cbe95f 100644 >>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c >>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>>>>> @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, >>>>>> + struct v4l2_subdev *sd) >>>>>> +{ >>>>>> + struct media_link *link; >>>>>> + >>>>>> + if (sd->entity.function != MEDIA_ENT_F_LENS && >>>>>> + sd->entity.function != MEDIA_ENT_F_FLASH) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, >>>>> Is there a guarantee at this point that notifier->sd->entity has already >>>>> been registered with the media_device ? That's done by >>>>> media_device_register_entity() called from >>>>> v4l2_device_register_subdev(). >>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the >>>> point that I've added the call to v4l2_async_try_create_links(), so I >>>> think that's covered there. >>> It calls it on sd, not notifier->sd. It's the latter that concerns me. >> Ah, you're right of course...I guess in that case the notifier->sd would >> get registered during the v4l2_async_match_notify() where the sensor >> driver's subdev is sd, but I don't think there's any guarantee that that >> would happen first...I haven't traced it through but my guess is that it >> would depend on the order in which the ipu3-cio2, sensor and lens >> controller drivers probed. I'll check to try and be sure how it works >> tomorrow > I was looking at media_device_register_entity(), and it sets > > INIT_LIST_HEAD(&entity->links); > entity->num_links = 0; > entity->num_backlinks = 0; > > If we create links before that, things may go bad. It looks like there _is_ a guarantee of ordering actually. When the ov8865's notifier is registered in v4l2_async_register_subdev_sensor(), v4l2_async_nf_try_all_subdevs() is called against it, but at that point v4l2_async_nf_find_v4l2_dev() won't find anything for the ov8865's notifier even if the dw9719 has already probed and has it's async_subdev waiting because the notifier has no parent and no directly assigned v4l2_dev, so the function exits before trying to match anything (this same logic guards all calls to v4l2_async_find_match()). Very shortly after that v4l2_async_register_subdev() is called for the ov8865's subdev which will match to ipu3-cio2's notifier. In v4l2_async_match_notify() for that match the ipu3-cio2's notifier is assigned as parent to the ov8865's notifier, but _after_ v4l2_device_register_subdev() is called for the ov8865. From that point on v4l2_async_nf_find_v4l2_dev() will return a pointer and the matching for the dw9719 will work correctly. So unless I've missed something, I think it's ok. This took me a long time to figure out, because I reset libcamera to master for some reason and then totally forgot that I had done that...which meant the auto-focus wasn't working when I tested it and I convinced myself that my deliberate screwing of the probe ordering _did_ break it. After tearing my hair out for an embarrassing amount of time I eventually figured out what I had done and got to the bottom of it - sorry for the delay! > >>>>>> + MEDIA_LNK_FL_ENABLED | >>>>>> + MEDIA_LNK_FL_IMMUTABLE); >>>>>> + >>>>>> + return IS_ERR(link) ? PTR_ERR(link) : 0; >>>>>> +} >>>>>> + >>>>>> +/* >>>>>> + * Setup links on behalf of the notifier and subdev, where it's obvious what >>>>> s/Setup/Create/ ("link setup" refers to another operation, enabling and >>>>> disabling links at runtime) >>>> Yes, good point; although that too is a piece of terminology I find a >>>> bit jarring to be honest; I would have named that function >>>> media_entity_configure_link() >>>> >>>>>> + * should be done. At the moment, we only support cases where the notifier >>>>>> + * is a sensor and the subdev is a lens. >>>>> s/sensor/camera sensor/ >>>>> s/lens/lens controller/ >>>>> >>>>>> + * >>>>>> + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE >>>>>> + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR >>>>>> + */ >>>>>> +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, >>>>>> + struct v4l2_subdev *sd) >>>>>> +{ >>>>>> + if (!notifier->sd) >>>>>> + return 0; >>>>>> + >>>>>> + switch (notifier->sd->entity.function) { >>>>>> + case MEDIA_ENT_F_CAM_SENSOR: >>>>>> + return __v4l2_async_create_ancillary_link(notifier, sd); >>>>>> + default: >>>>>> + return 0; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, >>>>>> struct v4l2_device *v4l2_dev, >>>>>> struct v4l2_subdev *sd, >>>>>> @@ -293,6 +332,18 @@ 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) >>>>> s/)/)./ >>>>> >>>>>> + */ >>>>>> + ret = v4l2_async_try_create_links(notifier, sd); >>>>>> + if (ret) { >>>>>> + v4l2_device_unregister_subdev(sd); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> /* Remove from the waiting list */ >>>>>> list_del(&asd->list); >>>>>> sd->asd = asd;
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 0404267f1ae4..6575b1cbe95f 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -275,6 +275,45 @@ 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_link(struct v4l2_async_notifier *notifier, + struct v4l2_subdev *sd) +{ + struct media_link *link; + + if (sd->entity.function != MEDIA_ENT_F_LENS && + sd->entity.function != MEDIA_ENT_F_FLASH) + return -EINVAL; + + link = media_create_ancillary_link(¬ifier->sd->entity, &sd->entity, + MEDIA_LNK_FL_ENABLED | + MEDIA_LNK_FL_IMMUTABLE); + + return IS_ERR(link) ? PTR_ERR(link) : 0; +} + +/* + * Setup links on behalf of the notifier and subdev, where it's obvious what + * should be done. At the moment, we only support cases where the notifier + * is a sensor and the subdev is a lens. + * + * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE + * and the subdev's is MEDIA_ENT_F_CAM_SENSOR + */ +static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier, + struct v4l2_subdev *sd) +{ + if (!notifier->sd) + return 0; + + switch (notifier->sd->entity.function) { + case MEDIA_ENT_F_CAM_SENSOR: + return __v4l2_async_create_ancillary_link(notifier, sd); + default: + return 0; + } +} + static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, struct v4l2_device *v4l2_dev, struct v4l2_subdev *sd, @@ -293,6 +332,18 @@ 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_try_create_links(notifier, sd); + if (ret) { + v4l2_device_unregister_subdev(sd); + return ret; + } + /* Remove from the waiting list */ list_del(&asd->list); sd->asd = asd;
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> --- Changes since the rfc: - None drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)