| Message ID | 292638fde9aef8b00e984245f43dc02a818cf322.1716816827.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: renesas: shmobile: Add drm_panic support | expand |
On 27/05/2024 15:34, Geert Uytterhoeven wrote: > Add support for the drm_panic module, which displays a message on > the screen when a kernel panic occurs. Thanks for your patch, I'm pleased that you find drm_panic useful. That looks good to me. Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> > > 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); > > return &splane->base; > }
Hi Geert, Thank you for the patch. 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. > > return &splane->base; > }
On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote: > Hi Geert, > > Thank you for the patch. > > 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. > > > > > return &splane->base; > > } > > -- > Regards, > > Laurent Pinchart
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. 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; > > > }
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. > > 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
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; > > > > > }
Hi, On 5/27/24 21:34, 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> > Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev> > --- > 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, Maybe a shmob_drm_plane_create_primary_plane() plus a shmob_drm_plane_create_overlay(). I remember Thomas told this way or something similiar, call untangle. > 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); > > return &splane->base; > } Anyway, it looks good to me. Best regards Sui
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@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) ? 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. > 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
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@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) ? > > 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; > > > > > > > }
On 29/05/2024 15:33, Laurent Pinchart wrote: > 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@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) ? >> >> 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'm not sure that drm_panic can figure out reliably on which plane it needs to draw. I think the driver has more information to take the right decision. Also if you prefer, you can add the get_scanout_buffer() callback for all planes (to use the same helper fops), and then filter out in the callback for planes that are not suitable. I just find it cleaner to not register planes that the driver knows they will never be suitable (like cursor planes). static void shmob_atomic_helper_get_scanout_buffer(struct drm_plane *plane, struct drm_scanout_buffer *sb)) { if (plane->type == DRM_PLANE_TYPE_PRIMARY) return drm_fb_dma_get_scanout_buffer(plane, sb); return -EOPNOTSUPP; } .get_scanout_buffer = shmob_atomic_helper_get_scanout_buffer,
On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfalempe@redhat.com> wrote: > > > > On 29/05/2024 15:33, Laurent Pinchart wrote: > > 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@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) ? > >> > >> 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'm not sure that drm_panic can figure out reliably on which plane it > needs to draw. I think the driver has more information to take the right > decision. I think there should be a default implementation which fits 80% of the cases (single fixed PRIMARY plane per output) but if the driver needs it, it should be able to override the behaviour. > Also if you prefer, you can add the get_scanout_buffer() callback for > all planes (to use the same helper fops), and then filter out in the > callback for planes that are not suitable. I just find it cleaner to not > register planes that the driver knows they will never be suitable (like > cursor planes). > > static void shmob_atomic_helper_get_scanout_buffer(struct drm_plane > *plane, struct drm_scanout_buffer *sb)) > { > if (plane->type == DRM_PLANE_TYPE_PRIMARY) > return drm_fb_dma_get_scanout_buffer(plane, sb); > return -EOPNOTSUPP; > } > > .get_scanout_buffer = shmob_atomic_helper_get_scanout_buffer, > > > -- > > Jocelyn > > > > >>> 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; > >>>>>>>> } > > >
Hi Sui, On Wed, May 29, 2024 at 1:31 PM Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > On 5/27/24 21:34, 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> > > Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> > > Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev> Thank you! > > --- 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, > > > Maybe a shmob_drm_plane_create_primary_plane() plus a > shmob_drm_plane_create_overlay(). > > I remember Thomas told this way or something similiar, call untangle. Hmm, that's what we had until commit c228823426ae509f ("drm: renesas: shmobile: Unify plane allocation")... > > > 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); > > > > return &splane->base; > > } > > > Anyway, it looks good to me. Thanks! Gr{oetje,eeting}s, Geert
On Thu, May 30, 2024 at 10:33 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfalempe@redhat.com> wrote: > > On 29/05/2024 15:33, Laurent Pinchart wrote: > > > 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: > > >>>>> 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) ? > > >> > > >> 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'm not sure that drm_panic can figure out reliably on which plane it > > needs to draw. I think the driver has more information to take the right > > decision. > > I think there should be a default implementation which fits 80% of the > cases (single fixed PRIMARY plane per output) but if the driver needs > it, it should be able to override the behaviour. Did anything like this materialize, or is this patch (and its rcar-du counterpart [1]) good to apply as-is? Thank you! [1] https://lore.kernel.org/r/b633568d2e3f405b21debdd60854fe39780254d6.1716816897.git.geert+renesas@glider.be/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 29/08/2024 15:53, Geert Uytterhoeven wrote: > On Thu, May 30, 2024 at 10:33 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> On Thu, 30 May 2024 at 11:16, Jocelyn Falempe <jfalempe@redhat.com> wrote: >>> On 29/05/2024 15:33, Laurent Pinchart wrote: >>>> 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: >>>>>>>> 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) ? >>>>> >>>>> 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'm not sure that drm_panic can figure out reliably on which plane it >>> needs to draw. I think the driver has more information to take the right >>> decision. >> >> I think there should be a default implementation which fits 80% of the >> cases (single fixed PRIMARY plane per output) but if the driver needs >> it, it should be able to override the behaviour. > > Did anything like this materialize, or is this patch (and its rcar-du > counterpart [1]) good to apply as-is? No I didn't find a better option yet, and I think it is good as-is. If duplicating the helper funcs is really a problem, I think the less intrusive option, would be to add a "bool panic_allow_overlay_plane" to struct drm_mode_config, and use that in drm_panic_register(), to register only primary planes if it's not set ? So drivers that want to draw the panic on overlay plane can opt-in. Best regards,
On Mon, 27 May 2024 15:34:48 +0200, Geert Uytterhoeven wrote: > Add support for the drm_panic module, which displays a message on > the screen when a kernel panic occurs. > > Applied to misc/kernel.git (drm-misc-next). Thanks! Maxime
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); return &splane->base; }
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(-)