On 14/12/2023 14:48, 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.
Ok, I can bring it back in v7.
---
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.
Yes, this code is a bit hacky, and on my test setup the framebuffer was
in RGB, so I didn't handle other formats.
Also it should be possible to add YUV format later, but I would like to
have a minimal drm_panic merged, before adding more features.
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)
Regarding the struct drm_framebuffer, I'm not sure how do you
differentiate an actual buffer from a dma-buf handle ?
* If the buffer is mappable by the CPU
If "dma_obj->vaddr" is not null, then it's already mapped by the CPU right ?
* If the buffer is in a format that the panic code can handle
* If the buffer uses a linear modifier
Yes, that must be checked too.
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?
Yes, I think I can do a generic implementation for the drivers using the
drm_gem_dma helper like imx6.
But for simpledrm, ast, or mgag200, I need to retrieve the VRAM address,
because it's not in the drm_framebuffer struct, so they won't be able to
use this generic implementation.
Maxime
Thanks for the review,
--
Jocelyn
