Re: [PATCH 04/19] drm/i915: get/put PC8 when we get/put a CRTC

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 21, 2013 at 01:47:18PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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@intel.com>

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