[PATCH 0/4] mgag200: Improve damage handling

[PATCH 0/4] mgag200: Improve damage handling

[Jocelyn Falempe]

This series improves the damage handling on Matrox gpu, and allows
Gnome/Wayland to run much better.
Also include some driver cleanup.

Tested on a Dell T310 with Matrox MGA G200eW WPCM450 (rev 0a)

Thanks,

Jocelyn Falempe (4):
  mgag200: Add FB_DAMAGE_CLIPS support
  mgag200: Optimize damage clips
  mgag200: remove unused flag
  mgag200: remove mgag200_probe_vram()

 drivers/gpu/drm/mgag200/mgag200_drv.c  |  3 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  3 --
 drivers/gpu/drm/mgag200/mgag200_mm.c   | 50 ++++----------------------
 drivers/gpu/drm/mgag200/mgag200_mode.c | 17 ++++++---
 4 files changed, 20 insertions(+), 53 deletions(-)

-- 
2.35.1

-- 

Jocelyn

[PATCH 1/4] mgag200: Add FB_DAMAGE_CLIPS support

[Jocelyn Falempe]

The driver does support damage clips, but doesn't advertise it.
So when running gnome/wayland on Matrox hardware, the full frame is
copied to the slow Matrox memory, which leads to very poor performances.

Add drm_plane_enable_fb_damage_clips() to advertise this capability to
userspace.

With this patch, gnome/wayland becomes usable on Matrox GPU.

Suggested-by: Jonas Ådahl <jadahl@gmail.com>
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 6e18d3bbd720..cff2e76f3fa0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1107,6 +1107,8 @@ int mgag200_modeset_init(struct mga_device *mdev)
 		return ret;
 	}
 
+	drm_plane_enable_fb_damage_clips(&pipe->plane);
+
 	/* FIXME: legacy gamma tables; convert to CRTC state */
 	drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
 
-- 
2.35.1

[PATCH 2/4] mgag200: Optimize damage clips

[Jocelyn Falempe]

when there are multiple damage clips, previous code merged them into one
big rectangle. As the Matrox memory is very slow, it's faster to copy each
damage clip.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index cff2e76f3fa0..2bc380a85996 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -855,10 +855,6 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
 
 	dst += drm_fb_clip_offset(fb->pitches[0], fb->format, clip);
 	drm_fb_memcpy_toio(dst, fb->pitches[0], vmap, fb, clip);
-
-	/* Always scanout image at VRAM offset 0 */
-	mgag200_set_startadd(mdev, (u32)0);
-	mgag200_set_offset(mdev, fb);
 }
 
 static void
@@ -904,6 +900,9 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	mgag200_enable_display(mdev);
 
 	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
+	/* Always scanout image at VRAM offset 0 */
+	mgag200_set_startadd(mdev, (u32)0);
+	mgag200_set_offset(mdev, fb);
 }
 
 static void
@@ -959,12 +958,18 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect damage;
+	struct drm_atomic_helper_damage_iter iter;
 
 	if (!fb)
 		return;
 
-	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
+	}
+	/* Always scanout image at VRAM offset 0 */
+	mgag200_set_startadd(mdev, (u32)0);
+	mgag200_set_offset(mdev, fb);
 }
 
 static struct drm_crtc_state *
-- 
2.35.1

[PATCH 3/4] mgag200: remove unused flag

[Jocelyn Falempe]

