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

[Jocelyn Falempe <jfalempe@redhat.com>]

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.

> 
>>>  * 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,

-- 

Jocelyn


> 
> Regards,
> 
> Tvrtko
>