[PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[Daniel Scally]

Creating links and registering subdev nodes during the .complete()
callback has the unfortunate effect of preventing all cameras that
connect to a notifier from working if any one of their drivers fails
to probe. Moving the functionality from .complete() to .bound() allows
those camera sensor drivers that did probe correctly to work regardless.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

This results in v4l2_device_register_subdev_nodes() being called multiple times
but since it's a no-op where the devnode exists already, I think that it's ok.

 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 65 +++++++------------
 1 file changed, 23 insertions(+), 42 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 93cc0577b6db..6a1bcb20725d 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1387,7 +1387,10 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
 {
 	struct cio2_device *cio2 = to_cio2_device(notifier);
 	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
+	struct device *dev = &cio2->pci_dev->dev;
 	struct cio2_queue *q;
+	unsigned int pad;
+	int ret;
 
 	if (cio2->queue[s_asd->csi2.port].sensor)
 		return -EBUSY;
@@ -1398,7 +1401,26 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
 	q->sensor = sd;
 	q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
 
-	return 0;
+	for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
+		if (q->sensor->entity.pads[pad].flags &
+					MEDIA_PAD_FL_SOURCE)
+			break;
+
+	if (pad == q->sensor->entity.num_pads) {
+		dev_err(dev, "failed to find src pad for %s\n",
+			q->sensor->name);
+		return -ENXIO;
+	}
+
+	ret = media_create_pad_link(&q->sensor->entity, pad, &q->subdev.entity,
+				    CIO2_PAD_SINK, 0);
+	if (ret) {
+		dev_err(dev, "failed to create link for %s\n",
+			q->sensor->name);
+		return ret;
+	}
+
+	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
 }
 
 /* The .unbind callback */
@@ -1412,50 +1434,9 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
 	cio2->queue[s_asd->csi2.port].sensor = NULL;
 }
 
-/* .complete() is called after all subdevices have been located */
-static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
-{
-	struct cio2_device *cio2 = to_cio2_device(notifier);
-	struct device *dev = &cio2->pci_dev->dev;
-	struct sensor_async_subdev *s_asd;
-	struct v4l2_async_subdev *asd;
-	struct cio2_queue *q;
-	unsigned int pad;
-	int ret;
-
-	list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
-		s_asd = to_sensor_asd(asd);
-		q = &cio2->queue[s_asd->csi2.port];
-
-		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
-			if (q->sensor->entity.pads[pad].flags &
-						MEDIA_PAD_FL_SOURCE)
-				break;
-
-		if (pad == q->sensor->entity.num_pads) {
-			dev_err(dev, "failed to find src pad for %s\n",
-				q->sensor->name);
-			return -ENXIO;
-		}
-
-		ret = media_create_pad_link(
-				&q->sensor->entity, pad,
-				&q->subdev.entity, CIO2_PAD_SINK,
-				0);
-		if (ret) {
-			dev_err(dev, "failed to create link for %s\n",
-				q->sensor->name);
-			return ret;
-		}
-	}
-
-	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
-}
-
 static const struct v4l2_async_notifier_operations cio2_async_ops = {
 	.bound = cio2_notifier_bound,
 	.unbind = cio2_notifier_unbind,
-	.complete = cio2_notifier_complete,
 };
 
 static int cio2_parse_firmware(struct cio2_device *cio2)
-- 
2.25.1

