Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Dave Gordon]

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(>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, _iter, 
obj->pages->nents, 0)
+   for_each_sg_page(obj->pages->sgl, _iter,
+obj->pages->nents, 0)
pages[n++] = sg_page_iter_page(_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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Tvrtko Ursulin]

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(>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).


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, _iter, 
obj->pages->nents, 0)
+   for_each_sg_page(obj->pages->sgl, _iter,
+obj->pages->nents, 0)
pages[n++] = sg_page_iter_page(_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() ?


Sounds good. (Including the is_vmalloc_addr() smartening in the shrinker.)

Regards,

Tvrtko


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

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

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(>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).

> > 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, _iter, 
> >obj->pages->nents, 0)
> >+for_each_sg_page(obj->pages->sgl, _iter,
> >+ obj->pages->nents, 0)
> > pages[n++] = sg_page_iter_page(_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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Tvrtko Ursulin]

On 05/04/16 13:57, Chris Wilson wrote:

If we want a contiguous mapping of a single page sized object, we can
forgo using vmap() and just use a regular kmap(). Note that this is only
suitable if the desired pgprot_t is comptabitible.


You noticed the spelling already. :)



Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_gem.c | 22 --
  1 file changed, 16 insertions(+), 6 deletions(-)

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(>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.



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, _iter, 
obj->pages->nents, 0)
+   for_each_sg_page(obj->pages->sgl, _iter,
+obj->pages->nents, 0)
pages[n++] = sg_page_iter_page(_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.


2. Shrinker will try to kick out kmapped objects because they have 
obj->vmapping set.


Regards,

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

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Matthew Auld]

I use :autocmd FileType gitcommit setlocal spell
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

On Tue, Apr 05, 2016 at 01:57:37PM +0100, Chris Wilson wrote:
> If we want a contiguous mapping of a single page sized object, we can
> forgo using vmap() and just use a regular kmap(). Note that this is only
> suitable if the desired pgprot_t is comptabitible.

One day, I will enable :set spell. Does anyone know if there is a .vim
rule to detect git? Or something to stuff in git config core.editor ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

If we want a contiguous mapping of a single page sized object, we can
forgo using vmap() and just use a regular kmap(). Note that this is only
suitable if the desired pgprot_t is comptabitible.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

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(>global_list);
 
if (obj->vmapping) {
-   vunmap(obj->vmapping);
+   if (obj->base.size == PAGE_SIZE)
+   kunmap(kmap_to_page(obj->vmapping));
+   else
+   vunmap(obj->vmapping);
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, _iter, 
obj->pages->nents, 0)
+   for_each_sg_page(obj->pages->sgl, _iter,
+obj->pages->nents, 0)
pages[n++] = sg_page_iter_page(_iter);
 
obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
-- 
2.8.0.rc3

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