All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
@ 2012-06-11 15:07 Daniel Vetter
  2012-06-11 15:35 ` Ville Syrjälä
  2012-06-11 15:58 ` Jani Nikula
  0 siblings, 2 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-11 15:07 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..6be01c8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary = false;
+
+	ap = alloc_apertures(1);
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+#ifdef CONFIG_X86
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+#endif
+
+	remove_conflicting_framebuffers(ap, "radeondrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1465,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto out_rmmap;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1499,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 15:07 [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt Daniel Vetter
@ 2012-06-11 15:35 ` Ville Syrjälä
  2012-06-11 16:13   ` Daniel Vetter
  2012-06-11 15:58 ` Jani Nikula
  1 sibling, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2012-06-11 15:35 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development

On Mon, Jun 11, 2012 at 05:07:03PM +0200, Daniel Vetter wrote:
> Especially vesafb likes to map everything as uc- (yikes), and if that
> mapping hangs around still while we try to map the gtt as wc the
> kernel will downgrade our request to uc-, resulting in abyssal
> performance.
> 
> Unfortunately we can't do this as early as readon does (i.e. as the
> first thing we do when initializing the hw) because our fb/mmio space
> region moves around on a per-gen basis. So I've had to move it below
> the gtt initialization, but that seems to work, too. The important
> thing is that we do this before we set up the gtt wc mapping.
> 
> Now an altogether different question is why people compile their
> kernels with vesafb enabled, but I guess making things just work isn't
> bad per se ...
> 
> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 262a74d..6be01c8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
>  	}
>  }
>  
> +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
> +{
> +	struct apertures_struct *ap;
> +	struct pci_dev *pdev = dev_priv->dev->pdev;
> +	bool primary = false;
> +
> +	ap = alloc_apertures(1);
> +	ap->ranges[0].base = dev_priv->dev->agp->base;
> +	ap->ranges[0].size =
> +		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
> +
> +#ifdef CONFIG_X86
> +	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> +#endif
> +
> +	remove_conflicting_framebuffers(ap, "radeondrmfb", primary);
                                             ^^^^^^^^^^^

Nice disguise :)

> +	kfree(ap);
> +}
> +
>  /**
>   * i915_driver_load - setup chip and create an initial config
>   * @dev: DRM device

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 15:07 [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt Daniel Vetter
  2012-06-11 15:35 ` Ville Syrjälä
@ 2012-06-11 15:58 ` Jani Nikula
  1 sibling, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2012-06-11 15:58 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

On Mon, 11 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Especially vesafb likes to map everything as uc- (yikes), and if that
> mapping hangs around still while we try to map the gtt as wc the
> kernel will downgrade our request to uc-, resulting in abyssal
> performance.
>
> Unfortunately we can't do this as early as readon does (i.e. as the
> first thing we do when initializing the hw) because our fb/mmio space
> region moves around on a per-gen basis. So I've had to move it below
> the gtt initialization, but that seems to work, too. The important
> thing is that we do this before we set up the gtt wc mapping.
>
> Now an altogether different question is why people compile their
> kernels with vesafb enabled, but I guess making things just work isn't
> bad per se ...
>
> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 262a74d..6be01c8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
>  	}
>  }
>  
> +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
> +{
> +	struct apertures_struct *ap;
> +	struct pci_dev *pdev = dev_priv->dev->pdev;
> +	bool primary = false;
> +
> +	ap = alloc_apertures(1);
> +	ap->ranges[0].base = dev_priv->dev->agp->base;
> +	ap->ranges[0].size =
> +		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
> +
> +#ifdef CONFIG_X86
> +	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> +#endif
> +
> +	remove_conflicting_framebuffers(ap, "radeondrmfb", primary);
> +	kfree(ap);
> +}
> +
>  /**
>   * i915_driver_load - setup chip and create an initial config
>   * @dev: DRM device
> @@ -1446,6 +1465,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto free_priv;
>  	}
>  
> +	dev_priv->mm.gtt = intel_gtt_get();
> +	if (!dev_priv->mm.gtt) {
> +		DRM_ERROR("Failed to initialize GTT\n");
> +		ret = -ENODEV;
> +		goto out_rmmap;
> +	}

Hi Daniel, the error handling goes wrong with this code move, at least
goto out_rmmap is wrong. Did not check much else in the patch.

BR,
Jani.



> +
> +	i915_kick_out_firmware_fb(dev_priv);
> +
>  	pci_set_master(dev->pdev);
>  
>  	/* overlay on gen2 is broken and can't address above 1G */
> @@ -1471,13 +1499,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto put_bridge;
>  	}
>  
> -	dev_priv->mm.gtt = intel_gtt_get();
> -	if (!dev_priv->mm.gtt) {
> -		DRM_ERROR("Failed to initialize GTT\n");
> -		ret = -ENODEV;
> -		goto out_rmmap;
> -	}
> -
>  	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
>  
>  	dev_priv->mm.gtt_mapping =
> -- 
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 15:35 ` Ville Syrjälä
@ 2012-06-11 16:13   ` Daniel Vetter
  2012-06-11 16:28     ` Daniel Vetter
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2012-06-11 16:13 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Daniel Vetter, Intel Graphics Development

On Mon, Jun 11, 2012 at 06:35:13PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 11, 2012 at 05:07:03PM +0200, Daniel Vetter wrote:
> > Especially vesafb likes to map everything as uc- (yikes), and if that
> > mapping hangs around still while we try to map the gtt as wc the
> > kernel will downgrade our request to uc-, resulting in abyssal
> > performance.
> > 
> > Unfortunately we can't do this as early as readon does (i.e. as the
> > first thing we do when initializing the hw) because our fb/mmio space
> > region moves around on a per-gen basis. So I've had to move it below
> > the gtt initialization, but that seems to work, too. The important
> > thing is that we do this before we set up the gtt wc mapping.
> > 
> > Now an altogether different question is why people compile their
> > kernels with vesafb enabled, but I guess making things just work isn't
> > bad per se ...
> > 
> > Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
> >  1 file changed, 28 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 262a74d..6be01c8 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
> >  	}
> >  }
> >  
> > +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
> > +{
> > +	struct apertures_struct *ap;
> > +	struct pci_dev *pdev = dev_priv->dev->pdev;
> > +	bool primary = false;
> > +
> > +	ap = alloc_apertures(1);
> > +	ap->ranges[0].base = dev_priv->dev->agp->base;
> > +	ap->ranges[0].size =
> > +		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
> > +
> > +#ifdef CONFIG_X86
> > +	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> > +#endif
> > +
> > +	remove_conflicting_framebuffers(ap, "radeondrmfb", primary);
>                                              ^^^^^^^^^^^
> 
> Nice disguise :)

