On Fri, Oct 9, 2020 at 12:42 PM Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Oct 09, 2020 at 12:01:39PM +0200, Daniel Vetter wrote:
On Fri, Oct 9, 2020 at 11:47 AM Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Oct 09, 2020 at 09:59:34AM +0200, Daniel Vetter wrote:
When trying to test my CONFIG_IO_STRICT_DEVMEM changes I realized they do nothing for i915. Because i915 doesn't request any regions, like pretty much all drm pci drivers. I guess this is some very old remnants from the userspace modesetting days, when we wanted to co-exist with the fbdev driver. Which usually requested these resources.
But makes me wonder why the pci subsystem doesn't just request resource automatically when we map a bar and a pci driver is bound?
Knowledge about which pci bars we need kludged together from intel_uncore.c and intel_gtt.c from i915 and intel-gtt.c over in the fake agp driver.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Jason Gunthorpe jgg@ziepe.ca Cc: Kees Cook keescook@chromium.org Cc: Dan Williams dan.j.williams@intel.com Cc: Andrew Morton akpm@linux-foundation.org Cc: John Hubbard jhubbard@nvidia.com Cc: Jérôme Glisse jglisse@redhat.com Cc: Jan Kara jack@suse.cz Cc: Dan Williams dan.j.williams@intel.com Cc: linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: Bjorn Helgaas bhelgaas@google.com Cc: linux-pci@vger.kernel.org
drivers/gpu/drm/i915/intel_uncore.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 54e201fdeba4..ce39049d8919 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1692,10 +1692,13 @@ static int uncore_mmio_setup(struct intel_uncore *uncore) struct pci_dev *pdev = i915->drm.pdev; int mmio_bar; int mmio_size;
int bar_selection;Signed bitmasks always make me uneasy. But looks like that's what it is in the pci api. So meh.
Yeah it's surprising.
int ret; mmio_bar = IS_GEN(i915, 2) ? 1 : 0;bar_selection = BIT (2) | BIT(mmio_bar);^spurious space
That's also not correct for gen2 I think.
gen2: 0 = GMADR 1 = MMADR 2 = IOBAR
gen3: 0 = MMADR 1 = IOBAR 2 = GMADR 3 = GTTADR
gen4+: 0+1 = GTTMMADR 2+3 = GMADR 4 = IOBAR
Maybe we should just have an explicit list of bars like that in a comment?
I'd also suggest sucking this bitmask calculation into a small helper so you can reuse it for the release.
tbh I just hacked this up for testing. Given how almost no other drm driver does this, I'm wondering whether we should or not.
Also the only reason why I didn't just use the pci_request_regions helper is to avoid the vga ioport range, since that's managed by vgaarbiter.
VGA io range isn't part of any bar. Or do you mean just the io decode enable bit in the pci command register? That should be just a matter or pci_enable_device() vs. pci_enable_device_mem() I think. So nothing to do with which bars we've requested IIRC.
So I think if we go for this for real we should:
- register the vga ioport range in the vgaarbiter
- have a pci_request_iomem_regions helper that grabs all mem bars
- roll that out to all drm pci drivers
Or something like that. The other complication is when we resize the iobar. So not really sure what to do here.
We resize it?
By default I thought firmware puts everything (well, squeezes) into the lower 32bit. Maybe they stopped doing that. So when we want the full bar (for discrete at least) we need to resize it and put it somewhere in the 64bit range above end of system memory.
So I guess correct phrasing is "we will resize it" :-) -Daniel