On Thu, Jun 27, 2024 at 8:34 AM Zack Rusin <zack.rusin@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Fixes: 7b0062036c3b ("drm/vmwgfx: Implement virtual crc generation")
> Cc: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> Cc: Martin Krastev <martin.krastev@xxxxxxxxxxxx>
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@xxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxxxxx>
Regards,
Martin
