[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended
Paulo Zanoni
przanoni at gmail.com
Mon Aug 11 16:29:21 CEST 2014
2014-08-11 8:32 GMT-03:00 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> If we're runtime suspended and try to use the plane interfaces, we
>> will get a lot of WARNs saying we did the wrong thing.
>>
>> For intel_crtc_update_cursor(), all we need to do is return if the
>> CRTC is not active, since writing the registers won't really have any
>> effect if the screen is not visible, and we will write the registers
>> later when enabling the screen.
>>
>> For all the other cases, we need to get runtime PM references to
>> pin/unpin the objects, and to change the fences. The pin/unpin
>> functions are the ideal places for this, but
>> intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
>> get/put calls inside it. There is no problem if we runtime suspend
>> right after these functions are finished, because the registers
>> weitten are forwarded to system memory.
>>
>> v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
>> v3: - Make get/put also surround the fence and unpin calls (Daniel and
>> Ville).
>> - Merge all the plane changes into a single patch since they're
>> the same fix.
>> - Add the comment requested by Daniel.
>>
>> Testcase: igt/pm_rpm/cursor
>> Testcase: igt/pm_rpm/cursor-dpms
>> Testcase: igt/pm_rpm/legacy-planes
>> Testcase: igt/pm_rpm/legacy-planes-dpms
>> Testcase: igt/pm_rpm/universal-planes
>> Testcase: igt/pm_rpm/universal-planes-dpms
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4f659eb..a86d67c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
>> if (need_vtd_wa(dev) && alignment < 256 * 1024)
>> alignment = 256 * 1024;
>>
>> + /*
>> + * Global gtt pte registers are special registers which actually forward
>> + * writes to a chunk of system memory. Which means that there is no risk
>> + * that the register values disappear as soon as we call
>> + * intel_runtime_pm_put(), so it is correct to wrap only the
>> + * pin/unpin/fence and not more.
>> + */
>> + intel_runtime_pm_get(dev_priv);
>> +
>> dev_priv->mm.interruptible = false;
>> ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
>> if (ret)
>> @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
>> i915_gem_object_pin_fence(obj);
>>
>> dev_priv->mm.interruptible = true;
>> + intel_runtime_pm_put(dev_priv);
>> return 0;
>>
>> err_unpin:
>> i915_gem_object_unpin_from_display_plane(obj);
>> err_interruptible:
>> dev_priv->mm.interruptible = true;
>> + intel_runtime_pm_put(dev_priv);
>> return ret;
>> }
>>
>> void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
>> {
>> - WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>> + struct drm_device *dev = obj->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> +
>> + intel_runtime_pm_get(dev_priv);
>>
>> i915_gem_object_unpin_fence(obj);
>> i915_gem_object_unpin_from_display_plane(obj);
>> +
>> + intel_runtime_pm_put(dev_priv);
>
> I don't think we touch the hardware during unpin so these aren't
> strictly needed.
>
Daniel requested them.
>> }
>>
>> /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
>> @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>> if (base == 0 && intel_crtc->cursor_base == 0)
>> return;
>>
>> + if (!intel_crtc->active)
>> + return;
>> +
>
> Did you actually manage to get by the base==0 check above with a
> disabled pipe? I don't think this should happen.
Yes, since we enabled runtime suspend during DPMS. Remember that
crtc->active != crtc->enabled.
To hit this return, just run "sudo ./pm_rpm --run-subtest cursor-dpms".
>
>> I915_WRITE(CURPOS(pipe), pos);
>>
>> if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
>> @@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>
>> /* we only need to pin inside GTT if cursor is non-phy */
>> mutex_lock(&dev->struct_mutex);
>> +
>> + /*
>> + * Global gtt pte registers are special registers which actually forward
>> + * writes to a chunk of system memory. Which means that there is no risk
>> + * that the register values disappear as soon as we call
>> + * intel_runtime_pm_put(), so it is correct to wrap only the
>> + * pin/unpin/fence and not more.
>> + */
>> + intel_runtime_pm_get(dev_priv);
>> +
>> if (!INTEL_INFO(dev)->cursor_needs_physical) {
>> unsigned alignment;
>>
>> +
>
> Spurious whitespace
Oops... Will fix.
>
> And now that I look at our cursor code I see we're doing dubious stuff
> like unpinning the old bo before even writing the cursor registers to
> use the new bo (really we should wait for vblank before unpinning). But
> that's just something else we have to fix at some point.
Yeah, these should be in other patches.
Thanks,
Paulo
>
>> if (obj->tiling_mode) {
>> DRM_DEBUG_KMS("cursor cannot be tiled\n");
>> ret = -EINVAL;
>> @@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>
>> i915_gem_track_fb(intel_crtc->cursor_bo, obj,
>> INTEL_FRONTBUFFER_CURSOR(pipe));
>> +
>> + if (obj)
>> + intel_runtime_pm_put(dev_priv);
>> +
>> mutex_unlock(&dev->struct_mutex);
>>
>> old_width = intel_crtc->cursor_width;
>> @@ -8413,6 +8449,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>> fail_unpin:
>> i915_gem_object_unpin_from_display_plane(obj);
>> fail_locked:
>> + intel_runtime_pm_put(dev_priv);
>> mutex_unlock(&dev->struct_mutex);
>> fail:
>> drm_gem_object_unreference_unlocked(&obj->base);
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Paulo Zanoni
More information about the Intel-gfx
mailing list