Re: [PATCH] drm/i915: Handle runtime pm in the CRC setup code

[Damien Lespiau <damien.lespiau@intel.com>]

On Mon, Nov 24, 2014 at 04:23:36PM +0100, Daniel Vetter wrote:
> The crc code doesn't handle anything really that could drop the
> register state (by design so that we have less complexity). Which
> means userspace may only start crc capture once the pipe is fully set
> up.
> 
> With an i-g-t patch this will be the case, but there's still the
> problem that this results in obscure unclaimed register write
> failures. Which is a pain to debug.
> 
> So instead make sure we don't have the basic unclaimed register write
> failure by grabbing runtime pm references. And reject completely
> invalid requests with -EIO. This is still racy of course, but for a
> test library we don't really care - if userspace shuts down the pipe
> right afterwards the entire setup will be lost anyway.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=86092
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 319da61354b0..68a76e58e8fd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3309,6 +3309,14 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  	if (pipe_crc->source && source)
>  		return -EINVAL;
>  
> +	intel_runtime_pm_get(dev_priv);


Do we need this intel_runtime_pm_get() here?

> +	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe))) {
> +		DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
> +		ret = -EIO;
> +		goto out;
> +	}
> +
>  	if (IS_GEN2(dev))
>  		ret = i8xx_pipe_crc_ctl_reg(&source, &val);
>  	else if (INTEL_INFO(dev)->gen < 5)
> @@ -3321,7 +3329,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>  
>  	if (ret != 0)
> -		return ret;
> +		goto out;
>  
>  	/* none -> real source transition */
>  	if (source) {
> @@ -3331,8 +3339,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		pipe_crc->entries = kzalloc(sizeof(*pipe_crc->entries) *
>  					    INTEL_PIPE_CRC_ENTRIES_NR,
>  					    GFP_KERNEL);
> -		if (!pipe_crc->entries)
> -			return -ENOMEM;
> +		if (!pipe_crc->entries) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  
>  		/*
>  		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> @@ -3383,8 +3393,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  
>  		hsw_enable_ips(crtc);
>  	}
> +out:
> +	intel_runtime_pm_get(dev_priv);


That's a second intel_runtime_pm_get();

>  
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx