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

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

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