linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] i915: Update VGA arbiter support for newer devices
@ 2013-08-24 14:53 Alex Williamson
  2013-08-26 12:23 ` Ville Syrjälä
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2013-08-24 14:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: airlied, linux-kernel, ville.syrjala

This is intended to add VGA arbiter support for Intel HD graphics on
Core processors.  The old GMCH registers no longer exist, so even
though it appears that i915 participates in VGA arbitration, it doesn't
work.  On Intel HD graphics we already attempt to disable VGA regions
of the device.  This makes registering as a VGA client unnecessary since
we don't intend to operate differently depending on how many VGA devices
are present.  We can disable VGA memory regions by clearing the memory
enable bit in the VGA MSR.  That only leaves VGA IO, which we update
the VGA arbiter to know that we don't participate in VGA memory
arbitration.  We also add a hook on unload to re-enable memory and
reinstate VGA memory arbitration.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

v2: I915_READ/WRITE accessors don't work in i915_disable_vga, use inb/outb
    directly.  Also, on the driver unbind VGA enable path, acquire legacy
    IO to re-enable VGA memory.  Correct comment.

As with v1, this depends on "vgaarb: Fixes for partial VGA opt-out".  With
all patches I'm able to assign a discrete PEG VGA device to a guest and
execute the VBIOS w/o interference from IGD or corruption of the IGD
framebuffer.

 drivers/gpu/drm/i915/i915_dma.c      |    9 ++++++---
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f466980..d9cf216 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1287,9 +1287,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * then we do not take part in VGA arbitration and the
 	 * vga_client_register() fails with -ENODEV.
 	 */
-	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
-	if (ret && ret != -ENODEV)
-		goto out;
+	if (!HAS_PCH_SPLIT(dev)) {
+		ret = vga_client_register(dev->pdev, dev, NULL,
+					  i915_vga_set_decode);
+		if (ret && ret != -ENODEV)
+			goto out;
+	}
 
 	intel_register_dsm_handler();
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5fb3058..2bb805c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9519,6 +9519,15 @@ static void i915_disable_vga(struct drm_device *dev)
 	outb(SR01, VGA_SR_INDEX);
 	sr1 = inb(VGA_SR_DATA);
 	outb(sr1 | 1<<5, VGA_SR_DATA);
+
+	/* Disable VGA memory on Intel HD */
+	if (HAS_PCH_SPLIT(dev)) {
+		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
+						   VGA_RSRC_NORMAL_IO |
+						   VGA_RSRC_NORMAL_MEM);
+	}
+
 	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
 	udelay(300);
 
@@ -9526,6 +9535,19 @@ static void i915_disable_vga(struct drm_device *dev)
 	POSTING_READ(vga_reg);
 }
 
+static void i915_enable_vga(struct drm_device *dev)
+{
+	/* Enable VGA memory on Intel HD */
+	if (HAS_PCH_SPLIT(dev)) {
+		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
+		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
+		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_MASK |
+						   VGA_RSRC_NORMAL_IO |
+						   VGA_RSRC_NORMAL_MEM);
+		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+	}
+}
+
 void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_init_power_well(dev);
@@ -9983,6 +10005,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_fbc(dev);
 
+	i915_enable_vga(dev);
+
 	intel_disable_gt_powersave(dev);
 
 	ironlake_teardown_rc6(dev);


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] i915: Update VGA arbiter support for newer devices
  2013-08-24 14:53 [PATCH v2] i915: Update VGA arbiter support for newer devices Alex Williamson
@ 2013-08-26 12:23 ` Ville Syrjälä
  2013-08-26 16:05   ` Alex Williamson
  0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjälä @ 2013-08-26 12:23 UTC (permalink / raw)
  To: Alex Williamson; +Cc: intel-gfx, airlied, linux-kernel

On Sat, Aug 24, 2013 at 08:53:40AM -0600, Alex Williamson wrote:
> This is intended to add VGA arbiter support for Intel HD graphics on
> Core processors.  The old GMCH registers no longer exist, so even
> though it appears that i915 participates in VGA arbitration, it doesn't
> work.  On Intel HD graphics we already attempt to disable VGA regions
> of the device.  This makes registering as a VGA client unnecessary since
> we don't intend to operate differently depending on how many VGA devices
> are present.  We can disable VGA memory regions by clearing the memory
> enable bit in the VGA MSR.  That only leaves VGA IO, which we update
> the VGA arbiter to know that we don't participate in VGA memory
> arbitration.  We also add a hook on unload to re-enable memory and
> reinstate VGA memory arbitration.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> v2: I915_READ/WRITE accessors don't work in i915_disable_vga, use inb/outb
>     directly.  Also, on the driver unbind VGA enable path, acquire legacy
>     IO to re-enable VGA memory.  Correct comment.
> 
> As with v1, this depends on "vgaarb: Fixes for partial VGA opt-out".  With
> all patches I'm able to assign a discrete PEG VGA device to a guest and
> execute the VBIOS w/o interference from IGD or corruption of the IGD
> framebuffer.

I glanced through those before, but I'll try to do a more thorough
review soon.

> 
>  drivers/gpu/drm/i915/i915_dma.c      |    9 ++++++---
>  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f466980..d9cf216 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1287,9 +1287,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	 * then we do not take part in VGA arbitration and the
>  	 * vga_client_register() fails with -ENODEV.
>  	 */
> -	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
> -	if (ret && ret != -ENODEV)
> -		goto out;
> +	if (!HAS_PCH_SPLIT(dev)) {
> +		ret = vga_client_register(dev->pdev, dev, NULL,
> +					  i915_vga_set_decode);
> +		if (ret && ret != -ENODEV)
> +			goto out;
> +	}

I think we should do something to fix i915_redisable_vga() for !pch
platforms too. The MSR method should work for everything, so I'm leaning
towards using it all the time. The other option would be add an mmio
codepath into intel_disable_vga(), but then we have several codepaths
to worry about.

>  
>  	intel_register_dsm_handler();
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5fb3058..2bb805c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9519,6 +9519,15 @@ static void i915_disable_vga(struct drm_device *dev)
>  	outb(SR01, VGA_SR_INDEX);
>  	sr1 = inb(VGA_SR_DATA);
>  	outb(sr1 | 1<<5, VGA_SR_DATA);
> +
> +	/* Disable VGA memory on Intel HD */
> +	if (HAS_PCH_SPLIT(dev)) {
> +		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> +						   VGA_RSRC_NORMAL_IO |
> +						   VGA_RSRC_NORMAL_MEM);
> +	}
> +
>  	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
>  	udelay(300);
>  
> @@ -9526,6 +9535,19 @@ static void i915_disable_vga(struct drm_device *dev)
>  	POSTING_READ(vga_reg);
>  }
>  
> +static void i915_enable_vga(struct drm_device *dev)
> +{

Not sure we need this actually. We don't re-enable the VGA display
on unload, so you'll never see anything on the display anyway. But
then again, I don't see any real harm in doing this either.

> +	/* Enable VGA memory on Intel HD */
> +	if (HAS_PCH_SPLIT(dev)) {
> +		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> +		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);

I was thinking we might save the MSR state during driver load and
restore to that same state, but it could be a bit overkill.

> +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_MASK |

Maybe spell out IO and MEM for legacy too. Otherwise it looks a
bit cryptic.

> +						   VGA_RSRC_NORMAL_IO |
> +						   VGA_RSRC_NORMAL_MEM);
> +		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> +	}
> +}
> +
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
>  	intel_init_power_well(dev);
> @@ -9983,6 +10005,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_fbc(dev);
>  
> +	i915_enable_vga(dev);
> +
>  	intel_disable_gt_powersave(dev);
>  
>  	ironlake_teardown_rc6(dev);

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] i915: Update VGA arbiter support for newer devices
  2013-08-26 12:23 ` Ville Syrjälä
