Re: [Nouveau] [REGRESSION] nouveau: Severe screen corruption on (0xaf, nv50)

2012-10-21 Thread Henrik Rydberg 
On Thu, Oct 18, 2012 at 11:58:09AM +0200, Henrik Rydberg wrote:
 Hi Ben,
 
 3.7-rc1 messed up the screen on my MacBookAir3,1 (nv50, 0xaf) pretty
 badly. Not surprisingly,
 
 commit 3863c9bc887e9638a9d905d55f6038641ece78d6
 Author: Ben Skeggs bske...@redhat.com
 Date:   Sat Jul 14 19:09:17 2012 +1000
 
 drm/nouveau/instmem: completely new implementation, as a subdev module
 
 is the first bad commit. Standing on that commit, booting and then
 starting X yields the output below. Hints are especially appreciated,
 considering the patch is almost 8000 lines.

Going through one suspend/resume cycle makes the corruption go away,
and there are no more errors in dmesg. Oddly enough, I have seen
something very similar when using i915 on the MBP10. Builtin modules
and systemd in both cases. Maybe this is a general drm issue. Any
thoughts?

Thanks,
Henrik
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] Linux 3.7-rc1 (nouveau_bios_score oops).

2012-10-21 Thread Lekensteyn 
On Sunday 21 October 2012 11:03:10 Heinz Diehl wrote:
 The machine is the same, the BIOS is updated since then. The output of
 the actual acpidump is here:
 
 http://www.fritha.org/acpidump.gz

The _ROM method is still the same. It looks like a general issue with Asus
laptops. Pawel also has the Asus laptop https://lkml.org/lkml/2012/10/21/11
and has the same _ROM method. Some other acpidumps I have checked 
before also expose this behavior.

Ben, I am wondering what machine provided a VBIOS  64KiB?

Regards,
Peter
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH] drm/nouveau/bios: use size provided by _ROM method

2012-10-21 Thread Lekensteyn 
From: Peter Wu lekenst...@gmail.com

Since commit drm/nouveau/bios: attempt to fetch entire acpi rom image in one
shot, the ACPI spec is broken in order to gain speed. In theory, since the
_ROM method is supposed to return 4 KiB only, the returned buffer size could be
less than the requested length. This could lead to reading past the buffer
boundaries which could make worse thing happen. To fix that, do not read more
than the buffer contains. As an extra side-effect, the function returns the
bytes that have really been read which is more natural.

Signed-off-by: Peter Wu lekenst...@gmail.com
---
 drivers/gpu/drm/nouveau/nouveau_acpi.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c 
b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 48783e1..368e45c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -356,6 +356,7 @@ static int nouveau_rom_call(acpi_handle rom_handle, uint8_t 
*bios,
return -ENODEV;
}
obj = (union acpi_object *)buffer.pointer;
+   len = min(len, (int)obj-buffer.size);
memcpy(bios+offset, obj-buffer.pointer, len);
kfree(buffer.pointer);
return len;
-- 
1.7.9.5


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] drm/nouveau/bios: use size provided by _ROM method

2012-10-21 Thread Marcin Slusarz 
On Sun, Oct 21, 2012 at 12:53:15PM +0200, Lekensteyn wrote:
 From: Peter Wu lekenst...@gmail.com
 
 Since commit drm/nouveau/bios: attempt to fetch entire acpi rom image in one
 shot, the ACPI spec is broken in order to gain speed. In theory, since the
 _ROM method is supposed to return 4 KiB only, the returned buffer size could 
 be
 less than the requested length. This could lead to reading past the buffer
 boundaries which could make worse thing happen. To fix that, do not read more
 than the buffer contains. As an extra side-effect, the function returns the
 bytes that have really been read which is more natural.
 
 Signed-off-by: Peter Wu lekenst...@gmail.com
 ---
  drivers/gpu/drm/nouveau/nouveau_acpi.c |1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c 
 b/drivers/gpu/drm/nouveau/nouveau_acpi.c
 index 48783e1..368e45c 100644
 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
 +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
 @@ -356,6 +356,7 @@ static int nouveau_rom_call(acpi_handle rom_handle, 
 uint8_t *bios,
   return -ENODEV;
   }
   obj = (union acpi_object *)buffer.pointer;
 + len = min(len, (int)obj-buffer.size);
   memcpy(bios+offset, obj-buffer.pointer, len);
   kfree(buffer.pointer);
   return len;
 -- 

BTW, _ROM method from https://lkml.org/lkml/2012/10/21/11 multiplies length
by 8, so maybe we can read 32kB in one batch?

Marcin
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] drm/nouveau/bios: use size provided by _ROM method

2012-10-21 Thread Lekensteyn 
On Sunday 21 October 2012 14:20:05 Marcin Slusarz wrote:
 BTW, _ROM method from https://lkml.org/lkml/2012/10/21/11 multiplies length
 by 8, so maybe we can read 32kB in one batch?
 
 Marcin

