| Message ID | 1380236762-1698-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 26, 2013 at 08:05:58PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > VGA whack-a-mole! > > We need VGA to be disabled whenever our driver is working. So even > without reproducible bugs this patch makes sense, but we do have a bug > solved by this patch. > > If you boot a Haswell machine with eDP+HDMI, then kill your display > manager and run: > echo 0 > /sys/class/vtconsole/vtcon1/bind > you'll get thousands of "unclaimed register" messages. Notice that > since we have eDP+HDMI, the power well is *enabled* when we run the > command, but if you look at dmesg you'll see that at some point during > the boot we disabled it and then reenabled it. This patch solves this > problem. > > I didn't do a deep analysis of the problem, but I guess vgacon gets > seriously confused when it sees that the VGA plane is enabled. > > Besides the command above, this problem can also be reproduced by the > "module_reload" test from intel-gpu-tools. > > Fixes regression introduced by: > commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41 > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > drm/i915: switch disable_power_well default value to 1 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813 > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 665fa8f..065ffed 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev) > } > > /* Disable the VGA plane that we never use */ > -static void i915_disable_vga(struct drm_device *dev) > +void i915_disable_vga(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > u8 sr1; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a17a86a..e63646a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -678,6 +678,7 @@ void > ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, > int dotclock); > bool intel_crtc_active(struct drm_crtc *crtc); > +void i915_disable_vga(struct drm_device *dev); > void i915_disable_vga_mem(struct drm_device *dev); > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2ac1c2f..7a8af95 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable) > if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & > HSW_PWR_WELL_STATE_ENABLED), 20)) > DRM_ERROR("Timeout enabling power well\n"); > + > + if (I915_READ(i915_vgacntrl_reg(dev)) != > + VGA_DISP_DISABLE) { > + i915_disable_vga(dev); > + i915_disable_vga_mem(dev); > + } Hmm. That makes no sense. So somehow the thrice cursed BIOS re-enabled the VGA plane behind our back? That really shouldn't cause any problems other than maybe corrupting the display or something. In any case it has nothing to do with vga resource decode. The real problem seems to be that vgacon is poking at vga resources we apparently decode but can't handle. If your patch really fixes it then it would seem to BIOS also enabled VGA memory decode. But that still doesn't explain the unclaimed register errors, as that would require the power well to be off. But since you say HDMI is enabled, the power well must be on as well. So all I can say is wtf? > } > } else { > if (enable_requested) { > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 01, 2013 at 04:47:04PM +0300, Ville Syrjälä wrote: > On Thu, Sep 26, 2013 at 08:05:58PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > VGA whack-a-mole! > > > > We need VGA to be disabled whenever our driver is working. So even > > without reproducible bugs this patch makes sense, but we do have a bug > > solved by this patch. > > > > If you boot a Haswell machine with eDP+HDMI, then kill your display > > manager and run: > > echo 0 > /sys/class/vtconsole/vtcon1/bind > > you'll get thousands of "unclaimed register" messages. Notice that > > since we have eDP+HDMI, the power well is *enabled* when we run the > > command, but if you look at dmesg you'll see that at some point during > > the boot we disabled it and then reenabled it. This patch solves this > > problem. > > > > I didn't do a deep analysis of the problem, but I guess vgacon gets > > seriously confused when it sees that the VGA plane is enabled. > > > > Besides the command above, this problem can also be reproduced by the > > "module_reload" test from intel-gpu-tools. > > > > Fixes regression introduced by: > > commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41 > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > > drm/i915: switch disable_power_well default value to 1 > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813 > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 665fa8f..065ffed 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev) > > } > > > > /* Disable the VGA plane that we never use */ > > -static void i915_disable_vga(struct drm_device *dev) > > +void i915_disable_vga(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > u8 sr1; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index a17a86a..e63646a 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -678,6 +678,7 @@ void > > ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, > > int dotclock); > > bool intel_crtc_active(struct drm_crtc *crtc); > > +void i915_disable_vga(struct drm_device *dev); > > void i915_disable_vga_mem(struct drm_device *dev); > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 2ac1c2f..7a8af95 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable) > > if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & > > HSW_PWR_WELL_STATE_ENABLED), 20)) > > DRM_ERROR("Timeout enabling power well\n"); > > + > > + if (I915_READ(i915_vgacntrl_reg(dev)) != > > + VGA_DISP_DISABLE) { > > + i915_disable_vga(dev); > > + i915_disable_vga_mem(dev); > > + } > > Hmm. That makes no sense. > > So somehow the thrice cursed BIOS re-enabled the VGA plane behind our > back? That really shouldn't cause any problems other than maybe > corrupting the display or something. In any case it has nothing to do > with vga resource decode. > > The real problem seems to be that vgacon is poking at vga resources we > apparently decode but can't handle. If your patch really fixes it then > it would seem to BIOS also enabled VGA memory decode. But that still > doesn't explain the unclaimed register errors, as that would require > the power well to be off. But since you say HDMI is enabled, the power > well must be on as well. So all I can say is wtf? Actually I tried it on my HSW, and the vga_cntrl reg always comes back as 0x80002900 after the power well has been off, which is the reset value. So your code w/ the '!=' comparoson would assume that the VGA plane is enabled even though it might not be. We should fix all the VGA plane enabled checks to use '&' instead of '!='. But if your patch actually fixes things, then I guess the BIOS really must have re-enabled vga memory decode and that's where the errors are coming from. Otherwise if it's just about vga registers, then redisabling vga memory decode and vga plane should have no effect on the outcome. One idea would be to lift the lid_notify hack from intel_lvds.c and plug it into eDP and see if that makes a difference...
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 665fa8f..065ffed 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev) } /* Disable the VGA plane that we never use */ -static void i915_disable_vga(struct drm_device *dev) +void i915_disable_vga(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; u8 sr1; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a17a86a..e63646a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -678,6 +678,7 @@ void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, int dotclock); bool intel_crtc_active(struct drm_crtc *crtc); +void i915_disable_vga(struct drm_device *dev); void i915_disable_vga_mem(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2ac1c2f..7a8af95 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable) if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED), 20)) DRM_ERROR("Timeout enabling power well\n"); + + if (I915_READ(i915_vgacntrl_reg(dev)) != + VGA_DISP_DISABLE) { + i915_disable_vga(dev); + i915_disable_vga_mem(dev); + } } } else { if (enable_requested) {