Re: [PATCH] drm/i915: Use the existing pages when retrying to DMA map

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 20/12/2016 13:42, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Rather than freeing and re-allocating the pages when DMA mapping
> in large chunks fails, we can just rebuild the sg table with no
> coalescing.
>
> Also change back the page counter to unsigned int because that
> is what the sg API supports.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> Only compile tested!
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5275f6248ce3..e73f9f5a5d23 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2340,12 +2340,36 @@ static void i915_sg_trim(struct sg_table *orig_st)
>  	*orig_st = new_st;
>  }
>
> +static void i915_sg_uncoalesce(struct sg_table *orig_st, unsigned long nents)
> +{
> +	struct sg_table new_st;
> +	struct scatterlist *new_sg;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
> +
> +	if (sg_alloc_table(&new_st, nents, GFP_KERNEL))
> +		return;
> +
> +	new_sg = new_st.sgl;
> +	for_each_sgt_page(page, sgt_iter, orig_st) {
> +		sg_set_page(new_sg, page, PAGE_SIZE, 0);
> +		/* called before being DMA mapped, no need to copy sg->dma_* */
> +		new_sg = sg_next(new_sg);
> +	}
> +
> +	GEM_BUG_ON(new_sg); /* Should walk exactly nents and hit the end */
> +
> +	sg_free_table(orig_st);
> +
> +	*orig_st = new_st;
> +}
> +
>  static struct sg_table *
>  i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> -	const unsigned long page_count = obj->base.size / PAGE_SIZE;
> -	unsigned long i;
> +	const unsigned int page_count = obj->base.size / PAGE_SIZE;
> +	unsigned int i;
>  	struct address_space *mapping;
>  	struct sg_table *st;
>  	struct scatterlist *sg;
> @@ -2371,7 +2395,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	if (st == NULL)
>  		return ERR_PTR(-ENOMEM);
>
> -rebuild_st:
>  	if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
>  		kfree(st);
>  		return ERR_PTR(-ENOMEM);
> @@ -2429,6 +2452,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	/* Trim unused sg entries to avoid wasting memory. */
>  	i915_sg_trim(st);
>
> +prepare_gtt:
>  	ret = i915_gem_gtt_prepare_pages(obj, st);
>  	if (ret) {
>  		/* DMA remapping failed? One possible cause is that
> @@ -2436,16 +2460,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  		 * for PAGE_SIZE chunks instead may be helpful.
>  		 */
>  		if (max_segment > PAGE_SIZE) {
> -			for_each_sgt_page(page, sgt_iter, st)
> -				put_page(page);
> -			sg_free_table(st);
> -
> +			i915_sg_uncoalesce(st, page_count);
>  			max_segment = PAGE_SIZE;
> -			goto rebuild_st;
> +			goto prepare_gtt;
>  		} else {
>  			dev_warn(&dev_priv->drm.pdev->dev,
> -				 "Failed to DMA remap %lu pages\n",
> -				 page_count);
> +				 "Failed to DMA remap %u pages\n", page_count);
>  			goto err_pages;
>  		}
>  	}
>

Are you still against this? As a reminder it saves a put_page/allocate 
page-from-shmemfs cycle on dma mapping failures.


Regards,

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