Re: [PATCH 10/16] drm/udl: Use damage iterator

[Thomas Zimmermann]

Hi

Am 05.10.22 um 00:28 schrieb Javier Martinez Canillas:

On 9/19/22 15:04, Thomas Zimmermann wrote:

Use a damage iterator to process damage areas individually. Merging
damage areas can resul tin large updates of unchanged framebuffer

result in

regions. As USB is rather slow, it's better to process damage areas
individually and hence minimize USB-transfered data.

Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
---

Reviewed-by: Javier Martinez Canillas <javi...@redhat.com>

but I've a comment below.

/*

@@ -301,16 +291,26 @@ static void udl_primary_plane_helper_atomic_update(struct 
drm_plane *plane,
        struct drm_shadow_plane_state *shadow_plane_state = 
to_drm_shadow_plane_state(plane_state);
        struct drm_framebuffer *fb = plane_state->fb;
        struct drm_plane_state *old_plane_state = 
drm_atomic_get_old_plane_state(state, plane);
-       struct drm_rect rect;
-       int idx;
+       struct drm_atomic_helper_damage_iter iter;
+       struct drm_rect damage;
+       int ret, idx;

+       ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+       if (ret)
                return;

- if (!drm_dev_enter(dev, &idx))

This is an unrelated change. The sync was made in udl_handle_damage() before
and you are moving to udl_primary_plane_helper_atomic_update() in this patch.

I don't have a strong opinion and I see that there are drivers that do once
before iterating over the damage areas and others do the sync for each damage
area update. But it would be good to mention that this change is done too and
provided some justification.

Best regards
Thomas

OK, I'll do that. Very briefly; it's for minimizing the overheads of the possible locking and synchronization, and to maybe it keeps possible writers out while we copy from the buffer.

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

OpenPGP_signature
Description: OpenPGP digital signature