<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
shuah: pinchartl, mchehab: Would you like to see separate patch for each driver to update it to Media Dev Allocator API - remember we decided that we should update all drivers to mt embed media device
<br> 15 drivers need updating
pinchartl: <u>shuah</u>: I'm fine with both
shuah: okay - personally I like individual patches - easier to review and isolate problems
mchehab: shuah, patches should not break bissectability
<br> also, I want to see bug fixes first, as they need to be c/c stable
shuah: <u>mchehab</u>: okay I will keep not breaking bisect in mind. Are there any specific fixes you are expecting?
mchehab: cdev and other bugs that affects older releases should be sent before the linked list patches
<br> as those patches should be c/c to stable releases, and we don't want them to depend on big changes
shuah: <u>mchehab</u>: right - the cdev life management for media device isn't that easy and might not be an isolated fix. I have been looking at some thread and patches that have cdev fix discussion
<br> anyway for now your input is helpful in patch planning -
HKitain: Hi! I'm trying to write a driver for a device that accepts a stereo image and makes a depth map a-la the BM algorithm in OpenCV. I'm trying to build it on top of the M2M framework. Is it possible to make the driver accept two src image queues instead of one without breaking too much?
<br> if anyone else has worked on such a driver, would be nice to copy their approach as well.
pinchartl: <u>HKitain</u>: you'll likely need to have multiple video nodes for such a use case, the m2m framework targets single-input, single-output devices
<br> although
<br> you could conceptually pass the stereo image as a single buffer with two images side by side
<br> but that might not be practical
HKitain: one solution I thought up was adding V4L2_FIELD_LEFT and V4L2_FIELD_RIGHT constants and signify the src accepts something like V4L2_FIELD_ALTERNATE_LR input
<br> Are there any existing devices that create multiple video nodes already?
pinchartl: another option is to use the multiplane API and pass the two stereo images in different planes
<br> but that's a hack
<br> good question regarding the fields
<br> the thing is, V4L2 doesn't support stereo images
HKitain: pinchartl, as for "side by side": neat idea, but i need to support dmabuf as well, so...
pinchartl: maybe we need to start defining a stereo/3D API
HKitain: it's a nifty idea, but i'm REALLY new to v4l, so I'm certainly not leading the charge.
pinchartl: <u>HKitain</u>: you can kickstart the discussion :-)
mchehab: <u>shuah</u>: the cdev fix is simple: it is either a single line patch, as suggested by hverkuil or it is my patch, if his approach doesn't work
larsc: is it?
shuah: well that didn't work - also for that to work media_device needs to not embedded
mchehab: <u>larsc</u>: hverkuil proposal is to set cdev->parent
shuah: I think if I remember correctly your patch allocates cdev - there is a discussion on that approach that it won't work
<br> right that is the recommended approach
larsc: <u>mchehab</u>: that's necessary but not sufficient
mchehab: <u>shuah</u>: actually, what is needed is to not embeed media_devnode, with is the struct that holds cdev
shuah: however our scenario is more complex in the senses that - we have more than one refcounted memory objects in the media_device
mchehab: I sent a patch sometime ago with that approach
<br> this is a not so complex patch and won't touch at driver's code
shuah: again not sufficient because we have struct device which is also refcounted in the media_device
larsc: a proper fix needs to touch all the drivers
mchehab: <u>larsc</u>: it is the core that embeeds media_devnode...
<br> and media_devnode is the one that has cdev embeded
larsc: you might get away with dynamically allocting the media_devnode in media_device
mchehab: the syscall handlers check for a flag at media_devnode to know if the drivers will be freeing media_device...
<br> <u>larsc</u>: yes, that's what my patch did
<br> due to the already-existing checks, the change is actually not complex...
shuah: unfortunately that might solve the cdev issue, however we will run into media ioctl use-after-free with that solution alone
mchehab: just de-embeeding media_devnode and dynamically allocating it is enough
shuah: <u>mchehab</u>: do you see media itolc use-after-free on media_device problem with your patch?
mchehab: no
shuah: the problem is we have a series of problems - like an onion
mchehab: there are still one unrelated race issue
shuah: so what happens is we run into the media itolc use-after-free on media_device problem before we see cdev use-after-free
mchehab: but I don't remember exactly what is
<br> <u>shuah</u>: there's no ioctl use-after-free!
<br> after dynamic allocation of media_devnode
shuah: ah okay that's what I was asking earlier
mchehab: https://patchwork.linuxtv.org/patch/33586/
larsc: but there is a use-while-free
<br> you need a lock to synchronize the unregister operation against the ioctl
shuah: <u>mchehab</u>: I am not sure your patch fixes the iocl use-ftare problem - can you confirm that on 4.6-rc3
mchehab: <u>larsc</u>: yes
<br> some locking schema is needed to avoid unregistering while the ioctl is running...
***: benjiG has left
mchehab: the media_device mutex does that...
<br> except that it might still have a small chance of it to be called too late
<br> on experimental tests, we were unable to see this happening
shuah: <u>mchehab</u>: yeah it is rather difficult to fix media_device use-after free without the global list and kref in the media_device
mchehab: doing ~10k bind/unbind loops
<br> <u>shuah</u>: global list is a separate issue
shuah: we can go lots of band-aids, but we won't get there
mchehab: kref is enough for that
<br> the global list is a replacement for the *_devres functions
<br> we should not mix it with kref
larsc: no kref, use the struct device in the media_devnode
<br> (which has a kref internally)
shuah: I believe we do that now if recall correctly - that alone doesn't do it
mchehab: the media_devnode kref patch was not added, I guess
<br> and it were used only for _devres
larsc: by setting the parent in the cdev to the struct device it will use the kref from the device
shuah: so mchehab - what you seem to be saying is we need a fix for stables which is your media_devnode patch?
mchehab: <u>shuah</u>: what I'm saying is that we need a patchset good enough to be applied on stable
<br> addressing it
shuah: okay here is what I will do - on 4.6-rc3, I will use the setting the parent in the cdev to the struct device and see if it solves the ioctl problem
mchehab: the global linked list is a separate issue and should be applied only for upstream
shuah: I understand
mchehab: we need to fix the existing troubles first
<br> as those changes should be backported
shuah: I am not very sure if we can fix the existing troubles fully on top of our current base - but I can give a quick try with thesetting the parent in the cdev to the struct device
larsc: setting the parent is part of the puzzle, but it is not enough
mchehab: my media_devnode patch (plus the conversion from spinlock to mutex) is very likely part of the solution
shuah: <u>larsc</u>: yes I know - I am testing with that with no results :)
larsc: if you want a simple solution, add a global media mutex that protects all operations
<br> the big-media-kernel-lock
<br> we don't care about contention at the moment
<br> that fixes the free-while-in-use
shuah: <u>mchehab</u>: would you like to focus on your patch + locking while I get the global list working?
mchehab: <u>larsc</u>: there is a lock like that for media device register/unregister
<br> currently, this is hold only when media_devnode is registered/unregistered
<br> <u>shuah</u>: very likely, I won't be able to address it this week...
<br> plus, I'm afraid that our patches will end by conflicting
shuah: <u>mchehab</u>: okay let's plan upon that - you work on the stable fix and I get the global list addressed
<br> we can deal with conflicts - they will be mainly in media core anyway
mchehab: ok, we can try this approach
shuah: <u>mchehab</u>: btw I sent in the devres remove patch
<br> last week I think - take a look and let me know if it looks good
mchehab: btw, I should be merging the spinlock->mutex patch this week upstream
shuah: <u>mchehab</u>: thanks for the heads up
mchehab: (the one that folds the fixup)
shuah: did we all review that one ? I lost track
larsc: <u>mchehab</u>: you can't protect a data structure from being freed by a lock contained within that data structure
<br> oh sorry
***: awalls1 has left
larsc: misread
mchehab: <u>shuah</u>: don't remember
<br> take a look at patchwork... it should track the acks... if not, please review
shuah: okay - thanks
***: prabhakarlad has left