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. 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. 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. So if you had a check that never got a corresponding commit, there might still be garbage left over in there. 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. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
