From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: fix plane/cursor handling when runtime suspended Date: Tue, 12 Aug 2014 11:20:40 +0300 Message-ID: <20140812082040.GS4193@intel.com> References: <1407360362-1774-1-git-send-email-przanoni@gmail.com> <20140811113231.GM4193@intel.com> <20140811144209.GQ4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 0BCD16E2C4 for ; Tue, 12 Aug 2014 01:20:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Daniel Vetter , Intel Graphics Development , Paulo Zanoni , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Aug 11, 2014 at 02:57:44PM -0300, Paulo Zanoni wrote: > 2014-08-11 11:42 GMT-03:00 Ville Syrj=E4l=E4 : > > On Mon, Aug 11, 2014 at 11:29:21AM -0300, Paulo Zanoni wrote: > >> 2014-08-11 8:32 GMT-03:00 Ville Syrj=E4l=E4 : > >> > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: > >> >> From: Paulo Zanoni > >> >> > >> >> 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 a= ny > >> >> 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 a= dd > >> >> 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() (Dani= el) > >> >> 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=3D81645 > >> >> Cc: stable@vger.kernel.org > >> >> Signed-off-by: Paulo Zanoni > >> >> --- > >> >> 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 =3D 256 * 1024; > >> >> > >> >> + /* > >> >> + * Global gtt pte registers are special registers which actua= lly 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 =3D false; > >> >> ret =3D i915_gem_object_pin_to_display_plane(obj, alignment, = pipelined); > >> >> if (ret) > >> >> @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_devic= e *dev, > >> >> i915_gem_object_pin_fence(obj); > >> >> > >> >> dev_priv->mm.interruptible =3D 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 =3D 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 =3D obj->base.dev; > >> >> + struct drm_i915_private *dev_priv =3D 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. b= ytes per pixel > >> >> @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct d= rm_crtc *crtc, > >> >> if (base =3D=3D 0 && intel_crtc->cursor_base =3D=3D 0) > >> >> return; > >> >> > >> >> + if (!intel_crtc->active) > >> >> + return; > >> >> + > >> > > >> > Did you actually manage to get by the base=3D=3D0 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 !=3D crtc->enabled. > > > > Then I think there's a bug somewhere a bit earlier. We should consider > > the cursor to be invisible when crtc->active=3D=3Dfalse. 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/in= tel_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 =3D { /* integer pixels */ - .x2 =3D intel_crtc->config.pipe_src_w, - .y2 =3D intel_crtc->config.pipe_src_h, + .x2 =3D intel_crtc->active ? intel_crtc->config.pipe_src_w = : 0, + .y2 =3D 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=E4l=E4 > > Intel OTC > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC