[PATCH v2 0/2] Fixes for omapdrm console

[PATCH v2 0/2] Fixes for omapdrm console

[Tony Lindgren]

Here are two fixes for omapdrm console.

Regards,

Tony

Changes since v1:

- Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with
  FB_DEFAULT_DEFERRED_OPS as suggested by Thomas

Tony Lindgren (2):
  drm/omapdrm: Fix console by implementing fb_dirty
  drm/omapdrm: Fix console with deferred ops

 drivers/gpu/drm/omapdrm/omap_fbdev.c | 39 +++++++++++++++++++---------
 include/linux/fb.h                   |  4 +++
 2 files changed, 31 insertions(+), 12 deletions(-)

-- 
2.43.1

[PATCH v2 1/2] drm/omapdrm: Fix console by implementing fb_dirty

[Tony Lindgren]

The framebuffer console stopped updating with commit f231af498c29
("drm/fb-helper: Disconnect damage worker from update logic").

Let's fix the issue by implementing fb_dirty similar to what was done
with commit 039a72ce7e57 ("drm/i915/fbdev: Implement fb_dirty for intel
custom fb helper").

Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from update logic")
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -238,8 +238,20 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	return ret;
 }
 
+static int omap_fbdev_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip)
+{
+	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
+		return 0;
+
+	if (helper->fb->funcs->dirty)
+		return helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
+
+	return 0;
+}
+
 static const struct drm_fb_helper_funcs omap_fb_helper_funcs = {
 	.fb_probe = omap_fbdev_create,
+	.fb_dirty = omap_fbdev_dirty,
 };
 
 static struct drm_fb_helper *get_fb(struct fb_info *fbi)
-- 
2.43.1

[PATCH v2 2/2] drm/omapdrm: Fix console with deferred ops

[Tony Lindgren]

Commit 95da53d63dcf ("drm/omapdrm: Use regular fbdev I/O helpers")
broke console because there is no damage handling in fb_sys_write()
unlike we have in drm_fb_helper_sys_write().

Let's fix the issue by adding FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to
use with FB_DEFAULT_DEFERRED_OPS as suggested by Thomas. We no longer
need omap_fbdev_fb_mmap() as FB_DEFAULT_DEFERRED_OPS sets it to
fb_deferred_io_mmap().

Fixes: 95da53d63dcf ("drm/omapdrm: Use regular fbdev I/O helpers")
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 27 +++++++++++++++------------
 include/linux/fb.h                   |  4 ++++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -51,6 +51,10 @@ static void pan_worker(struct work_struct *work)
 	omap_gem_roll(bo, fbi->var.yoffset * npages);
 }
 
+FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(omap_fbdev,
+				   drm_fb_helper_damage_range,
+				   drm_fb_helper_damage_area)
+
 static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
 		struct fb_info *fbi)
 {
@@ -76,15 +80,6 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
 	return drm_fb_helper_pan_display(var, fbi);
 }
 
