[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 at ffwll.ch
Mobile: +41 (0)79 365 57 48