The flag MGAG200_FLAG_HW_BUG_NO_STARTADD is no more used, because
the framebuffer is now always at offset 0.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
 drivers/gpu/drm/mgag200/mgag200_drv.h | 3 ---
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 217844d71ab5..8659e1ca8009 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -306,8 +306,7 @@ mgag200_device_create(struct pci_dev *pdev, enum mga_type type, unsigned long fl
 static const struct pci_device_id mgag200_pciidlist[] = {
 	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
 	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
-	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
-		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
+	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A },
 	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
 	{ PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV },
 	{ PCI_VENDOR_ID_MATROX, 0x532, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_WB },
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 4368112023f7..c7b6dc771ab3 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -201,9 +201,6 @@ enum mga_type {
 	G200_EW3,
 };
 
-/* HW does not handle 'startadd' field correct. */
-#define MGAG200_FLAG_HW_BUG_NO_STARTADD	(1ul << 8)
-
 #define MGAG200_TYPE_MASK	(0x000000ff)
 #define MGAG200_FLAG_MASK	(0x00ffff00)
 
-- 
2.35.1

[PATCH 4/4] mgag200: remove mgag200_probe_vram()

[Jocelyn Falempe]

This function writes some pattern to video memory, to check for conflict.
In case of conflicts, it returns a random memory capacity (offset of the
conflict).
Using devm_arch_io_reserve_memtype_wc() should garantee that no other
driver is using this memory region.
In case of real memory conflicts, as it is video memory, the user will
notice it easily. So there is no need for this function.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_mm.c | 50 ++++------------------------
 1 file changed, 7 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index fa996d46feed..68299b560a98 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -32,48 +32,6 @@
 
 #include "mgag200_drv.h"
 
-static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
-				 size_t size)
-{
-	int offset;
-	int orig;
-	int test1, test2;
-	int orig1, orig2;
-	size_t vram_size;
-
-	/* Probe */
-	orig = ioread16(mem);
-	iowrite16(0, mem);
-
-	vram_size = size;
-
-	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
-		vram_size = vram_size - 0x400000;
-
-	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
-		orig1 = ioread8(mem + offset);
-		orig2 = ioread8(mem + offset + 0x100);
-
-		iowrite16(0xaa55, mem + offset);
-		iowrite16(0xaa55, mem + offset + 0x100);
-
-		test1 = ioread16(mem + offset);
-		test2 = ioread16(mem);
-
-		iowrite16(orig1, mem + offset);
-		iowrite16(orig2, mem + offset + 0x100);
-
-		if (test1 != 0xaa55)
-			break;
-
-		if (test2)
-			break;
-	}
-
-	iowrite16(orig, mem);
-
-	return offset - 65536;
-}
 
 int mgag200_mm_init(struct mga_device *mdev)
 {
@@ -81,6 +39,7 @@ int mgag200_mm_init(struct mga_device *mdev)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	u8 misc;
 	resource_size_t start, len;
+	size_t vram_size;
 
 	WREG_ECRT(0x04, 0x00);
 
@@ -106,7 +65,12 @@ int mgag200_mm_init(struct mga_device *mdev)
 	if (!mdev->vram)
 		return -ENOMEM;
 
-	mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
+	vram_size = len;
+	/* G200_EW3 has only 12MB of memory */
+	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
+		vram_size -= 0x400000;
+
+	mdev->mc.vram_size = vram_size;
 	mdev->mc.vram_base = start;
 	mdev->mc.vram_window = len;
 
-- 
2.35.1

Re: [PATCH 0/4] mgag200: Improve damage handling

[Lyude Paul]

Nice work! For the whole series:

Reviewed-by: Lyude Paul <lyude@redhat.com>

Will probably let it sit on the ML for a little bit just to make sure that
Thomas gets a chance to look at this

On Tue, 2022-04-26 at 18:41 +0200, Jocelyn Falempe wrote:
> This series improves the damage handling on Matrox gpu, and allows
> Gnome/Wayland to run much better.
> Also include some driver cleanup.
> 
> Tested on a Dell T310 with Matrox MGA G200eW WPCM450 (rev 0a)
> 
> Thanks,
> 
> Jocelyn Falempe (4):
>   mgag200: Add FB_DAMAGE_CLIPS support
>   mgag200: Optimize damage clips
>   mgag200: remove unused flag
>   mgag200: remove mgag200_probe_vram()
> 
>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  3 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  3 --
>  drivers/gpu/drm/mgag200/mgag200_mm.c   | 50 ++++----------------------
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 ++++++---
>  4 files changed, 20 insertions(+), 53 deletions(-)
> 
> -- 
> 2.35.1
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 1/4] mgag200: Add FB_DAMAGE_CLIPS support

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1416 bytes --]



Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
> The driver does support damage clips, but doesn't advertise it.
> So when running gnome/wayland on Matrox hardware, the full frame is
> copied to the slow Matrox memory, which leads to very poor performances.
> 
> Add drm_plane_enable_fb_damage_clips() to advertise this capability to
> userspace.
> 
> With this patch, gnome/wayland becomes usable on Matrox GPU.
> 
> Suggested-by: Jonas Ådahl <jadahl@gmail.com>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

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

> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6e18d3bbd720..cff2e76f3fa0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1107,6 +1107,8 @@ int mgag200_modeset_init(struct mga_device *mdev)
>   		return ret;
>   	}
>   
> +	drm_plane_enable_fb_damage_clips(&pipe->plane);
> +
>   	/* FIXME: legacy gamma tables; convert to CRTC state */
>   	drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] mgag200: Optimize damage clips

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2692 bytes --]

Hi

Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
> when there are multiple damage clips, previous code merged them into one
> big rectangle. As the Matrox memory is very slow, it's faster to copy each
> damage clip.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

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

How do you measure the performance? It seems as if it depends a lot on 
the nature of the screen update.  But maybe using that loop is faster in 
the general case with other drivers as well.

Best regards
Thomas

> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index cff2e76f3fa0..2bc380a85996 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -855,10 +855,6 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
>   
>   	dst += drm_fb_clip_offset(fb->pitches[0], fb->format, clip);
>   	drm_fb_memcpy_toio(dst, fb->pitches[0], vmap, fb, clip);
> -
> -	/* Always scanout image at VRAM offset 0 */
> -	mgag200_set_startadd(mdev, (u32)0);
> -	mgag200_set_offset(mdev, fb);
>   }
>   
>   static void
> @@ -904,6 +900,9 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>   	mgag200_enable_display(mdev);
>   
>   	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
> +	/* Always scanout image at VRAM offset 0 */
> +	mgag200_set_startadd(mdev, (u32)0);
> +	mgag200_set_offset(mdev, fb);
>   }
>   
>   static void
> @@ -959,12 +958,18 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>   	struct drm_framebuffer *fb = state->fb;
>   	struct drm_rect damage;
> +	struct drm_atomic_helper_damage_iter iter;
>   
>   	if (!fb)
>   		return;
>   
> -	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
> +	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
>   		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
> +	}
> +	/* Always scanout image at VRAM offset 0 */
> +	mgag200_set_startadd(mdev, (u32)0);
> +	mgag200_set_offset(mdev, fb);
>   }
>   
>   static struct drm_crtc_state *

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/4] mgag200: remove unused flag

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2576 bytes --]

Hi

Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
> The flag MGAG200_FLAG_HW_BUG_NO_STARTADD is no more used, because
> the framebuffer is now always at offset 0.

Oh, well. I remember that thing. It took us a long time to find and fix 
this problem. Back then, mgag200 still used VRAM helpers, which do page 
flipping in video memory. Displays remained dark on some systems without 
any clear cause. We added the flag to work around the broken HW.

I left the flag in for reference. Instead of removing it, I think we 
should add a drm_WARN_ON_ONCE() in mgag200_set_start_add() if the flag 
is set and offset is non-zero.

Best regards
Thomas

> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
>   drivers/gpu/drm/mgag200/mgag200_drv.h | 3 ---
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 217844d71ab5..8659e1ca8009 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -306,8 +306,7 @@ mgag200_device_create(struct pci_dev *pdev, enum mga_type type, unsigned long fl
>   static const struct pci_device_id mgag200_pciidlist[] = {
>   	{ PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_PCI },
>   	{ PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_AGP },
> -	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> -		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
> +	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_A },
>   	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
>   	{ PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_EV },
>   	{ PCI_VENDOR_ID_MATROX, 0x532, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_WB },
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4368112023f7..c7b6dc771ab3 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -201,9 +201,6 @@ enum mga_type {
>   	G200_EW3,
>   };
>   
> -/* HW does not handle 'startadd' field correct. */
> -#define MGAG200_FLAG_HW_BUG_NO_STARTADD	(1ul << 8)
> -
>   #define MGAG200_TYPE_MASK	(0x000000ff)
>   #define MGAG200_FLAG_MASK	(0x00ffff00)
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/4] mgag200: remove mgag200_probe_vram()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3117 bytes --]

