On Mon, Aug 11, 2014 at 02:57:44PM -0300, Paulo Zanoni wrote: > 2014-08-11 11:42 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > > 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. > > Why? I am not very familiar with the cursor code and I don't see > what's the problem here. Just feels like duct tape for something that should have been caught by some earlier piece of code. > Please explain more: what exactly is the > problem, where is it and what is your suggested solution? I think this ought to fix it, and is exactly what we do with other planes: diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9b578e..c9f733d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11699,8 +11699,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, }; const struct drm_rect clip = { /* integer pixels */ - .x2 = intel_crtc->config.pipe_src_w, - .y2 = intel_crtc->config.pipe_src_h, + .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0, + .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0, }; bool visible; int ret; > Also, notice > that this is a patch to -stable, so I don't think we should block this > fix based on complicated rework ideas, or something that won't really > apply to kernels older than 5 hours. > > When I look at the backtrace, I see that we are already calling these > functions through drm_mode_cursor_universal(), so this should already > be handling the cursor plane in the same way it handles the other > planes. > > Also, I invoke Daniel to discuss here since he's the one that > introduced the difference between ->active and ->enabled. > > > > > -- > > Ville Syrjälä > > Intel OTC > > > > -- > Paulo Zanoni -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
