Re: [PATCH 6/9] drm/vmwgfx: Implement an infrastructure for write-coherent resources
From: Deepak Singh Rawat
Date: Mon Apr 22 2019 - 14:55:10 EST
Hi Thomas,
With minor comments below
Reviewed-by: Deepak Rawat <drawat@xxxxxxxxxx>
On Fri, 2019-04-12 at 09:04 -0700, Thomas Hellstrom wrote:
> This infrastructure will, for coherent resources, make sure that
> from the user-space point of view, data written by the CPU is
> immediately
> automatically available to the GPU at resource validation time.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> ---
> drivers/gpu/drm/vmwgfx/Kconfig | 1 +
> drivers/gpu/drm/vmwgfx/Makefile | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 5 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 26 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 410
> ++++++++++++++++++
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 57 +++
> drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 11 +
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 74 ++++
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 16 +-
> 11 files changed, 588 insertions(+), 20 deletions(-)
> create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
>
> diff --git a/drivers/gpu/drm/vmwgfx/Kconfig
> b/drivers/gpu/drm/vmwgfx/Kconfig
> index 6b28a326f8bb..d5fd81a521f6 100644
> --- a/drivers/gpu/drm/vmwgfx/Kconfig
> +++ b/drivers/gpu/drm/vmwgfx/Kconfig
> @@ -8,6 +8,7 @@ config DRM_VMWGFX
> select FB_CFB_IMAGEBLIT
> select DRM_TTM
> select FB
> + select AS_DIRTY_HELPERS
> # Only needed for the transitional use of drm_crtc_init - can
> be removed
> # again once vmwgfx sets up the primary plane itself.
> select DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/vmwgfx/Makefile
> b/drivers/gpu/drm/vmwgfx/Makefile
> index 8841bd30e1e5..c877a21a0739 100644
> --- a/drivers/gpu/drm/vmwgfx/Makefile
> +++ b/drivers/gpu/drm/vmwgfx/Makefile
> @@ -8,7 +8,7 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o
> vmwgfx_kms.o vmwgfx_drv.o \
> vmwgfx_cmdbuf_res.o vmwgfx_cmdbuf.o vmwgfx_stdu.o \
> vmwgfx_cotable.o vmwgfx_so.o vmwgfx_binding.o vmwgfx_msg.o
> \
> vmwgfx_simple_resource.o vmwgfx_va.o vmwgfx_blit.o \
> - vmwgfx_validation.o \
> + vmwgfx_validation.o vmwgfx_page_dirty.o \
> ttm_object.o ttm_lock.o
>
> obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index c0829d50eecc..90ca866640fe 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -463,6 +463,7 @@ void vmw_bo_bo_free(struct ttm_buffer_object *bo)
> {
> struct vmw_buffer_object *vmw_bo = vmw_buffer_object(bo);
>
> + WARN_ON(vmw_bo->dirty);
> vmw_bo_unmap(vmw_bo);
> kfree(vmw_bo);
> }
> @@ -476,8 +477,10 @@ void vmw_bo_bo_free(struct ttm_buffer_object
> *bo)
> static void vmw_user_bo_destroy(struct ttm_buffer_object *bo)
> {
> struct vmw_user_buffer_object *vmw_user_bo =
> vmw_user_buffer_object(bo);
> + struct vmw_buffer_object *vbo = &vmw_user_bo->vbo;
>
> - vmw_bo_unmap(&vmw_user_bo->vbo);
> + WARN_ON(vbo->dirty);
Is it possible for user-space to exploit this WARN? If yes then you
might want to change the logic?
> + vmw_bo_unmap(vbo);
> ttm_prime_object_kfree(vmw_user_bo, prime);
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 6165fe2c4504..74e94138877e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -857,6 +857,11 @@ static int vmw_driver_load(struct drm_device
> *dev, unsigned long chipset)
> DRM_ERROR("Failed initializing TTM buffer object
> driver.\n");
> goto out_no_bdev;
> }
> + dev_priv->vm_ops = *dev_priv->bdev.vm_ops;
> + dev_priv->vm_ops.fault = vmw_bo_vm_fault;
> + dev_priv->vm_ops.pfn_mkwrite = vmw_bo_vm_mkwrite;
> + dev_priv->vm_ops.page_mkwrite = vmw_bo_vm_mkwrite;
> + dev_priv->bdev.vm_ops = &dev_priv->vm_ops;
>
> /*
> * Enable VRAM, but initially don't use it until SVGA is
> enabled and
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index bd6919b90519..f05fce52fbb4 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -95,6 +95,7 @@ struct vmw_fpriv {
> * @dx_query_ctx: DX context if this buffer object is used as a DX
> query MOB
> * @map: Kmap object for semi-persistent mappings
> * @res_prios: Eviction priority counts for attached resources
> + * @dirty: structure for user-space dirty-tracking
> */
> struct vmw_buffer_object {
> struct ttm_buffer_object base;
> @@ -105,6 +106,7 @@ struct vmw_buffer_object {
> /* Protected by reservation */
> struct ttm_bo_kmap_obj map;
> u32 res_prios[TTM_MAX_BO_PRIORITY];
> + struct vmw_bo_dirty *dirty;
> };
>
> /**
> @@ -135,7 +137,8 @@ struct vmw_res_func;
> * @res_dirty: Resource contains data not yet in the backup buffer.
> Protected
> * by resource reserved.
> * @backup_dirty: Backup buffer contains data not yet in the HW
> resource.
> - * Protecte by resource reserved.
> + * Protected by resource reserved.
> + * @coherent: Emulate coherency by tracking vm accesses.
> * @backup: The backup buffer if any. Protected by resource
> reserved.
> * @backup_offset: Offset into the backup buffer if any. Protected
> by resource
> * reserved. Note that only a few resource types can have a
> @backup_offset
> @@ -152,14 +155,16 @@ struct vmw_res_func;
> * @hw_destroy: Callback to destroy the resource on the device, as
> part of
> * resource destruction.
> */
> +struct vmw_resource_dirty;
> struct vmw_resource {
> struct kref kref;
> struct vmw_private *dev_priv;
> int id;
> u32 used_prio;
> unsigned long backup_size;
> - bool res_dirty;
> - bool backup_dirty;
> + u32 res_dirty : 1;
> + u32 backup_dirty : 1;
Is there a reason you changed res_dirty and backup_dirty from bool to
u32. They are still areused as bool, right?
> + u32 coherent : 1;
> struct vmw_buffer_object *backup;
> unsigned long backup_offset;
> unsigned long pin_count;
> @@ -167,6 +172,7 @@ struct vmw_resource {
> struct list_head lru_head;
> struct list_head mob_head;
> struct list_head binding_head;
> + struct vmw_resource_dirty *dirty;
> void (*res_free) (struct vmw_resource *res);
> void (*hw_destroy) (struct vmw_resource *res);
> };
> @@ -607,6 +613,9 @@ struct vmw_private {
>
> /* Validation memory reservation */
> struct vmw_validation_mem vvm;
> +
> + /* VM operations */
> + struct vm_operations_struct vm_ops;
> };
>
> static inline struct vmw_surface *vmw_res_to_srf(struct vmw_resource
> *res)
> @@ -723,6 +732,8 @@ extern void vmw_resource_evict_all(struct
> vmw_private *dev_priv);
> extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo);
> void vmw_resource_mob_attach(struct vmw_resource *res);
> void vmw_resource_mob_detach(struct vmw_resource *res);
> +void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t
> start,
> + pgoff_t end);
>
> /**
> * vmw_resource_mob_attached - Whether a resource currently has a
> mob attached
> @@ -1411,6 +1422,15 @@ int vmw_host_log(const char *log);
> #define VMW_DEBUG_USER(fmt,
> ...) \
> DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__)
>
> +/* Resource dirtying - vmwgfx_page_dirty.c */
> +void vmw_bo_dirty_scan(struct vmw_buffer_object *vbo);
> +int vmw_bo_dirty_add(struct vmw_buffer_object *vbo);
> +void vmw_bo_dirty_transfer_to_res(struct vmw_resource *res);
> +void vmw_bo_dirty_clear_res(struct vmw_resource *res);
> +void vmw_bo_dirty_release(struct vmw_buffer_object *vbo);
> +vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf);
> +vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf);
> +
> /**
> * Inline helper functions
> */
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index da3ac0bc2e14..7cb22119f516 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -2483,7 +2483,6 @@ static int vmw_cmd_dx_check_subresource(struct
> vmw_private *dev_priv,
> offsetof(typeof(*cmd), sid));
>
> cmd = container_of(header, typeof(*cmd), header);
> -
> return vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
> VMW_RES_DIRTY_NONE,
> user_surface_converter,
> &cmd->sid, NULL);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> new file mode 100644
> index 000000000000..87e4a73b1175
> --- /dev/null
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*******************************************************************
> *******
> + *
> + * Copyright 2019 VMware, Inc., Palo Alto, CA., USA
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction,
> including
> + * without limitation the rights to use, copy, modify, merge,
> publish,
> + * distribute, sub license, and/or sell copies of the Software, and
> to
> + * permit persons to whom the Software is furnished to do so,
> subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the
> + * next paragraph) shall be included in all copies or substantial
> portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
> EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR
> ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> +
> *********************************************************************
> *****/
> +#include "vmwgfx_drv.h"
> +
> +/*
> + * Different methods for tracking dirty:
> + * VMW_BO_DIRTY_PAGETABLE - Scan the pagetable for hardware dirty
> bits
> + * VMW_BO_DIRTY_MKWRITE - Write-protect page table entries and
> record write-
> + * accesses in the VM mkwrite() callback
> + */
> +enum vmw_bo_dirty_method {
> + VMW_BO_DIRTY_PAGETABLE,
> + VMW_BO_DIRTY_MKWRITE,
> +};
> +
> +/*
> + * No dirtied pages at scan trigger a transition to the _MKWRITE
> method,
> + * similarly a certain percentage of dirty pages trigger a
> transition to
> + * the _PAGETABLE method. How many triggers should we wait for
> before
> + * changing method?
> + */
> +#define VMW_DIRTY_NUM_CHANGE_TRIGGERS 2
> +
> +/* Percentage to trigger a transition to the _PAGETABLE method */
> +#define VMW_DIRTY_PERCENTAGE 10
> +
> +/**
> + * struct vmw_bo_dirty - Dirty information for buffer objects
> + * @start: First currently dirty bit
> + * @end: Last currently dirty bit + 1
> + * @method: The currently used dirty method
> + * @change_count: Number of consecutive method change triggers
> + * @ref_count: Reference count for this structure
> + * @bitmap_size: The size of the bitmap in bits. Typically equal to
> the
> + * nuber of pages in the bo.
> + * @size: The accounting size for this struct.
> + * @bitmap: A bitmap where each bit represents a page. A set bit
> means a
> + * dirty page.
> + */
> +struct vmw_bo_dirty {
> + unsigned long start;
> + unsigned long end;
> + enum vmw_bo_dirty_method method;
> + unsigned int change_count;
> + unsigned int ref_count;
> + unsigned long bitmap_size;
> + size_t size;
> + unsigned long bitmap[0];
> +};
> +
> +/**
> + * vmw_bo_dirty_scan_pagetable - Perform a pagetable scan for dirty
> bits
> + * @vbo: The buffer object to scan
> + *
> + * Scans the pagetable for dirty bits. Clear those bits and modify
> the
> + * dirty structure with the results. This function may change the
> + * dirty-tracking method.
> + */
> +static void vmw_bo_dirty_scan_pagetable(struct vmw_buffer_object
> *vbo)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> + pgoff_t offset = drm_vma_node_start(&vbo->base.vma_node);
> + struct address_space *mapping = vbo->base.bdev->dev_mapping;
> + pgoff_t num_marked;
> +
> + num_marked = apply_as_clean(mapping,
> + offset, dirty->bitmap_size,
> + offset, &dirty->bitmap[0],
> + &dirty->start, &dirty->end);
> + if (num_marked == 0)
> + dirty->change_count++;
> + else
> + dirty->change_count = 0;
> +
> + if (dirty->change_count > VMW_DIRTY_NUM_CHANGE_TRIGGERS) {
> + dirty->change_count = 0;
> + dirty->method = VMW_BO_DIRTY_MKWRITE;
> + apply_as_wrprotect(mapping,
> + offset, dirty->bitmap_size);
> + apply_as_clean(mapping,
> + offset, dirty->bitmap_size,
> + offset, &dirty->bitmap[0],
> + &dirty->start, &dirty->end);
> + }
> +}
> +
> +/**
> + * vmw_bo_dirty_scan_mkwrite - Reset the mkwrite dirty-tracking
> method
> + * @vbo: The buffer object to scan
> + *
> + * Write-protect pages written to so that consecutive write accesses
> will
> + * trigger a call to mkwrite.
> + *
> + * This function may change the dirty-tracking method.
> + */
> +static void vmw_bo_dirty_scan_mkwrite(struct vmw_buffer_object *vbo)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> + unsigned long offset = drm_vma_node_start(&vbo->base.vma_node);
> + struct address_space *mapping = vbo->base.bdev->dev_mapping;
> + pgoff_t num_marked;
> +
> + if (dirty->end <= dirty->start)
> + return;
> +
> + num_marked = apply_as_wrprotect(vbo->base.bdev->dev_mapping,
> + dirty->start + offset,
> + dirty->end - dirty->start);
> +
> + if (100UL * num_marked / dirty->bitmap_size >
> + VMW_DIRTY_PERCENTAGE) {
> + dirty->change_count++;
> + } else {
> + dirty->change_count = 0;
> + }
> +
> + if (dirty->change_count > VMW_DIRTY_NUM_CHANGE_TRIGGERS) {
> + pgoff_t start = 0;
> + pgoff_t end = dirty->bitmap_size;
> +
> + dirty->method = VMW_BO_DIRTY_PAGETABLE;
> + apply_as_clean(mapping, offset, end, offset, &dirty-
> >bitmap[0],
> + &start, &end);
> + bitmap_clear(&dirty->bitmap[0], 0, dirty->bitmap_size);
> + if (dirty->start < dirty->end)
> + bitmap_set(&dirty->bitmap[0], dirty->start,
> + dirty->end - dirty->start);
> + dirty->change_count = 0;
> + }
> +}
> +
> +
> +/**
> + * vmw_bo_dirty_scan - Scan for dirty pages and add them to the
> dirty
> + * tracking structure
> + * @vbo: The buffer object to scan
> + *
> + * This function may change the dirty tracking method.
> + */
> +void vmw_bo_dirty_scan(struct vmw_buffer_object *vbo)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> + if (dirty->method == VMW_BO_DIRTY_PAGETABLE)
> + vmw_bo_dirty_scan_pagetable(vbo);
> + else
> + vmw_bo_dirty_scan_mkwrite(vbo);
> +}
> +
> +/**
> + * vmw_bo_dirty_add - Add a dirty-tracking user to a buffer object
> + * @vbo: The buffer object
> + *
> + * This function registers a dirty-tracking user to a buffer object.
> + * A user can be for example a resource or a vma in a special user-
> space
> + * mapping.
> + *
> + * Return: Zero on success, -ENOMEM on memory allocation failure.
> + */
> +int vmw_bo_dirty_add(struct vmw_buffer_object *vbo)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> + pgoff_t num_pages = vbo->base.num_pages;
> + size_t size, acc_size;
> + int ret;
> + static struct ttm_operation_ctx ctx = {
> + .interruptible = false,
> + .no_wait_gpu = false
> + };
> +
> + if (dirty) {
> + dirty->ref_count++;
> + return 0;
> + }
> +
> + size = sizeof(*dirty) + BITS_TO_LONGS(num_pages) *
> sizeof(long);
> + acc_size = ttm_round_pot(size);
> + ret = ttm_mem_global_alloc(&ttm_mem_glob, acc_size, &ctx);
> + if (ret) {
> + VMW_DEBUG_USER("Out of graphics memory for buffer
> object "
> + "dirty tracker.\n");
> + return ret;
> + }
> + dirty = kvzalloc(size, GFP_KERNEL);
> + if (!dirty) {
> + ret = -ENOMEM;
> + goto out_no_dirty;
> + }
> +
> + dirty->size = acc_size;
> + dirty->bitmap_size = num_pages;
> + dirty->start = dirty->bitmap_size;
> + dirty->end = 0;
> + dirty->ref_count = 1;
> + if (num_pages < PAGE_SIZE / sizeof(pte_t)) {
> + dirty->method = VMW_BO_DIRTY_PAGETABLE;
> + } else {
> + struct address_space *mapping = vbo->base.bdev-
> >dev_mapping;
> + pgoff_t offset = drm_vma_node_start(&vbo-
> >base.vma_node);
> +
> + dirty->method = VMW_BO_DIRTY_MKWRITE;
> +
> + /* Write-protect and then pick up already dirty bits */
> + apply_as_wrprotect(mapping, offset, num_pages);
> + apply_as_clean(mapping, offset, num_pages, offset,
> + &dirty->bitmap[0], &dirty->start,
> &dirty->end);
> + }
> +
> + vbo->dirty = dirty;
> +
> + return 0;
> +
> +out_no_dirty:
> + ttm_mem_global_free(&ttm_mem_glob, acc_size);
> + return ret;
> +}
> +
> +/**
> + * vmw_bo_dirty_release - Release a dirty-tracking user from a
> buffer object
> + * @vbo: The buffer object
> + *
> + * This function releases a dirty-tracking user from a buffer
> object.
> + * If the reference count reaches zero, then the dirty-tracking
> object is
> + * freed and the pointer to it cleared.
> + *
> + * Return: Zero on success, -ENOMEM on memory allocation failure.
> + */
> +void vmw_bo_dirty_release(struct vmw_buffer_object *vbo)
> +{
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> + if (dirty && --dirty->ref_count == 0) {
> + size_t acc_size = dirty->size;
> +
> + kvfree(dirty);
> + ttm_mem_global_free(&ttm_mem_glob, acc_size);
> + vbo->dirty = NULL;
> + }
> +}
> +
> +/**
> + * vmw_bo_dirty_transfer_to_res - Pick up a resource's dirty region
> from
> + * its backing mob.
> + * @res: The resource
> + *
> + * This function will pick up all dirty ranges affecting the
> resource from
> + * it's backup mob, and call vmw_resource_dirty_update() once for
> each
> + * range. The transferred ranges will be cleared from the backing
> mob's
> + * dirty tracking.
> + */
> +void vmw_bo_dirty_transfer_to_res(struct vmw_resource *res)
> +{
> + struct vmw_buffer_object *vbo = res->backup;
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> + pgoff_t start, cur, end;
> + unsigned long res_start = res->backup_offset;
> + unsigned long res_end = res->backup_offset + res->backup_size;
> +
> + WARN_ON_ONCE(res_start & ~PAGE_MASK);
> + res_start >>= PAGE_SHIFT;
> + res_end = DIV_ROUND_UP(res_end, PAGE_SIZE);
> +
> + if (res_start >= dirty->end || res_end <= dirty->start)
> + return;
> +
> + cur = max(res_start, dirty->start);
> + res_end = max(res_end, dirty->end);
> + while (cur < res_end) {
> + unsigned long num;
> +
> + start = find_next_bit(&dirty->bitmap[0], res_end, cur);
> + if (start >= res_end)
> + break;
> +
> + end = find_next_zero_bit(&dirty->bitmap[0], res_end,
> start + 1);
> + cur = end + 1;
> + num = end - start;
> + bitmap_clear(&dirty->bitmap[0], start, num);
> + vmw_resource_dirty_update(res, start, end);
> + }
> +
> + if (res_start <= dirty->start && res_end > dirty->start)
> + dirty->start = res_end;
> + if (res_start < dirty->end && res_end >= dirty->end)
> + dirty->end = res_start;
> +}
> +
> +/**
> + * vmw_bo_dirty_clear_res - Clear a resource's dirty region from
> + * its backing mob.
> + * @res: The resource
> + *
> + * This function will clar all dirty ranges affecting the resource
s/clar/clear
> from
> + * it's backup mob's dirty tracking.
> + */
> +void vmw_bo_dirty_clear_res(struct vmw_resource *res)
> +{
> + unsigned long res_start = res->backup_offset;
> + unsigned long res_end = res->backup_offset + res->backup_size;
> + struct vmw_buffer_object *vbo = res->backup;
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> + res_start >>= PAGE_SHIFT;
> + res_end = DIV_ROUND_UP(res_end, PAGE_SIZE);
> +
> + if (res_start >= dirty->end || res_end <= dirty->start)
> + return;
> +
> + res_start = max(res_start, dirty->start);
> + res_end = min(res_end, dirty->end);
> + bitmap_clear(&dirty->bitmap[0], res_start, res_end -
> res_start);
> +
> + if (res_start <= dirty->start && res_end > dirty->start)
> + dirty->start = res_end;
> + if (res_start < dirty->end && res_end >= dirty->end)
> + dirty->end = res_start;
> +}
> +
> +/* vmw_bo_vm_mkwrite - The vmwgfx page_mkwrite() or pfn_mkwrite()
> callback */
Function documentation format, IMO since this is callback you can get
rid of the above line, same for vmw_bo_vm_fault.
> +vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
> +{
> + struct vm_area_struct *vma = vmf->vma;
> + struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
> + vma->vm_private_data;
> + vm_fault_t ret;
> + unsigned long page_offset;
> + struct vmw_buffer_object *vbo =
> + container_of(bo, typeof(*vbo), base);
> +
> + ret = ttm_bo_vm_reserve(bo, vmf);
> + if (ret)
> + return ret;
> +
> + page_offset = vmf->pgoff - drm_vma_node_start(&bo->vma_node);
> + if (unlikely(page_offset >= bo->num_pages)) {
> + ret = VM_FAULT_SIGBUS;
> + goto out_unlock;
> + }
> +
> + if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> + !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> + __set_bit(page_offset, &dirty->bitmap[0]);
> + dirty->start = min(dirty->start, page_offset);
> + dirty->end = max(dirty->end, page_offset + 1);
> + }
> +
> +out_unlock:
> + reservation_object_unlock(bo->resv);
> + return ret;
> +}
> +
> +
> +/* vmw_bo_vm_fault - The vmwgfx fault() callback */
> +vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
> +{
> + struct vm_area_struct *vma = vmf->vma;
> + struct vm_area_struct cvma = *vma;
> + struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
> + vma->vm_private_data;
> + struct vmw_buffer_object *vbo =
> + container_of(bo, struct vmw_buffer_object, base);
> + pgoff_t num_prefault;
> + vm_fault_t ret;
> +
> + ret = ttm_bo_vm_reserve(bo, vmf);
> + if (ret)
> + return ret;
> +
> + /*
> + * This will cause mkwrite() to be called for each pte on
> + * write-enable vmas.
> + */
> + if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE)
> + cvma.vm_flags &= ~VM_WRITE;
> +
> + num_prefault = (vma->vm_flags & VM_RAND_READ) ? 0 :
> + TTM_BO_VM_NUM_PREFAULT;
> + ret = ttm_bo_vm_fault_reserved(vmf, &cvma, num_prefault);
> + if (ret == VM_FAULT_RETRY && !(vmf->flags &
> FAULT_FLAG_RETRY_NOWAIT))
> + return ret;
> +
> + reservation_object_unlock(bo->resv);
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index 88981c4dbae3..d35f4bd32cd9 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -121,6 +121,10 @@ static void vmw_resource_release(struct kref
> *kref)
> }
> res->backup_dirty = false;
> vmw_resource_mob_detach(res);
> + if (res->dirty)
> + res->func->dirty_free(res);
> + if (res->coherent)
> + vmw_bo_dirty_release(res->backup);
> ttm_bo_unreserve(bo);
> vmw_bo_unreference(&res->backup);
> }
> @@ -210,7 +214,9 @@ int vmw_resource_init(struct vmw_private
> *dev_priv, struct vmw_resource *res,
> res->backup_offset = 0;
> res->backup_dirty = false;
> res->res_dirty = false;
> + res->coherent = false;
> res->used_prio = 3;
> + res->dirty = NULL;
> if (delay_id)
> return 0;
> else
> @@ -397,6 +403,30 @@ static int vmw_resource_do_validate(struct
> vmw_resource *res,
> vmw_resource_mob_attach(res);
> }
>
> + /*
> + * Handle the case where the backup mob is marked coherent but
> + * the resource isn't.
> + */
> + if (func->dirty_alloc && vmw_resource_mob_attached(res) &&
> + !res->coherent) {
> + if (res->backup->dirty && !res->dirty) {
> + ret = func->dirty_alloc(res);
> + if (ret)
> + return ret;
> + } else if (!res->backup->dirty && res->dirty) {
> + func->dirty_free(res);
> + }
> + }
> +
> + /*
> + * Transfer the dirty regions to the resource and update
> + * the resource.
> + */
> + if (res->dirty) {
> + vmw_bo_dirty_transfer_to_res(res);
> + return func->dirty_sync(res);
> + }
> +
> return 0;
>
> out_bind_failed:
> @@ -435,16 +465,28 @@ void vmw_resource_unreserve(struct vmw_resource
> *res,
> if (switch_backup && new_backup != res->backup) {
> if (res->backup) {
> vmw_resource_mob_detach(res);
> + if (res->coherent)
> + vmw_bo_dirty_release(res->backup);
> vmw_bo_unreference(&res->backup);
> }
>
> if (new_backup) {
> res->backup = vmw_bo_reference(new_backup);
> +
> + /*
> + * The validation code should already have
> added a
> + * dirty tracker here.
> + */
> + WARN_ON(res->coherent && !new_backup->dirty);
> +
> vmw_resource_mob_attach(res);
> } else {
> res->backup = NULL;
> }
> + } else if (switch_backup && res->coherent) {
> + vmw_bo_dirty_release(res->backup);
> }
> +
> if (switch_backup)
> res->backup_offset = new_backup_offset;
>
> @@ -1009,3 +1051,18 @@ enum vmw_res_type vmw_res_type(const struct
> vmw_resource *res)
> {
> return res->func->res_type;
> }
> +
> +/**
> + * vmw_resource_update_dirty - Update a resource's dirty tracker
> with a
> + * sequential range of touched backing store memory.
> + * @res: The resource.
> + * @start: The first page touched.
> + * @end: The last page touched + 1.
> + */
> +void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t
> start,
> + pgoff_t end)
> +{
> + if (res->dirty)
> + res->func->dirty_range_add(res, start << PAGE_SHIFT,
> + end << PAGE_SHIFT);
> +}
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
> index 984e588c62ca..c85144286cfe 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
> @@ -71,6 +71,12 @@ struct vmw_user_resource_conv {
> * @commit_notify: If the resource is a command buffer managed
> resource,
> * callback to notify that a define or remove
> command
> * has been committed to the device.
> + * @dirty_alloc: Allocate a dirty tracker. NULL if dirty-
> tracking is not
> + * supported.
> + * @dirty_free: Free the dirty tracker.
> + * @dirty_sync: Upload the dirty mob contents to the
> resource.
> + * @dirty_add_range: Add a sequential dirty range to the resource
> + * dirty tracker.
> */
> struct vmw_res_func {
> enum vmw_res_type res_type;
> @@ -90,6 +96,11 @@ struct vmw_res_func {
> struct ttm_validate_buffer *val_buf);
> void (*commit_notify)(struct vmw_resource *res,
> enum vmw_cmdbuf_res_state state);
> + int (*dirty_alloc)(struct vmw_resource *res);
> + void (*dirty_free)(struct vmw_resource *res);
> + int (*dirty_sync)(struct vmw_resource *res);
> + void (*dirty_range_add)(struct vmw_resource *res, size_t start,
> + size_t end);
> };
>
> /**
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c
> index f611b2290a1b..5b0c928bb5ba 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c
> @@ -33,8 +33,13 @@
> * struct vmw_validation_bo_node - Buffer object validation
> metadata.
> * @base: Metadata used for TTM reservation- and validation.
> * @hash: A hash entry used for the duplicate detection hash table.
> + * @coherent_count: If switching backup buffers, number of new
> coherent
> + * resources that will have this buffer as a backup buffer.
> * @as_mob: Validate as mob.
> * @cpu_blit: Validate for cpu blit access.
> + * @coherent_alloced: In switching backup buffers for coherent
> resources:
I don't see the field below, remove this.
> + * The bo dirty tracker has been allocated and needs to be freed if
> + * reverting.
> *
> * Bit fields are used since these structures are allocated and
> freed in
> * large numbers and space conservation is desired.
> @@ -42,6 +47,7 @@
> struct vmw_validation_bo_node {
> struct ttm_validate_buffer base;
> struct drm_hash_item hash;
> + unsigned int coherent_count;
> u32 as_mob : 1;
> u32 cpu_blit : 1;
> };
> @@ -459,6 +465,19 @@ int vmw_validation_res_reserve(struct
> vmw_validation_context *ctx,
> if (ret)
> goto out_unreserve;
> }
> +
> + if (val->switching_backup && val->new_backup &&
> + res->coherent) {
> + struct vmw_validation_bo_node *bo_node =
> + vmw_validation_find_bo_dup(ctx,
> + val-
> >new_backup);
> +
> + if (WARN_ON(!bo_node)) {
> + ret = -EINVAL;
> + goto out_unreserve;
> + }
> + bo_node->coherent_count++;
> + }
> }
>
> return 0;
> @@ -562,6 +581,9 @@ int vmw_validation_bo_validate(struct
> vmw_validation_context *ctx, bool intr)
> int ret;
>
> list_for_each_entry(entry, &ctx->bo_list, base.head) {
> + struct vmw_buffer_object *vbo =
> + container_of(entry->base.bo, typeof(*vbo),
> base);
> +
> if (entry->cpu_blit) {
> struct ttm_operation_ctx ctx = {
> .interruptible = intr,
> @@ -576,6 +598,27 @@ int vmw_validation_bo_validate(struct
> vmw_validation_context *ctx, bool intr)
> }
> if (ret)
> return ret;
> +
> + /*
> + * Rather than having the resource code allocating the
> bo
> + * dirty tracker in resource_unreserve() where we can't
> fail,
> + * Do it here when validating the buffer object.
> + */
> + if (entry->coherent_count) {
> + unsigned int coherent_count = entry-
> >coherent_count;
> +
> + while (coherent_count) {
> + ret = vmw_bo_dirty_add(vbo);
> + if (ret)
> + return ret;
> +
> + coherent_count--;
> + }
> + entry->coherent_count -= coherent_count;
> + }
> +
> + if (vbo->dirty)
> + vmw_bo_dirty_scan(vbo);
> }
> return 0;
> }
> @@ -828,3 +871,34 @@ int vmw_validation_preload_res(struct
> vmw_validation_context *ctx,
> ctx->mem_size_left += size;
> return 0;
> }
> +
> +/**
> + * vmw_validation_bo_backoff - Unreserve buffer objects registered
> with a
> + * validation context
> + * @ctx: The validation context
> + *
> + * This function unreserves the buffer objects previously reserved
> using
> + * vmw_validation_bo_reserve. It's typically used as part of an
> error path
> + */
> +void vmw_validation_bo_backoff(struct vmw_validation_context *ctx)
> +{
> + struct vmw_validation_bo_node *entry;
> +
> + /*
> + * Switching coherent resource backup buffers failed.
> + * Release corresponding buffer object dirty trackers.
> + */
> + list_for_each_entry(entry, &ctx->bo_list, base.head) {
> + if (entry->coherent_count) {
> + unsigned int coherent_count = entry-
> >coherent_count;
> + struct vmw_buffer_object *vbo =
> + container_of(entry->base.bo,
> typeof(*vbo),
> + base);
> +
> + while (coherent_count--)
> + vmw_bo_dirty_release(vbo);
> + }
> + }
> +
> + ttm_eu_backoff_reservation(&ctx->ticket, &ctx->bo_list);
> +}
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h
> index 523f6ac5c335..058c7f2fbf83 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h
> @@ -172,20 +172,6 @@ vmw_validation_bo_reserve(struct
> vmw_validation_context *ctx,
> NULL);
> }
>
> -/**
> - * vmw_validation_bo_backoff - Unreserve buffer objects registered
> with a
> - * validation context
> - * @ctx: The validation context
> - *
> - * This function unreserves the buffer objects previously reserved
> using
> - * vmw_validation_bo_reserve. It's typically used as part of an
> error path
> - */
> -static inline void
> -vmw_validation_bo_backoff(struct vmw_validation_context *ctx)
> -{
> - ttm_eu_backoff_reservation(&ctx->ticket, &ctx->bo_list);
> -}
> -
> /**
> * vmw_validation_bo_fence - Unreserve and fence buffer objects
> registered
> * with a validation context
> @@ -268,4 +254,6 @@ int vmw_validation_preload_res(struct
> vmw_validation_context *ctx,
> unsigned int size);
> void vmw_validation_res_set_dirty(struct vmw_validation_context
> *ctx,
> void *val_private, u32 dirty);
> +void vmw_validation_bo_backoff(struct vmw_validation_context *ctx);
> +
> #endif
> --
> 2.20.1
>