[Intel-gfx] [PATCH] drm/i915: Handle runtime pm in the CRC setup code

[Daniel Vetter]

On Mon, Nov 24, 2014 at 4:36 PM, Damien Lespiau
<damien.lespiau at intel.com> 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 at 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?

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