Re: [PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[Daniel Scally]

Hello all

On 06/05/2022 22:15, Daniel Scally wrote:
> Creating links and registering subdev nodes during the .complete()
> callback has the unfortunate effect of preventing all cameras that
> connect to a notifier from working if any one of their drivers fails
> to probe. Moving the functionality from .complete() to .bound() allows
> those camera sensor drivers that did probe correctly to work regardless.
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>
> This results in v4l2_device_register_subdev_nodes() being called multiple times
> but since it's a no-op where the devnode exists already, I think that it's ok.


There ended up being a problem with this. If a camera sensor driver
registers a notifier via v4l2_async_register_subdev_sensor(), the
devnodes for any device that bound to that notifier (a lens controller
for example) would be created where v4l2_device_register_subdev_nodes()
is called by the ipu3-cio2 driver's .complete() callback. This is
because it won't be called until the lens controller's driver has
probed. On the other hand, if the lens controller's driver probes late
(after all the camera sensor drivers) then its devnodes _won't_ be
created because it'll miss the calls to
v4l2_device_register_subdev_nodes() when ipu3-cio2's .bound() is
triggered as their drivers probe. The effect of this is that we still
need a call to v4l2_device_register_subdev_nodes() in .complete() to
make sure we catch anything that's bound to a notifier registered by one
of the camera sensor drivers. This kinda defeats the purpose of the
patch as if an ISP has one sensor linked to a lens controller and one
one sensor without a lens controller, failure during probe of the driver
for the lens-less sensor will mean .complete() is never called, and so
the devnodes for the lens controller won't get created, and so the
sensor with a lens won't work properly anyway.


So - more thought needed on this I think. I still think it's the right
approach to refactor such that a failure in one sensor driver's probe
does not prevent any other sensors present from working provided _they_
probe correctly, but I'm not sure the best way to achieve it now.



>
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 65 +++++++------------
>  1 file changed, 23 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 93cc0577b6db..6a1bcb20725d 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1387,7 +1387,10 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>  {
>  	struct cio2_device *cio2 = to_cio2_device(notifier);
>  	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
> +	struct device *dev = &cio2->pci_dev->dev;
>  	struct cio2_queue *q;
> +	unsigned int pad;
> +	int ret;
>  
>  	if (cio2->queue[s_asd->csi2.port].sensor)
>  		return -EBUSY;
> @@ -1398,7 +1401,26 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>  	q->sensor = sd;
>  	q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
>  
> -	return 0;
> +	for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> +		if (q->sensor->entity.pads[pad].flags &
> +					MEDIA_PAD_FL_SOURCE)
> +			break;
> +
> +	if (pad == q->sensor->entity.num_pads) {
> +		dev_err(dev, "failed to find src pad for %s\n",
> +			q->sensor->name);
> +		return -ENXIO;
> +	}
> +
> +	ret = media_create_pad_link(&q->sensor->entity, pad, &q->subdev.entity,
> +				    CIO2_PAD_SINK, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to create link for %s\n",
> +			q->sensor->name);
> +		return ret;
> +	}
> +
> +	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
>  }
>  
>  /* The .unbind callback */
> @@ -1412,50 +1434,9 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
>  	cio2->queue[s_asd->csi2.port].sensor = NULL;
>  }
>  
> -/* .complete() is called after all subdevices have been located */
> -static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> -{
> -	struct cio2_device *cio2 = to_cio2_device(notifier);
> -	struct device *dev = &cio2->pci_dev->dev;
> -	struct sensor_async_subdev *s_asd;
> -	struct v4l2_async_subdev *asd;
> -	struct cio2_queue *q;
> -	unsigned int pad;
> -	int ret;
> -
> -	list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
> -		s_asd = to_sensor_asd(asd);
> -		q = &cio2->queue[s_asd->csi2.port];
> -
> -		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> -			if (q->sensor->entity.pads[pad].flags &
> -						MEDIA_PAD_FL_SOURCE)
> -				break;
> -
> -		if (pad == q->sensor->entity.num_pads) {
> -			dev_err(dev, "failed to find src pad for %s\n",
> -				q->sensor->name);
> -			return -ENXIO;
> -		}
> -
> -		ret = media_create_pad_link(
> -				&q->sensor->entity, pad,
> -				&q->subdev.entity, CIO2_PAD_SINK,
> -				0);
> -		if (ret) {
> -			dev_err(dev, "failed to create link for %s\n",
> -				q->sensor->name);
> -			return ret;
> -		}
> -	}
> -
> -	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
> -}
> -
>  static const struct v4l2_async_notifier_operations cio2_async_ops = {
>  	.bound = cio2_notifier_bound,
>  	.unbind = cio2_notifier_unbind,
> -	.complete = cio2_notifier_complete,
>  };
>  
>  static int cio2_parse_firmware(struct cio2_device *cio2)

