[RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
.atomic_check() callback") moved the allocation of the intermediate and
HW buffers from the encoder's .atomic_enable callback to primary plane's
.atomic_check callback.

This was suggested by Maxime Ripard because drivers aren't allowed to fail
after drm_atomic_helper_swap_state() has been called, and the encoder's
.atomic_enable happens after the new atomic state has been swapped.

But that change caused a performance regression in very slow platforms,
since now the allocation happens for every plane's atomic state commit.
For example, Geert Uytterhoeven reports that is the case on a VexRiscV
softcore (RISC-V CPU implementation on an FPGA).

To prevent that, move the move the buffers' allocation and free to the
CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
happens on a modeset. Since the intermediate buffer is only needed when
not using the controller native format (R1), doing the buffer allocation
at that CRTC's .atomic_check time would be enough.

Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
Hello,

This is a RFC because I'm not sure if there is a net benefit after this
change. I find the currect code much cleaner and less error prone, even
when Geert reports that performs worse on his (very slow) platform.

But I'm still posting it to see what others think. I've tested the patch
on an I2C ssd1306 OLED and found no regressions.

The patch is on top on Geert's latest patch-set that adds support for the
DRM_FORMAT_R1 to the ssd130x driver:

https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org

Best regards,
Javier

 drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 0d2b36ba4011..60536cd0c42d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
 	return ret;
 }
 
-static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						     struct drm_atomic_state *state)
-{
-	struct drm_device *drm = plane->dev;
-	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
-	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
-	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
-	unsigned int page_height = ssd130x->device_info->page_height;
-	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
-	const struct drm_format_info *fi;
-	unsigned int pitch;
-	int ret;
-
-	ret = drm_plane_helper_atomic_check(plane, state);
-	if (ret)
-		return ret;
-
-	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
-	if (!ssd130x_state->data_array)
-		return -ENOMEM;
-
-	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
-		fi = drm_format_info(DRM_FORMAT_R1);
-		if (!fi)
-			return -EINVAL;
-
-		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
-
-		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
-		if (!ssd130x_state->buffer) {
-			kfree(ssd130x_state->data_array);
-			/* Set to prevent a double free in .atomic_destroy_state() */
-			ssd130x_state->data_array = NULL;
-			return -ENOMEM;
-		}
-	}
-
-	return 0;
-}
-
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						       struct drm_atomic_state *state)
 {
@@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
 	if (!ssd130x_state)
 		return NULL;
 
-	/* The buffers are not duplicated and are allocated in .atomic_check */
-	ssd130x_state->buffer = NULL;
-	ssd130x_state->data_array = NULL;
-
 	new_shadow_plane_state = &ssd130x_state->base;
 
 	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
@@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
 {
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
 
-	kfree(ssd130x_state->data_array);
-	kfree(ssd130x_state->buffer);
-
 	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
 
 	kfree(ssd130x_state);
@@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
 	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
 };
@@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
 	return MODE_OK;
 }
 
+int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_device *drm = crtc->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
+	unsigned int page_height = ssd130x->device_info->page_height;
+	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+	const struct drm_format_info *fi;
+	unsigned int pitch;
+	int ret;
+
+	ret = drm_crtc_helper_atomic_check(crtc, state);
+	if (ret)
+		return ret;
+
+	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
+	if (!ssd130x_state->data_array)
+		return -ENOMEM;
+
+	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
+		fi = drm_format_info(DRM_FORMAT_R1);
+		if (!fi)
+			return -EINVAL;
+
+		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+		if (!ssd130x_state->buffer) {
+			kfree(ssd130x_state->data_array);
+			/* Set to prevent a double free in .atomic_destroy_state() */
+			ssd130x_state->data_array = NULL;
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
+				       struct drm_crtc_state *state)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
+	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
+
+	drm_atomic_helper_crtc_destroy_state(crtc, state);
+
+	kfree(ssd130x_state->data_array);
+	kfree(ssd130x_state->buffer);
+}
+
 /*
  * The CRTC is always enabled. Screen updates are performed by
  * the primary plane's atomic_update function. Disabling clears
@@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
  */
 static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
 	.mode_valid = ssd130x_crtc_helper_mode_valid,
-	.atomic_check = drm_crtc_helper_atomic_check,
+	.atomic_check = ssd130x_crtc_helper_atomic_check,
 };
 
 static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
@@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_destroy_state = ssd130x_crtc_destroy_state,
 };
 
 static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
-- 
2.41.0

[RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
.atomic_check() callback") moved the allocation of the intermediate and
HW buffers from the encoder's .atomic_enable callback to primary plane's
.atomic_check callback.

This was suggested by Maxime Ripard because drivers aren't allowed to fail
after drm_atomic_helper_swap_state() has been called, and the encoder's
.atomic_enable happens after the new atomic state has been swapped.

But that change caused a performance regression in very slow platforms,
since now the allocation happens for every plane's atomic state commit.
For example, Geert Uytterhoeven reports that is the case on a VexRiscV
softcore (RISC-V CPU implementation on an FPGA).

To prevent that, move the move the buffers' allocation and free to the
CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
happens on a modeset. Since the intermediate buffer is only needed when
not using the controller native format (R1), doing the buffer allocation
at that CRTC's .atomic_check time would be enough.

Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
Hello,

This is a RFC because I'm not sure if there is a net benefit after this
change. I find the currect code much cleaner and less error prone, even
when Geert reports that performs worse on his (very slow) platform.

But I'm still posting it to see what others think. I've tested the patch
on an I2C ssd1306 OLED and found no regressions.

The patch is on top on Geert's latest patch-set that adds support for the
DRM_FORMAT_R1 to the ssd130x driver:

https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org

Best regards,
Javier

 drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 0d2b36ba4011..60536cd0c42d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
 	return ret;
 }
 
-static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
-						     struct drm_atomic_state *state)
-{
-	struct drm_device *drm = plane->dev;
-	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
-	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
-	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
-	unsigned int page_height = ssd130x->device_info->page_height;
-	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
-	const struct drm_format_info *fi;
-	unsigned int pitch;
-	int ret;
-
-	ret = drm_plane_helper_atomic_check(plane, state);
-	if (ret)
-		return ret;
-
-	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
-	if (!ssd130x_state->data_array)
-		return -ENOMEM;
-
-	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
-		fi = drm_format_info(DRM_FORMAT_R1);
-		if (!fi)
-			return -EINVAL;
-
-		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
-
-		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
-		if (!ssd130x_state->buffer) {
-			kfree(ssd130x_state->data_array);
-			/* Set to prevent a double free in .atomic_destroy_state() */
-			ssd130x_state->data_array = NULL;
-			return -ENOMEM;
-		}
-	}
-
-	return 0;
-}
-
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
 						       struct drm_atomic_state *state)
 {
@@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
 	if (!ssd130x_state)
 		return NULL;
 
-	/* The buffers are not duplicated and are allocated in .atomic_check */
-	ssd130x_state->buffer = NULL;
-	ssd130x_state->data_array = NULL;
-
 	new_shadow_plane_state = &ssd130x_state->base;
 
 	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
@@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
 {
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
 
-	kfree(ssd130x_state->data_array);
-	kfree(ssd130x_state->buffer);
-
 	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
 
 	kfree(ssd130x_state);
@@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
+	.atomic_check = drm_plane_helper_atomic_check,
 	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
 	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
 };
@@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
 	return MODE_OK;
 }
 
+int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_device *drm = crtc->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
+	unsigned int page_height = ssd130x->device_info->page_height;
+	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+	const struct drm_format_info *fi;
+	unsigned int pitch;
+	int ret;
+
+	ret = drm_crtc_helper_atomic_check(crtc, state);
+	if (ret)
+		return ret;
+
+	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
+	if (!ssd130x_state->data_array)
+		return -ENOMEM;
+
+	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
+		fi = drm_format_info(DRM_FORMAT_R1);
+		if (!fi)
+			return -EINVAL;
+
+		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+		if (!ssd130x_state->buffer) {
+			kfree(ssd130x_state->data_array);
+			/* Set to prevent a double free in .atomic_destroy_state() */
+			ssd130x_state->data_array = NULL;
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
+				       struct drm_crtc_state *state)
+{
+	struct drm_plane *plane = crtc->primary;
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
+	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
+
+	drm_atomic_helper_crtc_destroy_state(crtc, state);
+
+	kfree(ssd130x_state->data_array);
+	kfree(ssd130x_state->buffer);
+}
+
 /*
  * The CRTC is always enabled. Screen updates are performed by
  * the primary plane's atomic_update function. Disabling clears
@@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
  */
 static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
 	.mode_valid = ssd130x_crtc_helper_mode_valid,
-	.atomic_check = drm_crtc_helper_atomic_check,
+	.atomic_check = ssd130x_crtc_helper_atomic_check,
 };
 
 static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
@@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_destroy_state = ssd130x_crtc_destroy_state,
 };
 
 static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
-- 
2.41.0

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Javier

Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).
> 
> To prevent that, move the move the buffers' allocation and free to the

Double 'move the'

