[PATCH 2/4] drm/vmwgfx: Make sure the screen surface is ref counted

[Martin Krastev]

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

Regards,
Martin