[RFC PATCH 0/1] meson/drm: Support drm_panic

[RFC PATCH 0/1] meson/drm: Support drm_panic

[Yao Zi]

This patch adds drm_panic support for meson-drm, has been tested on
A311D with both TTY and Wayland session.

It is an RFC since I am not sure whether AFBC enabled case is handled
properly and don't find a good test case. Thanks for your time and
advice.

Yao Zi (1):
  drm/meson: Support drm_panic

 drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

-- 
2.46.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH 0/1] meson/drm: Support drm_panic

[Yao Zi]

This patch adds drm_panic support for meson-drm, has been tested on
A311D with both TTY and Wayland session.

It is an RFC since I am not sure whether AFBC enabled case is handled
properly and don't find a good test case. Thanks for your time and
advice.

Yao Zi (1):
  drm/meson: Support drm_panic

 drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

-- 
2.46.0

[RFC PATCH 1/1] drm/meson: Support drm_panic

[Yao Zi]

This patch implements drm_plane_helper_funcs.get_scanout_buffer for
primary plane, enabling meson-drm to work with drm_panic.

This implementation tries to use current framebuffer as scanout buffer.
In case of AFBC enabled, we disable the decoder path and adjust OSD1
parameters in get_scanout_buffer to make the buffer linear.

Tested on TTY and Wayland session (Sway).

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index b43ac61201f3..b2def784c00d 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -20,6 +20,8 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_panic.h>
 
 #include "meson_plane.h"
 #include "meson_registers.h"
@@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
 	priv->viu.osd1_enabled = false;
 }
 
+static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
+					  struct drm_scanout_buffer *sb)
+{
+	struct meson_plane *meson_plane = to_meson_plane(plane);
+	struct meson_drm *priv = meson_plane->priv;
+	struct drm_framebuffer *fb;
+
+	if (!meson_plane->enabled)
+		return -ENODEV;
+
+	if (priv->viu.osd1_afbcd) {
+		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
+			writel_relaxed(0, priv->io_base +
+					  _REG(VIU_OSD1_BLK1_CFG_W4));
+			writel_relaxed(0, priv->io_base +
+					  _REG(VIU_OSD1_BLK2_CFG_W4));
+			writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
+					    priv->io_base +
+					    _REG(VIU_OSD1_BLK0_CFG_W0));
+			meson_viu_g12a_disable_osd1_afbc(priv);
+		} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
+			writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
+					    priv->io_base +
+					    _REG(VIU_OSD1_CTRL_STAT2));
+			meson_viu_gxm_disable_osd1_afbc(priv);
+		}
+	}
+
+	fb = plane->state->fb;
+	sb->format	= fb->format;
+	sb->width	= fb->width;
+	sb->height	= fb->height;
+	sb->pitch[0]	= fb->pitches[0];
+	drm_gem_fb_vmap(fb, sb->map, NULL);
+
+	return 0;
+}
+
 static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
-	.atomic_check	= meson_plane_atomic_check,
-	.atomic_disable	= meson_plane_atomic_disable,
-	.atomic_update	= meson_plane_atomic_update,
+	.atomic_check		= meson_plane_atomic_check,
+	.atomic_disable		= meson_plane_atomic_disable,
+	.atomic_update		= meson_plane_atomic_update,
+	.get_scanout_buffer	= meson_plane_get_scanout_buffer,
 };
 
 static bool meson_plane_format_mod_supported(struct drm_plane *plane,
-- 
2.46.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[RFC PATCH 1/1] drm/meson: Support drm_panic

[Yao Zi]

This patch implements drm_plane_helper_funcs.get_scanout_buffer for
primary plane, enabling meson-drm to work with drm_panic.

This implementation tries to use current framebuffer as scanout buffer.
In case of AFBC enabled, we disable the decoder path and adjust OSD1
parameters in get_scanout_buffer to make the buffer linear.

Tested on TTY and Wayland session (Sway).

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index b43ac61201f3..b2def784c00d 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -20,6 +20,8 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_panic.h>
 
 #include "meson_plane.h"
 #include "meson_registers.h"
@@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
 	priv->viu.osd1_enabled = false;
 }
 