Re: [PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[Jacopo Mondi]

Hi Dan,
   cc-ing Kieran which has proposed to discuss about this during media
summit

On Fri, May 06, 2022 at 10:15:55PM +0100, Daniel Scally wrote:
> Creating links and registering subdev nodes during the .complete()
> callback has the unfortunate effect of preventing all cameras that
> connect to a notifier from working if any one of their drivers fails
> to probe. Moving the functionality from .complete() to .bound() allows
> those camera sensor drivers that did probe correctly to work regardless.
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>
> This results in v4l2_device_register_subdev_nodes() being called multiple times
> but since it's a no-op where the devnode exists already, I think that it's ok.
>
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 65 +++++++------------
>  1 file changed, 23 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index 93cc0577b6db..6a1bcb20725d 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1387,7 +1387,10 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>  {
>  	struct cio2_device *cio2 = to_cio2_device(notifier);
>  	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
> +	struct device *dev = &cio2->pci_dev->dev;
>  	struct cio2_queue *q;
> +	unsigned int pad;
> +	int ret;
>
>  	if (cio2->queue[s_asd->csi2.port].sensor)
>  		return -EBUSY;
> @@ -1398,7 +1401,26 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>  	q->sensor = sd;
>  	q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
>
> -	return 0;
> +	for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> +		if (q->sensor->entity.pads[pad].flags &
> +					MEDIA_PAD_FL_SOURCE)
> +			break;
> +
> +	if (pad == q->sensor->entity.num_pads) {
> +		dev_err(dev, "failed to find src pad for %s\n",
> +			q->sensor->name);
> +		return -ENXIO;
> +	}
> +
> +	ret = media_create_pad_link(&q->sensor->entity, pad, &q->subdev.entity,
> +				    CIO2_PAD_SINK, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to create link for %s\n",
> +			q->sensor->name);
> +		return ret;
> +	}
> +
> +	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
>  }
>
>  /* The .unbind callback */
> @@ -1412,50 +1434,9 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
>  	cio2->queue[s_asd->csi2.port].sensor = NULL;
>  }
>
> -/* .complete() is called after all subdevices have been located */
> -static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> -{
> -	struct cio2_device *cio2 = to_cio2_device(notifier);
> -	struct device *dev = &cio2->pci_dev->dev;
> -	struct sensor_async_subdev *s_asd;
> -	struct v4l2_async_subdev *asd;
> -	struct cio2_queue *q;
> -	unsigned int pad;
> -	int ret;
> -
> -	list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
> -		s_asd = to_sensor_asd(asd);
> -		q = &cio2->queue[s_asd->csi2.port];
> -
> -		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> -			if (q->sensor->entity.pads[pad].flags &
> -						MEDIA_PAD_FL_SOURCE)
> -				break;
> -
> -		if (pad == q->sensor->entity.num_pads) {
> -			dev_err(dev, "failed to find src pad for %s\n",
> -				q->sensor->name);
> -			return -ENXIO;
> -		}
> -
> -		ret = media_create_pad_link(
> -				&q->sensor->entity, pad,
> -				&q->subdev.entity, CIO2_PAD_SINK,
> -				0);
> -		if (ret) {
> -			dev_err(dev, "failed to create link for %s\n",
> -				q->sensor->name);
> -			return ret;
> -		}
> -	}
> -
> -	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
> -}
> -
>  static const struct v4l2_async_notifier_operations cio2_async_ops = {
>  	.bound = cio2_notifier_bound,
>  	.unbind = cio2_notifier_unbind,
> -	.complete = cio2_notifier_complete,
>  };
>
>  static int cio2_parse_firmware(struct cio2_device *cio2)
> --
> 2.25.1
>

