notifier is skipped in some situations

notifier is skipped in some situations

[Jacob Chen]

Hi Sakari,

I encountered a problem when using async sub-notifiers.

It's like that:
    There are two notifiers, and they are waiting for one subdev.
    When this subdev is probing, only one notifier is completed and
the other one is skipped.

I found that in v15 of patch "v4l: async: Allow binding notifiers to
sub-devices", "v4l2_async_notifier_complete" is replaced by
v4l2_async_notifier_call_complete, which make it only complete one
notifier.

Why is it changed? Can this be fixed?

Re: notifier is skipped in some situations

[Sakari Ailus]

Hi Jacob,

On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
> Hi Sakari,
> 
> I encountered a problem when using async sub-notifiers.
> 
> It's like that:
>     There are two notifiers, and they are waiting for one subdev.
>     When this subdev is probing, only one notifier is completed and
> the other one is skipped.

Do you have a graph that has two master drivers (that register the
notifier) and both are connected to the same sub-device? Could you provide
exact graph you have?

> 
> I found that in v15 of patch "v4l: async: Allow binding notifiers to
> sub-devices", "v4l2_async_notifier_complete" is replaced by
> v4l2_async_notifier_call_complete, which make it only complete one
> notifier.
> 
> Why is it changed? Can this be fixed?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: notifier is skipped in some situations

[Tomasz Figa]

Hi Sakari,

We have the following graph:

    ISP (registers notifier for v4l2_dev)

     |

    PHY (registers notifier for v4l2_subdev, just like sensors for
flash/focuser)

  |       \

sensor0   sensor1

...

Both ISP and PHY are completely separate drivers not directly aware of
each other, since we have several different PHY IP blocks that we need
to support and some of them are multi-functional, such as CSI+DSI PHY
and need to be supported by drivers independent from the ISP driver.

Best regards,
Tomasz


On Fri, Nov 24, 2017 at 5:55 PM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Hi Jacob,
>
> On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
>> Hi Sakari,
>>
>> I encountered a problem when using async sub-notifiers.
>>
>> It's like that:
>>     There are two notifiers, and they are waiting for one subdev.
>>     When this subdev is probing, only one notifier is completed and
>> the other one is skipped.
>
> Do you have a graph that has two master drivers (that register the
> notifier) and both are connected to the same sub-device? Could you provide
> exact graph you have?
>
>>
>> I found that in v15 of patch "v4l: async: Allow binding notifiers to
>> sub-devices", "v4l2_async_notifier_complete" is replaced by
>> v4l2_async_notifier_call_complete, which make it only complete one
>> notifier.
>>
>> Why is it changed? Can this be fixed?
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: notifier is skipped in some situations

[Sakari Ailus]

Hi Tomasz,

On Fri, Nov 24, 2017 at 06:03:26PM +0900, Tomasz Figa wrote:
> Hi Sakari,
> 
> We have the following graph:
> 
>     ISP (registers notifier for v4l2_dev)
> 
>      |
> 
>     PHY (registers notifier for v4l2_subdev, just like sensors for
> flash/focuser)
> 
>   |       \
> 
> sensor0   sensor1
> 
> ...
> 
> Both ISP and PHY are completely separate drivers not directly aware of
> each other, since we have several different PHY IP blocks that we need
> to support and some of them are multi-functional, such as CSI+DSI PHY
> and need to be supported by drivers independent from the ISP driver.

That should work fine. In the above case there are two notifiers, indeed,
but they're not expecting the *same* sub-devices.

