Messages in this thread | | | Date | Fri, 13 Oct 2023 17:10:48 +0200 | Subject | Re: [PATCH v3 4/6] drm/ssd130x: Add support for the SSD132x OLED controller family | From | Thomas Zimmermann <> |
| |
Hi
Am 13.10.23 um 16:57 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas, > > Thanks a lot for your feedback. > >> 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. >> > > Yeah, I considered that but as mentioned in the commit message want to see > what are the needs of the SSD133x controller family (I bought a SSD1331 > display but haven't had time to play with it yet) before trying to factor > out the common bits in helper functions. > > [...] > >>> + >>> + 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. >> > > Correct. > >> 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. >> >> > > The idea of making it part of the plane state is that this buffer could be > optional, for example in the case of user-space using the native display > format instead of the emulated XRGB8888. > > In that case, an intermediate buffer won't be used because the shadow-plane > format will already be the native one (e.g: R1) and there won't be a need > to do any format conversion (only the conversion to the data format as is > expected by the controller). > > Take a look to Geert's patch adding R1 support to ssd130x for an example: > > https://lore.kernel.org/all/72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.geert@linux-m68k.org/ > > That's why it was decided that making it part of the plane state follows > better the KMS model, because when using R1 this buffer won't even be > allocated in the primary plane .atomic_check handler. > > [...] > >>> + 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. >> > > Regardless, splitting the blit and update rect might make sense and is an > intersesting idea. I need to explore this, thanks for the suggestion. > > As you mention that these could be follow-up changes, I assume that you > agree with the current approach. Should I expect your review / ack for > this patch-set?
Please take my ack for this patchset
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Best regards Thomas
> >> Best regards >> Thomas >> >
-- 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] | |