Messages in this thread | | | Date | Fri, 30 Sep 2022 12:11:00 +0200 | Subject | Re: [PATCH] drm/ssd130x: Iterate over damage clips instead of using a merged rect | From | Thomas Zimmermann <> |
| |
Hi
Am 30.09.22 um 11:25 schrieb Javier Martinez Canillas: > Hello Thomas, > > Thanks a lot for your feedback. > > On 9/30/22 10:26, Thomas Zimmermann wrote: >> Hi >> >> Am 30.09.22 um 10:01 schrieb Javier Martinez Canillas: >>> The drm_atomic_helper_damage_merged() helper merges all the damage clips >>> into one rectangle. If there are multiple damage clips that aren't close >>> to each other, the resulting rectangle could be quite big. >>> >>> Instead of using that function helper, iterate over all the damage clips >>> and update them one by one. >>> >>> Suggested-by: Jocelyn Falempe <jfalempe@redhat.com> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> drivers/gpu/drm/solomon/ssd130x.c | 18 ++++++++++-------- >>> 1 file changed, 10 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c >>> index bc41a5ae810a..2428f1813a8d 100644 >>> --- a/drivers/gpu/drm/solomon/ssd130x.c >>> +++ b/drivers/gpu/drm/solomon/ssd130x.c >>> @@ -578,21 +578,23 @@ static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, >>> 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_atomic_helper_damage_iter iter; >>> struct drm_device *drm = plane->dev; >>> - struct drm_rect src_clip, dst_clip; >>> + struct drm_rect dst_clip; >>> + struct drm_rect damage; >>> int idx; >>> >>> - if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip)) >>> - return; >>> - >>> dst_clip = plane_state->dst; >>> - if (!drm_rect_intersect(&dst_clip, &src_clip)) >>> - return; >>> - >>> if (!drm_dev_enter(drm, &idx)) >>> return; >>> >>> - ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip); >>> + drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state); >>> + drm_atomic_for_each_plane_damage(&iter, &damage) { >>> + if (!drm_rect_intersect(&dst_clip, &damage)) >>> + continue; >> >> dst_clip will be overwritten here. So need to init it within the loop first. >> > > Oh, indeed. I'll move it inside the loop. Thanks for catching this. > >>> + >>> + ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &damage); >> >> In simpledrm, we adjust the destination address with dst_clip like this: >> >> iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, >> &dst_clip)); >> >> How does this work in ssd130x? You never use dst_clip to adjust to the >> changed location. Won't you have out-of-bounds writes on the device? >> > > Right, in ssd130x what I do is: > > static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, > struct drm_rect *rect) > { > struct iosys_map dst; > ... > u8 *buf = NULL; > ... > buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL); > ... > iosys_map_set_vaddr(&dst, buf); > drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); > ... > ssd130x_update_rect(ssd130x, buf, rect); > } > > I understand that's correct too?
From what I understand about ssd130x, blit_rect looks correct up to the call to update_rect. The values in the rect parameter are for the damage area of the plane. In update_rect, the destination coords x and y are also taken from rect. But they should come from dst_clip, which is the on-screen location. Does that make sense?
Best regards Thomas
>
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [unhandled content-type:application/pgp-signature] | |