Meh, failure to git add :( I'll fix it up with the other thing Jani
noticed.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 16:13   ` Daniel Vetter
@ 2012-06-11 16:28     ` Daniel Vetter
  2012-06-11 23:43       ` Ben Widawsky
  2012-06-12  8:52       ` Chris Wilson
  0 siblings, 2 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-11 16:28 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..379cb14 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary = false;
+
+	ap = alloc_apertures(1);
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+#ifdef CONFIG_X86
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+#endif
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1465,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1499,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 16:28     ` Daniel Vetter
@ 2012-06-11 23:43       ` Ben Widawsky
  2012-06-12  8:12         ` Daniel Vetter
  2012-06-12  8:24         ` Daniel Vetter
  2012-06-12  8:52       ` Chris Wilson
  1 sibling, 2 replies; 16+ messages in thread
From: Ben Widawsky @ 2012-06-11 23:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development

We could probably fix this with a kernel command line too:
video=vesafb:mtrr:3


On Mon, 11 Jun 2012 18:28:12 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Especially vesafb likes to map everything as uc- (yikes), and if that
> mapping hangs around still while we try to map the gtt as wc the
> kernel will downgrade our request to uc-, resulting in abyssal
> performance.

Probably every graphics device running on x86 wants this, should
we consider trying to generalize it a bit more with a DRM helper? At
the very least, I think we should consider some Kconfig changes
(comments, excluding vesafb when i915 is selected, or something).

Just to satisfy my curiousity if vesafb is loaded after, we'll run into
the same problem (like GPU offloading with vesa display)?

s/abyssal/abysmal

> 
> Unfortunately we can't do this as early as readon does (i.e. as the
> first thing we do when initializing the hw) because our fb/mmio space
> region moves around on a per-gen basis. So I've had to move it below
> the gtt initialization, but that seems to work, too. The important
> thing is that we do this before we set up the gtt wc mapping.
> 
> Now an altogether different question is why people compile their
> kernels with vesafb enabled, but I guess making things just work isn't
> bad per se ...

The kernel comments recommend turning it on if unsure. As above, I
think that should be corrected.

> 
> v2:
> - s/radeondrmfb/inteldrmfb/
> - fix up error handling
> 
> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
  still, a couple comments below

> ---
>  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 262a74d..379cb14 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
>  	}
>  }
>  
> +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
> +{
> +	struct apertures_struct *ap;
> +	struct pci_dev *pdev = dev_priv->dev->pdev;
> +	bool primary = false;
> +
> +	ap = alloc_apertures(1);
> +	ap->ranges[0].base = dev_priv->dev->agp->base;
> +	ap->ranges[0].size =
> +		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
> +
> +#ifdef CONFIG_X86
> +	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> +#endif

I don't really understand the point of this bit, but I guess other
drivers do it, so meh. Also, the ifdef CONFIG_X86 is sort of pointless
in the i915 driver, but no biggie.

> +
> +	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
> +	kfree(ap);
> +}
> +
>  /**
>   * i915_driver_load - setup chip and create an initial config
>   * @dev: DRM device
> @@ -1446,6 +1465,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto free_priv;
>  	}
>  
> +	dev_priv->mm.gtt = intel_gtt_get();
> +	if (!dev_priv->mm.gtt) {
> +		DRM_ERROR("Failed to initialize GTT\n");
> +		ret = -ENODEV;
> +		goto put_bridge;
> +	}
> +
> +	i915_kick_out_firmware_fb(dev_priv);
> +
>  	pci_set_master(dev->pdev);
>  
>  	/* overlay on gen2 is broken and can't address above 1G */
> @@ -1471,13 +1499,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto put_bridge;
>  	}
>  
> -	dev_priv->mm.gtt = intel_gtt_get();
> -	if (!dev_priv->mm.gtt) {
> -		DRM_ERROR("Failed to initialize GTT\n");
> -		ret = -ENODEV;
> -		goto out_rmmap;
> -	}
> -
>  	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
>  
>  	dev_priv->mm.gtt_mapping =



-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 23:43       ` Ben Widawsky
@ 2012-06-12  8:12         ` Daniel Vetter
  2012-06-12  8:24         ` Daniel Vetter
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-12  8:12 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: Intel Graphics Development

On Tue, Jun 12, 2012 at 1:43 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> We could probably fix this with a kernel command line too:
> video=vesafb:mtrr:3

Well, to add insult to injury the affected system also didn't have a
free mtrr :(

> On Mon, 11 Jun 2012 18:28:12 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> Especially vesafb likes to map everything as uc- (yikes), and if that
>> mapping hangs around still while we try to map the gtt as wc the
>> kernel will downgrade our request to uc-, resulting in abyssal
>> performance.
>
> Probably every graphics device running on x86 wants this, should
> we consider trying to generalize it a bit more with a DRM helper? At
> the very least, I think we should consider some Kconfig changes
> (comments, excluding vesafb when i915 is selected, or something).

Well, the other ones already do this trick. The issue is that on
better platforms (e.g. openfirmware on ppc) you have a much more
powerful platform framebuffer. And you need to kick this one away
before you set up your own stuff, because otherwise every printk might
use the hw blitter. And because apple used both nvidia and ati chips
in it's machines, both drivers have these hooks in place already.

Note also that register a framebuffer driver does the same firmware fb
kicking, but usually that's way to late.

> Just to satisfy my curiousity if vesafb is loaded after, we'll run into
> the same problem (like GPU offloading with vesa display)?
>
> s/abyssal/abysmal

vesa checks whether it's framebuffer range is already occupied by a
real framebuffer driver and if so doesn't initialize.

>> Unfortunately we can't do this as early as readon does (i.e. as the
>> first thing we do when initializing the hw) because our fb/mmio space
>> region moves around on a per-gen basis. So I've had to move it below
>> the gtt initialization, but that seems to work, too. The important
>> thing is that we do this before we set up the gtt wc mapping.
>>
>> Now an altogether different question is why people compile their
>> kernels with vesafb enabled, but I guess making things just work isn't
>> bad per se ...
>
> The kernel comments recommend turning it on if unsure. As above, I
> think that should be corrected.

If you try to run a modern distro on a defaultconfig kernel, it won't
work. vesafb isn't the only thing that has a strange recommendatation.
Use make localmodconfig and be done with it imo.

>> v2:
>> - s/radeondrmfb/inteldrmfb/
>> - fix up error handling
>>
>> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
>> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>  still, a couple comments below
>
>> ---
>>  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 262a74d..379cb14 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
>>       }
>>  }
>>
>> +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
>> +{
>> +     struct apertures_struct *ap;
>> +     struct pci_dev *pdev = dev_priv->dev->pdev;
>> +     bool primary = false;
>> +
>> +     ap = alloc_apertures(1);
>> +     ap->ranges[0].base = dev_priv->dev->agp->base;
>> +     ap->ranges[0].size =
>> +             dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
>> +
>> +#ifdef CONFIG_X86
>> +     primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
>> +#endif
>
> I don't really understand the point of this bit, but I guess other
> drivers do it, so meh. Also, the ifdef CONFIG_X86 is sort of pointless
> in the i915 driver, but no biggie.

Neither do I exactly. Copy&paste cargo-culting ftw!

>> +
>> +     remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
>> +     kfree(ap);
>> +}
>> +
>>  /**
>>   * i915_driver_load - setup chip and create an initial config
>>   * @dev: DRM device
>> @@ -1446,6 +1465,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>               goto free_priv;
>>       }
>>
>> +     dev_priv->mm.gtt = intel_gtt_get();
>> +     if (!dev_priv->mm.gtt) {
>> +             DRM_ERROR("Failed to initialize GTT\n");
>> +             ret = -ENODEV;
>> +             goto put_bridge;
>> +     }
>> +
>> +     i915_kick_out_firmware_fb(dev_priv);
>> +
>>       pci_set_master(dev->pdev);
>>
>>       /* overlay on gen2 is broken and can't address above 1G */
>> @@ -1471,13 +1499,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>               goto put_bridge;
>>       }
>>
>> -     dev_priv->mm.gtt = intel_gtt_get();
>> -     if (!dev_priv->mm.gtt) {
>> -             DRM_ERROR("Failed to initialize GTT\n");
>> -             ret = -ENODEV;
>> -             goto out_rmmap;
>> -     }
>> -
>>       aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
>>
>>       dev_priv->mm.gtt_mapping =
>
>
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 23:43       ` Ben Widawsky
  2012-06-12  8:12         ` Daniel Vetter