Re: [PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[Kieran Bingham]

Quoting Daniel Scally (2022-06-06 18:04:40)
> Hello all
> 
> On 06/05/2022 22:15, Daniel Scally wrote:
> > Creating links and registering subdev nodes during the .complete()
> > callback has the unfortunate effect of preventing all cameras that
> > connect to a notifier from working if any one of their drivers fails
> > to probe. Moving the functionality from .complete() to .bound() allows
> > those camera sensor drivers that did probe correctly to work regardless.
> >
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> >
> > This results in v4l2_device_register_subdev_nodes() being called multiple times
> > but since it's a no-op where the devnode exists already, I think that it's ok.
> 
> 
> There ended up being a problem with this. If a camera sensor driver
> registers a notifier via v4l2_async_register_subdev_sensor(), the
> devnodes for any device that bound to that notifier (a lens controller
> for example) would be created where v4l2_device_register_subdev_nodes()
> is called by the ipu3-cio2 driver's .complete() callback. This is
> because it won't be called until the lens controller's driver has
> probed. On the other hand, if the lens controller's driver probes late
> (after all the camera sensor drivers) then its devnodes _won't_ be
> created because it'll miss the calls to
> v4l2_device_register_subdev_nodes() when ipu3-cio2's .bound() is
> triggered as their drivers probe. The effect of this is that we still
> need a call to v4l2_device_register_subdev_nodes() in .complete() to
> make sure we catch anything that's bound to a notifier registered by one
> of the camera sensor drivers. This kinda defeats the purpose of the
> patch as if an ISP has one sensor linked to a lens controller and one
> one sensor without a lens controller, failure during probe of the driver
> for the lens-less sensor will mean .complete() is never called, and so
> the devnodes for the lens controller won't get created, and so the
> sensor with a lens won't work properly anyway.
> 
> 
> So - more thought needed on this I think. I still think it's the right
> approach to refactor such that a failure in one sensor driver's probe
> does not prevent any other sensors present from working provided _they_
> probe correctly, but I'm not sure the best way to achieve it now.


As highlighted by Jacopo, I've proposed [0] that we talk more about this and
really figure out a path to solving this (and planning to get it done I
hope) at the media summit which will hopefully be held in Dublin.

I last discussed this at the media summit in 2018! (life moves fast...)
and my slides are at [1].  I guess we've had time to think about it all
a bit more now since 2018, ... And certainly more usecases for this have
come up since.

Of course, ELCE/Summit is in September, so we can still work on this
topic before then too!

[0] https://lore.kernel.org/linux-media/165337661126.402452.10107682065782670158@Monstersaurus/
[1] https://docs.google.com/presentation/d/1Vdydt46QQtcYC0Sb7pn4qpL-ifnQdaCl0AkTSUIKE3U

--
Kieran



> >  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 65 +++++++------------
> >  1 file changed, 23 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > index 93cc0577b6db..6a1bcb20725d 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > @@ -1387,7 +1387,10 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
> >  {
> >       struct cio2_device *cio2 = to_cio2_device(notifier);
> >       struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
> > +     struct device *dev = &cio2->pci_dev->dev;
> >       struct cio2_queue *q;
> > +     unsigned int pad;
> > +     int ret;
> >  
> >       if (cio2->queue[s_asd->csi2.port].sensor)
> >               return -EBUSY;
> > @@ -1398,7 +1401,26 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
> >       q->sensor = sd;
> >       q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
> >  
> > -     return 0;
> > +     for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> > +             if (q->sensor->entity.pads[pad].flags &
> > +                                     MEDIA_PAD_FL_SOURCE)
> > +                     break;
> > +
> > +     if (pad == q->sensor->entity.num_pads) {
> > +             dev_err(dev, "failed to find src pad for %s\n",
> > +                     q->sensor->name);
> > +             return -ENXIO;
> > +     }
> > +
> > +     ret = media_create_pad_link(&q->sensor->entity, pad, &q->subdev.entity,
> > +                                 CIO2_PAD_SINK, 0);
> > +     if (ret) {
> > +             dev_err(dev, "failed to create link for %s\n",
> > +                     q->sensor->name);
> > +             return ret;
> > +     }
> > +
> > +     return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
> >  }
> >  
> >  /* The .unbind callback */
> > @@ -1412,50 +1434,9 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
> >       cio2->queue[s_asd->csi2.port].sensor = NULL;
> >  }
> >  
> > -/* .complete() is called after all subdevices have been located */
> > -static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> > -{
> > -     struct cio2_device *cio2 = to_cio2_device(notifier);
> > -     struct device *dev = &cio2->pci_dev->dev;
> > -     struct sensor_async_subdev *s_asd;
> > -     struct v4l2_async_subdev *asd;
> > -     struct cio2_queue *q;
> > -     unsigned int pad;
> > -     int ret;
> > -
> > -     list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
> > -             s_asd = to_sensor_asd(asd);
> > -             q = &cio2->queue[s_asd->csi2.port];
> > -
> > -             for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
> > -                     if (q->sensor->entity.pads[pad].flags &
> > -                                             MEDIA_PAD_FL_SOURCE)
> > -                             break;
> > -
> > -             if (pad == q->sensor->entity.num_pads) {
> > -                     dev_err(dev, "failed to find src pad for %s\n",
> > -                             q->sensor->name);
> > -                     return -ENXIO;
> > -             }
> > -
> > -             ret = media_create_pad_link(
> > -                             &q->sensor->entity, pad,
> > -                             &q->subdev.entity, CIO2_PAD_SINK,
> > -                             0);
> > -             if (ret) {
> > -                     dev_err(dev, "failed to create link for %s\n",
> > -                             q->sensor->name);
> > -                     return ret;
> > -             }
> > -     }
> > -
> > -     return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
> > -}
> > -
> >  static const struct v4l2_async_notifier_operations cio2_async_ops = {
> >       .bound = cio2_notifier_bound,
> >       .unbind = cio2_notifier_unbind,
> > -     .complete = cio2_notifier_complete,
> >  };
> >  
> >  static int cio2_parse_firmware(struct cio2_device *cio2)

