| Message ID | 20240627053452.2908605-3-zack.rusin@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix various buffer mapping/import issues | expand |
On Thu, Jun 27, 2024 at 8:34 AM Zack Rusin <zack.rusin@broadcom.com> wrote: > > Fix races issues in virtual crc generation by making sure the surface > the code uses for crc computation is properly ref counted. > > Crc generation was trying to be too clever by allowing the surfaces > to go in and out of scope, with the hope of always having some kind > of screen present. That's not always the code, in particular during > atomic disable, so to make sure the surface, when present, is not > being actively destroyed at the same time, hold a reference to it. > > Signed-off-by: Zack Rusin <zack.rusin@broadcom.com> > Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation") > Cc: Zack Rusin <zack.rusin@broadcom.com> > Cc: Martin Krastev <martin.krastev@broadcom.com> > Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 37 +++++++++++++++++----------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > index 3bfcf671fcd5..c35f7df99977 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > @@ -130,22 +130,26 @@ crc_generate_worker(struct work_struct *work) > return; > > spin_lock_irq(&du->vkms.crc_state_lock); > - surf = du->vkms.surface; > + surf = vmw_surface_reference(du->vkms.surface); > spin_unlock_irq(&du->vkms.crc_state_lock); > > - if (vmw_surface_sync(vmw, surf)) { > - drm_warn(crtc->dev, "CRC worker wasn't able to sync the crc surface!\n"); > - return; > + if (surf) { > + if (vmw_surface_sync(vmw, surf)) { > + drm_warn( > + crtc->dev, > + "CRC worker wasn't able to sync the crc surface!\n"); > + return; > + } > + > + ret = compute_crc(crtc, surf, &crc32); > + if (ret) > + return; > + vmw_surface_unreference(&surf); So compute_crc effectively never errs here, so the vmw_surface_unreference is a given, but wouldn't it correct to have the vmw_surface_unreference precede the error-check early-out? > } > > - ret = compute_crc(crtc, surf, &crc32); > - if (ret) > - return; > - > spin_lock_irq(&du->vkms.crc_state_lock); > frame_start = du->vkms.frame_start; > frame_end = du->vkms.frame_end; > - crc_pending = du->vkms.crc_pending; > du->vkms.frame_start = 0; > du->vkms.frame_end = 0; > du->vkms.crc_pending = false; > @@ -164,7 +168,7 @@ vmw_vkms_vblank_simulate(struct hrtimer *timer) > struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer); > struct drm_crtc *crtc = &du->crtc; > struct vmw_private *vmw = vmw_priv(crtc->dev); > - struct vmw_surface *surf = NULL; > + bool has_surface = false; > u64 ret_overrun; > bool locked, ret; > > @@ -179,10 +183,10 @@ vmw_vkms_vblank_simulate(struct hrtimer *timer) > WARN_ON(!ret); > if (!locked) > return HRTIMER_RESTART; > - surf = du->vkms.surface; > + has_surface = du->vkms.surface != NULL; > vmw_vkms_unlock(crtc); > > - if (du->vkms.crc_enabled && surf) { > + if (du->vkms.crc_enabled && has_surface) { > u64 frame = drm_crtc_accurate_vblank_count(crtc); > > spin_lock(&du->vkms.crc_state_lock); > @@ -336,6 +340,8 @@ vmw_vkms_crtc_cleanup(struct drm_crtc *crtc) > { > struct vmw_display_unit *du = vmw_crtc_to_du(crtc); > > + if (du->vkms.surface) > + vmw_surface_unreference(&du->vkms.surface); > WARN_ON(work_pending(&du->vkms.crc_generator_work)); > hrtimer_cancel(&du->vkms.timer); > } > @@ -497,9 +503,12 @@ vmw_vkms_set_crc_surface(struct drm_crtc *crtc, > struct vmw_display_unit *du = vmw_crtc_to_du(crtc); > struct vmw_private *vmw = vmw_priv(crtc->dev); > > - if (vmw->vkms_enabled) { > + if (vmw->vkms_enabled && du->vkms.surface != surf) { > WARN_ON(atomic_read(&du->vkms.atomic_lock) != VMW_VKMS_LOCK_MODESET); > - du->vkms.surface = surf; > + if (du->vkms.surface) > + vmw_surface_unreference(&du->vkms.surface); > + if (surf) > + du->vkms.surface = vmw_surface_reference(surf); > } > } > > -- > 2.40.1 > Otherwise LGTM. Reviewed-by: Martin Krastev <martin.krastev@broadcom.com> Regards, Martin
On Thu, Jun 27, 2024 at 8:37 AM Martin Krastev <martin.krastev@broadcom.com> wrote: > > On Thu, Jun 27, 2024 at 8:34 AM Zack Rusin <zack.rusin@broadcom.com> wrote: > > > > Fix races issues in virtual crc generation by making sure the surface > > the code uses for crc computation is properly ref counted. > > > > Crc generation was trying to be too clever by allowing the surfaces > > to go in and out of scope, with the hope of always having some kind > > of screen present. That's not always the code, in particular during > > atomic disable, so to make sure the surface, when present, is not > > being actively destroyed at the same time, hold a reference to it. > > > > Signed-off-by: Zack Rusin <zack.rusin@broadcom.com> > > Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation") > > Cc: Zack Rusin <zack.rusin@broadcom.com> > > Cc: Martin Krastev <martin.krastev@broadcom.com> > > Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 37 +++++++++++++++++----------- > > 1 file changed, 23 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > > index 3bfcf671fcd5..c35f7df99977 100644 > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c > > @@ -130,22 +130,26 @@ crc_generate_worker(struct work_struct *work) > > return; > > > > spin_lock_irq(&du->vkms.crc_state_lock); > > - surf = du->vkms.surface; > > + surf = vmw_surface_reference(du->vkms.surface); > > spin_unlock_irq(&du->vkms.crc_state_lock); > > > > - if (vmw_surface_sync(vmw, surf)) { > > - drm_warn(crtc->dev, "CRC worker wasn't able to sync the crc surface!\n"); > > - return; > > + if (surf) { > > + if (vmw_surface_sync(vmw, surf)) { > > + drm_warn( > > + crtc->dev, > > + "CRC worker wasn't able to sync the crc surface!\n"); > > + return; > > + } > > + > > + ret = compute_crc(crtc, surf, &crc32); > > + if (ret) > > + return; > > + vmw_surface_unreference(&surf); > > So compute_crc effectively never errs here, so the > vmw_surface_unreference is a given, but > wouldn't it correct to have the vmw_surface_unreference precede the > error-check early-out? Yes, good catch on both counts. I'll just make compute_crc return void in v2 instead of unconditionally returning 0, this way we won't have to deal with multiple unref paths. z
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c index 3bfcf671fcd5..c35f7df99977 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c @@ -130,22 +130,26 @@ crc_generate_worker(struct work_struct *work) return; spin_lock_irq(&du->vkms.crc_state_lock); - surf = du->vkms.surface; + surf = vmw_surface_reference(du->vkms.surface); spin_unlock_irq(&du->vkms.crc_state_lock); - if (vmw_surface_sync(vmw, surf)) { - drm_warn(crtc->dev, "CRC worker wasn't able to sync the crc surface!\n"); - return; + if (surf) { + if (vmw_surface_sync(vmw, surf)) { + drm_warn( + crtc->dev, + "CRC worker wasn't able to sync the crc surface!\n"); + return; + } + + ret = compute_crc(crtc, surf, &crc32); + if (ret) + return; + vmw_surface_unreference(&surf); } - ret = compute_crc(crtc, surf, &crc32); - if (ret) - return; - spin_lock_irq(&du->vkms.crc_state_lock); frame_start = du->vkms.frame_start; frame_end = du->vkms.frame_end; - crc_pending = du->vkms.crc_pending; du->vkms.frame_start = 0; du->vkms.frame_end = 0; du->vkms.crc_pending = false; @@ -164,7 +168,7 @@ vmw_vkms_vblank_simulate(struct hrtimer *timer) struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer); struct drm_crtc *crtc = &du->crtc; struct vmw_private *vmw = vmw_priv(crtc->dev); - struct vmw_surface *surf = NULL; + bool has_surface = false; u64 ret_overrun; bool locked, ret; @@ -179,10 +183,10 @@ vmw_vkms_vblank_simulate(struct hrtimer *timer) WARN_ON(!ret); if (!locked) return HRTIMER_RESTART; - surf = du->vkms.surface; + has_surface = du->vkms.surface != NULL; vmw_vkms_unlock(crtc); - if (du->vkms.crc_enabled && surf) { + if (du->vkms.crc_enabled && has_surface) { u64 frame = drm_crtc_accurate_vblank_count(crtc); spin_lock(&du->vkms.crc_state_lock); @@ -336,6 +340,8 @@ vmw_vkms_crtc_cleanup(struct drm_crtc *crtc) { struct vmw_display_unit *du = vmw_crtc_to_du(crtc); + if (du->vkms.surface) + vmw_surface_unreference(&du->vkms.surface); WARN_ON(work_pending(&du->vkms.crc_generator_work)); hrtimer_cancel(&du->vkms.timer); } @@ -497,9 +503,12 @@ vmw_vkms_set_crc_surface(struct drm_crtc *crtc, struct vmw_display_unit *du = vmw_crtc_to_du(crtc); struct vmw_private *vmw = vmw_priv(crtc->dev); - if (vmw->vkms_enabled) { + if (vmw->vkms_enabled && du->vkms.surface != surf) { WARN_ON(atomic_read(&du->vkms.atomic_lock) != VMW_VKMS_LOCK_MODESET); - du->vkms.surface = surf; + if (du->vkms.surface) + vmw_surface_unreference(&du->vkms.surface); + if (surf) + du->vkms.surface = vmw_surface_reference(surf); } }
Fix races issues in virtual crc generation by making sure the surface the code uses for crc computation is properly ref counted. Crc generation was trying to be too clever by allowing the surfaces to go in and out of scope, with the hope of always having some kind of screen present. That's not always the code, in particular during atomic disable, so to make sure the surface, when present, is not being actively destroyed at the same time, hold a reference to it. Signed-off-by: Zack Rusin <zack.rusin@broadcom.com> Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation") Cc: Zack Rusin <zack.rusin@broadcom.com> Cc: Martin Krastev <martin.krastev@broadcom.com> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 37 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 14 deletions(-)