mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption
@ 2025-09-12 16:42 Iker Pedrosa
  2025-09-12 16:42 ` [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*() Iker Pedrosa
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

This patch series improves the Solomon SSD130x DRM driver by adopting
existing DRM helpers, improving code clarity, and following kernel
coding standards.

* Patch #1 moves DRM GEM framebuffer CPU access calls to make critical
  sections more visible and maintainable.
* Patch #2 replaces WARN_ON with drm_WARN_ON_ONCE to prevent log spam.
* Patch #3 adopts drm_crtc_helper_mode_valid_fixed() for mode
  validation.
* Patch #4 adopts drm_connector_helper_get_modes_fixed() for mode
  management.
* Patch #5 enforces one assignment per line per kernel coding style.

These improvements reduce code duplication by leveraging existing DRM
infrastructure and enhance code readability without changing
functionality.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
Iker Pedrosa (5):
      drm/solomon: Move calls to drm_gem_fb_end_cpu*()
      drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON
      drm/solomon: Simplify mode_valid() using DRM helper
      drm/solomon: Simplify get_modes() using DRM helper
      drm/solomon: Enforce one assignment per line

 drivers/gpu/drm/solomon/ssd130x.c | 77 +++++++++++++++------------------------
 1 file changed, 29 insertions(+), 48 deletions(-)
---
base-commit: c571cb70e1ed43ee543c70151e61a001ab2eefa2
change-id: 20250912-improve-ssd130x-b45b89ff4693

Best regards,
-- 
Iker Pedrosa <ikerpedrosam@gmail.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*()
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
@ 2025-09-12 16:42 ` Iker Pedrosa
  2025-09-17  9:06   ` Javier Martinez Canillas
  2025-09-12 16:42 ` [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON Iker Pedrosa
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Calls to drm_gem_fb_end_cpu*() should be between the calls to
drm_dev*(), and not hidden inside some other function. This way the
critical section code is visible at a glance, keeping it short and
improving maintainability.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index dd2006d51c7a2fc8501904565da806aa47333ad6..297593c7fd20a5a5da81f1e1fcfda9092b19cf90 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1016,15 +1016,9 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
 
 	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
-
 	iosys_map_set_vaddr(&dst, buf);
 	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-
 	ssd130x_update_rect(ssd130x, rect, buf, data_array);
 
 	return ret;
@@ -1048,15 +1042,9 @@ static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
 
 	dst_pitch = drm_rect_width(rect);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
-
 	iosys_map_set_vaddr(&dst, buf);
 	drm_fb_xrgb8888_to_gray8(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-
 	ssd132x_update_rect(ssd130x, rect, buf, data_array);
 
 	return ret;
@@ -1078,15 +1066,9 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
 
 	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
-
 	iosys_map_set_vaddr(&dst, data_array);
 	drm_fb_xrgb8888_to_rgb332(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-
 	ssd133x_update_rect(ssd130x, rect, data_array, dst_pitch);
 
 	return ret;
@@ -1232,6 +1214,9 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
+		return;
+
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		dst_clip = plane_state->dst;
@@ -1245,6 +1230,8 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
 				     &shadow_plane_state->fmtcnv_state);
 	}
 
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
 	drm_dev_exit(idx);
 }
 
@@ -1267,6 +1254,9 @@ static void ssd132x_primary_plane_atomic_update(struct drm_plane *plane,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
+		return;
+
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		dst_clip = plane_state->dst;
@@ -1280,6 +1270,8 @@ static void ssd132x_primary_plane_atomic_update(struct drm_plane *plane,
 				     &shadow_plane_state->fmtcnv_state);
 	}
 
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
 	drm_dev_exit(idx);
 }
 
@@ -1301,6 +1293,9 @@ static void ssd133x_primary_plane_atomic_update(struct drm_plane *plane,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
+		return;
+
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		dst_clip = plane_state->dst;
@@ -1313,6 +1308,8 @@ static void ssd133x_primary_plane_atomic_update(struct drm_plane *plane,
 				     &shadow_plane_state->fmtcnv_state);
 	}
 
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
 	drm_dev_exit(idx);
 }
 

-- 
2.51.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
  2025-09-12 16:42 ` [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*() Iker Pedrosa
@ 2025-09-12 16:42 ` Iker Pedrosa
  2025-09-17  9:23   ` Javier Martinez Canillas
  2025-09-12 16:42 ` [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper Iker Pedrosa
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

To prevent log spam, convert all instances to the DRM-specific
drm_WARN_ON_ONCE() macro. This ensures that a warning is emitted only
the first time the condition is met for a given device instance, which
is the desired behavior within the graphics subsystem.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 297593c7fd20a5a5da81f1e1fcfda9092b19cf90..c6939377ec4b5a42ce0f40d070ce60d544d8516d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1390,7 +1390,7 @@ static void ssd130x_primary_plane_reset(struct drm_plane *plane)
 {
 	struct ssd130x_plane_state *ssd130x_state;
 
-	WARN_ON(plane->state);
+	drm_WARN_ON_ONCE(plane->dev, plane->state);
 
 	ssd130x_state = kzalloc(sizeof(*ssd130x_state), GFP_KERNEL);
 	if (!ssd130x_state)
@@ -1405,7 +1405,7 @@ static struct drm_plane_state *ssd130x_primary_plane_duplicate_state(struct drm_
 	struct ssd130x_plane_state *old_ssd130x_state;
 	struct ssd130x_plane_state *ssd130x_state;
 
-	if (WARN_ON(!plane->state))
+	if (drm_WARN_ON_ONCE(plane->dev, !plane->state))
 		return NULL;
 
 	old_ssd130x_state = to_ssd130x_plane_state(plane->state);
@@ -1555,7 +1555,7 @@ static void ssd130x_crtc_reset(struct drm_crtc *crtc)
 {
 	struct ssd130x_crtc_state *ssd130x_state;
 
-	WARN_ON(crtc->state);
+	drm_WARN_ON_ONCE(crtc->dev, crtc->state);
 
 	ssd130x_state = kzalloc(sizeof(*ssd130x_state), GFP_KERNEL);
 	if (!ssd130x_state)
@@ -1569,7 +1569,7 @@ static struct drm_crtc_state *ssd130x_crtc_duplicate_state(struct drm_crtc *crtc
 	struct ssd130x_crtc_state *old_ssd130x_state;
 	struct ssd130x_crtc_state *ssd130x_state;
 
-	if (WARN_ON(!crtc->state))
+	if (drm_WARN_ON_ONCE(crtc->dev, !crtc->state))
 		return NULL;
 
 	old_ssd130x_state = to_ssd130x_crtc_state(crtc->state);

-- 
2.51.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
  2025-09-12 16:42 ` [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*() Iker Pedrosa
  2025-09-12 16:42 ` [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON Iker Pedrosa
@ 2025-09-12 16:42 ` Iker Pedrosa
  2025-09-17  9:27   ` Javier Martinez Canillas
  2025-09-12 16:42 ` [PATCH 4/5] drm/solomon: Simplify get_modes() " Iker Pedrosa
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

The ssd130x_crtc_mode_valid() function contains a manual implementation
to validate the display mode against the panel's single fixed resolution.

This pattern is common for simple displays, and the DRM core already
provides the drm_crtc_helper_mode_valid_fixed() helper for this exact
use case.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index c6939377ec4b5a42ce0f40d070ce60d544d8516d..2058d188159c3eae28de1614b9fffb06ac5551be 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1470,15 +1470,7 @@ static enum drm_mode_status ssd130x_crtc_mode_valid(struct drm_crtc *crtc,
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(crtc->dev);
 
-	if (mode->hdisplay != ssd130x->mode.hdisplay &&
-	    mode->vdisplay != ssd130x->mode.vdisplay)
-		return MODE_ONE_SIZE;
-	else if (mode->hdisplay != ssd130x->mode.hdisplay)
-		return MODE_ONE_WIDTH;
-	else if (mode->vdisplay != ssd130x->mode.vdisplay)
-		return MODE_ONE_HEIGHT;
-
-	return MODE_OK;
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &ssd130x->mode);
 }
 
 static int ssd130x_crtc_atomic_check(struct drm_crtc *crtc,

-- 
2.51.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/5] drm/solomon: Simplify get_modes() using DRM helper
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
                   ` (2 preceding siblings ...)
  2025-09-12 16:42 ` [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper Iker Pedrosa
@ 2025-09-12 16:42 ` Iker Pedrosa
  2025-09-17  9:28   ` Javier Martinez Canillas
  2025-09-12 16:42 ` [PATCH 5/5] drm/solomon: Enforce one assignment per line Iker Pedrosa
  2025-09-15 11:24 ` [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Thomas Zimmermann
  5 siblings, 1 reply; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

The ssd130x_connector_get_modes function contains a manual implementation
to manage modes.

This pattern is common for simple displays, and the DRM core already
provides the drm_connector_helper_get_modes_fixed() helper for this exact
use case.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 2058d188159c3eae28de1614b9fffb06ac5551be..7bdccb5140195a45d8ffd01e139dd4eb2e3cc327 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1729,20 +1729,8 @@ static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
 static int ssd130x_connector_get_modes(struct drm_connector *connector)
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
-	struct drm_display_mode *mode;
-	struct device *dev = ssd130x->dev;
-
-	mode = drm_mode_duplicate(connector->dev, &ssd130x->mode);
-	if (!mode) {
-		dev_err(dev, "Failed to duplicated mode\n");
-		return 0;
-	}
-
-	drm_mode_probed_add(connector, mode);
-	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
 
-	/* There is only a single mode */
-	return 1;
+	return drm_connector_helper_get_modes_fixed(connector, &ssd130x->mode);
 }
 
 static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {

-- 
2.51.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 5/5] drm/solomon: Enforce one assignment per line
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
                   ` (3 preceding siblings ...)
  2025-09-12 16:42 ` [PATCH 4/5] drm/solomon: Simplify get_modes() " Iker Pedrosa
@ 2025-09-12 16:42 ` Iker Pedrosa
  2025-09-17  9:29   ` Javier Martinez Canillas
  2025-09-15 11:24 ` [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Thomas Zimmermann
  5 siblings, 1 reply; 13+ messages in thread
From: Iker Pedrosa @ 2025-09-12 16:42 UTC (permalink / raw)
  To: Javier Martinez Canillas, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

The code contains several instances of chained assignments. The Linux
kernel coding style generally favors clarity and simplicity over terse
syntax. Refactor the code to use a separate line for each assignment.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 7bdccb5140195a45d8ffd01e139dd4eb2e3cc327..a09e64719f62562126851e67c4f77d779b861148 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1864,10 +1864,14 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
 
 	mode->type = DRM_MODE_TYPE_DRIVER;
 	mode->clock = 1;
-	mode->hdisplay = mode->htotal = ssd130x->width;
-	mode->hsync_start = mode->hsync_end = ssd130x->width;
-	mode->vdisplay = mode->vtotal = ssd130x->height;
-	mode->vsync_start = mode->vsync_end = ssd130x->height;
+	mode->hdisplay = ssd130x->width;
+	mode->htotal = ssd130x->width;
+	mode->hsync_start = ssd130x->width;
+	mode->hsync_end = ssd130x->width;
+	mode->vdisplay = ssd130x->height;
+	mode->vtotal = ssd130x->height;
+	mode->vsync_start = ssd130x->height;
+	mode->vsync_end = ssd130x->height;
 	mode->width_mm = 27;
 	mode->height_mm = 27;
 

-- 
2.51.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption
  2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
                   ` (4 preceding siblings ...)
  2025-09-12 16:42 ` [PATCH 5/5] drm/solomon: Enforce one assignment per line Iker Pedrosa
@ 2025-09-15 11:24 ` Thomas Zimmermann
  5 siblings, 0 replies; 13+ messages in thread
From: Thomas Zimmermann @ 2025-09-15 11:24 UTC (permalink / raw)
  To: Iker Pedrosa, Javier Martinez Canillas, Maarten Lankhorst,
	Maxime Ripard, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel

Hi

Am 12.09.25 um 18:42 schrieb Iker Pedrosa:
> This patch series improves the Solomon SSD130x DRM driver by adopting
> existing DRM helpers, improving code clarity, and following kernel
> coding standards.
>
> * Patch #1 moves DRM GEM framebuffer CPU access calls to make critical
>    sections more visible and maintainable.
> * Patch #2 replaces WARN_ON with drm_WARN_ON_ONCE to prevent log spam.
> * Patch #3 adopts drm_crtc_helper_mode_valid_fixed() for mode
>    validation.
> * Patch #4 adopts drm_connector_helper_get_modes_fixed() for mode
>    management.
> * Patch #5 enforces one assignment per line per kernel coding style.
>
> These improvements reduce code duplication by leveraging existing DRM
> infrastructure and enhance code readability without changing
> functionality.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>

For the series

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

It should also get Javier's reviews before getting merged.

Best regards
Thomas

> ---
> Iker Pedrosa (5):
>        drm/solomon: Move calls to drm_gem_fb_end_cpu*()
>        drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON
>        drm/solomon: Simplify mode_valid() using DRM helper
>        drm/solomon: Simplify get_modes() using DRM helper
>        drm/solomon: Enforce one assignment per line
>
>   drivers/gpu/drm/solomon/ssd130x.c | 77 +++++++++++++++------------------------
>   1 file changed, 29 insertions(+), 48 deletions(-)
> ---
> base-commit: c571cb70e1ed43ee543c70151e61a001ab2eefa2
> change-id: 20250912-improve-ssd130x-b45b89ff4693
>
> Best regards,

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*()
  2025-09-12 16:42 ` [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*() Iker Pedrosa
@ 2025-09-17  9:06   ` Javier Martinez Canillas
  2025-09-17  9:20     ` Thomas Zimmermann
  0 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2025-09-17  9:06 UTC (permalink / raw)
  To: Iker Pedrosa, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Iker Pedrosa <ikerpedrosam@gmail.com> writes:

Hello Iker,

Thanks for your patch.

> Calls to drm_gem_fb_end_cpu*() should be between the calls to
> drm_dev*(), and not hidden inside some other function. This way the
> critical section code is visible at a glance, keeping it short and
> improving maintainability.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>

[...]

> @@ -1232,6 +1214,9 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>  	if (!drm_dev_enter(drm, &idx))
>  		return;
>  
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		return;
> +

In this error path you should call drm_dev_exit(). The convention in the
kernel usually is to have a goto label for this, e.g.:

       if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
              goto out_drm_dev_exit;

>  	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>  	drm_atomic_for_each_plane_damage(&iter, &damage) {
>  		dst_clip = plane_state->dst;
> @@ -1245,6 +1230,8 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>  				     &shadow_plane_state->fmtcnv_state);
>  	}
>  
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +

and then here before the call you could have the label.

out_drm_dev_exit:

>  	drm_dev_exit(idx);

Same comments for the other places where you are adding the
drm_gem_fb_end_cpu*() calls next to the drm_dev*() ones.

After the mentioned changes:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*()
  2025-09-17  9:06   ` Javier Martinez Canillas
@ 2025-09-17  9:20     ` Thomas Zimmermann
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Zimmermann @ 2025-09-17  9:20 UTC (permalink / raw)
  To: Javier Martinez Canillas, Iker Pedrosa, Maarten Lankhorst,
	Maxime Ripard, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel

grr, I should have noticed that before giving the r-b

Am 17.09.25 um 11:06 schrieb Javier Martinez Canillas:
> Iker Pedrosa <ikerpedrosam@gmail.com> writes:
>
> Hello Iker,
>
> Thanks for your patch.
>
>> Calls to drm_gem_fb_end_cpu*() should be between the calls to
>> drm_dev*(), and not hidden inside some other function. This way the
>> critical section code is visible at a glance, keeping it short and
>> improving maintainability.
>>
>> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
>> ---
>>   drivers/gpu/drm/solomon/ssd130x.c | 33 +++++++++++++++------------------
>>   1 file changed, 15 insertions(+), 18 deletions(-)
>>
> [...]
>
>> @@ -1232,6 +1214,9 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>>   	if (!drm_dev_enter(drm, &idx))
>>   		return;
>>   
>> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
>> +		return;
>> +
> In this error path you should call drm_dev_exit(). The convention in the
> kernel usually is to have a goto label for this, e.g.:
>
>         if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
>                goto out_drm_dev_exit;
>
>>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
>>   		dst_clip = plane_state->dst;
>> @@ -1245,6 +1230,8 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
>>   				     &shadow_plane_state->fmtcnv_state);
>>   	}
>>   
>> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +
> and then here before the call you could have the label.
>
> out_drm_dev_exit:
>
>>   	drm_dev_exit(idx);
> Same comments for the other places where you are adding the
> drm_gem_fb_end_cpu*() calls next to the drm_dev*() ones.
>
> After the mentioned changes:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON
  2025-09-12 16:42 ` [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON Iker Pedrosa
@ 2025-09-17  9:23   ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2025-09-17  9:23 UTC (permalink / raw)
  To: Iker Pedrosa, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Iker Pedrosa <ikerpedrosam@gmail.com> writes:

> To prevent log spam, convert all instances to the DRM-specific
> drm_WARN_ON_ONCE() macro. This ensures that a warning is emitted only
> the first time the condition is met for a given device instance, which
> is the desired behavior within the graphics subsystem.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper
  2025-09-12 16:42 ` [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper Iker Pedrosa
@ 2025-09-17  9:27   ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2025-09-17  9:27 UTC (permalink / raw)
  To: Iker Pedrosa, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Iker Pedrosa <ikerpedrosam@gmail.com> writes:

> The ssd130x_crtc_mode_valid() function contains a manual implementation
> to validate the display mode against the panel's single fixed resolution.
>
> This pattern is common for simple displays, and the DRM core already
> provides the drm_crtc_helper_mode_valid_fixed() helper for this exact
> use case.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---

Indeed.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] drm/solomon: Simplify get_modes() using DRM helper
  2025-09-12 16:42 ` [PATCH 4/5] drm/solomon: Simplify get_modes() " Iker Pedrosa
@ 2025-09-17  9:28   ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2025-09-17  9:28 UTC (permalink / raw)
  To: Iker Pedrosa, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Iker Pedrosa <ikerpedrosam@gmail.com> writes:

> The ssd130x_connector_get_modes function contains a manual implementation
> to manage modes.
>
> This pattern is common for simple displays, and the DRM core already
> provides the drm_connector_helper_get_modes_fixed() helper for this exact
> use case.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] drm/solomon: Enforce one assignment per line
  2025-09-12 16:42 ` [PATCH 5/5] drm/solomon: Enforce one assignment per line Iker Pedrosa
@ 2025-09-17  9:29   ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2025-09-17  9:29 UTC (permalink / raw)
  To: Iker Pedrosa, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Iker Pedrosa

Iker Pedrosa <ikerpedrosam@gmail.com> writes:

> The code contains several instances of chained assignments. The Linux
> kernel coding style generally favors clarity and simplicity over terse
> syntax. Refactor the code to use a separate line for each assignment.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-09-17  9:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-12 16:42 [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Iker Pedrosa
2025-09-12 16:42 ` [PATCH 1/5] drm/solomon: Move calls to drm_gem_fb_end_cpu*() Iker Pedrosa
2025-09-17  9:06   ` Javier Martinez Canillas
2025-09-17  9:20     ` Thomas Zimmermann
2025-09-12 16:42 ` [PATCH 2/5] drm/solomon: Use drm_WARN_ON_ONCE instead of WARN_ON Iker Pedrosa
2025-09-17  9:23   ` Javier Martinez Canillas
2025-09-12 16:42 ` [PATCH 3/5] drm/solomon: Simplify mode_valid() using DRM helper Iker Pedrosa
2025-09-17  9:27   ` Javier Martinez Canillas
2025-09-12 16:42 ` [PATCH 4/5] drm/solomon: Simplify get_modes() " Iker Pedrosa
2025-09-17  9:28   ` Javier Martinez Canillas
2025-09-12 16:42 ` [PATCH 5/5] drm/solomon: Enforce one assignment per line Iker Pedrosa
2025-09-17  9:29   ` Javier Martinez Canillas
2025-09-15 11:24 ` [PATCH 0/5] drm/solomon: Code improvements and DRM helper adoption Thomas Zimmermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®