Re: [PATCH] drm/xe/display: mark dpt as uncached

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

On 14.3.2024 19.05, Matthew Auld wrote:
> On 14/03/2024 16:20, Juha-Pekka Heikkila wrote:
>> make dpt as uncached to avoid pipe faults on some devices
> 
> s/make/Mark/
> 
> Also missing full stop.
> 
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> 
> Did this help on ADL btw? I don't think there are any PAT bits in the 
> GGTT on that hw so this just noops, right? Or am I misremembering...

adlp go through here setting up dpt..but turn out there was bit of 
mismatch is results. I don't have proper hw to test and my friend 
initially thought this fixed dpt issues on adlp. Turn out he had changed 
fbdev configs that affected results, which maybe point towards some 
issue on stolen. I'll yet need to ask for more logs from him.

> 
> In addition to this I think we might also want something like:
> https://patchwork.freedesktop.org/series/131148/
> 
>> ---
>>   drivers/gpu/drm/xe/display/xe_fb_pin.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c 
>> b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> index 722c84a56607..98592994c8d4 100644
>> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>> @@ -30,7 +30,7 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map 
>> *map, u32 *dpt_ofs, u32 bo_
>>           for (row = 0; row < height; row++) {
>>               u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * 
>> XE_PAGE_SIZE,
>> -                                  xe->pat.idx[XE_CACHE_WB]);
>> +                                  xe->pat.idx[XE_CACHE_NONE]);
>>               iosys_map_wr(map, *dpt_ofs, u64, pte);
>>               *dpt_ofs += 8;
>> @@ -62,7 +62,7 @@ write_dpt_remapped(struct xe_bo *bo, struct 
>> iosys_map *map, u32 *dpt_ofs,
>>           for (column = 0; column < width; column++) {
>>               iosys_map_wr(map, *dpt_ofs, u64,
>>                        pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
>> -                     xe->pat.idx[XE_CACHE_WB]));
>> +                     xe->pat.idx[XE_CACHE_NONE]));
>>               *dpt_ofs += 8;
>>               src_idx++;
>> @@ -119,7 +119,7 @@ static int __xe_pin_fb_vma_dpt(struct 
>> intel_framebuffer *fb,
>>           for (x = 0; x < size / XE_PAGE_SIZE; x++) {
>>               u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x * XE_PAGE_SIZE,
>> -                                  xe->pat.idx[XE_CACHE_WB]);
>> +                                  xe->pat.idx[XE_CACHE_NONE]);
>>               iosys_map_wr(&dpt->vmap, x * 8, u64, pte);
>>           }
>> @@ -165,7 +165,7 @@ write_ggtt_rotated(struct xe_bo *bo, struct 
>> xe_ggtt *ggtt, u32 *ggtt_ofs, u32 bo
>>           for (row = 0; row < height; row++) {
>>               u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx * 
>> XE_PAGE_SIZE,
>> -                                  xe->pat.idx[XE_CACHE_WB]);
>> +                                  xe->pat.idx[XE_CACHE_NONE]);
>>               xe_ggtt_set_pte(ggtt, *ggtt_ofs, pte);
>>               *ggtt_ofs += XE_PAGE_SIZE;
>> @@ -211,7 +211,7 @@ static int __xe_pin_fb_vma_ggtt(struct 
>> intel_framebuffer *fb,
>>           for (x = 0; x < size; x += XE_PAGE_SIZE) {
>>               u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x,
>> -                                  xe->pat.idx[XE_CACHE_WB]);
>> +                                  xe->pat.idx[XE_CACHE_NONE]);
> 
> This looks unrelated to DPT? Maybe tweak the commit title/message.
> 
> Anyway, I think change looks reasonable since you usually don't want to 
> mess around with caching for display stuff.
> 
> With commit title/message tweaked,
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Thanks

/Juha-Pekka

> 
>>               xe_ggtt_set_pte(ggtt, vma->node.start + x, pte);
>>           }