[Intel-gfx] [PATCH v2 1/3] drm: Add support for panic message output

[Daniel Vetter]

On Thu, Mar 14, 2019 at 03:51:13AM +0100, Ahmed S. Darwish wrote:
> 
> [[ Adding Sebastian, who is quite experienced in intricate
>    locking situations due to daily PREEMPT_RT work.. ]]
> 
> On Wed, Mar 13, 2019 at 09:37:10AM +0100, Daniel Vetter wrote:
> > On Wed, Mar 13, 2019 at 08:49:17AM +0100, John Ogness wrote:
> > > On 2019-03-12, Ahmed S. Darwish <darwish.07 at gmail.com> wrote:
> > > > On Wed, Mar 13, 2019 at 09:37:10AM +0100, Daniel Vetter wrote:
> [……]
> > > > >
> > > > > If we aim for perfect this should be a trylock still, maybe using
> > > > > our own device list.
> > > > >
> > > >
> > > > I definitely agree here.
> > > >
> > > > The lock may already be locked either by a stopped CPU, or by the
> > > > very same CPU we execute panic() on (e.g. NMI panic() on the
> > > > printing CPU).
> > > >
> > > > This is why it's very common for example in serial consoles, which
> > > > are usually careful about re-entrance and panic contexts, to do:
> > > >
> > > >   xxxxxx_console_write(...) {
> > > > 	if (oops_in_progress)
> > > > 		locked = spin_trylock_irqsave(&port->lock, flags);
> > > > 	else
> > > > 		spin_lock_irqsave(&port->lock, flags);
> > > >   }
> > > >
> > > > I'm quite positive we should do the same for panic drm drivers.
> > >
> > > This construction will continue, even if the trylock fails. It only
> > > makes sense to do this if the driver has a chance of being
> > > successful. Ignoring locks is a serious issue. I personally am quite
> > > unhappy that the serial drivers do this, which was part of my motivation
> > > for the new printk design I'm working on.
> > >
> > > If the driver is not capable of doing something useful on a failed
> > > trylock, then I recommend just skipping that device. Maybe trying it
> > > again later after trying all the devices?
> >
> > Ah yes missed that. If the trylock fails anywhere, we must bail out.
> >
> > Not sure retrying is useful, my experience from at least drm is that
> > either you're lucky, and drm wasn't doing anything right when the machine
> > blew up, and then the trylocks will all go through. Or you're unlucky, and
> > most likely that means drm itself blew up, and no amount of retrying is
> > going to help. I wouldn't bother.
> >
> 
> Ack, retrying most probably won't help.
> 
> I see that, at least in x86:
> 
>   kernel/panic.c::panic()
>     => kernel/panic.c::smp_send_stop()
>       => arch/x86/incluse/smp.h::smp_send_stop(0)
>         => arch/x86/kernel/smp.c::native_stop_other_cpus(wait)
>           /*
>            * Don't wait longer than a second if the caller
>            * didn't ask us to wait.
>            */
>            timeout = USEC_PER_SEC;
>            while (num_online_cpus() > 1 && (wait || timeout--))
>                    udelay(1);
> 
> So the panic()-ing core wait by default for _a full second_ until
> all other cores finish their non-preemptible regions. If that did
> not work out, it even tries more aggressively with NMIs.
> 
> So if one of the other non panic()-ing cores was holding a DRM
> related lock through spin_lock_irqsave() for more than one second,
> well, it's a bug in the DRM layer code anyway..      [A]
> 
> Problem is, what will happen if the non panic()-ing core was
> holding a DRM lock through barebone spin_lock()? Interrupts are
> enabled there, so the IPI will be handled, and thus that core will
> effectively die while the lock is forever held :-(   [B]

Just to hammer this in even more:

If any of those locks are held, the hw/driver is in an undefined state. If
you then touch the hw there's really good chances you kill the entire
machine. Happened plenty with the old oops handling, which tried to do
that. That's why we want none of any of these. Whethere it's modesetting
or a gpu reset, gpu hw and drivers is just way too complex to make that
feasible.

That's why we came up with the trylock + immediate bail out design if that
fails. Plus really only render the oops int whatever is the current
display buffer, so that we don't have to do any hw programming at all.
-Daniel
> 
> But, well, yeah, I don't think there's a solution for the [B] part
> except by spin_trylock() the fewest amount of locks possible, and
> bailing out if anyone of them is held.
> 
> For reference, I asked over IRC why not just resetting GPU state,
> but it's not easy as it sounds to people outside of the gfx world:
> 
>   [abridged]
>   <darwi>  So it seems Windows is forcing driver writers to
>            implement a gpu reset method [*]
>   <darwi>  since Windows vista
>   <danvet> yeah
>   <danvet> not going to do that from panic
>   <danvet> there's a non-zero chance that gpu reset takes down
>            the system with at least lots of hw/driver combos
>   <danvet> I think all our drivers also have gpu reset support
>   <danvet> if something is still rendering while we panic, mea culpa
>   <danvet> only way around that is switching planes
>   <danvet> which we tried with the old approach, and that's just
>            too hard
>   <danvet> you end up running a few 100kloc of code worst case
>   <danvet> no way to audit/validate that for panic context
>   <danvet> but the rendering shouldn't really matter
>   <anholt> yeah, actually modesetting in a panic seems hopeless.
>   <danvet> at least for modern compositors
>   <danvet> anholt, yeah I killed that idea
>   <danvet> so they only render to the backbuffer
>   <danvet> and we won't show that with a panic
>   <anholt> yep.  just draw on a plane.  for everything but
>            uncomposited x11, it'll already be idle.
>   <danvet> so only really an issue on boot splash and frontbuffer x11
>   <danvet> and there a few cursors/characters drawn won't really
>            matter in most cases
>   <danvet> anholt, yeah that's the idea, we pick the currently
>            showing plane and have a simple draw_xy functions which
> 	   knows how to draw bright/dark pixels for any drm_fourcc
>   <danvet> so you can see the oops even if it's yuv or whatever
>   <anholt> nice
> 
> Thanks!
> 
> [*] DXGKDDI_RESETFROMTIMEOUT callback function:
>     https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/d3dkmddi/nc-d3dkmddi-dxgkddi_resetfromtimeout
> 
> --
> darwi
> http://darwish.chasingpointers.com

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch