On Mon, Nov 24, 2014 at 4:36 PM, Damien Lespiau <damien.lespiau@xxxxxxxxx> wrote: > 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@xxxxxxxxx> >> --- >> 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? intel_displaye_power_is_enabled needs it. I'm not sure whether we should move the check for rpm into it or not ... >> + 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(); Oops, will fix. -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
