| Message ID | 20230412133912.610294-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mgag200: Use 24bit format in VRAM | expand |
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(-) > > >
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,
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, >