subject:"Re\: \[Intel\-gfx\] \[PATCH 07\/13\] drm\/i915\: Introduce dedicated object VMA iterator"

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-13 Thread Tvrtko Ursulin



On 12/01/16 16:19, Daniel Vetter wrote:

On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote:


On 11/01/16 08:43, Daniel Vetter wrote:

On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote:


On 08/01/16 11:29, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

Purpose is to catch places which iterate the object VMA list
without holding the big lock.

Implemented by open coding list_for_each_entry to make the
macro compatible with existing call sites.

Signed-off-by: Tvrtko Ursulin 
Cc: Daniel Vetter 
---
   drivers/gpu/drm/i915/i915_debugfs.c  |  8 
   drivers/gpu/drm/i915/i915_drv.h  |  6 ++
   drivers/gpu/drm/i915/i915_gem.c  | 24 
   drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
   drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
   6 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 714a45cf8a51..d7c2a3201161 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
drm_i915_gem_object *obj)
u64 size = 0;
struct i915_vma *vma;

-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
if (i915_is_ggtt(vma->vm) &&
drm_mm_node_allocated(>node))
size += vma->node.size;
@@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
if (obj->base.name)
seq_printf(m, " (name: %d)", obj->base.name);
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
if (vma->pin_count > 0)
pin_count++;
}
@@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
seq_printf(m, " (display)");
if (obj->fence_reg != I915_FENCE_REG_NONE)
seq_printf(m, " (fence: %d)", obj->fence_reg);
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
   i915_is_ggtt(vma->vm) ? "g" : "pp",
   vma->node.start, vma->node.size);
@@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
stats->shared += obj->base.size;

if (USES_FULL_PPGTT(obj->base.dev)) {
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
struct i915_hw_ppgtt *ppgtt;

if (!drm_mm_node_allocated(>node))
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b77a5d84eac2..0406a020dfcc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
*i915_gem_object_create_from_data(
   void i915_gem_free_object(struct drm_gem_object *obj);
   void i915_gem_vma_destroy(struct i915_vma *vma);

+#define i915_gem_obj_for_each_vma(vma, obj) \
+   for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
+vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
+>vma_link != (&(obj)->vma_list); \
+vma = list_next_entry(vma, vma_link))
+



Unfortunately error capture is not happy with this approach. Can't even see
that error capture attempts to grab the mutex anywhere.

So what? Drop the idea or add a "doing error capture" flag somewhere?


Fix the bugs. Not surprise at all that we've screwed this up all over the
place ;-) Afaics modeset code isn't much better either ...


Ok I'll drop this patch then since the series contains fixes to all but one
related issues. The remaining one is then:

[   17.370366] [ cut here ]
[   17.375633] WARNING: CPU: 0 PID: 1128 at 
drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 
[i915]()
[   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
[   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet 
libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper 
acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
[   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G U  
4.4.0-rc8-160107+ #105
[   17.428771] Hardware name: Intel Corporation Broadwell Client 
platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
[   17.443161]  a0227790 8800a98439b8 81280d82 
8800a9843a00
[   17.451677]

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-12 Thread Daniel Vetter

On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote:
> 
> On 11/01/16 08:43, Daniel Vetter wrote:
> > On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote:
> >>
> >> On 08/01/16 11:29, Tvrtko Ursulin wrote:
> >>> From: Tvrtko Ursulin 
> >>>
> >>> Purpose is to catch places which iterate the object VMA list
> >>> without holding the big lock.
> >>>
> >>> Implemented by open coding list_for_each_entry to make the
> >>> macro compatible with existing call sites.
> >>>
> >>> Signed-off-by: Tvrtko Ursulin 
> >>> Cc: Daniel Vetter 
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_debugfs.c  |  8 
> >>>   drivers/gpu/drm/i915/i915_drv.h  |  6 ++
> >>>   drivers/gpu/drm/i915/i915_gem.c  | 24 
> >>>   drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
> >>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
> >>>   drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
> >>>   6 files changed, 26 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> >>> b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> index 714a45cf8a51..d7c2a3201161 100644
> >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
> >>> drm_i915_gem_object *obj)
> >>>   u64 size = 0;
> >>>   struct i915_vma *vma;
> >>>
> >>> - list_for_each_entry(vma, >vma_list, vma_link) {
> >>> + i915_gem_obj_for_each_vma(vma, obj) {
> >>>   if (i915_is_ggtt(vma->vm) &&
> >>>   drm_mm_node_allocated(>node))
> >>>   size += vma->node.size;
> >>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct 
> >>> drm_i915_gem_object *obj)
> >>>  obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> >>>   if (obj->base.name)
> >>>   seq_printf(m, " (name: %d)", obj->base.name);
> >>> - list_for_each_entry(vma, >vma_list, vma_link) {
> >>> + i915_gem_obj_for_each_vma(vma, obj) {
> >>>   if (vma->pin_count > 0)
> >>>   pin_count++;
> >>>   }
> >>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct 
> >>> drm_i915_gem_object *obj)
> >>>   seq_printf(m, " (display)");
> >>>   if (obj->fence_reg != I915_FENCE_REG_NONE)
> >>>   seq_printf(m, " (fence: %d)", obj->fence_reg);
> >>> - list_for_each_entry(vma, >vma_list, vma_link) {
> >>> + i915_gem_obj_for_each_vma(vma, obj) {
> >>>   seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> >>>  i915_is_ggtt(vma->vm) ? "g" : "pp",
> >>>  vma->node.start, vma->node.size);
> >>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void 
> >>> *data)
> >>>   stats->shared += obj->base.size;
> >>>
> >>>   if (USES_FULL_PPGTT(obj->base.dev)) {
> >>> - list_for_each_entry(vma, >vma_list, vma_link) {
> >>> + i915_gem_obj_for_each_vma(vma, obj) {
> >>>   struct i915_hw_ppgtt *ppgtt;
> >>>
> >>>   if (!drm_mm_node_allocated(>node))
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> >>> b/drivers/gpu/drm/i915/i915_drv.h
> >>> index b77a5d84eac2..0406a020dfcc 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
> >>> *i915_gem_object_create_from_data(
> >>>   void i915_gem_free_object(struct drm_gem_object *obj);
> >>>   void i915_gem_vma_destroy(struct i915_vma *vma);
> >>>
> >>> +#define i915_gem_obj_for_each_vma(vma, obj) \
> >>> + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> >>> +  vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >>> +  >vma_link != (&(obj)->vma_list); \
> >>> +  vma = list_next_entry(vma, vma_link))
> >>> +
> >>
> >>
> >> Unfortunately error capture is not happy with this approach. Can't even see
> >> that error capture attempts to grab the mutex anywhere.
> >>
> >> So what? Drop the idea or add a "doing error capture" flag somewhere?
> > 
> > Fix the bugs. Not surprise at all that we've screwed this up all over the
> > place ;-) Afaics modeset code isn't much better either ...
> 
> Ok I'll drop this patch then since the series contains fixes to all but one
> related issues. The remaining one is then:
> 
> [   17.370366] [ cut here ]
> [   17.375633] WARNING: CPU: 0 PID: 1128 at 
> drivers/gpu/drm/i915/i915_gem.c:5166 
> i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
> [   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
> [   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet 
> libphy mii i915 gpio_lynxpoint i2c_hid

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-12 Thread Maarten Lankhorst

Op 12-01-16 om 17:19 schreef Daniel Vetter:
> On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote:
>> On 11/01/16 08:43, Daniel Vetter wrote:
>>> On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote:
 On 08/01/16 11:29, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
>
> Purpose is to catch places which iterate the object VMA list
> without holding the big lock.
>
> Implemented by open coding list_for_each_entry to make the
> macro compatible with existing call sites.
>
> Signed-off-by: Tvrtko Ursulin 
> Cc: Daniel Vetter 
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  |  8 
>   drivers/gpu/drm/i915/i915_drv.h  |  6 ++
>   drivers/gpu/drm/i915/i915_gem.c  | 24 
>   drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>   drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
>   6 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 714a45cf8a51..d7c2a3201161 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
> drm_i915_gem_object *obj)
>   u64 size = 0;
>   struct i915_vma *vma;
>
> - list_for_each_entry(vma, >vma_list, vma_link) {
> + i915_gem_obj_for_each_vma(vma, obj) {
>   if (i915_is_ggtt(vma->vm) &&
>   drm_mm_node_allocated(>node))
>   size += vma->node.size;
> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct 
> drm_i915_gem_object *obj)
>  obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>   if (obj->base.name)
>   seq_printf(m, " (name: %d)", obj->base.name);
> - list_for_each_entry(vma, >vma_list, vma_link) {
> + i915_gem_obj_for_each_vma(vma, obj) {
>   if (vma->pin_count > 0)
>   pin_count++;
>   }
> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct 
> drm_i915_gem_object *obj)
>   seq_printf(m, " (display)");
>   if (obj->fence_reg != I915_FENCE_REG_NONE)
>   seq_printf(m, " (fence: %d)", obj->fence_reg);
> - list_for_each_entry(vma, >vma_list, vma_link) {
> + i915_gem_obj_for_each_vma(vma, obj) {
>   seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>  i915_is_ggtt(vma->vm) ? "g" : "pp",
>  vma->node.start, vma->node.size);
> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void 
> *data)
>   stats->shared += obj->base.size;
>
>   if (USES_FULL_PPGTT(obj->base.dev)) {
> - list_for_each_entry(vma, >vma_list, vma_link) {
> + i915_gem_obj_for_each_vma(vma, obj) {
>   struct i915_hw_ppgtt *ppgtt;
>
>   if (!drm_mm_node_allocated(>node))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h
> index b77a5d84eac2..0406a020dfcc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
> *i915_gem_object_create_from_data(
>   void i915_gem_free_object(struct drm_gem_object *obj);
>   void i915_gem_vma_destroy(struct i915_vma *vma);
>
> +#define i915_gem_obj_for_each_vma(vma, obj) \
> + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> +  vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> +  >vma_link != (&(obj)->vma_list); \
> +  vma = list_next_entry(vma, vma_link))
> +

 Unfortunately error capture is not happy with this approach. Can't even see
 that error capture attempts to grab the mutex anywhere.

 So what? Drop the idea or add a "doing error capture" flag somewhere?
>>> Fix the bugs. Not surprise at all that we've screwed this up all over the
>>> place ;-) Afaics modeset code isn't much better either ...
>> Ok I'll drop this patch then since the series contains fixes to all but one
>> related issues. The remaining one is then:
>>
>> [   17.370366] [ cut here ]
>> [   17.375633] WARNING: CPU: 0 PID: 1128 at 
>> drivers/gpu/drm/i915/i915_gem.c:5166 
>> i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
>> [   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
>> [   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet 
>>

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-11 Thread Daniel Vetter

On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote:
> 
> On 08/01/16 11:29, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin 
> >
> >Purpose is to catch places which iterate the object VMA list
> >without holding the big lock.
> >
> >Implemented by open coding list_for_each_entry to make the
> >macro compatible with existing call sites.
> >
> >Signed-off-by: Tvrtko Ursulin 
> >Cc: Daniel Vetter 
> >---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  8 
> >  drivers/gpu/drm/i915/i915_drv.h  |  6 ++
> >  drivers/gpu/drm/i915/i915_gem.c  | 24 
> >  drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
> >  drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
> >  6 files changed, 26 insertions(+), 20 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> >b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 714a45cf8a51..d7c2a3201161 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
> >drm_i915_gem_object *obj)
> > u64 size = 0;
> > struct i915_vma *vma;
> >
> >-list_for_each_entry(vma, >vma_list, vma_link) {
> >+i915_gem_obj_for_each_vma(vma, obj) {
> > if (i915_is_ggtt(vma->vm) &&
> > drm_mm_node_allocated(>node))
> > size += vma->node.size;
> >@@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct 
> >drm_i915_gem_object *obj)
> >obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> > if (obj->base.name)
> > seq_printf(m, " (name: %d)", obj->base.name);
> >-list_for_each_entry(vma, >vma_list, vma_link) {
> >+i915_gem_obj_for_each_vma(vma, obj) {
> > if (vma->pin_count > 0)
> > pin_count++;
> > }
> >@@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct 
> >drm_i915_gem_object *obj)
> > seq_printf(m, " (display)");
> > if (obj->fence_reg != I915_FENCE_REG_NONE)
> > seq_printf(m, " (fence: %d)", obj->fence_reg);
> >-list_for_each_entry(vma, >vma_list, vma_link) {
> >+i915_gem_obj_for_each_vma(vma, obj) {
> > seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> >i915_is_ggtt(vma->vm) ? "g" : "pp",
> >vma->node.start, vma->node.size);
> >@@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> > stats->shared += obj->base.size;
> >
> > if (USES_FULL_PPGTT(obj->base.dev)) {
> >-list_for_each_entry(vma, >vma_list, vma_link) {
> >+i915_gem_obj_for_each_vma(vma, obj) {
> > struct i915_hw_ppgtt *ppgtt;
> >
> > if (!drm_mm_node_allocated(>node))
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> >b/drivers/gpu/drm/i915/i915_drv.h
> >index b77a5d84eac2..0406a020dfcc 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
> >*i915_gem_object_create_from_data(
> >  void i915_gem_free_object(struct drm_gem_object *obj);
> >  void i915_gem_vma_destroy(struct i915_vma *vma);
> >
> >+#define i915_gem_obj_for_each_vma(vma, obj) \
> >+for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> >+ vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >+ >vma_link != (&(obj)->vma_list); \
> >+ vma = list_next_entry(vma, vma_link))
> >+
> 
> 
> Unfortunately error capture is not happy with this approach. Can't even see
> that error capture attempts to grab the mutex anywhere.
> 
> So what? Drop the idea or add a "doing error capture" flag somewhere?

Fix the bugs. Not surprise at all that we've screwed this up all over the
place ;-) Afaics modeset code isn't much better either ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-11 Thread Tvrtko Ursulin


On 11/01/16 08:43, Daniel Vetter wrote:
> On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote:
>>
>> On 08/01/16 11:29, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin 
>>>
>>> Purpose is to catch places which iterate the object VMA list
>>> without holding the big lock.
>>>
>>> Implemented by open coding list_for_each_entry to make the
>>> macro compatible with existing call sites.
>>>
>>> Signed-off-by: Tvrtko Ursulin 
>>> Cc: Daniel Vetter 
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c  |  8 
>>>   drivers/gpu/drm/i915/i915_drv.h  |  6 ++
>>>   drivers/gpu/drm/i915/i915_gem.c  | 24 
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
>>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>>   drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
>>>   6 files changed, 26 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 714a45cf8a51..d7c2a3201161 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
>>> drm_i915_gem_object *obj)
>>> u64 size = 0;
>>> struct i915_vma *vma;
>>>
>>> -   list_for_each_entry(vma, >vma_list, vma_link) {
>>> +   i915_gem_obj_for_each_vma(vma, obj) {
>>> if (i915_is_ggtt(vma->vm) &&
>>> drm_mm_node_allocated(>node))
>>> size += vma->node.size;
>>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct 
>>> drm_i915_gem_object *obj)
>>>obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>> if (obj->base.name)
>>> seq_printf(m, " (name: %d)", obj->base.name);
>>> -   list_for_each_entry(vma, >vma_list, vma_link) {
>>> +   i915_gem_obj_for_each_vma(vma, obj) {
>>> if (vma->pin_count > 0)
>>> pin_count++;
>>> }
>>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct 
>>> drm_i915_gem_object *obj)
>>> seq_printf(m, " (display)");
>>> if (obj->fence_reg != I915_FENCE_REG_NONE)
>>> seq_printf(m, " (fence: %d)", obj->fence_reg);
>>> -   list_for_each_entry(vma, >vma_list, vma_link) {
>>> +   i915_gem_obj_for_each_vma(vma, obj) {
>>> seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>>i915_is_ggtt(vma->vm) ? "g" : "pp",
>>>vma->node.start, vma->node.size);
>>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>> stats->shared += obj->base.size;
>>>
>>> if (USES_FULL_PPGTT(obj->base.dev)) {
>>> -   list_for_each_entry(vma, >vma_list, vma_link) {
>>> +   i915_gem_obj_for_each_vma(vma, obj) {
>>> struct i915_hw_ppgtt *ppgtt;
>>>
>>> if (!drm_mm_node_allocated(>node))
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index b77a5d84eac2..0406a020dfcc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
>>> *i915_gem_object_create_from_data(
>>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>>>
>>> +#define i915_gem_obj_for_each_vma(vma, obj) \
>>> +   for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>>> +vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>>> +>vma_link != (&(obj)->vma_list); \
>>> +vma = list_next_entry(vma, vma_link))
>>> +
>>
>>
>> Unfortunately error capture is not happy with this approach. Can't even see
>> that error capture attempts to grab the mutex anywhere.
>>
>> So what? Drop the idea or add a "doing error capture" flag somewhere?
> 
> Fix the bugs. Not surprise at all that we've screwed this up all over the
> place ;-) Afaics modeset code isn't much better either ...

Ok I'll drop this patch then since the series contains fixes to all but one
related issues. The remaining one is then:

[   17.370366] [ cut here ]
[   17.375633] WARNING: CPU: 0 PID: 1128 at 
drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 
[i915]()
[   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
[   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet 
libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper 
acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
[   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G U  
4.4.0-rc8-160107+ #105
[   17.428771] Hardware name: Intel Corporation Broadwell Client 
platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
[   17.443161]

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-11 Thread Daniel Vetter

On Fri, Jan 08, 2016 at 11:44:04AM +, Chris Wilson wrote:
> On Fri, Jan 08, 2016 at 11:29:46AM +, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin 
> > 
> > Purpose is to catch places which iterate the object VMA list
> > without holding the big lock.
> > 
> > Implemented by open coding list_for_each_entry to make the
> > macro compatible with existing call sites.
> > 
> > Signed-off-by: Tvrtko Ursulin 
> > Cc: Daniel Vetter 
> > +#define i915_gem_obj_for_each_vma(vma, obj) \
> > +   for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> 
> Let's not go around adding WARN(!mutex_locked) to GEM code when
> lockdep_assert_held doesn't add overhead outside of testing.

Hm yeah I still prefere WARN_ON for modeset code (where it doesn't matter)
because of increased test coverage. But for gem it indeed makes more sense
to only do this for lockdep-enabled builds. CI runs with lockdep, so we're
good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-08 Thread Chris Wilson

On Fri, Jan 08, 2016 at 11:29:46AM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Purpose is to catch places which iterate the object VMA list
> without holding the big lock.
> 
> Implemented by open coding list_for_each_entry to make the
> macro compatible with existing call sites.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Daniel Vetter 
> +#define i915_gem_obj_for_each_vma(vma, obj) \
> + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \

Let's not go around adding WARN(!mutex_locked) to GEM code when
lockdep_assert_held doesn't add overhead outside of testing.
-Chris

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

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

2016-01-08 Thread Tvrtko Ursulin



On 08/01/16 11:29, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

Purpose is to catch places which iterate the object VMA list
without holding the big lock.

Implemented by open coding list_for_each_entry to make the
macro compatible with existing call sites.

Signed-off-by: Tvrtko Ursulin 
Cc: Daniel Vetter 
---
  drivers/gpu/drm/i915/i915_debugfs.c  |  8 
  drivers/gpu/drm/i915/i915_drv.h  |  6 ++
  drivers/gpu/drm/i915/i915_gem.c  | 24 
  drivers/gpu/drm/i915/i915_gem_gtt.c  |  2 +-
  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
  drivers/gpu/drm/i915/i915_gpu_error.c|  4 ++--
  6 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 714a45cf8a51..d7c2a3201161 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct 
drm_i915_gem_object *obj)
u64 size = 0;
struct i915_vma *vma;

-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
if (i915_is_ggtt(vma->vm) &&
drm_mm_node_allocated(>node))
size += vma->node.size;
@@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
if (obj->base.name)
seq_printf(m, " (name: %d)", obj->base.name);
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
if (vma->pin_count > 0)
pin_count++;
}
@@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object 
*obj)
seq_printf(m, " (display)");
if (obj->fence_reg != I915_FENCE_REG_NONE)
seq_printf(m, " (fence: %d)", obj->fence_reg);
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
   i915_is_ggtt(vma->vm) ? "g" : "pp",
   vma->node.start, vma->node.size);
@@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
stats->shared += obj->base.size;

if (USES_FULL_PPGTT(obj->base.dev)) {
-   list_for_each_entry(vma, >vma_list, vma_link) {
+   i915_gem_obj_for_each_vma(vma, obj) {
struct i915_hw_ppgtt *ppgtt;

if (!drm_mm_node_allocated(>node))
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b77a5d84eac2..0406a020dfcc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2852,6 +2852,12 @@ struct drm_i915_gem_object 
*i915_gem_object_create_from_data(
  void i915_gem_free_object(struct drm_gem_object *obj);
  void i915_gem_vma_destroy(struct i915_vma *vma);

+#define i915_gem_obj_for_each_vma(vma, obj) \
+   for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
+vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
+>vma_link != (&(obj)->vma_list); \
+vma = list_next_entry(vma, vma_link))
+



Unfortunately error capture is not happy with this approach. Can't even 
see that error capture attempts to grab the mutex anywhere.


So what? Drop the idea or add a "doing error capture" flag somewhere?

Regards,

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

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator

8 matches

Site Navigation

Mail list logo

Footer information