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@xxxxxxxxxx>
> > > > ---
> > > > 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
Attachment:
signature.asc
Description: PGP signature
