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