-static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
-	struct drm_fb_helper *helper = info->par;
-	struct drm_framebuffer *fb = helper->fb;
-	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
-
-	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
-}
-
 static void omap_fbdev_fb_destroy(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -94,6 +89,7 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
 
 	DBG();
 
+	fb_deferred_io_cleanup(info);
 	drm_fb_helper_fini(helper);
 
 	omap_gem_unpin(bo);
@@ -106,15 +102,13 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
 
 static const struct fb_ops omap_fb_ops = {
 	.owner = THIS_MODULE,
-	__FB_DEFAULT_DMAMEM_OPS_RDWR,
+	FB_DEFAULT_DEFERRED_OPS(omap_fbdev),
 	.fb_check_var	= drm_fb_helper_check_var,
 	.fb_set_par	= drm_fb_helper_set_par,
 	.fb_setcmap	= drm_fb_helper_setcmap,
 	.fb_blank	= drm_fb_helper_blank,
 	.fb_pan_display = omap_fbdev_pan_display,
-	__FB_DEFAULT_DMAMEM_OPS_DRAW,
 	.fb_ioctl	= drm_fb_helper_ioctl,
-	.fb_mmap	= omap_fbdev_fb_mmap,
 	.fb_destroy	= omap_fbdev_fb_destroy,
 };
 
@@ -213,6 +207,15 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	fbi->fix.smem_start = dma_addr;
 	fbi->fix.smem_len = bo->size;
 
+	/* deferred I/O */
+	helper->fbdefio.delay = HZ / 20;
+	helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+	fbi->fbdefio = &helper->fbdefio;
+	ret = fb_deferred_io_init(fbi);
+	if (ret)
+		goto fail;
+
 	/* if we have DMM, then we can use it for scrolling by just
 	 * shuffling pages around in DMM rather than doing sw blit.
 	 */
diff --git a/include/linux/fb.h b/include/linux/fb.h
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -686,6 +686,10 @@ extern int fb_deferred_io_fsync(struct file *file, loff_t start,
 	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
 	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
 
+#define FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(__prefix, __damage_range, __damage_area) \
+	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
+	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
+
 /*
  * Initializes struct fb_ops for deferred I/O.
  */
-- 
2.43.1

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

[Thomas Zimmermann]

Hi,

looks good. For the series:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Am 25.02.24 um 07:46 schrieb Tony Lindgren:
> Here are two fixes for omapdrm console.
>
> Regards,
>
> Tony
>
> Changes since v1:
>
> - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with
>    FB_DEFAULT_DEFERRED_OPS as suggested by Thomas
>
> Tony Lindgren (2):
>    drm/omapdrm: Fix console by implementing fb_dirty
>    drm/omapdrm: Fix console with deferred ops
>
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 39 +++++++++++++++++++---------
>   include/linux/fb.h                   |  4 +++
>   2 files changed, 31 insertions(+), 12 deletions(-)
>

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

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

[Tomi Valkeinen]

Hi Tony,

On 25/02/2024 08:46, Tony Lindgren wrote:
> Here are two fixes for omapdrm console.

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.

  Tomi

> 
> Regards,
> 
> Tony
> 
> Changes since v1:
> 
> - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with
>    FB_DEFAULT_DEFERRED_OPS as suggested by Thomas
> 
> Tony Lindgren (2):
>    drm/omapdrm: Fix console by implementing fb_dirty
>    drm/omapdrm: Fix console with deferred ops
> 
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 39 +++++++++++++++++++---------
>   include/linux/fb.h                   |  4 +++
>   2 files changed, 31 insertions(+), 12 deletions(-)
> 

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

[Tomi Valkeinen]

On 26/02/2024 10:26, Tomi Valkeinen wrote:
> Hi Tony,
> 
> On 25/02/2024 08:46, Tony Lindgren wrote:
>> Here are two fixes for omapdrm console.
> 
> 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.

After applying your patches, I see a lot of cache-related artifacts on 
the screen when updating the fb.

  Tomi

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

[Thomas Zimmermann]

Hi

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:
> On 26/02/2024 10:26, Tomi Valkeinen wrote:
>> Hi Tony,
>>
>> On 25/02/2024 08:46, Tony Lindgren wrote:
>>> Here are two fixes for omapdrm console.
>>
>> 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

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

Best regards
Thomas

>
>  Tomi
>

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

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

[Tony Lindgren]

* Thomas Zimmermann <tzimmermann@suse.de> [240226 09:10]:
> Hi
> 
> Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:
> > On 26/02/2024 10:26, Tomi Valkeinen wrote:
> > > Hi Tony,
> > > 
> > > On 25/02/2024 08:46, Tony Lindgren wrote:
> > > > Here are two fixes for omapdrm console.
> > > 
> > > 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.

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

I can easily test this if you have some suggested patch to try.

Hmm so I wonder if we now have double updates happening on HDMI?

Regards,

Tony

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

[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));
 
 	vma->vm_ops = &fb_deferred_io_vm_ops;
 	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);

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

[Thomas Zimmermann]

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)

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

[Tony Lindgren]

* Thomas Zimmermann <tzimmermann@suse.de> [240227 07:56]:
> 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?
> > 
> > [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!

OK :)

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

OK that sounds good to me, I'll post v3 set of patches.

Regards,

Tony

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

[Thomas Zimmermann]

Hi

Am 27.02.24 um 09:01 schrieb Tony Lindgren:
> * Thomas Zimmermann <tzimmermann@suse.de> [240227 07:56]:
>> 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?
>>>
>>> [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!
> OK :)
>
>> 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?
> OK that sounds good to me, I'll post v3 set of patches.

I just realized the fb_deferred_io_mmap() is already exported. So please 
use it instead of duplicating the code in omapdrm.

[1] 
https://elixir.bootlin.com/linux/v6.7/source/drivers/video/fbdev/core/fb_defio.c#L237

I also noticed that omapdrm does not yet select the correct Kconfig 
symbols. That can be fixed by

  1) creating Kconfig FB_DMAMEM_HELPERS_DEFERRED that are similar to 
their SYSMEM equivalent at [2]. The tokens should look like this

configFB_DMAMEM_HELPERS_DEFERRED  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS_DEFERRED>
bool
depends onFB_CORE  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_CORE>
selectFB_DEFERRED_IO  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_DEFERRED_IO>
selectFB_DMAMEM_HELPERS  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS>


   2) and selecting it instead of FB_DMAMEM_HELPERS under omapdrm's 
Kconfig symbol.

[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/Kconfig#L147

Best regards
Thomas

>
> Regards,
>
> Tony
>

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

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

[Tony Lindgren]

* Thomas Zimmermann <tzimmermann@suse.de> [240227 09:16]:
> I just realized the fb_deferred_io_mmap() is already exported. So please use
> it instead of duplicating the code in omapdrm.
> 
> [1] https://elixir.bootlin.com/linux/v6.7/source/drivers/video/fbdev/core/fb_defio.c#L237

Yeah I have now:

static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));

	return fb_deferred_io_mmap(info, vma);
}

> I also noticed that omapdrm does not yet select the correct Kconfig symbols.
> That can be fixed by
> 
>  1) creating Kconfig FB_DMAMEM_HELPERS_DEFERRED that are similar to their
> SYSMEM equivalent at [2]. The tokens should look like this
> 
> configFB_DMAMEM_HELPERS_DEFERRED  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS_DEFERRED>
> bool
> depends onFB_CORE  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_CORE>
> selectFB_DEFERRED_IO  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_DEFERRED_IO>
> selectFB_DMAMEM_HELPERS  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS>

OK

>   2) and selecting it instead of FB_DMAMEM_HELPERS under omapdrm's Kconfig
> symbol.

OK

Regards,

Tony

> [2] https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/Kconfig#L147

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [240227 11:47]:
> * Thomas Zimmermann <tzimmermann@suse.de> [240227 09:16]:
> > I just realized the fb_deferred_io_mmap() is already exported. So please use
> > it instead of duplicating the code in omapdrm.
> > 
> > [1] https://elixir.bootlin.com/linux/v6.7/source/drivers/video/fbdev/core/fb_defio.c#L237
> 
> Yeah I have now:
> 
> static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> 	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> 
> 	return fb_deferred_io_mmap(info, vma);
> }
> 
> > I also noticed that omapdrm does not yet select the correct Kconfig symbols.
> > That can be fixed by
> > 
> >  1) creating Kconfig FB_DMAMEM_HELPERS_DEFERRED that are similar to their
> > SYSMEM equivalent at [2]. The tokens should look like this
> > 
> > configFB_DMAMEM_HELPERS_DEFERRED  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS_DEFERRED>
> > bool
> > depends onFB_CORE  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_CORE>
> > selectFB_DEFERRED_IO  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_DEFERRED_IO>
> > selectFB_DMAMEM_HELPERS  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS>
> 
> OK
> 
> >   2) and selecting it instead of FB_DMAMEM_HELPERS under omapdrm's Kconfig
> > symbol.
> 
> OK

So here's what I have now, does that look OK?

Regards,

Tony

8< -------------------------
diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
--- a/drivers/gpu/drm/omapdrm/Kconfig
+++ b/drivers/gpu/drm/omapdrm/Kconfig
@@ -4,7 +4,7 @@ config DRM_OMAP
 	depends on DRM && OF
 	depends on ARCH_OMAP2PLUS
 	select DRM_KMS_HELPER
-	select FB_DMAMEM_HELPERS if DRM_FBDEV_EMULATION
+	select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION
 	select VIDEOMODE_HELPERS
 	select HDMI
 	default n
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -51,6 +51,10 @@ static void pan_worker(struct work_struct *work)
 	omap_gem_roll(bo, fbi->var.yoffset * npages);
 }
 
+FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(omap_fbdev,
+				   drm_fb_helper_damage_range,
+				   drm_fb_helper_damage_area)
+
 static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
 		struct fb_info *fbi)
 {
@@ -78,11 +82,9 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
 
 static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
-	struct drm_fb_helper *helper = info->par;
-	struct drm_framebuffer *fb = helper->fb;
-	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
+	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
 
-	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
+	return fb_deferred_io_mmap(info, vma);
 }
 
 static void omap_fbdev_fb_destroy(struct fb_info *info)
@@ -94,6 +96,7 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
 
 	DBG();
 
+	fb_deferred_io_cleanup(info);
 	drm_fb_helper_fini(helper);
 
 	omap_gem_unpin(bo);
@@ -104,15 +107,19 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
 	kfree(fbdev);
 }
 
