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

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

[Daniel Scally]

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(+)

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

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

[Daniel Scally]

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(+)

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jacopo Mondi]

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
>

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

[Jacopo Mondi]

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
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Daniel Scally]

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

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

[Daniel Scally]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jacopo Mondi]

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

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

[Jacopo Mondi]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Daniel Scally]

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

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

[Daniel Scally]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip