Hi Niklas, Laurent,
On 7/3/19 10:17 AM, Niklas Söderlund wrote:
Hi Shauah, Laurent,
On 2019-06-30 14:41:02 +0300, Laurent Pinchart wrote:
Hi Shuah,
On Fri, Jun 28, 2019 at 10:41:07AM -0600, Shuah Khan wrote:
On 6/16/19 12:45 PM, Laurent Pinchart wrote:
On Fri, Jun 14, 2019 at 05:26:46PM -0600, Shuah Khan wrote:
On 6/13/19 7:24 AM, Helen Koike wrote:
On 6/13/19 2:44 AM, Hans Verkuil wrote:
On 5/24/19 5:31 AM, Shuah Khan wrote:
media_device is embedded in struct vimc_device and when vimc is removed
vimc_device and the embedded media_device goes with it, while the active
stream and vimc_capture continue to access it.
Fix the media_device lifetime problem by changing vimc to create shared
media_device using Media Device Allocator API and vimc_capture getting
a reference to vimc module. With this change, vimc module can be removed
only when the references are gone. vimc can be removed after vimc_capture
is removed.
Media Device Allocator API supports just USB devices. Enhance it
adding a genetic device allocate interface to support other media
drivers.
The new interface takes pointer to struct device instead and creates
media device. This interface allows a group of drivers that have a
common root device to share media device resource and ensure media
device doesn't get deleted as long as one of the drivers holds its
reference.
The new interface has been tested with vimc component driver to fix
panics when vimc module is removed while streaming is in progress.
Helen, can you review this series? I'm not sure this is the right approach
for a driver like vimc, and even if it is, then it is odd that vimc-capture
is the only vimc module that's handled here.
Hi Hans,
Yes, I can take a look. Sorry, I've been a bit busy these days but I'll
try to take a look at this patch series (and the others) asap.
Helen
My gut feeling is that this should be handled inside vimc directly and not
using the media-dev-allocator.
Hi Hans and Helen,
I explored fixing the problem within vimc before I went down the path to
use Media Device Allocator API. I do think that it is cleaner to go this
way and easier to maintain.
vimc is a group pf component drivers that rely on the media device vimc
in vimc and falls into the use-case Media Device Allocator API is added
to address. The release and life-time management happens without vimc
component drivers being changed other than using the API to get and put
media device reference.
Our replies crossed each other, please see my reply to Hans. I would
just like to comment here that if having multiple kernel modules causes
issue, they can all be merged together. There's no need for vimc to be
handled through multiple modules (I actually think it's quite
counterproductive, it only makes it more complex, for no added value).
There are several problems in this group of drivers as far as lifetime
management is concerned. I explained some of it in the patch 2/2
If vimc module is removed while streaming is active, vimc_exit runs
into NULL pointer dereference error when streaming thread tries to
access and lock graph_mutex in the struct media_device.
The primary reason for this is that:
media_device is embedded in struct vimc_device and when vimc is removed
vimc_device and the embedded media_device goes with it, while the active
stream and vimc_capture continue to access it.
The issue isn't so much that media_devic is embedded in vimc_device, but
that vimc_device is released too early. Not only does the thread need to
access the graph_mutex lock in the media_device structure, but it can
potentially access fields of the device-specific structures as well. The
proper solution is to propagate media_device_release() one level up, in
order to only release the top-level structure containing media_device
when the last reference to the media_device is dropped.
Yes. vimc_device is the master device for all the component drivers and
it being released early definitely causes problems. I tried to solve
this by isolating the media_device embedded in it and taking that out
of contention for release later. This problem could be solved by making
sure vimc_device sticks around and I am on that solution now.
I have seen similar problems with rcar-vin, the device specific data is
released to early. In my case it was not triggered by the struct
media_device but with a struct v4l2_device embedded in the device
specific data IIRC.
This was when I tried to address the lifetime issues of the video device
when binding/unbinding the device to the driver and not when unloading
the module. This was quiet a while ago so I don't recall specifics,
sorry about that. One finding was that there are also unsolved problems
when it comes async notifiers and unloading/unbinding and then
loading/binding subdevices as well as the driver controlling the video
device. It was such a mess I gave up.
Yes. You will find such problems with various media drivers. It could be
the v4l2 device or some other device that gets released while still in
use.
I'm happy to see activity in this area but I fear it might need work on
a higher level and not trying to work around the problem in drivers.
Drivers still need to handle such issues anyway. Is there a reason why
you think it is a work-around?
If we chose to keep these drivers as component drivers, media device
needs to stick around until all components stop using it. This is tricky
because there is no tie between these set of drivers. vimc module can
be deleted while others are still active. As vimc gets removed, other
component drivers start wanting to access the media device tree.
Reference-counting is the key.
This is classic media device lifetime problem which could be solved
easily with the way I solved it with this series. I saw this as a
variation on the same use-case we had with sound and media drivers
sharing the media device.
This isn't about solving it easily, it's about solving it properly. The
media device allocator as used here is a hack and takes us in the
opposite direction of a proper fix.
Labeling this hack doesn't accurate. I agree though that this might be a
big hammer and there might be other solutions that can be limited to
just vimc scope. :)
I have a TODO request from you asking to extend Media Device Allocator
API to generic case and not restrict it to USB devices. My thinking is
that this gives a perfect test case to extend the API to be generic
and use to solve this problem.
The biggest issue at the moment with the media device allocator, which I
have pointed out numerous times and has never been addressed (and which
explains why I didn't think the code was ready to be merged) is that the
media_device contains operations that are based on having a single
driver controlling the media device. A proper shared media device
allocator needs to drop the concept of a single master for the media
device, and thus needs to refactor those operations to allow any user of
the media device to implement them (the .link_notify() operation is a
prime example, and the recently added request operations will make this
even more challenging - think of how this patch series would prevent
vimc from properly implementing the request API). As long as these issue
are not fixed I will be firmly opposed to spreading the usage of the
media device allocator beyond what exists today.
During the reviews, it was deemed necessary to make media driver as the
master for creating parts of the tree and provide hooks for other
drivers to add their own media components to the tree. The same is
extended to other interfaces. This feature was on ice for so long,
I don't recall all the details on how it evolved.
thanks,
-- Shuah