+/*
+ * For now, we cannot use FB_DEFAULT_DEFERRED_OPS and fb_deferred_io_mmap()
+ * because we use write-combine.
+ */
 static const struct fb_ops omap_fb_ops = {
 	.owner = THIS_MODULE,
-	__FB_DEFAULT_DMAMEM_OPS_RDWR,
+	__FB_DEFAULT_DEFERRED_OPS_RDWR(omap_fbdev),
 	.fb_check_var	= drm_fb_helper_check_var,
 	.fb_set_par	= drm_fb_helper_set_par,
 	.fb_setcmap	= drm_fb_helper_setcmap,
 	.fb_blank	= drm_fb_helper_blank,
 	.fb_pan_display = omap_fbdev_pan_display,
-	__FB_DEFAULT_DMAMEM_OPS_DRAW,
+	__FB_DEFAULT_DEFERRED_OPS_DRAW(omap_fbdev),
 	.fb_ioctl	= drm_fb_helper_ioctl,
 	.fb_mmap	= omap_fbdev_fb_mmap,
 	.fb_destroy	= omap_fbdev_fb_destroy,
@@ -213,6 +220,15 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	fbi->fix.smem_start = dma_addr;
 	fbi->fix.smem_len = bo->size;
 
+	/* deferred I/O */
+	helper->fbdefio.delay = HZ / 20;
+	helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+	fbi->fbdefio = &helper->fbdefio;
+	ret = fb_deferred_io_init(fbi);
+	if (ret)
+		goto fail;
+
 	/* if we have DMM, then we can use it for scrolling by just
 	 * shuffling pages around in DMM rather than doing sw blit.
 	 */
diff --git a/drivers/video/fbdev/core/Kconfig b/drivers/video/fbdev/core/Kconfig
--- a/drivers/video/fbdev/core/Kconfig
+++ b/drivers/video/fbdev/core/Kconfig
@@ -144,6 +144,12 @@ config FB_DMAMEM_HELPERS
 	select FB_SYS_IMAGEBLIT
 	select FB_SYSMEM_FOPS
 
+config FB_DMAMEM_HELPERS_DEFERRED
+	bool
+	depends on FB_CORE
+	select FB_DEFERRED_IO
+	select FB_DMAMEM_HELPERS
+
 config FB_IOMEM_FOPS
 	tristate
 	depends on FB_CORE
diff --git a/include/linux/fb.h b/include/linux/fb.h
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -686,6 +686,10 @@ extern int fb_deferred_io_fsync(struct file *file, loff_t start,
 	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
 	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
 
+#define FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(__prefix, __damage_range, __damage_area) \
+	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
+	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
+
 /*
  * Initializes struct fb_ops for deferred I/O.
  */
-- 
2.43.1

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

[Thomas Zimmermann]

Hi

Am 27.02.24 um 11:19 schrieb Tony Lindgren:
> * Tony Lindgren <tony@atomide.com> [240227 11:47]:
>> * Thomas Zimmermann <tzimmermann@suse.de> [240227 09:16]:
>>> I just realized the fb_deferred_io_mmap() is already exported. So please use
>>> it instead of duplicating the code in omapdrm.
>>>
>>> [1] https://elixir.bootlin.com/linux/v6.7/source/drivers/video/fbdev/core/fb_defio.c#L237
>> Yeah I have now:
>>
>> static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> {
>> 	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>
>> 	return fb_deferred_io_mmap(info, vma);
>> }
>>
>>> I also noticed that omapdrm does not yet select the correct Kconfig symbols.
>>> That can be fixed by
>>>
>>>   1) creating Kconfig FB_DMAMEM_HELPERS_DEFERRED that are similar to their
>>> SYSMEM equivalent at [2]. The tokens should look like this
>>>
>>> configFB_DMAMEM_HELPERS_DEFERRED  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS_DEFERRED>
>>> bool
>>> depends onFB_CORE  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_CORE>
>>> selectFB_DEFERRED_IO  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_DEFERRED_IO>
>>> selectFB_DMAMEM_HELPERS  <https://elixir.bootlin.com/linux/latest/K/ident/CONFIG_FB_SYSMEM_HELPERS>
>> OK
>>
>>>    2) and selecting it instead of FB_DMAMEM_HELPERS under omapdrm's Kconfig
>>> symbol.
>> OK
> So here's what I have now, does that look OK?

Looks good. You can add

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

to the final patch.

Best regards
Thomas

