Messages in this thread Patch in this message | | | Date | Fri, 13 Oct 2023 09:35:31 +0200 | Subject | Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family | From | Thomas Zimmermann <> |
| |
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) [unhandled content-type:application/pgp-signature] | |