@ 2012-06-12  8:24         ` Daniel Vetter
  2012-06-12  8:43           ` Daniel Vetter
  2012-06-12  9:28           ` Daniel Vetter
  1 sibling, 2 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-12  8:24 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..4f5e416 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,23 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary = false;
+
+	ap = alloc_apertures(1);
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1463,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1497,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12  8:24         ` Daniel Vetter
@ 2012-06-12  8:43           ` Daniel Vetter
  2012-06-12  9:28           ` Daniel Vetter
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-12  8:43 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.

v4: Jani Nikula complained about the pointless bool primary
initialization.

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..f6cfa95 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,23 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary;
+
+	ap = alloc_apertures(1);
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1463,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1497,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-11 16:28     ` Daniel Vetter
  2012-06-11 23:43       ` Ben Widawsky
@ 2012-06-12  8:52       ` Chris Wilson
  1 sibling, 0 replies; 16+ messages in thread
From: Chris Wilson @ 2012-06-12  8:52 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

On Mon, 11 Jun 2012 18:28:12 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Especially vesafb likes to map everything as uc- (yikes), and if that
> mapping hangs around still while we try to map the gtt as wc the
> kernel will downgrade our request to uc-, resulting in abyssal
> performance.
> 
> Unfortunately we can't do this as early as readon does (i.e. as the
> first thing we do when initializing the hw) because our fb/mmio space
> region moves around on a per-gen basis. So I've had to move it below
> the gtt initialization, but that seems to work, too. The important
> thing is that we do this before we set up the gtt wc mapping.
> 
> Now an altogether different question is why people compile their
> kernels with vesafb enabled, but I guess making things just work isn't
> bad per se ...
> 
> v2:
> - s/radeondrmfb/inteldrmfb/
> - fix up error handling
> 
> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 262a74d..379cb14 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1401,6 +1401,25 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
>  	}
>  }
>  
> +static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
> +{
> +	struct apertures_struct *ap;
> +	struct pci_dev *pdev = dev_priv->dev->pdev;
> +	bool primary = false;
> +
> +	ap = alloc_apertures(1);

Potential malloc failure needs handling.

> +	ap->ranges[0].base = dev_priv->dev->agp->base;
> +	ap->ranges[0].size =
> +		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
> +
> +#ifdef CONFIG_X86
> +	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;

That seems fraught with danger. Do we still get the ROM_SHADOW flag for
a primary device with no rom? Would checking the pci_dev for the VGA
class be safer (and not introduce a CONFIG_X86 :)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12  8:24         ` Daniel Vetter
  2012-06-12  8:43           ` Daniel Vetter
@ 2012-06-12  9:28           ` Daniel Vetter
  2012-06-12 11:43             ` Singh, Satyeshwar
  2012-06-13 11:30             ` Daniel Vetter
  1 sibling, 2 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-12  9:28 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.

