On Wed, May 29, 2024 at 04:28:44PM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 12:55:06PM +0300, Laurent Pinchart wrote:
> > 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@xxxxxxxxx>
> > > > > > > ---
> > > > > > > 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) ?
>
> On contemporary MSM devices any plane can cover any part of the screen,
> including not having a plane that covers the full screen at all.
Ah, you meant picking the plane that is currently covering most of the
screen. I thought you were talking about devices where some planes were
restricted by the hardware to a subset of the CRTC.
I agree it would make sense to take both plane position and z-pos, as
well as visibility and other related parameters, to pick the plane that
is the most visible. Ideally this should be handled in drm_panic, not
duplicated in drivers.
> > 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
