hverkuil: ping
svarbanov: pong
hverkuil: do you have a chance to think more on m2m scheduling issue ?
hverkuil: IMO v4l2_m2m_try_run() should be skipped in cases when the we can have multiple m2m_ctx running on the same time
I have a question: this scheduling issue is also present in the v3 patch series, right? It has nothing to do with my request that the v4l2_m2m_ioctl_ helpers are used?
hverkuil: in v3 I do not use m2m API's so there is no scheduling issue there
Ah, now I see it. You're not using v4l2-mem2mem.h at all.
hverkuil: yes, correct. I have WIP driver which is using m2m APIs now, and it is almost working
hverkuil: and this WIP also include few extensions to the m2m API
You probably have a line similar to this in the probe(): dev->m2m_dev = v4l2_m2m_init(&m2m_ops);
right?
hverkuil: correct
What I was thinking is that I believe you can do that in the open() instead: so each filehandle has its own m2m_dev, and since there is only one m2m_dev per context there is also effectively no scheduling.
hverkuil: yes, I have tried this and it obviously works but I think that this is not correct usage of m2m API
I may be missing something, but I don't offhand see a reason why it wouldn't work.
svarbanov: why wouldn't it be correct usage of the m2m API?
hverkuil: because most of drivers create m2m device in probe, i.e only one m2m_dev per device video node :)
svarbanov: so?
In your case the fw does the scheduling, so creating a m2m_dev per instance makes sense here.
hverkuil: ok, if you are agree with that I will do it that way
Let's see how it looks in the code.
hverkuil: ok, then patchset v4 will use m2m API, plus the m2m extensions for removing buffers from ctx queue lists
Good. Please add comments describing why m2m_dev is per instance.
ok, I will
hverkuil: thanks!
no problem, it's an interesting variation of m2m devices :-)
hverkuil: yeah, this driver is pure software wrapper of the firmware interface, there is no hardware bits, registers ... nothing :)
hverkuil: I'm not happy because the whole beauty of the hardware is inside firmware code :(
hverkuil: what does v4l2-mem bring to drivers when there's no scheduling to be handled in software ?
neg: Hej!
sailus: we probably have to discuss how to go forward with our patch series. Do you mind rebasing on top of media dev allocator api and use that allocation in your series
I am planning to upload my series to my kernel.org git repo and also start working on applying your series on top of mine and start testing
pinchartl ^^ your thoughts n this proposal
mchehab said he won't have time to review my patch series until after 4.10-rc1 - if we can agree on a plan, maybe we can start working towards 4.11 target
shuah: I said I *likely* won't have time
mchehab: okay that is good news - there is a hope you just might - thanks
there are too much patches pending review
it would help if sub-maintainers could try to review their patches too
  LinuxTV community                                                     : 260
  Laurent Pinchart <laurent.pinchart@ideasonboard.com>                  : 247
  Hans Verkuil <hansverk@cisco.com>                                     : 111
  Guennadi Liakhovetski <g.liakhovetski@gmx.de>                         : 71
  Kamil Debski <k.debski@samsung.com>                                   : 27
