linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] media-device: Report if graph is complete
@ 2020-06-10 23:05 Niklas Söderlund
  2020-06-10 23:05 ` [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology Niklas Söderlund
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Hi,

This series is an attempt to scratch an old itch, it's problematic to
support unbind and then a second call to complete in v4l2-async. When
the second complete call happens a lot of things can go wrong.

When v4l2-async complete callback is called multiple video devices are
registered with video_register_device(). Then if a v4l2-async unbind
happens they are unregistered with video_unregister_device().

Their are multiple problems with this, specially for R-Car VIN.

1. Depending on which subdevice is unbound parts of the video pipeline
   can still function. There are for example two CSI-2 receivers
   connected to two different CSI-2 buses in the pipeline. If one of the
   receivers are unbound the other can still function perfectly well.
   But with the current setup everything goes away, this is bad for
   operational safety.

2. The struct video_device contains a static struct device, which in
   turn contains a static struct kref. When the kref is release by
   calling video_unregister_device() and then later trying to
   re-register the video device video_register_device() the kref life
   time management kicks in and produces warnings in later kernels or
   OOPS in older ones.

It has been discussed in the past at various conferences that it could
be OK to not video_unregister_device() if a v4l2-async unbind happens.
The argument against it was that user-space needed a way to check if a
pipeline was completely probed or not. And this used to be that the
video devices where only present if everything was available.

It was agreed in principle that if an alternate way for media controller
centric devices could be found to inform user-space of this fact could
be found it would be OK to not unregister video devices in case of an
unbind or even allow registering the video devices at probe time instead
of at v4l2-async complete time.

This series aims to provide such a mechanism using the media device
itself to report if the media graph is complete or not.

Niklas Söderlund (5):
  uapi/linux/media.h: add flags field to struct media_v2_topology
  media-device: Add a complete flag to struct media_device
  v4l2-async: Flag when media graph is complete
  mc-device.c: Report graph complete to user-space
  rcar-vin: Do not unregister video device when a subdevice is unbound

 drivers/media/mc/mc-device.c                | 2 +-
 drivers/media/platform/rcar-vin/rcar-core.c | 5 -----
 drivers/media/v4l2-core/v4l2-async.c        | 5 +++++
 include/media/media-device.h                | 2 ++
 include/uapi/linux/media.h                  | 4 +++-
 5 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
@ 2020-06-10 23:05 ` Niklas Söderlund
  2020-06-11  3:12   ` Laurent Pinchart
  2020-06-10 23:05 ` [PATCH 2/5] media-device: Add a complete flag to struct media_device Niklas Söderlund
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Add a flags field to the media_v2_topology structure by taking one
of the reserved u32 fields. Also define a flag to carry information
about if the graph is complete. The use-case is to have a way to report
to user-space if the media graph contains all subdevices.

The other bits in the flags field are unused for now, but could be
claimed to carry other type of information in the future.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/mc/mc-device.c | 2 +-
 include/uapi/linux/media.h   | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index da8088351135298a..c2ef5bb512a5fba0 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -242,6 +242,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 	int ret = 0;
 
 	topo->topology_version = mdev->topology_version;
+	topo->flags = 0;
 
 	/* Get entities and number of entities */
 	i = 0;
@@ -269,7 +270,6 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 		uentity++;
 	}
 	topo->num_entities = i;
