| Message ID | 20220606225149.2941160-1-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rkisp1: Don't create data links for non-sensor subdevs | expand |
Hi Dan On Mon, Jun 06, 2022 at 11:51:49PM +0100, Daniel Scally wrote: > With the introduction of ancillary links, not all subdevs linked to > the ISP's v4l2_dev necessarily represent sensors / bridges. Check the > function for the subdevs and skip any that represent lens or flash > controllers before creating data links. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > > This should fix the issues that have been noticed, but perhaps a new flag like > MEDIA_ENT_FL_HAS_SOURCE or something would be a better way to denote subdevs > that need data links? > I agree this a bit fragile... I noticed ancillary links are only created for subdev notifiers, which have a populated 'sd' and consequentially an entity. Could an helper that walks the links of the notifier's subdev links and checks if the subdev at hand is already linked, help ? Maybe with an optional set of link flags to match on ? > drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > index 3f5cfa7eb937..e90f0216cb06 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > @@ -134,6 +134,10 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1) > sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) > continue; > > + if (sd->entity.function == MEDIA_ENT_F_LENS || > + sd->entity.function == MEDIA_ENT_F_FLASH) > + continue; > + > ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, > MEDIA_PAD_FL_SOURCE); > if (ret < 0) { > -- > 2.25.1 >
Hi Jacopo On 07/06/2022 17:41, Jacopo Mondi wrote: > Hi Dan > > On Mon, Jun 06, 2022 at 11:51:49PM +0100, Daniel Scally wrote: >> With the introduction of ancillary links, not all subdevs linked to >> the ISP's v4l2_dev necessarily represent sensors / bridges. Check the >> function for the subdevs and skip any that represent lens or flash >> controllers before creating data links. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> >> This should fix the issues that have been noticed, but perhaps a new flag like >> MEDIA_ENT_FL_HAS_SOURCE or something would be a better way to denote subdevs >> that need data links? >> > I agree this a bit fragile... > > I noticed ancillary links are only created for subdev notifiers, > which have a populated 'sd' and consequentially an entity. Could an > helper that walks the links of the notifier's subdev links and checks > if the subdev at hand is already linked, help ? Maybe with an optional > set of link flags to match on ? Or maybe just check if the subdev's notifier is the same as the rkisp1's notifier? Like: if(sd->notifier!= &rkisp1->notifier) continue That's a bit less clunky than both other solutions I think > > >> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> index 3f5cfa7eb937..e90f0216cb06 100644 >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> @@ -134,6 +134,10 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1) >> sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) >> continue; >> >> + if (sd->entity.function == MEDIA_ENT_F_LENS || >> + sd->entity.function == MEDIA_ENT_F_FLASH) >> + continue; >> + >> ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >> MEDIA_PAD_FL_SOURCE); >> if (ret < 0) { >> -- >> 2.25.1 >>
Hi Dan, On Wed, Jun 08, 2022 at 03:25:36PM +0100, Daniel Scally wrote: > Hi Jacopo > > On 07/06/2022 17:41, Jacopo Mondi wrote: > > Hi Dan > > > > On Mon, Jun 06, 2022 at 11:51:49PM +0100, Daniel Scally wrote: > >> With the introduction of ancillary links, not all subdevs linked to > >> the ISP's v4l2_dev necessarily represent sensors / bridges. Check the > >> function for the subdevs and skip any that represent lens or flash > >> controllers before creating data links. > >> > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> > >> --- > >> > >> This should fix the issues that have been noticed, but perhaps a new flag like > >> MEDIA_ENT_FL_HAS_SOURCE or something would be a better way to denote subdevs > >> that need data links? > >> > > I agree this a bit fragile... > > > > I noticed ancillary links are only created for subdev notifiers, > > which have a populated 'sd' and consequentially an entity. Could an > > helper that walks the links of the notifier's subdev links and checks > > if the subdev at hand is already linked, help ? Maybe with an optional > > set of link flags to match on ? This is actually a mess, as the list of links to be walked is the list of the sensor's notifier, not the one of the rkisp1. Bad advice, sorry.. > > > Or maybe just check if the subdev's notifier is the same as the rkisp1's > notifier? Like: > > > if(sd->notifier!= &rkisp1->notifier) Not all subdevs will have a notifier, won't they ? In facts only sensor that registers a notifier for their connected lenses/flashes will have one. Anyway, I think the issue here is that we walk the list of all subdevs registered to the root notifier's v4l2_dev. All async subdevices matched in the notifiers chain will end up being registered to the root notifier's v4l2_dev, hence also lenses and flashes will appear in this list. list_for_each_entry(sd, &rkisp1->v4l2_dev.subdevs, list) { } Can't we do like the CIO2 does, by walking the list of async subdevs registered to the root notifier only ? This list should not include lenses and flashes if I'm not mistaken. list_for_each_entry(asd, &rkisp1->notifier.asd_list, asd_list) { } You can cast the struct v4l2_async_subdev back to the wrapping struct rkisp1_sensor_async and from there get the sd to create the links on. Could this work in your opinion ? I'm sorry I can't test it right away... > continue > That's a bit less clunky than both other solutions I think > > > > > >> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> index 3f5cfa7eb937..e90f0216cb06 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> @@ -134,6 +134,10 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1) > >> sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) > >> continue; > >> > >> + if (sd->entity.function == MEDIA_ENT_F_LENS || > >> + sd->entity.function == MEDIA_ENT_F_FLASH) > >> + continue; > >> + > >> ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, > >> MEDIA_PAD_FL_SOURCE); > >> if (ret < 0) { > >> -- > >> 2.25.1 > >>
Hi Jacopo On 08/06/2022 16:34, Jacopo Mondi wrote: > Hi Dan, > > On Wed, Jun 08, 2022 at 03:25:36PM +0100, Daniel Scally wrote: >> Hi Jacopo >> >> On 07/06/2022 17:41, Jacopo Mondi wrote: >>> Hi Dan >>> >>> On Mon, Jun 06, 2022 at 11:51:49PM +0100, Daniel Scally wrote: >>>> With the introduction of ancillary links, not all subdevs linked to >>>> the ISP's v4l2_dev necessarily represent sensors / bridges. Check the >>>> function for the subdevs and skip any that represent lens or flash >>>> controllers before creating data links. >>>> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>> --- >>>> >>>> This should fix the issues that have been noticed, but perhaps a new flag like >>>> MEDIA_ENT_FL_HAS_SOURCE or something would be a better way to denote subdevs >>>> that need data links? >>>> >>> I agree this a bit fragile... >>> >>> I noticed ancillary links are only created for subdev notifiers, >>> which have a populated 'sd' and consequentially an entity. Could an >>> helper that walks the links of the notifier's subdev links and checks >>> if the subdev at hand is already linked, help ? Maybe with an optional >>> set of link flags to match on ? > This is actually a mess, as the list of links to be walked is the list > of the sensor's notifier, not the one of the rkisp1. Bad advice, > sorry.. No problem! >> >> Or maybe just check if the subdev's notifier is the same as the rkisp1's >> notifier? Like: >> >> >> if(sd->notifier!= &rkisp1->notifier) > Not all subdevs will have a notifier, won't they ? In facts only > sensor that registers a notifier for their connected lenses/flashes > will have one. sd->notifier is the one the subdev binds to, so the rkisp1 in this case. The notifier that's registered by the sensor would be sd->subdev_notifier...so I think this will work - any subdev that gets to this point should have a notifier, which will be the rkisp1 for the sensors and the sensor for the VCM. > > Anyway, I think the issue here is that we walk the list of all subdevs > registered to the root notifier's v4l2_dev. > > All async subdevices matched in the notifiers chain will end up being > registered to the root notifier's v4l2_dev, hence also lenses and > flashes will appear in this list. > > list_for_each_entry(sd, &rkisp1->v4l2_dev.subdevs, list) { > > } > > Can't we do like the CIO2 does, by walking the list of async subdevs > registered to the root notifier only ? This list should not include > lenses and flashes if I'm not mistaken. > > list_for_each_entry(asd, &rkisp1->notifier.asd_list, asd_list) { > > } > > You can cast the struct v4l2_async_subdev back to the wrapping struct > rkisp1_sensor_async and from there get the sd to create the links on. > Could this work in your opinion ? I'm sorry I can't test it right > away... Yeah I think this should work fine too (it's why the ipu3-cio2 driver doesn't experience the same problem) - I can see how easy it is to switch it over > > > >> continue >> That's a bit less clunky than both other solutions I think >>> >>>> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >>>> index 3f5cfa7eb937..e90f0216cb06 100644 >>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >>>> @@ -134,6 +134,10 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1) >>>> sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) >>>> continue; >>>> >>>> + if (sd->entity.function == MEDIA_ENT_F_LENS || >>>> + sd->entity.function == MEDIA_ENT_F_FLASH) >>>> + continue; >>>> + >>>> ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, >>>> MEDIA_PAD_FL_SOURCE); >>>> if (ret < 0) { >>>> -- >>>> 2.25.1 >>>>
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index 3f5cfa7eb937..e90f0216cb06 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c @@ -134,6 +134,10 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1) sd == &rkisp1->resizer_devs[RKISP1_SELFPATH].sd) continue; + if (sd->entity.function == MEDIA_ENT_F_LENS || + sd->entity.function == MEDIA_ENT_F_FLASH) + continue; + ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode, MEDIA_PAD_FL_SOURCE); if (ret < 0) {
With the introduction of ancillary links, not all subdevs linked to the ISP's v4l2_dev necessarily represent sensors / bridges. Check the function for the subdevs and skip any that represent lens or flash controllers before creating data links. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- This should fix the issues that have been noticed, but perhaps a new flag like MEDIA_ENT_FL_HAS_SOURCE or something would be a better way to denote subdevs that need data links? drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 4 ++++ 1 file changed, 4 insertions(+)