@ 2013-08-26 16:05   ` Alex Williamson
  2013-08-26 17:47     ` Ville Syrjälä
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2013-08-26 16:05 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, airlied, linux-kernel

On Mon, 2013-08-26 at 15:23 +0300, Ville Syrjälä wrote:
> On Sat, Aug 24, 2013 at 08:53:40AM -0600, Alex Williamson wrote:
> > This is intended to add VGA arbiter support for Intel HD graphics on
> > Core processors.  The old GMCH registers no longer exist, so even
> > though it appears that i915 participates in VGA arbitration, it doesn't
> > work.  On Intel HD graphics we already attempt to disable VGA regions
> > of the device.  This makes registering as a VGA client unnecessary since
> > we don't intend to operate differently depending on how many VGA devices
> > are present.  We can disable VGA memory regions by clearing the memory
> > enable bit in the VGA MSR.  That only leaves VGA IO, which we update
> > the VGA arbiter to know that we don't participate in VGA memory
> > arbitration.  We also add a hook on unload to re-enable memory and
> > reinstate VGA memory arbitration.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > v2: I915_READ/WRITE accessors don't work in i915_disable_vga, use inb/outb
> >     directly.  Also, on the driver unbind VGA enable path, acquire legacy
> >     IO to re-enable VGA memory.  Correct comment.
> > 
> > As with v1, this depends on "vgaarb: Fixes for partial VGA opt-out".  With
> > all patches I'm able to assign a discrete PEG VGA device to a guest and
> > execute the VBIOS w/o interference from IGD or corruption of the IGD
> > framebuffer.
> 
> I glanced through those before, but I'll try to do a more thorough
> review soon.

