Re: [PATCH] drm/ssd130x: Iterate over damage clips instead of using a merged rect

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Thomas,

On 9/30/22 12:11, Thomas Zimmermann wrote:

[...]

>>
>>>> +
>>>> +		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?
> 

I believe you are correct. Then what I should do is to not pass the damage
area to ssd130x_fb_blit_rect() as the struct drm_rect argument but instead
the dst_clip as filled by drm_rect_intersect(). Does that sound correct ?

In other words, the following:

	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;

		ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
	}

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat