| Message ID | 20231103145526.628138-7-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a drm panic handler | expand |
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@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
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@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
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@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. 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,
On Thu, Dec 14, 2023 at 04:03:04PM +0100, Jocelyn Falempe wrote: > > > +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. Sure. Having a minimal panic code is reasonable, but we should properly handle not supporting them still :) There's cases where, with the current architecture anyway, you just won't be able to print a panic message and that's fine. The important part is not crashing the kernel further and being as loud as we can that we couldn't print a panic message on the screen because of the setup. > > 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 ? Its backing drm_gem_object should be set and have the field import_attach set > > * If the buffer is mappable by the CPU > > If "dma_obj->vaddr" is not null, then it's already mapped by the CPU right ? I'm not sure. drm_gem_dma_create will only ever create CPU-mappable buffers, but drm_gem_dma_prime_import_sg_table won't for example. > > * 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. Sure :) I guess we could have a CRTC function then that by default will just return the current primary plane framebuffer (or could be a plane function?), and if it's not there just grabs the one from the current active state. Maxime
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; + sb->height = plane->state->fb->height; + sb->width = plane->state->fb->width; + sb->pitch = plane->state->fb->pitches[0]; + return 0; + } + return -ENODEV; +} + static const struct drm_driver imx_drm_driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC, DRM_GEM_DMA_DRIVER_OPS_WITH_DUMB_CREATE(imx_drm_dumb_create), @@ -172,6 +201,7 @@ static const struct drm_driver imx_drm_driver = { .major = 1, .minor = 0, .patchlevel = 0, + .get_scanout_buffer = imx_drm_get_scanout_buffer, }; static int compare_of(struct device *dev, void *data)
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@redhat.com> --- drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+)