Re: [PATCH] media: rkisp1: Don't create data links for non-sensor subdevs

[Jacopo Mondi <jacopo@jmondi.org>]

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