What this could be about is that in some version of the set I disabled the
complete callback on the sub-notifiers for two reasons: there was no need
seen for them and the complete callback is problematic in general (there's
been discussion on that, mostly related to earlier versions of the fwnode
parsing patchset, on #v4l and along the Renesas rcar-csi2 patchsets).

> 
> Best regards,
> Tomasz
> 
> 
> On Fri, Nov 24, 2017 at 5:55 PM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > Hi Jacob,
> >
> > On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
> >> Hi Sakari,
> >>
> >> I encountered a problem when using async sub-notifiers.
> >>
> >> It's like that:
> >>     There are two notifiers, and they are waiting for one subdev.
> >>     When this subdev is probing, only one notifier is completed and
> >> the other one is skipped.
> >
> > Do you have a graph that has two master drivers (that register the
> > notifier) and both are connected to the same sub-device? Could you provide
> > exact graph you have?
> >
> >>
> >> I found that in v15 of patch "v4l: async: Allow binding notifiers to
> >> sub-devices", "v4l2_async_notifier_complete" is replaced by
> >> v4l2_async_notifier_call_complete, which make it only complete one
> >> notifier.
> >>
> >> Why is it changed? Can this be fixed?
> >
> > --
> > Sakari Ailus
> > sakari.ailus@linux.intel.com

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: notifier is skipped in some situations

[Tomasz Figa]

On Fri, Nov 24, 2017 at 6:17 PM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Hi Tomasz,
>
> On Fri, Nov 24, 2017 at 06:03:26PM +0900, Tomasz Figa wrote:
>> Hi Sakari,
>>
>> We have the following graph:
>>
>>     ISP (registers notifier for v4l2_dev)
>>
>>      |
>>
>>     PHY (registers notifier for v4l2_subdev, just like sensors for
>> flash/focuser)
>>
>>   |       \
>>
>> sensor0   sensor1
>>
>> ...
>>
>> Both ISP and PHY are completely separate drivers not directly aware of
>> each other, since we have several different PHY IP blocks that we need
>> to support and some of them are multi-functional, such as CSI+DSI PHY
>> and need to be supported by drivers independent from the ISP driver.
>
> That should work fine. In the above case there are two notifiers, indeed,
> but they're not expecting the *same* sub-devices.

Got it.

Jacob, could you make sure there are no mistakes in the Device Tree source?

Best regards,
Tomasz

>
> What this could be about is that in some version of the set I disabled the
> complete callback on the sub-notifiers for two reasons: there was no need
> seen for them and the complete callback is problematic in general (there's
> been discussion on that, mostly related to earlier versions of the fwnode
> parsing patchset, on #v4l and along the Renesas rcar-csi2 patchsets).
>
>>
>> Best regards,
>> Tomasz
>>
>>
>> On Fri, Nov 24, 2017 at 5:55 PM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>> > Hi Jacob,
>> >
>> > On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
>> >> Hi Sakari,
>> >>
>> >> I encountered a problem when using async sub-notifiers.
>> >>
>> >> It's like that:
>> >>     There are two notifiers, and they are waiting for one subdev.
>> >>     When this subdev is probing, only one notifier is completed and
>> >> the other one is skipped.
>> >
>> > Do you have a graph that has two master drivers (that register the
>> > notifier) and both are connected to the same sub-device? Could you provide
>> > exact graph you have?
>> >
>> >>
>> >> I found that in v15 of patch "v4l: async: Allow binding notifiers to
>> >> sub-devices", "v4l2_async_notifier_complete" is replaced by
>> >> v4l2_async_notifier_call_complete, which make it only complete one
>> >> notifier.
>> >>
>> >> Why is it changed? Can this be fixed?
>> >
>> > --
>> > Sakari Ailus
>> > sakari.ailus@linux.intel.com
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: notifier is skipped in some situations

[Sakari Ailus]

Hi Jacob,

On Mon, Nov 27, 2017 at 03:03:59PM +0800, Jacob Chen wrote:
> Hi,
> 
> On 2017年11月25日星期六 CST 下午1:05:42 you wrote:
> > Hi,
> > 
> > On 2017年11月24日星期五 CST 下午6:19:36 you wrote:
> > > On Fri, Nov 24, 2017 at 6:17 PM, Sakari Ailus
> > > 
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > Hi Tomasz,
> > > > 
> > > > On Fri, Nov 24, 2017 at 06:03:26PM +0900, Tomasz Figa wrote:
> > > >> Hi Sakari,
> > > >> 
> > > >> We have the following graph:
> > > >>     ISP (registers notifier for v4l2_dev)
> > > >>     
> > > >>     
> > > >>     
> > > >>     PHY (registers notifier for v4l2_subdev, just like sensors for
> > > >> 
> > > >> flash/focuser)
> > > >> 
> > > >>   |       \
> > > >> 
> > > >> sensor0   sensor1
> > > >> 
> > > >> ...
> > > >> 
> > > >> Both ISP and PHY are completely separate drivers not directly aware of
> > > >> each other, since we have several different PHY IP blocks that we need
> > > >> to support and some of them are multi-functional, such as CSI+DSI PHY
> > > >> and need to be supported by drivers independent from the ISP driver.
> > > > 
> > > > That should work fine. In the above case there are two notifiers,
> > > > indeed,
> > > > but they're not expecting the *same* sub-devices.
> > > 
> > > Got it.
> > > 
> > > Jacob, could you make sure there are no mistakes in the Device Tree
> > > source?
> > > 
> > > Best regards,
> > > Tomasz
> > 
> > I see...
> > This problem might be sloved by moving `v4l2_async_subdev_notifier_register`
> > after `v4l2_async_register_subdev`. I will test it.
> > 
> > > > What this could be about is that in some version of the set I disabled
> > > > the
> > > > complete callback on the sub-notifiers for two reasons: there was no
> > > > need
> > > > seen for them and the complete callback is problematic in general
> > > > (there's
> > > > been discussion on that, mostly related to earlier versions of the
> > > > fwnode
> > > > parsing patchset, on #v4l and along the Renesas rcar-csi2 patchsets).
> > > > 
> > > >> Best regards,
> > > >> Tomasz
> > > >> 
> > > >> 
> > > >> On Fri, Nov 24, 2017 at 5:55 PM, Sakari Ailus
> > > >> 
> > > >> <sakari.ailus@linux.intel.com> wrote:
> > > >> > Hi Jacob,
> > > >> > 
> > > >> > On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
> > > >> >> Hi Sakari,
> > > >> >> 
> > > >> >> I encountered a problem when using async sub-notifiers.
> > > >> >> 
> > > >> >> It's like that:
> > > >> >>     There are two notifiers, and they are waiting for one subdev.
> > > >> >>     When this subdev is probing, only one notifier is completed and
> > > >> >> 
> > > >> >> the other one is skipped.
> > > >> > 
> > > >> > Do you have a graph that has two master drivers (that register the
> > > >> > notifier) and both are connected to the same sub-device? Could you
> > > >> > provide
> > > >> > exact graph you have?
> > > >> > 
> > > >> >> I found that in v15 of patch "v4l: async: Allow binding notifiers to
> > > >> >> sub-devices", "v4l2_async_notifier_complete" is replaced by
> > > >> >> v4l2_async_notifier_call_complete, which make it only complete one
> > > >> >> notifier.
> > > >> >> 
> > > >> >> Why is it changed? Can this be fixed?
> > > >> > 
> > > >> > --
> > > >> > Sakari Ailus
> > > >> > sakari.ailus@linux.intel.com
> > > > 
> > > > --
> > > > Sakari Ailus
> > > > sakari.ailus@linux.intel.com
> 
> I make a mistake, they are not expecting same subdev.....
> 
> The problem is that a notifier regsitered by  
> `v4l2_async_subdev_notifier_register` will never be completed, becuase 
> 1.`notifier->waiting` is always not empty, so v4l2_async_notifier_try_complete 
> won't be called.
> 2. In old code, it's called by its parent, but now it won't.

Could you provide a bit more context, what exactly fails and in which order
the notifiers are registered and async sub-device matches are found?

> 
> 
> > static int v4l2_async_notifier_try_complete(
> > 	struct v4l2_async_notifier *notifier)
> > {
> > 	/* Quick check whether there are still more sub-devices here. */
> > 	if (!list_empty(&notifier->waiting))
> > 		return 0;
> 
> not empty
> 
> > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> > 				   struct v4l2_device *v4l2_dev,
> > 				   struct v4l2_subdev *sd,
> > 				   struct v4l2_async_subdev *asd)
> > {
> > 	struct v4l2_async_notifier *subdev_notifier;
> > 	int ret;
> > 
> > 	ret = v4l2_device_register_subdev(v4l2_dev, sd);
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> > 	if (ret < 0) {
> > 		v4l2_device_unregister_subdev(sd);
> > 		return ret;
> > 	}
> > 
> > 	/* Remove from the waiting list */
> > 	list_del(&asd->list);
> 
> asd is removed from the waiting list in `v4l2_async_match_notify`, but
> 
> >static int v4l2_async_notifier_try_all_subdevs(
> >	struct v4l2_async_notifier *notifier)
> >{
> >	...
> >	if (!v4l2_dev)
> >		return 0;
> 
> A notifier regsitered by  `v4l2_async_subdev_notifier_register` will return in 
> here.
> 
> >	...
> >		ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
>     
> 
> >int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >{
> >	...
> >	list_for_each_entry(notifier, &notifier_list, list) {
> >		struct v4l2_device *v4l2_dev =
> >			v4l2_async_notifier_find_v4l2_dev(notifier);
> >		struct v4l2_async_subdev *asd;
> >
> >		if (!v4l2_dev)
> >			continue;
> >
> 
> same
> 
> >		asd = v4l2_async_find_match(notifier, sd);
> >		if (!asd)
> >			continue;
> >
> >		ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
> >					      asd);
> 
> 
> 
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: notifier is skipped in some situations

[Sakari Ailus]

Hi Jacob,

On Thu, Nov 30, 2017 at 09:37:06AM +0800, Jacob Chen wrote:
> Hi Sakari,
> 
> On 2017年11月29日星期三 CST 下午2:29:03 you wrote:
> > Hi Jacob,
> > 
> > On Mon, Nov 27, 2017 at 03:03:59PM +0800, Jacob Chen wrote:
> > > Hi,
> > > 
> > > On 2017年11月25日星期六 CST 下午1:05:42 you wrote:
> > > > Hi,
> > > > 
> > > > On 2017年11月24日星期五 CST 下午6:19:36 you wrote:
> > > > > On Fri, Nov 24, 2017 at 6:17 PM, Sakari Ailus
> > > > > 
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > Hi Tomasz,
> > > > > > 
> > > > > > On Fri, Nov 24, 2017 at 06:03:26PM +0900, Tomasz Figa wrote:
> > > > > >> Hi Sakari,
> > > > > >> 
> > > > > >> We have the following graph:
> > > > > >>     ISP (registers notifier for v4l2_dev)
> > > > > >>     
> > > > > >>     
> > > > > >>     
> > > > > >>     PHY (registers notifier for v4l2_subdev, just like sensors for
> > > > > >> 
> > > > > >> flash/focuser)
> > > > > >> 
> > > > > >>   |       \
> > > > > >> 
> > > > > >> sensor0   sensor1
> > > > > >> 
> > > > > >> ...
> > > > > >> 
> > > > > >> Both ISP and PHY are completely separate drivers not directly aware
> > > > > >> of
> > > > > >> each other, since we have several different PHY IP blocks that we
> > > > > >> need
> > > > > >> to support and some of them are multi-functional, such as CSI+DSI
> > > > > >> PHY
> > > > > >> and need to be supported by drivers independent from the ISP
> > > > > >> driver.
> > > > > > 
> > > > > > That should work fine. In the above case there are two notifiers,
> > > > > > indeed,
> > > > > > but they're not expecting the *same* sub-devices.
> > > > > 
> > > > > Got it.
> > > > > 
> > > > > Jacob, could you make sure there are no mistakes in the Device Tree
> > > > > source?
> > > > > 
> > > > > Best regards,
> > > > > Tomasz
> > > > 
> > > > I see...
> > > > This problem might be sloved by moving
> > > > `v4l2_async_subdev_notifier_register` after
> > > > `v4l2_async_register_subdev`. I will test it.
> > > > 
> > > > > > What this could be about is that in some version of the set I
> > > > > > disabled
> > > > > > the
> > > > > > complete callback on the sub-notifiers for two reasons: there was no
> > > > > > need
> > > > > > seen for them and the complete callback is problematic in general
> > > > > > (there's
> > > > > > been discussion on that, mostly related to earlier versions of the
> > > > > > fwnode
> > > > > > parsing patchset, on #v4l and along the Renesas rcar-csi2
> > > > > > patchsets).
> > > > > > 
> > > > > >> Best regards,
> > > > > >> Tomasz
> > > > > >> 
> > > > > >> 
> > > > > >> On Fri, Nov 24, 2017 at 5:55 PM, Sakari Ailus
> > > > > >> 
> > > > > >> <sakari.ailus@linux.intel.com> wrote:
> > > > > >> > Hi Jacob,
> > > > > >> > 
> > > > > >> > On Fri, Nov 24, 2017 at 09:00:14AM +0800, Jacob Chen wrote:
> > > > > >> >> Hi Sakari,
> > > > > >> >> 
> > > > > >> >> I encountered a problem when using async sub-notifiers.
> > > > > >> >> 
> > > > > >> >> It's like that:
> > > > > >> >>     There are two notifiers, and they are waiting for one
> > > > > >> >>     subdev.
> > > > > >> >>     When this subdev is probing, only one notifier is completed
> > > > > >> >>     and
> > > > > >> >> 
> > > > > >> >> the other one is skipped.
> > > > > >> > 
> > > > > >> > Do you have a graph that has two master drivers (that register
> > > > > >> > the
> > > > > >> > notifier) and both are connected to the same sub-device? Could
> > > > > >> > you
> > > > > >> > provide
> > > > > >> > exact graph you have?
> > > > > >> > 
> > > > > >> >> I found that in v15 of patch "v4l: async: Allow binding
> > > > > >> >> notifiers to
> > > > > >> >> sub-devices", "v4l2_async_notifier_complete" is replaced by
> > > > > >> >> v4l2_async_notifier_call_complete, which make it only complete
> > > > > >> >> one
> > > > > >> >> notifier.
> > > > > >> >> 
> > > > > >> >> Why is it changed? Can this be fixed?
> > > > > >> > 
> > > > > >> > --
> > > > > >> > Sakari Ailus
> > > > > >> > sakari.ailus@linux.intel.com
> > > > > > 
> > > > > > --
> > > > > > Sakari Ailus
> > > > > > sakari.ailus@linux.intel.com
> > > 
> > > I make a mistake, they are not expecting same subdev.....
> > > 
> > > The problem is that a notifier regsitered by
> > > `v4l2_async_subdev_notifier_register` will never be completed, becuase
> > > 1.`notifier->waiting` is always not empty, so
> > > v4l2_async_notifier_try_complete won't be called.
> > > 2. In old code, it's called by its parent, but now it won't.
> > 
> > Could you provide a bit more context, what exactly fails and in which order
> > the notifiers are registered and async sub-device matches are found?
> > 
> 
> The order doesn't matter.
> Both of below orders will fail.
> 1.
> v4l2_async_notifier_parse_fwnode_endpoints_by_port(NOTIFIER1)
> v4l2_async_subdev_notifier_register(NOTIFIER1, PHY)
> v4l2_async_register_subdev(PHY)
> 
> 1.
> v4l2_async_register_subdev(PHY)
> v4l2_async_notifier_parse_fwnode_endpoints_by_port(NOTIFIER1)
> v4l2_async_subdev_notifier_register(NOTIFIER1, PHY)
> 
> found Match:
> NOTIFIER1 --> SENSOR
> 
> dts:
> PHY --> SENSOR
> 
> By having a quick look at the code, i don't find a way that a notifier 
> regsitered by v4l2_async_subdev_notifier_register can be completed.

That's right. As I previously explained, it is a design decision not to
support complete callback for sub-device notifiers. This is also
documented in KernelDoc for struct v4l2_async_notifier_operations but it
could be added to v4l2_async_subdev_notifier_register for clarity.

Do you have a particular need for calling the complete callback for the
sub-device notifier?

> 
> 
> > > > static int v4l2_async_notifier_try_complete(
> > > > 
> > > > 	struct v4l2_async_notifier *notifier)
> > > > 
> > > > {
> > > > 
> > > > 	/* Quick check whether there are still more sub-devices here. */
> > > > 	if (!list_empty(&notifier->waiting))
> > > > 	
> > > > 		return 0;
> > > 
> > > not empty
> > > 
> > > > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> > > > 
> > > > 				   struct v4l2_device *v4l2_dev,
> > > > 				   struct v4l2_subdev *sd,
> > > > 				   struct v4l2_async_subdev *asd)
> > > > 
> > > > {
> > > > 
> > > > 	struct v4l2_async_notifier *subdev_notifier;
> > > > 	int ret;
> > > > 	
> > > > 	ret = v4l2_device_register_subdev(v4l2_dev, sd);
> > > > 	if (ret < 0)
> > > > 	
> > > > 		return ret;
> > > > 	
> > > > 	ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> > > > 	if (ret < 0) {
> > > > 	
> > > > 		v4l2_device_unregister_subdev(sd);
> > > > 		return ret;
> > > > 	
> > > > 	}
> > > > 	
> > > > 	/* Remove from the waiting list */
> > > > 	list_del(&asd->list);
> > > 
> > > asd is removed from the waiting list in `v4l2_async_match_notify`, but
> > > 
> > > >static int v4l2_async_notifier_try_all_subdevs(
> > > >
> > > >	struct v4l2_async_notifier *notifier)
> > > >
> > > >{
> > > >
> > > >	...
> > > >	if (!v4l2_dev)
> > > >	
> > > >		return 0;
> > > 
> > > A notifier regsitered by  `v4l2_async_subdev_notifier_register` will
> > > return in here.
> > > 
> > > >	...
> > > >	
> > > >		ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd);
> > > >
> > > >int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > > >{
> > > >
> > > >	...
> > > >	list_for_each_entry(notifier, &notifier_list, list) {
> > > >	
> > > >		struct v4l2_device *v4l2_dev =
> > > >		
> > > >			v4l2_async_notifier_find_v4l2_dev(notifier);
> > > >		
> > > >		struct v4l2_async_subdev *asd;
> > > >		
> > > >		if (!v4l2_dev)
> > > >		
> > > >			continue;
> > > 
> > > same
> > > 
> > > >		asd = v4l2_async_find_match(notifier, sd);
> > > >		if (!asd)
> > > >		
> > > >			continue;
> > > >		
> > > >		ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
> > > >		
> > > >					      asd);
> 
> 
> 
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com