Re: [PATCH v2] i915: Update VGA arbiter support for newer devices

[Ville Syrjälä]

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@xxxxxxxxxx>
> > > ---
> > > 
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/