v4: Jani Nikula complained about the pointless bool primary
initialization.

v5: Don't oops if we can't allocate, noticed by Chris Wilson.

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 262a74d..2e37b92 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,27 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary;
+
+	ap = alloc_apertures(1);
+
+	if (!ap)
+		return;
+
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1467,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1501,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12  9:28           ` Daniel Vetter
@ 2012-06-12 11:43             ` Singh, Satyeshwar
  2012-06-12 11:58               ` Chris Wilson
  2012-06-13 11:30             ` Daniel Vetter
  1 sibling, 1 reply; 16+ messages in thread
From: Singh, Satyeshwar @ 2012-06-12 11:43 UTC (permalink / raw)
  To: Daniel Vetter, Intel Graphics Development; +Cc: Ben Widawsky

Hi,
I just want to confirm something here. We have a situation where we draw a splash screen through our firmware's graphics component (UEFI GOP driver) onto its frame buffer which resides in stolen memory. When we transition from firmware to the kernel, we would like to ensure that the same splash screen seamlessly continues to be displayed so we remap the pages from stolen memory into GTT. If we have purged firmware framebuffers before claiming the gtt, then would we have not essentially lost the chance to remap those pages?
-Satyeshwar

-----Original Message-----
From: intel-gfx-bounces+satyeshwar.singh=intel.com@lists.freedesktop.org [mailto:intel-gfx-bounces+satyeshwar.singh=intel.com@lists.freedesktop.org] On Behalf Of Daniel Vetter
Sent: Tuesday, June 12, 2012 2:28 AM
To: Intel Graphics Development
Cc: Daniel Vetter; Ben Widawsky
Subject: [Intel-gfx] [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt

Especially vesafb likes to map everything as uc- (yikes), and if that mapping hangs around still while we try to map the gtt as wc the kernel will downgrade our request to uc-, resulting in abyssal performance.

Unfortunately we can't do this as early as readon does (i.e. as the first thing we do when initializing the hw) because our fb/mmio space region moves around on a per-gen basis. So I've had to move it below the gtt initialization, but that seems to work, too. The important thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their kernels with vesafb enabled, but I guess making things just work isn't bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.

v4: Jani Nikula complained about the pointless bool primary initialization.

v5: Don't oops if we can't allocate, noticed by Chris Wilson.

Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 262a74d..2e37b92 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,27 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private 
+*dev_priv) {
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary;
+
+	ap = alloc_apertures(1);
+
+	if (!ap)
+		return;
+
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & 
+IORESOURCE_ROM_SHADOW;
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1467,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */ @@ -1471,13 +1501,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
--
1.7.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12 11:43             ` Singh, Satyeshwar
@ 2012-06-12 11:58               ` Chris Wilson
  2012-06-12 12:16                 ` Daniel Vetter
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2012-06-12 11:58 UTC (permalink / raw)
  To: Singh, Satyeshwar, Daniel Vetter, Intel Graphics Development; +Cc: Ben Widawsky

On Tue, 12 Jun 2012 11:43:50 +0000, "Singh, Satyeshwar" <satyeshwar.singh@intel.com> wrote:
> Hi,
> I just want to confirm something here. We have a situation where we draw a splash screen through our firmware's graphics component (UEFI GOP driver) onto its frame buffer which resides in stolen memory. When we transition from firmware to the kernel, we would like to ensure that the same splash screen seamlessly continues to be displayed so we remap the pages from stolen memory into GTT. If we have purged firmware framebuffers before claiming the gtt, then would we have not essentially lost the chance to remap those pages?

That's a separate issue. What this patch is addressing is removing a
conflicting fbdev driver. If you have such a driver loaded all bets are
off concerning the state of memory from the transition of UEFI to KMS
(rule: don't use more one driver for the same hw). The challenge of
preserving the contents and mode of UEFI is the task of Jesse's fastboot
work. As noted elsewhere, using the GOP queries might simplify the task
of reading back hw state -- except that we need such information anyway
whether or not we have a UEFI system.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12 11:58               ` Chris Wilson
@ 2012-06-12 12:16                 ` Daniel Vetter
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-12 12:16 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel Graphics Development, Ben Widawsky, Daniel Vetter

On Tue, Jun 12, 2012 at 12:58:20PM +0100, Chris Wilson wrote:
> On Tue, 12 Jun 2012 11:43:50 +0000, "Singh, Satyeshwar" <satyeshwar.singh@intel.com> wrote:
> > Hi,
> > I just want to confirm something here. We have a situation where we draw a splash screen through our firmware's graphics component (UEFI GOP driver) onto its frame buffer which resides in stolen memory. When we transition from firmware to the kernel, we would like to ensure that the same splash screen seamlessly continues to be displayed so we remap the pages from stolen memory into GTT. If we have purged firmware framebuffers before claiming the gtt, then would we have not essentially lost the chance to remap those pages?
> 
> That's a separate issue. What this patch is addressing is removing a
> conflicting fbdev driver. If you have such a driver loaded all bets are
> off concerning the state of memory from the transition of UEFI to KMS
> (rule: don't use more one driver for the same hw). The challenge of
> preserving the contents and mode of UEFI is the task of Jesse's fastboot
> work. As noted elsewhere, using the GOP queries might simplify the task
> of reading back hw state -- except that we need such information anyway
> whether or not we have a UEFI system.

Actually, this patch should also help in kicking the native uefi
framebuffer driver - in case people use such a thing. But otherwise Chris
is right - preserving the existing output state and framebuffer contents
is a fully orthogonal issue. This patch only solves driver arbitrage
within the linux kernel.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
  2012-06-12  9:28           ` Daniel Vetter
  2012-06-12 11:43             ` Singh, Satyeshwar
@ 2012-06-13 11:30             ` Daniel Vetter
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-06-13 11:30 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky

On Tue, Jun 12, 2012 at 11:28:17AM +0200, Daniel Vetter wrote:
> Especially vesafb likes to map everything as uc- (yikes), and if that
> mapping hangs around still while we try to map the gtt as wc the
> kernel will downgrade our request to uc-, resulting in abyssal
> performance.
> 
> Unfortunately we can't do this as early as readon does (i.e. as the
> first thing we do when initializing the hw) because our fb/mmio space
> region moves around on a per-gen basis. So I've had to move it below
> the gtt initialization, but that seems to work, too. The important
> thing is that we do this before we set up the gtt wc mapping.
> 
> Now an altogether different question is why people compile their
> kernels with vesafb enabled, but I guess making things just work isn't
> bad per se ...
> 
> v2:
> - s/radeondrmfb/inteldrmfb/
> - fix up error handling
> 
> v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.
> 
> v4: Jani Nikula complained about the pointless bool primary
> initialization.
> 
> v5: Don't oops if we can't allocate, noticed by Chris Wilson.
> 
> Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've pushed this to dinq with Chris' irc bikeshed fixed and r-b added.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt
@ 2012-07-01 15:09 Daniel Vetter
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2012-07-01 15:09 UTC (permalink / raw)
  To: Intel Graphics Development
  Cc: Dave Airlie, Daniel Vetter, stable, DRI Development

