Re: [PATCH v5 6/6] drm/imx: Add drm_panic support

From: Maxime Ripard <mripard@kernel.org>
To: Jocelyn Falempe <jfalempe@redhat.com>
Cc: bluescreen_avenger@verizon.net, tzimmermann@suse.de,
	javierm@redhat.com, dri-devel@lists.freedesktop.org,
	gpiccoli@igalia.com, noralf@tronnes.org, airlied@redhat.com
Subject: Re: [PATCH v5 6/6] drm/imx: Add drm_panic support
Date: Thu, 14 Dec 2023 19:37:08 +0100	[thread overview]
Message-ID: <va2vitynjawcvtayi562qvwq32l4m7bt3ils2bvnbbv2hcsbgb@j3gy552jgseh> (raw)
In-Reply-To: <3081e418-275c-4069-b22c-c3e9770fc641@redhat.com>

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

On Thu, Dec 14, 2023 at 04:03:04PM +0100, Jocelyn Falempe wrote:
> > > +static int imx_drm_get_scanout_buffer(struct drm_device *dev,
> > > +				      struct drm_scanout_buffer *sb)
> > > +{
> > > +	struct drm_plane *plane;
> > > +	struct drm_gem_dma_object *dma_obj;
> > > +
> > > +	drm_for_each_plane(plane, dev) {
> > > +		if (!plane->state || !plane->state->fb || !plane->state->visible ||
> > > +		    plane->type != DRM_PLANE_TYPE_PRIMARY)
> > > +			continue;
> > > +
> > > +		dma_obj = drm_fb_dma_get_gem_obj(plane->state->fb, 0);
> > > +		if (!dma_obj->vaddr)
> > > +			continue;
> > > +
> > > +		iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
> > > +		sb->format = plane->state->fb->format;
> > 
> > Planes can be using a framebuffer in one of the numerous YUV format the
> > driver advertises.
> > 
> > > +		sb->height = plane->state->fb->height;
> > > +		sb->width = plane->state->fb->width;
> > > +		sb->pitch = plane->state->fb->pitches[0];
> > 
> > And your code assumes that the buffer will be large enough for an RGB
> > buffer, which probably isn't the case for a single-planar YUV format,
> > and certainly not the case for a multi-planar one.
> 
> Yes, this code is a bit hacky, and on my test setup the framebuffer was in
> RGB, so I didn't handle other formats.
> Also it should be possible to add YUV format later, but I would like to have
> a minimal drm_panic merged, before adding more features.

Sure. Having a minimal panic code is reasonable, but we should properly
handle not supporting them still :)

There's cases where, with the current architecture anyway, you just
won't be able to print a panic message and that's fine. The important
part is not crashing the kernel further and being as loud as we can that
we couldn't print a panic message on the screen because of the setup.

> > When the driver gives back its current framebuffer, the code should check:
> > 
> >    * If the buffer backed by an actual buffer (and not a dma-buf handle)
> 
> Regarding the struct drm_framebuffer, I'm not sure how do you differentiate
> an actual buffer from a dma-buf handle ?

Its backing drm_gem_object should be set and have the field import_attach set

> >    * If the buffer is mappable by the CPU
> 
> If "dma_obj->vaddr" is not null, then it's already mapped by the CPU right ?

I'm not sure. drm_gem_dma_create will only ever create CPU-mappable
buffers, but drm_gem_dma_prime_import_sg_table won't for example.

> >    * If the buffer is in a format that the panic code can handle
> >    * If the buffer uses a linear modifier
> 
> Yes, that must be checked too.
> 
> > 
> > Failing that, your code cannot work at the moment. We need to be clear
> > about that and "gracefully" handle things instead of going forward and
> > writing pixels to places we might not be able to write to.
> > 
> > Which kind of makes me think, why do we need to involve the driver at
> > all there?
> > 
> > If in the panic code, we're going over all enabled CRTCs, finding the
> > primary plane currently setup for them and getting the drm_framebuffer
> > assigned to them, it should be enough to get us all the informations we
> > need, right?
> 
> Yes, I think I can do a generic implementation for the drivers using the
> drm_gem_dma helper like imx6.
> But for simpledrm, ast, or mgag200, I need to retrieve the VRAM address,
> because it's not in the drm_framebuffer struct, so they won't be able to use
> this generic implementation.

Sure :)

I guess we could have a CRTC function then that by default will just
return the current primary plane framebuffer (or could be a plane
function?), and if it's not there just grabs the one from the current
active state.

Maxime

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