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

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 29.1.2024 13.33, Matthew Auld wrote:
> 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?

Hi Matthew, thanks for the comments. I went ahead with making 
has_sealed_pat_index and it did make things much more clean. It's good 
idea, thanks. I'll soon send new version. Here I didn't go limit this to 
xe2 as the limit is coming from framebuffer code, if there come other 
use for this pat index sealing it doesn't need to be about xe2 on this part.

> 
>> +
>>       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);