Re: [PATCH] media: ipu3-cio2: Move functionality from .complete() to .bound()

[Daniel Scally]

Morning Kieran

On 07/06/2022 08:35, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-06-06 18:04:40)
>> Hello all
>>
>> On 06/05/2022 22:15, Daniel Scally wrote:
>>> Creating links and registering subdev nodes during the .complete()
>>> callback has the unfortunate effect of preventing all cameras that
>>> connect to a notifier from working if any one of their drivers fails
>>> to probe. Moving the functionality from .complete() to .bound() allows
>>> those camera sensor drivers that did probe correctly to work regardless.
>>>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>> ---
>>>
>>> This results in v4l2_device_register_subdev_nodes() being called multiple times
>>> but since it's a no-op where the devnode exists already, I think that it's ok.
>>
>> There ended up being a problem with this. If a camera sensor driver
>> registers a notifier via v4l2_async_register_subdev_sensor(), the
>> devnodes for any device that bound to that notifier (a lens controller
>> for example) would be created where v4l2_device_register_subdev_nodes()
>> is called by the ipu3-cio2 driver's .complete() callback. This is
>> because it won't be called until the lens controller's driver has
>> probed. On the other hand, if the lens controller's driver probes late
>> (after all the camera sensor drivers) then its devnodes _won't_ be
>> created because it'll miss the calls to
>> v4l2_device_register_subdev_nodes() when ipu3-cio2's .bound() is
>> triggered as their drivers probe. The effect of this is that we still
>> need a call to v4l2_device_register_subdev_nodes() in .complete() to
>> make sure we catch anything that's bound to a notifier registered by one
>> of the camera sensor drivers. This kinda defeats the purpose of the
>> patch as if an ISP has one sensor linked to a lens controller and one
>> one sensor without a lens controller, failure during probe of the driver
>> for the lens-less sensor will mean .complete() is never called, and so
>> the devnodes for the lens controller won't get created, and so the
>> sensor with a lens won't work properly anyway.
>>
>>
>> So - more thought needed on this I think. I still think it's the right
>> approach to refactor such that a failure in one sensor driver's probe
>> does not prevent any other sensors present from working provided _they_
>> probe correctly, but I'm not sure the best way to achieve it now.
>
> As highlighted by Jacopo, I've proposed [0] that we talk more about this and
> really figure out a path to solving this (and planning to get it done I
> hope) at the media summit which will hopefully be held in Dublin.
>
> I last discussed this at the media summit in 2018! (life moves fast...)
> and my slides are at [1].  I guess we've had time to think about it all
> a bit more now since 2018, ... And certainly more usecases for this have
> come up since.
>
> Of course, ELCE/Summit is in September, so we can still work on this
> topic before then too!


