[Intel-gfx] [PATCH] drm/i915: kick any firmware framebuffers before claiming the gtt

[Daniel Vetter]

On Tue, Jun 12, 2012 at 1:43 AM, Ben Widawsky <ben at 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 at 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 at intel.com>
>> Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Reviewed-by: Ben Widawsky <ben at 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 at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch