[PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Keith Packard]

We want to disable the (unused) VGA plane on the intel hardware, which
should be a simple matter of writing the vga control register. However,
in 2009 (commit 24f119c769bacac5729297b682fec7811a983cc6), that simple
code was changed to also smash the SR01 VGA register to fix "random
crash and lockups".

When running efifb on an nVidia card, executing this store to SR01
ends up causing the screen to go black.

I'm not sure if this store is still required; this patch limits it to
when the intel card is primary.

Signed-off-by: Keith Packard <keithp@keithp.com>
---

 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c714d4d..545b271 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9038,12 +9038,14 @@ static void i915_disable_vga(struct drm_device *dev)
 	u8 sr1;
 	u32 vga_reg = i915_vgacntrl_reg(dev);
 
-	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-	outb(SR01, VGA_SR_INDEX);
-	sr1 = inb(VGA_SR_DATA);
-	outb(sr1 | 1<<5, VGA_SR_DATA);
-	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
-	udelay(300);
+	if (dev->pdev == vga_default_device()) {
+		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
+		outb(SR01, VGA_SR_INDEX);
+		sr1 = inb(VGA_SR_DATA);
+		outb(sr1 | 1<<5, VGA_SR_DATA);
+		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+		udelay(300);
+	}
 
 	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
 	POSTING_READ(vga_reg);
-- 
1.8.5.1

[PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Keith Packard]

We want to disable the (unused) VGA plane on the intel hardware, which
should be a simple matter of writing the vga control register. However,
in 2009 (commit 24f119c769bacac5729297b682fec7811a983cc6), that simple
code was changed to also smash the SR01 VGA register to fix "random
crash and lockups".

When running efifb on an nVidia card, executing this store to SR01
ends up causing the screen to go black.

I'm not sure if this store is still required; this patch limits it to
when the intel card is primary.

Signed-off-by: Keith Packard <keithp@keithp.com>
---

 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c714d4d..545b271 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9038,12 +9038,14 @@ static void i915_disable_vga(struct drm_device *dev)
 	u8 sr1;
 	u32 vga_reg = i915_vgacntrl_reg(dev);
 
-	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
-	outb(SR01, VGA_SR_INDEX);
-	sr1 = inb(VGA_SR_DATA);
-	outb(sr1 | 1<<5, VGA_SR_DATA);
-	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
-	udelay(300);
+	if (dev->pdev == vga_default_device()) {
+		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
+		outb(SR01, VGA_SR_INDEX);
+		sr1 = inb(VGA_SR_DATA);
+		outb(sr1 | 1<<5, VGA_SR_DATA);
+		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+		udelay(300);
+	}
 
 	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
 	POSTING_READ(vga_reg);
-- 
1.8.5.1

Re: [Intel-gfx] [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Chris Wilson]

On Tue, Dec 17, 2013 at 09:12:10AM -0800, Keith Packard wrote:
> We want to disable the (unused) VGA plane on the intel hardware, which
> should be a simple matter of writing the vga control register. However,
> in 2009 (commit 24f119c769bacac5729297b682fec7811a983cc6), that simple
> code was changed to also smash the SR01 VGA register to fix "random
> crash and lockups".
> 
> When running efifb on an nVidia card, executing this store to SR01
> ends up causing the screen to go black.
> 
> I'm not sure if this store is still required; this patch limits it to
> when the intel card is primary.

The bspec still says we must assert SR01 bit5 prior to disabling the VGA
plane.

Perhaps the test should be whether (vga_reg & VGA_DISP_DISABLE) == 0 and
do nothing if the plane is already off.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Chris Wilson]

On Tue, Dec 17, 2013 at 09:12:10AM -0800, Keith Packard wrote:
> We want to disable the (unused) VGA plane on the intel hardware, which
> should be a simple matter of writing the vga control register. However,
> in 2009 (commit 24f119c769bacac5729297b682fec7811a983cc6), that simple
> code was changed to also smash the SR01 VGA register to fix "random
> crash and lockups".
> 
> When running efifb on an nVidia card, executing this store to SR01
> ends up causing the screen to go black.
> 
> I'm not sure if this store is still required; this patch limits it to
> when the intel card is primary.

The bspec still says we must assert SR01 bit5 prior to disabling the VGA
plane.

Perhaps the test should be whether (vga_reg & VGA_DISP_DISABLE) == 0 and
do nothing if the plane is already off.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The bspec still says we must assert SR01 bit5 prior to disabling the VGA
> plane.
>
> Perhaps the test should be whether (vga_reg & VGA_DISP_DISABLE) == 0 and
> do nothing if the plane is already off.

The problem is that for some reason we're smashing *some other video
card* when it's being used via efifb.

