[PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family
Thomas Zimmermann
tzimmermann at suse.de
Fri Oct 13 07:35:31 UTC 2023
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231013/8360edc6/attachment-0001.sig>
More information about the dri-devel
mailing list