On 12/03/2024 17:44, Sui Jingfeng wrote:
Hi,
I'm get attracted by your patch, so I want to comment.
Please correct or ignore me if I say something wrong.
On 2024/3/7 17:14, Jocelyn Falempe wrote:
This was initialy done for imx6, but should work on most drivers
using drm_fb_dma_helper.
v8:
* Replace get_scanout_buffer() logic with drm_panic_set_buffer()
(Thomas Zimmermann)
v9:
* go back to get_scanout_buffer()
* move get_scanout_buffer() to plane helper functions
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++
include/drm/drm_fb_dma_helper.h | 4 +++
2 files changed, 51 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c
b/drivers/gpu/drm/drm_fb_dma_helper.c
index 3b535ad1b07c..010327069ad4 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,49 @@ 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)
+/**
+ * @plane: DRM primary plane
+ * @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_plane *plane,
+ struct drm_scanout_buffer *sb)
+{
+ struct drm_gem_dma_object *dma_obj;
+ struct drm_framebuffer *fb;
+
+ fb = plane->state->fb;
+ /* Only support linear modifier */
+ if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
+ return -ENODEV;
I think, the framebuffer format check clause here should be moved to the
core layer.
For example, move this check into thedrm_panic_is_format_supported()
function. And update the drm_panic_is_format_supported() function to
make it take 'struct drm_framebuffer *fb'
as argument. Because this check is needed for all implement, not driver
specific or
implement specific.
I'm unsure about this. I think for some drivers it might be easier to
give a memory buffer different from the plane's drm_framebuffer, and do
their own specific things to display it. So forcing the use of a
drm_framebuffer will remove some flexibility.
I know that some display controller support TILE format, but then you
can try to trigger modeset
to let the display controller using linear format to display. Or, you
can also convert local
linear buffer(with content pained) to the device specific TILED
framebuffer, then feed TILED
framebuffer to the display controller to scanout. This is something like
reverse the process of
resolve, but the convert program is running on the CPU. As panic is not
performance critical,
so it's possible. This maybe a bit more difficult, but the idea behind
this is that the goal of
this drm_panic_gem_get_scanout_buffer() function is just to get a buffer
scanout-able. Other
things beyond that (like format checking) can be moved to upper
level(the caller of it). So you
don't need to modify(update) the specific implement if one day you have
some fancy idea implemented.
Correct me if I'm wrong, but the tiled format are mostly hardware
dependent, and I don't think we can have a generic implementation, that
can cover multiple hardware.
I want to add support for multi-planar format like YUV for drm_panic
later, but for tiled buffer, I think it should be easier to deactivate
tiling on the hardware itself.
+ dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+
+ /* Buffer should be accessible from the CPU */
+ if (dma_obj->base.import_attach)
+ return -ENODEV;
+
+ /* Buffer should be already mapped to CPU */
+ if (!dma_obj->vaddr)
+ return -ENODEV;
How about to call drm_gem_vmap_unlocked(dma_obj, &sb->map); ?
It is not safe to call it from panic context, because it takes locks:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem.c#L1212
It may lockup the panic handler, and prevent other panic routine to run
(like kdump).
So it's better to call drm_gem_vmap_unlocked() when the driver prepares
the buffer for the primary plane, than doing it from the panic handler.
Best regards,
--
Jocelyn
+ 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;
+}
+#else
+int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
+ struct drm_scanout_buffer *sb)
+{
+ return -ENODEV;
+}
+#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..61f24c2aba2f 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_plane *plane,
+ struct drm_scanout_buffer *sb);
+
#endif