And maybe buffer's rather than buffers'

> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> happens on a modeset. Since the intermediate buffer is only needed when
> not using the controller native format (R1), doing the buffer allocation
> at that CRTC's .atomic_check time would be enough.
> 
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Hello,
> 
> This is a RFC because I'm not sure if there is a net benefit after this
> change. I find the currect code much cleaner and less error prone, even
> when Geert reports that performs worse on his (very slow) platform.
> 
> But I'm still posting it to see what others think. I've tested the patch
> on an I2C ssd1306 OLED and found no regressions.
> 
> The patch is on top on Geert's latest patch-set that adds support for the
> DRM_FORMAT_R1 to the ssd130x driver:
> 
> https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org
> 
> Best regards,
> Javier
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
>   1 file changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d2b36ba4011..60536cd0c42d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>   	return ret;
>   }
>   
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *state)
> -{
> -	struct drm_device *drm = plane->dev;
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> -	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> -	unsigned int page_height = ssd130x->device_info->page_height;
> -	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> -	const struct drm_format_info *fi;
> -	unsigned int pitch;
> -	int ret;
> -
> -	ret = drm_plane_helper_atomic_check(plane, state);
> -	if (ret)
> -		return ret;
> -
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array)
> -		return -ENOMEM;
> -
> -	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> -		fi = drm_format_info(DRM_FORMAT_R1);
> -		if (!fi)
> -			return -EINVAL;
> -
> -		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> -
> -		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -		if (!ssd130x_state->buffer) {
> -			kfree(ssd130x_state->data_array);
> -			/* Set to prevent a double free in .atomic_destroy_state() */
> -			ssd130x_state->data_array = NULL;
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						       struct drm_atomic_state *state)
>   {
> @@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
>   	if (!ssd130x_state)
>   		return NULL;
>   
> -	/* The buffers are not duplicated and are allocated in .atomic_check */
> -	ssd130x_state->buffer = NULL;
> -	ssd130x_state->data_array = NULL;
> -
>   	new_shadow_plane_state = &ssd130x_state->base;
>   
>   	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> @@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   {
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
>   
> -	kfree(ssd130x_state->data_array);
> -	kfree(ssd130x_state->buffer);
> -
>   	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>   
>   	kfree(ssd130x_state);
> @@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   
>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>   };
> @@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> +int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane *plane = crtc->primary;

Using 'primary' is deprecated. You can only refer from plane to crtc 
easily. The other direction is complicated.

> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);

I'd store these pointers in the CRTC state and fetch them from within 
ssd130x_primary_plane_helper_atomic_update() during the display update. 
This guarantees that the pointer addresses are always legal.

Besides the pointers, the CRTC state can also store the primary plane 
format, which you update from the plane's atomic check. By doing so, you 
wont need to refer to the plane state from the CRTC's atomic_check. The 
plane's atomic_check runs before the CRTC's atomic_check. [1]

The struct ssd130x_plane_state can then be replaced by stardard DRM 
shadow-plane state. It also solves the problem with 'crtc->primary'.

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L974

> +	unsigned int page_height = ssd130x->device_info->page_height;
> +	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);

Do you really need kcalloc here? kmalloc seems good enough.

> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
> +
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);

Same question about kcalloc.

> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +
> +	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +
> +	kfree(ssd130x_state->data_array);
> +	kfree(ssd130x_state->buffer);

This cross references among state-helpers has the potential to blow up. 
It's not really clear if there even is a plane state here.

Also see my comment on the allocation of these buffers., which would 
solve this problem as well.

Best regards
Thomas

> +}
> +
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = drm_crtc_helper_atomic_check,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_destroy_state = ssd130x_crtc_destroy_state,
>   };
>   
>   static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Javier

Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).
> 
> To prevent that, move the move the buffers' allocation and free to the

Double 'move the'

And maybe buffer's rather than buffers'

> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> happens on a modeset. Since the intermediate buffer is only needed when
> not using the controller native format (R1), doing the buffer allocation
> at that CRTC's .atomic_check time would be enough.
> 
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Hello,
> 
> This is a RFC because I'm not sure if there is a net benefit after this
> change. I find the currect code much cleaner and less error prone, even
> when Geert reports that performs worse on his (very slow) platform.
> 
> But I'm still posting it to see what others think. I've tested the patch
> on an I2C ssd1306 OLED and found no regressions.
> 
> The patch is on top on Geert's latest patch-set that adds support for the
> DRM_FORMAT_R1 to the ssd130x driver:
> 
> https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org
> 
> Best regards,
> Javier
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
>   1 file changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d2b36ba4011..60536cd0c42d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>   	return ret;
>   }
>   
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *state)
> -{
> -	struct drm_device *drm = plane->dev;
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> -	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> -	unsigned int page_height = ssd130x->device_info->page_height;
> -	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> -	const struct drm_format_info *fi;
> -	unsigned int pitch;
> -	int ret;
> -
> -	ret = drm_plane_helper_atomic_check(plane, state);
> -	if (ret)
> -		return ret;
> -
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array)
> -		return -ENOMEM;
> -
> -	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> -		fi = drm_format_info(DRM_FORMAT_R1);
> -		if (!fi)
> -			return -EINVAL;
> -
> -		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> -
> -		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -		if (!ssd130x_state->buffer) {
> -			kfree(ssd130x_state->data_array);
> -			/* Set to prevent a double free in .atomic_destroy_state() */
> -			ssd130x_state->data_array = NULL;
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						       struct drm_atomic_state *state)
>   {
> @@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
>   	if (!ssd130x_state)
>   		return NULL;
>   
> -	/* The buffers are not duplicated and are allocated in .atomic_check */
> -	ssd130x_state->buffer = NULL;
> -	ssd130x_state->data_array = NULL;
> -
>   	new_shadow_plane_state = &ssd130x_state->base;
>   
>   	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> @@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   {
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
>   
> -	kfree(ssd130x_state->data_array);
> -	kfree(ssd130x_state->buffer);
> -
>   	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>   
>   	kfree(ssd130x_state);
> @@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   
>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>   };
> @@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> +int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane *plane = crtc->primary;

Using 'primary' is deprecated. You can only refer from plane to crtc 
easily. The other direction is complicated.

> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);

I'd store these pointers in the CRTC state and fetch them from within 
ssd130x_primary_plane_helper_atomic_update() during the display update. 
This guarantees that the pointer addresses are always legal.

Besides the pointers, the CRTC state can also store the primary plane 
format, which you update from the plane's atomic check. By doing so, you 
wont need to refer to the plane state from the CRTC's atomic_check. The 
plane's atomic_check runs before the CRTC's atomic_check. [1]

The struct ssd130x_plane_state can then be replaced by stardard DRM 
shadow-plane state. It also solves the problem with 'crtc->primary'.

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L974

> +	unsigned int page_height = ssd130x->device_info->page_height;
> +	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);

Do you really need kcalloc here? kmalloc seems good enough.

> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
> +
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);

Same question about kcalloc.

> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +
> +	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +
> +	kfree(ssd130x_state->data_array);
> +	kfree(ssd130x_state->buffer);

This cross references among state-helpers has the potential to blow up. 
It's not really clear if there even is a plane state here.

Also see my comment on the allocation of these buffers., which would 
solve this problem as well.

Best regards
Thomas

> +}
> +
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = drm_crtc_helper_atomic_check,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_destroy_state = ssd130x_crtc_destroy_state,
>   };
>   
>   static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > .atomic_check() callback") moved the allocation of the intermediate and
> > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > .atomic_check callback.
> >
> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > .atomic_enable happens after the new atomic state has been swapped.
> >
> > But that change caused a performance regression in very slow platforms,
> > since now the allocation happens for every plane's atomic state commit.
> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > softcore (RISC-V CPU implementation on an FPGA).
> >
> > To prevent that, move the move the buffers' allocation and free to the
>
> Double 'move the'
>
> And maybe buffer's rather than buffers'
>
> > CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> > happens on a modeset. Since the intermediate buffer is only needed when
> > not using the controller native format (R1), doing the buffer allocation
> > at that CRTC's .atomic_check time would be enough.
> >
> > Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Javier: thanks for your patch!

> Besides the pointers, the CRTC state can also store the primary plane
> format, which you update from the plane's atomic check. By doing so, you
> wont need to refer to the plane state from the CRTC's atomic_check. The
> plane's atomic_check runs before the CRTC's atomic_check. [1]

I haven't tested Javier's patch yet, but does this mean that his patch
won't help?

The problem I saw was that these buffers were allocated and freed
over and over again on each flash of the cursor of the text console
on top of the emulated frame buffer device.

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > .atomic_check() callback") moved the allocation of the intermediate and
> > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > .atomic_check callback.
> >
> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > .atomic_enable happens after the new atomic state has been swapped.
> >
> > But that change caused a performance regression in very slow platforms,
> > since now the allocation happens for every plane's atomic state commit.
> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > softcore (RISC-V CPU implementation on an FPGA).
> >
> > To prevent that, move the move the buffers' allocation and free to the
>
> Double 'move the'
>
> And maybe buffer's rather than buffers'
>
> > CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> > happens on a modeset. Since the intermediate buffer is only needed when
> > not using the controller native format (R1), doing the buffer allocation
> > at that CRTC's .atomic_check time would be enough.
> >
> > Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Javier: thanks for your patch!

