2013/12/4 Daniel Vetter <daniel@xxxxxxxx>: > On Thu, Nov 21, 2013 at 01:47:18PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> Currently, PC8 is enabled at modeset_global_resources, which is called >> after intel_modeset_update_state. Due to this, there's a small race >> condition on the case where we start enabling PC8, then do a modeset >> while PC8 is still being enabled. The racing condition triggers a WARN >> because intel_modeset_update_state will mark the CRTC as enabled, then >> the thread that's still enabling PC8 might look at the data structure >> and think that PC8 is being enabled while a pipe is enabled. Despite >> the WARN, this is not really a bug since we'll wait for the >> PC8-enabling thread to finish when we call modeset_global_resources. >> >> So this patch makes sure we get/put PC8 before we update >> drm_crtc->enabled, because if a get() call triggers a PC8 disable, >> we'll call cancel_delayed_work_sync(), which will wait for the thread >> that's enabling PC8, then, after this, we'll disable PC8. >> >> The side-effect benefit of this patch is that we have a nice place to >> track enabled/disabled CRTCs, so we may want to move some code from >> modeset_global_resources to intel_crtc_set_state in the future. >> >> The problem fixed by this patch can be reproduced by the >> modeset-lpsp-stress-no-wait subtest from the pc8 test of >> intel-gpu-tools. >> >> v2: - No need for pc8.lock since we already have >> cancel_delayed_work_sync(). >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > Now that Imre's big rework has landed this looks a bit strange. We now > have the display power wells (properly refcounted) and the hsw pc8 stuff > here. Which imo doesn't make much sense since to have a working display > power well we can't go into pc8. This patch has nothing to do with power wells. We're checking enabled/disabled CRTCs here, not power wells. The problem this patch solves also happens on LPSP mode, where the power well is disabled. I'm confused, please clarify. > > So the right thing to do is to only grab the topmost power well/domain > reference. Those in turn then need to grab the lower-level domains. I.e. > on hsw the crtc get/put should also do a pc8 get/put and then that in turn > should do a D3 get/put (if we keep them split up). > Same as above. > With that change it'd also make tons of sense to move all the hsw pc8 > stuff into intel_pm.c together with the other power well stuff. Imo the > approach with use_count in Imre's code is easier to understand. > > Now for the actual issue you're trying to fix here: That's just a race in > the check in assert_can_disable_lcpll - you access crtc->base.enabled > without taking the right locks. Which ones are the "right locks"? > And if the work runs concurrently to > re-enabling the display power then it'll get confused. What exactly do you mean when you say "re-enabling the display power"? Power wells? > The other issue > with this check is that crtc->base.enabled is the wrong thing to check: It > only tracks sw state, e.g. it's still set when we're in dpms off mode. The > right thing to check would be crtc->active, and for that one we have the > correct ordering between get/put calls and updating the field already. No, we only set crtc->active to true after we call ironlake_crtc_enable, and that's too late for this specific bug. Also, we don't enable PC8 nor disable power wells while in DPMS off, there's a lot of work to do if we want to enable that, and this patch here is just a bug fix. > > Plan B would be to just ditch this check. The "crtc->base.enabled == enabled" check? That's not possible, it would result in unbalanced get/put calls. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
