On Thu, Dec 14, 2023 at 02:48:21PM +0100, Maxime Ripard wrote:
> Hi,
>
> On Fri, Nov 03, 2023 at 03:53:30PM +0100, Jocelyn Falempe wrote:
> > Proof of concept to add drm_panic support on an arm based GPU.
> > I've tested it with X11/llvmpipe, because I wasn't able to have
> > 3d rendering with etnaviv on my imx6 board.
> >
> > Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
>
> Like I said in the v6, this shouldn't be dropped because it also kind of
> documents and shows what we are expecting from a "real" driver.
>
> > ---
> > drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 30 ++++++++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> > index 4a866ac60fff..db24b4976c61 100644
> > --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> > @@ -10,6 +10,7 @@
> > #include <linux/dma-buf.h>
> > #include <linux/module.h>
> > #include <linux/platform_device.h>
> > +#include <linux/iosys-map.h>
> >
> > #include <video/imx-ipu-v3.h>
> >
> > @@ -17,9 +18,12 @@
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_drv.h>
> > #include <drm/drm_fbdev_dma.h>
> > +#include <drm/drm_fb_dma_helper.h>
> > +#include <drm/drm_framebuffer.h>
> > #include <drm/drm_gem_dma_helper.h>
> > #include <drm/drm_gem_framebuffer_helper.h>
> > #include <drm/drm_managed.h>
> > +#include <drm/drm_panic.h>
> > #include <drm/drm_of.h>
> > #include <drm/drm_probe_helper.h>
> > #include <drm/drm_vblank.h>
> > @@ -160,6 +164,31 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
> > return ret;
> > }
> >
> > +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.
>
> When the driver gives back its current framebuffer, the code should check:
Oh, and also, you need to keep an eye on the solid fill support:
https://lore.kernel.org/all/20231027-solid-fill-v7-0-780188bfa7b2@xxxxxxxxxxx/
Because with that, a plane might not have a framebuffer anymore.
> * If the buffer backed by an actual buffer (and not a dma-buf handle)
> * If the buffer is mappable by the CPU
> * If the buffer is in a format that the panic code can handle
> * If the buffer uses a linear modifier
>
> 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?
>
> Maxime
Attachment:
signature.asc
Description: PGP signature