Hi

Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
> This function writes some pattern to video memory, to check for conflict.
> In case of conflicts, it returns a random memory capacity (offset of the
> conflict).
> Using devm_arch_io_reserve_memtype_wc() should garantee that no other
> driver is using this memory region.
> In case of real memory conflicts, as it is video memory, the user will
> notice it easily. So there is no need for this function.

I don't think we can remove this function. It doesn't test for 
concurrent access, but for non-existing video memory. Apparently, some 
systems have larger PCI apertures than actual video memory.

Best regards
Thomas

> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mm.c | 50 ++++------------------------
>   1 file changed, 7 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
> index fa996d46feed..68299b560a98 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
> @@ -32,48 +32,6 @@
>   
>   #include "mgag200_drv.h"
>   
> -static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
> -				 size_t size)
> -{
> -	int offset;
> -	int orig;
> -	int test1, test2;
> -	int orig1, orig2;
> -	size_t vram_size;
> -
> -	/* Probe */
> -	orig = ioread16(mem);
> -	iowrite16(0, mem);
> -
> -	vram_size = size;
> -
> -	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
> -		vram_size = vram_size - 0x400000;
> -
> -	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
> -		orig1 = ioread8(mem + offset);
> -		orig2 = ioread8(mem + offset + 0x100);
> -
> -		iowrite16(0xaa55, mem + offset);
> -		iowrite16(0xaa55, mem + offset + 0x100);
> -
> -		test1 = ioread16(mem + offset);
> -		test2 = ioread16(mem);
> -
> -		iowrite16(orig1, mem + offset);
> -		iowrite16(orig2, mem + offset + 0x100);
> -
> -		if (test1 != 0xaa55)
> -			break;
> -
> -		if (test2)
> -			break;
> -	}
> -
> -	iowrite16(orig, mem);
> -
> -	return offset - 65536;
> -}
>   
>   int mgag200_mm_init(struct mga_device *mdev)
>   {
> @@ -81,6 +39,7 @@ int mgag200_mm_init(struct mga_device *mdev)
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>   	u8 misc;
>   	resource_size_t start, len;
> +	size_t vram_size;
>   
>   	WREG_ECRT(0x04, 0x00);
>   
> @@ -106,7 +65,12 @@ int mgag200_mm_init(struct mga_device *mdev)
>   	if (!mdev->vram)
>   		return -ENOMEM;
>   
> -	mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
> +	vram_size = len;
> +	/* G200_EW3 has only 12MB of memory */
> +	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
> +		vram_size -= 0x400000;
> +
> +	mdev->mc.vram_size = vram_size;
>   	mdev->mc.vram_base = start;
>   	mdev->mc.vram_window = len;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] mgag200: Optimize damage clips

[Jocelyn Falempe]

On 04/05/2022 12:04, Thomas Zimmermann wrote:
> Hi
> 
> Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
>> when there are multiple damage clips, previous code merged them into one
>> big rectangle. As the Matrox memory is very slow, it's faster to copy 
>> each
>> damage clip.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> How do you measure the performance? It seems as if it depends a lot on 
> the nature of the screen update.  But maybe using that loop is faster in 
> the general case with other drivers as well.

I used ktime_get_ts64() to measure the time to copy the damage 
rectangles to VRAM.
It was always faster to copy multiple small rectangles than one big. 
(using Gnome shell/Wayland, dragging windows around).
The memory bandwith of mgag200 is so slow, that accessing it in a more 
"linear" way, is clearly not worth.

