[PATCH v2 0/3] mgag200: Improve damage handling

[PATCH v2 0/3] mgag200: Improve damage handling

[Jocelyn Falempe]

mgag200: Improve damage handling

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)

v2:
 Drop remove mgag200_probe_vram()
 Rewrote the patch to warn if startaddr is not 0, and hw doesn't
  support it. (instead of removing the unused flag).

Thanks,

-- 

Jocelyn


Jocelyn Falempe (3):
  mgag200: Add FB_DAMAGE_CLIPS support
  mgag200: Optimize damage clips
  mgag200: Warn once if trying to set start address on broken hardware.

 drivers/gpu/drm/mgag200/mgag200_mode.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

-- 
2.35.1

[PATCH v2 1/3] 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>
Reviewed-by: Lyude Paul <lyude@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);
 
-- 
2.35.1

[PATCH v2 2/3] 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>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 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 v2 3/3] mgag200: Warn once if trying to set start address on broken hardware.

[Jocelyn Falempe]

Some MGA200 hardware are broken, and can't use a start address > 0

v2: Warn if startaddr is not 0, and hw doesn't support it.
  (instead of removing MGAG200_FLAG_HW_BUG_NO_STARTADD)

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 2bc380a85996..d2cf26b3cee0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -223,6 +223,9 @@ static void mgag200_set_startadd(struct mga_device *mdev,
 
 	startadd = offset / 8;
 
+	if (startadd > 0)
+		drm_WARN_ON_ONCE(dev, mdev->flags & MGAG200_FLAG_HW_BUG_NO_STARTADD);
+
 	/*
 	 * Can't store addresses any higher than that, but we also
 	 * don't have more than 16 MiB of memory, so it should be fine.
-- 
2.35.1

Re: [PATCH v2 3/3] mgag200: Warn once if trying to set start address on broken hardware.

[Thomas Zimmermann]

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

Hi

Am 04.05.22 um 15:40 schrieb Jocelyn Falempe:
> Some MGA200 hardware are broken, and can't use a start address > 0

I'm not a native speaker, but 'is broken' (without comma) would sound 
better to me. The period at the end of the sentence is missing.

Best regards
Thomas

> 
> v2: Warn if startaddr is not 0, and hw doesn't support it.
>    (instead of removing MGAG200_FLAG_HW_BUG_NO_STARTADD)
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 2bc380a85996..d2cf26b3cee0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -223,6 +223,9 @@ static void mgag200_set_startadd(struct mga_device *mdev,
>   
>   	startadd = offset / 8;
>   
> +	if (startadd > 0)
> +		drm_WARN_ON_ONCE(dev, mdev->flags & MGAG200_FLAG_HW_BUG_NO_STARTADD);
> +
>   	/*
>   	 * Can't store addresses any higher than that, but we also
>   	 * don't have more than 16 MiB of memory, so it should be fine.

-- 
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 v2 0/3] mgag200: Improve damage handling

[Thomas Zimmermann]

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

Hi Jocelyn,

with my comment on 3/3 considered, you can add

Reviewed-by: Thomas Zimemrmann <tzimemrmann@suse.de>

to this patchset.

You should get commit access for drm-misc, so that you can land the 
patchset on your own. Please see

   https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

for how to do that.

Best regards
Thomas

Am 04.05.22 um 15:40 schrieb Jocelyn Falempe:
> mgag200: Improve damage handling
> 
> 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)
> 
> v2:
>   Drop remove mgag200_probe_vram()
>   Rewrote the patch to warn if startaddr is not 0, and hw doesn't
>    support it. (instead of removing the unused flag).
> 
> Thanks,
> 

-- 
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 v2 0/3] mgag200: Improve damage handling

[Jocelyn Falempe]

On 04/05/2022 16:59, Thomas Zimmermann wrote:
> Hi Jocelyn,
> 
> with my comment on 3/3 considered, you can add
> 
> Reviewed-by: Thomas Zimemrmann <tzimemrmann@suse.de>
> 
> to this patchset.
> 
> You should get commit access for drm-misc, so that you can land the 
> patchset on your own. Please see
> 
>    https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html
> 
> for how to do that.

Thanks for your reviews.
I created the issue to get commit access :
https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/433

Best regards,

-- 

Jocelyn
> 
> Best regards
> Thomas
> 
> Am 04.05.22 um 15:40 schrieb Jocelyn Falempe:
>> mgag200: Improve damage handling
>>
>> 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)
>>
>> v2:
>>   Drop remove mgag200_probe_vram()
>>   Rewrote the patch to warn if startaddr is not 0, and hw doesn't
>>    support it. (instead of removing the unused flag).
>>
>> Thanks,
>>
> 

Re: [PATCH v2 0/3] mgag200: Improve damage handling

[Jocelyn Falempe]

On 04/05/2022 16:59, Thomas Zimmermann wrote:
> Hi Jocelyn,
> 
> with my comment on 3/3 considered, you can add
> 
> Reviewed-by: Thomas Zimemrmann <tzimemrmann@suse.de>
> 
> to this patchset.
> 
> You should get commit access for drm-misc, so that you can land the 
> patchset on your own. Please see
> 
>    https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html
> 
> for how to do that.

I pushed the patches with the requested changes to drm-misc-next last 
Thursday.

Thanks a lot for your help.

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
> Am 04.05.22 um 15:40 schrieb Jocelyn Falempe:
>> mgag200: Improve damage handling
>>
>> 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)
>>
>> v2:
>>   Drop remove mgag200_probe_vram()
>>   Rewrote the patch to warn if startaddr is not 0, and hw doesn't
>>    support it. (instead of removing the unused flag).
>>
>> Thanks,
>>
>