On Thu, Aug 15, 2024 at 6:34 PM Zack Rusin <zack.rusin@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.19+
> Signed-off-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> ---
> 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, ¬_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
>
LGTM.
Reviewed-by: Martin Krastev <martin.krastev@xxxxxxxxxxxx>
Regards,
Martin