-	topo->reserved1 = 0;
 
 	/* Get interfaces and number of interfaces */
 	i = 0;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 383ac7b7d8f07eca..7c07b9939252c768 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -351,7 +351,7 @@ struct media_v2_topology {
 	__u64 topology_version;
 
 	__u32 num_entities;
-	__u32 reserved1;
+	__u32 flags;
 	__u64 ptr_entities;
 
 	__u32 num_interfaces;
@@ -367,6 +367,8 @@ struct media_v2_topology {
 	__u64 ptr_links;
 } __attribute__ ((packed));
 
+#define MEDIA_TOPOLOGY_FLAG_COMPLETE	(1 << 0)
+
 /* ioctls */
 
 #define MEDIA_IOC_DEVICE_INFO	_IOWR('|', 0x00, struct media_device_info)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] media-device: Add a complete flag to struct media_device
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
  2020-06-10 23:05 ` [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology Niklas Söderlund
@ 2020-06-10 23:05 ` Niklas Söderlund
  2020-06-11  3:13   ` Laurent Pinchart
  2020-06-10 23:05 ` [PATCH 3/5] v4l2-async: Flag when media graph is complete Niklas Söderlund
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Add a complete flag to indicate if the media graph is complete or not.
The use-case is for v4l2-async to set the flag when all subdevices are
bound and that the flag be reported to user-space so it can learn when a
graph is completely populated.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/media/media-device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/media/media-device.h b/include/media/media-device.h
index fa089543072052cf..cd685c3a791c6c04 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -83,6 +83,7 @@ struct media_device_ops {
  * @serial:	Device serial number (optional)
  * @bus_info:	Unique and stable device location identifier
  * @hw_revision: Hardware device revision
+ * @complete:	Graph completed flag
  * @topology_version: Monotonic counter for storing the version of the graph
  *		topology. Should be incremented each time the topology changes.
  * @id:		Unique ID used on the last registered graph object
@@ -151,6 +152,7 @@ struct media_device {
 	char serial[40];
 	char bus_info[32];
 	u32 hw_revision;
+	bool complete;
 
 	u64 topology_version;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] v4l2-async: Flag when media graph is complete
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
  2020-06-10 23:05 ` [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology Niklas Söderlund
  2020-06-10 23:05 ` [PATCH 2/5] media-device: Add a complete flag to struct media_device Niklas Söderlund
@ 2020-06-10 23:05 ` Niklas Söderlund
  2020-06-11  3:10   ` Laurent Pinchart
  2020-06-11 10:50   ` Sakari Ailus
  2020-06-10 23:05 ` [PATCH 4/5] mc-device.c: Report graph complete to user-space Niklas Söderlund
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

When the notifier completes set the complete flag in the struct
media_device. This flag can can then be reported to user-space to let it
know the graph is complete.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 8bde33c21ce45f98..331594ca5b3bb723 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -217,6 +217,11 @@ v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
 	if (!v4l2_async_notifier_can_complete(notifier))
 		return 0;
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (notifier->v4l2_dev->mdev)
+		notifier->v4l2_dev->mdev->complete = true;
+#endif
+
 	return v4l2_async_notifier_call_complete(notifier);
 }
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] mc-device.c: Report graph complete to user-space
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
                   ` (2 preceding siblings ...)
  2020-06-10 23:05 ` [PATCH 3/5] v4l2-async: Flag when media graph is complete Niklas Söderlund
@ 2020-06-10 23:05 ` Niklas Söderlund
  2020-06-11  3:14   ` Laurent Pinchart
  2020-06-10 23:05 ` [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound Niklas Söderlund
  2020-06-12 10:45 ` [PATCH 0/5] media-device: Report if graph is complete Hans Verkuil
  5 siblings, 1 reply; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

If the graph in the media device is complete report it to userspace by
setting the MEDIA_TOPOLOGY_FLAG_COMPLETE flag.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/mc/mc-device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c2ef5bb512a5fba0..d63792cc014279fc 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -242,7 +242,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 	int ret = 0;
 
 	topo->topology_version = mdev->topology_version;
-	topo->flags = 0;
+	topo->flags = mdev->complete ? MEDIA_TOPOLOGY_FLAG_COMPLETE : 0;
 
 	/* Get entities and number of entities */
 	i = 0;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
                   ` (3 preceding siblings ...)
  2020-06-10 23:05 ` [PATCH 4/5] mc-device.c: Report graph complete to user-space Niklas Söderlund