+static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
+					  struct drm_scanout_buffer *sb)
+{
+	struct meson_plane *meson_plane = to_meson_plane(plane);
+	struct meson_drm *priv = meson_plane->priv;
+	struct drm_framebuffer *fb;
+
+	if (!meson_plane->enabled)
+		return -ENODEV;
+
+	if (priv->viu.osd1_afbcd) {
+		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
+			writel_relaxed(0, priv->io_base +
+					  _REG(VIU_OSD1_BLK1_CFG_W4));
+			writel_relaxed(0, priv->io_base +
+					  _REG(VIU_OSD1_BLK2_CFG_W4));
+			writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
+					    priv->io_base +
+					    _REG(VIU_OSD1_BLK0_CFG_W0));
+			meson_viu_g12a_disable_osd1_afbc(priv);
+		} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
+			writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
+					    priv->io_base +
+					    _REG(VIU_OSD1_CTRL_STAT2));
+			meson_viu_gxm_disable_osd1_afbc(priv);
+		}
+	}
+
+	fb = plane->state->fb;
+	sb->format	= fb->format;
+	sb->width	= fb->width;
+	sb->height	= fb->height;
+	sb->pitch[0]	= fb->pitches[0];
+	drm_gem_fb_vmap(fb, sb->map, NULL);
+
+	return 0;
+}
+
 static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
-	.atomic_check	= meson_plane_atomic_check,
-	.atomic_disable	= meson_plane_atomic_disable,
-	.atomic_update	= meson_plane_atomic_update,
+	.atomic_check		= meson_plane_atomic_check,
+	.atomic_disable		= meson_plane_atomic_disable,
+	.atomic_update		= meson_plane_atomic_update,
+	.get_scanout_buffer	= meson_plane_get_scanout_buffer,
 };
 
 static bool meson_plane_format_mod_supported(struct drm_plane *plane,
-- 
2.46.0

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Neil Armstrong]

Hi !

On 01/10/2024 23:04, Yao Zi wrote:
> This patch implements drm_plane_helper_funcs.get_scanout_buffer for
> primary plane, enabling meson-drm to work with drm_panic.
> 
> This implementation tries to use current framebuffer as scanout buffer.
> In case of AFBC enabled, we disable the decoder path and adjust OSD1
> parameters in get_scanout_buffer to make the buffer linear.
> 
> Tested on TTY and Wayland session (Sway).

Thanks for enabling this!

> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>   drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
>   1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index b43ac61201f3..b2def784c00d 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -20,6 +20,8 @@
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_panic.h>
>   
>   #include "meson_plane.h"
>   #include "meson_registers.h"
> @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
>   	priv->viu.osd1_enabled = false;
>   }
>   
> +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
> +					  struct drm_scanout_buffer *sb)
> +{
> +	struct meson_plane *meson_plane = to_meson_plane(plane);
> +	struct meson_drm *priv = meson_plane->priv;
> +	struct drm_framebuffer *fb;
> +
> +	if (!meson_plane->enabled)
> +		return -ENODEV;
> +
> +	if (priv->viu.osd1_afbcd) {
> +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {

This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)

You should call:

			if (priv->afbcd.ops) {
				priv->afbcd.ops->reset(priv);
				priv->afbcd.ops->disable(priv);
			}

> +			writel_relaxed(0, priv->io_base +
> +					  _REG(VIU_OSD1_BLK1_CFG_W4));
> +			writel_relaxed(0, priv->io_base +
> +					  _REG(VIU_OSD1_BLK2_CFG_W4));
> +			writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
> +					    priv->io_base +
> +					    _REG(VIU_OSD1_BLK0_CFG_W0));

This won't work, drop it, the canvas isn't correctly configured, you should instead call:
			meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
					    priv->viu.osd1_addr,
					    priv->viu.osd1_stride,
					    priv->viu.osd1_height,
					    MESON_CANVAS_WRAP_NONE,
					    MESON_CANVAS_BLKMODE_LINEAR, 0);

> +			meson_viu_g12a_disable_osd1_afbc(priv);
> +		} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {

And here meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)

> +			writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
> +					    priv->io_base +
> +					    _REG(VIU_OSD1_CTRL_STAT2));

Ok, you should also call meson_canvas_config()

