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

From: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>,
	bluescreen_avenger@verizon.net, javierm@redhat.com,
	dri-devel@lists.freedesktop.org, gpiccoli@igalia.com,
	noralf@tronnes.org, Daniel Vetter <daniel@ffwll.ch>,
	airlied@redhat.com
Subject: Re: Re: [PATCH v7 6/9] drm/simpledrm: Add drm_panic support
Date: Thu, 18 Jan 2024 11:33:43 +0100	[thread overview]
Message-ID: <evrz2pixfsze6a4splahb3tgfwqlo22qxyfmvrcxlkwvdl5our@rk4o3qrfo6i5> (raw)
In-Reply-To: <d46b3361-297f-4361-bc6b-68c4109a63aa@suse.de>

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]