That holds for that specific model, but it is not standard (in ACPI). Ben 
Skeggs has a laptop (Lenovo?) W530 that allows you to read the whole VBIOS in 
one call (that means chunk size  4KiB), but there is another machine where 
this does not work (https://bugs.freedesktop.org/show_bug.cgi?id=55948). That 
machine allows you to read lengths of 4 KiB only (the buffer size is hard-coded 
to 4096, the upper bound of the returned size).

I'd say, stick to 4 KiB for the first read, then try to read the remaining 
VBIOS in one shot and finally fallback to reading in chunks of min(4096, bios-
size - i). That should give a count of least 512 bytes. On my machine the 
length is 0x7d which translates to exactly 64 KB (not KiB).

After your patch in https://lkml.org/lkml/2012/10/20/150, drm/nouveau/bios: 
attempt to fetch entire acpi rom image in one shot could be improved to only 
read the unknown VBIOS:
 /* disobey the acpi spec - much faster on at least w530 ... */
- ret = nouveau_acpi_get_bios_chunk(bios-data, 0, bios-size);
+ ret = nouveau_acpi_get_bios_chunk(bios-data, 4096, bios-size - 4096);

Peter
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] drm/nouveau: fix nouveau_mm/nouveau_mm_node leak

2012-10-21 Thread Marcin Slusarz 
On Fri, Oct 19, 2012 at 04:05:14PM +1000, Ben Skeggs wrote:
 On Thu, Oct 11, 2012 at 11:53:09PM +0200, Marcin Slusarz wrote:
  Signed-off-by: Marcin Slusarz marcin.slus...@gmail.com
  ---
   drivers/gpu/drm/nouveau/core/core/gpuobj.c | 6 +-
   drivers/gpu/drm/nouveau/core/include/core/gpuobj.h | 3 +++
   2 files changed, 8 insertions(+), 1 deletion(-)
  
  diff --git a/drivers/gpu/drm/nouveau/core/core/gpuobj.c 
  b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
  index c2a7608..48121d2 100644
  --- a/drivers/gpu/drm/nouveau/core/core/gpuobj.c
  +++ b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
  @@ -39,8 +39,11 @@ nouveau_gpuobj_destroy(struct nouveau_gpuobj *gpuobj)
  nv_wo32(gpuobj, i, 0x);
  }
   
  +   if (gpuobj-node)
  +   nouveau_mm_free(gpuobj-node_heap, gpuobj-node);
  +
 if (gpuobj-node) {
   nouveau_mm_free(nv_gpuobj(gpuobj-parent)-heap,
   gpuobj-node);
 }
 
 Or something to that effect, instead of having to store the heap
 pointer again.

Oh, right.
(Somehow I assumed pargpu to be different object than parent when
I first read it.)

  if (gpuobj-heap.block_size)
  -   nouveau_mm_fini(gpuobj-heap);
  +   WARN_ON(nouveau_mm_fini(gpuobj-heap));
 Alright, I get this.  However, perhaps we should go the full hog here
 and make nouveau_mm_fini() directly do the WARN_ON() in this situation?
 
 There was, once upon a time, reasons for it not doing this, I don't
 believe they're valid anymore though.

OK.

Marcin
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH v2] drm/nouveau: fix nouveau_mm/nouveau_mm_node leak

2012-10-21 Thread Marcin Slusarz 
v2: use already existing parent

Signed-off-by: Marcin Slusarz marcin.slus...@gmail.com
---
 drivers/gpu/drm/nouveau/core/core/gpuobj.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/core/core/gpuobj.c 
b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
index c2a7608..18b3f5c 100644
--- a/drivers/gpu/drm/nouveau/core/core/gpuobj.c
+++ b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
@@ -39,6 +39,11 @@ nouveau_gpuobj_destroy(struct nouveau_gpuobj *gpuobj)
nv_wo32(gpuobj, i, 0x);
}
 
+   if (gpuobj-node) {
+   nouveau_mm_free(nv_gpuobj(gpuobj-parent)-heap,
+   gpuobj-node);
+   }
+
if (gpuobj-heap.block_size)
nouveau_mm_fini(gpuobj-heap);
 
-- 
1.7.12

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH] drm/nouveau: warn when trying to free mm which is still in use

2012-10-21 Thread Marcin Slusarz 
Signed-off-by: Marcin Slusarz marcin.slus...@gmail.com
---
 drivers/gpu/drm/nouveau/core/core/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/mm.c 
b/drivers/gpu/drm/nouveau/core/core/mm.c
index bfddf87..4d62064 100644
--- a/drivers/gpu/drm/nouveau/core/core/mm.c
+++ b/drivers/gpu/drm/nouveau/core/core/mm.c
@@ -236,7 +236,7 @@ nouveau_mm_fini(struct nouveau_mm *mm)
int nodes = 0;
 
list_for_each_entry(node, mm-nodes, nl_entry) {
-   if (nodes++ == mm-heap_nodes)
+   if (WARN_ON(nodes++ == mm-heap_nodes))
return -EBUSY;
}
 
-- 
1.7.12

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau