On Fri, 26 Oct 2012 13:54:24 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> As a quick hack we make the old intel_gtt structure mutable so we can
> fool a bunch of the existing code which depends on elements in that data
> structure. We can/should try to remove this in a subsequent patch.
>
> This should preserve the old gtt init behavior which upon writing these
> patches seems incorrect. The next patch will fix these things.
>
> The one exception is VLV which doesn't have the preserved flush control
> write behavior. Since we want to do that for all GEN6+ stuff, we'll
> handle that in a later patch. Mainstream VLV support doesn't actually
> exist yet anyway.
>
> v2: Update the comment to remove the "voodoo"
> Check that the last pte written matches what we readback
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> +static void gen6_ggtt_bind_object(struct drm_i915_gem_object *obj,
> + enum i915_cache_level level)
> +{
> + struct drm_device *dev = obj->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct sg_table *st = obj->pages;
> + struct scatterlist *sg = st->sgl;
> + const int first_entry = obj->gtt_space->start >> PAGE_SHIFT;
> + const int max_entries = dev_priv->mm.gtt->gtt_total_entries - first_entry;
> + gtt_pte_t __iomem *gtt_entries = dev_priv->mm.gtt->gtt + first_entry;
> + int unused, i = 0;
> + unsigned int len, m = 0;
> + dma_addr_t addr;
> +
> + for_each_sg(st->sgl, sg, st->nents, unused) {
> + len = sg_dma_len(sg) >> PAGE_SHIFT;
> + for (m = 0; m < len; m++) {
> + addr = sg_dma_address(sg) + (m << PAGE_SHIFT);
> + gtt_entries[i] = pte_encode(dev, addr, level);
> + i++;
> + if (WARN_ON(i > max_entries))
> + goto out;
> + }
> + }
In the clear range above, your detection of a programming error is much
better. And this is truly a BUG_ON() event because state is then
completely screwed up.
> +int i915_gem_gtt_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + phys_addr_t gtt_bus_addr;
> + u16 snb_gmch_ctl;
> + u32 tmp;
> + int ret;
> +
> + /* On modern platforms we need not worry ourself with the legacy
> + * hostbridge query stuff. Skip it entirely
> + */
> + if (INTEL_INFO(dev)->gen < 6) {
> + ret = intel_gmch_probe(dev_priv->bridge_dev, dev->pdev, NULL);
> + if (!ret) {
> + DRM_ERROR("failed to set up gmch\n");
> + return -EIO;
> + }
> +
> + dev_priv->mm.gtt = intel_gtt_get();
> + if (!dev_priv->mm.gtt) {
> + DRM_ERROR("Failed to initialize GTT\n");
> + intel_gmch_remove();
> + return -ENODEV;
> + }
> + return 0;
> + }
> +
> +
Two blank lines for the price of one. You really like to separate
yourself from the pre-HSW legacy...
> + dev_priv->mm.gtt = kzalloc(sizeof(*dev_priv->mm.gtt), GFP_KERNEL);
> + if (!dev_priv->mm.gtt)
> + return -ENOMEM;
> +
> + if (!pci_set_dma_mask(dev->pdev, DMA_BIT_MASK(40)))
> + pci_set_consistent_dma_mask(dev->pdev, DMA_BIT_MASK(40));
> +
> + pci_read_config_dword(dev->pdev, PCI_BASE_ADDRESS_0, &tmp);
> + /* For GEN6+ the PTEs for the ggtt live at 2MB + BAR0 */
> + gtt_bus_addr = (tmp & PCI_BASE_ADDRESS_MEM_MASK) + (2<<20);
> +
> + dev_priv->mm.gtt->gtt_mappable_entries = pci_resource_len(dev->pdev, 2) >> PAGE_SHIFT;
Probably best to check the value here and throw a paranoid tantrum in
case it is too small or too large.
> + pci_read_config_dword(dev->pdev, PCI_BASE_ADDRESS_2, &tmp);
> + dev_priv->mm.gtt->gma_bus_addr = tmp & PCI_BASE_ADDRESS_MEM_MASK;
> +
> + /* i9xx_setup */
> + pci_read_config_word(dev->pdev, SNB_GMCH_CTRL, &snb_gmch_ctl);
> + dev_priv->mm.gtt->gtt_total_entries =
> + gen6_get_total_gtt_size(snb_gmch_ctl) / sizeof(gtt_pte_t);
> + dev_priv->mm.gtt->stolen_size = gen6_get_stolen_size(snb_gmch_ctl);
> +
> + ret = setup_scratch_page(dev);
> + if (ret)
> + return ret;
Missing cleanup.
> +
> + dev_priv->mm.gtt->gtt = ioremap(gtt_bus_addr,
> + dev_priv->mm.gtt->gtt_total_entries * sizeof(gtt_pte_t));
Can fail.
> + /* GMADR is the PCI aperture used by SW to access tiled GFX surfaces in a linear fashion. */
> + DRM_DEBUG_DRIVER("GMADR size = %dM\n", dev_priv->mm.gtt->gtt_mappable_entries >> 8);
> + DRM_DEBUG_DRIVER("GTT total entries = %dK\n", dev_priv->mm.gtt->gtt_total_entries >> 10);
> + DRM_DEBUG_DRIVER("GTT stolen size = %dM\n", dev_priv->mm.gtt->stolen_size >> 20);
I think it would be nice for our users to have a one line INFO summarizing
the memory availabe to the graphics device.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
