media: intel-ipu3: allow the media graph to be used even if a subdev fails

Currently there's only a function to register device nodes for all subdevs of a v4l2 device that are marked with the V4L2_SUBDEV_FL_HAS_DEVNODE flag.

But drivers may want to register device nodes for individual subdevices, so add a v4l2_device_register_subdev_node() for this purpose.

A use case for this function is for media device drivers to register the device nodes in the v4l2 async notifier .bound callback instead of doing a registration for all subdevices in the .complete callback.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/v4l2-core/v4l2-device.c | 90 ++++++++++++++++-----------
 include/media/v4l2-device.h           | 10 +++
 2 files changed, 63 insertions(+), 37 deletions(-)

drivers/media/v4l2-core/v4l2-device.c +53 -37

index 3940e55c72f1..e5fc51b6604c 100644
@@ -222,9 +222,59 @@ static void v4l2_device_release_subdev_node(struct video_device *vdev)
 	kfree(vdev);
 }
 
-int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+int v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+				     struct v4l2_subdev *sd)
 {
 	struct video_device *vdev;
+	int err;
+
+	if (!(sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE))
+		return -EINVAL;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	video_set_drvdata(vdev, sd);
+	strlcpy(vdev->name, sd->name, sizeof(vdev->name));
+	vdev->v4l2_dev = v4l2_dev;
+	vdev->fops = &v4l2_subdev_fops;
+	vdev->release = v4l2_device_release_subdev_node;
+	vdev->ctrl_handler = sd->ctrl_handler;
+	err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
+				      sd->owner);
+	if (err < 0) {
+		kfree(vdev);
+		return err;
+	}
+	sd->devnode = vdev;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	sd->entity.info.dev.major = VIDEO_MAJOR;
+	sd->entity.info.dev.minor = vdev->minor;
+
+	/* Interface is created by __video_register_device() */
+	if (vdev->v4l2_dev->mdev) {
+		struct media_link *link;
+
+		link = media_create_intf_link(&sd->entity,
+					      &vdev->intf_devnode->intf,
+					      MEDIA_LNK_FL_ENABLED |
+					      MEDIA_LNK_FL_IMMUTABLE);
+		if (!link) {
+			err = -ENOMEM;
+			video_unregister_device(sd->devnode);
+			return err;
+		}
+	}
+#endif
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_node);
+
+int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+{
+
 	struct v4l2_subdev *sd;
 	int err;
 

@@ -238,43 +288,9 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		if (sd->devnode)
 			continue;
 
-		vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
-		if (!vdev) {
-			err = -ENOMEM;
-			goto clean_up;
-		}
-
-		video_set_drvdata(vdev, sd);
-		strlcpy(vdev->name, sd->name, sizeof(vdev->name));
-		vdev->v4l2_dev = v4l2_dev;
-		vdev->fops = &v4l2_subdev_fops;
-		vdev->release = v4l2_device_release_subdev_node;
-		vdev->ctrl_handler = sd->ctrl_handler;
-		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
-					      sd->owner);
-		if (err < 0) {
-			kfree(vdev);
+		err = v4l2_device_register_subdev_node(v4l2_dev, sd);
+		if (err)
 			goto clean_up;
-		}
-		sd->devnode = vdev;
-#if defined(CONFIG_MEDIA_CONTROLLER)
-		sd->entity.info.dev.major = VIDEO_MAJOR;
-		sd->entity.info.dev.minor = vdev->minor;
-
-		/* Interface is created by __video_register_device() */
-		if (vdev->v4l2_dev->mdev) {
-			struct media_link *link;
-
-			link = media_create_intf_link(&sd->entity,
-						      &vdev->intf_devnode->intf,
-						      MEDIA_LNK_FL_ENABLED |
-						      MEDIA_LNK_FL_IMMUTABLE);
-			if (!link) {
-				err = -ENOMEM;
-				goto clean_up;
-			}
-		}
-#endif
 	}
 	return 0;
 