You should call:

			if (priv->afbcd.ops) {
				priv->afbcd.ops->reset(priv);
				priv->afbcd.ops->disable(priv);
			}

> +			meson_viu_gxm_disable_osd1_afbc(priv);
> +		}
> +	}

I thing the code should look like:

if (priv->viu.osd1_afbcd) {
	meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
			    priv->viu.osd1_addr,
			    priv->viu.osd1_stride,
			    priv->viu.osd1_height,
			    MESON_CANVAS_WRAP_NONE,
			    MESON_CANVAS_BLKMODE_LINEAR, 0);

	if (priv->afbcd.ops) {
		priv->afbcd.ops->reset(priv);
		priv->afbcd.ops->disable(priv);
	}

	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
		writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
				    priv->io_base +
				    _REG(VIU_OSD1_BLK0_CFG_W0));
		meson_viu_g12a_disable_osd1_afbc(priv);
	} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
		writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
				    priv->io_base +
				    _REG(VIU_OSD1_CTRL_STAT2));
		meson_viu_gxm_disable_osd1_afbc(priv);
	}
}

AFBC is quite hard to test since it requires DRM_FORMAT_XBGR8888, but
I think sway should perhaps support it, Mesa should also support AFBC.

At some point I made some memory dumps of AFBC buffers, perhaps they could
be useful here.

Another way would be to simply ignore the AFBC case, and bail out since
it would be a very rare case.

> +
> +	fb = plane->state->fb;
> +	sb->format	= fb->format;
> +	sb->width	= fb->width;
> +	sb->height	= fb->height;
> +	sb->pitch[0]	= fb->pitches[0];
> +	drm_gem_fb_vmap(fb, sb->map, NULL);
> +
> +	return 0;
> +}
> +
>   static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
> -	.atomic_check	= meson_plane_atomic_check,
> -	.atomic_disable	= meson_plane_atomic_disable,
> -	.atomic_update	= meson_plane_atomic_update,
> +	.atomic_check		= meson_plane_atomic_check,
> +	.atomic_disable		= meson_plane_atomic_disable,
> +	.atomic_update		= meson_plane_atomic_update,
> +	.get_scanout_buffer	= meson_plane_get_scanout_buffer,
>   };
>   
>   static bool meson_plane_format_mod_supported(struct drm_plane *plane,

Thanks,
Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Neil Armstrong]

Hi !

On 01/10/2024 23:04, Yao Zi wrote:
> This patch implements drm_plane_helper_funcs.get_scanout_buffer for
> primary plane, enabling meson-drm to work with drm_panic.
> 
> This implementation tries to use current framebuffer as scanout buffer.
> In case of AFBC enabled, we disable the decoder path and adjust OSD1
> parameters in get_scanout_buffer to make the buffer linear.
> 
> Tested on TTY and Wayland session (Sway).

Thanks for enabling this!

> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>   drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
>   1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index b43ac61201f3..b2def784c00d 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -20,6 +20,8 @@
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_panic.h>
>   
>   #include "meson_plane.h"
>   #include "meson_registers.h"
> @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
>   	priv->viu.osd1_enabled = false;
>   }
>   
> +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
> +					  struct drm_scanout_buffer *sb)
> +{
> +	struct meson_plane *meson_plane = to_meson_plane(plane);
> +	struct meson_drm *priv = meson_plane->priv;
> +	struct drm_framebuffer *fb;
> +
> +	if (!meson_plane->enabled)
> +		return -ENODEV;
> +
> +	if (priv->viu.osd1_afbcd) {
> +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {

This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)

You should call:

			if (priv->afbcd.ops) {
				priv->afbcd.ops->reset(priv);
				priv->afbcd.ops->disable(priv);
			}

> +			writel_relaxed(0, priv->io_base +
> +					  _REG(VIU_OSD1_BLK1_CFG_W4));
> +			writel_relaxed(0, priv->io_base +
> +					  _REG(VIU_OSD1_BLK2_CFG_W4));
> +			writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
> +					    priv->io_base +
> +					    _REG(VIU_OSD1_BLK0_CFG_W0));

This won't work, drop it, the canvas isn't correctly configured, you should instead call:
			meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
					    priv->viu.osd1_addr,
					    priv->viu.osd1_stride,
					    priv->viu.osd1_height,
					    MESON_CANVAS_WRAP_NONE,
					    MESON_CANVAS_BLKMODE_LINEAR, 0);

