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:
signature.asc
Description: PGP signature
