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