> +			meson_viu_g12a_disable_osd1_afbc(priv);
> +		} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {

And here meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)

> +			writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
> +					    priv->io_base +
> +					    _REG(VIU_OSD1_CTRL_STAT2));

Ok, you should also call meson_canvas_config()

You should call:

			if (priv->afbcd.ops) {
				priv->afbcd.ops->reset(priv);
				priv->afbcd.ops->disable(priv);
			}

> +			meson_viu_gxm_disable_osd1_afbc(priv);
> +		}
> +	}

I thing the code should look like:

if (priv->viu.osd1_afbcd) {
	meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
			    priv->viu.osd1_addr,
			    priv->viu.osd1_stride,
			    priv->viu.osd1_height,
			    MESON_CANVAS_WRAP_NONE,
			    MESON_CANVAS_BLKMODE_LINEAR, 0);

	if (priv->afbcd.ops) {
		priv->afbcd.ops->reset(priv);
		priv->afbcd.ops->disable(priv);
	}

	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
		writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
				    priv->io_base +
				    _REG(VIU_OSD1_BLK0_CFG_W0));
		meson_viu_g12a_disable_osd1_afbc(priv);
	} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
		writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
				    priv->io_base +
				    _REG(VIU_OSD1_CTRL_STAT2));
		meson_viu_gxm_disable_osd1_afbc(priv);
	}
}

AFBC is quite hard to test since it requires DRM_FORMAT_XBGR8888, but
I think sway should perhaps support it, Mesa should also support AFBC.

At some point I made some memory dumps of AFBC buffers, perhaps they could
be useful here.

Another way would be to simply ignore the AFBC case, and bail out since
it would be a very rare case.

> +
> +	fb = plane->state->fb;
> +	sb->format	= fb->format;
> +	sb->width	= fb->width;
> +	sb->height	= fb->height;
> +	sb->pitch[0]	= fb->pitches[0];
> +	drm_gem_fb_vmap(fb, sb->map, NULL);
> +
> +	return 0;
> +}
> +
>   static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
> -	.atomic_check	= meson_plane_atomic_check,
> -	.atomic_disable	= meson_plane_atomic_disable,
> -	.atomic_update	= meson_plane_atomic_update,
> +	.atomic_check		= meson_plane_atomic_check,
> +	.atomic_disable		= meson_plane_atomic_disable,
> +	.atomic_update		= meson_plane_atomic_update,
> +	.get_scanout_buffer	= meson_plane_get_scanout_buffer,
>   };
>   
>   static bool meson_plane_format_mod_supported(struct drm_plane *plane,

Thanks,
Neil

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Yao Zi]

On Wed, Oct 02, 2024 at 09:59:57AM +0200, Neil Armstrong wrote:
> I thing the code should look like:
> 
> if (priv->viu.osd1_afbcd) {
> 	meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
> 			    priv->viu.osd1_addr,
> 			    priv->viu.osd1_stride,
> 			    priv->viu.osd1_height,
> 			    MESON_CANVAS_WRAP_NONE,
> 			    MESON_CANVAS_BLKMODE_LINEAR, 0);
> 
> 	if (priv->afbcd.ops) {
> 		priv->afbcd.ops->reset(priv);
> 		priv->afbcd.ops->disable(priv);
> 	}
> 
> 	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> 		writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
> 				    priv->io_base +
> 				    _REG(VIU_OSD1_BLK0_CFG_W0));
> 		meson_viu_g12a_disable_osd1_afbc(priv);
> 	} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> 		writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
> 				    priv->io_base +
> 				    _REG(VIU_OSD1_CTRL_STAT2));
> 		meson_viu_gxm_disable_osd1_afbc(priv);
> 	}
> }

Thanks for correction!

> AFBC is quite hard to test since it requires DRM_FORMAT_XBGR8888, but
> I think sway should perhaps support it, Mesa should also support AFBC.

Have tried with Sway 1.9 and weston 14.0.0 and didn't find a way to make
them create buffers with AFBC modifiers. modetest util in libdrm doesn't
support it either.

