Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family

From: Thomas Zimmermann
Date: Fri Oct 13 2023 - 03:35:40 EST

Hi Javier,

thanks for this patch.

Am 12.10.23 um 23:38 schrieb Javier Martinez Canillas:
[...]
+static int ssd132x_fb_blit_rect(struct drm_framebuffer *fb,
+ const struct iosys_map *vmap,
+ struct drm_rect *rect, u8 *buf,
+ u8 *data_array)
+{
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+ unsigned int dst_pitch = drm_rect_width(rect);
+ struct iosys_map dst;
+ int ret = 0;
+
+ /* Align x to display segment boundaries */
+ rect->x1 = round_down(rect->x1, SSD132X_SEGMENT_WIDTH);
+ rect->x2 = min_t(unsigned int, round_up(rect->x2, SSD132X_SEGMENT_WIDTH),
+ ssd130x->width);
+
+ 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);

Here's an idea for a follow-up patchset.

You could attempt to integrate the gray8 and mono conversions into drm_fb_blit(). With some the right parameters, both, ssd130x and ssd132x could use the same blitting code from BO to buffer.

+
+ drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+ ssd132x_update_rect(ssd130x, rect, buf, data_array);
+
+ return ret;
+}
+
static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
struct drm_atomic_state *state)
{
@@ -677,6 +901,43 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
return 0;
}
+static int ssd132x_primary_plane_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);
+ struct drm_crtc *crtc = plane_state->crtc;
+ struct drm_crtc_state *crtc_state;
+ const struct drm_format_info *fi;
+ unsigned int pitch;
+ int ret;
+
+ if (!crtc)
+ return -EINVAL;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ ret = drm_plane_helper_atomic_check(plane, state);
+ if (ret)
+ return ret;
+
+ fi = drm_format_info(DRM_FORMAT_R8);
+ 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)
+ return -ENOMEM;

It's unrelated to these patches and I know it's been discussed endlessly, but I have a questions about buffer allocation. That memory acts as another shadow buffer for the device's memory, such that format conversion becomes easier.

But then, why is ->buffer part of the plane_state? Shouldn't it be part of the plane and never be re-allocated? The real size of that buffer is <width> times <height> (not <pitch>). That size is static over the lifetime of the device. That would represent the semantics much better.

This would allow for additional changes: blit_rect and update_rect would be much easier to separate: no more segment adjustments for the blit code; only for updates. If the update code has high latency (IDK), you could push it into a worker thread to run besides the DRM logic. The gud and repaper drivers do something to this effect.


+
+ return 0;
+}
+
static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
{
@@ -711,6 +972,40 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
drm_dev_exit(idx);
}
+static void ssd132x_primary_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_state *state)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+ struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+ struct ssd130x_crtc_state *ssd130x_crtc_state = to_ssd130x_crtc_state(crtc_state);
+ struct ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_state);
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct drm_atomic_helper_damage_iter iter;
+ struct drm_device *drm = plane->dev;
+ struct drm_rect dst_clip;
+ struct drm_rect damage;
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ 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;
+
+ if (!drm_rect_intersect(&dst_clip, &damage))
+ continue;
+
+ ssd132x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
+ ssd130x_plane_state->buffer,
+ ssd130x_crtc_state->data_array);
+ }

Here's another idea for a another follow-up patchset:

You are allocating state->buffer to cover the whole display, right? It's <pitch> times <height> IIRC. Maybe it would make sense to split the damage loop into two loops and inline the driver's blit_rect() function. Something like that

begin_cpu_access()

for_each(damage) {
drm_fb_blit( "from GEM BO to buffer" )
}

end_cpu_access()

for_each(damge) {
update_rect( "from buffer to device" )
}

With the changes from the other comments, the first loop could become entirely device-neutral AFAICT.

Best regards
Thomas

+
+ drm_dev_exit(idx);
+}
+
static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
struct drm_atomic_state *state)
{
@@ -735,6 +1030,30 @@ static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
drm_dev_exit(idx);
}
+static void ssd132x_primary_plane_atomic_disable(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 drm_crtc_state *crtc_state;
+ struct ssd130x_crtc_state *ssd130x_crtc_state;
+ int idx;
+
+ if (!plane_state->crtc)
+ return;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+ ssd130x_crtc_state = to_ssd130x_crtc_state(crtc_state);
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ ssd132x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
+
+ drm_dev_exit(idx);
+}
+
/* Called during init to allocate the plane's atomic state. */
static void ssd130x_primary_plane_reset(struct drm_plane *plane)
{
@@ -785,11 +1104,19 @@ static void ssd130x_primary_plane_destroy_state(struct drm_plane *plane,
kfree(ssd130x_state);
}
-static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
- DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
- .atomic_check = ssd130x_primary_plane_atomic_check,
- .atomic_update = ssd130x_primary_plane_atomic_update,
- .atomic_disable = ssd130x_primary_plane_atomic_disable,
+static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[] = {
+ [SSD130X_FAMILY] = {
+ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ .atomic_check = ssd130x_primary_plane_atomic_check,
+ .atomic_update = ssd130x_primary_plane_atomic_update,
+ .atomic_disable = ssd130x_primary_plane_atomic_disable,
+ },
+ [SSD132X_FAMILY] = {
+ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ .atomic_check = ssd132x_primary_plane_atomic_check,
+ .atomic_update = ssd132x_primary_plane_atomic_update,
+ .atomic_disable = ssd132x_primary_plane_atomic_disable,
+ }
};
static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
@@ -838,6 +1165,27 @@ static int ssd130x_crtc_atomic_check(struct drm_crtc *crtc,
return 0;
}
+static int ssd132x_crtc_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_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ struct ssd130x_crtc_state *ssd130x_state = to_ssd130x_crtc_state(crtc_state);
+ unsigned int columns = DIV_ROUND_UP(ssd130x->width, SSD132X_SEGMENT_WIDTH);
+ int ret;
+
+ ret = drm_crtc_helper_atomic_check(crtc, state);
+ if (ret)
+ return ret;
+
+ ssd130x_state->data_array = kmalloc(columns * ssd130x->height, GFP_KERNEL);
+ if (!ssd130x_state->data_array)
+ return -ENOMEM;
+
+ return 0;
+}
+
/* Called during init to allocate the CRTC's atomic state. */
static void ssd130x_crtc_reset(struct drm_crtc *crtc)
{
@@ -890,9 +1238,15 @@ static void ssd130x_crtc_destroy_state(struct drm_crtc *crtc,
* the primary plane's atomic_update function. Disabling clears
* the screen in the primary plane's atomic_disable function.
*/
-static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
- .mode_valid = ssd130x_crtc_mode_valid,
- .atomic_check = ssd130x_crtc_atomic_check,
+static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs[] = {
+ [SSD130X_FAMILY] = {
+ .mode_valid = ssd130x_crtc_mode_valid,
+ .atomic_check = ssd130x_crtc_atomic_check,
+ },
+ [SSD132X_FAMILY] = {
+ .mode_valid = ssd130x_crtc_mode_valid,
+ .atomic_check = ssd132x_crtc_atomic_check,
+ },
};
static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
@@ -930,6 +1284,31 @@ static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
return;
}
+static void ssd132x_encoder_atomic_enable(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
+{
+ struct drm_device *drm = encoder->dev;
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+ int ret;
+
+ ret = ssd130x_power_on(ssd130x);
+ if (ret)
+ return;
+
+ ret = ssd132x_init(ssd130x);
+ if (ret)
+ goto power_off;
+
+ ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
+
+ backlight_enable(ssd130x->bl_dev);
+
+ return;
+
+power_off:
+ ssd130x_power_off(ssd130x);
+}
+
static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
struct drm_atomic_state *state)
{
@@ -943,9 +1322,15 @@ static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
ssd130x_power_off(ssd130x);
}
-static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs = {
- .atomic_enable = ssd130x_encoder_atomic_enable,
- .atomic_disable = ssd130x_encoder_atomic_disable,
+static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {
+ [SSD130X_FAMILY] = {
+ .atomic_enable = ssd130x_encoder_atomic_enable,
+ .atomic_disable = ssd130x_encoder_atomic_disable,
+ },
+ [SSD132X_FAMILY] = {
+ .atomic_enable = ssd132x_encoder_atomic_enable,
+ .atomic_disable = ssd130x_encoder_atomic_disable,
+ }
};
static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
@@ -1079,6 +1464,7 @@ static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
{
+ enum ssd130x_family_ids family_id = ssd130x->device_info->family_id;
struct drm_display_mode *mode = &ssd130x->mode;
struct device *dev = ssd130x->dev;
struct drm_device *drm = &ssd130x->drm;
@@ -1129,7 +1515,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
return ret;
}
- drm_plane_helper_add(primary_plane, &ssd130x_primary_plane_helper_funcs);
+ drm_plane_helper_add(primary_plane, &ssd130x_primary_plane_helper_funcs[family_id]);
drm_plane_enable_fb_damage_clips(primary_plane);
@@ -1143,7 +1529,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
return ret;
}
- drm_crtc_helper_add(crtc, &ssd130x_crtc_helper_funcs);
+ drm_crtc_helper_add(crtc, &ssd130x_crtc_helper_funcs[family_id]);
/* Encoder */
@@ -1155,7 +1541,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
return ret;
}
- drm_encoder_helper_add(encoder, &ssd130x_encoder_helper_funcs);
+ drm_encoder_helper_add(encoder, &ssd130x_encoder_helper_funcs[family_id]);
encoder->possible_crtcs = drm_crtc_mask(crtc);
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index a5a25e054d2f..acf7cedf0c1a 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -25,7 +25,8 @@
#define SSD13XX_COMMAND 0x80
enum ssd130x_family_ids {
- SSD130X_FAMILY
+ SSD130X_FAMILY,
+ SSD132X_FAMILY
};
enum ssd130x_variants {
@@ -35,6 +36,10 @@ enum ssd130x_variants {
SSD1306_ID,
SSD1307_ID,
SSD1309_ID,
+ /* ssd132x family */
+ SSD1322_ID,
+ SSD1325_ID,
+ SSD1327_ID,
NR_SSD130X_VARIANTS
};

--
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: OpenPGP_signature.asc
Description: OpenPGP digital signature