Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, 22 Sep 2011 22:09:39 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, 22 Sep 2011 21:50:49 -0700
> Keith Packard <keithp@keithp.com> wrote:
> 
> > On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > 
> > > It requires an additional IOMMU patch.
> > 
> > Can we collect those two patches into one sequence?
> > 
> 
> Yes. Although not sure who would do the pull request to Linus.
> 
> > > +	if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> > > +	     gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> > > +	     intel_private.base.needs_dmar)
> > > +		intel_private.base.do_idle_maps = 1;
> > > +
> > 
> > I'd like to make this conditional on whether IOMMU is actually in use;
> > needs_dmar is based solely on whether the DMA_API is compiled into the
> > kernel and the GTT gen is > 2.
> 
> David did mention a way for me to detect it. I wasn't sure if there was a
> deadline to get this patch out so I submitted it now. I will work on the
> detection portion of it while the rest gets review. It's something like, check
> if the bus address == the physical address and if not, assume the IOMMU is in
> use.

And for starters, we can check no_iommu. But really we want
intel_iommu.c to report whether or not it enabled VTd on the gfx.
 
> > 
> > > -	if (lists_empty)
> > > +	if (lists_empty && !!dev_priv->mm.gtt->do_idle_maps)
> > >  		return 0;
> > 
> > Is it necessary to change the semantic of this function in cases which
> > aren't related to GTT remapping? Seems like you're imposing a fairly
> > high cost on operations which don't actually need it.
> > 
> 
> Not sure I follow. On that specific hunk it's apparently needed. I tried the
> patch without this and we got some data back which suggested it didn't work. It
> would be nice if we could reproduce this locally...

Keith is right, there should be no reason to touch that function,
and especially not for the normal code paths.

Also we should not introduce BUG_ON() trivially into user code paths
(/dev/agp).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre