[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@xxxxxxxxxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxx

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