I didn't save the numbers, but copying the whole frame was around 130ms 
(for 1280x1024x32).

It may also depends on the userspace optimizations (like if a userspace 
send overlapping rectangles).

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index cff2e76f3fa0..2bc380a85996 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -855,10 +855,6 @@ mgag200_handle_damage(struct mga_device *mdev, 
>> struct drm_framebuffer *fb,
>>       dst += drm_fb_clip_offset(fb->pitches[0], fb->format, clip);
>>       drm_fb_memcpy_toio(dst, fb->pitches[0], vmap, fb, clip);
>> -
>> -    /* Always scanout image at VRAM offset 0 */
>> -    mgag200_set_startadd(mdev, (u32)0);
>> -    mgag200_set_offset(mdev, fb);
>>   }
>>   static void
>> @@ -904,6 +900,9 @@ mgag200_simple_display_pipe_enable(struct 
>> drm_simple_display_pipe *pipe,
>>       mgag200_enable_display(mdev);
>>       mgag200_handle_damage(mdev, fb, &fullscreen, 
>> &shadow_plane_state->data[0]);
>> +    /* Always scanout image at VRAM offset 0 */
>> +    mgag200_set_startadd(mdev, (u32)0);
>> +    mgag200_set_offset(mdev, fb);
>>   }
>>   static void
>> @@ -959,12 +958,18 @@ mgag200_simple_display_pipe_update(struct 
>> drm_simple_display_pipe *pipe,
>>       struct drm_shadow_plane_state *shadow_plane_state = 
>> to_drm_shadow_plane_state(state);
>>       struct drm_framebuffer *fb = state->fb;
>>       struct drm_rect damage;
>> +    struct drm_atomic_helper_damage_iter iter;
>>       if (!fb)
>>           return;
>> -    if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>> +    drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> +    drm_atomic_for_each_plane_damage(&iter, &damage) {
>>           mgag200_handle_damage(mdev, fb, &damage, 
>> &shadow_plane_state->data[0]);
>> +    }
>> +    /* Always scanout image at VRAM offset 0 */
>> +    mgag200_set_startadd(mdev, (u32)0);
>> +    mgag200_set_offset(mdev, fb);
>>   }
>>   static struct drm_crtc_state *
> 

Re: [PATCH 3/4] mgag200: remove unused flag

[Jocelyn Falempe]

On 04/05/2022 12:12, Thomas Zimmermann wrote:
> Hi
> 
> Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
>> The flag MGAG200_FLAG_HW_BUG_NO_STARTADD is no more used, because
>> the framebuffer is now always at offset 0.
> 
> Oh, well. I remember that thing. It took us a long time to find and fix 
> this problem. Back then, mgag200 still used VRAM helpers, which do page 
> flipping in video memory. Displays remained dark on some systems without 
> any clear cause. We added the flag to work around the broken HW.
> 
> I left the flag in for reference. Instead of removing it, I think we 
> should add a drm_WARN_ON_ONCE() in mgag200_set_start_add() if the flag 
> is set and offset is non-zero.

sure, I can do that in v2.