include/media/v4l2-device.h +0 -0

index b330e4a08a6b..bf25418a1ad6 100644

include/media/v4l2-device.h +5 -0

+ * @sd: pointer to &struct v4l2_subdev
+ */
+int __must_check v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+						  struct v4l2_subdev *sd);
+
 /**
  * v4l2_device_register_subdev_nodes - Registers device nodes for all subdevs
  *	of the v4l2 device that are marked with

Hi Javier,

Quoted 9 lines · Javier Martinez Canillas

Currently there's only a function to register device nodes for all subdevs of a v4l2 device that are marked with the V4L2_SUBDEV_FL_HAS_DEVNODE flag.

But drivers may want to register device nodes for individual subdevices, so add a v4l2_device_register_subdev_node() for this purpose.

A use case for this function is for media device drivers to register the device nodes in the v4l2 async notifier .bound callback instead of doing a registration for all subdevices in the .complete callback.

Thanks for the set.

I've been doing some work to add events to MC; with Hans's property API set, assuming it could be used to tell the registration is complete, we have all bits for a complete solution.

As the driver is buggy and fails to work correctly in the case if not every sub-devices probes successfully, I see no reason to postpone applying the two patches now.

One more comment below. (No need to resend just for that IMO.)

Quoted 2 lines

Signed-off-by Javier Martinez Canillas <[email protected]>

Quoted 6 lines

+/**
+ * v4l2_device_register_subdev_node - Registers a device node for a subdev
+ * of the v4l2 device.
+ *
+ * @v4l2_dev: pointer to struct v4l2_device

struct -> &struct

include/media/v4l2-device.h int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, · line 200

Quoted 8 lines

+ * @sd: pointer to &struct v4l2_subdev
+ */

include/media/v4l2-device.h +3 -0