Thanks!

> > 
> >  drivers/gpu/drm/i915/i915_dma.c      |    9 ++++++---
> >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index f466980..d9cf216 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1287,9 +1287,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	 * then we do not take part in VGA arbitration and the
> >  	 * vga_client_register() fails with -ENODEV.
> >  	 */
> > -	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
> > -	if (ret && ret != -ENODEV)
> > -		goto out;
> > +	if (!HAS_PCH_SPLIT(dev)) {
> > +		ret = vga_client_register(dev->pdev, dev, NULL,
> > +					  i915_vga_set_decode);
> > +		if (ret && ret != -ENODEV)
> > +			goto out;
> > +	}
> 
> I think we should do something to fix i915_redisable_vga() for !pch
> platforms too. The MSR method should work for everything, so I'm leaning
> towards using it all the time. The other option would be add an mmio
> codepath into intel_disable_vga(), but then we have several codepaths
> to worry about.

Do you have time, systems, and interest to be able to test !pch?  I
assume that at some point in the past VGA arbiter did work on i915 with
the right hardware, so I didn't want to go breaking code that I can't
test.
  
> >  	intel_register_dsm_handler();
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5fb3058..2bb805c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9519,6 +9519,15 @@ static void i915_disable_vga(struct drm_device *dev)
> >  	outb(SR01, VGA_SR_INDEX);
> >  	sr1 = inb(VGA_SR_DATA);
> >  	outb(sr1 | 1<<5, VGA_SR_DATA);
> > +
> > +	/* Disable VGA memory on Intel HD */
> > +	if (HAS_PCH_SPLIT(dev)) {
> > +		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> > +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> > +						   VGA_RSRC_NORMAL_IO |
> > +						   VGA_RSRC_NORMAL_MEM);
> > +	}
> > +
> >  	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> >  	udelay(300);
> >  
> > @@ -9526,6 +9535,19 @@ static void i915_disable_vga(struct drm_device *dev)
> >  	POSTING_READ(vga_reg);
> >  }
> >  
> > +static void i915_enable_vga(struct drm_device *dev)
> > +{
> 
> Not sure we need this actually. We don't re-enable the VGA display
> on unload, so you'll never see anything on the display anyway. But
> then again, I don't see any real harm in doing this either.

Right, I actually get a flood of DMAR errors from VT-d when I unbind the
device from i915, but that happens regardless of this patch.  I did
confirm that VGA memory is re-added to VGA arbitration though, which is
important because even if i915 doesn't leave the device in a usable
state we might still be able to pass it to a guest, which will rely on
the VGA arbiter control being re-enabled.

> > +	/* Enable VGA memory on Intel HD */
> > +	if (HAS_PCH_SPLIT(dev)) {
> > +		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> > +		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> 
> I was thinking we might save the MSR state during driver load and
> restore to that same state, but it could be a bit overkill.

It seems unnecessary for just this bit, but if i915 wanted to restore
the device to a working VGA state it might make sense.  I have no idea
what would be involved for that.

> > +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_MASK |
> 
> Maybe spell out IO and MEM for legacy too. Otherwise it looks a
> bit cryptic.

Ok.  Thanks,

Alex

> > +						   VGA_RSRC_NORMAL_IO |
> > +						   VGA_RSRC_NORMAL_MEM);
> > +		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> > +	}
> > +}
> > +
> >  void intel_modeset_init_hw(struct drm_device *dev)
> >  {
> >  	intel_init_power_well(dev);
> > @@ -9983,6 +10005,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  
> >  	intel_disable_fbc(dev);
> >  
> > +	i915_enable_vga(dev);
> > +
> >  	intel_disable_gt_powersave(dev);
> >  
> >  	ironlake_teardown_rc6(dev);
> 




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] i915: Update VGA arbiter support for newer devices
  2013-08-26 16:05   ` Alex Williamson
@ 2013-08-26 17:47     ` Ville Syrjälä
  0 siblings, 0 replies; 4+ messages in thread