> At some point I made some memory dumps of AFBC buffers, perhaps they could
> be useful here.
> 
> Another way would be to simply ignore the AFBC case, and bail out since
> it would be a very rare case.

I'm not sure the use case of an AFBC-enabled primary plane. It should be
rare the whole primary plane is filled with pixel data from GPU or video
codec.

If my guess is true, bailing out when AFBC is enabled should be
acceptable. Will try to do some AFBC tests if possible and consider
it as a latest solution.

btw, I forget to check whether drm_gem_fb_vmap() succeeds. Will fix it
later.

Thanks for your advice again!

Best regards,
Yao Zi

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Yao Zi]

On Wed, Oct 02, 2024 at 09:59:57AM +0200, Neil Armstrong wrote:
> I thing the code should look like:
> 
> if (priv->viu.osd1_afbcd) {
> 	meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
> 			    priv->viu.osd1_addr,
> 			    priv->viu.osd1_stride,
> 			    priv->viu.osd1_height,
> 			    MESON_CANVAS_WRAP_NONE,
> 			    MESON_CANVAS_BLKMODE_LINEAR, 0);
> 
> 	if (priv->afbcd.ops) {
> 		priv->afbcd.ops->reset(priv);
> 		priv->afbcd.ops->disable(priv);
> 	}
> 
> 	if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> 		writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
> 				    priv->io_base +
> 				    _REG(VIU_OSD1_BLK0_CFG_W0));
> 		meson_viu_g12a_disable_osd1_afbc(priv);
> 	} else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> 		writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
> 				    priv->io_base +
> 				    _REG(VIU_OSD1_CTRL_STAT2));
> 		meson_viu_gxm_disable_osd1_afbc(priv);
> 	}
> }

Thanks for correction!

> AFBC is quite hard to test since it requires DRM_FORMAT_XBGR8888, but
> I think sway should perhaps support it, Mesa should also support AFBC.

Have tried with Sway 1.9 and weston 14.0.0 and didn't find a way to make
them create buffers with AFBC modifiers. modetest util in libdrm doesn't
support it either.

> At some point I made some memory dumps of AFBC buffers, perhaps they could
> be useful here.
> 
> Another way would be to simply ignore the AFBC case, and bail out since
> it would be a very rare case.

I'm not sure the use case of an AFBC-enabled primary plane. It should be
rare the whole primary plane is filled with pixel data from GPU or video
codec.

If my guess is true, bailing out when AFBC is enabled should be
acceptable. Will try to do some AFBC tests if possible and consider
it as a latest solution.

btw, I forget to check whether drm_gem_fb_vmap() succeeds. Will fix it
later.

Thanks for your advice again!

Best regards,
Yao Zi

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1805 bytes --]

+ Jocelyn