@ 2020-06-10 23:05 ` Niklas Söderlund
  2020-06-11  8:56   ` Sergei Shtylyov
  2020-06-12 10:45 ` [PATCH 0/5] media-device: Report if graph is complete Hans Verkuil
  5 siblings, 1 reply; 15+ messages in thread
From: Niklas Söderlund @ 2020-06-10 23:05 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

If the v4l2-async notifier have once been complete and the video
device(s) have been register, do not unregister them if one subdevice is
unbound. Depending on which subdevice is unbound other parts of the
pipeline could still be functional. For example if one of multiple
sensors connected to a CSI-2 transmitter is unbound other sensors in
that pipeline are still useable.

This problem is extra critical for R-Car VIN which registers two
independent CSI-2 receivers in the same media graph as they can both be
used by the same dma-engines. If one of the CSI-2 receivers are unbound
the other CSI-2 receiver pipeline is still fully functional.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c8965d27e64f..6b0f13618556cac4 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -525,7 +525,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 
 static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
-	rvin_v4l2_unregister(vin);
 	vin->parallel->subdev = NULL;
 
 	if (!vin->info->use_mc) {
@@ -747,10 +746,6 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
-	for (i = 0; i < RCAR_VIN_NUM; i++)
-		if (vin->group->vin[i])
-			rvin_v4l2_unregister(vin->group->vin[i]);
-
 	mutex_lock(&vin->group->lock);
 
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] v4l2-async: Flag when media graph is complete
  2020-06-10 23:05 ` [PATCH 3/5] v4l2-async: Flag when media graph is complete Niklas Söderlund
@ 2020-06-11  3:10   ` Laurent Pinchart
  2020-06-11 10:50   ` Sakari Ailus
  1 sibling, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2020-06-11  3:10 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Hans Verkuil, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Jun 11, 2020 at 01:05:39AM +0200, Niklas Söderlund wrote:
> When the notifier completes set the complete flag in the struct
> media_device. This flag can can then be reported to user-space to let it
> know the graph is complete.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce45f98..331594ca5b3bb723 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -217,6 +217,11 @@ v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
>  	if (!v4l2_async_notifier_can_complete(notifier))
>  		return 0;
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (notifier->v4l2_dev->mdev)
> +		notifier->v4l2_dev->mdev->complete = true;
> +#endif

Does this work with sub-notifiers ?

> +
>  	return v4l2_async_notifier_call_complete(notifier);

Isn't there a race here, if we report the complete flag before the
notifier calls the .complete() operation ?

>  }
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology
  2020-06-10 23:05 ` [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology Niklas Söderlund
@ 2020-06-11  3:12   ` Laurent Pinchart
  2020-06-11 10:51     ` Sakari Ailus
  0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2020-06-11  3:12 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Hans Verkuil, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Jun 11, 2020 at 01:05:37AM +0200, Niklas Söderlund wrote:
> Add a flags field to the media_v2_topology structure by taking one
> of the reserved u32 fields. Also define a flag to carry information
> about if the graph is complete. The use-case is to have a way to report
> to user-space if the media graph contains all subdevices.
> 
> The other bits in the flags field are unused for now, but could be
> claimed to carry other type of information in the future.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/mc/mc-device.c | 2 +-
>  include/uapi/linux/media.h   | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index da8088351135298a..c2ef5bb512a5fba0 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -242,6 +242,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
>  	int ret = 0;
>  
>  	topo->topology_version = mdev->topology_version;
> +	topo->flags = 0;
>  
>  	/* Get entities and number of entities */
>  	i = 0;
> @@ -269,7 +270,6 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
>  		uentity++;
>  	}
>  	topo->num_entities = i;
> -	topo->reserved1 = 0;
>  
>  	/* Get interfaces and number of interfaces */
>  	i = 0;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 383ac7b7d8f07eca..7c07b9939252c768 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -351,7 +351,7 @@ struct media_v2_topology {
>  	__u64 topology_version;
>  
>  	__u32 num_entities;
> -	__u32 reserved1;
> +	__u32 flags;
>  	__u64 ptr_entities;
>  
>  	__u32 num_interfaces;
> @@ -367,6 +367,8 @@ struct media_v2_topology {
>  	__u64 ptr_links;
>  } __attribute__ ((packed));
>  
> +#define MEDIA_TOPOLOGY_FLAG_COMPLETE	(1 << 0)
> +

Missing documentation :-)

Should we use the BIT() macro ?

>  /* ioctls */
>  
>  #define MEDIA_IOC_DEVICE_INFO	_IOWR('|', 0x00, struct media_device_info)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] media-device: Add a complete flag to struct media_device
  2020-06-10 23:05 ` [PATCH 2/5] media-device: Add a complete flag to struct media_device Niklas Söderlund
