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

Chris Wilson
2015-06-09T08:56:57Z

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

--
Chris Wilson, Intel Open Source Technology Centre