From: Ville Syrjälä @ 2013-08-26 17:47 UTC (permalink / raw)
  To: Alex Williamson; +Cc: intel-gfx, airlied, linux-kernel

On Mon, Aug 26, 2013 at 10:05:36AM -0600, Alex Williamson wrote:
> On Mon, 2013-08-26 at 15:23 +0300, Ville Syrjälä wrote:
> > On Sat, Aug 24, 2013 at 08:53:40AM -0600, Alex Williamson wrote:
> > > This is intended to add VGA arbiter support for Intel HD graphics on
> > > Core processors.  The old GMCH registers no longer exist, so even
> > > though it appears that i915 participates in VGA arbitration, it doesn't
> > > work.  On Intel HD graphics we already attempt to disable VGA regions
> > > of the device.  This makes registering as a VGA client unnecessary since
> > > we don't intend to operate differently depending on how many VGA devices
> > > are present.  We can disable VGA memory regions by clearing the memory
> > > enable bit in the VGA MSR.  That only leaves VGA IO, which we update
> > > the VGA arbiter to know that we don't participate in VGA memory
> > > arbitration.  We also add a hook on unload to re-enable memory and
> > > reinstate VGA memory arbitration.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > > 
> > > v2: I915_READ/WRITE accessors don't work in i915_disable_vga, use inb/outb
> > >     directly.  Also, on the driver unbind VGA enable path, acquire legacy
> > >     IO to re-enable VGA memory.  Correct comment.
> > > 
> > > As with v1, this depends on "vgaarb: Fixes for partial VGA opt-out".  With
> > > all patches I'm able to assign a discrete PEG VGA device to a guest and
> > > execute the VBIOS w/o interference from IGD or corruption of the IGD
> > > framebuffer.
> > 
> > I glanced through those before, but I'll try to do a more thorough
> > review soon.
> 
> Thanks!
> 
> > > 
> > >  drivers/gpu/drm/i915/i915_dma.c      |    9 ++++++---
> > >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++++
> > >  2 files changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index f466980..d9cf216 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1287,9 +1287,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
> > >  	 * then we do not take part in VGA arbitration and the
> > >  	 * vga_client_register() fails with -ENODEV.
> > >  	 */
> > > -	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
> > > -	if (ret && ret != -ENODEV)
> > > -		goto out;
> > > +	if (!HAS_PCH_SPLIT(dev)) {
> > > +		ret = vga_client_register(dev->pdev, dev, NULL,
> > > +					  i915_vga_set_decode);
> > > +		if (ret && ret != -ENODEV)
> > > +			goto out;
> > > +	}
> > 
> > I think we should do something to fix i915_redisable_vga() for !pch
> > platforms too. The MSR method should work for everything, so I'm leaning
> > towards using it all the time. The other option would be add an mmio
> > codepath into intel_disable_vga(), but then we have several codepaths
> > to worry about.
> 
> Do you have time, systems, and interest to be able to test !pch?  I
> assume that at some point in the past VGA arbiter did work on i915 with
> the right hardware, so I didn't want to go breaking code that I can't
> test.

