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

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

[Geert Uytterhoeven]

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);
 
 	return &splane->base;
 }
-- 
2.34.1

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

[Jocelyn Falempe]

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;
>   }

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

[Laurent Pinchart]

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;
>  }

-- 
Regards,

Laurent Pinchart

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[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;
> > >  }

-- 
Regards,

Laurent Pinchart

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[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;
> > > > >  }

-- 
Regards,

Laurent Pinchart

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[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;
> > > > > > >  }

-- 
Regards,

Laurent Pinchart

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

[Jocelyn Falempe]

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,


-- 

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;
>>>>>>>>   }
> 

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

[Dmitry Baryshkov]

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;
> >>>>>>>>   }
> >
>


-- 
With best wishes
Dmitry

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

[Geert Uytterhoeven]

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

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

[Jocelyn Falempe]

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,

-- 

Jocelyn


> 
> 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
> 

Re: drm: renesas: shmobile: Add drm_panic support

[Sui Jingfeng]

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

Re: drm: renesas: shmobile: Add drm_panic support

[Geert Uytterhoeven]

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

-- 
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

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

[Maxime Ripard]

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