| Message ID | 20240104160301.185915-7-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a drm panic handler | expand |
On Thu, Jan 04, 2024 at 05:00:50PM +0100, Jocelyn Falempe wrote: > Add support for the drm_panic module, which displays a user-friendly > message to the screen when a kernel panic occurs. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > index 7ce1c4617675..6dd2afee84d4 100644 > --- a/drivers/gpu/drm/tiny/simpledrm.c > +++ b/drivers/gpu/drm/tiny/simpledrm.c > @@ -25,6 +25,7 @@ > #include <drm/drm_gem_shmem_helper.h> > #include <drm/drm_managed.h> > #include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_panic.h> > #include <drm/drm_probe_helper.h> > > #define DRIVER_NAME "simpledrm" > @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, > return sdev; > } > > +static int simpledrm_get_scanout_buffer(struct drm_device *dev, > + struct drm_scanout_buffer *sb) > +{ > + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); So I guess simpledrm is the reason why the get_scanout_buffer hook is at the device level and not at the plane level. Even from the few drivers you have in your series it seems very much the exception, so I'm not sure whether that's the best design. I guess we'll know when we see the plane iterator code with the right locking, whether it's ok to have that in driver hooks or it's better to pull it out into shared code. -Sima > + > + sb->width = sdev->mode.hdisplay; > + sb->height = sdev->mode.vdisplay; > + sb->pitch = sdev->pitch; > + sb->format = sdev->format; > + sb->map = sdev->screen_base; > + return 0; > +} > + > /* > * DRM driver > */ > @@ -1000,6 +1014,7 @@ static struct drm_driver simpledrm_driver = { > .minor = DRIVER_MINOR, > .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, > .fops = &simpledrm_fops, > + .get_scanout_buffer = simpledrm_get_scanout_buffer, > }; > > /* > -- > 2.43.0 >
On Fri, Jan 12, 2024 at 02:44:57PM +0100, Daniel Vetter wrote: > On Thu, Jan 04, 2024 at 05:00:50PM +0100, Jocelyn Falempe wrote: > > Add support for the drm_panic module, which displays a user-friendly > > message to the screen when a kernel panic occurs. > > > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > > --- > > drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > > index 7ce1c4617675..6dd2afee84d4 100644 > > --- a/drivers/gpu/drm/tiny/simpledrm.c > > +++ b/drivers/gpu/drm/tiny/simpledrm.c > > @@ -25,6 +25,7 @@ > > #include <drm/drm_gem_shmem_helper.h> > > #include <drm/drm_managed.h> > > #include <drm/drm_modeset_helper_vtables.h> > > +#include <drm/drm_panic.h> > > #include <drm/drm_probe_helper.h> > > > > #define DRIVER_NAME "simpledrm" > > @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, > > return sdev; > > } > > > > +static int simpledrm_get_scanout_buffer(struct drm_device *dev, > > + struct drm_scanout_buffer *sb) > > +{ > > + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); > > So I guess simpledrm is the reason why the get_scanout_buffer hook is at > the device level and not at the plane level. Even from the few drivers you > have in your series it seems very much the exception, so I'm not sure > whether that's the best design. > > I guess we'll know when we see the plane iterator code with the right > locking, whether it's ok to have that in driver hooks or it's better to > pull it out into shared code. Wouldn't the CRTC level be better than the planes? Maxime
Am 04.01.24 um 17:00 schrieb Jocelyn Falempe: > Add support for the drm_panic module, which displays a user-friendly > message to the screen when a kernel panic occurs. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > index 7ce1c4617675..6dd2afee84d4 100644 > --- a/drivers/gpu/drm/tiny/simpledrm.c > +++ b/drivers/gpu/drm/tiny/simpledrm.c > @@ -25,6 +25,7 @@ > #include <drm/drm_gem_shmem_helper.h> > #include <drm/drm_managed.h> > #include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_panic.h> > #include <drm/drm_probe_helper.h> > > #define DRIVER_NAME "simpledrm" > @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, > return sdev; > } > > +static int simpledrm_get_scanout_buffer(struct drm_device *dev, > + struct drm_scanout_buffer *sb) > +{ > + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); > + > + sb->width = sdev->mode.hdisplay; > + sb->height = sdev->mode.vdisplay; > + sb->pitch = sdev->pitch; > + sb->format = sdev->format; > + sb->map = sdev->screen_base; > + return 0; > +} > + > /* > * DRM driver > */ > @@ -1000,6 +1014,7 @@ static struct drm_driver simpledrm_driver = { > .minor = DRIVER_MINOR, > .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, > .fops = &simpledrm_fops, > + .get_scanout_buffer = simpledrm_get_scanout_buffer, > }; > > /*
Hi Am 12.01.24 um 14:58 schrieb Maxime Ripard: > On Fri, Jan 12, 2024 at 02:44:57PM +0100, Daniel Vetter wrote: >> On Thu, Jan 04, 2024 at 05:00:50PM +0100, Jocelyn Falempe wrote: >>> Add support for the drm_panic module, which displays a user-friendly >>> message to the screen when a kernel panic occurs. >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> --- >>> drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c >>> index 7ce1c4617675..6dd2afee84d4 100644 >>> --- a/drivers/gpu/drm/tiny/simpledrm.c >>> +++ b/drivers/gpu/drm/tiny/simpledrm.c >>> @@ -25,6 +25,7 @@ >>> #include <drm/drm_gem_shmem_helper.h> >>> #include <drm/drm_managed.h> >>> #include <drm/drm_modeset_helper_vtables.h> >>> +#include <drm/drm_panic.h> >>> #include <drm/drm_probe_helper.h> >>> >>> #define DRIVER_NAME "simpledrm" >>> @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, >>> return sdev; >>> } >>> >>> +static int simpledrm_get_scanout_buffer(struct drm_device *dev, >>> + struct drm_scanout_buffer *sb) >>> +{ >>> + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); >> >> So I guess simpledrm is the reason why the get_scanout_buffer hook is at >> the device level and not at the plane level. Even from the few drivers you >> have in your series it seems very much the exception, so I'm not sure >> whether that's the best design. >> >> I guess we'll know when we see the plane iterator code with the right >> locking, whether it's ok to have that in driver hooks or it's better to >> pull it out into shared code. > > Wouldn't the CRTC level be better than the planes? What's in favor of the CRTC level? I'd put a hook at the plane level and do the drm_for_each_primary_visible_plane() in the panic handler. Simpledrm would fit into this pattern nicely. But it's not like I have strong feeling about this. The current callbacks are simple enough. Best regards Thomas > > Maxime
On Wed, Jan 17, 2024 at 04:22:01PM +0100, Thomas Zimmermann wrote: > Hi > > Am 12.01.24 um 14:58 schrieb Maxime Ripard: > > On Fri, Jan 12, 2024 at 02:44:57PM +0100, Daniel Vetter wrote: > > > On Thu, Jan 04, 2024 at 05:00:50PM +0100, Jocelyn Falempe wrote: > > > > Add support for the drm_panic module, which displays a user-friendly > > > > message to the screen when a kernel panic occurs. > > > > > > > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > > > > --- > > > > drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > > > > index 7ce1c4617675..6dd2afee84d4 100644 > > > > --- a/drivers/gpu/drm/tiny/simpledrm.c > > > > +++ b/drivers/gpu/drm/tiny/simpledrm.c > > > > @@ -25,6 +25,7 @@ > > > > #include <drm/drm_gem_shmem_helper.h> > > > > #include <drm/drm_managed.h> > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > +#include <drm/drm_panic.h> > > > > #include <drm/drm_probe_helper.h> > > > > #define DRIVER_NAME "simpledrm" > > > > @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, > > > > return sdev; > > > > } > > > > +static int simpledrm_get_scanout_buffer(struct drm_device *dev, > > > > + struct drm_scanout_buffer *sb) > > > > +{ > > > > + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); > > > > > > So I guess simpledrm is the reason why the get_scanout_buffer hook is at > > > the device level and not at the plane level. Even from the few drivers you > > > have in your series it seems very much the exception, so I'm not sure > > > whether that's the best design. > > > > > > I guess we'll know when we see the plane iterator code with the right > > > locking, whether it's ok to have that in driver hooks or it's better to > > > pull it out into shared code. > > > > Wouldn't the CRTC level be better than the planes? > > What's in favor of the CRTC level? > > I'd put a hook at the plane level and do the > drm_for_each_primary_visible_plane() in the panic handler. Simpledrm would > fit into this pattern nicely. > > But it's not like I have strong feeling about this. The current callbacks > are simple enough. An active CRTC is guaranteed to have an active plane, and knows what the full blending story is. An active plane does have a CRTC too, but there might be other planes we want to consider, and if you're doing blending with multiple primary planes it will get quite funny very fast :) Plus, some CRTC have internal SRAMs we could use as fallback in the case where we won't be able to get a proper framebuffer. Maxime
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 7ce1c4617675..6dd2afee84d4 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -25,6 +25,7 @@ #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_panic.h> #include <drm/drm_probe_helper.h> #define DRIVER_NAME "simpledrm" @@ -985,6 +986,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, return sdev; } +static int simpledrm_get_scanout_buffer(struct drm_device *dev, + struct drm_scanout_buffer *sb) +{ + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev); + + sb->width = sdev->mode.hdisplay; + sb->height = sdev->mode.vdisplay; + sb->pitch = sdev->pitch; + sb->format = sdev->format; + sb->map = sdev->screen_base; + return 0; +} + /* * DRM driver */ @@ -1000,6 +1014,7 @@ static struct drm_driver simpledrm_driver = { .minor = DRIVER_MINOR, .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, .fops = &simpledrm_fops, + .get_scanout_buffer = simpledrm_get_scanout_buffer, }; /*
Add support for the drm_panic module, which displays a user-friendly message to the screen when a kernel panic occurs. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/tiny/simpledrm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)