2014-08-11 8:32 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxx >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