Especially vesafb likes to map everything as uc- (yikes), and if that
mapping hangs around still while we try to map the gtt as wc the
kernel will downgrade our request to uc-, resulting in abyssal
performance.

Unfortunately we can't do this as early as readon does (i.e. as the
first thing we do when initializing the hw) because our fb/mmio space
region moves around on a per-gen basis. So I've had to move it below
the gtt initialization, but that seems to work, too. The important
thing is that we do this before we set up the gtt wc mapping.

Now an altogether different question is why people compile their
kernels with vesafb enabled, but I guess making things just work isn't
bad per se ...

v2:
- s/radeondrmfb/inteldrmfb/
- fix up error handling

v3: Kill #ifdef X86, this is Intel after all. Noticed by Ben Widawsky.

v4: Jani Nikula complained about the pointless bool primary
initialization.

v5: Don't oops if we can't allocate, noticed by Chris Wilson.

v6: Resolve conflicts with agp rework and fixup whitespace.

This is commit e188719a2891f01b3100d in drm-next.

Backport to 3.5 -fixes queue requested by Dave Airlie - due to grub
using vesa on fedora their initrd seems to load vesafb before loading
the real kms driver. So tons more people actually experience a
dead-slow gpu. Hence also the Cc: stable.

Cc: Dave Airlie <airlied@linux.ie>
Cc: stable@vger.kernel.org
Reported-and-tested-by: "Kilarski, Bernard R" <bernard.r.kilarski@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |   37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f947926..36822b9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1401,6 +1401,27 @@ i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
 	}
 }
 
+static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
+{
+	struct apertures_struct *ap;
+	struct pci_dev *pdev = dev_priv->dev->pdev;
+	bool primary;
+
+	ap = alloc_apertures(1);
+	if (!ap)
+		return;
+
+	ap->ranges[0].base = dev_priv->dev->agp->base;
+	ap->ranges[0].size =
+		dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
+	primary =
+		pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+
+	remove_conflicting_framebuffers(ap, "inteldrmfb", primary);
+
+	kfree(ap);
+}
+
 /**
  * i915_driver_load - setup chip and create an initial config
  * @dev: DRM device
@@ -1446,6 +1467,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+	dev_priv->mm.gtt = intel_gtt_get();
+	if (!dev_priv->mm.gtt) {
+		DRM_ERROR("Failed to initialize GTT\n");
+		ret = -ENODEV;
+		goto put_bridge;
+	}
+
+	i915_kick_out_firmware_fb(dev_priv);
+
 	pci_set_master(dev->pdev);
 
 	/* overlay on gen2 is broken and can't address above 1G */
@@ -1471,13 +1501,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	dev_priv->mm.gtt = intel_gtt_get();
-	if (!dev_priv->mm.gtt) {
-		DRM_ERROR("Failed to initialize GTT\n");
-		ret = -ENODEV;
-		goto out_rmmap;
-	}
-
 	aperture_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	dev_priv->mm.gtt_mapping =
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-07-01 15:09 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-11 15:07 [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt Daniel Vetter
2012-06-11 15:35 ` Ville Syrjälä
2012-06-11 16:13   ` Daniel Vetter
2012-06-11 16:28     ` Daniel Vetter
2012-06-11 23:43       ` Ben Widawsky
2012-06-12  8:12         ` Daniel Vetter
2012-06-12  8:24         ` Daniel Vetter
2012-06-12  8:43           ` Daniel Vetter
2012-06-12  9:28           ` Daniel Vetter
2012-06-12 11:43             ` Singh, Satyeshwar
2012-06-12 11:58               ` Chris Wilson
2012-06-12 12:16                 ` Daniel Vetter
2012-06-13 11:30             ` Daniel Vetter
2012-06-12  8:52       ` Chris Wilson
2012-06-11 15:58 ` Jani Nikula
2012-07-01 15:09 Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.