[PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

Hi Jocelyn,

On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
> On 05/10/2023 10:18, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index 89e2706cac56..e538c87116d3 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -43,6 +43,7 @@ struct dma_buf_attachment;
> > >   struct drm_display_mode;
> > >   struct drm_mode_create_dumb;
> > >   struct drm_printer;
> > > +struct drm_scanout_buffer;
> > >   struct sg_table;
> > >   /**
> > > @@ -408,6 +409,19 @@ struct drm_driver {
> > >   	 */
> > >   	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
> > > +	/**
> > > +	 * @get_scanout_buffer:
> > > +	 *
> > > +	 * Get the current scanout buffer, to display a panic message with drm_panic.
> > > +	 * It is called from a panic callback, and must follow its restrictions.
> > > +	 *
> > > +	 * Returns:
> > > +	 *
> > > +	 * Zero on success, negative errno on failure.
> > > +	 */
> > > +	int (*get_scanout_buffer)(struct drm_device *dev,
> > > +				  struct drm_scanout_buffer *sb);
> > > +
> > 
> > What is the format of that buffer? What is supposed to happen if the
> > planes / CRTC are setup in a way that is incompatible with the buffer
> > format?
> 
> Currently, it only supports linear format, either in system memory, or
> iomem.
> But really what is needed is the screen size, and a way to write pixels to
> it.
> For more complex GPU, I don't know if it's easier to reprogram the GPU to
> linear format, or to add a simple "tiled" support to drm_panic.
> What would you propose as a panic interface to handle those complex format ?

It's not just about tiling, but also about YUV formats. If the display
engine is currently playing a video at the moment, it's probably going
to output some variation of multi-planar YUV and you won't have an RGB
buffer available.

Same story if you're using a dma-buf buffer. You might not even be able
to access that buffer at all from the CPU or the kernel.

I really think we should have some emergency state ready to commit on
the side, and possibly a panic_commit function to prevent things like
sleeping or waiting that regular atomic_commit can use.

That way, you know have all the resources available to you any time.

> Sometime it's also just not possible to write pixels to the screen, like if
> the panic occurs in the middle of suspend/resume, or during a mode-setting,
> and the hardware state is broken. In this case it's ok to return an error,
> and nothing will get displayed.

And yeah, you won't be able to do it every time, but if it's never for
some workload it's going to be a concern.

Anyway, we should at the very least document what we expect here.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231006/f11a3286/attachment.sig>