Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The `drm_atomic_helper_damage_iter_init()` function is called with its `old_state` and `new_state` arguments swapped. Pre-existing issues: - [High] Acquiring a blocking mutex (`vbox->hw_mutex`) inside a plane atomic update callback violates atomic context rules. -- commit d10e9cef3fd78d35bcfae0e63eb85f53c33aa092 Author: Thomas Zimmermann <[email protected]> drm/vboxvideo: Use damage iterator and plane damage clips This commit updates the vboxvideo driver to use the damage iterator for handling updates without damage clips correctly, ensuring the entire display area is updated when no clips are given. It also enables plane damage clips to minimize the area of each display update. > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c > b/drivers/gpu/drm/vboxvideo/vbox_mode.c > index 8e4e5fc9d3c5a..237115c2e1f19 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 is a pre-existing issue, but does this mutex_lock() violate atomic context rules? The DRM subsystem guidelines specify that plane atomic update callbacks execute in atomic context during non-blocking commits, where sleeping functions like mutex_lock() are forbidden. Calling this here, and in vbox_crtc_set_base_and_mode() which is called just above, could lead to "scheduling while atomic" warnings or deadlocks. > > - for (i = 0; i < num_clips; ++i, ++clips) { > + drm_atomic_helper_damage_iter_init(&iter, new_state, old_plane_state); [Severity: High] Are the old and new state arguments swapped here? Looking at the signature for drm_atomic_helper_damage_iter_init(), it expects the old state as the second argument and the new state as the third argument. Passing old_plane_state as the third argument means the iterator evaluates old_plane_state->visible to determine if it should yield updates. If the plane is transitioning from disabled to enabled, this evaluates to false, causing the iterator to skip all updates and leaving the screen blank. It will also read damage clips and source dimensions from the old state instead of the 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)) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