+int __must_check v4l2_device_register_subdev_node(struct v4l2_device *v4l2_dev,
+						  struct v4l2_subdev *sd);
+
 /**
  * v4l2_device_register_subdev_nodes - Registers device nodes for all subdevs
  *	of the v4l2 device that are marked with

Hello,

This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe.

Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback.

Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device.

Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that.

Best regards,
Javier

Javier Martinez Canillas (2):
[media] v4l: allow to register dev nodes for individual v4l2 subdevs
media: intel-ipu3: create pad links and register subdev nodes at bound
time

Hello,

This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe.

Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback.

Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device.

Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that.

Best regards,
Javier

Javier Martinez Canillas (2):
[media] v4l: allow to register dev nodes for individual v4l2 subdevs
media: intel-ipu3: create pad links and register subdev nodes at bound
time

 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 66 ++++++-----------
 drivers/media/v4l2-core/v4l2-device.c    | 90 ++++++++++++++----------
 include/media/v4l2-device.h              | 10 +++
 3 files changed, 85 insertions(+), 81 deletions(-)

Hi Javier,

Quoted 12 lines · Javier Martinez Canillas

Hello,

This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe.

Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback.

Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device.

Quoted 2 lines

Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that.

Sorry, I missed this series until you pointed to it on irc just now :-)

I have discussed this topic before with Sakari and Laurent. My main problem with this is how an application can discover that not everything is online? And which parts are offline?

Perhaps a car with 10 cameras can function with 9, but not with 8. How would userspace know?

I completely agree that we need to support these advanced scenarios (including what happens when a camera suddenly fails), but it is the userspace aspects for which I would like to see an RFC first before you can do these things.

Regards,

Hans

Quoted 15 lines

Best regards,
Javier

Javier Martinez Canillas (2):
[media] v4l: allow to register dev nodes for individual v4l2 subdevs
media: intel-ipu3: create pad links and register subdev nodes at bound
time

 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 66 ++++++-----------
 drivers/media/v4l2-core/v4l2-device.c    | 90 ++++++++++++++----------
 include/media/v4l2-device.h              | 10 +++
 3 files changed, 85 insertions(+), 81 deletions(-)

Em Thu, 27 Sep 2018 11:52:35 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 15 lines

Hi Javier,

Quoted 12 lines · Javier Martinez Canillas

Hello,

This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe.

Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback.

Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device.

Quoted 8 lines

Quoted 2 lines

Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that.

Sorry, I missed this series until you pointed to it on irc just now :-)

I have discussed this topic before with Sakari and Laurent. My main problem with this is how an application can discover that not everything is online? And which parts are offline?

Via the media controller? It should be possible for an application to see if a videonode is missing using it.

Quoted 2 lines

Perhaps a car with 10 cameras can function with 9, but not with 8. How would userspace know?

I guess this is not the only case where someone submitted a patch for a driver that would keep working if some device node registration fails.

It could be just déjà vu, but I have a vague sensation that I merged something similar to it in the past on another driver, but I can't remember any details.

Quoted 1 line

Quoted 3 lines

I completely agree that we need to support these advanced scenarios (including what happens when a camera suddenly fails), but it is the userspace aspects for which I would like to see an RFC first before you can do these things.

Dynamic runtime fails should likely rise some signal. Perhaps a sort of media controller event?

Quoted 22 lines

Regards,

Hans

Quoted 15 lines

Best regards,
Javier

Javier Martinez Canillas (2):
[media] v4l: allow to register dev nodes for individual v4l2 subdevs
media: intel-ipu3: create pad links and register subdev nodes at bound
time

 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 66 ++++++-----------
 drivers/media/v4l2-core/v4l2-device.c    | 90 ++++++++++++++----------
 include/media/v4l2-device.h              | 10 +++
 3 files changed, 85 insertions(+), 81 deletions(-)

Thanks,
Mauro

Quoted 44 lines · Mauro Carvalho Chehab

Em Thu, 27 Sep 2018 11:52:35 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 15 lines

Hi Javier,

Quoted 12 lines · Javier Martinez Canillas

Hello,

This series allows the ipu3-cio2 driver to properly expose a subset of the media graph even if some drivers for the pending subdevices fail to probe.

Currently the driver exposes a non-functional graph since the pad links are created and the subdev dev nodes are registered in the v4l2 async .complete callback. Instead, these operations should be done in the .bound callback.

Patch #1 just adds a v4l2_device_register_subdev_node() function to allow registering a single device node for a subdev of a v4l2 device.

Quoted 8 lines

Quoted 2 lines

Patch #2 moves the logic of the ipu3-cio2 .complete callback to the .bound callback. The .complete callback is just removed since is empy after that.

Sorry, I missed this series until you pointed to it on irc just now :-)

I have discussed this topic before with Sakari and Laurent. My main problem with this is how an application can discover that not everything is online? And which parts are offline?

Via the media controller? It should be possible for an application to see if a videonode is missing using it.

Quoted 2 lines

Perhaps a car with 10 cameras can function with 9, but not with 8. How would userspace know?

I guess this is not the only case where someone submitted a patch for a driver that would keep working if some device node registration fails.

It could be just déjà vu, but I have a vague sensation that I merged something similar to it in the past on another driver, but I can't remember any details.

Quoted 4 lines

I completely agree that we need to support these advanced scenarios (including what happens when a camera suddenly fails), but it is the userspace aspects for which I would like to see an RFC first before you can do these things.

Quoted 2 lines

Dynamic runtime fails should likely rise some signal. Perhaps a sort of media controller event?

See this old discussion: https://patchwork.kernel.org/patch/9849317/

My point is that someone needs to think about this and make a proposal. There may well be a simple approach, but it needs to be specced first.

1) What about adding time-out support? Today we wait forever until all components are found, but wouldn't it make sense to optionally time-out? And if so, then we can keep most of the code the same and decide in the complete() callback whether or not we accept missing components. And decide how badly 'impaired' the system is. We can also still bring up all the devices in the complete rather than one-by-one as you proposed (and which I am not sure I like).

2) This can be hard to test, so perhaps we should support some form of error injection to easily test what happens if something doesn't come up.