On Mon, Aug 11, 2014 at 11:29:21AM -0300, Paulo Zanoni wrote: > 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. Then I think there's a bug somewhere a bit earlier. We should consider the cursor to be invisible when crtc->active==false. That's how we deal with all other planes. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
