Re: [PATCH] drm: renesas: shmobile: Add drm_panic support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, May 29, 2024 at 12:25:56PM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 12:10:18PM +0300, Laurent Pinchart wrote:
> > Hi Dmitry,
> > 
> > On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote:
> > > On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote:
> > > > On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
> > > > > Add support for the drm_panic module, which displays a message on
> > > > > the screen when a kernel panic occurs.
> > > > > 
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > ---
> > > > > Tested on Armadillo-800-EVA.
> > > > > ---
> > > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +++++++++++++-
> > > > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > index 07ad17d24294d5e6..9d166ab2af8bd231 100644
> > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs shmob_drm_plane_helper_funcs = {
> > > > >  	.atomic_disable = shmob_drm_plane_atomic_disable,
> > > > >  };
> > > > >  
> > > > > +static const struct drm_plane_helper_funcs shmob_drm_primary_plane_helper_funcs = {
> > > > > +	.atomic_check = shmob_drm_plane_atomic_check,
> > > > > +	.atomic_update = shmob_drm_plane_atomic_update,
> > > > > +	.atomic_disable = shmob_drm_plane_atomic_disable,
> > > > > +	.get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> > > > > +};
> > > > > +
> > > > >  static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> > > > >  	.update_plane = drm_atomic_helper_update_plane,
> > > > >  	.disable_plane = drm_atomic_helper_disable_plane,
> > > > > @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev,
> > > > >  
> > > > >  	splane->index = index;
> > > > >  
> > > > > -	drm_plane_helper_add(&splane->base, &shmob_drm_plane_helper_funcs);
> > > > > +	if (type == DRM_PLANE_TYPE_PRIMARY)
> > > > > +		drm_plane_helper_add(&splane->base,
> > > > > +				     &shmob_drm_primary_plane_helper_funcs);
> > > > > +	else
> > > > > +		drm_plane_helper_add(&splane->base,
> > > > > +				     &shmob_drm_plane_helper_funcs);
> > > > 
> > > > It's not very nice to have to provide different operations for the
> > > > primary and overlay planes. The documentation of
> > > > drm_fb_dma_get_scanout_buffer() states
> > > > 
> > > >  * @plane: DRM primary plane
> > > > 
> > > > If the intent is that only primary planes will be used to display the
> > > > panic message, shouldn't drm_panic_register() skip overlay planes ? It
> > > > would simplify drivers.
> > > 
> > > What about the drivers where all the planes are actually universal?
> > > In such a case the planes registered as primary can easily get replaced
> > > by 'overlay' planes.
> > 
> > Good point.
> > 
> > Another option, if we wanted to avoid duplicating the drm_plane_funcs,
> > would be to add a field to drm_plane to indicate whether the plane is
> > suitable for drm_panic.
> 
> ... or maybe let the driver decide. For the fully-universal-plane
> devices we probably want to select the planes which cover the largest
> part of the CRTC.

Are there devices where certain planes can only cover a subset of the
CRTC (apart from planes meant for cursors) ?

I think that what would matter the most in the end is selecting the
plane that is on top of the stack, and that doesn't seem to be addressed
by the drm_panic infrastructure. This is getting out of scope for this
patch though :-)

> > I don't think this patch should be blocked just for this reason, but I'm
> > a bit bothered by duplicating the ops structure to indicate drm_panic
> > support.
> > 
> > > > >  
> > > > >  	return &splane->base;
> > > > >  }

-- 
Regards,

Laurent Pinchart