I'm wondering if vgaarb just doesn't work because efifb isn't telling
vgaarb that it's using those registers (I mean, how would it even know?)

The other simple option is to just not disable VGA if the card isn't
primary; presumably it wasn't ever enabled.

And, yes, I know that the card probably won't work at all if it isn't
primary because so much currently depends on the BIOS setting up bits of
the card that we can't autodetect. So, another simple option would be to
just refuse to load the driver if the card is secondary...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Chris Wilson]

On Tue, Dec 17, 2013 at 10:17:24AM -0800, Keith Packard wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The bspec still says we must assert SR01 bit5 prior to disabling the VGA
> > plane.
> >
> > Perhaps the test should be whether (vga_reg & VGA_DISP_DISABLE) == 0 and
> > do nothing if the plane is already off.
> 
> The problem is that for some reason we're smashing *some other video
> card* when it's being used via efifb.

Ok, so as no vgaarb_clients have yet been registered and so the call to
grab the IO resource does not actually disable VGA IO routing to the
nvidia card.
 
> I'm wondering if vgaarb just doesn't work because efifb isn't telling
> vgaarb that it's using those registers (I mean, how would it even know?)

Agreed, that does seem to be the nub of the problem.
 
> The other simple option is to just not disable VGA if the card isn't
> primary; presumably it wasn't ever enabled.
> 
> And, yes, I know that the card probably won't work at all if it isn't
> primary because so much currently depends on the BIOS setting up bits of
> the card that we can't autodetect. So, another simple option would be to
> just refuse to load the driver if the card is secondary...

If you care to update the changelog to explain the problem is that
vgaarb is ineffective before all clients are registered, then I think
this is a good temporary hack. It should be possible for vgaarb to mark
resources as locked if the device is interpretting IO access and has no
method for disabling the IO grab (and then a vga_tryget() check here).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Chris Wilson]

On Tue, Dec 17, 2013 at 10:17:24AM -0800, Keith Packard wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The bspec still says we must assert SR01 bit5 prior to disabling the VGA
> > plane.
> >
> > Perhaps the test should be whether (vga_reg & VGA_DISP_DISABLE) == 0 and
> > do nothing if the plane is already off.
> 
> The problem is that for some reason we're smashing *some other video
> card* when it's being used via efifb.

Ok, so as no vgaarb_clients have yet been registered and so the call to
grab the IO resource does not actually disable VGA IO routing to the
nvidia card.
 
> I'm wondering if vgaarb just doesn't work because efifb isn't telling
> vgaarb that it's using those registers (I mean, how would it even know?)

Agreed, that does seem to be the nub of the problem.
 
> The other simple option is to just not disable VGA if the card isn't
> primary; presumably it wasn't ever enabled.
> 
> And, yes, I know that the card probably won't work at all if it isn't
> primary because so much currently depends on the BIOS setting up bits of
> the card that we can't autodetect. So, another simple option would be to
> just refuse to load the driver if the card is secondary...

If you care to update the changelog to explain the problem is that
vgaarb is ineffective before all clients are registered, then I think
this is a good temporary hack. It should be possible for vgaarb to mark
resources as locked if the device is interpretting IO access and has no
method for disabling the IO grab (and then a vga_tryget() check here).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Ok, so as no vgaarb_clients have yet been registered and so the call to
> grab the IO resource does not actually disable VGA IO routing to the
> nvidia card.

Yikes! This explains a lot.

> If you care to update the changelog to explain the problem is that
> vgaarb is ineffective before all clients are registered, then I think
> this is a good temporary hack. It should be possible for vgaarb to mark
> resources as locked if the device is interpretting IO access and has no
> method for disabling the IO grab (and then a vga_tryget() check here).

Sounds like vgaarb should not assume that all devices using VGA are
registered drivers. That also sounds like a significantly harder fix.

If SR01 is required before disabling VGA, then perhaps we should not
disable VGA at all in this case though. That sounds safer, although if
VGA was enabled, it will suck a bit more power?

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] drm/intel: Only smash VGA SR01 register if intel is default VGA device

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 981 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Ok, so as no vgaarb_clients have yet been registered and so the call to
> grab the IO resource does not actually disable VGA IO routing to the
> nvidia card.

Yikes! This explains a lot.

> If you care to update the changelog to explain the problem is that
> vgaarb is ineffective before all clients are registered, then I think
> this is a good temporary hack. It should be possible for vgaarb to mark
> resources as locked if the device is interpretting IO access and has no
> method for disabling the IO grab (and then a vga_tryget() check here).

Sounds like vgaarb should not assume that all devices using VGA are
registered drivers. That also sounds like a significantly harder fix.

If SR01 is required before disabling VGA, then perhaps we should not
disable VGA at all in this case though. That sounds safer, although if
VGA was enabled, it will suck a bit more power?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx