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. 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). 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. And if the work runs concurrently to re-enabling the display power then it'll get confused. 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. Plan B would be to just ditch this check. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
