mbox series

[0/5] media-device: Report if graph is complete

Message ID 20200610230541.1603067-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
Headers show
Series media-device: Report if graph is complete | expand

Message

Niklas Söderlund June 10, 2020, 11:05 p.m. UTC
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(-)

Comments

Hans Verkuil June 12, 2020, 10:45 a.m. UTC | #1
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(-)
>