[Intel-gfx] [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Paulo Zanoni]

2014-03-05 10:25 GMT-03:00 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Feb 21, 2014 at 01:52:21PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> Otherwise, when we run intel_modeset_check_state we may already be
>> runtime suspended, and our state checking code will read registers
>> while the device is suspended. This can only happen if your
>> autosuspend_delay_ms is low (not the default 10s).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 10ec401..c64fb7f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>>                         struct drm_display_mode *mode,
>>                         int x, int y, struct drm_framebuffer *fb)
>>  {
>> +     struct drm_device *dev = crtc->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>       int ret;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       ret = __intel_set_mode(crtc, mode, x, y, fb);
>>
>>       if (ret == 0)
>>               intel_modeset_check_state(crtc->dev);
>>
>> +     intel_runtime_pm_put(dev_priv);
>
> I've thought our code has the relevant power domain checks sprinkled all
> over the get_config/state functions already? If that's not the case I
> prefer we fix that, similar to my suggestion in reply to Imre's patches of
> moving the power domain checking into the callbacks.
>
> Wrt -fixes: Since the default autosuspend delay will make it impossible to
> hit this I think we can punt. Rules for late -rc and cc: stable is that it
> needs to be a real-world problem with a real bug report.
>
> And one more: To make pm_pc8 more useful, could we just set the
> autosuspend delay to 0 while the test is running? Then restore it again
> with an igt atexit handler. That should help with our coverage and hitting
> this tiny little issues you've fixed in this series.

We already set autosuspend_delay_msg to zero when the test is running,
we just don't restore the original value later. The problem is that we
currently have autosuspend_delay_ms and i915.pc8_timeout as non-zero
default, and we don't have support to change i915.pc8_timeout at
runtime. If we merge the series that's currently under review (00/23
Merge PC8 with runtime PM, v2), we'll just kill i915.pc8_timeout, so
we should be fine, and QA will automatically start running everything
with zero timeouts.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni