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 framebufferresult inregions. As USB is rather slow, it's better to process damage areas individually and hence minimize USB-transfered data. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> ---Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> 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;- if (!drm_dev_enter(dev, &idx))+ ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); + if (ret) return;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.
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.
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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
