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