[RFC PATCH 0/2] drm/mgag200: Use 24bit format in VRAM

[RFC PATCH 0/2] drm/mgag200: Use 24bit format in VRAM

[Jocelyn Falempe]

The bandwidth between system memory and VRAM is very limited
on G200.
So when using a 32bit framebuffer on system memory, convert it to 24bit
when copying the frame to the VRAM, this allows to go 33% faster.
Converting the format on the fly is negligible, even on low end CPU.

[PATCH 1/2] drm/mgag200: simplify offset and scale computation.
[PATCH 2/2] drm/mgag200: Use 24bit format in VRAM

drivers/gpu/drm/mgag200/mgag200_mode.c | 87 ++++++++++++++++++++++++++++++++++++---------------------------------------------------
 1 file changed, 36 insertions(+), 51 deletions(-)

[PATCH 1/2] drm/mgag200: simplify offset and scale computation.

[Jocelyn Falempe]

Now that the driver handles only 16, 24 and 32-bit framebuffer,
it can be simplified.

No functional changes.

offset:
16bit: (bppshift = 1)
offset = width >> (4 - bppshift) => width / 8 => pitch / 16

24bit:  (bppshift = 0)
offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16

32bit:  (bppshift = 2)
offset = width >> (4 - bppshift) => width / 4 => pitch / 16

scale:
16bit:
scale = (1 << bppshift) - 1 => 1
24bit:
scale = ((1 << bppshift) * 3) - 1 => 2
32bit:
scale = (1 << bppshift) - 1 => 3

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 0a5aaf78172a..e3f0da338b95 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
 	WREG8(MGA_MISC_OUT, misc);
 }
 
-static u8 mgag200_get_bpp_shift(const struct drm_format_info *format)
-{
-	static const u8 bpp_shift[] = {0, 1, 0, 2};
-
-	return bpp_shift[format->cpp[0] - 1];
-}
-
 /*
  * Calculates the HW offset value from the framebuffer's pitch. The
  * offset is a multiple of the pixel size and depends on the display
- * format.
+ * format. With width in pixels and pitch in bytes, the formula is:
+ * offset = width * bpp / 128 = pitch / 16
  */
 static u32 mgag200_calculate_offset(struct mga_device *mdev,
 				    const struct drm_framebuffer *fb)
 {
-	u32 offset = fb->pitches[0] / fb->format->cpp[0];
-	u8 bppshift = mgag200_get_bpp_shift(fb->format);
-
-	if (fb->format->cpp[0] * 8 == 24)
-		offset = (offset * 3) >> (4 - bppshift);
-	else
-		offset = offset >> (4 - bppshift);
-
-	return offset;
+	return fb->pitches[0] >> 4;
 }
 
 static void mgag200_set_offset(struct mga_device *mdev,
@@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device *mdev,
 void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format)
 {
 	struct drm_device *dev = &mdev->base;
-	unsigned int bpp, bppshift, scale;
+	unsigned int scale;
 	u8 crtcext3, xmulctrl;
 
-	bpp = format->cpp[0] * 8;
-
-	bppshift = mgag200_get_bpp_shift(format);
-	switch (bpp) {
-	case 24:
-		scale = ((1 << bppshift) * 3) - 1;
-		break;
-	default:
-		scale = (1 << bppshift) - 1;
-		break;
-	}
-
-	RREG_ECRT(3, crtcext3);
-
-	switch (bpp) {
-	case 8:
-		xmulctrl = MGA1064_MUL_CTL_8bits;
-		break;
-	case 16:
-		if (format->depth == 15)
-			xmulctrl = MGA1064_MUL_CTL_15bits;
-		else
-			xmulctrl = MGA1064_MUL_CTL_16bits;
+	switch (format->format) {
+	case DRM_FORMAT_RGB565:
+		xmulctrl = MGA1064_MUL_CTL_16bits;
 		break;
-	case 24:
+	case DRM_FORMAT_RGB888:
 		xmulctrl = MGA1064_MUL_CTL_24bits;
 		break;
-	case 32:
+	case DRM_FORMAT_XRGB8888:
 		xmulctrl = MGA1064_MUL_CTL_32_24bits;
 		break;
 	default:
 		/* BUG: We should have caught this problem already. */
-		drm_WARN_ON(dev, "invalid format depth\n");
+		drm_WARN_ON(dev, "invalid drm format\n");
 		return;
 	}
 
-	crtcext3 &= ~GENMASK(2, 0);
-	crtcext3 |= scale;
-
 	WREG_DAC(MGA1064_MUL_CTL, xmulctrl);
 
 	WREG_GFX(0, 0x00);
@@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in
 	WREG_GFX(7, 0x0f);
 	WREG_GFX(8, 0x0f);
 
+	/* scale is the number of bytes per pixels - 1 */
+	scale = format->cpp[0] - 1;
+
+	RREG_ECRT(3, crtcext3);
+	crtcext3 &= ~GENMASK(2, 0);
+	crtcext3 |= scale;
 	WREG_ECRT(3, crtcext3);
 }
 
-- 
2.39.2

[PATCH 2/2] drm/mgag200: Use 24bit format in VRAM

[Jocelyn Falempe]

The bandwidth between system memory and VRAM is very limited
on G200.
So when using a 32bit framebuffer on system memory, convert it to 24bit
when copying the frame to the VRAM, this allows to go 33% faster.
Converting the format on the fly is negligible, even on low end CPU.

small benchmark on my Dell T310:
1280x1024 32bits: ~125ms to transfert a single frame.
1280x1024 24bits: ~95ms

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e3f0da338b95..a8d6b08bf959 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -289,6 +289,8 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
 static u32 mgag200_calculate_offset(struct mga_device *mdev,
 				    const struct drm_framebuffer *fb)
 {
+	if (fb->format->format == DRM_FORMAT_XRGB8888)
+		return (fb->pitches[0] * 3) >> 6;
 	return fb->pitches[0] >> 4;
 }
 
@@ -314,17 +316,16 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in
 	struct drm_device *dev = &mdev->base;
 	unsigned int scale;
 	u8 crtcext3, xmulctrl;
+	u8 cpp;
 
 	switch (format->format) {
 	case DRM_FORMAT_RGB565:
 		xmulctrl = MGA1064_MUL_CTL_16bits;
 		break;
+	case DRM_FORMAT_XRGB8888: /* use 24bit format in VRAM */
 	case DRM_FORMAT_RGB888:
 		xmulctrl = MGA1064_MUL_CTL_24bits;
 		break;
-	case DRM_FORMAT_XRGB8888:
-		xmulctrl = MGA1064_MUL_CTL_32_24bits;
-		break;
 	default:
 		/* BUG: We should have caught this problem already. */
 		drm_WARN_ON(dev, "invalid drm format\n");
@@ -346,8 +347,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in
 	WREG_GFX(7, 0x0f);
 	WREG_GFX(8, 0x0f);
 
+	cpp = format->cpp[0];
+	if (cpp == 4) /* use 24 bit format in VRAM */
+		cpp = 3;
+
 	/* scale is the number of bytes per pixels - 1 */
-	scale = format->cpp[0] - 1;
+	scale = cpp - 1;
 
 	RREG_ECRT(3, crtcext3);
 	crtcext3 &= ~GENMASK(2, 0);
@@ -403,8 +408,19 @@ static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_ma
 {
 	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
 
-	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
-	drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
+	if (fb->format->format == DRM_FORMAT_XRGB8888) {
+		/* use 24 bit format for VRAM, to save memory bandwidth,
+		 * converting on the fly is much faster than sending the bytes
+		 */
+		u32 dst_pitch[3] = {(fb->pitches[0] * 3) / 4,
+				    (fb->pitches[1] * 3) / 4,
+				    (fb->pitches[2] * 3) / 4};
+		iosys_map_incr(&dst, clip->y1 * dst_pitch[0] + clip->x1 * 3);
+		drm_fb_xrgb8888_to_rgb888(&dst, dst_pitch, vmap, fb, clip);
+	} else {
+		iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
+		drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
+	}
 }
 
 /*
-- 
2.39.2

Re: [PATCH 1/2] drm/mgag200: simplify offset and scale computation.

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

Hello Jocelyn,

> Now that the driver handles only 16, 24 and 32-bit framebuffer,
> it can be simplified.
>
> No functional changes.
>
> offset:
> 16bit: (bppshift = 1)
> offset = width >> (4 - bppshift) => width / 8 => pitch / 16
>
> 24bit:  (bppshift = 0)
> offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16
>
> 32bit:  (bppshift = 2)
> offset = width >> (4 - bppshift) => width / 4 => pitch / 16
>
> scale:
> 16bit:
> scale = (1 << bppshift) - 1 => 1
> 24bit:
> scale = ((1 << bppshift) * 3) - 1 => 2
> 32bit:
> scale = (1 << bppshift) - 1 => 3
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---

Thanks a nice simplication indeed.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/2] drm/mgag200: Use 24bit format in VRAM

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

> The bandwidth between system memory and VRAM is very limited
> on G200.
> So when using a 32bit framebuffer on system memory, convert it to 24bit
> when copying the frame to the VRAM, this allows to go 33% faster.
> Converting the format on the fly is negligible, even on low end CPU.
>
> small benchmark on my Dell T310:
> 1280x1024 32bits: ~125ms to transfert a single frame.
> 1280x1024 24bits: ~95ms
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---

I assume that the alpha channel is unused on this display HW and is just
exposed to user-space to make the DRM driver more compatible ?

If so, probably has to be mentioned in the changelog but other than that:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/2] drm/mgag200: Use 24bit format in VRAM

[Jocelyn Falempe]

On 12/04/2023 17:13, Javier Martinez Canillas wrote:
> Jocelyn Falempe <jfalempe@redhat.com> writes:
> 
>> The bandwidth between system memory and VRAM is very limited
>> on G200.
>> So when using a 32bit framebuffer on system memory, convert it to 24bit
>> when copying the frame to the VRAM, this allows to go 33% faster.
>> Converting the format on the fly is negligible, even on low end CPU.
>>
>> small benchmark on my Dell T310:
>> 1280x1024 32bits: ~125ms to transfert a single frame.
>> 1280x1024 24bits: ~95ms
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
> 
> I assume that the alpha channel is unused on this display HW and is just
> exposed to user-space to make the DRM driver more compatible ?

Yes, alpha channel is dropped by the hardware, and has no impact on the 
display.
Most userspace now prefer 32bit framebuffer. I know Gnome/wayland 
default to 32bit, and probably other DE are doing the same.

> 
> If so, probably has to be mentioned in the changelog but other than that:

ok I will add this in the commit message.

> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

Thanks,

-- 

Jocelyn

Re: [RFC PATCH 0/2] drm/mgag200: Use 24bit format in VRAM

[Thomas Zimmermann]

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

Hi

Am 12.04.23 um 15:39 schrieb Jocelyn Falempe:
> The bandwidth between system memory and VRAM is very limited
> on G200.
> So when using a 32bit framebuffer on system memory, convert it to 24bit
> when copying the frame to the VRAM, this allows to go 33% faster.
> Converting the format on the fly is negligible, even on low end CPU.

I'm skeptical about this idea. We emulated a number of formats in 
simpledrm and got a lot of flames and pushback. The argument was that we 
should export the formats that hardware supports and not pretend to 
support anything else. The only exception allowed was emulating 
XRGB8888, because it's the common ground hat everything in userspace 
supports.

I see that this is a bit different from your patches, but not so much. 
When userspace wants 32-bit XRGB, it should get it if possible.

I'd rather suggest to set the console to 16 bit and also resort the 
formats array. It is supposed to be sorted by preference. RGB565 should 
maybe be the top most entry, followed by RGB888. Then you'd have to 
teach userspace to respect these settings. I'm not sure if all 
compositors do.

Best regards
Thomas

> 
> [PATCH 1/2] drm/mgag200: simplify offset and scale computation.
> [PATCH 2/2] drm/mgag200: Use 24bit format in VRAM
> 
> drivers/gpu/drm/mgag200/mgag200_mode.c | 87 ++++++++++++++++++++++++++++++++++++---------------------------------------------------
>   1 file changed, 36 insertions(+), 51 deletions(-)
> 
> 
> 

-- 
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: [RFC PATCH 0/2] drm/mgag200: Use 24bit format in VRAM

[Jocelyn Falempe]

On 13/04/2023 21:29, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.04.23 um 15:39 schrieb Jocelyn Falempe:
>> The bandwidth between system memory and VRAM is very limited
>> on G200.
>> So when using a 32bit framebuffer on system memory, convert it to 24bit
>> when copying the frame to the VRAM, this allows to go 33% faster.
>> Converting the format on the fly is negligible, even on low end CPU.
> 
> I'm skeptical about this idea. We emulated a number of formats in 
> simpledrm and got a lot of flames and pushback. The argument was that we 
> should export the formats that hardware supports and not pretend to 
> support anything else. The only exception allowed was emulating 
> XRGB8888, because it's the common ground hat everything in userspace 
> supports.
> 
> I see that this is a bit different from your patches, but not so much. 
> When userspace wants 32-bit XRGB, it should get it if possible.

The hardware will drop the 8bit alpha anyway, there is no image quality 
loss. So I find it better to drop it before sending it to the hardware 
to save bandwidth. As the mgag200 doesn't expose any other 
functionality, the userspace can't even read the VRAM back, so it's 
unlikely to cause issue.

> 
> I'd rather suggest to set the console to 16 bit and also resort the 
> formats array. It is supposed to be sorted by preference. RGB565 should 
> maybe be the top most entry, followed by RGB888. Then you'd have to 
> teach userspace to respect these settings. I'm not sure if all 
> compositors do.
> 

I don't think userspace cares much about very old hardware like this 
one. I would rather make it work as good as possible with current userspace.
For example Gnome/Wayland won't work in 16bit or 24bit pixel depth, and 
it would be much harder to add support in the compositor than this ~36 
lines patch. Other compositors are probably expecting 32bit hardware too.
mgag200 is also likely the last hardware from this era that's still 
alive, so we can't expect userspace to add specific support for it.


We can still change the format array order, but I would put 24bit first, 
as 16 bit is a bit ugly nowadays.

> Best regards
> Thomas
> 
>>
>> [PATCH 1/2] drm/mgag200: simplify offset and scale computation.
>> [PATCH 2/2] drm/mgag200: Use 24bit format in VRAM
>>
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 87 
>> ++++++++++++++++++++++++++++++++++++---------------------------------------------------
>>   1 file changed, 36 insertions(+), 51 deletions(-)
>>
>>
>>
> 


Best regards,

-- 

Jocelyn

Re: [RFC PATCH 0/2] drm/mgag200: Use 24bit format in VRAM

[Thomas Zimmermann]

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

Hi

Am 14.04.23 um 09:59 schrieb Jocelyn Falempe:
> On 13/04/2023 21:29, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.04.23 um 15:39 schrieb Jocelyn Falempe:
>>> The bandwidth between system memory and VRAM is very limited
>>> on G200.
>>> So when using a 32bit framebuffer on system memory, convert it to 24bit
>>> when copying the frame to the VRAM, this allows to go 33% faster.
>>> Converting the format on the fly is negligible, even on low end CPU.
>>
>> I'm skeptical about this idea. We emulated a number of formats in 
>> simpledrm and got a lot of flames and pushback. The argument was that 
>> we should export the formats that hardware supports and not pretend to 
>> support anything else. The only exception allowed was emulating 
>> XRGB8888, because it's the common ground hat everything in userspace 
>> supports.
>>
>> I see that this is a bit different from your patches, but not so much. 
>> When userspace wants 32-bit XRGB, it should get it if possible.
> 
> The hardware will drop the 8bit alpha anyway, there is no image quality 
> loss. So I find it better to drop it before sending it to the hardware 
> to save bandwidth. As the mgag200 doesn't expose any other 
> functionality, the userspace can't even read the VRAM back, so it's 
> unlikely to cause issue.

Believe me, I know that. :) We also made such arguments in that 
discussion around simpledrm and the answer was a big NO. This is 
considered a "rendering problem," which are better solved in userspace. 
If you get overall consent from DRM devs that this optimization is OK, 
I'm not going to block it. But I'm not going to fight for it either.

> 
>>
>> I'd rather suggest to set the console to 16 bit and also resort the 
>> formats array. It is supposed to be sorted by preference. RGB565 
>> should maybe be the top most entry, followed by RGB888. Then you'd 
>> have to teach userspace to respect these settings. I'm not sure if all 
>> compositors do.
>>
> 
> I don't think userspace cares much about very old hardware like this 
> one. I would rather make it work as good as possible with current 
> userspace.
> For example Gnome/Wayland won't work in 16bit or 24bit pixel depth, and 
> it would be much harder to add support in the compositor than this ~36 
> lines patch. Other compositors are probably expecting 32bit hardware too.
> mgag200 is also likely the last hardware from this era that's still 
> alive, so we can't expect userspace to add specific support for it.
> 
> 
> We can still change the format array order, but I would put 24bit first, 
> as 16 bit is a bit ugly nowadays.

24-bit is dead. Most userspace doesn't support it or it's broken or 
looks garbled. Even a decade ago, only pixman got it right. That 
probably didn't improve since.

Best regards
Thomas

> 
>> Best regards
>> Thomas
>>
>>>
>>> [PATCH 1/2] drm/mgag200: simplify offset and scale computation.
>>> [PATCH 2/2] drm/mgag200: Use 24bit format in VRAM
>>>
>>> drivers/gpu/drm/mgag200/mgag200_mode.c | 87 
>>> ++++++++++++++++++++++++++++++++++++---------------------------------------------------
>>>   1 file changed, 36 insertions(+), 51 deletions(-)
>>>
>>>
>>>
>>
> 
> 
> Best regards,
> 

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