On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote: > On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote: >> Hi Daniel, >> >> It seems that one check with one good commit followed by many zeroed >> intel_crtc->atomic commits is again in place. > > Can you elaborate on what you mean by this? With atomic it's possible > to have a check with no commit after it (if the check or prepare fail, > or if it's a 'test only' operation), but if you're seeing commits > without corresponding checks, that sounds like a bug. Ah so yes, this is the bug, when moving a cursor we have many commits without checks. So it is only one check followed by many commits... so it commits with intel_crtc->atomic all zeroed by a previous finish_commit. > > Can you provide a dmesg with drm.debug turned on so we can see what's > going on? Or add some dump_stack()'s so that we can see the backtrace > that led us to this situation. I lost those logs, but if you put one print in check and one in commit you also should be able to see that since I heard about this false positive from many people. > > Actually, I wonder if the problem is actually the opposite of what you > say above. Now that I look at this again, we only zero out > intel_crtc->atomic in intel_finish_crtc_commit which is part of the > commit path. >From what I heard and what I saw on the logs I thought it was ok to have 1 check than as many commits as possible without the check. If that was the case we would have a fail that is to zero the structure on finish_commit. But seems this isn't the case. Sorry. > So if you had a check that never got a corresponding > commit, there might still be garbage left over in there. No it is the opposite. Commits with no check. causing commit of intel_crtc->atomic zeroed. > Ultimately we > should be handling all of this with real intel_crtc_state handling which > we don't quite have yet. Maybe in the meantime we need to be clearing > out intel_crtc->atomic at the beginning of a top-level atomic > transaction? I'll send a patch that makes this change for you to try > shortly. Thanks! I thought about this but got afraid that clearing this on top of check could cause a race since one plane doing a check could clean the atomic being commited on cursor movement, unless we hat that for planes, but then we would have to iterate over all planes on finish_commit. We can also put this patch that fix the only known issue so far with a FIXME comment while we don't have the final fix. > > > Matt > >> >> So I'm getting that annoying false positives on latest -nightly. >> >> Shouldn't we just merge this patch while atomic modeset design doesn't >> get fixed at all? >> >> Unfortunately nothing comes to my mind than moving all >> intel_crtc->atomic set to commit time and let pre_commit just with >> pm_get... >> Ohter than that just a full redesign of atomic modeset. >> >> Thanks, >> Rodrigo. >> >> >> On Fri, Feb 13, 2015 at 12:48 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> > On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote: >> >> No, we had solved old frontbuffer false positives... some missing >> >> flush somewhere at that time... >> >> >> >> So, I added a bunch of printk and I insist that it is conceptually >> >> wrong to set intel_crtc_atomic_commit on check times when you do >> >> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); >> >> on every finish_commit. >> >> >> >> With exception of atomic.disabled_planes I believe the rest shouldn't >> >> work in the way it is implemented because you can have one check >> >> followed by many commits, but after the first commit all atomic >> >> variables are zeroed, except the disabled_planes that is set outside >> >> check... >> > >> > Ok here's the trouble: Every commit should have at exactly one check for >> > the new state objects. Unfortunately in the transition that seems to have >> > been lost for some cases. >> > >> >> For instance: on every cursor movement atomic.fb_bits was 0x000 when >> >> it should be 0x002. This is why this patch solved the false positive, >> >> i.e. setting it on commit instead on check time we get it propperly >> >> set. One of the problems is the false positive but also it breaks >> >> entirely SW tracking on VLV/CHV.... >> >> >> >> I believe wait_for flips, update_fbc, watermarks, etc should keep the >> >> value got on check for the commit or the check should be done at >> >> commit plane instead of on check. >> >> >> >> I started doing a patch here to move all atomic sets from check to >> >> commit functions but gave up on middle when I noticed the >> >> prepare_commit would almost get empty... >> > >> > All state precomputation must be done in check, at commit time you have a >> > lot less information since the old state is somewhat gone. You can still >> > get at it, but as soon as we add an async flip queue that will get really >> > ugly. The current placement is imo the correct one. Instead we need to >> > figure out where we're doing a ->commit without properly calling ->check >> > beforehand. >> > >> >> Another idea was to make a atomic set per plane and just memset(0) on >> >> begin of every check... But this would require reliable access to the >> >> plane being updated on finish_commit... I believe loop on all planes >> >> would be messy and cause other issues... >> >> >> >> So, I'll be out returning only next wed. Please let me know if you >> >> have any suggestion of best changes to do that I can implement the >> >> changes. >> > >> > Since you've done this testing I've landed Matt's patches to switch legacy >> > plane entry points over to atomic. Which means cursor updates should now >> > be done properly using atomic, always. But even then the old transitional >> > plane helpers should have called the check functions ... So not sure where >> > exactly we're loosing that check call. >> > >> > Matt Roper might have more insights. >> > >> > Thanks, Daniel >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >> >> >> >> -- >> Rodrigo Vivi >> Blog: http://blog.vivi.eng.br > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
