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(-)
--
2.17.1
The driver create the pad links and registers the device nodes for bound
subdevices in the v4l2 async notififer .complete callback. But that will
prevent the media graph to be usable if for example one of the drivers
for a subdevice fails to probe.
In that case, the media entity will be registered but there will be not
pad links created nor the subdev device node will be registered.
So do these operations in the .bound callback instead of doing it at
.complete time.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 66 ++++++++----------------
1 file changed, 22 insertions(+), 44 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 29027159eced..4eb80b690e3f 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1403,6 +1403,8 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
struct sensor_async_subdev *s_asd = container_of(asd,
struct sensor_async_subdev, asd);
struct cio2_queue *q;
+ unsigned int pad;
+ int ret;
if (cio2->queue[s_asd->csi2.port].sensor)
return -EBUSY;
@@ -1413,7 +1415,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(&cio2->pci_dev->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(&cio2->pci_dev->dev, "failed to create link for %s\n",
+ q->sensor->name);
+ return ret;
+ }
+
+ return v4l2_device_register_subdev_node(&cio2->v4l2_dev, sd);
}
/* The .unbind callback */
@@ -1429,52 +1450,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 = container_of(notifier, struct cio2_device,
- notifier);
- struct sensor_async_subdev *s_asd;
- struct cio2_queue *q;
- unsigned int i, pad;
- int ret;
-
- for (i = 0; i < notifier->num_subdevs; i++) {
- s_asd = container_of(cio2->notifier.subdevs[i],
- struct sensor_async_subdev, 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(&cio2->pci_dev->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(&cio2->pci_dev->dev,
- "failed to create link for %s\n",
- cio2->queue[i].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_fwnode_parse(struct device *dev,
--
2.17.1
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(-)
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 3940e55c72f1..e5fc51b6604c 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -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;
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index b330e4a08a6b..bf25418a1ad6 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -185,6 +185,16 @@ int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
*/
void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
+/**
+ * v4l2_device_register_subdev_node - Registers a device node for a subdev
+ * of the v4l2 device.
+ *
+ * @v4l2_dev: pointer to struct v4l2_device
+ * @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
--
2.17.1
Hi Tianshu and Sakari,
On 9/4/18 1:30 PM, Javier Martinez Canillas wrote:
> 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
>
Any comments about these patches?
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Hi Javier,
On Tue, Sep 04, 2018 at 01:30:17PM +0200, Javier Martinez Canillas wrote:
> 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.)
>
> 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(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 3940e55c72f1..e5fc51b6604c 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -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;
>
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index b330e4a08a6b..bf25418a1ad6 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -185,6 +185,16 @@ int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
> */
> void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
>
> +/**
> + * 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
> + * @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
--
Regards,
Sakari Ailus
[email protected]
Hi Sakari,
On 9/17/18 6:46 PM, Sakari Ailus wrote:
> Hi Javier,
>
> On Tue, Sep 04, 2018 at 01:30:17PM +0200, Javier Martinez Canillas wrote:
>> 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.
>
Great.
> 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.
>
Yes, agreed.
> One more comment below. (No need to resend just for that IMO.)
>
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
[snip]
>>
>> +/**
>> + * 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
>
Thanks, I'm not well versed in kernel-doc / Sphinx markup syntax so I missed it.
BTW, I copied from another place in include/media/v4l2-device.h, and now I
notice that it has a mix of "&struct foo", "struct &foo" and "struct foo".
It would be nice to fix this so cross-reference works properly in all cases.
>> + * @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
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
Hi Javier,
On 09/04/2018 01:30 PM, Javier Martinez Canillas wrote:
> 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.
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
>
> 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:
> Hi Javier,
>
> On 09/04/2018 01:30 PM, Javier Martinez Canillas wrote:
> > 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.
>
> 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.
> 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.
>
> 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?
>
> Regards,
>
> Hans
>
> >
> > 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
On 09/27/2018 12:13 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 27 Sep 2018 11:52:35 +0200
> Hans Verkuil <[email protected]> escreveu:
>
>> Hi Javier,
>>
>> On 09/04/2018 01:30 PM, Javier Martinez Canillas wrote:
>>> 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.
>>
>> 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.
>
>> 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.
>
>>
>> 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?
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.
Regards,
Hans
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> 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
>
Hi Hans,
On Thu, Sep 27, 2018 at 7:22 PM Hans Verkuil <[email protected]> wrote:
>
> On 09/27/2018 12:13 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 27 Sep 2018 11:52:35 +0200
> > Hans Verkuil <[email protected]> escreveu:
> >
> >> Hi Javier,
> >>
> >> On 09/04/2018 01:30 PM, Javier Martinez Canillas wrote:
> >>> 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.
> >>
> >> 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.
> >
> >> 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.
> >
> >>
> >> 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?
>
> 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.
In that thread, you seem to have mentioned that having a Kconfig
option, disabled by default, to allow registering an incomplete media
topology would be an acceptable option. Do you think we could revive
that idea?
Quoting some of the discussion points you mentioned:
Some discussion points:
> 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).
It sounds like an interesting extension, not a must have for handling
incomplete topologies.
I can also imagine the timeout handling introducing a lot of confusion
to the userspace, for example, with a long timeout, the whole
initialization would have to wait for the timeout to elapse, which for
a smartphone user could mean that they can't start the camera
application (or it times out on its own and fails) until then.
> 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.
This is a very good point, but I think we should be able to do away
with something simpler, just blacklist particular device from being
bound to a driver. How to do it, is another question, though... (I can
imagine binding a dummy driver to the device as an example solution.)
Best regards,
Tomasz