[PATCH v2] drm/vboxvideo: Use damage iterator and plane damage clips

[PATCH v2] drm/vboxvideo: Use damage iterator and plane damage clips

[Thomas Zimmermann]

Use damage iterator to handle updates without damage clips correctly.
If no damage clips have been given, drivers should update the entire
display area.

Also enable damage clips for the plane to minimize the area of each
display update.

v2:
- fix plane-state order when calling _damage_iter_init() (Sashiko)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index 8e4e5fc9d3c5..cd78071b6cbb 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -15,6 +15,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
@@ -282,9 +283,10 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
 									   plane);
 	struct drm_crtc *crtc = new_state->crtc;
 	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
 	struct vbox_private *vbox = to_vbox_dev(fb->dev);
-	struct drm_mode_rect *clips;
-	uint32_t num_clips, i;
+	struct drm_rect damage;
+	struct drm_atomic_helper_damage_iter iter;
 
 	vbox_crtc_set_base_and_mode(crtc, fb,
 				    new_state->src_x >> 16,
@@ -292,22 +294,17 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
 
 	/* Send information about dirty rectangles to VBVA. */
 
-	clips = drm_plane_get_damage_clips(new_state);
-	num_clips = drm_plane_get_damage_clips_count(new_state);
-
-	if (!num_clips)
-		return;
-
 	mutex_lock(&vbox->hw_mutex);
 
-	for (i = 0; i < num_clips; ++i, ++clips) {
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		struct vbva_cmd_hdr cmd_hdr;
 		unsigned int crtc_id = to_vbox_crtc(crtc)->crtc_id;
 
-		cmd_hdr.x = (s16)clips->x1;
-		cmd_hdr.y = (s16)clips->y1;
-		cmd_hdr.w = (u16)clips->x2 - clips->x1;
-		cmd_hdr.h = (u16)clips->y2 - clips->y1;
+		cmd_hdr.x = (s16)damage.x1;
+		cmd_hdr.y = (s16)damage.y1;
+		cmd_hdr.w = (u16)drm_rect_width(&damage);
+		cmd_hdr.h = (u16)drm_rect_height(&damage);
 
 		if (!vbva_buffer_begin_update(&vbox->vbva_info[crtc_id],
 					      vbox->guest_pool))
@@ -539,6 +536,7 @@ static struct drm_plane *vbox_create_plane(struct vbox_private *vbox,
 		goto free_plane;
 
 	drm_plane_helper_add(plane, helper_funcs);
+	drm_plane_enable_fb_damage_clips(plane);
 
 	return plane;
 

base-commit: a2d82f27ac35c691c1c65b40c72b13d8b2a7f91f
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: a5a973e527c88a5b47053d7a72aefe0b550197cb
prerequisite-patch-id: b9adc9622920a3e70168e672c2c92795b3e3a106
prerequisite-patch-id: 5030de433a01c2e99056cadb676a8e2ba35f055a
-- 
2.54.0

Re: [PATCH v2] drm/vboxvideo: Use damage iterator and plane damage clips

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the plane atomic update callback `vbox_primary_atomic_update` calls `mutex_lock(&vbox->hw_mutex)`, which is a sleeping function. Calling a sleeping function in an atomic context violates DRM/KMS design rules.
--

commit 5090cc8b95319e5f88f32549af73d197156ce225
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/vboxvideo: Use damage iterator and plane damage clips

This commit modifies the vboxvideo driver to use the damage iterator
for plane updates. It also enables damage clips for the plane to minimize
the display update area.

> diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> index 8e4e5fc9d3c5a..cd78071b6cbbf 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
[ ... ]
> @@ -282,9 +283,10 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
>  									   plane);
>  	struct drm_crtc *crtc = new_state->crtc;
>  	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>  	struct vbox_private *vbox = to_vbox_dev(fb->dev);
> -	struct drm_mode_rect *clips;
> -	uint32_t num_clips, i;
> +	struct drm_rect damage;
> +	struct drm_atomic_helper_damage_iter iter;
>  
>  	vbox_crtc_set_base_and_mode(crtc, fb,
>  				    new_state->src_x >> 16,
> @@ -292,22 +294,17 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
>  
>  	/* Send information about dirty rectangles to VBVA. */
>  
> -	clips = drm_plane_get_damage_clips(new_state);
> -	num_clips = drm_plane_get_damage_clips_count(new_state);
> -
> -	if (!num_clips)
> -		return;
> -
>  	mutex_lock(&vbox->hw_mutex);

[Severity: High]
This isn't a bug introduced by this patch, but vbox_primary_atomic_update()
calls mutex_lock(&vbox->hw_mutex).

Can calling this sleeping function in an atomic context cause kernel warnings
or deadlocks during non-blocking commits? The same mutex is also acquired
earlier in this callback by vbox_crtc_set_base_and_mode().

>  
> -	for (i = 0; i < num_clips; ++i, ++clips) {
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
>  		struct vbva_cmd_hdr cmd_hdr;
>  		unsigned int crtc_id = to_vbox_crtc(crtc)->crtc_id;
>  
> -		cmd_hdr.x = (s16)clips->x1;
> -		cmd_hdr.y = (s16)clips->y1;
> -		cmd_hdr.w = (u16)clips->x2 - clips->x1;
> -		cmd_hdr.h = (u16)clips->y2 - clips->y1;
> +		cmd_hdr.x = (s16)damage.x1;
> +		cmd_hdr.y = (s16)damage.y1;
> +		cmd_hdr.w = (u16)drm_rect_width(&damage);
> +		cmd_hdr.h = (u16)drm_rect_height(&damage);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703151841.375364-1-tzimmermann@suse.de?part=1

Re: [PATCH v2] drm/vboxvideo: Use damage iterator and plane damage clips

[Thomas Zimmermann]

Am 03.07.26 um 17:28 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the plane atomic update callback `vbox_primary_atomic_update` calls `mutex_lock(&vbox->hw_mutex)`, which is a sleeping function. Calling a sleeping function in an atomic context violates DRM/KMS design rules.

This is a false positive.

> --
>
> commit 5090cc8b95319e5f88f32549af73d197156ce225
> Author: Thomas Zimmermann <tzimmermann@suse.de>
>
> drm/vboxvideo: Use damage iterator and plane damage clips
>
> This commit modifies the vboxvideo driver to use the damage iterator
> for plane updates. It also enables damage clips for the plane to minimize
> the display update area.
>
>> diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
>> index 8e4e5fc9d3c5a..cd78071b6cbbf 100644
>> --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
>> +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
> [ ... ]
>> @@ -282,9 +283,10 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
>>   									   plane);
>>   	struct drm_crtc *crtc = new_state->crtc;
>>   	struct drm_framebuffer *fb = new_state->fb;
>> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>>   	struct vbox_private *vbox = to_vbox_dev(fb->dev);
>> -	struct drm_mode_rect *clips;
>> -	uint32_t num_clips, i;
>> +	struct drm_rect damage;
>> +	struct drm_atomic_helper_damage_iter iter;
>>   
>>   	vbox_crtc_set_base_and_mode(crtc, fb,
>>   				    new_state->src_x >> 16,
>> @@ -292,22 +294,17 @@ static void vbox_primary_atomic_update(struct drm_plane *plane,
>>   
>>   	/* Send information about dirty rectangles to VBVA. */
>>   
>> -	clips = drm_plane_get_damage_clips(new_state);
>> -	num_clips = drm_plane_get_damage_clips_count(new_state);
>> -
>> -	if (!num_clips)
>> -		return;
>> -
>>   	mutex_lock(&vbox->hw_mutex);
> [Severity: High]
> This isn't a bug introduced by this patch, but vbox_primary_atomic_update()
> calls mutex_lock(&vbox->hw_mutex).
>
> Can calling this sleeping function in an atomic context cause kernel warnings
> or deadlocks during non-blocking commits? The same mutex is also acquired
> earlier in this callback by vbox_crtc_set_base_and_mode().
>
>>   
>> -	for (i = 0; i < num_clips; ++i, ++clips) {
>> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_state);
>> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
>>   		struct vbva_cmd_hdr cmd_hdr;
>>   		unsigned int crtc_id = to_vbox_crtc(crtc)->crtc_id;
>>   
>> -		cmd_hdr.x = (s16)clips->x1;
>> -		cmd_hdr.y = (s16)clips->y1;
>> -		cmd_hdr.w = (u16)clips->x2 - clips->x1;
>> -		cmd_hdr.h = (u16)clips->y2 - clips->y1;
>> +		cmd_hdr.x = (s16)damage.x1;
>> +		cmd_hdr.y = (s16)damage.y1;
>> +		cmd_hdr.w = (u16)drm_rect_width(&damage);
>> +		cmd_hdr.h = (u16)drm_rect_height(&damage);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)