> Besides the pointers, the CRTC state can also store the primary plane
> format, which you update from the plane's atomic check. By doing so, you
> wont need to refer to the plane state from the CRTC's atomic_check. The
> plane's atomic_check runs before the CRTC's atomic_check. [1]

I haven't tested Javier's patch yet, but does this mean that his patch
won't help?

The problem I saw was that these buffers were allocated and freed
over and over again on each flash of the cursor of the text console
on top of the emulated frame buffer device.

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Geert

Am 30.08.23 um 09:40 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
>>> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>>> .atomic_check() callback") moved the allocation of the intermediate and
>>> HW buffers from the encoder's .atomic_enable callback to primary plane's
>>> .atomic_check callback.
>>>
>>> This was suggested by Maxime Ripard because drivers aren't allowed to fail
>>> after drm_atomic_helper_swap_state() has been called, and the encoder's
>>> .atomic_enable happens after the new atomic state has been swapped.
>>>
>>> But that change caused a performance regression in very slow platforms,
>>> since now the allocation happens for every plane's atomic state commit.
>>> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>>> softcore (RISC-V CPU implementation on an FPGA).
>>>
>>> To prevent that, move the move the buffers' allocation and free to the
>>
>> Double 'move the'
>>

>> And maybe buffer's rather than buffers'

Scratch that remark.

>>
>>> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
>>> happens on a modeset. Since the intermediate buffer is only needed when
>>> not using the controller native format (R1), doing the buffer allocation
>>> at that CRTC's .atomic_check time would be enough.
>>>
>>> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
>>> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Javier: thanks for your patch!
> 
>> Besides the pointers, the CRTC state can also store the primary plane
>> format, which you update from the plane's atomic check. By doing so, you
>> wont need to refer to the plane state from the CRTC's atomic_check. The
>> plane's atomic_check runs before the CRTC's atomic_check. [1]
> 
> I haven't tested Javier's patch yet, but does this mean that his patch
> won't help?
> 
> The problem I saw was that these buffers were allocated and freed
> over and over again on each flash of the cursor of the text console
> on top of the emulated frame buffer device.

Javier's current patch should resolve this problem. The temporary 
buffers are now only allocated on display-mode/format changes, but not 
on each single screen update. My review concerns only the implementation.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Geert

Am 30.08.23 um 09:40 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, Aug 30, 2023 at 9:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
>>> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>>> .atomic_check() callback") moved the allocation of the intermediate and
>>> HW buffers from the encoder's .atomic_enable callback to primary plane's
>>> .atomic_check callback.
>>>
>>> This was suggested by Maxime Ripard because drivers aren't allowed to fail
>>> after drm_atomic_helper_swap_state() has been called, and the encoder's
>>> .atomic_enable happens after the new atomic state has been swapped.
>>>
>>> But that change caused a performance regression in very slow platforms,
>>> since now the allocation happens for every plane's atomic state commit.
>>> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>>> softcore (RISC-V CPU implementation on an FPGA).
>>>
>>> To prevent that, move the move the buffers' allocation and free to the
>>
>> Double 'move the'
>>

>> And maybe buffer's rather than buffers'

Scratch that remark.

>>
>>> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
>>> happens on a modeset. Since the intermediate buffer is only needed when
>>> not using the controller native format (R1), doing the buffer allocation
>>> at that CRTC's .atomic_check time would be enough.
>>>
>>> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
>>> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Javier: thanks for your patch!
> 
>> Besides the pointers, the CRTC state can also store the primary plane
>> format, which you update from the plane's atomic check. By doing so, you
>> wont need to refer to the plane state from the CRTC's atomic_check. The
>> plane's atomic_check runs before the CRTC's atomic_check. [1]
> 
> I haven't tested Javier's patch yet, but does this mean that his patch
> won't help?
> 
> The problem I saw was that these buffers were allocated and freed
> over and over again on each flash of the cursor of the text console
> on top of the emulated frame buffer device.

Javier's current patch should resolve this problem. The temporary 
buffers are now only allocated on display-mode/format changes, but not 
on each single screen update. My review concerns only the implementation.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Javier,

another idea about this patch: why not just keep the allocation in the 
plane's atomic check, but store the temporary buffers in a plane struct. 
You'd only grow the arrays length in atomic_check and later fetch the 
pointers in atomic_update. It needs some locking, but nothing complicated.

Best regards
Thomas

Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).
> 
> To prevent that, move the move the buffers' allocation and free to the
> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> happens on a modeset. Since the intermediate buffer is only needed when
> not using the controller native format (R1), doing the buffer allocation
> at that CRTC's .atomic_check time would be enough.
> 
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Hello,
> 
> This is a RFC because I'm not sure if there is a net benefit after this
> change. I find the currect code much cleaner and less error prone, even
> when Geert reports that performs worse on his (very slow) platform.
> 
> But I'm still posting it to see what others think. I've tested the patch
> on an I2C ssd1306 OLED and found no regressions.
> 
> The patch is on top on Geert's latest patch-set that adds support for the
> DRM_FORMAT_R1 to the ssd130x driver:
> 
> https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org
> 
> Best regards,
> Javier
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
>   1 file changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d2b36ba4011..60536cd0c42d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>   	return ret;
>   }
>   
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *state)
> -{
> -	struct drm_device *drm = plane->dev;
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> -	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> -	unsigned int page_height = ssd130x->device_info->page_height;
> -	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> -	const struct drm_format_info *fi;
> -	unsigned int pitch;
> -	int ret;
> -
> -	ret = drm_plane_helper_atomic_check(plane, state);
> -	if (ret)
> -		return ret;
> -
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array)
> -		return -ENOMEM;
> -
> -	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> -		fi = drm_format_info(DRM_FORMAT_R1);
> -		if (!fi)
> -			return -EINVAL;
> -
> -		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> -
> -		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -		if (!ssd130x_state->buffer) {
> -			kfree(ssd130x_state->data_array);
> -			/* Set to prevent a double free in .atomic_destroy_state() */
> -			ssd130x_state->data_array = NULL;
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						       struct drm_atomic_state *state)
>   {
> @@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
>   	if (!ssd130x_state)
>   		return NULL;
>   
> -	/* The buffers are not duplicated and are allocated in .atomic_check */
> -	ssd130x_state->buffer = NULL;
> -	ssd130x_state->data_array = NULL;
> -
>   	new_shadow_plane_state = &ssd130x_state->base;
>   
>   	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> @@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   {
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
>   
> -	kfree(ssd130x_state->data_array);
> -	kfree(ssd130x_state->buffer);
> -
>   	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>   
>   	kfree(ssd130x_state);
> @@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   
>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>   };
> @@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> +int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +	unsigned int page_height = ssd130x->device_info->page_height;
> +	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
> +
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +
> +	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +
> +	kfree(ssd130x_state->data_array);
> +	kfree(ssd130x_state->buffer);
> +}
> +
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = drm_crtc_helper_atomic_check,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_destroy_state = ssd130x_crtc_destroy_state,
>   };
>   
>   static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi Javier,

another idea about this patch: why not just keep the allocation in the 
plane's atomic check, but store the temporary buffers in a plane struct. 
You'd only grow the arrays length in atomic_check and later fetch the 
pointers in atomic_update. It needs some locking, but nothing complicated.

Best regards
Thomas

Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).
> 
> To prevent that, move the move the buffers' allocation and free to the
> CRTC's .atomic_check and .atomic_destroy_state callbacks, so that only
> happens on a modeset. Since the intermediate buffer is only needed when
> not using the controller native format (R1), doing the buffer allocation
> at that CRTC's .atomic_check time would be enough.
> 
> Fixes: 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's .atomic_check() callback")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> Hello,
> 
> This is a RFC because I'm not sure if there is a net benefit after this
> change. I find the currect code much cleaner and less error prone, even
> when Geert reports that performs worse on his (very slow) platform.
> 
> But I'm still posting it to see what others think. I've tested the patch
> on an I2C ssd1306 OLED and found no regressions.
> 
> The patch is on top on Geert's latest patch-set that adds support for the
> DRM_FORMAT_R1 to the ssd130x driver:
> 
> https://lore.kernel.org/dri-devel/cover.1692888745.git.geert@linux-m68k.org
> 
> Best regards,
> Javier
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 106 ++++++++++++++++--------------
>   1 file changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d2b36ba4011..60536cd0c42d 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -650,46 +650,6 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>   	return ret;
>   }
>   
> -static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> -						     struct drm_atomic_state *state)
> -{
> -	struct drm_device *drm = plane->dev;
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> -	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> -	unsigned int page_height = ssd130x->device_info->page_height;
> -	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> -	const struct drm_format_info *fi;
> -	unsigned int pitch;
> -	int ret;
> -
> -	ret = drm_plane_helper_atomic_check(plane, state);
> -	if (ret)
> -		return ret;
> -
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array)
> -		return -ENOMEM;
> -
> -	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> -		fi = drm_format_info(DRM_FORMAT_R1);
> -		if (!fi)
> -			return -EINVAL;
> -
> -		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> -
> -		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -		if (!ssd130x_state->buffer) {
> -			kfree(ssd130x_state->data_array);
> -			/* Set to prevent a double free in .atomic_destroy_state() */
> -			ssd130x_state->data_array = NULL;
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   						       struct drm_atomic_state *state)
>   {
> @@ -762,10 +722,6 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
>   	if (!ssd130x_state)
>   		return NULL;
>   
> -	/* The buffers are not duplicated and are allocated in .atomic_check */
> -	ssd130x_state->buffer = NULL;
> -	ssd130x_state->data_array = NULL;
> -
>   	new_shadow_plane_state = &ssd130x_state->base;
>   
>   	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> @@ -778,9 +734,6 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   {
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
>   
> -	kfree(ssd130x_state->data_array);
> -	kfree(ssd130x_state->buffer);
> -
>   	__drm_gem_destroy_shadow_plane_state(&ssd130x_state->base);
>   
>   	kfree(ssd130x_state);
> @@ -788,7 +741,7 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
>   
>   static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
>   	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> -	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_check = drm_plane_helper_atomic_check,
>   	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
>   	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
>   };
> @@ -818,6 +771,59 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> +int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +	unsigned int page_height = ssd130x->device_info->page_height;
> +	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
> +
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
> +
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
> +				       struct drm_crtc_state *state)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state->state, plane);
> +	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
> +
> +	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +
> +	kfree(ssd130x_state->data_array);
> +	kfree(ssd130x_state->buffer);
> +}
> +
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -825,7 +831,7 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = drm_crtc_helper_atomic_check,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> @@ -834,7 +840,7 @@ static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
>   	.set_config = drm_atomic_helper_set_config,
>   	.page_flip = drm_atomic_helper_page_flip,
>   	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_destroy_state = ssd130x_crtc_destroy_state,
>   };
>   
>   static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi Javier,
>
> another idea about this patch: why not just keep the allocation in the 
> plane's atomic check, but store the temporary buffers in a plane struct. 
> You'd only grow the arrays length in atomic_check and later fetch the 
> pointers in atomic_update. It needs some locking, but nothing complicated.
>

Yes, that would work too. Another option is to just move the buffers to
struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
Allocate buffer in the plane's .atomic_check() callback") but just make
them fixed arrays with the size of the biggest format.

That will be some memory wasted but will prevent the problem of trying to
allocate buffers after drm_atomic_helper_swap_state() has been called.

> Best regards
> Thomas
>
> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi Javier,
>
> another idea about this patch: why not just keep the allocation in the 
> plane's atomic check, but store the temporary buffers in a plane struct. 
> You'd only grow the arrays length in atomic_check and later fetch the 
> pointers in atomic_update. It needs some locking, but nothing complicated.
>

Yes, that would work too. Another option is to just move the buffers to
struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
Allocate buffer in the plane's .atomic_check() callback") but just make
them fixed arrays with the size of the biggest format.

That will be some memory wasted but will prevent the problem of trying to
allocate buffers after drm_atomic_helper_swap_state() has been called.

> Best regards
> Thomas
>
> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

Hi,

On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).

I'd like to have numbers on that. It's a bit surprising to me that,
given how many objects we already allocate during a commit, two small
additional allocations affect performances so dramatically, even on a
slow platform.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

Hi,

On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> .atomic_check() callback") moved the allocation of the intermediate and
> HW buffers from the encoder's .atomic_enable callback to primary plane's
> .atomic_check callback.
> 
> This was suggested by Maxime Ripard because drivers aren't allowed to fail
> after drm_atomic_helper_swap_state() has been called, and the encoder's
> .atomic_enable happens after the new atomic state has been swapped.
> 
> But that change caused a performance regression in very slow platforms,
> since now the allocation happens for every plane's atomic state commit.
> For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> softcore (RISC-V CPU implementation on an FPGA).

I'd like to have numbers on that. It's a bit surprising to me that,
given how many objects we already allocate during a commit, two small
additional allocations affect performances so dramatically, even on a
slow platform.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

On Fri, Sep 01, 2023 at 09:48:09AM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> > Hi Javier,
> >
> > another idea about this patch: why not just keep the allocation in the 
> > plane's atomic check, but store the temporary buffers in a plane struct. 
> > You'd only grow the arrays length in atomic_check and later fetch the 
> > pointers in atomic_update. It needs some locking, but nothing complicated.
> >
> 
> Yes, that would work too. Another option is to just move the buffers to
> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
> Allocate buffer in the plane's .atomic_check() callback") but just make
> them fixed arrays with the size of the biggest format.
> 
> That will be some memory wasted but will prevent the problem of trying to
> allocate buffers after drm_atomic_helper_swap_state() has been called.

If we want to go that road, we don't even need an extra allocation, it
can be part of the state or object structure itself.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

On Fri, Sep 01, 2023 at 09:48:09AM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> > Hi Javier,
> >
> > another idea about this patch: why not just keep the allocation in the 
> > plane's atomic check, but store the temporary buffers in a plane struct. 
> > You'd only grow the arrays length in atomic_check and later fetch the 
> > pointers in atomic_update. It needs some locking, but nothing complicated.
> >
> 
> Yes, that would work too. Another option is to just move the buffers to
> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
> Allocate buffer in the plane's .atomic_check() callback") but just make
> them fixed arrays with the size of the biggest format.
> 
> That will be some memory wasted but will prevent the problem of trying to
> allocate buffers after drm_atomic_helper_swap_state() has been called.

If we want to go that road, we don't even need an extra allocation, it
can be part of the state or object structure itself.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Maxime,

On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > .atomic_check() callback") moved the allocation of the intermediate and
> > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > .atomic_check callback.
> >
> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > .atomic_enable happens after the new atomic state has been swapped.
> >
> > But that change caused a performance regression in very slow platforms,
> > since now the allocation happens for every plane's atomic state commit.
> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > softcore (RISC-V CPU implementation on an FPGA).
>
> I'd like to have numbers on that. It's a bit surprising to me that,
> given how many objects we already allocate during a commit, two small
> additional allocations affect performances so dramatically, even on a
> slow platform.

To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
all these other allocations (and whatever else happens).

I just find it extremely silly to allocate a buffer over and over again,
while we know that buffer is needed for each and every display update.

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Maxime,

On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > .atomic_check() callback") moved the allocation of the intermediate and
> > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > .atomic_check callback.
> >
> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > .atomic_enable happens after the new atomic state has been swapped.
> >
> > But that change caused a performance regression in very slow platforms,
> > since now the allocation happens for every plane's atomic state commit.
> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > softcore (RISC-V CPU implementation on an FPGA).
>
> I'd like to have numbers on that. It's a bit surprising to me that,
> given how many objects we already allocate during a commit, two small
> additional allocations affect performances so dramatically, even on a
> slow platform.

To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
all these other allocations (and whatever else happens).

I just find it extremely silly to allocate a buffer over and over again,
while we know that buffer is needed for each and every display update.

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> On Fri, Sep 01, 2023 at 09:48:09AM +0200, Javier Martinez Canillas wrote:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> > Hi Javier,
>> >
>> > another idea about this patch: why not just keep the allocation in the 
>> > plane's atomic check, but store the temporary buffers in a plane struct. 
>> > You'd only grow the arrays length in atomic_check and later fetch the 
>> > pointers in atomic_update. It needs some locking, but nothing complicated.
>> >
>> 
>> Yes, that would work too. Another option is to just move the buffers to
>> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
>> Allocate buffer in the plane's .atomic_check() callback") but just make
>> them fixed arrays with the size of the biggest format.
>> 
>> That will be some memory wasted but will prevent the problem of trying to
>> allocate buffers after drm_atomic_helper_swap_state() has been called.
>
> If we want to go that road, we don't even need an extra allocation, it
> can be part of the state or object structure itself.
>

Yes, I meant to have it as fixed-length arrays. But still the best option
seems to be to allocate them but in the CRTC's .atomic_check() and store
them in a CRTC private state as Thomas suggested.

Geert will post a v2 of his R1 support patches, I'll wait for those and
after that try to implement Thomas' suggestion.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> On Fri, Sep 01, 2023 at 09:48:09AM +0200, Javier Martinez Canillas wrote:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> > Hi Javier,
>> >
>> > another idea about this patch: why not just keep the allocation in the 
>> > plane's atomic check, but store the temporary buffers in a plane struct. 
>> > You'd only grow the arrays length in atomic_check and later fetch the 
>> > pointers in atomic_update. It needs some locking, but nothing complicated.
>> >
>> 
>> Yes, that would work too. Another option is to just move the buffers to
>> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
>> Allocate buffer in the plane's .atomic_check() callback") but just make
>> them fixed arrays with the size of the biggest format.
>> 
>> That will be some memory wasted but will prevent the problem of trying to
>> allocate buffers after drm_atomic_helper_swap_state() has been called.
>
> If we want to go that road, we don't even need an extra allocation, it
> can be part of the state or object structure itself.
>

Yes, I meant to have it as fixed-length arrays. But still the best option
seems to be to allocate them but in the CRTC's .atomic_check() and store
them in a CRTC private state as Thomas suggested.

Geert will post a v2 of his R1 support patches, I'll wait for those and
after that try to implement Thomas' suggestion.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Maxime,
>
> On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > .atomic_check() callback") moved the allocation of the intermediate and
>> > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > .atomic_check callback.
>> >
>> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > .atomic_enable happens after the new atomic state has been swapped.
>> >
>> > But that change caused a performance regression in very slow platforms,
>> > since now the allocation happens for every plane's atomic state commit.
>> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > softcore (RISC-V CPU implementation on an FPGA).
>>
>> I'd like to have numbers on that. It's a bit surprising to me that,
>> given how many objects we already allocate during a commit, two small
>> additional allocations affect performances so dramatically, even on a
>> slow platform.
>
> To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> all these other allocations (and whatever else happens).
>
> I just find it extremely silly to allocate a buffer over and over again,
> while we know that buffer is needed for each and every display update.
>

Is not efficient that's true, but on the other hand we have other objects
that are destroyed and created on each atomic update.

In the ssd1307fb driver, the buffer is allocated on ssd1307fb_update_rect()
(by calling the ssd1307fb_alloc_array() function), so it also happens for
every display update in the fbdev driver.

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Maxime,
>
> On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > .atomic_check() callback") moved the allocation of the intermediate and
>> > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > .atomic_check callback.
>> >
>> > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > .atomic_enable happens after the new atomic state has been swapped.
>> >
>> > But that change caused a performance regression in very slow platforms,
>> > since now the allocation happens for every plane's atomic state commit.
>> > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > softcore (RISC-V CPU implementation on an FPGA).
>>
>> I'd like to have numbers on that. It's a bit surprising to me that,
>> given how many objects we already allocate during a commit, two small
>> additional allocations affect performances so dramatically, even on a
>> slow platform.
>
> To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> all these other allocations (and whatever else happens).
>
> I just find it extremely silly to allocate a buffer over and over again,
> while we know that buffer is needed for each and every display update.
>

Is not efficient that's true, but on the other hand we have other objects
that are destroyed and created on each atomic update.

In the ssd1307fb driver, the buffer is allocated on ssd1307fb_update_rect()
(by calling the ssd1307fb_alloc_array() function), so it also happens for
every display update in the fbdev driver.

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi

Am 01.09.23 um 09:48 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Hi Javier,
>>
>> another idea about this patch: why not just keep the allocation in the
>> plane's atomic check, but store the temporary buffers in a plane struct.
>> You'd only grow the arrays length in atomic_check and later fetch the
>> pointers in atomic_update. It needs some locking, but nothing complicated.
>>
> 
> Yes, that would work too. Another option is to just move the buffers to
> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:

Adding something like a struct ssd130x_plane that holds the temporary 
memory has the advantage of making a clear connection between the memory 
and the plane. If nothing else, to the next programmer reading the code.

> Allocate buffer in the plane's .atomic_check() callback") but just make
> them fixed arrays with the size of the biggest format.

What is the size of the biggest format? I haven't read the driver code, 
but a shadow plane can be up to 4096 pixels wide. It's 16 KiB for 
XRGB888. Not too much, but not nothing either.

To reduce allocation and/or locking overhead, you could try to update 
the pointers in the plane struct with RCU semantics. Plane updates would 
use whatever pointer they saw, while the plane's atomic_check could grow 
the memory buffers as necessary.

Best regards
Thomas

> 
> That will be some memory wasted but will prevent the problem of trying to
> allocate buffers after drm_atomic_helper_swap_state() has been called.
> 
>> Best regards
>> Thomas
>>
>> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Thomas Zimmermann]

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

Hi

Am 01.09.23 um 09:48 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Hi Javier,
>>
>> another idea about this patch: why not just keep the allocation in the
>> plane's atomic check, but store the temporary buffers in a plane struct.
>> You'd only grow the arrays length in atomic_check and later fetch the
>> pointers in atomic_update. It needs some locking, but nothing complicated.
>>
> 
> Yes, that would work too. Another option is to just move the buffers to
> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:

Adding something like a struct ssd130x_plane that holds the temporary 
memory has the advantage of making a clear connection between the memory 
and the plane. If nothing else, to the next programmer reading the code.

> Allocate buffer in the plane's .atomic_check() callback") but just make
> them fixed arrays with the size of the biggest format.

What is the size of the biggest format? I haven't read the driver code, 
but a shadow plane can be up to 4096 pixels wide. It's 16 KiB for 
XRGB888. Not too much, but not nothing either.

To reduce allocation and/or locking overhead, you could try to update 
the pointers in the plane struct with RCU semantics. Plane updates would 
use whatever pointer they saw, while the plane's atomic_check could grow 
the memory buffers as necessary.

Best regards
Thomas

> 
> That will be some memory wasted but will prevent the problem of trying to
> allocate buffers after drm_atomic_helper_swap_state() has been called.
> 
>> Best regards
>> Thomas
>>
>> Am 30.08.23 um 08:25 schrieb Javier Martinez Canillas:
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi
>
> Am 01.09.23 um 09:48 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Hi Javier,
>>>
>>> another idea about this patch: why not just keep the allocation in the
>>> plane's atomic check, but store the temporary buffers in a plane struct.
>>> You'd only grow the arrays length in atomic_check and later fetch the
>>> pointers in atomic_update. It needs some locking, but nothing complicated.
>>>
>> 
>> Yes, that would work too. Another option is to just move the buffers to
>> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
>
> Adding something like a struct ssd130x_plane that holds the temporary 
> memory has the advantage of making a clear connection between the memory 
> and the plane. If nothing else, to the next programmer reading the code.
>

Ok, I'm confused now. The current version of the driver already has a
struct ssd130x_plane_state that stores the buffers:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/solomon/ssd130x.c#n144

But what you are saying is that instead of setting those pointers to NULL
in the ssd130x_primary_plane_duplicate_state() function, it can just be
allocated once and the pointers copied when duplicating the new state?

So you want me to add some locking when accesing those since will be
shared between the old and new state?

In that case another option is to just kref the buffers, I think that
should be enough?

>> Allocate buffer in the plane's .atomic_check() callback") but just make
>> them fixed arrays with the size of the biggest format.
>
> What is the size of the biggest format? I haven't read the driver code, 
> but a shadow plane can be up to 4096 pixels wide. It's 16 KiB for 
> XRGB888. Not too much, but not nothing either.
>

The shadow plane yes, but I was talking about the buffers in:

struct ssd130x_plane_state {
	struct drm_shadow_plane_state base;
	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
	u8 *buffer;
	/* Buffer to store pixels in HW format and written to the panel */
	u8 *data_array;
};

These are not the size of the shadow buffer but the of the displayed area,
that depends on the panel fixed resolution (defined in the Device Tree).

The biggest resolution for ssd130x panels is 132x64 (SSD1305 and SH1106),
so that means the biggest buffer will be 132 * 64 * R1 pitch = 1056 bytes.

> To reduce allocation and/or locking overhead, you could try to update 
> the pointers in the plane struct with RCU semantics. Plane updates would 
> use whatever pointer they saw, while the plane's atomic_check could grow 
> the memory buffers as necessary.
>

I'm still unsure the added complexity is worth it. As mentioned, the fbdev
driver also allocats the buffers on each display update and at least the
intermediate .buffer that stores the XRGB8888 -> R1 conversion is tied to
the plane, the .data_array that sends the actual data to the controller
can be argued to be tied to the CRTC.

But given that a format change doesn't trigger a CRTC mode set (as Maxime
pointed out to me) then at least the .buffer allocation can't be done in
the CRTC .atomic_check() handler.

I could move the .data_array allocatoin there or even do it at probe time,
but since the .buffer allocation would be done in the plane
.atomic_check() anyways, I would rather keep as is and have both in the
same function.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi
>
> Am 01.09.23 um 09:48 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Hi Javier,
>>>
>>> another idea about this patch: why not just keep the allocation in the
>>> plane's atomic check, but store the temporary buffers in a plane struct.
>>> You'd only grow the arrays length in atomic_check and later fetch the
>>> pointers in atomic_update. It needs some locking, but nothing complicated.
>>>
>> 
>> Yes, that would work too. Another option is to just move the buffers to
>> struct ssd130x_device as it was before commit 45b58669e532 ("drm/ssd130x:
>
> Adding something like a struct ssd130x_plane that holds the temporary 
> memory has the advantage of making a clear connection between the memory 
> and the plane. If nothing else, to the next programmer reading the code.
>

Ok, I'm confused now. The current version of the driver already has a
struct ssd130x_plane_state that stores the buffers:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/solomon/ssd130x.c#n144

But what you are saying is that instead of setting those pointers to NULL
in the ssd130x_primary_plane_duplicate_state() function, it can just be
allocated once and the pointers copied when duplicating the new state?

So you want me to add some locking when accesing those since will be
shared between the old and new state?

In that case another option is to just kref the buffers, I think that
should be enough?

>> Allocate buffer in the plane's .atomic_check() callback") but just make
>> them fixed arrays with the size of the biggest format.
>
> What is the size of the biggest format? I haven't read the driver code, 
> but a shadow plane can be up to 4096 pixels wide. It's 16 KiB for 
> XRGB888. Not too much, but not nothing either.
>

The shadow plane yes, but I was talking about the buffers in:

struct ssd130x_plane_state {
	struct drm_shadow_plane_state base;
	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
	u8 *buffer;
	/* Buffer to store pixels in HW format and written to the panel */
	u8 *data_array;
};

These are not the size of the shadow buffer but the of the displayed area,
that depends on the panel fixed resolution (defined in the Device Tree).

The biggest resolution for ssd130x panels is 132x64 (SSD1305 and SH1106),
so that means the biggest buffer will be 132 * 64 * R1 pitch = 1056 bytes.

> To reduce allocation and/or locking overhead, you could try to update 
> the pointers in the plane struct with RCU semantics. Plane updates would 
> use whatever pointer they saw, while the plane's atomic_check could grow 
> the memory buffers as necessary.
>

I'm still unsure the added complexity is worth it. As mentioned, the fbdev
driver also allocats the buffers on each display update and at least the
intermediate .buffer that stores the XRGB8888 -> R1 conversion is tied to
the plane, the .data_array that sends the actual data to the controller
can be argued to be tied to the CRTC.

But given that a format change doesn't trigger a CRTC mode set (as Maxime
pointed out to me) then at least the .buffer allocation can't be done in
the CRTC .atomic_check() handler.

I could move the .data_array allocatoin there or even do it at probe time,
but since the .buffer allocation would be done in the plane
.atomic_check() anyways, I would rather keep as is and have both in the
same function.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > .atomic_check() callback") moved the allocation of the intermediate and
> > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > .atomic_check callback.
> > >
> > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > .atomic_enable happens after the new atomic state has been swapped.
> > >
> > > But that change caused a performance regression in very slow platforms,
> > > since now the allocation happens for every plane's atomic state commit.
> > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > softcore (RISC-V CPU implementation on an FPGA).
> >
> > I'd like to have numbers on that. It's a bit surprising to me that,
> > given how many objects we already allocate during a commit, two small
> > additional allocations affect performances so dramatically, even on a
> > slow platform.
> 
> To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> all these other allocations (and whatever else happens).
> 
> I just find it extremely silly to allocate a buffer over and over again,
> while we know that buffer is needed for each and every display update.

Maybe it's silly, but I guess it depends on what you want to optimize
for. You won't know the size of that buffer before you're in
atomic_check. So it's a different trade-off than you would like, but I
wouldn't call it extremely silly.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

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

On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > .atomic_check() callback") moved the allocation of the intermediate and
> > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > .atomic_check callback.
> > >
> > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > .atomic_enable happens after the new atomic state has been swapped.
> > >
> > > But that change caused a performance regression in very slow platforms,
> > > since now the allocation happens for every plane's atomic state commit.
> > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > softcore (RISC-V CPU implementation on an FPGA).
> >
> > I'd like to have numbers on that. It's a bit surprising to me that,
> > given how many objects we already allocate during a commit, two small
> > additional allocations affect performances so dramatically, even on a
> > slow platform.
> 
> To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> all these other allocations (and whatever else happens).
> 
> I just find it extremely silly to allocate a buffer over and over again,
> while we know that buffer is needed for each and every display update.

Maybe it's silly, but I guess it depends on what you want to optimize
for. You won't know the size of that buffer before you're in
atomic_check. So it's a different trade-off than you would like, but I
wouldn't call it extremely silly.

Maxime

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

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Maxime,

On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > > .atomic_check() callback") moved the allocation of the intermediate and
> > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > > .atomic_check callback.
> > > >
> > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > > .atomic_enable happens after the new atomic state has been swapped.
> > > >
> > > > But that change caused a performance regression in very slow platforms,
> > > > since now the allocation happens for every plane's atomic state commit.
> > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > > softcore (RISC-V CPU implementation on an FPGA).
> > >
> > > I'd like to have numbers on that. It's a bit surprising to me that,
> > > given how many objects we already allocate during a commit, two small
> > > additional allocations affect performances so dramatically, even on a
> > > slow platform.
> >
> > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> > all these other allocations (and whatever else happens).
> >
> > I just find it extremely silly to allocate a buffer over and over again,
> > while we know that buffer is needed for each and every display update.
>
> Maybe it's silly, but I guess it depends on what you want to optimize
> for. You won't know the size of that buffer before you're in
> atomic_check. So it's a different trade-off than you would like, but I
> wouldn't call it extremely silly.

The size of ssd130x_plane_state.data_array[] is fixed, and depends
on the actual display connected.
The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
on the plane's size (which is currently fixed to the display size).

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Geert Uytterhoeven]

Hi Maxime,

On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
> On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > > .atomic_check() callback") moved the allocation of the intermediate and
> > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > > .atomic_check callback.
> > > >
> > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > > .atomic_enable happens after the new atomic state has been swapped.
> > > >
> > > > But that change caused a performance regression in very slow platforms,
> > > > since now the allocation happens for every plane's atomic state commit.
> > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > > softcore (RISC-V CPU implementation on an FPGA).
> > >
> > > I'd like to have numbers on that. It's a bit surprising to me that,
> > > given how many objects we already allocate during a commit, two small
> > > additional allocations affect performances so dramatically, even on a
> > > slow platform.
> >
> > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> > all these other allocations (and whatever else happens).
> >
> > I just find it extremely silly to allocate a buffer over and over again,
> > while we know that buffer is needed for each and every display update.
>
> Maybe it's silly, but I guess it depends on what you want to optimize
> for. You won't know the size of that buffer before you're in
> atomic_check. So it's a different trade-off than you would like, but I
> wouldn't call it extremely silly.

The size of ssd130x_plane_state.data_array[] is fixed, and depends
on the actual display connected.
The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
on the plane's size (which is currently fixed to the display size).

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: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Maxime,
>
> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>> On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
>> > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > > > .atomic_check() callback") moved the allocation of the intermediate and
>> > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > > > .atomic_check callback.
>> > > >
>> > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > > > .atomic_enable happens after the new atomic state has been swapped.
>> > > >
>> > > > But that change caused a performance regression in very slow platforms,
>> > > > since now the allocation happens for every plane's atomic state commit.
>> > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > > > softcore (RISC-V CPU implementation on an FPGA).
>> > >
>> > > I'd like to have numbers on that. It's a bit surprising to me that,
>> > > given how many objects we already allocate during a commit, two small
>> > > additional allocations affect performances so dramatically, even on a
>> > > slow platform.
>> >
>> > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
>> > all these other allocations (and whatever else happens).
>> >
>> > I just find it extremely silly to allocate a buffer over and over again,
>> > while we know that buffer is needed for each and every display update.
>>
>> Maybe it's silly, but I guess it depends on what you want to optimize
>> for. You won't know the size of that buffer before you're in
>> atomic_check. So it's a different trade-off than you would like, but I
>> wouldn't call it extremely silly.
>
> The size of ssd130x_plane_state.data_array[] is fixed, and depends
> on the actual display connected.

Agreed.

> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
> on the plane's size (which is currently fixed to the display size).
>

Well, one could say that also depends on the format chosen. That is, if
XRGB8888 is used then its size should be the fixed display size but if R1
is used its size could be 0 (since the shadow plane will already store the
pixels in R1 format).

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Maxime,
>
> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>> On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
>> > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > > > .atomic_check() callback") moved the allocation of the intermediate and
>> > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > > > .atomic_check callback.
>> > > >
>> > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > > > .atomic_enable happens after the new atomic state has been swapped.
>> > > >
>> > > > But that change caused a performance regression in very slow platforms,
>> > > > since now the allocation happens for every plane's atomic state commit.
>> > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > > > softcore (RISC-V CPU implementation on an FPGA).
>> > >
>> > > I'd like to have numbers on that. It's a bit surprising to me that,
>> > > given how many objects we already allocate during a commit, two small
>> > > additional allocations affect performances so dramatically, even on a
>> > > slow platform.
>> >
>> > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
>> > all these other allocations (and whatever else happens).
>> >
>> > I just find it extremely silly to allocate a buffer over and over again,
>> > while we know that buffer is needed for each and every display update.
>>
>> Maybe it's silly, but I guess it depends on what you want to optimize
>> for. You won't know the size of that buffer before you're in
>> atomic_check. So it's a different trade-off than you would like, but I
>> wouldn't call it extremely silly.
>
> The size of ssd130x_plane_state.data_array[] is fixed, and depends
> on the actual display connected.

Agreed.

> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
> on the plane's size (which is currently fixed to the display size).
>

Well, one could say that also depends on the format chosen. That is, if
XRGB8888 is used then its size should be the fixed display size but if R1
is used its size could be 0 (since the shadow plane will already store the
pixels in R1 format).

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

On Fri, Sep 01, 2023 at 02:08:11PM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > > > .atomic_check() callback") moved the allocation of the intermediate and
> > > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > > > .atomic_check callback.
> > > > >
> > > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > > > .atomic_enable happens after the new atomic state has been swapped.
> > > > >
> > > > > But that change caused a performance regression in very slow platforms,
> > > > > since now the allocation happens for every plane's atomic state commit.
> > > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > > > softcore (RISC-V CPU implementation on an FPGA).
> > > >
> > > > I'd like to have numbers on that. It's a bit surprising to me that,
> > > > given how many objects we already allocate during a commit, two small
> > > > additional allocations affect performances so dramatically, even on a
> > > > slow platform.
> > >
> > > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> > > all these other allocations (and whatever else happens).
> > >
> > > I just find it extremely silly to allocate a buffer over and over again,
> > > while we know that buffer is needed for each and every display update.
> >
> > Maybe it's silly, but I guess it depends on what you want to optimize
> > for. You won't know the size of that buffer before you're in
> > atomic_check. So it's a different trade-off than you would like, but I
> > wouldn't call it extremely silly.
> 
> The size of ssd130x_plane_state.data_array[] is fixed, and depends
> on the actual display connected.

That one can be tied to the CRTC state if needed. It would only be
allocated on each modeset, so probably once for that kind of device.

> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
> on the plane's size (which is currently fixed to the display size).

Doesn't it depend on the format as well?

Maxime

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Maxime Ripard]

On Fri, Sep 01, 2023 at 02:08:11PM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
> > > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
> > > > > .atomic_check() callback") moved the allocation of the intermediate and
> > > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
> > > > > .atomic_check callback.
> > > > >
> > > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
> > > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
> > > > > .atomic_enable happens after the new atomic state has been swapped.
> > > > >
> > > > > But that change caused a performance regression in very slow platforms,
> > > > > since now the allocation happens for every plane's atomic state commit.
> > > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
> > > > > softcore (RISC-V CPU implementation on an FPGA).
> > > >
> > > > I'd like to have numbers on that. It's a bit surprising to me that,
> > > > given how many objects we already allocate during a commit, two small
> > > > additional allocations affect performances so dramatically, even on a
> > > > slow platform.
> > >
> > > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
> > > all these other allocations (and whatever else happens).
> > >
> > > I just find it extremely silly to allocate a buffer over and over again,
> > > while we know that buffer is needed for each and every display update.
> >
> > Maybe it's silly, but I guess it depends on what you want to optimize
> > for. You won't know the size of that buffer before you're in
> > atomic_check. So it's a different trade-off than you would like, but I
> > wouldn't call it extremely silly.
> 
> The size of ssd130x_plane_state.data_array[] is fixed, and depends
> on the actual display connected.

That one can be tied to the CRTC state if needed. It would only be
allocated on each modeset, so probably once for that kind of device.

> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
> on the plane's size (which is currently fixed to the display size).

Doesn't it depend on the format as well?

Maxime

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> On Fri, Sep 01, 2023 at 02:08:11PM +0200, Geert Uytterhoeven wrote:
>> Hi Maxime,
>> 
>> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>> > On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
>> > > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> > > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > > > > .atomic_check() callback") moved the allocation of the intermediate and
>> > > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > > > > .atomic_check callback.
>> > > > >
>> > > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > > > > .atomic_enable happens after the new atomic state has been swapped.
>> > > > >
>> > > > > But that change caused a performance regression in very slow platforms,
>> > > > > since now the allocation happens for every plane's atomic state commit.
>> > > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > > > > softcore (RISC-V CPU implementation on an FPGA).
>> > > >
>> > > > I'd like to have numbers on that. It's a bit surprising to me that,
>> > > > given how many objects we already allocate during a commit, two small
>> > > > additional allocations affect performances so dramatically, even on a
>> > > > slow platform.
>> > >
>> > > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
>> > > all these other allocations (and whatever else happens).
>> > >
>> > > I just find it extremely silly to allocate a buffer over and over again,
>> > > while we know that buffer is needed for each and every display update.
>> >
>> > Maybe it's silly, but I guess it depends on what you want to optimize
>> > for. You won't know the size of that buffer before you're in
>> > atomic_check. So it's a different trade-off than you would like, but I
>> > wouldn't call it extremely silly.
>> 
>> The size of ssd130x_plane_state.data_array[] is fixed, and depends
>> on the actual display connected.
>
> That one can be tied to the CRTC state if needed. It would only be
> allocated on each modeset, so probably once for that kind of device.
>

Yes.

>> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
>> on the plane's size (which is currently fixed to the display size).
>
> Doesn't it depend on the format as well?
>

Yes and no. The buffer[] size is fixed, but whether that intermediate
buffer is needed or not will depend if the native format was chosen.

So one could say that is either 0 (not used) or the fixed size needed
to do the format conversion from XRGB8888 to R1.

> Maxime
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

