Re: [PATCH 06/10 v2] drm/i915: Stop using AGP layer for GEN6+

[Chris Wilson <chris@chris-wilson.co.uk>]

On Fri, 26 Oct 2012 13:54:24 -0700, Ben Widawsky <ben@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@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