@ 2020-06-11  3:13   ` Laurent Pinchart
  0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2020-06-11  3:13 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Hans Verkuil, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Jun 11, 2020 at 01:05:38AM +0200, Niklas Söderlund wrote:
> Add a complete flag to indicate if the media graph is complete or not.
> The use-case is for v4l2-async to set the flag when all subdevices are
> bound and that the flag be reported to user-space so it can learn when a
> graph is completely populated.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  include/media/media-device.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index fa089543072052cf..cd685c3a791c6c04 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -83,6 +83,7 @@ struct media_device_ops {
>   * @serial:	Device serial number (optional)
>   * @bus_info:	Unique and stable device location identifier
>   * @hw_revision: Hardware device revision
> + * @complete:	Graph completed flag

This requires more documentation too.

>   * @topology_version: Monotonic counter for storing the version of the graph
>   *		topology. Should be incremented each time the topology changes.
>   * @id:		Unique ID used on the last registered graph object
> @@ -151,6 +152,7 @@ struct media_device {
>  	char serial[40];
>  	char bus_info[32];
>  	u32 hw_revision;
> +	bool complete;
>  
>  	u64 topology_version;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] mc-device.c: Report graph complete to user-space
  2020-06-10 23:05 ` [PATCH 4/5] mc-device.c: Report graph complete to user-space Niklas Söderlund
