On la, 2015-07-11 at 21:54 +0100, Chris Wilson wrote: > On Thu, Jul 09, 2015 at 12:59:05PM +0300, Imre Deak wrote: > > +static int > > +__i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj, > > + struct page **pvec, int num_pages) > > +{ > > + int ret; > > + > > + ret = st_set_pages(&obj->pages, pvec, num_pages); > > + if (ret) > > + return ret; > > + > > + ret = i915_gem_gtt_prepare_object(obj); > > + if (ret) { > > + sg_free_table(obj->pages); > > + kfree(obj->pages); > > + obj->pages = NULL; > > Oh dear, we just leaked a ref one each page. To summarize the IRC discussion on this: it would be logical that sg_set_page() takes a ref - and in that case this would result in leaking those refs - but this is not so. Instead we rely on the GUP refs which we keep in case of success by setting pinned=0 (release_pages will be a nop) and drop in case of failure by passing the original pinned value to release_pages(). So after checking both the sync and async userptr paths this looks ok to me. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx