Re: [PATCH 2/4] drm/xe: store bind time pat index to xe_bo

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

On 22/01/2024 18:26, Juha-Pekka Heikkila wrote:
> Hi Matthew, thanks for looking into these. Below few thoughts.
> 
> On 19.1.2024 17.45, Matthew Auld wrote:
>> On 18/01/2024 15:27, Juha-Pekka Heikkila wrote:
>>> Store pat index from xe_vma to xe_bo
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_pt.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>>> index de1030a47588..4b76db698878 100644
>>> --- a/drivers/gpu/drm/xe/xe_pt.c
>>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>>> @@ -1252,6 +1252,10 @@ __xe_pt_bind_vma(struct xe_tile *tile, struct 
>>> xe_vma *vma, struct xe_exec_queue
>>>           return ERR_PTR(-ENOMEM);
>>>       }
>>> +    if (xe_vma_bo(vma)) {
>>> +        xe_vma_bo(vma)->pat_index = vma->pat_index;
>>
>> Multiple mappings will trash this I think. Is that OK for your 
>> usecase? It can be useful to map the same resource as compressed and 
>> uncompressed to facilitate in-place decompression/compression.
> 
> On i915 I think we did map framebuffers only once and did stay with it 
> until fb was destroyed. XE_BO_SCANOUT_BIT is for buffers that are meant 
> to be framebuffers? I could make it so pat index given first is not 
> allowed to change for buffers with this bit set?

Yeah, sealing the pat_index for such objects might be the simplest option.

> 
>>
>> Also would be good to be clear about what happens if the KMD doesn't 
>> do anything to prevent compression with non-tile4? Is it just a bit of 
>> display corruption or something much worse that we need to prevent? Is 
>> this just a best effort check to help userspace? Otherwise it is hard 
>> to evaluate how solid we need to be here in our checking to prevent 
>> this scenario. For example how is binding vs display races handled? 
>> What happens if the bind appears after the display check?
> 
> For what happen with incorrect buffers going for display I've seen they 
> are corrupted on screen but my testing is very minimal. On bspec 67158 
> it just said linear and tile X formats are not supported with 
> decompression on display, so it is broken config. Couldn't say generally 
> how robust display hw is for broken configs. I remember Ville had found 
> with TGL broken configs caused unrecoverable issues which followed ccs 
> getting blocked on some steppings because it was only way to block 
> broken config Ville found. I'll add Ville here on cc if he has views on 
> this what's needed here for Xe2.
> 
> /Juha-Pekka
> 
>>
>>> +    }
>>> +
>>>       fence = xe_migrate_update_pgtables(tile->migrate,
>>>                          vm, xe_vma_bo(vma), q,
>>>                          entries, num_entries,
>