> 
> Best regards
> Thomas
> 
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
>>   drivers/gpu/drm/mgag200/mgag200_drv.h | 3 ---
>>   2 files changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
>> b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 217844d71ab5..8659e1ca8009 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -306,8 +306,7 @@ mgag200_device_create(struct pci_dev *pdev, enum 
>> mga_type type, unsigned long fl
>>   static const struct pci_device_id mgag200_pciidlist[] = {
>>       { PCI_VENDOR_ID_MATROX, 0x520, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_PCI },
>>       { PCI_VENDOR_ID_MATROX, 0x521, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_AGP },
>> -    { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>> -        G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>> +    { PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_SE_A },
>>       { PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_SE_B },
>>       { PCI_VENDOR_ID_MATROX, 0x530, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_EV },
>>       { PCI_VENDOR_ID_MATROX, 0x532, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
>> G200_WB },
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h 
>> b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 4368112023f7..c7b6dc771ab3 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -201,9 +201,6 @@ enum mga_type {
>>       G200_EW3,
>>   };
>> -/* HW does not handle 'startadd' field correct. */
>> -#define MGAG200_FLAG_HW_BUG_NO_STARTADD    (1ul << 8)
>> -
>>   #define MGAG200_TYPE_MASK    (0x000000ff)
>>   #define MGAG200_FLAG_MASK    (0x00ffff00)
> 

Re: [PATCH 4/4] mgag200: remove mgag200_probe_vram()

[Jocelyn Falempe]

On 04/05/2022 12:16, Thomas Zimmermann wrote:
> Hi
> 
> Am 26.04.22 um 18:41 schrieb Jocelyn Falempe:
>> This function writes some pattern to video memory, to check for conflict.
>> In case of conflicts, it returns a random memory capacity (offset of the
>> conflict).
>> Using devm_arch_io_reserve_memtype_wc() should garantee that no other
>> driver is using this memory region.
>> In case of real memory conflicts, as it is video memory, the user will
>> notice it easily. So there is no need for this function.
> 
> I don't think we can remove this function. It doesn't test for 
> concurrent access, but for non-existing video memory. Apparently, some 
> systems have larger PCI apertures than actual video memory.
Ok, I tough writing to non-existing memory would raise an exception/Bus 
error. If it's silently ignored by hardware, this is probably the only 
way to make sure the memory is there.

let's drop this patch in v2 ;)

> 
> Best regards
> Thomas
> 
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mm.c | 50 ++++------------------------
>>   1 file changed, 7 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> index fa996d46feed..68299b560a98 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> @@ -32,48 +32,6 @@
>>   #include "mgag200_drv.h"
>> -static size_t mgag200_probe_vram(struct mga_device *mdev, void 
>> __iomem *mem,
>> -                 size_t size)
>> -{
>> -    int offset;
>> -    int orig;
>> -    int test1, test2;
>> -    int orig1, orig2;
>> -    size_t vram_size;
>> -
>> -    /* Probe */
>> -    orig = ioread16(mem);
>> -    iowrite16(0, mem);
>> -
>> -    vram_size = size;
>> -
>> -    if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
>> -        vram_size = vram_size - 0x400000;
>> -
>> -    for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
>> -        orig1 = ioread8(mem + offset);
>> -        orig2 = ioread8(mem + offset + 0x100);
>> -
>> -        iowrite16(0xaa55, mem + offset);
>> -        iowrite16(0xaa55, mem + offset + 0x100);
>> -
>> -        test1 = ioread16(mem + offset);
>> -        test2 = ioread16(mem);
>> -
>> -        iowrite16(orig1, mem + offset);
>> -        iowrite16(orig2, mem + offset + 0x100);
>> -
>> -        if (test1 != 0xaa55)
>> -            break;
>> -
>> -        if (test2)
>> -            break;
>> -    }
>> -
>> -    iowrite16(orig, mem);
>> -
>> -    return offset - 65536;
>> -}
>>   int mgag200_mm_init(struct mga_device *mdev)
>>   {
>> @@ -81,6 +39,7 @@ int mgag200_mm_init(struct mga_device *mdev)
>>       struct pci_dev *pdev = to_pci_dev(dev->dev);
>>       u8 misc;
>>       resource_size_t start, len;
>> +    size_t vram_size;
>>       WREG_ECRT(0x04, 0x00);
>> @@ -106,7 +65,12 @@ int mgag200_mm_init(struct mga_device *mdev)
>>       if (!mdev->vram)
>>           return -ENOMEM;
>> -    mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
>> +    vram_size = len;
>> +    /* G200_EW3 has only 12MB of memory */
>> +    if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
>> +        vram_size -= 0x400000;
>> +
>> +    mdev->mc.vram_size = vram_size;
>>       mdev->mc.vram_base = start;
>>       mdev->mc.vram_window = len;
>