Hi Daniel, It seems that one check with one good commit followed by many zeroed intel_crtc->atomic commits is again in place. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
