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

[Zack Rusin]

On Thu, Jun 27, 2024 at 8:37 AM Martin Krastev
<martin.krastev at broadcom.com> wrote:
>
> 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?

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