Re: [PATCH 3/5] drm/xe: store bind time pat index to xe_bo

[Matthew Auld <matthew.auld@intel.com>]

On 26/01/2024 21:08, Juha-Pekka Heikkila wrote:
> Store pat index from xe_vma to xe_bo and check if bo was pinned
> as framebuffer and verify pat index is not changing for pinned
> framebuffers.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>   drivers/gpu/drm/xe/xe_pt.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index de1030a47588..0a5d7c7543b1 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -1208,10 +1208,11 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>   	struct dma_fence *fence;
>   	struct invalidation_fence *ifence = NULL;
>   	struct xe_range_fence *rfence;
> +	struct xe_bo *bo = xe_vma_bo(vma);
>   	int err;
>   
>   	bind_pt_update.locked = false;
> -	xe_bo_assert_held(xe_vma_bo(vma));
> +	xe_bo_assert_held(bo);
>   	xe_vm_assert_held(vm);
>   
>   	vm_dbg(&xe_vma_vm(vma)->xe->drm,
> @@ -1252,8 +1253,22 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
> +	/*
> +	 * BO which has XE_BO_SCANOUT_BIT set and was pinned as framebuffer
> +	 * before with different PAT index cannot be bound with different PAT
> +	 * index. This is to prevent switching CCS on/off from framebuffers
> +	 * on the fly.
> +	 */
> +	if (bo) {
> +		if (bo->flags & XE_BO_SCANOUT_BIT && bo->pat_index_scanout &&

Note that pat_index = 0 is usually a valid index...

> +		    bo->pat_index_scanout != vma->pat_index)
> +			return ERR_PTR(-EINVAL);
> +
> +		bo->pat_index = vma->pat_index;
> +	}

...what about something like:

if (bo.has_sealed_pat_index && bo.sealed_pat_index != vma->pat_index)
     return ERR_PTR();
else if (SCANOUT) {
     bo.has_sealed_pat_index = true;
     bo.sealed_pat_index = vma->pat_index;
}

Also, this and the previous patch should probably be squashed together? 
Other question is if we should only apply this on xe2?

> +
>   	fence = xe_migrate_update_pgtables(tile->migrate,
> -					   vm, xe_vma_bo(vma), q,
> +					   vm, bo, q,
>   					   entries, num_entries,
>   					   syncs, num_syncs,
>   					   &bind_pt_update.base);
> @@ -1287,8 +1302,8 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_exec_queue
>   				   DMA_RESV_USAGE_KERNEL :
>   				   DMA_RESV_USAGE_BOOKKEEP);
>   
> -		if (!xe_vma_has_no_bo(vma) && !xe_vma_bo(vma)->vm)
> -			dma_resv_add_fence(xe_vma_bo(vma)->ttm.base.resv, fence,
> +		if (!xe_vma_has_no_bo(vma) && !bo->vm)
> +			dma_resv_add_fence(bo->ttm.base.resv, fence,
>   					   DMA_RESV_USAGE_BOOKKEEP);
>   		xe_pt_commit_bind(vma, entries, num_entries, rebind,
>   				  bind_pt_update.locked ? &deferred : NULL);