[Intel-gfx] [PATCH v3 3/4] drm/i915: Add a partial GGTT view type

[Joonas Lahtinen]

Hi,

As discussed in IRC, this patch is not relevant. The interface is bit
misbehaving. CC'd Imre who agreed to go and change the interface to more
intuitive one.

Regards, Joonas

On ti, 2015-06-09 at 09:56 +0100, Chris Wilson wrote:
> On Wed, May 06, 2015 at 02:35:38PM +0300, Joonas Lahtinen wrote:
> > +static struct sg_table *
> > +intel_partial_pages(const struct i915_ggtt_view *view,
> > +		    struct drm_i915_gem_object *obj)
> > +{
> > +	struct sg_table *st;
> > +	struct scatterlist *sg;
> > +	struct sg_page_iter obj_sg_iter;
> > +	int ret = -ENOMEM;
> > +
> > +	st = kmalloc(sizeof(*st), GFP_KERNEL);
> > +	if (!st)
> > +		goto err_st_alloc;
> > +
> > +	ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_sg_alloc;
> > +
> > +	sg = st->sgl;
> > +	st->nents = 0;
> > +	for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
> > +		view->params.partial.offset)
> > +	{
> > +		if (st->nents >= view->params.partial.size)
> > +			break;
> 
> This is a nasty bug, as is the converse where st->nents <
> st->orig_nents.
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a7e39d4..115df10 100644
> @@ -2890,7 +2900,7 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>                     struct drm_i915_gem_object *obj)
>  {
>         struct sg_table *st;
> -       struct scatterlist *sg;
> +       struct scatterlist *sg, *end;
>         struct sg_page_iter obj_sg_iter;
>         int ret = -ENOMEM;
>  
> @@ -2902,24 +2912,31 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>         if (ret)
>                 goto err_sg_alloc;
>  
> -       sg = st->sgl;
> +       end = sg = st->sgl;
>         st->nents = 0;
>         for_each_sg_page(obj->pages->sgl, &obj_sg_iter, obj->pages->nents,
>                 view->params.partial.offset)
>         {
> -               if (st->nents >= view->params.partial.size)
> -                       break;
> +               if (WARN_ON(st->nents >= view->params.partial.size)) {
> +                       ret = -ENODEV;
> +                       goto err_pages;
> +               }
>  
>                 sg_set_page(sg, NULL, PAGE_SIZE, 0);
>                 sg_dma_address(sg) = sg_page_iter_dma_address(&obj_sg_iter);
>                 sg_dma_len(sg) = PAGE_SIZE;
>  
> +               end = sg;
>                 sg = sg_next(sg);
>                 st->nents++;
>         }
> +       sg_mark_end(end);
>  
>         return st;
>  
> +err_pages:
> +       sg_free_table(st);
>  err_sg_alloc:
>         kfree(st);
>  err_st_alloc:
> 
> -Chris
>