Re: [PATCH v2 0/2] Fixes for omapdrm console

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.02.24 um 08:06 schrieb Tony Lindgren:
> * Tony Lindgren <tony@atomide.com> [240226 13:26]:
>> * Thomas Zimmermann <tzimmermann@suse.de> [240226 09:10]:
>>> Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:
>>>> On 26/02/2024 10:26, Tomi Valkeinen wrote:
>>>>> How is it broken? I don't usually use the console (or fbdev) but
>>>>> enabling it now, it seems to work fine for me, on DRA76 EVM with
>>>>> HDMI output.
>>> Omapdrm implements drm_framebuffer_funcs.dirty withomap_framebuffer_dirty().
>>> AFAIK DRM semantics requires to run the dirty helper after writing to the
>>> framebuffer's memory. Userspace does this via the DIRTYFB ioctl. [1] But (at
>>> least) for correctness the console needs to do the same.
>>>
>>> [1] https://elixir.bootlin.com/linux/v6.7.6/source/drivers/gpu/drm/drm_ioctl.c#L679
>> Yes I noticed console not updating and bisected it down to the two
>> commits listed. I did the bisect on a droid4 though with command mode
>> LCD. I did not test with HDMI, will give that a try too.
> I can reproduce the cache issue with Tomi's omapfb-tests [2] below:
>
> while true;
>        do dd if=/dev/urandom of=/dev/fb0
>        ~/src/omapfb-tests/test
>        sleep 1
> done
>
> That produces short random data stripes on the test image.
>
>>>> After applying your patches, I see a lot of cache-related artifacts on
>>>> the screen when updating the fb.
>>> I guess we might need a dma-specific mmap helper to make this work
>>> correctly.
> Comparing the difference between drm_gem_mmap_obj() and
> fb_deferred_io_mmap(), the following test patch makes the cache issue
> go away for me. Not sure if this can be set based on some flag, or if
> we need a separate fb_deferred_io_wc_mmap() or something like that?
>
> Regards,
>
> Tony
>
> [2] https://github.com/tomba/omapfb-tests
>
> 8< --------------------
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -224,6 +224,7 @@ static const struct address_space_operations fb_deferred_io_aops = {
>   int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>   {
>   	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> +	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));

Great, that's exactly what I had in mind!

My proposal is to add this mmap function directly to omapdrm. I'll later 
take care of integrating this into the overall framework. I have a few 
other ideas in mind that are related to this issue. Ok?

Best regards
Thomas

>   
>   	vma->vm_ops = &fb_deferred_io_vm_ops;
>   	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);

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