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

[Daniel Vetter]

On Wed, Mar 13, 2019 at 04:54:50PM +0100, Christian König wrote:
> Am 13.03.19 um 16:38 schrieb Michel Dänzer:
> > On 2019-03-13 2:37 p.m., Christian König wrote:
> > > Am 13.03.19 um 14:31 schrieb Ville Syrjälä:
> > > > On Wed, Mar 13, 2019 at 10:35:08AM +0100, Michel Dänzer wrote:
> > > > > On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:
> > > > > > Den 12.03.2019 17.17, skrev Ville Syrjälä:
> > > > > > > On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:
> > > > > > > > On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:
> > > > > > > > > This adds support for outputting kernel messages on panic().
> > > > > > > > > A kernel message dumper is used to dump the log. The dumper iterates
> > > > > > > > > over each DRM device and it's crtc's to find suitable framebuffers.
> > > > > > > > > 
> > > > > > > > > All the other dumpers are run before this one except mtdoops.
> > > > > > > > > Only atomic drivers are supported.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> > > > > > > > > ---
> > > > > > > > >    [...]
> > > > > > > > > 
> > > > > > > > > diff --git a/include/drm/drm_framebuffer.h
> > > > > > > > > b/include/drm/drm_framebuffer.h
> > > > > > > > > index f0b34c977ec5..f3274798ecfe 100644
> > > > > > > > > --- a/include/drm/drm_framebuffer.h
> > > > > > > > > +++ b/include/drm/drm_framebuffer.h
> > > > > > > > > @@ -94,6 +94,44 @@ struct drm_framebuffer_funcs {
> > > > > > > > >                 struct drm_file *file_priv, unsigned flags,
> > > > > > > > >                 unsigned color, struct drm_clip_rect *clips,
> > > > > > > > >                 unsigned num_clips);
> > > > > > > > > +
> > > > > > > > > +    /**
> > > > > > > > > +     * @panic_vmap:
> > > > > > > > > +     *
> > > > > > > > > +     * Optional callback for panic handling.
> > > > > > > > > +     *
> > > > > > > > > +     * For vmapping the selected framebuffer in a panic context.
> > > > > > > > > Must
> > > > > > > > > +     * be super careful about locking (only trylocking allowed).
> > > > > > > > > +     *
> > > > > > > > > +     * RETURNS:
> > > > > > > > > +     *
> > > > > > > > > +     * NULL if it didn't work out, otherwise an opaque cookie
> > > > > > > > > which is
> > > > > > > > > +     * passed to @panic_draw_xy. It can be anything: vmap area,
> > > > > > > > > structure
> > > > > > > > > +     * with more details, just a few flags, ...
> > > > > > > > > +     */
> > > > > > > > > +    void *(*panic_vmap)(struct drm_framebuffer *fb);
> > > > > > > > FWIW, the panic_vmap hook cannot work in general with the
> > > > > > > > amdgpu/radeon
> > > > > > > > drivers:
> > > > > > > > 
> > > > > > > > Framebuffers are normally tiled, writing to them with the CPU
> > > > > > > > results in
> > > > > > > > garbled output.
> > > > > > > > 
> > > > > > In which case the driver needs to support the ->panic_draw_xy callback,
> > > > > > or maybe it's possible to make a generic helper for tiled buffers.
> > > > > I'm afraid that won't help, at least not without porting big chunks of
> > > > > https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib
> > > > > into the kernel, none of which will be used for anything else.
> > > > > 
> > > > > 
> > > > > > > > There would need to be a mechanism for switching scanout to a linear,
> > > > > > > > CPU accessible framebuffer.
> > > > > > > I suppose panic_vmap() could just provide a linear temp buffer
> > > > > > > to the panic handler, and panic_unmap() could copy the contents
> > > > > > > over to the real fb.
> > > > > Copy how? Using a GPU engine?
> > > > CPU maybe? Though I suppose that won't work if the buffer isn't CPU
> > > > accesible :/
> > > Well we do have a debug path for accessing invisible memory with the CPU.
> > > 
> > > E.g. three registers: DATA and auto increment OFFSET_LO/HI. So you can
> > > just read/write DATA over and over again if you want to access some memory.
> > Right. I assume that'll be very slow, but I guess it could do when the
> > memory isn't directly CPU accessible.
> 
> Just made a quick test and reading 33423360 bytes (4096x2040x4) using that
> interfaces takes about 13 seconds.
> 
> IIRC we don't use the auto increment optimization yet, so that can probably
> be improved by a factor of 3 or more.
> 
> > > But turning of tilling etc is still extremely tricky when the system is
> > > already unstable.
> > Maybe we could add a little hook to the display code, which just
> > disables tiling for scanout and maybe disables non-primary planes, but
> > doesn't touch anything else. Harry / Nicholas, does that seem feasible?
> > 
> > 
> > I'm coming around from "this is never going to work" to "it might
> > actually work" with our hardware...
> 
> Yeah, agree. It's a bit tricky, but doable.
> 
> Takeaway for Noralf is that this whole vmap on panic won't even remotely
> work. We need to get the data byte by byte without a page mapping if that is
> ever going to fly.

Don't be hung up on the vmap, the main interface is a pixel-by-pixel
panic_draw_xy. Plus panic_prepare/cleanup to do whatever setup you need to
do (like disable tiling to avoid having a copy of the detiling library in
the kernel for nothing else than oops printing). vmap is just what
conveniently works for noral on his cma drivers.

The important bit is that we run the minimal amount of code to draw pixels
onto the current scanout buffers. For multiple I think best is to just
pick the biggest visible rectangle, but not do stuff like disable planes -
that sounds way too risky. Usually there's a main window that should be
big enough to show the oops.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch