diff mbox series

[v2,17/17] drm/i915: Properly request PCI BARs

Message ID 20201009075934.3509076-18-daniel.vetter@ffwll.ch
State New
Headers show
Series follow_pfn and other iomap races | expand

Commit Message

Daniel Vetter Oct. 9, 2020, 7:59 a.m. UTC
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(-)

Comments

Ville Syrjala Oct. 9, 2020, 9:47 a.m. UTC | #1
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.

> +	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.

>  	/*

> -	 * Before gen4, the registers and the GTT are behind different BARs.

> +	 * On gen3 the registers and the GTT are behind different BARs.

>  	 * However, from gen4 onwards, the registers and the GTT are shared

>  	 * in the same BAR, so we want to restrict this ioremap from

>  	 * clobbering the GTT which we want ioremap_wc instead. Fortunately,

> @@ -1703,6 +1706,8 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)

>  	 * generations up to Ironlake.

>  	 * For dgfx chips register range is expanded to 4MB.

>  	 */

> +	if (INTEL_GEN(i915) == 3)

> +		bar_selection |= BIT(3);

>  	if (INTEL_GEN(i915) < 5)

>  		mmio_size = 512 * 1024;

>  	else if (IS_DGFX(i915))

> @@ -1710,8 +1715,15 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)

>  	else

>  		mmio_size = 2 * 1024 * 1024;

>  

> +	ret = pci_request_selected_regions(pdev, bar_selection, "i915");

> +	if (ret < 0) {

> +		drm_err(&i915->drm, "failed to request pci bars\n");

> +		return ret;

> +	}

> +

>  	uncore->regs = pci_iomap(pdev, mmio_bar, mmio_size);

>  	if (uncore->regs == NULL) {

> +		pci_release_selected_regions(pdev, bar_selection);

>  		drm_err(&i915->drm, "failed to map registers\n");

>  		return -EIO;

>  	}

> @@ -1721,9 +1733,18 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)

>  

>  static void uncore_mmio_cleanup(struct intel_uncore *uncore)

>  {

> -	struct pci_dev *pdev = uncore->i915->drm.pdev;

> +	struct drm_i915_private *i915 = uncore->i915;

> +	struct pci_dev *pdev = i915->drm.pdev;

> +	int mmio_bar;

> +	int bar_selection;

> +

> +	mmio_bar = IS_GEN(i915, 2) ? 1 : 0;

> +	bar_selection = BIT (2) | BIT(mmio_bar);

> +	if (INTEL_GEN(i915) == 3)

> +		bar_selection |= BIT(3);

>  

>  	pci_iounmap(pdev, uncore->regs);

> +	pci_release_selected_regions(pdev, bar_selection);

>  }

>  

>  void intel_uncore_init_early(struct intel_uncore *uncore,

> -- 

> 2.28.0

> 

> _______________________________________________

> dri-devel mailing list

> dri-devel@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Ville Syrjälä
Intel
Daniel Vetter Oct. 9, 2020, 10:01 a.m. UTC | #2
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.

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.
-Daniel

>
> >       /*
> > -      * Before gen4, the registers and the GTT are behind different BARs.
> > +      * On gen3 the registers and the GTT are behind different BARs.
> >        * However, from gen4 onwards, the registers and the GTT are shared
> >        * in the same BAR, so we want to restrict this ioremap from
> >        * clobbering the GTT which we want ioremap_wc instead. Fortunately,
> > @@ -1703,6 +1706,8 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)
> >        * generations up to Ironlake.
> >        * For dgfx chips register range is expanded to 4MB.
> >        */
> > +     if (INTEL_GEN(i915) == 3)
> > +             bar_selection |= BIT(3);
> >       if (INTEL_GEN(i915) < 5)
> >               mmio_size = 512 * 1024;
> >       else if (IS_DGFX(i915))
> > @@ -1710,8 +1715,15 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)
> >       else
> >               mmio_size = 2 * 1024 * 1024;
> >
> > +     ret = pci_request_selected_regions(pdev, bar_selection, "i915");
> > +     if (ret < 0) {
> > +             drm_err(&i915->drm, "failed to request pci bars\n");
> > +             return ret;
> > +     }
> > +
> >       uncore->regs = pci_iomap(pdev, mmio_bar, mmio_size);
> >       if (uncore->regs == NULL) {
> > +             pci_release_selected_regions(pdev, bar_selection);
> >               drm_err(&i915->drm, "failed to map registers\n");
> >               return -EIO;
> >       }
> > @@ -1721,9 +1733,18 @@ static int uncore_mmio_setup(struct intel_uncore *uncore)
> >
> >  static void uncore_mmio_cleanup(struct intel_uncore *uncore)
> >  {
> > -     struct pci_dev *pdev = uncore->i915->drm.pdev;
> > +     struct drm_i915_private *i915 = uncore->i915;
> > +     struct pci_dev *pdev = i915->drm.pdev;
> > +     int mmio_bar;
> > +     int bar_selection;
> > +
> > +     mmio_bar = IS_GEN(i915, 2) ? 1 : 0;
> > +     bar_selection = BIT (2) | BIT(mmio_bar);
> > +     if (INTEL_GEN(i915) == 3)
> > +             bar_selection |= BIT(3);
> >
> >       pci_iounmap(pdev, uncore->regs);
> > +     pci_release_selected_regions(pdev, bar_selection);
> >  }
> >
> >  void intel_uncore_init_early(struct intel_uncore *uncore,
> > --
> > 2.28.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
Daniel Vetter Oct. 9, 2020, 2:18 p.m. UTC | #3
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
diff mbox series

Patch

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;
+	int ret;
 
 	mmio_bar = IS_GEN(i915, 2) ? 1 : 0;
+	bar_selection = BIT (2) | BIT(mmio_bar);
 	/*
-	 * Before gen4, the registers and the GTT are behind different BARs.
+	 * On gen3 the registers and the GTT are behind different BARs.
 	 * However, from gen4 onwards, the registers and the GTT are shared
 	 * in the same BAR, so we want to restrict this ioremap from
 	 * clobbering the GTT which we want ioremap_wc instead. Fortunately,
@@ -1703,6 +1706,8 @@  static int uncore_mmio_setup(struct intel_uncore *uncore)
 	 * generations up to Ironlake.
 	 * For dgfx chips register range is expanded to 4MB.
 	 */
+	if (INTEL_GEN(i915) == 3)
+		bar_selection |= BIT(3);
 	if (INTEL_GEN(i915) < 5)
 		mmio_size = 512 * 1024;
 	else if (IS_DGFX(i915))
@@ -1710,8 +1715,15 @@  static int uncore_mmio_setup(struct intel_uncore *uncore)
 	else
 		mmio_size = 2 * 1024 * 1024;
 
+	ret = pci_request_selected_regions(pdev, bar_selection, "i915");
+	if (ret < 0) {
+		drm_err(&i915->drm, "failed to request pci bars\n");
+		return ret;
+	}
+
 	uncore->regs = pci_iomap(pdev, mmio_bar, mmio_size);
 	if (uncore->regs == NULL) {
+		pci_release_selected_regions(pdev, bar_selection);
 		drm_err(&i915->drm, "failed to map registers\n");
 		return -EIO;
 	}
@@ -1721,9 +1733,18 @@  static int uncore_mmio_setup(struct intel_uncore *uncore)
 
 static void uncore_mmio_cleanup(struct intel_uncore *uncore)
 {
-	struct pci_dev *pdev = uncore->i915->drm.pdev;
+	struct drm_i915_private *i915 = uncore->i915;
+	struct pci_dev *pdev = i915->drm.pdev;
+	int mmio_bar;
+	int bar_selection;
+
+	mmio_bar = IS_GEN(i915, 2) ? 1 : 0;
+	bar_selection = BIT (2) | BIT(mmio_bar);
+	if (INTEL_GEN(i915) == 3)
+		bar_selection |= BIT(3);
 
 	pci_iounmap(pdev, uncore->regs);
+	pci_release_selected_regions(pdev, bar_selection);
 }
 
 void intel_uncore_init_early(struct intel_uncore *uncore,