Re: [PATCH v7 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

From: Daniel Vetter <daniel@ffwll.ch>
To: Jocelyn Falempe <jfalempe@redhat.com>
Cc: bluescreen_avenger@verizon.net, tzimmermann@suse.de,
	javierm@redhat.com, mripard@kernel.org, gpiccoli@igalia.com,
	noralf@tronnes.org, dri-devel@lists.freedesktop.org,
	airlied@redhat.com
Subject: Re: [PATCH v7 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
Date: Fri, 12 Jan 2024 14:41:53 +0100	[thread overview]
Message-ID: <ZaFBofhe217zCmWN@phenom.ffwll.local> (raw)
In-Reply-To: <20240104160301.185915-6-jfalempe@redhat.com>

On Thu, Jan 04, 2024 at 05:00:49PM +0100, Jocelyn Falempe wrote:
> This was initialy done for imx6, but should work on most drivers
> using drm_fb_dma_helper.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_dma_helper.c | 55 +++++++++++++++++++++++++++++
>  include/drm/drm_fb_dma_helper.h     |  4 +++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> index 3b535ad1b07c..caed2935df4f 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_dma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_panic.h>
>  #include <drm/drm_plane.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
> @@ -148,3 +149,57 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
>  	}
>  }
>  EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent);
> +
> +#if defined(CONFIG_DRM_PANIC)
> +/**
> + * @dev: DRM device
> + * @drm_scanout_buffer: scanout buffer for the panic handler
> + * Returns: 0 or negative error code
> + *
> + * Generic get_scanout_buffer() implementation, for drivers that uses the
> + * drm_fb_dma_helper.
> + */
> +int drm_panic_gem_get_scanout_buffer(struct drm_device *dev,
> +				     struct drm_scanout_buffer *sb)
> +{
> +	struct drm_plane *plane;
> +	struct drm_gem_dma_object *dma_obj;
> +	struct drm_framebuffer *fb;
> +
> +	drm_for_each_primary_visible_plane(plane, dev) {

Ok that's not enough locking by far. You can't just hope that nothing
disappears while you're in a panic handler. We've been there and ended up
reliably oopsing in the panic handler itself. So you _have_ to follow the
full set of locking rules for all drm structures, or things will just get
worse at the worst possible moment.

But also, you're not allowed to do anything else than trylock, because a
panic handler might run from nmi context, and so you cannot even acquire
irq-safe spinlocks reliably.

Which means:

- You need to be safe against concurrent drm_dev_unregister. Using the
  atomic panic notifier directly for each device should take care of that
  (but maybe that stuff is still not nmi safe, not sure).

- You _have_ to use all the locks. Luckily iterating over the plane list
  doesn't need one, but you have to trylock the plane's modeset lock.
  Which means your nice iterator macro is already toast, because that
  already looks at state it's not allowed to look at without a lock. Or
  well, the plane->state pointer is no-go already.

Given the locking issues I'm not sure whether the
drm_for_each_primary_visible_plane iterator is going to work, you'd need
something like iter_init/next/end we have for walking the connector list.
Plus it would be very panic specific due to the trylock, so maybe

drm_for_each_visible_plane_in_panic_handler()

or something like that.

One thing I was wondering is whether we should lift this iteration over
all planes into the shared code, and move the ->get_scanout_buffer
function to the drm_plane_funcs structure instead?

> +		fb = plane->state->fb;
> +		/* Only support linear modifier */
> +		if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
> +			continue;
> +
> +		/* Check if color format is supported */
> +		if (!drm_panic_is_format_supported(fb->format->format))
> +			continue;
> +
> +		dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +
> +		/* Buffer should be accessible from the CPU */
> +		if (dma_obj->base.import_attach)

This might be a bit too restrictive, since some drivers import dma-buf
including a vmap. So just checking for ->vaddr might be better. But can be
changed later on.

> +			continue;
> +
> +		/* Buffer should be already mapped to CPU */

I'd clarify this comment to state that vaddr is invariant over the
lifetime of the buffer and therefore needs no locking. Correct locking
that a) takes all the locks b) never ever stalls for one is absolutely
crucial for a panic handler that won't make the situation worse.

> +		if (!dma_obj->vaddr)

> +			continue;
> +
> +		iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
> +		sb->format = fb->format;
> +		sb->height = fb->height;
> +		sb->width = fb->width;
> +		sb->pitch = fb->pitches[0];
> +		return 0;

Otherwise this lgtm.
-Sima

> +	}
> +	return -ENODEV;
> +}
> +#else
> +int drm_panic_gem_get_scanout_buffer(struct drm_device *dev,
> +				     struct drm_scanout_buffer *sb)
> +{
> +	return 0;
> +}
> +#endif
> +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer);
> diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h
> index d5e036c57801..2ae432865079 100644
> --- a/include/drm/drm_fb_dma_helper.h
> +++ b/include/drm/drm_fb_dma_helper.h
> @@ -7,6 +7,7 @@
>  struct drm_device;
>  struct drm_framebuffer;
>  struct drm_plane_state;
> +struct drm_scanout_buffer;
>  
>  struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb,
>  	unsigned int plane);
> @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
>  				  struct drm_plane_state *old_state,
>  				  struct drm_plane_state *state);
>  
> +int drm_panic_gem_get_scanout_buffer(struct drm_device *dev,
> +				     struct drm_scanout_buffer *sb);
> +
>  #endif
>  
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch