IRC Logs of #dri-devel on for 2015-01-25

Previous dayChoose dateNext day Show menu

05:34 #dri-devel: < robclark> airlied_, btw, after rebasing on latest drm-next, I still have and in my branch..
05:34 #dri-devel: < robclark> the first one, I need for msm dp.. so I can include in my pull req if you want
05:34 #dri-devel: < robclark> (second one should probably land in various -fixes branches)
09:38 #dri-devel: < robclark> danvet, we should just go w/ my patch to not loose hotplug events unless you have a good idea for the splat I just sent..
11:20 #dri-devel: < pinchartl> robclark: I'm trying to convert omapdrm to the atomic update API and I'm running into a vblank issue
11:20 #dri-devel: < pinchartl> I was wondering if you could help me
11:20 #dri-devel: * robclark can try
11:20 #dri-devel: < pinchartl> in a nutshell, the call chain is
11:21 #dri-devel: < pinchartl> .set_config = drm_crtc_helper_set_config
11:21 #dri-devel: < pinchartl> drm_crtc_helper_set_mode
11:21 #dri-devel: < pinchartl> .mode_set = drm_helper_crtc_mode_set
11:21 #dri-devel: < pinchartl> drm_helper_crtc_mode_set_base
11:21 #dri-devel: < pinchartl> drm_plane_helper_commit(crtc->primary)
11:21 #dri-devel: < pinchartl> drm_crtc_wait_one_vblank
11:21 #dri-devel: < pinchartl> at that point I get a timeout
11:21 #dri-devel: < robclark> hmm, so vblank is not enabled yet (or at least not getting irqs)?
11:21 #dri-devel: < pinchartl> as we're in the middle of a mode set the crtc .prepare operation has been called, which disable the outputs
11:21 #dri-devel: < pinchartl> and .commit hasn't been called yet to reenable the outputs
11:22 #dri-devel: < robclark> hmm..
11:22 #dri-devel: < pinchartl> I assume that this is why the DSS doesn't generate any vblank interrupt
11:22 #dri-devel: < robclark> sounds likely..
11:22 #dri-devel: < pinchartl> how am I supposed to solve that ? :-)
11:22 #dri-devel: < pinchartl> outputs need to be disabled during modeset
11:22 #dri-devel: < robclark> hmm..
11:23 #dri-devel: < robclark> well.. I never did use the transitional helpers..
11:23 #dri-devel: < robclark> but seems like wait_one_vblank() should really happen after things are enabled again..
11:23 #dri-devel: < robclark> could be transitional helpers are expecting tings to be enabled somewhere else, I guess..
11:24 #dri-devel: < pinchartl> maybe, but I don't see where
11:24 #dri-devel: < pinchartl> or maybe they're broken :-)
11:24 #dri-devel: < robclark> not sure how much it is worth spending time on that.. vs just switching to atomic which moves things around a bit more ;-)
11:24 #dri-devel: < robclark> atomic helpers
11:25 #dri-devel: < pinchartl> I guess I should go straight to atomic updates then
11:25 #dri-devel: < pinchartl> or fix the transitional helpers
11:25 #dri-devel: < pinchartl> not on a Sunday evening though
11:25 #dri-devel: < robclark> I would think so.. might as well only debug one set of problems ;-)
11:25 #dri-devel: < robclark> omapdrm isn't such a huge driver, compared to i915 or other desktop drivers which are supporting many generations..
11:26 #dri-devel: < robclark> so I don't see strong advantage to transitional helpers there
11:26 #dri-devel: < pinchartl> one thing bothers me though
11:26 #dri-devel: < pinchartl> in mdp4 and mdp5
11:26 #dri-devel: < pinchartl> you use drm_helper_crtc_mode_set
11:26 #dri-devel: < pinchartl> so you end up with the same call stack as me
11:27 #dri-devel: < pinchartl> -> drm_helper_crtc_mode_set_base -> drm_plane_helper_commit(crtc->primary) -> drm_crtc_wait_one_vblank
11:27 #dri-devel: < robclark> hmm..
11:28 #dri-devel: < robclark> no, because drm_atomic_helper_set_config
11:28 #dri-devel: < robclark> so.. I think I could get rid of plugging in drm_helper_crtc_mode_set helper.. I don't think atomic helpers use it..
11:29 #dri-devel: < pinchartl> right
11:30 #dri-devel: < pinchartl> danvet: are the transitional helpers supposed to work ? ;-)
11:30 #dri-devel: < robclark> danvet, perhaps we should mark a few of the helper fxn ptrs as unused by atomic helpers and should not be filled in..
11:30 #dri-devel: < pinchartl> and document that :-D
11:30 #dri-devel: < robclark> right
11:31 #dri-devel: < pinchartl> Daniel's blog post on driver conversion to atomic update is nice but already outdated it seems :-/
11:31 #dri-devel: < robclark> my guess is transitional helpers for for how prepare/commit/etc are supposed to be used, but omapdrm does something non-standard..
11:32 #dri-devel: < robclark> yeah, I think blog post was when this was all still partially theoretical :-P
11:32 #dri-devel: < robclark> at any rate, atomic does wait-vblank after calling crtc->commit()
11:32 #dri-devel: < pinchartl> yes, that's much better
11:32 #dri-devel: < robclark> so I don't *think* you'll have same problem (famous last words)
11:32 #dri-devel: < pinchartl> waiting for vblank before commit seems pretty broken to me
11:33 #dri-devel: < pinchartl> :-)
11:33 #dri-devel: < robclark> sounds like transitional helpers are borked, but I guess that is a question for danvet
11:33 #dri-devel: < robclark> yeah
11:33 #dri-devel: < danvet> robclark, pinchartl you're just doing it wrong
11:33 #dri-devel: < danvet> helpers assume that your vblank code is up to par with i915
11:33 #dri-devel: < danvet> and when the pipe is off return some error code
11:33 #dri-devel: < pinchartl> I'll finish simplifying omap_irq.c and then I'll switch directly to full atomic update
11:34 #dri-devel: < danvet> indicating that vblank waits aren't a good idea
11:34 #dri-devel: < danvet> it /should/ skip the vblank wait in that case
11:34 #dri-devel: < pinchartl> danvet: ok, nice to know
11:34 #dri-devel: < danvet> also blog should still be up to date really
11:34 #dri-devel: < danvet> only thing that changed is that now you can also do legacy dpms on top of atomic with one more helper
11:34 #dri-devel: < danvet> well, rsn since not yet in drm-next
11:35 #dri-devel: < danvet> I think the blog is a bit too unclear about the precise semantics the atomic helpers expect
11:35 #dri-devel: < pinchartl> danvet: I've found the blog post very interesting, but there were still quite a few unanswered questions after reading it
11:35 #dri-devel: < danvet> which tagr run into when he tried to wire up legacy dpms and it didn't work
11:35 #dri-devel: < pinchartl> yes, that's the main problem
11:35 #dri-devel: < danvet> so I think I'll clarify that later on
11:36 #dri-devel: < danvet> there's also a patch in-flight which renames the enable/disable hooks to enable/disable
11:36 #dri-devel: < robclark> hmm.. looks like msm vblank code should return some errors when pipe off..
11:36 #dri-devel: < pinchartl> wouldn't it be worth it adding it to Documentation/drm/ ?
11:36 #dri-devel: < danvet> so that the historically grown names from the crtc helpers aren't as confusing any more
11:36 #dri-devel: < danvet> pinchartl, I want it as kerneldoc, once that contract gig to add markdown to kerneldoc has gone through
11:37 #dri-devel: < danvet> so that we can do proper formatting and all in DOC: sections
11:37 #dri-devel: < danvet> meanwhile tagr will do a presentation about the tegra conversion at fosdem
11:37 #dri-devel: * danvet volunteered him ;-)
11:37 #dri-devel: < pinchartl> danvet: in the meantime couldn't we just add it as a .txt file to Documentation/drm/ ? it would be easier to find it and keep it up to date
11:38 #dri-devel: < danvet> well I'm pretty good at not keeping docs up to day even when they're inline ...
11:39 #dri-devel: < danvet> *date
11:39 #dri-devel: < pinchartl> that's the main point, someone else could do it :-)
11:39 #dri-devel: < danvet> thus far the approach was that I've done "how to convert your arm drm driver to atomic" consulting on irc ;-)
11:39 #dri-devel: < pinchartl> while it's hard for other developers to keep your blog post up to date
11:39 #dri-devel: < pinchartl> :-)
11:40 #dri-devel: < pinchartl> robclark: should I submit a patch to remove the unneeded helpers from mdp4 and mdp5 ?
11:40 #dri-devel: < pinchartl> or are you doing it already ?
11:41 #dri-devel: < pinchartl> danvet: kerneldoc documents .enable_vblank as
11:41 #dri-devel: < robclark> pinchartl, I haven't done it yet.. was waiting to see what danvet said... I only assume we should not use them any more, but wanted to check that danvet didn't have plans for them..
11:41 #dri-devel: < pinchartl> * Enable vblank interrupts for @crtc. If the device doesn't have
11:41 #dri-devel: < pinchartl> * a hardware vblank counter, this routine should be a no-op, since
11:41 #dri-devel: < pinchartl> * interrupts will have to stay on to keep the count accurate.
11:42 #dri-devel: < pinchartl> I'll add a sentence to explain that drivers should return an error if the crtc isn't active
11:42 #dri-devel: < danvet> which helpers are we talking about?
11:42 #dri-devel: < robclark> danvet, we shouldn't use mode_set or mode_set_base any longer, right?
11:42 #dri-devel: < pinchartl> but is it still tre that devices without hw vblank counters should implement enable_vblank as a no-op ?
11:42 #dri-devel: < pinchartl> s/tre/true/
11:42 #dri-devel: < danvet> crtc_helper_funcs->mode_set/_base are no longer in use with atomic/transitional helpers
11:43 #dri-devel: < danvet> I think
11:43 #dri-devel: < robclark> they aren't currently used
11:43 #dri-devel: < danvet> the only one left is mode_set_nofb
11:43 #dri-devel: < danvet> well didn't check fb helper's panic handler
11:43 #dri-devel: < danvet> pinchartl, that's fairly orthogonal to atomic stuff ...
11:43 #dri-devel: < danvet> i.e. don't ask me ;-)
11:44 #dri-devel: < danvet> we recently added new interfaces for drivers to use to explicitly enable/disable vblank on a pipe
11:44 #dri-devel: < danvet> drm_vblank_on/off
11:44 #dri-devel: < danvet> which those I guess ->enable could be a noop
11:44 #dri-devel: < pinchartl> yes, I know it's totally orthogonal :-)
11:44 #dri-devel: * danvet not sure because w/e ;-)
11:48 #dri-devel: < danvet> hm, not sure how the vblank stuff actually works again
11:48 #dri-devel: < pinchartl> I'm not sure either :-/
11:48 #dri-devel: < pinchartl> I think I recall that it works correctly :-)
11:48 #dri-devel: < danvet> the idea is that if the vblank_get fails the pipe is off and the helpers will drop the vblank wait
11:49 #dri-devel: < danvet> but not sure any more how that's all wired up again ....
11:49 #dri-devel: < pinchartl> for the transitional helpers it should work properly
11:49 #dri-devel: < pinchartl> but I'm worried it might break other code paths
11:49 #dri-devel: < pinchartl> I guess I'll need to check that :-/
11:49 #dri-devel: < danvet> in the kernel or userspace?
11:50 #dri-devel: < pinchartl> kernelspace
11:50 #dri-devel: < danvet> since with userspace the semantics the helpers assume is what i915 does
11:50 #dri-devel: < danvet> or at least should do if no one did break a few igts meanwhile
11:52 #dri-devel: < pinchartl> what I mean is that not all drivers return an error in .enable_vblank if the crtc is disabled
11:52 #dri-devel: < pinchartl> and fixing that might introduce breakages elsewhere
11:52 #dri-devel: < danvet> well I'm not sure any more whether that's still how it works on i915 even
11:53 #dri-devel: < pinchartl> ok, now we're getting somewhere :-D
11:53 #dri-devel: < pinchartl> that's what your transitional helpers expect at least
11:54 #dri-devel: < danvet> yup
11:54 #dri-devel: < pinchartl> ah... i915 has no .enable_vblank...
11:55 #dri-devel: < danvet> we have 5
11:55 #dri-devel: < danvet> one per major vblank platform
11:56 #dri-devel: < pinchartl> yes, my bad
11:56 #dri-devel: < pinchartl> it's the w/e for me too :-)
11:56 #dri-devel: < pinchartl> they all start with
11:56 #dri-devel: < pinchartl> if (!i915_pipe_enabled(dev, pipe))
11:56 #dri-devel: < pinchartl> return -EINVAL;
11:57 #dri-devel: < danvet> it still works on i915
11:57 #dri-devel: < danvet> well yeah
11:58 #dri-devel: < danvet> but i915 uses vblank_off/on for when the pipe is off
11:58 #dri-devel: < danvet> and vblank_get only calls ->enable when the count goes 0->1
11:58 #dri-devel: < danvet> but vblank_off/on hold a fake count I think
11:58 #dri-devel: < danvet> or I'm dense
11:58 #dri-devel: < pinchartl> how are drm_vblank_on and drm_vblank_off supposed to be used ?
11:59 #dri-devel: < danvet> so I don't see how vblank_get is called for a vblank wait ioctl from userspace when the pipe is off
11:59 #dri-devel: < danvet> but I _do_ get an -EINVAL
11:59 #dri-devel: < danvet> do it like i915
12:00 #dri-devel: < danvet> essentially we added new off/on vblank funcs since not breaking ums was too hard
12:00 #dri-devel: < danvet> they should be in your enable/disable hooks for crtc
12:00 #dri-devel: < danvet> or prepare/commit as they're still called
12:00 #dri-devel: < danvet> other way round I mean
12:01 #dri-devel: < pinchartl> there's only 3 drivers calling drm_vblank_on...
12:02 #dri-devel: < pinchartl> this is what bothers me with kerneldoc
12:02 #dri-devel: < pinchartl> new functions are documented
12:02 #dri-devel: < pinchartl> with comments that explain what they do
12:02 #dri-devel: < pinchartl> but we don't have documentation explaining how and when they should be used
12:03 #dri-devel: < pinchartl> that's what the docbook documentation was trying to fix
12:05 #dri-devel: < pinchartl> and regarding enable_vblank, there are only 3 drivers returning an error when the pipe is disabled
12:05 #dri-devel: < pinchartl> i915, rockchip and exynos
12:05 #dri-devel: < pinchartl> and exynos doesn't seem to do it in all cases, but I could be mistaken
12:12 #dri-devel: < danvet> oh lovely cargo-cult
12:12 #dri-devel: < danvet> so with drm_vblank_off/on vblank_get will dtrt I think with or without an ->enable_vblank hook
12:12 #dri-devel: < danvet> all those checks in i915 are ums cargo-culting
12:13 #dri-devel: * danvet writes test to put that theory under the grill now
12:13 #dri-devel: < danvet> *patch
12:13 #dri-devel: < danvet> tests we have ;-)
12:13 #dri-devel: < danvet> pinchartl, wrt docs: I think the bigger problem is that we move i915 around a lot but other drivers stay still
12:15 #dri-devel: < danvet> and the kerneldoc for drm_crtc_vblank_off/on (which are the recommended version for kms drivers) should be upgraded to a MUST instead of CAN
12:15 #dri-devel: < danvet> or at least a really strong SHOULD
12:15 #dri-devel: < pinchartl> you can't blame other drivers for not following if you don't tell them where to go ;-)
12:18 #dri-devel: < danvet> oh I don't blame them
12:18 #dri-devel: < danvet> just explaining
12:18 #dri-devel: < pinchartl> so .enable_vblank doesn't have to return an error if the driver calls drm_vblank_off/on in .prepare/.commit ?
12:19 #dri-devel: < danvet> I'm testing that right now
12:19 #dri-devel: < danvet> I think so
12:20 #dri-devel: < danvet> pinchartl, btw we'd in much better shape if someone would enable igt to run on top of libkms
12:20 #dri-devel: < pinchartl> I think that's right only if vblank was enabled in the first place
12:20 #dri-devel: < danvet> since we do have all the tests for these cornercases already
12:20 #dri-devel: < pinchartl> drm_vblank_get() checks ->enabled only if the refcount wasn't initially 0
12:20 #dri-devel: < pinchartl> aren't we getting rid of libkms ?
12:20 #dri-devel: < danvet> well, dumb fb ioctls would work too
12:21 #dri-devel: < danvet> just saying that we do have tests for all these cornercases
12:21 #dri-devel: < danvet> so drivers could have tools to keep up and all that much easier
12:21 #dri-devel: < danvet> but porting igt is not really something I can sell to my boss ;-)
12:22 #dri-devel: < pinchartl> that would be very interesting, yes
12:22 #dri-devel: < pinchartl> :-)
12:22 #dri-devel: < pinchartl> you could sell it to linaro ;-)
12:22 #dri-devel: < danvet> but I'd be very much willing to take patches and take the additional maintenance burden
12:22 #dri-devel: < danvet> well I chatted a bit with mlankhorst about how to do it all
12:23 #dri-devel: < danvet> but not sure whether linaro is doing anything serious with kernel gfx
12:23 #dri-devel: < danvet> last time I chatted with them it didn't sound like
12:23 #dri-devel: < pinchartl> good question
12:24 #dri-devel: < pinchartl> I'm not sure either
12:24 #dri-devel: < danvet> at least that was my take-away from chatting with john stultz
12:24 #dri-devel: < pinchartl> I guess I could ask them, I'll be at linaro connect in two weeks
12:43 #dri-devel: < pinchartl> danvet: should drivers wait for pending page flips to complete before calling drm_vblank_off() ?
12:43 #dri-devel: < danvet> no and yes
12:44 #dri-devel: < danvet> with atomic updates should always be ordered, but at a higher level
12:44 #dri-devel: < danvet> i.e. if you do async anything your async_commit should do that when you have a sync commit
12:44 #dri-devel: < pinchartl> right
12:45 #dri-devel: < pinchartl> and without atomic updates ?
12:45 #dri-devel: < danvet> long-term plan is to rework drm_irq.c (*ugh*) and write a generic async helper using the flip queue stuff
12:45 #dri-devel: < danvet> yeah you need to
12:45 #dri-devel: < danvet> otherwise the ts will be bonkers
12:46 #dri-devel: < danvet> well _off will send out any pending events right away
12:46 #dri-devel: < danvet> but since your pageflip might still be in some other queue that might not happen
12:46 #dri-devel: < danvet> and hilarity ensues instead
12:46 #dri-devel: < danvet> making drm_irq.c sane and useful for in-kernel code in a generic way is what we need to fix all that
12:46 #dri-devel: < pinchartl> :-)
12:47 #dri-devel: < pinchartl> I agree, the core should be able to handle that
12:47 #dri-devel: < danvet> unfortunately I've already fried pzanoni in the first attempt to volunteer someone for it ;-)
12:47 #dri-devel: < pinchartl> :-D
12:55 #dri-devel: < pinchartl> what the ... omapdrm doesn't call drm_vblank_get() when a page flip is queued
12:56 #dri-devel: < pinchartl> one problem at a time. one problem at a time.
12:56 #dri-devel: < pinchartl> :-)
13:06 #dri-devel: < pinchartl> there might be something I'm missing. drm_mode_page_flip_ioctl() unreferences the old fb right after the .page_flip operation returns
13:06 #dri-devel: < pinchartl> that's before the page flip completes
13:06 #dri-devel: < pinchartl> are drivers supposed to take a reference to the new fb in the .page_flip handler, and release it when the flip completes ?
13:07 #dri-devel: < pinchartl> otherwise the old fb can be destroyed while still being scanned out
13:07 #dri-devel: < pinchartl> or am I missing something ?
13:18 #dri-devel: < pinchartl> it looks like omapdrm takes references to the gem objects instead
13:19 #dri-devel: < pinchartl> that should work, but it isn't very pretty
13:47 #dri-devel: < pinchartl> robclark: what's the chance to get sgx to move to dma-buf fences ? I'd like to get rid of the omapdrm private implementation
13:48 #dri-devel: < pinchartl> do you know if it's "just" a matter of implementing that in their kernel driver ?
13:52 #dri-devel: < danvet> pinchartl, with atomic the state structures/helpers will take a ref of the fb for you
13:53 #dri-devel: < danvet> and omapdrm probably precedes fb refcounting, so bo refcounting was the way to go
13:53 #dri-devel: < danvet> i915 still works like that
13:54 #dri-devel: < danvet> wrt lack of vblank_get: the core will fake some timestamp if you don't have a running vblank
13:54 #dri-devel: < pinchartl> omapdrm uses fb refcounting for planes and bo refcounting for page flips
13:54 #dri-devel: < danvet> e.g. for virtual drivers
13:54 #dri-devel: < pinchartl> don't ask me why :-)
13:55 #dri-devel: < danvet> blame robclark and run away ;-)
13:55 #dri-devel: < danvet> anyway getting late here
13:55 #dri-devel: * danvet ^Z
13:56 #dri-devel: < pinchartl> I should go to bed too
13:56 #dri-devel: < pinchartl> g'night
13:59 #dri-devel: < robclark> pinchartl, the refcounting predated fb refcnting...
13:59 #dri-devel: < robclark> I retrofitted it at some point (I think) to use fb refcounting instead of bo refcounting... completely possible that I missed some bits..
13:59 #dri-devel: < robclark> so blame anything funny looking on legacy :-P
14:00 #dri-devel: < pinchartl> :-)

Written by Christoph Brill © 2007-2015

Valid XHTML 1.0 Transitional

Available in Android Market