On 06/04/16 11:05, Chris Wilson wrote:
On Wed, Apr 06, 2016 at 10:49:36AM +0100, Tvrtko Ursulin wrote:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 985f067c1f0e..dc8e1b76c896 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
list_del(&obj->global_list);
if (obj->vmapping) {
- vunmap(obj->vmapping);
+ if (obj->base.size == PAGE_SIZE)
+ kunmap(kmap_to_page(obj->vmapping));
+ else
+ vunmap(obj->vmapping);
Can't think of a reason why it would be better but there is also
is_vmalloc_addr(addr) as used by kvfree.
For consistency with the shrinker (see below).
What I don't like here is the repetition (and correlation) of the
PAGE_SIZE test, which has to be kept in sync with the corresponding one
at the point where the mapping was set up. If we're going to overload
the same field to store two different types of mapping, there should be
an explicit flag to say which we chose. Or failing that, then actually
test the mapping itself (as in is_vmalloc_addr()).
obj->vmapping = NULL;
}
@@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
i915_gem_object_pin_pages(obj);
if (obj->vmapping == NULL) {
- struct sg_page_iter sg_iter;
struct page **pages;
- int n;
- n = obj->base.size >> PAGE_SHIFT;
- pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
+ pages = NULL;
+ if (obj->base.size == PAGE_SIZE)
+ obj->vmapping = kmap(sg_page(obj->pages->sgl));
+ else
+ pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
+ sizeof(*pages),
+ GFP_TEMPORARY);
if (pages != NULL) {
+ struct sg_page_iter sg_iter;
+ int n;
+
n = 0;
- for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
+ for_each_sg_page(obj->pages->sgl, &sg_iter,
+ obj->pages->nents, 0)
pages[n++] = sg_page_iter_page(&sg_iter);
obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
Two problems I can spot are:
1. Callers of pin_vmap now don't know which kind of address they are
getting. Maybe call it pin_kvmap or something? Just mention in
kerneldoc could be enough.
I think just mention, and we can rename this to i915_gem_object_pin_map().
Hmm. I liked the pin in the name since it ties to to pin_pages (later
I plan to change that to get_pages and get_vmap/get_map as the pin
becomes implicit).
2. Shrinker will try to kick out kmapped objects because they have
obj->vmapping set.
Not caring that much since the vmap_purge is very heavy weight, but we
can apply is_vmalloc_addr() to the shrinker.
Ok, happy to call this obj->mapping and i915_gem_object_pin_map() ?
-Chris
Quite happy with the rename, and returning either type (a (virtual)
address is just an address), but not with the implementation repeating
the decision code.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