Ah great, thanks. And nice summary of the problem too - much better than
my attempts! Well, I'll keep it in mind and see if I can come up with a
better way of handling this, as it's a bit of a pain when parts of the
media graph are still in development, but a wider discussion about it
sounds like a great idea to me.

> [0] https://lore.kernel.org/linux-media/165337661126.402452.10107682065782670158@Monstersaurus/
> [1] https://docs.google.com/presentation/d/1Vdydt46QQtcYC0Sb7pn4qpL-ifnQdaCl0AkTSUIKE3U
>
> --
> Kieran
>
>
>
>>>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 65 +++++++------------
>>>  1 file changed, 23 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>>> index 93cc0577b6db..6a1bcb20725d 100644
>>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
>>> @@ -1387,7 +1387,10 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>>>  {
>>>       struct cio2_device *cio2 = to_cio2_device(notifier);
>>>       struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
>>> +     struct device *dev = &cio2->pci_dev->dev;
>>>       struct cio2_queue *q;
>>> +     unsigned int pad;
>>> +     int ret;
>>>  
>>>       if (cio2->queue[s_asd->csi2.port].sensor)
>>>               return -EBUSY;
>>> @@ -1398,7 +1401,26 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>>>       q->sensor = sd;
>>>       q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port);
>>>  
>>> -     return 0;
>>> +     for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
>>> +             if (q->sensor->entity.pads[pad].flags &
>>> +                                     MEDIA_PAD_FL_SOURCE)
>>> +                     break;
>>> +
>>> +     if (pad == q->sensor->entity.num_pads) {
>>> +             dev_err(dev, "failed to find src pad for %s\n",
>>> +                     q->sensor->name);
>>> +             return -ENXIO;
>>> +     }
>>> +
>>> +     ret = media_create_pad_link(&q->sensor->entity, pad, &q->subdev.entity,
>>> +                                 CIO2_PAD_SINK, 0);
>>> +     if (ret) {
>>> +             dev_err(dev, "failed to create link for %s\n",
>>> +                     q->sensor->name);
>>> +             return ret;
>>> +     }
>>> +
>>> +     return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
>>>  }
>>>  
>>>  /* The .unbind callback */
>>> @@ -1412,50 +1434,9 @@ static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier,
>>>       cio2->queue[s_asd->csi2.port].sensor = NULL;
>>>  }
>>>  
>>> -/* .complete() is called after all subdevices have been located */
>>> -static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>>> -{
>>> -     struct cio2_device *cio2 = to_cio2_device(notifier);
>>> -     struct device *dev = &cio2->pci_dev->dev;
>>> -     struct sensor_async_subdev *s_asd;
>>> -     struct v4l2_async_subdev *asd;
>>> -     struct cio2_queue *q;
>>> -     unsigned int pad;
>>> -     int ret;
>>> -
>>> -     list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
>>> -             s_asd = to_sensor_asd(asd);
>>> -             q = &cio2->queue[s_asd->csi2.port];
>>> -
>>> -             for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
>>> -                     if (q->sensor->entity.pads[pad].flags &
>>> -                                             MEDIA_PAD_FL_SOURCE)
>>> -                             break;
>>> -
>>> -             if (pad == q->sensor->entity.num_pads) {
>>> -                     dev_err(dev, "failed to find src pad for %s\n",
>>> -                             q->sensor->name);
>>> -                     return -ENXIO;
>>> -             }
>>> -
>>> -             ret = media_create_pad_link(
>>> -                             &q->sensor->entity, pad,
>>> -                             &q->subdev.entity, CIO2_PAD_SINK,
>>> -                             0);
>>> -             if (ret) {
>>> -                     dev_err(dev, "failed to create link for %s\n",
>>> -                             q->sensor->name);
>>> -                     return ret;
>>> -             }
>>> -     }
>>> -
>>> -     return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
>>> -}
>>> -
>>>  static const struct v4l2_async_notifier_operations cio2_async_ops = {
>>>       .bound = cio2_notifier_bound,
>>>       .unbind = cio2_notifier_unbind,
>>> -     .complete = cio2_notifier_complete,
>>>  };
>>>  
>>>  static int cio2_parse_firmware(struct cio2_device *cio2)