From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm/i915: Ironlake GPU with VT-d fix Date: Fri, 23 Sep 2011 20:03:18 +0100 Message-ID: References: <1316736712-5887-1-git-send-email-ben@bwidawsk.net> <20110922220939.3c78a1be@bwidawsk.net> <20110923083849.7bff0502@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 957CC9F019 for ; Fri, 23 Sep 2011 12:03:23 -0700 (PDT) In-Reply-To: <20110923083849.7bff0502@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: Dave Airlie , Woodhouse , intel-gfx@lists.freedesktop.org, David@freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 23 Sep 2011 08:38:49 -0700, Ben Widawsky wrote: > On Fri, 23 Sep 2011 08:37:10 +0100 > Chris Wilson wrote: > > > On Thu, 22 Sep 2011 22:09:39 -0700, Ben Widawsky > > wrote: > > > On Thu, 22 Sep 2011 21:50:49 -0700 > > > Keith Packard wrote: > > > > > > > On Thu, 22 Sep 2011 17:11:52 -0700, Ben Widawsky > > > > 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. > > Crap... This is a bug in the patch. I'm still can't explain the necessity of forcing the i915_ring_idle() here. If those two lists are empty, then each of the ring write/active lists should also be empty and it will be a no-op. Hence bafflement and time to be slightly afraid. > > > > Also we should not introduce BUG_ON() trivially into user code paths > > (/dev/agp). > > Remove 'em then? Not quite, in this case I think returning -ENODEV is the sensible approach. -Chris -- Chris Wilson, Intel Open Source Technology Centre