Re: [Intel-gfx] [PATCH 16/42] drm/i915: Refactor object page API

2016-10-13 Thread Chris Wilson 
On Thu, Oct 13, 2016 at 02:04:18PM +0300, Joonas Lahtinen wrote:
> On pe, 2016-10-07 at 10:46 +0100, Chris Wilson wrote:
> > @@ -2483,24 +2474,25 @@ i915_gem_object_get_pages(struct 
> > drm_i915_gem_object *obj)
> >  
> >     lockdep_assert_held(>base.dev->struct_mutex);
> >  
> > -   if (obj->pages)
> > +   if (obj->mm.pages)
> >     return 0;
> >  
> > -   if (obj->madv != I915_MADV_WILLNEED) {
> > +   if (obj->mm.madv != I915_MADV_WILLNEED) {
> >     DRM_DEBUG("Attempting to obtain a purgeable object\n");
> > +   __i915_gem_object_unpin_pages(obj);
> 
> Confusing to have teardown of another function in here.
> 
> >     return -EFAULT;
> >     }
> >  
> > -   BUG_ON(obj->pages_pin_count);
> > -
> >     ret = ops->get_pages(obj);
> > -   if (ret)
> > +   if (ret) {
> > +   __i915_gem_object_unpin_pages(obj);
> 
> And if you like *really* have to, at least try not to duplicate code.
> Bonus points form moving this to be proper teardown path where it has a
> counter-part.

It is here, so that the unlikely error handling is not inside the
inlined function, but where we expect the code to grow to handle the
more complex locking requirements.
-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 16/42] drm/i915: Refactor object page API

2016-10-13 Thread Joonas Lahtinen 
On pe, 2016-10-07 at 10:46 +0100, Chris Wilson wrote:
> +static inline int __must_check
> +i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
>  {
> - BUG_ON(obj->pages == NULL);
> - obj->pages_pin_count++;
> + lockdep_assert_held(>base.dev->struct_mutex);

\n here.

> + if (obj->mm.pages_pin_count++)
> + return 0;
> +
> + return __i915_gem_object_get_pages(obj);

I also second John here, what's up with the dummy wrapper convetion.
Add TODO comments if you intend to introduce new stuff relying on that.

> +}
> +
> +static inline void
> +__i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
> +{
> + lockdep_assert_held(>base.dev->struct_mutex);

Ditto.

> + GEM_BUG_ON(!obj->mm.pages);
> + obj->mm.pages_pin_count++;
> +}
> +
> +static inline bool
> +i915_gem_object_has_pinned_pages(struct drm_i915_gem_object *obj)
> +{
> + return obj->mm.pages_pin_count;
> +}
> +
> +static inline void
> +__i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
> +{
> + lockdep_assert_held(>base.dev->struct_mutex);
> + GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
> + GEM_BUG_ON(!obj->mm.pages);

\n at least here.

> + obj->mm.pages_pin_count--;
>  }
>  



> @@ -812,7 +809,7 @@ int i915_gem_obj_prepare_shmem_write(struct 
> drm_i915_gem_object *obj,
>   obj->cache_dirty = true;
>  
>   intel_fb_obj_invalidate(obj, ORIGIN_CPU);
> - obj->dirty = 1;
> + obj->mm.dirty = true;

The type is not bool dirty : 1, do we get yelled at by compiler?

> @@ -1268,7 +1261,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
>   goto out_unpin;
>  
>   intel_fb_obj_invalidate(obj, ORIGIN_CPU);
> - obj->dirty = true;
> + obj->mm.dirty = true;

I assume not as this has not been changed. Consistency is good anyway.

> @@ -2483,24 +2474,25 @@ i915_gem_object_get_pages(struct drm_i915_gem_object 
> *obj)
>  
>   lockdep_assert_held(>base.dev->struct_mutex);
>  
> - if (obj->pages)
> + if (obj->mm.pages)
>   return 0;
>  
> - if (obj->madv != I915_MADV_WILLNEED) {
> + if (obj->mm.madv != I915_MADV_WILLNEED) {
>   DRM_DEBUG("Attempting to obtain a purgeable object\n");
> + __i915_gem_object_unpin_pages(obj);

Confusing to have teardown of another function in here.

>   return -EFAULT;
>   }
>  
> - BUG_ON(obj->pages_pin_count);
> -
>   ret = ops->get_pages(obj);
> - if (ret)
> + if (ret) {
> + __i915_gem_object_unpin_pages(obj);

And if you like *really* have to, at least try not to duplicate code.
Bonus points form moving this to be proper teardown path where it has a
counter-part.

Strange side effects on failing function call do not lead to very
understandable code structure, but major spaghetti. Can be fixed in
follow-up patch too.

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/42] drm/i915: Refactor object page API

2016-10-11 Thread Tvrtko Ursulin 


On 07/10/2016 10:46, Chris Wilson wrote:

The plan is to make obtaining the backing storage for the object avoid
struct_mutex (i.e. use its own locking). The first step is to update the
API so that normal users only call pin/unpin whilst working on the
backing storage.

Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_cmd_parser.c   |   2 +-
  drivers/gpu/drm/i915/i915_debugfs.c  |  17 +--
  drivers/gpu/drm/i915/i915_drv.h  |  89 
  drivers/gpu/drm/i915/i915_gem.c  | 207 +--
  drivers/gpu/drm/i915/i915_gem_batch_pool.c   |   3 +-
  drivers/gpu/drm/i915/i915_gem_dmabuf.c   |  14 +-
  drivers/gpu/drm/i915/i915_gem_execbuffer.c   |   2 +-
  drivers/gpu/drm/i915/i915_gem_fence.c|   4 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c  |  10 +-
  drivers/gpu/drm/i915/i915_gem_internal.c |  19 +--
  drivers/gpu/drm/i915/i915_gem_render_state.c |   2 +-
  drivers/gpu/drm/i915/i915_gem_shrinker.c |  10 +-
  drivers/gpu/drm/i915/i915_gem_stolen.c   |  24 ++--
  drivers/gpu/drm/i915/i915_gem_tiling.c   |   8 +-
  drivers/gpu/drm/i915/i915_gem_userptr.c  |  30 ++--
  drivers/gpu/drm/i915/i915_gpu_error.c|   4 +-
  drivers/gpu/drm/i915/intel_lrc.c |   6 +-
  17 files changed, 234 insertions(+), 217 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c 
b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 70980f82a15b..8d20020cb9f9 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1290,7 +1290,7 @@ int intel_engine_cmd_parser(struct intel_engine_cs 
*engine,
}
  
  	if (ret == 0 && needs_clflush_after)

-   drm_clflush_virt_range(shadow_batch_obj->mapping, batch_len);
+   drm_clflush_virt_range(shadow_batch_obj->mm.mapping, batch_len);
i915_gem_object_unpin_map(shadow_batch_obj);
  
  	return ret;

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index e4b5ba771bea..b807ddf65e04 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -112,7 +112,7 @@ static char get_global_flag(struct drm_i915_gem_object *obj)
  
  static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)

  {
-   return obj->mapping ? 'M' : ' ';
+   return obj->mm.mapping ? 'M' : ' ';
  }
  
  static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)

@@ -158,8 +158,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
   i915_gem_active_get_seqno(>last_write,
 >base.dev->struct_mutex),
   i915_cache_level_str(dev_priv, obj->cache_level),
-  obj->dirty ? " dirty" : "",
-  obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
+  obj->mm.dirty ? " dirty" : "",
+  obj->mm.madv == I915_MADV_DONTNEED ? " purgeable" : "");
if (obj->base.name)
seq_printf(m, " (name: %d)", obj->base.name);
list_for_each_entry(vma, >vma_list, obj_link) {
@@ -411,12 +411,12 @@ static int i915_gem_object_info(struct seq_file *m, void 
*data)
size += obj->base.size;
++count;
  
-		if (obj->madv == I915_MADV_DONTNEED) {

+   if (obj->mm.madv == I915_MADV_DONTNEED) {
purgeable_size += obj->base.size;
++purgeable_count;
}
  
-		if (obj->mapping) {

+   if (obj->mm.mapping) {
mapped_count++;
mapped_size += obj->base.size;
}
@@ -433,12 +433,12 @@ static int i915_gem_object_info(struct seq_file *m, void 
*data)
++dpy_count;
}
  
-		if (obj->madv == I915_MADV_DONTNEED) {

+   if (obj->mm.madv == I915_MADV_DONTNEED) {
purgeable_size += obj->base.size;
++purgeable_count;
}
  
-		if (obj->mapping) {

+   if (obj->mm.mapping) {
mapped_count++;
mapped_size += obj->base.size;
}
@@ -2018,7 +2018,7 @@ static void i915_dump_lrc_obj(struct seq_file *m,
seq_printf(m, "\tBound in GGTT at 0x%08x\n",
   i915_ggtt_offset(vma));
  
-	if (i915_gem_object_get_pages(vma->obj)) {

+   if (i915_gem_object_pin_pages(vma->obj)) {
seq_puts(m, "\tFailed to get pages for context object\n\n");
return;
}
@@ -2037,6 +2037,7 @@ static void i915_dump_lrc_obj(struct seq_file *m,
kunmap_atomic(reg_state);
}
  
+	i915_gem_object_unpin_pages(vma->obj);

seq_putc(m, '\n');
  }
  
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h

index a96b446d8db4..3c22d49005fe

Re: [Intel-gfx] [PATCH 16/42] drm/i915: Refactor object page API

2016-10-10 Thread John Harrison 

On 07/10/2016 10:46, Chris Wilson wrote:

The plan is to make obtaining the backing storage for the object avoid
struct_mutex (i.e. use its own locking). The first step is to update the
API so that normal users only call pin/unpin whilst working on the
backing storage.

Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_cmd_parser.c   |   2 +-
  drivers/gpu/drm/i915/i915_debugfs.c  |  17 +--
  drivers/gpu/drm/i915/i915_drv.h  |  89 
  drivers/gpu/drm/i915/i915_gem.c  | 207 +--
  drivers/gpu/drm/i915/i915_gem_batch_pool.c   |   3 +-
  drivers/gpu/drm/i915/i915_gem_dmabuf.c   |  14 +-
  drivers/gpu/drm/i915/i915_gem_execbuffer.c   |   2 +-
  drivers/gpu/drm/i915/i915_gem_fence.c|   4 +-
  drivers/gpu/drm/i915/i915_gem_gtt.c  |  10 +-
  drivers/gpu/drm/i915/i915_gem_internal.c |  19 +--
  drivers/gpu/drm/i915/i915_gem_render_state.c |   2 +-
  drivers/gpu/drm/i915/i915_gem_shrinker.c |  10 +-
  drivers/gpu/drm/i915/i915_gem_stolen.c   |  24 ++--
  drivers/gpu/drm/i915/i915_gem_tiling.c   |   8 +-
  drivers/gpu/drm/i915/i915_gem_userptr.c  |  30 ++--
  drivers/gpu/drm/i915/i915_gpu_error.c|   4 +-
  drivers/gpu/drm/i915/intel_lrc.c |   6 +-
  17 files changed, 234 insertions(+), 217 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c 
b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 70980f82a15b..8d20020cb9f9 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1290,7 +1290,7 @@ int intel_engine_cmd_parser(struct intel_engine_cs 
*engine,
}
  
  	if (ret == 0 && needs_clflush_after)

-   drm_clflush_virt_range(shadow_batch_obj->mapping, batch_len);
+   drm_clflush_virt_range(shadow_batch_obj->mm.mapping, batch_len);
i915_gem_object_unpin_map(shadow_batch_obj);
  
  	return ret;

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index e4b5ba771bea..b807ddf65e04 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -112,7 +112,7 @@ static char get_global_flag(struct drm_i915_gem_object *obj)
  
  static char get_pin_mapped_flag(struct drm_i915_gem_object *obj)

  {
-   return obj->mapping ? 'M' : ' ';
+   return obj->mm.mapping ? 'M' : ' ';
  }
  
  static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)

@@ -158,8 +158,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
   i915_gem_active_get_seqno(>last_write,
 >base.dev->struct_mutex),
   i915_cache_level_str(dev_priv, obj->cache_level),
-  obj->dirty ? " dirty" : "",
-  obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
+  obj->mm.dirty ? " dirty" : "",
+  obj->mm.madv == I915_MADV_DONTNEED ? " purgeable" : "");
if (obj->base.name)
seq_printf(m, " (name: %d)", obj->base.name);
list_for_each_entry(vma, >vma_list, obj_link) {
@@ -411,12 +411,12 @@ static int i915_gem_object_info(struct seq_file *m, void 
*data)
size += obj->base.size;
++count;
  
-		if (obj->madv == I915_MADV_DONTNEED) {

+   if (obj->mm.madv == I915_MADV_DONTNEED) {
purgeable_size += obj->base.size;
++purgeable_count;
}
  
-		if (obj->mapping) {

+   if (obj->mm.mapping) {
mapped_count++;
mapped_size += obj->base.size;
}
@@ -433,12 +433,12 @@ static int i915_gem_object_info(struct seq_file *m, void 
*data)
++dpy_count;
}
  
-		if (obj->madv == I915_MADV_DONTNEED) {

+   if (obj->mm.madv == I915_MADV_DONTNEED) {
purgeable_size += obj->base.size;
++purgeable_count;
}
  
-		if (obj->mapping) {

+   if (obj->mm.mapping) {
mapped_count++;
mapped_size += obj->base.size;
}
@@ -2018,7 +2018,7 @@ static void i915_dump_lrc_obj(struct seq_file *m,
seq_printf(m, "\tBound in GGTT at 0x%08x\n",
   i915_ggtt_offset(vma));
  
-	if (i915_gem_object_get_pages(vma->obj)) {

+   if (i915_gem_object_pin_pages(vma->obj)) {
seq_puts(m, "\tFailed to get pages for context object\n\n");

Error message needs updating to match the function call change.


return;
}
@@ -2037,6 +2037,7 @@ static void i915_dump_lrc_obj(struct seq_file *m,
kunmap_atomic(reg_state);
}
  
+	i915_gem_object_unpin_pages(vma->obj);

seq_putc(m, '\n');
  }
  
diff --git a/drivers/gpu/drm/i915/i915_drv.h