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

[Maxime Ripard]

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 at redhat.com>
> 
> 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@quicinc.com/

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


-------------- 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/20231214/2f1a64b6/attachment.sig>