[PATCH v2 0/2] Fixes for omapdrm console

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

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

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

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)

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

On 26/02/2024 10:26, Tomi Valkeinen wrote:

Hi Tony,

On 25/02/2024 08:46, Tony Lindgren wrote:

quoted

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

Hi

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:

On 26/02/2024 10:26, Tomi Valkeinen wrote:

quoted

Hi Tony,

On 25/02/2024 08:46, Tony Lindgren wrote:

quoted

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)

* Thomas Zimmermann [off-list ref] [240226 09:10]:

Hi

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:

quoted

On 26/02/2024 10:26, Tomi Valkeinen wrote:

quoted

Hi Tony,

On 25/02/2024 08:46, Tony Lindgren wrote:

quoted

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.

quoted

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

* Tony Lindgren [off-list ref] [240226 13:26]:

* Thomas Zimmermann [off-list ref] [240226 09:10]:

quoted

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:

quoted

On 26/02/2024 10:26, Tomi Valkeinen wrote:

quoted

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.

quoted

quoted

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

Hi

Am 27.02.24 um 08:06 schrieb Tony Lindgren:

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)

* Thomas Zimmermann [off-list ref] [240227 07:56]:

Am 27.02.24 um 08:06 schrieb Tony Lindgren:

quoted

* Tony Lindgren [off-list ref] [240226 13:26]:

quoted

* Thomas Zimmermann [off-list ref] [240226 09:10]:

quoted

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:

quoted

On 26/02/2024 10:26, Tomi Valkeinen wrote:

quoted

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.

quoted

quoted

quoted

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

Hi

Am 27.02.24 um 09:01 schrieb Tony Lindgren:

* Thomas Zimmermann [off-list ref] [240227 07:56]:

quoted

Am 27.02.24 um 08:06 schrieb Tony Lindgren:

quoted

* Tony Lindgren [off-list ref] [240226 13:26]:

quoted

* Thomas Zimmermann [off-list ref] [240226 09:10]:

quoted

Am 26.02.24 um 10:01 schrieb Tomi Valkeinen:

quoted

On 26/02/2024 10:26, Tomi Valkeinen wrote:

quoted

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.

quoted

quoted

quoted

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

quoted

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)

* Thomas Zimmermann [off-list ref] [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

* Tony Lindgren [off-list ref] [240227 11:47]:

* Thomas Zimmermann [off-list ref] [240227 09:16]:

quoted

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

quoted

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

quoted

  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

Hi

Am 27.02.24 um 11:19 schrieb Tony Lindgren:

* Tony Lindgren [off-list ref] [240227 11:47]:

quoted

* Thomas Zimmermann [off-list ref] [240227 09:16]:

quoted

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

quoted

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

quoted

   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

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