| Message ID | 20240307091936.576689-6-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a drm panic handler | expand |
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@redhat.com> > --- > 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 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. > + 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); ? > + 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 >
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@redhat.com> >> --- >> 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,
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; + + 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; + + 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
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@redhat.com> --- drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++ include/drm/drm_fb_dma_helper.h | 4 +++ 2 files changed, 51 insertions(+)