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

[Chris Wilson]

On Thu, Nov 21, 2013 at 01:47:18PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at 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 at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5ea32a8..846f2de 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9124,6 +9124,24 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  	return false;
>  }
>  
> +/* Sets crtc->base.enabled and gets/puts whatever resources are needed by the
> + * CRTC. */
> +static void intel_crtc_set_state(struct intel_crtc *crtc, bool enabled)
set_state is too generic a term, intel_crtc_set_enabled()

> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (enabled == crtc->base.enabled)
> +		return;
> +
> +	if (enabled)
> +		hsw_disable_package_c8(dev_priv);
> +	else
> +		hsw_enable_package_c8(dev_priv);

We can reduce the code slightly if this was also
hsw_package_c8_set_enabled().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre