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