From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable Date: Fri, 20 Sep 2013 10:10:55 +0300 Message-ID: <20130920071055.GM4531@intel.com> References: <1379342317-27690-1-git-send-email-ville.syrjala@linux.intel.com> <1379342317-27690-7-git-send-email-ville.syrjala@linux.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 D002FE5D18 for ; Fri, 20 Sep 2013 00:10:58 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 19, 2013 at 07:05:14PM -0300, Paulo Zanoni wrote: > 2013/9/16 : > > From: Ville Syrj=E4l=E4 > > > > VGA registers live inside the power well on HSW, so in order to write > > the VGA MSR register we need the power well to be on. > > > > We really must write to the register to properly clear the > > VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when > > the power well is down. It seems that the implicit zeroing done by > > the power well is not enough to propagate the VGA_MSR_MEM_EN bit to > > whomever is actually responsible for the memory decode ranges. > > > > If we leave VGA memory decode enabled, and then turn off the power well, > > all VGA memory reads will return zeroes. But if we first disable VGA > > memory deocde and then turn off the power well, VGA memory reads > > return all ones, indicating that the access wasn't claimed by anyone. > > For the vga arbiter to function correctly the IGD must not claim the > > VGA memory accesses. > > > > Previously we were doing the VGA_MSR register access while the power we= ll > > was excplicitly powered up during driver init. But ever since > > > > commit 6e1b4fdad5157bb9e88777d525704aba24389bee > > Author: Ville Syrj=E4l=E4 > > Date: Thu Sep 5 20:40:52 2013 +0300 > > > > drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff= is done > > > > we delay the VGA memory disable until fbcon has initialized, and so > > there's a possibility that the power well got turned off during the > > fbcon modeset. Also vgacon_save_screen() will need the power well to be > > on to be able to read the VGA memory. > > > > So immediately after enabling the power well during init grab a refence > > for VGA purposes, and after all the VGA handling is done, release it. > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/i915_dma.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i91= 5_dma.c > > index e5c7b10..23f965d 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_devi= ce *dev) > > > > intel_init_power_well(dev); > > > > + /* Keep VGA alive until i915_disable_vga_mem() */ > > + intel_display_power_get(dev, POWER_DOMAIN_VGA); > > + > > intel_modeset_gem_init(dev); > > > > /* Always safe in the mode setting case. */ > > @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_devi= ce *dev) > > * vgacon_save_screen() works during the handover. > > */ > > i915_disable_vga_mem(dev); > > + intel_display_power_put(dev, POWER_DOMAIN_VGA); > = > There's a "return 0" between the get and the put. (I was already at > patch 8 when I realized that!) Ugh. That whole 0 pipes thing seems rather wonky. Does that thing even have many of the display resources we touch before the return 0 there? I guess it's not a real problem for HSW at this point since there aren't HSWs w/o the display block yet. But I'll add a put call there just to keep the resemblence of sanity for that case. > = > While reviewing your series I started thinking: shouldn't we just > get/put POWER_DOMAIN_VGA directly inside the functions that touch the > VGA code? This shouldn't really be an expensive thing, and it will > make our code simpler and harder to break. While your series seems > correct, there is code that depends on state left by other code, which > IMHO increases the complexity of the already-too-complex init/resume > paths. This is just a suggestion, not a requirement :) We need VGA to stay alive (and hence the power well) the whole time until i915_disable_vga_mem() is called. I didn't want to put the get/put calls inside some random functions we call during init since then it becomes much harder to see the relationship between the get/put calls. > = > > > > /* Only enable hotplug handling once the fbdev is fully set up.= */ > > dev_priv->enable_hotplug_processing =3D true; > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC