[PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

It can be called from atomic context, e.g. when switching to console for panic
output.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43941

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/radeon/atom.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index 14cc88a..d99dbb6 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -666,7 +666,7 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
 	if (arg == ATOM_UNIT_MICROSEC)
 		udelay(count);
 	else
-		msleep(count);
+		mdelay(count);
 }
 
 static void atom_op_div(atom_exec_context *ctx, int *ptr, int arg)
-- 
1.7.7.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Dave Airlie]

2012/1/3 Michel Dänzer <michel@daenzer.net>:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> It can be called from atomic context, e.g. when switching to console for panic
> output.
>
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43941

I wonder how ugly it would be to check for atomic context or not,
mdelay is quite a long busy delay to take a CPU, esp wrt to async
driver loading stuff.

Dave.

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Alan Cox]

On Tue,  3 Jan 2012 19:04:00 +0100
Michel Dänzer <michel@daenzer.net> wrote:

> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> It can be called from atomic context, e.g. when switching to console for panic
> output.

Is this only special cases like a panic - if so can it not be called in a
way that distinguishes between normality and nasty cases. mS plus spin
loops are horrendous for anyone trying to do real time, or even for
keeping the clock straight.

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Michel Dänzer]

On Die, 2012-01-03 at 18:09 +0000, Dave Airlie wrote: 
> 2012/1/3 Michel Dänzer <michel@daenzer.net>:
> > From: Michel Dänzer <michel.daenzer@amd.com>
> >
> > It can be called from atomic context, e.g. when switching to console for panic
> > output.
> >
> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> 
> I wonder how ugly it would be to check for atomic context or not,

So do I. :) The comment in include/linux/hardirq.h that ends in 'Do not
use in_atomic() in driver code.' sounds rathery scary...


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Michel Dänzer]

On Die, 2012-01-03 at 18:09 +0000, Alan Cox wrote: 
> On Tue,  3 Jan 2012 19:04:00 +0100
> Michel Dänzer <michel@daenzer.net> wrote:
> 
> > From: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > It can be called from atomic context, e.g. when switching to console for panic
> > output.
> 
> Is this only special cases like a panic - if so can it not be called in a
> way that distinguishes between normality and nasty cases.

No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
theory it could be indirectly called from anywhere that uses ATOM BIOS.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Daniel Vetter]

On Tue, Jan 03, 2012 at 07:16:25PM +0100, Michel Dänzer wrote:
> On Die, 2012-01-03 at 18:09 +0000, Dave Airlie wrote: 
> > 2012/1/3 Michel Dänzer <michel@daenzer.net>:
> > > From: Michel Dänzer <michel.daenzer@amd.com>
> > >
> > > It can be called from atomic context, e.g. when switching to console for panic
> > > output.
> > >
> > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> > 
> > I wonder how ugly it would be to check for atomic context or not,
> 
> So do I. :) The comment in include/linux/hardirq.h that ends in 'Do not
> use in_atomic() in driver code.' sounds rathery scary...

We already use in_atomic checks in similar delay code in drm/i915 for the
same reasons. I think the ugly mess that results from the panic notifier
calling into kms code is justification enough to neglect the the comment
about not using in_atomic in drivers.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Alan Cox]

On Tue, 03 Jan 2012 19:25:46 +0100
Michel Dänzer <michel@daenzer.net> wrote:

> On Die, 2012-01-03 at 18:09 +0000, Alan Cox wrote: 
> > On Tue,  3 Jan 2012 19:04:00 +0100
> > Michel Dänzer <michel@daenzer.net> wrote:
> > 
> > > From: Michel Dänzer <michel.daenzer@amd.com>
> > > 
> > > It can be called from atomic context, e.g. when switching to console for panic
> > > output.
> > 
> > Is this only special cases like a panic - if so can it not be called in a
> > way that distinguishes between normality and nasty cases.
> 
> No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
> theory it could be indirectly called from anywhere that uses ATOM BIOS.

So lets stick to practice, and the real world. Screwing up everything
else because of a crappy problem in your Atom BIOS code sucks but hey it
happens. screwing up everything because of a theoretical concern is just
dumb.

I would start by making it some kind of context flag for your interpreter
and making people involve the interpreter with a different function call
if they can't sleep. At that point you'll be able to define the problem
in the real kernel, document the rule and spot further people trying to
jump off cliffs before they do.

Alan

[PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

It can be the case e.g. when switching to console for panic output.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43941

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

v2: Still call msleep() in the normal case. Only compile tested.

 drivers/gpu/drm/radeon/atom.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index 14cc88a..4092e59 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -665,6 +665,8 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
 	SDEBUG("   count: %d\n", count);
 	if (arg == ATOM_UNIT_MICROSEC)
 		udelay(count);
+	else if (in_interrupt() || irqs_disabled() || in_atomic())
+		mdelay(count);
 	else
 		msleep(count);
 }
-- 
1.7.7.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Michel Dänzer]

On Die, 2012-01-03 at 21:04 +0100, Daniel Vetter wrote: 
> On Tue, Jan 03, 2012 at 07:16:25PM +0100, Michel Dänzer wrote:
> > On Die, 2012-01-03 at 18:09 +0000, Dave Airlie wrote: 
> > > 2012/1/3 Michel Dänzer <michel@daenzer.net>:
> > > > From: Michel Dänzer <michel.daenzer@amd.com>
> > > >
> > > > It can be called from atomic context, e.g. when switching to console for panic
> > > > output.
> > > >
> > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> > > 
> > > I wonder how ugly it would be to check for atomic context or not,
> > 
> > So do I. :) The comment in include/linux/hardirq.h that ends in 'Do not
> > use in_atomic() in driver code.' sounds rathery scary...
> 
> We already use in_atomic checks in similar delay code in drm/i915 for the
> same reasons. I think the ugly mess that results from the panic notifier
> calling into kms code is justification enough to neglect the the comment
> about not using in_atomic in drivers.

Okay, v2 sent.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Michel Dänzer]

On Mit, 2012-01-04 at 00:52 +0000, Alan Cox wrote: 
> On Tue, 03 Jan 2012 19:25:46 +0100
> Michel Dänzer <michel@daenzer.net> wrote:
> > On Die, 2012-01-03 at 18:09 +0000, Alan Cox wrote: 
> > > On Tue,  3 Jan 2012 19:04:00 +0100
> > > Michel Dänzer <michel@daenzer.net> wrote:
> > > 
> > > > From: Michel Dänzer <michel.daenzer@amd.com>
> > > > 
> > > > It can be called from atomic context, e.g. when switching to console for panic
> > > > output.
> > > 
> > > Is this only special cases like a panic - if so can it not be called in a
> > > way that distinguishes between normality and nasty cases.
> > 
> > No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
> > theory it could be indirectly called from anywhere that uses ATOM BIOS.
> 
> So lets stick to practice, and the real world. Screwing up everything
> else because of a crappy problem in your Atom BIOS code sucks but hey it
> happens. screwing up everything because of a theoretical concern is just
> dumb.

Thanks for the flowers, but it's not just a theoretical concern, see the
bug report.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Daniel Vetter]

On Wed, Jan 04, 2012 at 11:33:40AM +0100, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> It can be the case e.g. when switching to console for panic output.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> v2: Still call msleep() in the normal case. Only compile tested.
> 
>  drivers/gpu/drm/radeon/atom.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
> index 14cc88a..4092e59 100644
> --- a/drivers/gpu/drm/radeon/atom.c
> +++ b/drivers/gpu/drm/radeon/atom.c
> @@ -665,6 +665,8 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
>  	SDEBUG("   count: %d\n", count);
>  	if (arg == ATOM_UNIT_MICROSEC)
>  		udelay(count);
> +	else if (in_interrupt() || irqs_disabled() || in_atomic())
> +		mdelay(count);

Afaics in_atomic subsumes in_interrupt. irqs_disabled looks like a nice
addition to cover up the !CONFIG_PREEMPT case. i915 (in intel_drv.h) also
checks for in_dbg_master() to take care of kdbg.

Can I bother you to create a small helper like in_atomic_kms_context that
checks these three things (and also use it in drm/i915)?

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Alan Cox]

> > So lets stick to practice, and the real world. Screwing up everything
> > else because of a crappy problem in your Atom BIOS code sucks but hey it
> > happens. screwing up everything because of a theoretical concern is just
> > dumb.
> 
> Thanks for the flowers, but it's not just a theoretical concern, see the
> bug report.

The bug report is a single specific case. Figure out the calling
paths afflicted by it, don't be lazy and just dump on everyone on every
path because of a special case you apparently can't be bothered to root
cause.

Alan

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Michel Dänzer]

On Mit, 2012-01-04 at 11:54 +0100, Daniel Vetter wrote: 
> On Wed, Jan 04, 2012 at 11:33:40AM +0100, Michel Dänzer wrote:
> > From: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > It can be the case e.g. when switching to console for panic output.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> > 
> > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > ---
> > 
> > v2: Still call msleep() in the normal case. Only compile tested.
> > 
> >  drivers/gpu/drm/radeon/atom.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
> > index 14cc88a..4092e59 100644
> > --- a/drivers/gpu/drm/radeon/atom.c
> > +++ b/drivers/gpu/drm/radeon/atom.c
> > @@ -665,6 +665,8 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
> >  	SDEBUG("   count: %d\n", count);
> >  	if (arg == ATOM_UNIT_MICROSEC)
> >  		udelay(count);
> > +	else if (in_interrupt() || irqs_disabled() || in_atomic())
> > +		mdelay(count);
> 
> Afaics in_atomic subsumes in_interrupt. irqs_disabled looks like a nice
> addition to cover up the !CONFIG_PREEMPT case. i915 (in intel_drv.h) also
> checks for in_dbg_master() to take care of kdbg.
> 
> Can I bother you to create a small helper like in_atomic_kms_context that
> checks these three things (and also use it in drm/i915)?

Sorry, I've already spent way more time on this than I meant to, and
Alan just killed what little motivation I still had to spend more.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Daniel Vetter]

On Wed, Jan 04, 2012 at 12:29:24PM +0100, Michel Dänzer wrote:
> On Mit, 2012-01-04 at 11:54 +0100, Daniel Vetter wrote: 
> > On Wed, Jan 04, 2012 at 11:33:40AM +0100, Michel Dänzer wrote:
> > > From: Michel Dänzer <michel.daenzer@amd.com>
> > > 
> > > It can be the case e.g. when switching to console for panic output.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> > > 
> > > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > > ---
> > > 
> > > v2: Still call msleep() in the normal case. Only compile tested.
> > > 
> > >  drivers/gpu/drm/radeon/atom.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
> > > index 14cc88a..4092e59 100644
> > > --- a/drivers/gpu/drm/radeon/atom.c
> > > +++ b/drivers/gpu/drm/radeon/atom.c
> > > @@ -665,6 +665,8 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
> > >  	SDEBUG("   count: %d\n", count);
> > >  	if (arg == ATOM_UNIT_MICROSEC)
> > >  		udelay(count);
> > > +	else if (in_interrupt() || irqs_disabled() || in_atomic())
> > > +		mdelay(count);
> > 
> > Afaics in_atomic subsumes in_interrupt. irqs_disabled looks like a nice
> > addition to cover up the !CONFIG_PREEMPT case. i915 (in intel_drv.h) also
> > checks for in_dbg_master() to take care of kdbg.
> > 
> > Can I bother you to create a small helper like in_atomic_kms_context that
> > checks these three things (and also use it in drm/i915)?
> 
> Sorry, I've already spent way more time on this than I meant to, and
> Alan just killed what little motivation I still had to spend more.

I think Alan's simply wrong. The msleep checks in i915 are only used for
two cases:
- when using kdbg
- when displaying a panic
Afaics radeon has the exact same issue, at least I've seen my radeon
machine here go down after an oops. Splattering the entire driver for
these two corner cases which don't happen at all under normal
circumstances is imho utter nonsense.

I.e. I'd be very happy to smash a r-b onto your patch if you unifiy the
magical checks with i915 in a common function and add a small comment.
Does the prospect of an up-front r-b and me promising to harass Dave to
merge it restore your motivation?

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Michel Dänzer]

On Mit, 2012-01-04 at 12:44 +0100, Daniel Vetter wrote: 
> On Wed, Jan 04, 2012 at 12:29:24PM +0100, Michel Dänzer wrote:
> > On Mit, 2012-01-04 at 11:54 +0100, Daniel Vetter wrote: 
> > > On Wed, Jan 04, 2012 at 11:33:40AM +0100, Michel Dänzer wrote:
> > > > From: Michel Dänzer <michel.daenzer@amd.com>
> > > > 
> > > > It can be the case e.g. when switching to console for panic output.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43941
> > > > 
> > > > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > > > ---
> > > > 
> > > > v2: Still call msleep() in the normal case. Only compile tested.
> > > > 
> > > >  drivers/gpu/drm/radeon/atom.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
> > > > index 14cc88a..4092e59 100644
> > > > --- a/drivers/gpu/drm/radeon/atom.c
> > > > +++ b/drivers/gpu/drm/radeon/atom.c
> > > > @@ -665,6 +665,8 @@ static void atom_op_delay(atom_exec_context *ctx, int *ptr, int arg)
> > > >  	SDEBUG("   count: %d\n", count);
> > > >  	if (arg == ATOM_UNIT_MICROSEC)
> > > >  		udelay(count);
> > > > +	else if (in_interrupt() || irqs_disabled() || in_atomic())
> > > > +		mdelay(count);
> > > 
> > > Afaics in_atomic subsumes in_interrupt. irqs_disabled looks like a nice
> > > addition to cover up the !CONFIG_PREEMPT case. i915 (in intel_drv.h) also
> > > checks for in_dbg_master() to take care of kdbg.
> > > 
> > > Can I bother you to create a small helper like in_atomic_kms_context that
> > > checks these three things (and also use it in drm/i915)?
> > 
> > Sorry, I've already spent way more time on this than I meant to, and
> > Alan just killed what little motivation I still had to spend more.
> 
> I think Alan's simply wrong. The msleep checks in i915 are only used for
> two cases:
> - when using kdbg
> - when displaying a panic
> Afaics radeon has the exact same issue, at least I've seen my radeon
> machine here go down after an oops. Splattering the entire driver for
> these two corner cases which don't happen at all under normal
> circumstances is imho utter nonsense.
> 
> I.e. I'd be very happy to smash a r-b onto your patch if you unifiy the
> magical checks with i915 in a common function and add a small comment.
> Does the prospect of an up-front r-b and me promising to harass Dave to
> merge it restore your motivation?

It's certainly helping. :) I'll see what I can do.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Dave Airlie]

2012/1/4 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> > So lets stick to practice, and the real world. Screwing up everything
>> > else because of a crappy problem in your Atom BIOS code sucks but hey it
>> > happens. screwing up everything because of a theoretical concern is just
>> > dumb.
>>
>> Thanks for the flowers, but it's not just a theoretical concern, see the
>> bug report.
>
> The bug report is a single specific case. Figure out the calling
> paths afflicted by it, don't be lazy and just dump on everyone on every
> path because of a special case you apparently can't be bothered to root
> cause.
>

I'm not sure what we'd gain by passing the atomic context information
in from the top of the atom interpreter and using it in this one place
vs getting the atomic info in this one place.

Every modesetting operation can happen in atomic context or not in
atomic context, thanks to the wonder of oops printing and also kgdb,
trying to figure out which table in this one person's bios is the
"root" cause and special casing for it is insane since tomorrow 20
other BIOSes could contain that table or a new variant we haven't seen
before.

Unless one of us is totally misinterpreting what the other is saying
or what the atombios interpreter is.

Dave.

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Alan Cox]

> I think Alan's simply wrong.

In what way ? You stated this, then go on below to say what I did ?

> Splattering the entire driver for
> these two corner cases which don't happen at all under normal
> circumstances is imho utter nonsense.

Which is what I said

| > > Is this only special cases like a panic - if so can it not be called
in a
| > > way that distinguishes between normality and nasty cases.  
| > 
| > No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
| > theory it could be indirectly called from anywhere that uses ATOM
BIOS.  

| So lets stick to practice, and the real world. Screwing up everything
| else because of a crappy problem in your Atom BIOS code sucks but hey it
| happens. screwing up everything because of a theoretical concern is just
| dumb.


We don't want to do mdelays when they are not needed just because it is
theoretically possible they are not needed. There are several ways to fix
it, one is to stuff in_atomic() etc in the awkward spots. However if
you've got something where you have lots of call points to an interface
this is actually a bad idea, because you grow more insidiously, or a
change elsewhere in the kernel silently turns all your sleeps into delays
and things like live music work stop being doable with radeon graphics.

So the better way to fix it is to actually have an interface

	atom_execute_table_atomic()

so that such situations are called out clearly, and any change elsewhere
that messes it all up causes kernel spewage that can be dealt with
properly - either by using _atomic if its something obscure like a panic,
fixing the issue or reverting the problematic change further up the tree.

Another way to approach it would be to have

	radeon_atom_atomic(rdev);
	radeon_atom_whateverfoo(rdev, ...)
	radeon_atom_atomic_end(rdev);

which set and cleared a bitflag that the mdelay/msleep opcode tested.

That's quicker to implement but a spot less clean. Again it calls out any
such cases and makes them explicit. It also means any accidental changes
that affect this will result in spewage and debugging not silent trashing
of performance for stuff like machine control, music and some gaming.

It should only be for panic, although accel calls for printk spewage can
hit in atomic context in some situations but I don't think that becomes
an atombios path ?

Alan

Re: [PATCH] radeon: Use mdelay() instead of msleep() in atom_op_delay().

[Alan Cox]

> Every modesetting operation can happen in atomic context or not in
> atomic context, thanks to the wonder of oops printing and also kgdb,
> trying to figure out which table in this one person's bios is the
> "root" cause and special casing for it is insane since tomorrow 20
> other BIOSes could contain that table or a new variant we haven't seen
> before.
> 
> Unless one of us is totally misinterpreting what the other is saying
> or what the atombios interpreter is.

Its a sort of bytecode engine that allows the BIOS to describe to the
OS how the GPU should be handled. So what we want to avoid is
accidentally having some other path become spinlock based due to a
random unrelated change and ending up doing 20mS spinning delays without
us getting a warning.

Explicitly calling out that mode setting does this isn't a big deal - we
don't modeset often while doing stuff that is latency sensitive (gaming,
live music, telephony, etc) and if you have the in_atomic check as well
we'd probably only hit it on panic().

The problem is if we just band-aid it without doing this it will (and
always has) ended in tears later.

So as per the other mail all I'm really saying is we want something like


	radeon_atom_mode_atomic_begin(rdev)
	{
		rdev->mode_info.atom_context->atomic = 1;
	}

	radeon_atom_mode_atomic_end(rdev)
	{
		rdev->mode_info.atom_context->atomic = 0;
	}

	/* above maybe with sanity checks to stop missed ones */


	and to do

	else {
		if (!in_atomic())
			msleep(count);
		else {
			WARN_ON(!ctx->atomic);
			mdelay(count);
		}
	}

so that we know it isn't getting hit in places we've not carefully
considered the impact of a huge stall.

Alan

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Daniel Vetter]

On Wed, Jan 04, 2012 at 12:09:36PM +0000, Alan Cox wrote:
> > I think Alan's simply wrong.
> 
> In what way ? You stated this, then go on below to say what I did ?
> 
> > Splattering the entire driver for
> > these two corner cases which don't happen at all under normal
> > circumstances is imho utter nonsense.
> 
> Which is what I said
> 
> | > > Is this only special cases like a panic - if so can it not be called
> in a
> | > > way that distinguishes between normality and nasty cases.  
> | > 
> | > No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
> | > theory it could be indirectly called from anywhere that uses ATOM
> BIOS.  
> 
> | So lets stick to practice, and the real world. Screwing up everything
> | else because of a crappy problem in your Atom BIOS code sucks but hey it
> | happens. screwing up everything because of a theoretical concern is just
> | dumb.
> 
> 
> We don't want to do mdelays when they are not needed just because it is
> theoretically possible they are not needed. There are several ways to fix
> it, one is to stuff in_atomic() etc in the awkward spots. However if
> you've got something where you have lots of call points to an interface
> this is actually a bad idea, because you grow more insidiously, or a
> change elsewhere in the kernel silently turns all your sleeps into delays
> and things like live music work stop being doable with radeon graphics.
> 
> So the better way to fix it is to actually have an interface
> 
> 	atom_execute_table_atomic()
> 
> so that such situations are called out clearly, and any change elsewhere
> that messes it all up causes kernel spewage that can be dealt with
> properly - either by using _atomic if its something obscure like a panic,
> fixing the issue or reverting the problematic change further up the tree.
> 
> Another way to approach it would be to have
> 
> 	radeon_atom_atomic(rdev);
> 	radeon_atom_whateverfoo(rdev, ...)
> 	radeon_atom_atomic_end(rdev);
> 
> which set and cleared a bitflag that the mdelay/msleep opcode tested.
> 
> That's quicker to implement but a spot less clean. Again it calls out any
> such cases and makes them explicit. It also means any accidental changes
> that affect this will result in spewage and debugging not silent trashing
> of performance for stuff like machine control, music and some gaming.
> 
> It should only be for panic, although accel calls for printk spewage can
> hit in atomic context in some situations but I don't think that becomes
> an atombios path ?

Well, if your dear X server changed the modesetting layout we need to
change it back to the kms fb layout so that we can properly display the
panic.  Which is done by atombios calls afaik (and in i915 has tons of
rather long delays, too).

I agree with you that we should have a decent grip on where we can
actually end up in atomic modeset calls (or for radeon, atombios in
general), which imo should only be the panic handler and kdbg (and similar
unnusual contexts). I'm not sure whether your proposed instrumentation is
really worth it though - in i915-land we don't bother with this. But maybe
with the firmware provided and randomly b0rked atombios tables it might
make sense to add the WARN_ON_ONCE you've proposed in the other mail
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Daniel Vetter]

On Wed, Jan 04, 2012 at 12:09:36PM +0000, Alan Cox wrote:
> > I think Alan's simply wrong.
> 
> In what way ? You stated this, then go on below to say what I did ?
> 
> > Splattering the entire driver for
> > these two corner cases which don't happen at all under normal
> > circumstances is imho utter nonsense.
> 
> Which is what I said
> 
> | > > Is this only special cases like a panic - if so can it not be called
> in a
> | > > way that distinguishes between normality and nasty cases.  
> | > 
> | > No idea, to be honest. It's an ATOM BIOS interpreter opcode, so in
> | > theory it could be indirectly called from anywhere that uses ATOM
> BIOS.  
> 
> | So lets stick to practice, and the real world. Screwing up everything
> | else because of a crappy problem in your Atom BIOS code sucks but hey it
> | happens. screwing up everything because of a theoretical concern is just
> | dumb.

Meh, missed the first part of your mail here. Looks like a
misunderstanding on my side, I was assuming you're proposing to add an
atom_exec_atomic thing which
- would unconditionally do the spinning delay (by completely abolishing
  the in_atomic checks) and
- which would require to pass around a flag from at least the panic
  handler throught the entire modeset code.

Your actual proposal makes much more sense, and as I've said I kinda like
the warning, altough I'm not really sold on the usefulness of it all.
After all we already have all the nice ftrace instrumentation to put blame
for hoggin the cpu in atomic contexts where it needs to be put.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Alan Cox]

> unnusual contexts). I'm not sure whether your proposed instrumentation is
> really worth it though - in i915-land we don't bother with this. But maybe

In i915 land it's specific code paths so effectively it is marked up. We
don't do it for every random delay in all the connectors and other bits.
The bits of code believing they are safe use sleeping locks and we'll get
spewage if that breaks. Ditto at this point I hope gma500 although I
would not be surpised to find ones I still need to fix from being mdelay.

> with the firmware provided and randomly b0rked atombios tables it might
> make sense to add the WARN_ON_ONCE you've proposed in the other mail

I think we need it because it can be added by firmware, it could be
silently done and the atombios paths cover so many different things
beyond modesetting using that exact same function we need the mark up.

If some vendor puts a 100ms delay in a connector related hotplug check we
are going to have a horrible time debugging 'bugzilla #zillion, 'poor
performance in OpenGL game with Radeon foozillion'

Alan

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Daniel Vetter]

On Wed, Jan 04, 2012 at 01:28:28PM +0000, Alan Cox wrote:
> > unnusual contexts). I'm not sure whether your proposed instrumentation is
> > really worth it though - in i915-land we don't bother with this. But maybe
> 
> In i915 land it's specific code paths so effectively it is marked up. We
> don't do it for every random delay in all the connectors and other bits.
> The bits of code believing they are safe use sleeping locks and we'll get
> spewage if that breaks. Ditto at this point I hope gma500 although I
> would not be surpised to find ones I still need to fix from being mdelay.

The atomic context check is in the _wait_for macro in intel_drv.h which is
used all over the place. Like in the wait_for_vblank function. So I don't
think you can use i915.ko as a shiny beacon of how to do it ;-)

> > with the firmware provided and randomly b0rked atombios tables it might
> > make sense to add the WARN_ON_ONCE you've proposed in the other mail
> 
> I think we need it because it can be added by firmware, it could be
> silently done and the atombios paths cover so many different things
> beyond modesetting using that exact same function we need the mark up.
> 
> If some vendor puts a 100ms delay in a connector related hotplug check we
> are going to have a horrible time debugging 'bugzilla #zillion, 'poor
> performance in OpenGL game with Radeon foozillion'

Well, hotplug is alreay a giant pain because of the single lock to rule
them all design of the current kms code: Long delays already stall
everything else (well, cursor updates and pageflips) and we have tons of
bugreports that complain about it. In a sense your example undermines my
point that we have lower hanger fruit to fix all over the place already ...

But as I've said I like the WARN_ON you want to add, but otoh hand think
it shouldn't be used to stall the small&hackish fix of adding the relevant
in_atomic checks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] radeon: Use mdelay() instead of msleep() when we can't sleep in atom_op_delay().

[Alan Cox]

> Well, hotplug is alreay a giant pain because of the single lock to rule
> them all design of the current kms code: Long delays already stall
> everything else (well, cursor updates and pageflips) and we have tons of

Yes I hit this with the gma500, it's one reason I did the GTT based
scrolling.

> bugreports that complain about it. In a sense your example undermines my
> point that we have lower hanger fruit to fix all over the place already ...

Yes.. I'm trying to stop the rotting fruit getting any further down the
tree!

The KMS locking isn't just a KMS problem, KMS inherits some of it from
the framebuffer which inherits some of it from printk which gets some of
it from panic an in IRQ error handling. It's all very messy as a result.

I keep poking at the vt and tty layer end of this to try and sort it out
from the bottom but it's like a bad cess pit, every time you poke it the
smell drives you back to other things.

> But as I've said I like the WARN_ON you want to add, but otoh hand think
> it shouldn't be used to stall the small&hackish fix of adding the relevant
> in_atomic checks.

Then I think we basically agree.

Alan