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.vet...@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