>
> Regards,
>
> Tony
>
> 8< -------------------------
> diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig
> --- a/drivers/gpu/drm/omapdrm/Kconfig
> +++ b/drivers/gpu/drm/omapdrm/Kconfig
> @@ -4,7 +4,7 @@ config DRM_OMAP
>   	depends on DRM && OF
>   	depends on ARCH_OMAP2PLUS
>   	select DRM_KMS_HELPER
> -	select FB_DMAMEM_HELPERS if DRM_FBDEV_EMULATION
> +	select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION
>   	select VIDEOMODE_HELPERS
>   	select HDMI
>   	default n
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -51,6 +51,10 @@ static void pan_worker(struct work_struct *work)
>   	omap_gem_roll(bo, fbi->var.yoffset * npages);
>   }
>   
> +FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(omap_fbdev,
> +				   drm_fb_helper_damage_range,
> +				   drm_fb_helper_damage_area)
> +
>   static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   		struct fb_info *fbi)
>   {
> @@ -78,11 +82,9 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   
>   static int omap_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>   {
> -	struct drm_fb_helper *helper = info->par;
> -	struct drm_framebuffer *fb = helper->fb;
> -	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
> +	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>   
> -	return drm_gem_mmap_obj(bo, omap_gem_mmap_size(bo), vma);
> +	return fb_deferred_io_mmap(info, vma);
>   }
>   
>   static void omap_fbdev_fb_destroy(struct fb_info *info)
> @@ -94,6 +96,7 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
>   
>   	DBG();
>   
> +	fb_deferred_io_cleanup(info);
>   	drm_fb_helper_fini(helper);
>   
>   	omap_gem_unpin(bo);
> @@ -104,15 +107,19 @@ static void omap_fbdev_fb_destroy(struct fb_info *info)
>   	kfree(fbdev);
>   }
>   
> +/*
> + * For now, we cannot use FB_DEFAULT_DEFERRED_OPS and fb_deferred_io_mmap()
> + * because we use write-combine.
> + */
>   static const struct fb_ops omap_fb_ops = {
>   	.owner = THIS_MODULE,
> -	__FB_DEFAULT_DMAMEM_OPS_RDWR,
> +	__FB_DEFAULT_DEFERRED_OPS_RDWR(omap_fbdev),
>   	.fb_check_var	= drm_fb_helper_check_var,
>   	.fb_set_par	= drm_fb_helper_set_par,
>   	.fb_setcmap	= drm_fb_helper_setcmap,
>   	.fb_blank	= drm_fb_helper_blank,
>   	.fb_pan_display = omap_fbdev_pan_display,
> -	__FB_DEFAULT_DMAMEM_OPS_DRAW,
> +	__FB_DEFAULT_DEFERRED_OPS_DRAW(omap_fbdev),
>   	.fb_ioctl	= drm_fb_helper_ioctl,
>   	.fb_mmap	= omap_fbdev_fb_mmap,
>   	.fb_destroy	= omap_fbdev_fb_destroy,
> @@ -213,6 +220,15 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   	fbi->fix.smem_start = dma_addr;
>   	fbi->fix.smem_len = bo->size;
>   
> +	/* deferred I/O */
> +	helper->fbdefio.delay = HZ / 20;
> +	helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +
> +	fbi->fbdefio = &helper->fbdefio;
> +	ret = fb_deferred_io_init(fbi);
> +	if (ret)
> +		goto fail;
> +
>   	/* if we have DMM, then we can use it for scrolling by just
>   	 * shuffling pages around in DMM rather than doing sw blit.
>   	 */
> diff --git a/drivers/video/fbdev/core/Kconfig b/drivers/video/fbdev/core/Kconfig
> --- a/drivers/video/fbdev/core/Kconfig
> +++ b/drivers/video/fbdev/core/Kconfig
> @@ -144,6 +144,12 @@ config FB_DMAMEM_HELPERS
>   	select FB_SYS_IMAGEBLIT
>   	select FB_SYSMEM_FOPS
>   
> +config FB_DMAMEM_HELPERS_DEFERRED
> +	bool
> +	depends on FB_CORE
> +	select FB_DEFERRED_IO
> +	select FB_DMAMEM_HELPERS
> +
>   config FB_IOMEM_FOPS
>   	tristate
>   	depends on FB_CORE
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -686,6 +686,10 @@ extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>   	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
>   	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
>   
> +#define FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(__prefix, __damage_range, __damage_area) \
> +	__FB_GEN_DEFAULT_DEFERRED_OPS_RDWR(__prefix, __damage_range, sys) \
> +	__FB_GEN_DEFAULT_DEFERRED_OPS_DRAW(__prefix, __damage_area, sys)
> +
>   /*
>    * Initializes struct fb_ops for deferred I/O.
>    */

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