[PATCH 00/19] shadow page table support V2

[Christian König]

Again patch #7 looks like an unrelated fix to me. Please split that from 
the patch set, add my rb and commit it preliminary.

Apart from that patch #1:
> +	amdgpu_ttm_placement_init(adev, &placement,
> +				  placements, AMDGPU_GEM_DOMAIN_GTT,
> +				  AMDGPU_GEM_CREATE_CPU_GTT_USWC);
> +
> +	return amdgpu_bo_create_restricted(adev, size, byte_align, true,
> +					   AMDGPU_GEM_DOMAIN_GTT,
> +					   AMDGPU_GEM_CREATE_CPU_GTT_USWC,
> +					   NULL, &placement,
> +					   bo->tbo.resv,
> +					   &bo->shadow);
You need to set bo->shadow->parent to the parent BO when you use the 
reservation object here. See the VM code on how to do this, otherwise 
TTM could free the parent reservation object first and the crash when it 
wants to free the shadow.

Additional to that do we really need the placement here? That looks 
quite odd.

Patch #2, #3 is Reviewed-by: Christian König <christian.koenig at amd.com>.

Patch #4:
> +enum amdgpu_shadow_flag {
> +	AMDGPU_SHADOW_FLAG_SYNC_TO_NONE = 0,
> +	AMDGPU_SHADOW_FLAG_SYNC_TO_PARENT,
> +	AMDGPU_SHADOW_FLAG_SYNC_TO_SHADOW,
> +};
Either use defines here.

> +	/* indicate if need to sync between bo and shadow */
> +	u32                             shadow_flag;
Or the named enum here.

I would use the named enum, cuase it doesn't make sense to combine them 
as flags. And renaming it to something like "backup_shadow" would 
probably make sense as well.

Patch #5:
> +	r = amdgpu_bo_pin(bo, bo->prefered_domains, &bo_addr);
> +	if (r) {
> +		DRM_ERROR("Failed to pin bo object\n");
> +		goto err1;
> +	}
> +	r = amdgpu_bo_pin(bo->shadow, bo->shadow->prefered_domains, &shadow_addr);
> +	if (r) {
> +		DRM_ERROR("Failed to pin bo shadow object\n");
> +		goto err2;
> +	}
Don't use pin here, just use amdgpu_bo_offset when you need the offset.

I need to work on the S3 issue again now, going to come back to this 
patch set when I have more time.

Regards,
Christian.

Am 05.08.2016 um 11:38 schrieb Chunming Zhou:
> Since we cannot make sure VRAM is safe after gpu reset, page table backup
> is neccessary, shadow page table is sense way to recovery page talbe when
> gpu reset happens.
> We need to allocate GTT bo as the shadow of VRAM bo when creating page table,
> and make them same. After gpu reset, we will need to use SDMA to copy GTT bo
> content to VRAM bo, then page table will be recoveried.
>
> V2:
> Shadow bo uses a shadow entity running on normal run queue, after gpu reset,
> we need to wait for all shadow jobs finished first, then recovery page table from shadow.
>
> Chunming Zhou (19):
>    drm/amdgpu: add shadow bo support
>    drm/amdgpu: validate shadow as well when validating bo
>    drm/amdgpu: allocate shadow for pd/pt bo
>    drm/amdgpu: add shadow flag
>    drm/amdgpu: sync bo and shadow
>    drm/amdgpu: implement vm recovery function from shadow
>    drm/amdgpu: fix vm init error path
>    drm/amdgpu: add shadow_entity for shadow page table updates
>    drm/amdgpu: update pd shadow bo
>    drm/amdgpu: update pt shadow
>    drm/amd: add last fence in sched entity
>    drm/amdgpu: link all vm clients
>    drm/amdgpu: add vm_list_lock
>    drm/amd: add block entity function
>    drm/amdgpu: add shadow fence owner
>    drm/amd: block entity
>    drm/amdgpu: recover page tables after gpu reset
>    drm/amdgpu: add need backup function
>    drm/amdgpu: add backup condition for vm
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  25 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  82 +++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    |  88 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    | 104 +++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h    |   5 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c      |   3 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 270 +++++++++++++++++++-------
>   drivers/gpu/drm/amd/scheduler/gpu_scheduler.c |  38 +++-
>   drivers/gpu/drm/amd/scheduler/gpu_scheduler.h |   5 +
>   include/uapi/drm/amdgpu_drm.h                 |   2 +
>   10 files changed, 519 insertions(+), 103 deletions(-)
>