@ 2020-06-11  3:14   ` Laurent Pinchart
  0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2020-06-11  3:14 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Hans Verkuil, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, Jun 11, 2020 at 01:05:40AM +0200, Niklas Söderlund wrote:
> If the graph in the media device is complete report it to userspace by
> setting the MEDIA_TOPOLOGY_FLAG_COMPLETE flag.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/mc/mc-device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index c2ef5bb512a5fba0..d63792cc014279fc 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -242,7 +242,7 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
>  	int ret = 0;
>  
>  	topo->topology_version = mdev->topology_version;
> -	topo->flags = 0;
> +	topo->flags = mdev->complete ? MEDIA_TOPOLOGY_FLAG_COMPLETE : 0;
>  
>  	/* Get entities and number of entities */
>  	i = 0;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound
  2020-06-10 23:05 ` [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound Niklas Söderlund
@ 2020-06-11  8:56   ` Sergei Shtylyov
  0 siblings, 0 replies; 15+ messages in thread
From: Sergei Shtylyov @ 2020-06-11  8:56 UTC (permalink / raw)
  To: Niklas Söderlund, Hans Verkuil, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

Hello!

On 11.06.2020 2:05, Niklas Söderlund wrote:

> If the v4l2-async notifier have once been complete and the video
> device(s) have been register, do not unregister them if one subdevice is

    Registered.

> unbound. Depending on which subdevice is unbound other parts of the
> pipeline could still be functional. For example if one of multiple
> sensors connected to a CSI-2 transmitter is unbound other sensors in
> that pipeline are still useable.

    Usable.

> This problem is extra critical for R-Car VIN which registers two
> independent CSI-2 receivers in the same media graph as they can both be
> used by the same dma-engines. If one of the CSI-2 receivers are unbound

    DMA engines, maybe?

> the other CSI-2 receiver pipeline is still fully functional.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] v4l2-async: Flag when media graph is complete
  2020-06-10 23:05 ` [PATCH 3/5] v4l2-async: Flag when media graph is complete Niklas Söderlund
  2020-06-11  3:10   ` Laurent Pinchart
@ 2020-06-11 10:50   ` Sakari Ailus
  2020-06-11 10:52     ` Sakari Ailus
  1 sibling, 1 reply; 15+ messages in thread
From: Sakari Ailus @ 2020-06-11 10:50 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Hans Verkuil, Laurent Pinchart, linux-media, linux-renesas-soc

Hi Niklas,

On Thu, Jun 11, 2020 at 01:05:39AM +0200, Niklas Söderlund wrote:
> When the notifier completes set the complete flag in the struct
> media_device. This flag can can then be reported to user-space to let it
> know the graph is complete.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce45f98..331594ca5b3bb723 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -217,6 +217,11 @@ v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
>  	if (!v4l2_async_notifier_can_complete(notifier))
>  		return 0;
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (notifier->v4l2_dev->mdev)
> +		notifier->v4l2_dev->mdev->complete = true;
> +#endif
> +
>  	return v4l2_async_notifier_call_complete(notifier);
>  }
>  

Do we need this? Could you not use the complete callback from the main
notifier, that gets called only when all async subdevs have been bound?

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology
  2020-06-11  3:12   ` Laurent Pinchart
@ 2020-06-11 10:51     ` Sakari Ailus
  0 siblings, 0 replies; 15+ messages in thread
From: Sakari Ailus @ 2020-06-11 10:51 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Niklas Söderlund, Hans Verkuil, linux-media, linux-renesas-soc

On Thu, Jun 11, 2020 at 06:12:26AM +0300, Laurent Pinchart wrote:
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index 383ac7b7d8f07eca..7c07b9939252c768 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -351,7 +351,7 @@ struct media_v2_topology {
> >  	__u64 topology_version;
> >  
> >  	__u32 num_entities;
> > -	__u32 reserved1;
> > +	__u32 flags;
> >  	__u64 ptr_entities;
> >  
> >  	__u32 num_interfaces;
> > @@ -367,6 +367,8 @@ struct media_v2_topology {
> >  	__u64 ptr_links;
> >  } __attribute__ ((packed));
> >  
> > +#define MEDIA_TOPOLOGY_FLAG_COMPLETE	(1 << 0)
> > +
> 
> Missing documentation :-)
> 
> Should we use the BIT() macro ?

No, it's uAPI.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] v4l2-async: Flag when media graph is complete
  2020-06-11 10:50   ` Sakari Ailus
@ 2020-06-11 10:52     ` Sakari Ailus
  0 siblings, 0 replies; 15+ messages in thread
From: Sakari Ailus @ 2020-06-11 10:52 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Hans Verkuil, Laurent Pinchart, linux-media, linux-renesas-soc

On Thu, Jun 11, 2020 at 01:50:19PM +0300, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Thu, Jun 11, 2020 at 01:05:39AM +0200, Niklas Söderlund wrote:
> > When the notifier completes set the complete flag in the struct
> > media_device. This flag can can then be reported to user-space to let it
> > know the graph is complete.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce45f98..331594ca5b3bb723 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -217,6 +217,11 @@ v4l2_async_notifier_try_complete(struct v4l2_async_notifier *notifier)
> >  	if (!v4l2_async_notifier_can_complete(notifier))
> >  		return 0;
> >  
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	if (notifier->v4l2_dev->mdev)
> > +		notifier->v4l2_dev->mdev->complete = true;
> > +#endif
> > +
> >  	return v4l2_async_notifier_call_complete(notifier);
> >  }
> >  
> 
> Do we need this? Could you not use the complete callback from the main
> notifier, that gets called only when all async subdevs have been bound?

Ah. Please ignore the comment. Something like this is needed for the flag.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] media-device: Report if graph is complete
  2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
                   ` (4 preceding siblings ...)
  2020-06-10 23:05 ` [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound Niklas Söderlund
@ 2020-06-12 10:45 ` Hans Verkuil
  5 siblings, 0 replies; 15+ messages in thread
From: Hans Verkuil @ 2020-06-12 10:45 UTC (permalink / raw)
  To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc

Hi Niklas,

On 11/06/2020 01:05, Niklas Söderlund wrote:
> Hi,
> 
> This series is an attempt to scratch an old itch, it's problematic to
> support unbind and then a second call to complete in v4l2-async. When
> the second complete call happens a lot of things can go wrong.
> 
> When v4l2-async complete callback is called multiple video devices are
> registered with video_register_device(). Then if a v4l2-async unbind
> happens they are unregistered with video_unregister_device().
> 
> Their are multiple problems with this, specially for R-Car VIN.
> 
> 1. Depending on which subdevice is unbound parts of the video pipeline
>    can still function. There are for example two CSI-2 receivers
>    connected to two different CSI-2 buses in the pipeline. If one of the
>    receivers are unbound the other can still function perfectly well.
>    But with the current setup everything goes away, this is bad for
>    operational safety.

But even with this series, the R-Car VIN will still wait at boot time until
the graph is completed before registering the devices, right?

So this doesn't solve the case where e.g. one of the two CSI-2 receivers
is broken or a sensor is broken, but you still want to be able to work
with the remaining pipeline.

> 
> 2. The struct video_device contains a static struct device, which in
>    turn contains a static struct kref. When the kref is release by
>    calling video_unregister_device() and then later trying to
>    re-register the video device video_register_device() the kref life
>    time management kicks in and produces warnings in later kernels or
>    OOPS in older ones.

This is a bug in the driver or v4l2 core code (I would need to know the
details first). And this should be fixed rather than basically papering
over it.

I think this relates to this kobject warnings:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg117573.html

> 
> It has been discussed in the past at various conferences that it could
> be OK to not video_unregister_device() if a v4l2-async unbind happens.
> The argument against it was that user-space needed a way to check if a
> pipeline was completely probed or not. And this used to be that the
> video devices where only present if everything was available.
> 
> It was agreed in principle that if an alternate way for media controller
> centric devices could be found to inform user-space of this fact could
> be found it would be OK to not unregister video devices in case of an
> unbind or even allow registering the video devices at probe time instead
> of at v4l2-async complete time.
> 
> This series aims to provide such a mechanism using the media device
> itself to report if the media graph is complete or not.

This series really addresses only a small corner case of a much larger
issue: what to do if for some reason only part of the media topology
comes up or if a part disappears/breaks during operation?

This larger issue requires a proper RFC.

It may well be that this complete flag is still needed when you look at
the big picture, but in this series it feels very much like a hack.

Regards,

	Hans

> 
> Niklas Söderlund (5):
>   uapi/linux/media.h: add flags field to struct media_v2_topology
>   media-device: Add a complete flag to struct media_device
>   v4l2-async: Flag when media graph is complete
>   mc-device.c: Report graph complete to user-space
>   rcar-vin: Do not unregister video device when a subdevice is unbound
> 
>  drivers/media/mc/mc-device.c                | 2 +-
>  drivers/media/platform/rcar-vin/rcar-core.c | 5 -----
>  drivers/media/v4l2-core/v4l2-async.c        | 5 +++++
>  include/media/media-device.h                | 2 ++
>  include/uapi/linux/media.h                  | 4 +++-
>  5 files changed, 11 insertions(+), 7 deletions(-)
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-06-12 10:45 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 23:05 [PATCH 0/5] media-device: Report if graph is complete Niklas Söderlund
2020-06-10 23:05 ` [PATCH 1/5] uapi/linux/media.h: add flags field to struct media_v2_topology Niklas Söderlund
2020-06-11  3:12   ` Laurent Pinchart
2020-06-11 10:51     ` Sakari Ailus
2020-06-10 23:05 ` [PATCH 2/5] media-device: Add a complete flag to struct media_device Niklas Söderlund
2020-06-11  3:13   ` Laurent Pinchart
2020-06-10 23:05 ` [PATCH 3/5] v4l2-async: Flag when media graph is complete Niklas Söderlund
2020-06-11  3:10   ` Laurent Pinchart
2020-06-11 10:50   ` Sakari Ailus
2020-06-11 10:52     ` Sakari Ailus
2020-06-10 23:05 ` [PATCH 4/5] mc-device.c: Report graph complete to user-space Niklas Söderlund
2020-06-11  3:14   ` Laurent Pinchart
2020-06-10 23:05 ` [PATCH 5/5] rcar-vin: Do not unregister video device when a subdevice is unbound Niklas Söderlund
2020-06-11  8:56   ` Sergei Shtylyov
2020-06-12 10:45 ` [PATCH 0/5] media-device: Report if graph is complete Hans Verkuil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).