Re: [PATCH] drm/i915/dmabuf: Flush the cache in vmap

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 31.10.25 um 16:04 schrieb Jocelyn Falempe:
> On 31/10/2025 14:12, Tvrtko Ursulin wrote:
>>
>> On 27/10/2025 10:26, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 27.10.25 um 10:46 schrieb Jocelyn Falempe:
>>>> On 24/10/2025 17:55, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 24/10/2025 16:18, Thomas Zimmermann wrote:
>>>>>> Hi
>>>>>>
>>>>>> Am 24.10.25 um 15:33 schrieb Jocelyn Falempe:
>>>>>>> On 24/10/2025 14:40, Thomas Zimmermann wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> Am 24.10.25 um 13:53 schrieb Tvrtko Ursulin:
>>>>>>>>>
>>>>>>>>> On 24/10/2025 12:04, Jocelyn Falempe wrote:
>>>>>>>>>> On a lenovo se100 server, when using i915 GPU for rendering, 
>>>>>>>>>> and the
>>>>>>>>>> ast driver for display, the graphic output is corrupted, and 
>>>>>>>>>> almost
>>>>>>>>>> unusable.
>>>>>>>>>>
>>>>>>>>>> Adding a clflush call in the vmap function fixes this issue
>>>>>>>>>> completely.
>>>>>>>>>
>>>>>>>>> AST is importing i915 allocated buffer in this use case, or 
>>>>>>>>> how exactly is the relationship?
>>>>>>>>>
>>>>>>>>> Wondering if some path is not calling dma_buf_begin/ 
>>>>>>>>> end_cpu_access().
>>>>>>>>
>>>>>>>> Yes, ast doesn't call begin/end_cpu_access in [1].
>>>>>>>>
>>>>>>>> Jocelyn, if that fixes the issue, feel free to send me a patch 
>>>>>>>> for review.
>>>>>>>>
>>>>>>>> [1] 
>>>>>>>> https://elixir.bootlin.com/linux/v6.17.4/source/drivers/gpu/ 
>>>>>>>> drm/ ast/ ast_mode.c
>>>>>>>
>>>>>>> I tried the following patch, but that doesn't fix the graphical 
>>>>>>> issue:
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>>>>>>> b/drivers/gpu/drm/ast/ ast_mode.c
>>>>>>> index b4e8edc7c767..e50f95a4c8a9 100644
>>>>>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>>>>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>>>>>> @@ -564,6 +564,7 @@ static void 
>>>>>>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>>>>>         struct drm_crtc_state *crtc_state = 
>>>>>>> drm_atomic_get_new_crtc_state(state, crtc);
>>>>>>>         struct drm_rect damage;
>>>>>>>         struct drm_atomic_helper_damage_iter iter;
>>>>>>> +       int ret;
>>>>>>>
>>>>>>>         if (!old_fb || (fb->format != old_fb->format) || 
>>>>>>> crtc_state- >mode_changed) {
>>>>>>>                 struct ast_crtc_state *ast_crtc_state = 
>>>>>>> to_ast_crtc_state(crtc_state);
>>>>>>> @@ -572,11 +573,16 @@ static void 
>>>>>>> ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>>>>>>                 ast_set_vbios_color_reg(ast, fb->format, 
>>>>>>> ast_crtc_state->vmode);
>>>>>>>         }
>>>>>>>
>>>>>>> +       ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>>>>>> +       pr_info("AST begin_cpu_access %d\n", ret);
>>>>>>
>>>>>> Presumably, you end up in [1]. I cannot find the cflush there or 
>>>>>> in [2]. Maybe you need to add this call somewhere in there, 
>>>>>> similar to [3]. Just guessing.
>>>>>
>>>>> Near [2] clflush can happen at [4] *if* the driver thinks it is 
>>>>> needed. Most GPUs are cache coherent so mostly it isn't. But if 
>>>>> this is a Meteorlake machine (when I google Lenovo se100 it makes 
>>>>> me think so?) then the userspace has some responsibility to manage 
>>>>> things since there it is only 1-way coherency. Or userspace could 
>>>>> have even told the driver to stay off in which case it then needs 
>>>>> to manage everything. From the top of my head I am not sure how 
>>>>> exactly this used to work, or how it is supposed to interact with 
>>>>> exported buffers.
>>>>>
>>>>> If this is indeed on Meteorlake, maybe Joonas or Rodrigo remember 
>>>>> better how the special 1-way coherency is supposed to be managed 
>>>>> there?
>>>>
>>>> I've made an experiment, and if I add:
>>>>
>>>> * a calls to drm_gem_fb_begin_cpu_access() in the ast driver.
>>>> * and in i915_gem_domain.c flush_write_domain():
>>>>         case I915_GEM_DOMAIN_RENDER:
>>>> +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC | 
>>>> I915_CLFLUSH_FORCE);
>>>>
>>>> Then that fixes the issue too.
>>>>
>>>> So I think there are two things to fix:
>>>>  * The missing call to drm_gem_fb_begin_cpu_access() in ast.
>>>
>>> Yes. We definitely want to add these calls, as they are expected for 
>>> this case.
>>
>> Browsing around a bit, I notice 
>> ast_primary_plane_helper_atomic_update() calls 
>> to_drm_shadow_plane_state() to get the source of the memcpy. Should 
>> there somewhere be calls to drm_gem_begin_shadow_fb_access() and 
>> drm_gem_end_shadow_fb_access()? Or those should be set as vfuncs by 
>> someone? Sorry I get lost easily in the DRM maze of 
>> helpers<->vfuncs<-  >helpers<->vfuncs..
>
> I've already submitted [1] for the ast driver.

Don't be confused by the similar naming. The shadow-plane functions do 
the vmap(). The cpu-access functions do the caching and DMA sync.

Best regards
Thomas

>
>>
>>>>  * The missing cache flush in i915 for the Arrowlake iGPU (but 
>>>> probably not the way I've done it).
>>>
>>> You call begin_cpu_access with DMA_FROM_DEVICE, but there's no 
>>> support for that flag in i915 AFAICT. Maybe this needs to be added 
>>> somehow?
>>
>> AFAIR the premise is GPU writes will not get stuck in the last level 
>> cache but I might be remembering a reverse of what Meteorlake 1-way 
>> coherency means. This area of the driver ended up a mess and was 
>> never properly cleaned up. I even had a series to try and do it but 
>> it never happened. We will need someone who actually remembers how 
>> Meteorlake works.
>
> Logically clflush() shouldn't have any effect, because it flushes the 
> CPU cache, and then ast is reading from the CPU.
> But from the tests, it also flushes the iGPU cache, and the iGPU cache 
> is not coherent with the CPU cache (at least for iGPU writes), or this 
> problem won't exist.
>
> I've the setup ready, and this is easily reproducible, so you can send 
> me patches, and I can quickly check if that works or not.
>
> [1] 
> https://lore.kernel.org/dri-devel/908ff7f7-cdf3-4596-9246-0100e4c15820@suse.de/T/#t
>
> Best regards,
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)