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.hindex a5a25e054d2f..acf7cedf0c1a 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -25,7 +25,8 @@ #define SSD13XX_COMMAND 0x80enum 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)
OpenPGP_signature.asc
Description: OpenPGP digital signature