that's the current patch counts per reviewers, plus the ones not classified
last week, there was +450 patches to handle
at the "LinuxTX community" unclassified patchset
mchehab: right - we are already ay 4.9-rc6 - even so, we have to com up with an action plan for these patch series sailus pinchartl - thoughts
yep. I'll freeze patch merge this week
(except for serious fixups)
one request for you is please consider taking devres cleanup patch - we never used these interfaces ever. It is an indepedent cleanup patch in my opinion
I prefer applying it together with the media allocator patch series, as it makes easier to review
as I want to verify that the things that were considered there were somehow addressed by your new approach
also, there's no need to rush in that code removal
okay
doing it altogether seems optimal, IMHO
shuah: Let's address the existing issues in the media device allocator patches first.
I'm not too happy it's making the object lifetime issues worse than what they currently are.
The current bugs should be fixed first and then the framework can be extended, rather than extending it first and then try to figure out how it can be fixed...
sailus: I am not clear on how media dev allocator api is making lifetimne issues worse - do you mean the 3 fixes that went earlier or the patch you reviewed
I did address your review comments and sent new version out
ah I see your second comment now - so sounds like you would like to see media dev allocator api based on top of your RFC series - did I understand that correctly?
pinchartl, sailus. mchehab: we are kind of at a stalemate guys - can we decide on a going formward path?
options:
1. add media dev allocator api and get the snd_usb_audio work done and then get Sakari's RFC series in (this requires changes to drivers etc.)
2. Other way round RFC series first and then media dev allocator and snd_usb_aduio. I am getting pressure from Audio folks for this work ro be wrapped up
Let's get to some action plan  that is workable
sailus need to send a non-RFC patchset
as I said several times, patches should be incremental, and not reverting some previous fixes
shuah: was you able to reproduce the issues sailus pointed on your tests?
shuah: I'd prefer if we merged Sakari's patches first
(I think he's currently driving back home)
mchehab: Sakari told me he was planning to send a non-RFC version after receiving Hans' review
pinchartl: well, he need first to submit such patch series
That is a very long pole in the tent - as you can see with driver changed and so on
mchehab: I think one problem he is seeing is use-after-free on video dev if device is removed while streaming
I'll only start reviewing his series when it becomes something mergeable
let me ask the question anyway, can we merge media dev allocator api and snd_usb_audio first?
shuah: were you able to reproduce it?
no I haven't been able yet - I am going to try again - I ran several bind_unbind loops with 3 applications running
mchehab: please don't be stubborn. you can review at least patches from 4 onwards
pinchartl: patches 1 to 3 are regressions
shuah: it's easy to reproduce. open the video device, start streaming, unbind it, stop streaming, close it
mchehab: ignore them for now
shuah: at least with omap3isp
how can I identify if the remaining patches aren't causing regressions, if the entire series start with regressions?
I expect other devices to be similarly affected
mchehab: you don't need to perform an in-depth review at this time
what would be useful to move this forward is your opinion on the approach and the end result
pinchartl: okay I can try it on em28xx and au0828 amd see what happens
to see if you at least agree on the direction
this is the kind of series that requires in-depth and tests to review
shuah: do you have access to any platform device ? I expect USB device to be similarly affected, but I haven't checked that myself
pinchartl: for what its worth Sakari's series won't be complete until all the drivers are changed and tested
mchehab: so was your MC rework series, and you repeatidly refused to squash fixes into the earlier patches that introduced issues. those were regressions too
no, nothing of these is related to MC rework
au0828 is a complex case with snd_usb_audio in the mix
those bugs are there since day zero
I'm telling you you merged a patch series in which you introduced regressions and then fixed them up in latter patches
so you've done exactly what you're saying shouldn't be done here
pinchartl: shit happens, but no regressions were introduced by purpose
yes they were
we've pointed out regressions
I rebased it countless times, every time someone pointed regressions
and you added patches on top instead of reworking the series
and at some point you stopped rebasing
so it was a deliberate decision
only when people stopped reviewing
no fake excuse please
I did a rebase at the very last moment before applying it
let's get these problems fixed
anyway, the discussion here is unrelated
if you're committed to getting the MC mess fixed, please review the patches
otherwise please let us work
pinchartl: I assume you didn't try the video streaming issue with media dev allocator api? Is it possible for you send me the dmesg for the issue
shuah: no I haven't
I will review, as soon as it is ready for review
and I think Sakari posted an oops report trigerred by the sequence I just described
shuah: if the problem is reproducible with platform drivers, I guess you can use either exynos-gsc or exynos-mfc to test
javier__: possibly
pinchartl: is this related to media controller or does it happen even when driver doesn't use media controller
shuah: it's related to MC
I'm trying to find the oops report
shuah: if you apply Sakari's patch series, can you reproduce its bug on em28xx?
mchehab: that is something that needs to be tested - haven't tried that
pinchartl: will you have time to reproduce the problem with media dev allocator api?
shuah: ah, I misunderstood. If is related to MC, then you can't use those exynos platform drivers
right
mchehab: see the last e-mail in the "Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed" thread
okay here is what I am going to - I will try to reproduce the problw Sarai reported with media dev allocator api first since I have that ready to go
Testing with Sakari's sries will require changes to em28xx before we can test
we can decide based on the results
shuah: I've just reproduced the issue with the Renesas VSP1
with the sequence I've described
I'll send you the oops report
and will try your patch series
pinchartl: you will need to make changes to the driver to use media dev allocator api before you can run the test - I am sure I am stating the obvious :)
pinchartl: are you using devm_* on vsp1 driver?
thanks for the oops - I will compare it with my test on au0828
shuah: v4 of the series and v5 of 1/3 ?
correct
well I only need 1/3
you probably just need that one patch media dev allocator v5 - the other two patches are driver changes
right 1/3 v5
as sailus answered: "around 95% of the changes are related to removing the user of devm_()"
shuah: there's no non-USB API in v5...
pinchartl: yes you are right - okay you won't be able to test it with the same driver
support for non-USB devices is kind of a critical feature :-)
pinchartl: yes my plan to add that - I don't have a device to test non-USB case at the moment
I can test a v6 that will add support for non-USB devices with the VSP, but I'm afraid that v5 isn't usable for me
I understand - I am assuming not supporting non-USB won't be a show stopper for now
pinchartl: The oops you sent, is that 4.9-rc4 or 4.9-rc4+patches
it's 2bfc04d64db66fa62021a988740e8028f018b9c3
the HEAD of the media tree master branch when I last pulled it
(it's been updated since)
(with a single patch that touches an lirc driver only)
pinchartl, mchehab: I can't reproduce th problem on 4.9-rc5 on au0828 - jus 4.9-rc5 with no additional changes
I did unbind on au0828 while vlc is streaming - no issues
I did test with media dev allocator api as well - same results.
shuah: I assume au0828 doesn't use media_entity_pipeline_start()/media_entity_pipeline_stop() ?
yes it does
it uses the non-locking versions
I can generate the graph for you while vlc is treaming
but I'm not sure that makes a difference
I don't know why it doesn't crash with that driver
that might make a difference with mutex_lock_nested() in your oops
yes but you lock the same mutex in the function that calls media_entity_pipeline_stop()
I first tested with media dev allocator api and then went back to 4.9-rc5 stock
so I don't think that's the problem
right - that is not it - it is bahaving like do_exit path references a resource that is gone - similar to what we used to see with media device cdev exit path
pinchartl, shuah: au0828 doesn't use devm_()
vsp1 and omap3 does
correct - using devm causes all sorts of problems - so I think this is what happens - devm resources get treleased in device_release() path before v4l2_release() is called
pinchartl: devm is very problematic
after reading and answering the long 00/21 reply from Sakari (with I missed somehow), I'm almost sure that the problems with omap3 are either due to devm_*() or due to some dev_foo() print after driver unregister
shuah: no kidding. I've been trying to raise awareness of devm's issues for more than a year
pinchartl: yes thanks to you I learned a lot
mchehab: no they're not. devm isn't the culprit there. without devm we would have to call kfree too early as media_device has no release callback
I debugged the a similar issue when I had au0828 and snd_usb_audio using devres interfaces I added to media controller api
pinchartl: either way we have to figure out what is this problem first though
btw adding printks could make this problem disappear
shuah: Sakari has already debugged the issue, and the result is the patch series that we're discussing
yeah I understand - if it is a driver specific issue and can't be reproduced on other drivers, then we have to look closely instead of changing media-core
pinchartl: you could either override dev.release() or to add a callback inside media_devnode_release() to call a driver specific callback
e. g. instead of:
devnode->dev.release = media_devnode_release;
devnode->dev.release = my_device_release;
and inside my_device_release, call media_devnode_release
on the drivers that require to release other things
mchehab: I think we should stop discussing this now
agreed... I still have 244 patches to review
hverkuil: you don't pick up patches for staging/media/davinci/vpfe, do you ?
pinchartl: they are automatically delegated to you, so no, I generally don't pick them up.
hverkuil: I'll pick them up. I usually don't get CC'ed so I miss them
is there anyone still maintaining the davinci platform ?
pinchartl: I see that Sekhar Nori (nsekhar) maintains the platform bits
not sure about the media platform driver
davinci is still not multiplatform :-/
pinchartl: Sekhar Nori
he's on #armlinux (but /away now)
I hope he has plans to move davinci to DT :-)
pinchartl: only DA850 supports DT, I doubt that the real DaVincis will ever be converted
or even DA830
there's ongoing effort to convert the USB drivers to DT
like OHCI and MUSB glue
mchehab: 63 patches left (out of 247), I'll call it a night