> On Fri, Sep 01, 2023 at 02:08:11PM +0200, Geert Uytterhoeven wrote:
>> Hi Maxime,
>> 
>> On Fri, Sep 1, 2023 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>> > On Fri, Sep 01, 2023 at 10:36:17AM +0200, Geert Uytterhoeven wrote:
>> > > On Fri, Sep 1, 2023 at 10:22 AM Maxime Ripard <mripard@kernel.org> wrote:
>> > > > On Wed, Aug 30, 2023 at 08:25:08AM +0200, Javier Martinez Canillas wrote:
>> > > > > The commit 45b58669e532 ("drm/ssd130x: Allocate buffer in the plane's
>> > > > > .atomic_check() callback") moved the allocation of the intermediate and
>> > > > > HW buffers from the encoder's .atomic_enable callback to primary plane's
>> > > > > .atomic_check callback.
>> > > > >
>> > > > > This was suggested by Maxime Ripard because drivers aren't allowed to fail
>> > > > > after drm_atomic_helper_swap_state() has been called, and the encoder's
>> > > > > .atomic_enable happens after the new atomic state has been swapped.
>> > > > >
>> > > > > But that change caused a performance regression in very slow platforms,
>> > > > > since now the allocation happens for every plane's atomic state commit.
>> > > > > For example, Geert Uytterhoeven reports that is the case on a VexRiscV
>> > > > > softcore (RISC-V CPU implementation on an FPGA).
>> > > >
>> > > > I'd like to have numbers on that. It's a bit surprising to me that,
>> > > > given how many objects we already allocate during a commit, two small
>> > > > additional allocations affect performances so dramatically, even on a
>> > > > slow platform.
>> > >
>> > > To be fair, I didn't benchmark that.  Perhaps it's just too slow due to
>> > > all these other allocations (and whatever else happens).
>> > >
>> > > I just find it extremely silly to allocate a buffer over and over again,
>> > > while we know that buffer is needed for each and every display update.
>> >
>> > Maybe it's silly, but I guess it depends on what you want to optimize
>> > for. You won't know the size of that buffer before you're in
>> > atomic_check. So it's a different trade-off than you would like, but I
>> > wouldn't call it extremely silly.
>> 
>> The size of ssd130x_plane_state.data_array[] is fixed, and depends
>> on the actual display connected.
>
> That one can be tied to the CRTC state if needed. It would only be
> allocated on each modeset, so probably once for that kind of device.
>

Yes.

>> The size of ssd130x_plane_state.buffer[]  is also fixed, and depends
>> on the plane's size (which is currently fixed to the display size).
>
> Doesn't it depend on the format as well?
>

Yes and no. The buffer[] size is fixed, but whether that intermediate
buffer is needed or not will depend if the native format was chosen.

So one could say that is either 0 (not used) or the fixed size needed
to do the format conversion from XRGB8888 to R1.

> Maxime
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[kernel test robot]

:::::: 
:::::: Manual check reason: "git am base is a link in commit message"
:::::: 

BCC: lkp@intel.com
CC: llvm@lists.linux.dev
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230830062546.720679-1-javierm@redhat.com>
References: <20230830062546.720679-1-javierm@redhat.com>
TO: Javier Martinez Canillas <javierm@redhat.com>

Hi Javier,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on linux-next/master]
[cannot apply to linus/master v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/drm-ssd130x-Allocate-buffer-in-the-CRTC-s-atomic_check-callback/20230830-142757
base:   linux-next/master
patch link:    https://lore.kernel.org/r/20230830062546.720679-1-javierm%40redhat.com
patch subject: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: arm-randconfig-r011-20230830 (https://download.01.org/0day-ci/archive/20230830/202308302310.DJgZdxyk-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230830/202308302310.DJgZdxyk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202308302310.DJgZdxyk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/solomon/ssd130x.c:774:5: warning: no previous prototype for function 'ssd130x_crtc_helper_atomic_check' [-Wmissing-prototypes]
     774 | int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
         |     ^
   drivers/gpu/drm/solomon/ssd130x.c:774:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
     774 | int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
         | ^
         | static 
   1 warning generated.


vim +/ssd130x_crtc_helper_atomic_check +774 drivers/gpu/drm/solomon/ssd130x.c

a61732e808672c Javier Martinez Canillas 2022-02-14  773  
61df7954abc151 Javier Martinez Canillas 2023-08-30 @774  int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
61df7954abc151 Javier Martinez Canillas 2023-08-30  775  {
61df7954abc151 Javier Martinez Canillas 2023-08-30  776  	struct drm_device *drm = crtc->dev;
61df7954abc151 Javier Martinez Canillas 2023-08-30  777  	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
61df7954abc151 Javier Martinez Canillas 2023-08-30  778  	struct drm_plane *plane = crtc->primary;
61df7954abc151 Javier Martinez Canillas 2023-08-30  779  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
61df7954abc151 Javier Martinez Canillas 2023-08-30  780  	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
61df7954abc151 Javier Martinez Canillas 2023-08-30  781  	unsigned int page_height = ssd130x->device_info->page_height;
61df7954abc151 Javier Martinez Canillas 2023-08-30  782  	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
61df7954abc151 Javier Martinez Canillas 2023-08-30  783  	const struct drm_format_info *fi;
61df7954abc151 Javier Martinez Canillas 2023-08-30  784  	unsigned int pitch;
61df7954abc151 Javier Martinez Canillas 2023-08-30  785  	int ret;
61df7954abc151 Javier Martinez Canillas 2023-08-30  786  
61df7954abc151 Javier Martinez Canillas 2023-08-30  787  	ret = drm_crtc_helper_atomic_check(crtc, state);
61df7954abc151 Javier Martinez Canillas 2023-08-30  788  	if (ret)
61df7954abc151 Javier Martinez Canillas 2023-08-30  789  		return ret;
61df7954abc151 Javier Martinez Canillas 2023-08-30  790  
61df7954abc151 Javier Martinez Canillas 2023-08-30  791  	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
61df7954abc151 Javier Martinez Canillas 2023-08-30  792  	if (!ssd130x_state->data_array)
61df7954abc151 Javier Martinez Canillas 2023-08-30  793  		return -ENOMEM;
61df7954abc151 Javier Martinez Canillas 2023-08-30  794  
61df7954abc151 Javier Martinez Canillas 2023-08-30  795  	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
61df7954abc151 Javier Martinez Canillas 2023-08-30  796  		fi = drm_format_info(DRM_FORMAT_R1);
61df7954abc151 Javier Martinez Canillas 2023-08-30  797  		if (!fi)
61df7954abc151 Javier Martinez Canillas 2023-08-30  798  			return -EINVAL;
61df7954abc151 Javier Martinez Canillas 2023-08-30  799  
61df7954abc151 Javier Martinez Canillas 2023-08-30  800  		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
61df7954abc151 Javier Martinez Canillas 2023-08-30  801  
61df7954abc151 Javier Martinez Canillas 2023-08-30  802  		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
61df7954abc151 Javier Martinez Canillas 2023-08-30  803  		if (!ssd130x_state->buffer) {
61df7954abc151 Javier Martinez Canillas 2023-08-30  804  			kfree(ssd130x_state->data_array);
61df7954abc151 Javier Martinez Canillas 2023-08-30  805  			/* Set to prevent a double free in .atomic_destroy_state() */
61df7954abc151 Javier Martinez Canillas 2023-08-30  806  			ssd130x_state->data_array = NULL;
61df7954abc151 Javier Martinez Canillas 2023-08-30  807  			return -ENOMEM;
61df7954abc151 Javier Martinez Canillas 2023-08-30  808  		}
61df7954abc151 Javier Martinez Canillas 2023-08-30  809  	}
61df7954abc151 Javier Martinez Canillas 2023-08-30  810  
61df7954abc151 Javier Martinez Canillas 2023-08-30  811  	return 0;
61df7954abc151 Javier Martinez Canillas 2023-08-30  812  }
61df7954abc151 Javier Martinez Canillas 2023-08-30  813  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback

[kernel test robot]

:::::: 
:::::: Manual check reason: "git am base is a link in commit message"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230830062546.720679-1-javierm@redhat.com>
References: <20230830062546.720679-1-javierm@redhat.com>
TO: Javier Martinez Canillas <javierm@redhat.com>

Hi Javier,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on linux-next/master]
[cannot apply to linus/master v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/drm-ssd130x-Allocate-buffer-in-the-CRTC-s-atomic_check-callback/20230830-142757
base:   linux-next/master
patch link:    https://lore.kernel.org/r/20230830062546.720679-1-javierm%40redhat.com
patch subject: [RFC PATCH] drm/ssd130x: Allocate buffer in the CRTC's .atomic_check() callback
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230830/202308301645.bMtYLYr9-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230830/202308301645.bMtYLYr9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202308301645.bMtYLYr9-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/solomon/ssd130x.c:774:5: warning: no previous prototype for 'ssd130x_crtc_helper_atomic_check' [-Wmissing-prototypes]
     774 | int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/ssd130x_crtc_helper_atomic_check +774 drivers/gpu/drm/solomon/ssd130x.c

a61732e808672c Javier Martinez Canillas 2022-02-14  773  
61df7954abc151 Javier Martinez Canillas 2023-08-30 @774  int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
61df7954abc151 Javier Martinez Canillas 2023-08-30  775  {
61df7954abc151 Javier Martinez Canillas 2023-08-30  776  	struct drm_device *drm = crtc->dev;
61df7954abc151 Javier Martinez Canillas 2023-08-30  777  	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
61df7954abc151 Javier Martinez Canillas 2023-08-30  778  	struct drm_plane *plane = crtc->primary;
61df7954abc151 Javier Martinez Canillas 2023-08-30  779  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
61df7954abc151 Javier Martinez Canillas 2023-08-30  780  	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
61df7954abc151 Javier Martinez Canillas 2023-08-30  781  	unsigned int page_height = ssd130x->device_info->page_height;
61df7954abc151 Javier Martinez Canillas 2023-08-30  782  	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
61df7954abc151 Javier Martinez Canillas 2023-08-30  783  	const struct drm_format_info *fi;
61df7954abc151 Javier Martinez Canillas 2023-08-30  784  	unsigned int pitch;
61df7954abc151 Javier Martinez Canillas 2023-08-30  785  	int ret;
61df7954abc151 Javier Martinez Canillas 2023-08-30  786  
61df7954abc151 Javier Martinez Canillas 2023-08-30  787  	ret = drm_crtc_helper_atomic_check(crtc, state);
61df7954abc151 Javier Martinez Canillas 2023-08-30  788  	if (ret)
61df7954abc151 Javier Martinez Canillas 2023-08-30  789  		return ret;
61df7954abc151 Javier Martinez Canillas 2023-08-30  790  
61df7954abc151 Javier Martinez Canillas 2023-08-30  791  	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
61df7954abc151 Javier Martinez Canillas 2023-08-30  792  	if (!ssd130x_state->data_array)
61df7954abc151 Javier Martinez Canillas 2023-08-30  793  		return -ENOMEM;
61df7954abc151 Javier Martinez Canillas 2023-08-30  794  
61df7954abc151 Javier Martinez Canillas 2023-08-30  795  	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
61df7954abc151 Javier Martinez Canillas 2023-08-30  796  		fi = drm_format_info(DRM_FORMAT_R1);
61df7954abc151 Javier Martinez Canillas 2023-08-30  797  		if (!fi)
61df7954abc151 Javier Martinez Canillas 2023-08-30  798  			return -EINVAL;
61df7954abc151 Javier Martinez Canillas 2023-08-30  799  
61df7954abc151 Javier Martinez Canillas 2023-08-30  800  		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
61df7954abc151 Javier Martinez Canillas 2023-08-30  801  
61df7954abc151 Javier Martinez Canillas 2023-08-30  802  		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
61df7954abc151 Javier Martinez Canillas 2023-08-30  803  		if (!ssd130x_state->buffer) {
61df7954abc151 Javier Martinez Canillas 2023-08-30  804  			kfree(ssd130x_state->data_array);
61df7954abc151 Javier Martinez Canillas 2023-08-30  805  			/* Set to prevent a double free in .atomic_destroy_state() */
61df7954abc151 Javier Martinez Canillas 2023-08-30  806  			ssd130x_state->data_array = NULL;
61df7954abc151 Javier Martinez Canillas 2023-08-30  807  			return -ENOMEM;
61df7954abc151 Javier Martinez Canillas 2023-08-30  808  		}
61df7954abc151 Javier Martinez Canillas 2023-08-30  809  	}
61df7954abc151 Javier Martinez Canillas 2023-08-30  810  
61df7954abc151 Javier Martinez Canillas 2023-08-30  811  	return 0;
61df7954abc151 Javier Martinez Canillas 2023-08-30  812  }
61df7954abc151 Javier Martinez Canillas 2023-08-30  813  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki