* [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®