confused about a BUG_ON in drm_mm_dump_table

confused about a BUG_ON in drm_mm_dump_table

[Christopher Harvey]

I've been trying to wrap my head around ttm and gem these last couple of
weeks. I found the nice 'drm_mm_dump_table' function and ran it on the
mgag200 drm_mm struct. Instant BUG in include/drm/drm_mm.h line 100.

static inline unsigned long drm_mm_hole_node_start(struct drm_mm_node *hole_node)
{
	BUG_ON(!hole_node->hole_follows);
	return __drm_mm_hole_node_start(hole_node);
}

where drm_mm_hole_node_start is called from drm_mm_dump_table like so:

int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
{
	struct drm_mm_node *entry;
	unsigned long total_used = 0, total_free = 0, total = 0;
	unsigned long hole_start, hole_end, hole_size;

	hole_start = drm_mm_hole_node_start(&mm->head_node);
...
...
...

as far as I can tell the head_node is a list of page offsets that point
to free memory chunks. It seems drm_mm_dump_table expects a free chunk
at the beginning of the list. What if all the memory is used? How can we
ALWAYS expect a free chunk at the first element? Is this a bug in
drm_mm_dump_table? Can't we just remove the first print and let the loop
do all the work that comes right after? They look the same to me.

This all started from me trying to figure out where ttm/gem is putting
buffer objects in VRAM. All the variables in a ttm_buffer_object that
looks like they hold variables either have address outside of the total
VRAM size, or 0.

When I removed the first print in drm_mm_dump_table I get the following
output:
0x00100000-0x001007e9: 0x000007e9: used
0x001007e9-0x10100000: 0x0ffff817: free
total: 268435456, used 2025 free 268433431

The board only has 16M of VRAM.

thanks,
Chris

[PATCH] drm/mm: fix dump table BUG

[Daniel Vetter]

In

commit 9e8944ab564f2e3dde90a518cd32048c58918608
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:17 2012 +0000

    drm: Introduce an iterator over holes in the drm_mm range manager

helpers and iterators for hole handling have been introduced with some
debug BUG_ONs sprinkled over. Unfortunately this broke the mm dumper
which unconditionally tried to compute the size of the very first
hole.

Reported-by: Christopher Harvey <charvey@matrox.com>
Cc: Christopher Harvey <charvey@matrox.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_mm.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index db1e2d6..a0ba3a1 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -759,14 +759,15 @@ int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
 {
 	struct drm_mm_node *entry;
 	unsigned long total_used = 0, total_free = 0, total = 0;
-	unsigned long hole_start, hole_end, hole_size;
+	unsigned long hole_start, hole_end, hole_size = 0;
 
-	hole_start = drm_mm_hole_node_start(&mm->head_node);
-	hole_end = drm_mm_hole_node_end(&mm->head_node);
-	hole_size = hole_end - hole_start;
-	if (hole_size)
+	if (mm->head_node.hole_follows) {
+		hole_start = drm_mm_hole_node_start(&mm->head_node);
+		hole_end = drm_mm_hole_node_end(&mm->head_node);
+		hole_size = hole_end - hole_start;
 		seq_printf(m, "0x%08lx-0x%08lx: 0x%08lx: free\n",
 				hole_start, hole_end, hole_size);
+	}
 	total_free += hole_size;
 
 	drm_mm_for_each_node(entry, mm) {
-- 
1.7.10.4

Re: [PATCH] drm/mm: fix dump table BUG

[Chris Wilson]

On Sat, Apr 20, 2013 at 12:08:11PM +0200, Daniel Vetter wrote:
> In
> 
> commit 9e8944ab564f2e3dde90a518cd32048c58918608
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:17 2012 +0000
> 
>     drm: Introduce an iterator over holes in the drm_mm range manager
> 
> helpers and iterators for hole handling have been introduced with some
> debug BUG_ONs sprinkled over. Unfortunately this broke the mm dumper
> which unconditionally tried to compute the size of the very first
> hole.
> 
> Reported-by: Christopher Harvey <charvey@matrox.com>
> Cc: Christopher Harvey <charvey@matrox.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_mm.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index db1e2d6..a0ba3a1 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -759,14 +759,15 @@ int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
>  {
>  	struct drm_mm_node *entry;
>  	unsigned long total_used = 0, total_free = 0, total = 0;
> -	unsigned long hole_start, hole_end, hole_size;
> +	unsigned long hole_start, hole_end, hole_size = 0;
>  
> -	hole_start = drm_mm_hole_node_start(&mm->head_node);
> -	hole_end = drm_mm_hole_node_end(&mm->head_node);

Use __drm_mm_hole_node_start and __drm_mm_hole_node_end instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/mm: fix dump table BUG

[Daniel Vetter]

On Sat, Apr 20, 2013 at 12:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Apr 20, 2013 at 12:08:11PM +0200, Daniel Vetter wrote:
>> In
>>
>> commit 9e8944ab564f2e3dde90a518cd32048c58918608
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Thu Nov 15 11:32:17 2012 +0000
>>
>>     drm: Introduce an iterator over holes in the drm_mm range manager
>>
>> helpers and iterators for hole handling have been introduced with some
>> debug BUG_ONs sprinkled over. Unfortunately this broke the mm dumper
>> which unconditionally tried to compute the size of the very first
>> hole.
>>
>> Reported-by: Christopher Harvey <charvey@matrox.com>
>> Cc: Christopher Harvey <charvey@matrox.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/drm_mm.c |   11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
>> index db1e2d6..a0ba3a1 100644
>> --- a/drivers/gpu/drm/drm_mm.c
>> +++ b/drivers/gpu/drm/drm_mm.c
>> @@ -759,14 +759,15 @@ int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
>>  {
>>       struct drm_mm_node *entry;
>>       unsigned long total_used = 0, total_free = 0, total = 0;
>> -     unsigned long hole_start, hole_end, hole_size;
>> +     unsigned long hole_start, hole_end, hole_size = 0;
>>
>> -     hole_start = drm_mm_hole_node_start(&mm->head_node);
>> -     hole_end = drm_mm_hole_node_end(&mm->head_node);
>
> Use __drm_mm_hole_node_start and __drm_mm_hole_node_end instead.

I've figured I could make the condition more symmetric with the one in
the loop, so that both check ->hole_follows.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/mm: fix dump table BUG

[Chris Wilson]

On Sat, Apr 20, 2013 at 01:28:53PM +0200, Daniel Vetter wrote:
> On Sat, Apr 20, 2013 at 12:11 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Sat, Apr 20, 2013 at 12:08:11PM +0200, Daniel Vetter wrote:
> >> In
> >>
> >> commit 9e8944ab564f2e3dde90a518cd32048c58918608
> >> Author: Chris Wilson <chris@chris-wilson.co.uk>
> >> Date:   Thu Nov 15 11:32:17 2012 +0000
> >>
> >>     drm: Introduce an iterator over holes in the drm_mm range manager
> >>
> >> helpers and iterators for hole handling have been introduced with some
> >> debug BUG_ONs sprinkled over. Unfortunately this broke the mm dumper
> >> which unconditionally tried to compute the size of the very first
> >> hole.
> >>
[snip]
> > Use __drm_mm_hole_node_start and __drm_mm_hole_node_end instead.
> 
> I've figured I could make the condition more symmetric with the one in
> the loop, so that both check ->hole_follows.

Your change log didn't explain your intentions to refactor the code, only
to avoid the extra BUG_ON...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre