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

[Maxime Ripard]

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:

  * 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/4311889f/attachment.sig>