I do have a few gen4ish machines sitting around. I guess I could try it on
them at some point when I have a bit of time. But you're right that
maybe it's better to not touch them before that. i915_disable_vga() is
already potentially broken on them, so your patch is not introducing
any new regressions, just maintaining the status quo.

>   
> > >  	intel_register_dsm_handler();
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 5fb3058..2bb805c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9519,6 +9519,15 @@ static void i915_disable_vga(struct drm_device *dev)
> > >  	outb(SR01, VGA_SR_INDEX);
> > >  	sr1 = inb(VGA_SR_DATA);
> > >  	outb(sr1 | 1<<5, VGA_SR_DATA);
> > > +
> > > +	/* Disable VGA memory on Intel HD */
> > > +	if (HAS_PCH_SPLIT(dev)) {
> > > +		outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> > > +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> > > +						   VGA_RSRC_NORMAL_IO |
> > > +						   VGA_RSRC_NORMAL_MEM);
> > > +	}
> > > +
> > >  	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> > >  	udelay(300);
> > >  
> > > @@ -9526,6 +9535,19 @@ static void i915_disable_vga(struct drm_device *dev)
> > >  	POSTING_READ(vga_reg);
> > >  }
> > >  
> > > +static void i915_enable_vga(struct drm_device *dev)
> > > +{
> > 
> > Not sure we need this actually. We don't re-enable the VGA display
> > on unload, so you'll never see anything on the display anyway. But
> > then again, I don't see any real harm in doing this either.
> 
> Right, I actually get a flood of DMAR errors from VT-d when I unbind the
> device from i915, but that happens regardless of this patch.  I did
> confirm that VGA memory is re-added to VGA arbitration though, which is
> important because even if i915 doesn't leave the device in a usable
> state we might still be able to pass it to a guest, which will rely on
> the VGA arbiter control being re-enabled.

Right.

> 
> > > +	/* Enable VGA memory on Intel HD */
> > > +	if (HAS_PCH_SPLIT(dev)) {
> > > +		vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> > > +		outb(inb(VGA_MSR_READ) | VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> > 
> > I was thinking we might save the MSR state during driver load and
> > restore to that same state, but it could be a bit overkill.
> 
> It seems unnecessary for just this bit, but if i915 wanted to restore
> the device to a working VGA state it might make sense.  I have no idea
> what would be involved for that.

Haven't looked in the docs actually, but based in my gut, it doesn't
seem all that much work since in theory we shouldn't even need to poke
at the VGA registers that much. Well, unless disabling the VGA plane
resets all that.

So we'd just need to set a mode, disable the normal planes, re-enable
the VGA plane, and program the panel fitter in a suitable fashion. I
guess it should even possible to cook up a suitable config and then
just call the normal modeset path, and that'd take care of most of it.

Not sure if anyone has tried that though.

> > > +		vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_MASK |
> > 
> > Maybe spell out IO and MEM for legacy too. Otherwise it looks a
> > bit cryptic.
> 
> Ok.  Thanks,
> 
> Alex
> 
> > > +						   VGA_RSRC_NORMAL_IO |
> > > +						   VGA_RSRC_NORMAL_MEM);
> > > +		vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> > > +	}
> > > +}
> > > +
> > >  void intel_modeset_init_hw(struct drm_device *dev)
> > >  {
> > >  	intel_init_power_well(dev);
> > > @@ -9983,6 +10005,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
> > >  
> > >  	intel_disable_fbc(dev);
> > >  
> > > +	i915_enable_vga(dev);
> > > +
> > >  	intel_disable_gt_powersave(dev);
> > >  
> > >  	ironlake_teardown_rc6(dev);
> > 
> 
> 

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-08-26 17:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-24 14:53 [PATCH v2] i915: Update VGA arbiter support for newer devices Alex Williamson
2013-08-26 12:23 ` Ville Syrjälä
2013-08-26 16:05   ` Alex Williamson
2013-08-26 17:47     ` Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).