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)
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.
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.
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.
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...
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.
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
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.
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.
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.
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.
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
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.
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
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.
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
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); }
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
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.
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
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.
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
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Alan Cox -
Daniel Vetter -
Dave Airlie -
Michel Dänzer