[PATCH v7 6/9] drm/simpledrm: Add drm_panic support

[Maxime Ripard]

On Wed, Jan 17, 2024 at 04:22:01PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.01.24 um 14:58 schrieb Maxime Ripard:
> > On Fri, Jan 12, 2024 at 02:44:57PM +0100, Daniel Vetter wrote:
> > > On Thu, Jan 04, 2024 at 05:00:50PM +0100, Jocelyn Falempe wrote:
> > > > Add support for the drm_panic module, which displays a user-friendly
> > > > message to the screen when a kernel panic occurs.
> > > > 
> > > > Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> > > > ---
> > > >   drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++
> > > >   1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > > > index 7ce1c4617675..6dd2afee84d4 100644
> > > > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > > > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > > > @@ -25,6 +25,7 @@
> > > >   #include <drm/drm_gem_shmem_helper.h>
> > > >   #include <drm/drm_managed.h>
> > > >   #include <drm/drm_modeset_helper_vtables.h>
> > > > +#include <drm/drm_panic.h>
> > > >   #include <drm/drm_probe_helper.h>
> > > >   #define DRIVER_NAME	"simpledrm"
> > > > @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >   	return sdev;
> > > >   }
> > > > +static int simpledrm_get_scanout_buffer(struct drm_device *dev,
> > > > +					struct drm_scanout_buffer *sb)
> > > > +{
> > > > +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
> > > 
> > > So I guess simpledrm is the reason why the get_scanout_buffer hook is at
> > > the device level and not at the plane level. Even from the few drivers you
> > > have in your series it seems very much the exception, so I'm not sure
> > > whether that's the best design.
> > > 
> > > I guess we'll know when we see the plane iterator code with the right
> > > locking, whether it's ok to have that in driver hooks or it's better to
> > > pull it out into shared code.
> > 
> > Wouldn't the CRTC level be better than the planes?
> 
> What's in favor of the CRTC level?
> 
> I'd put a hook at the plane level and do the
> drm_for_each_primary_visible_plane() in the panic handler. Simpledrm would
> fit into this pattern nicely.
> 
> But it's not like I have strong feeling about this. The current callbacks
> are simple enough.

An active CRTC is guaranteed to have an active plane, and knows what the
full blending story is. An active plane does have a CRTC too, but there
might be other planes we want to consider, and if you're doing blending
with multiple primary planes it will get quite funny very fast :)

Plus, some CRTC have internal SRAMs we could use as fallback in the case
where we won't be able to get a proper framebuffer.

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/20240118/0e96cc98/attachment.sig>