[PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Marcin Slusarz]

We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
because nouveau_fifo_swmthd looks for matching gpuobj in
ramht_refs list.
It fixes kernel panic in nouveau_gpuobj_ref_find.

Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
index b57cc4a..6e57426 100644
--- a/drivers/gpu/drm/nouveau/nv50_instmem.c
+++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
@@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
 	chan->file_priv = (struct drm_file *)-2;
 	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
 
+	INIT_LIST_HEAD(&chan->ramht_refs);
+
 	/* Channel's PRAMIN object + heap */
 	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
 							NULL, &chan->ramin);
-- 
1.7.1.1

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Marcin Slusarz]

On Sun, Aug 22, 2010 at 08:54:08PM +0200, Marcin Slusarz wrote:
> We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
> because nouveau_fifo_swmthd looks for matching gpuobj in
> ramht_refs list.
> It fixes kernel panic in nouveau_gpuobj_ref_find.
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> index b57cc4a..6e57426 100644
> --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> @@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
>  	chan->file_priv = (struct drm_file *)-2;
>  	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
>  
> +	INIT_LIST_HEAD(&chan->ramht_refs);
> +
>  	/* Channel's PRAMIN object + heap */
>  	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
>  							NULL, &chan->ramin);
> -- 

ping

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Ben Skeggs]

On Tue, 2010-08-31 at 23:22 +0200, Marcin Slusarz wrote:
> On Sun, Aug 22, 2010 at 08:54:08PM +0200, Marcin Slusarz wrote:
> > We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
> > because nouveau_fifo_swmthd looks for matching gpuobj in
> > ramht_refs list.
> > It fixes kernel panic in nouveau_gpuobj_ref_find.
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > index b57cc4a..6e57426 100644
> > --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> > +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > @@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
> >  	chan->file_priv = (struct drm_file *)-2;
> >  	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
> >  
> > +	INIT_LIST_HEAD(&chan->ramht_refs);
> > +
> >  	/* Channel's PRAMIN object + heap */
> >  	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
> >  							NULL, &chan->ramin);
> > -- 
> 
> ping
Pong!  Thank you for the patch.

I meant to reply to this earlier but it slipped my mind.  I have a
number of pending cleanups to all this stuff that should make this patch
unnecessary.

Ben.
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Maxim Levitsky]

Hi,

I just tried that patch, but unfortunately nether with nor without it
kdb seems not to work.
It could be id10t error from my side, but I did test the kdb in the past
with few KMS patches, and it seemed to work.

Now I can't even get its prompt on the console.

This is what I do:

echo kbd | sudo tee /sys/module/kgdboc/parameters/kgdboc
(also tried booting with kgdboc=kbd)

Both seems to start kdb, because

maxim@maxim-laptop:~$ cat /sys/module/kgdboc/parameters/kgdboc 
kbd

maxim@maxim-laptop:~$ dmesg | grep kgdb
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-2.6.36-rc3+ root=UUID=52341b68-74f3-4c96-aaf8-7586a06c4b4e ro splash video=nouveau nmi_watchdog=lapic kgdboc=kbd
[    0.000000] Kernel command line: BOOT_IMAGE=/boot/vmlinuz-2.6.36-rc3+ root=UUID=52341b68-74f3-4c96-aaf8-7586a06c4b4e ro splash video=nouveau nmi_watchdog=lapic kgdboc=kbd
[    2.671914] kgdb: Registered I/O driver kgdboc.


Now when I first switch to console and then press Alt+SysRQ+G, I just get SYSRQ: DEBUG, and system hangs.
It could be printk level, but that should be ok, no?

maxim@maxim-laptop:~$ cat /proc/sys/kernel/printk
7	4	1	7


Also if I remember correct inputting 'g' then ENTER should resume the system, but it doesn't.

So I think kdb doesn't work at all here

Best regards,
	Maxim Levitsky

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Maxim Levitsky]

Sorry, wrong thread!

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Marcin Slusarz]

On Wed, Sep 01, 2010 at 07:52:58AM +1000, Ben Skeggs wrote:
> On Tue, 2010-08-31 at 23:22 +0200, Marcin Slusarz wrote:
> > On Sun, Aug 22, 2010 at 08:54:08PM +0200, Marcin Slusarz wrote:
> > > We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
> > > because nouveau_fifo_swmthd looks for matching gpuobj in
> > > ramht_refs list.
> > > It fixes kernel panic in nouveau_gpuobj_ref_find.
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > index b57cc4a..6e57426 100644
> > > --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > @@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
> > >  	chan->file_priv = (struct drm_file *)-2;
> > >  	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
> > >  
> > > +	INIT_LIST_HEAD(&chan->ramht_refs);
> > > +
> > >  	/* Channel's PRAMIN object + heap */
> > >  	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
> > >  							NULL, &chan->ramin);
> > > -- 
> > 
> > ping
> Pong!  Thank you for the patch.
> 
> I meant to reply to this earlier but it slipped my mind.  I have a
> number of pending cleanups to all this stuff that should make this patch
> unnecessary.

Oh, come on. It's a simple one-line bugfix for easily triggerable kernel panic.
Patch like this should not wait a week for any response - it should be applied
ASAP and go to Linus possibly with -stable tag.

Marcin

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Ben Skeggs]

On Thu, 2010-09-02 at 12:45 +0200, Marcin Slusarz wrote:
> On Wed, Sep 01, 2010 at 07:52:58AM +1000, Ben Skeggs wrote:
> > On Tue, 2010-08-31 at 23:22 +0200, Marcin Slusarz wrote:
> > > On Sun, Aug 22, 2010 at 08:54:08PM +0200, Marcin Slusarz wrote:
> > > > We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
> > > > because nouveau_fifo_swmthd looks for matching gpuobj in
> > > > ramht_refs list.
> > > > It fixes kernel panic in nouveau_gpuobj_ref_find.
> > > > 
> > > > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > index b57cc4a..6e57426 100644
> > > > --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > @@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
> > > >  	chan->file_priv = (struct drm_file *)-2;
> > > >  	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
> > > >  
> > > > +	INIT_LIST_HEAD(&chan->ramht_refs);
> > > > +
> > > >  	/* Channel's PRAMIN object + heap */
> > > >  	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
> > > >  							NULL, &chan->ramin);
> > > > -- 
> > > 
> > > ping
> > Pong!  Thank you for the patch.
> > 
> > I meant to reply to this earlier but it slipped my mind.  I have a
> > number of pending cleanups to all this stuff that should make this patch
> > unnecessary.
> 
> Oh, come on. It's a simple one-line bugfix for easily triggerable kernel panic.
> Patch like this should not wait a week for any response - it should be applied
> ASAP and go to Linus possibly with -stable tag.
Oh please, it's hardly easily triggerable.  I've *never* seen it
personally, and triggering errors on those channels isn't something that
generally happens.  If you feel that strongly about it I'll send it to
Dave to go to Linus.

Ben.
> 
> Marcin
> 

Re: [PATCH] drm/nv50: initialize ramht_refs list for faked 0 channel

[Marcin Slusarz]

On Thu, Sep 02, 2010 at 10:31:55PM +1000, Ben Skeggs wrote:
> On Thu, 2010-09-02 at 12:45 +0200, Marcin Slusarz wrote:
> > On Wed, Sep 01, 2010 at 07:52:58AM +1000, Ben Skeggs wrote:
> > > On Tue, 2010-08-31 at 23:22 +0200, Marcin Slusarz wrote:
> > > > On Sun, Aug 22, 2010 at 08:54:08PM +0200, Marcin Slusarz wrote:
> > > > > We need it for PFIFO_INTR_CACHE_ERROR interrupt handling,
> > > > > because nouveau_fifo_swmthd looks for matching gpuobj in
> > > > > ramht_refs list.
> > > > > It fixes kernel panic in nouveau_gpuobj_ref_find.
> > > > > 
> > > > > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > ---
> > > > >  drivers/gpu/drm/nouveau/nv50_instmem.c |    2 ++
> > > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/nouveau/nv50_instmem.c b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > > index b57cc4a..6e57426 100644
> > > > > --- a/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > > +++ b/drivers/gpu/drm/nouveau/nv50_instmem.c
> > > > > @@ -139,6 +139,8 @@ nv50_instmem_init(struct drm_device *dev)
> > > > >  	chan->file_priv = (struct drm_file *)-2;
> > > > >  	dev_priv->fifos[0] = dev_priv->fifos[127] = chan;
> > > > >  
> > > > > +	INIT_LIST_HEAD(&chan->ramht_refs);
> > > > > +
> > > > >  	/* Channel's PRAMIN object + heap */
> > > > >  	ret = nouveau_gpuobj_new_fake(dev, 0, c_offset, c_size, 0,
> > > > >  							NULL, &chan->ramin);
> > > > > -- 
> > > > 
> > > > ping
> > > Pong!  Thank you for the patch.
> > > 
> > > I meant to reply to this earlier but it slipped my mind.  I have a
> > > number of pending cleanups to all this stuff that should make this patch
> > > unnecessary.
> > 
> > Oh, come on. It's a simple one-line bugfix for easily triggerable kernel panic.
> > Patch like this should not wait a week for any response - it should be applied
> > ASAP and go to Linus possibly with -stable tag.
> Oh please, it's hardly easily triggerable.  I've *never* seen it
> personally, and triggering errors on those channels isn't something that
> generally happens. 

Are you sure you have *never* seen it? Do you have netconsole enabled all the time?
I'm asking because once you hit this bug, machine is dead and you won't find anything
in the logs.

I'm attaching the testcase. Yes, it's that simple.

--
#include <stdio.h>

#include <libdrm/nouveau_drmif.h>
#include <nouveau/nouveau_channel.h>

int main()
{
	struct nouveau_device *dev = NULL;
	struct nouveau_channel *chan = NULL;

	if (nouveau_device_open(&dev, "")) {
		perror("nouveau_device_open");
		return 1;
	}

	if (nouveau_channel_alloc(dev, 0, 0, &chan)) {
		perror("nouveau_channel_alloc");
		return 2;
	}

	return 0;
}