On Wed, Oct 02, 2024 at 09:59:57AM GMT, Neil Armstrong wrote:
> > diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> > index b43ac61201f3..b2def784c00d 100644
> > --- a/drivers/gpu/drm/meson/meson_plane.c
> > +++ b/drivers/gpu/drm/meson/meson_plane.c
> > @@ -20,6 +20,8 @@
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_gem_atomic_helper.h>
> >   #include <drm/drm_gem_dma_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_panic.h>
> >   #include "meson_plane.h"
> >   #include "meson_registers.h"
> > @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
> >   	priv->viu.osd1_enabled = false;
> >   }
> > +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
> > +					  struct drm_scanout_buffer *sb)
> > +{
> > +	struct meson_plane *meson_plane = to_meson_plane(plane);
> > +	struct meson_drm *priv = meson_plane->priv;
> > +	struct drm_framebuffer *fb;
> > +
> > +	if (!meson_plane->enabled)
> > +		return -ENODEV;
> > +
> > +	if (priv->viu.osd1_afbcd) {
> > +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> 
> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
> 
> You should call:
> 
> 			if (priv->afbcd.ops) {
> 				priv->afbcd.ops->reset(priv);
> 				priv->afbcd.ops->disable(priv);
> 			}

I'm not sure it's a good idea. This code is run in the panic path, and
it comes with strict requirements that these functions don't have.

It'll be incredibly easy to add a sleeping call or a lock in there.

On a more fundamental level, I'm not sure we should be disableing AFBC
at all. Do we even have the guarantee that the buffer is large enough to
hold XRGB8888 pixels?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

+ Jocelyn

On Wed, Oct 02, 2024 at 09:59:57AM GMT, Neil Armstrong wrote:
> > diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> > index b43ac61201f3..b2def784c00d 100644
> > --- a/drivers/gpu/drm/meson/meson_plane.c
> > +++ b/drivers/gpu/drm/meson/meson_plane.c
> > @@ -20,6 +20,8 @@
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_gem_atomic_helper.h>
> >   #include <drm/drm_gem_dma_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_panic.h>
> >   #include "meson_plane.h"
> >   #include "meson_registers.h"
> > @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
> >   	priv->viu.osd1_enabled = false;
> >   }
> > +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
> > +					  struct drm_scanout_buffer *sb)
> > +{
> > +	struct meson_plane *meson_plane = to_meson_plane(plane);
> > +	struct meson_drm *priv = meson_plane->priv;
> > +	struct drm_framebuffer *fb;
> > +
> > +	if (!meson_plane->enabled)
> > +		return -ENODEV;
> > +
> > +	if (priv->viu.osd1_afbcd) {
> > +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> 
> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
> 
> You should call:
> 
> 			if (priv->afbcd.ops) {
> 				priv->afbcd.ops->reset(priv);
> 				priv->afbcd.ops->disable(priv);
> 			}

I'm not sure it's a good idea. This code is run in the panic path, and
it comes with strict requirements that these functions don't have.

It'll be incredibly easy to add a sleeping call or a lock in there.

On a more fundamental level, I'm not sure we should be disableing AFBC
at all. Do we even have the guarantee that the buffer is large enough to
hold XRGB8888 pixels?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Neil Armstrong]

Hi,

On 02/10/2024 13:02, Maxime Ripard wrote:
> + Jocelyn
> 
> On Wed, Oct 02, 2024 at 09:59:57AM GMT, Neil Armstrong wrote:
>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>>> index b43ac61201f3..b2def784c00d 100644
>>> --- a/drivers/gpu/drm/meson/meson_plane.c
>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>>> @@ -20,6 +20,8 @@
>>>    #include <drm/drm_framebuffer.h>
>>>    #include <drm/drm_gem_atomic_helper.h>
>>>    #include <drm/drm_gem_dma_helper.h>
>>> +#include <drm/drm_gem_framebuffer_helper.h>
>>> +#include <drm/drm_panic.h>
>>>    #include "meson_plane.h"
>>>    #include "meson_registers.h"
>>> @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
>>>    	priv->viu.osd1_enabled = false;
>>>    }
>>> +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
>>> +					  struct drm_scanout_buffer *sb)
>>> +{
>>> +	struct meson_plane *meson_plane = to_meson_plane(plane);
>>> +	struct meson_drm *priv = meson_plane->priv;
>>> +	struct drm_framebuffer *fb;
>>> +
>>> +	if (!meson_plane->enabled)
>>> +		return -ENODEV;
>>> +
>>> +	if (priv->viu.osd1_afbcd) {
>>> +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
>>
>> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
>>
>> You should call:
>>
>> 			if (priv->afbcd.ops) {
>> 				priv->afbcd.ops->reset(priv);
>> 				priv->afbcd.ops->disable(priv);
>> 			}
> 
> I'm not sure it's a good idea. This code is run in the panic path, and
> it comes with strict requirements that these functions don't have.
> 
> It'll be incredibly easy to add a sleeping call or a lock in there.
> 
> On a more fundamental level, I'm not sure we should be disableing AFBC
> at all. Do we even have the guarantee that the buffer is large enough to
> hold XRGB8888 pixels?

Yes it does, "compressed" is in he way pixels are ordered in memory, meaning
it will be faster to scanout when the image is simple, but with a fully random
image the buffer will be larger.

But per my comment, AFBC is really only used on Android producst since it requires
DRM_FORMAT_XBGR8888, so the best is to bail out when AFBC is enabled.

Neil

> 
> Maxime


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Neil Armstrong]

Hi,

On 02/10/2024 13:02, Maxime Ripard wrote:
> + Jocelyn
> 
> On Wed, Oct 02, 2024 at 09:59:57AM GMT, Neil Armstrong wrote:
>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>>> index b43ac61201f3..b2def784c00d 100644
>>> --- a/drivers/gpu/drm/meson/meson_plane.c
>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>>> @@ -20,6 +20,8 @@
>>>    #include <drm/drm_framebuffer.h>
>>>    #include <drm/drm_gem_atomic_helper.h>
>>>    #include <drm/drm_gem_dma_helper.h>
>>> +#include <drm/drm_gem_framebuffer_helper.h>
>>> +#include <drm/drm_panic.h>
>>>    #include "meson_plane.h"
>>>    #include "meson_registers.h"
>>> @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct drm_plane *plane,
>>>    	priv->viu.osd1_enabled = false;
>>>    }
>>> +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
>>> +					  struct drm_scanout_buffer *sb)
>>> +{
>>> +	struct meson_plane *meson_plane = to_meson_plane(plane);
>>> +	struct meson_drm *priv = meson_plane->priv;
>>> +	struct drm_framebuffer *fb;
>>> +
>>> +	if (!meson_plane->enabled)
>>> +		return -ENODEV;
>>> +
>>> +	if (priv->viu.osd1_afbcd) {
>>> +		if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
>>
>> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
>>
>> You should call:
>>
>> 			if (priv->afbcd.ops) {
>> 				priv->afbcd.ops->reset(priv);
>> 				priv->afbcd.ops->disable(priv);
>> 			}
> 
> I'm not sure it's a good idea. This code is run in the panic path, and
> it comes with strict requirements that these functions don't have.
> 
> It'll be incredibly easy to add a sleeping call or a lock in there.
> 
> On a more fundamental level, I'm not sure we should be disableing AFBC
> at all. Do we even have the guarantee that the buffer is large enough to
> hold XRGB8888 pixels?

Yes it does, "compressed" is in he way pixels are ordered in memory, meaning
it will be faster to scanout when the image is simple, but with a fully random
image the buffer will be larger.

But per my comment, AFBC is really only used on Android producst since it requires
DRM_FORMAT_XBGR8888, so the best is to bail out when AFBC is enabled.

Neil

> 
> Maxime

Re: [RFC PATCH 1/1] drm/meson: Support drm_panic

[Jocelyn Falempe]

On 02/10/2024 09:59, Neil Armstrong wrote:
> Hi !
> 
> On 01/10/2024 23:04, Yao Zi wrote:
>> This patch implements drm_plane_helper_funcs.get_scanout_buffer for
>> primary plane, enabling meson-drm to work with drm_panic.
>>
>> This implementation tries to use current framebuffer as scanout buffer.
>> In case of AFBC enabled, we disable the decoder path and adjust OSD1
>> parameters in get_scanout_buffer to make the buffer linear.
>>
>> Tested on TTY and Wayland session (Sway).
> 
> Thanks for enabling this!
> 
>>
>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>> ---
>>   drivers/gpu/drm/meson/meson_plane.c | 47 +++++++++++++++++++++++++++--
>>   1 file changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/ 
>> meson/meson_plane.c
>> index b43ac61201f3..b2def784c00d 100644
>> --- a/drivers/gpu/drm/meson/meson_plane.c
>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>> @@ -20,6 +20,8 @@
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_gem_dma_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_panic.h>
>>   #include "meson_plane.h"
>>   #include "meson_registers.h"
>> @@ -419,10 +421,49 @@ static void meson_plane_atomic_disable(struct 
>> drm_plane *plane,
>>       priv->viu.osd1_enabled = false;
>>   }
>> +static int meson_plane_get_scanout_buffer(struct drm_plane *plane,
>> +                      struct drm_scanout_buffer *sb)
>> +{
>> +    struct meson_plane *meson_plane = to_meson_plane(plane);
>> +    struct meson_drm *priv = meson_plane->priv;
>> +    struct drm_framebuffer *fb;
>> +
>> +    if (!meson_plane->enabled)
>> +        return -ENODEV;
>> +
>> +    if (priv->viu.osd1_afbcd) {
>> +        if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> 
> This should be meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)
> 
> You should call:
> 
>              if (priv->afbcd.ops) {
>                  priv->afbcd.ops->reset(priv);
>                  priv->afbcd.ops->disable(priv);
>              }
> 
>> +            writel_relaxed(0, priv->io_base +
>> +                      _REG(VIU_OSD1_BLK1_CFG_W4));
>> +            writel_relaxed(0, priv->io_base +
>> +                      _REG(VIU_OSD1_BLK2_CFG_W4));
>> +            writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
>> +                        priv->io_base +
>> +                        _REG(VIU_OSD1_BLK0_CFG_W0));
> 
> This won't work, drop it, the canvas isn't correctly configured, you 
> should instead call:
>              meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
>                          priv->viu.osd1_addr,
>                          priv->viu.osd1_stride,
>                          priv->viu.osd1_height,
>                          MESON_CANVAS_WRAP_NONE,
>                          MESON_CANVAS_BLKMODE_LINEAR, 0);

The problem is meson_canvas_config() takes a spin_lock, which can block 
forever in a panic handler.
So you can rewrite a version without lock.
Considering the addr, stride, and height don't change in a panic, it can 
probably be simplified for this case.

Also this will be the last image to be drawn, and the last driver code 
to be executed before a reboot, so it's ok if it's a bit dirty, and if 
the software state is not up-to-date with the hardware state.

> 
>> +            meson_viu_g12a_disable_osd1_afbc(priv);
>> +        } else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> 
> And here meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)
> 
>> +            writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
>> +                        priv->io_base +
>> +                        _REG(VIU_OSD1_CTRL_STAT2));
> 
> Ok, you should also call meson_canvas_config()
> 
> You should call:
> 
>              if (priv->afbcd.ops) {
>                  priv->afbcd.ops->reset(priv);
>                  priv->afbcd.ops->disable(priv);
>              }
> 
>> +            meson_viu_gxm_disable_osd1_afbc(priv);
>> +        }
>> +    }
> 
> I thing the code should look like:
> 
> if (priv->viu.osd1_afbcd) {
>      meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
>                  priv->viu.osd1_addr,
>                  priv->viu.osd1_stride,
>                  priv->viu.osd1_height,
>                  MESON_CANVAS_WRAP_NONE,
>                  MESON_CANVAS_BLKMODE_LINEAR, 0);
> 
>      if (priv->afbcd.ops) {
>          priv->afbcd.ops->reset(priv);
>          priv->afbcd.ops->disable(priv);
>      }
> 
>      if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>          writel_bits_relaxed(OSD_ENDIANNESS_LE, OSD_ENDIANNESS_LE,
>                      priv->io_base +
>                      _REG(VIU_OSD1_BLK0_CFG_W0));
>          meson_viu_g12a_disable_osd1_afbc(priv);
>      } else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
>          writel_bits_relaxed(OSD_DPATH_MALI_AFBCD, 0,
>                      priv->io_base +
>                      _REG(VIU_OSD1_CTRL_STAT2));
>          meson_viu_gxm_disable_osd1_afbc(priv);
>      }
> }
> 
> AFBC is quite hard to test since it requires DRM_FORMAT_XBGR8888, but
> I think sway should perhaps support it, Mesa should also support AFBC.
> 
> At some point I made some memory dumps of AFBC buffers, perhaps they could
> be useful here.
> 
> Another way would be to simply ignore the AFBC case, and bail out since
> it would be a very rare case.

panic handler is a best effort mode, so if it's too complex to support 
AFBC, and it's not very widespread, then it's ok to bail out in this case.
> 
>> +
>> +    fb = plane->state->fb;
>> +    sb->format    = fb->format;
>> +    sb->width    = fb->width;
>> +    sb->height    = fb->height;
>> +    sb->pitch[0]    = fb->pitches[0];
>> +    drm_gem_fb_vmap(fb, sb->map, NULL);
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
>> -    .atomic_check    = meson_plane_atomic_check,
>> -    .atomic_disable    = meson_plane_atomic_disable,
>> -    .atomic_update    = meson_plane_atomic_update,
>> +    .atomic_check        = meson_plane_atomic_check,
>> +    .atomic_disable        = meson_plane_atomic_disable,
>> +    .atomic_update        = meson_plane_atomic_update,
>> +    .get_scanout_buffer    = meson_plane_get_scanout_buffer,
>>   };
>>   static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> 
> Thanks,
> Neil
> 

Best regards,

-- 

Jocelyn