[PATCH v2 1/2] drm/vmwgfx: Prevent unmapping active read buffers

[Ian Forbes]

Looks Good
Thanks,

Reviewed-by: Ian Forbes <ian.forbes at broadcom.com>

On Wed, Aug 14, 2024 at 2:28 PM Zack Rusin <zack.rusin at broadcom.com> wrote:
>
> The kms paths keep a persistent map active to read and compare the cursor
> buffer. These maps can race with each other in simple scenario where:
> a) buffer "a" mapped for update
> b) buffer "a" mapped for compare
> c) do the compare
> d) unmap "a" for compare
> e) update the cursor
> f) unmap "a" for update
> At step "e" the buffer has been unmapped and the read contents is bogus.
>
> Prevent unmapping of active read buffers by simply keeping a count of
> how many paths have currently active maps and unmap only when the count
> reaches 0.
>
> v2: Update doc strings
>
> Fixes: 485d98d472d5 ("drm/vmwgfx: Add support for CursorMob and CursorBypass 4")
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list at broadcom.com>
> Cc: dri-devel at lists.freedesktop.org
> Cc: <stable at vger.kernel.org> # v5.19+
> Signed-off-by: Zack Rusin <zack.rusin at broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 13 +++++++++++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_bo.h |  3 +++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index f42ebc4a7c22..a0e433fbcba6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -360,6 +360,8 @@ void *vmw_bo_map_and_cache_size(struct vmw_bo *vbo, size_t size)
>         void *virtual;
>         int ret;
>
> +       atomic_inc(&vbo->map_count);
> +
>         virtual = ttm_kmap_obj_virtual(&vbo->map, &not_used);
>         if (virtual)
>                 return virtual;
> @@ -383,11 +385,17 @@ void *vmw_bo_map_and_cache_size(struct vmw_bo *vbo, size_t size)
>   */
>  void vmw_bo_unmap(struct vmw_bo *vbo)
>  {
> +       int map_count;
> +
>         if (vbo->map.bo == NULL)
>                 return;
>
> -       ttm_bo_kunmap(&vbo->map);
> -       vbo->map.bo = NULL;
> +       map_count = atomic_dec_return(&vbo->map_count);
> +
> +       if (!map_count) {
> +               ttm_bo_kunmap(&vbo->map);
> +               vbo->map.bo = NULL;
> +       }
>  }
>
>
> @@ -421,6 +429,7 @@ static int vmw_bo_init(struct vmw_private *dev_priv,
>         vmw_bo->tbo.priority = 3;
>         vmw_bo->res_tree = RB_ROOT;
>         xa_init(&vmw_bo->detached_resources);
> +       atomic_set(&vmw_bo->map_count, 0);
>
>         params->size = ALIGN(params->size, PAGE_SIZE);
>         drm_gem_private_object_init(vdev, &vmw_bo->tbo.base, params->size);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> index 62b4342d5f7c..43b5439ec9f7 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> @@ -71,6 +71,8 @@ struct vmw_bo_params {
>   * @map: Kmap object for semi-persistent mappings
>   * @res_tree: RB tree of resources using this buffer object as a backing MOB
>   * @res_prios: Eviction priority counts for attached resources
> + * @map_count: The number of currently active maps. Will differ from the
> + * cpu_writers because it includes kernel maps.
>   * @cpu_writers: Number of synccpu write grabs. Protected by reservation when
>   * increased. May be decreased without reservation.
>   * @dx_query_ctx: DX context if this buffer object is used as a DX query MOB
> @@ -90,6 +92,7 @@ struct vmw_bo {
>         u32 res_prios[TTM_MAX_BO_PRIORITY];
>         struct xarray detached_resources;
>
> +       atomic_t map_count;
>         atomic_t cpu_writers;
>         /* Not ref-counted.  Protected by binding_mutex */
>         struct vmw_resource *dx_query_ctx;
> --
> 2.43.0
>