| Message ID | 20250325171716.154097-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/cirrus-qemu: Various fixes | expand |
On Tue, Mar 25, 2025 at 06:12:51PM +0100, Thomas Zimmermann wrote: > Remove internal adjustments to framebuffer format from cirrus-qemu > driver. The driver did this to support higher resolutions by reducing > the per-pixel memory consumption. > > DRM has a policy of exporting formats as they are implemented in > hardware. So avoid internal adjustments if possible. Well. While this policy makes sense for modern hardware this is IMHO not the case for the cirrus. First, because there is almost no userspace which can handle the ancient 24 bpp format (DRM_FORMAT_RGB888). Second, because there is no way to communicate the hardware constrains of the cirrus. userspace can query the formats, and userspace can query the resolutions, but there is no way to tell userspace that not all combinations are valid and that you have to go for the DRM_FORMAT_RGB565 format if you want higher resolutions. Essentially the format conversations allows the driver to hide the oddities of the prehistoric hardware from userspace, so things are more smooth when running wayland on the cirrus. take care, Gerd PS: https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/ still applies of course.
Hi, first of all, what about the other patches? - Patch 1 is a bugfix. - Patch 4 depends on this one. - Patch 2 should be given consideration. Am 26.03.25 um 11:35 schrieb Gerd Hoffmann: > On Tue, Mar 25, 2025 at 06:12:51PM +0100, Thomas Zimmermann wrote: >> Remove internal adjustments to framebuffer format from cirrus-qemu >> driver. The driver did this to support higher resolutions by reducing >> the per-pixel memory consumption. >> >> DRM has a policy of exporting formats as they are implemented in >> hardware. So avoid internal adjustments if possible. > Well. While this policy makes sense for modern hardware this is IMHO > not the case for the cirrus. > > First, because there is almost no userspace which can handle the ancient > 24 bpp format (DRM_FORMAT_RGB888). True, there's really just 32 bpp or 16. > > Second, because there is no way to communicate the hardware constrains > of the cirrus. userspace can query the formats, and userspace can query > the resolutions, but there is no way to tell userspace that not all > combinations are valid and that you have to go for the DRM_FORMAT_RGB565 > format if you want higher resolutions. The viable strategy for user space is to allocate a variety of different configs and check them one by one, thus filtering out the ones that work. Weston failed to do this for me while I experimented with such low-end scenarios. It tried to run an atomic state with a resolution at 32 bpp, which did not work. Weston just ignored the problem and the display went stale. Xorg was clever enough to pick 16-bpp colors. > > Essentially the format conversations allows the driver to hide the > oddities of the prehistoric hardware from userspace, so things are > more smooth when running wayland on the cirrus. I'm aware of the situation. We've had similar discussions about other low-end hardware, but generally went with the hardware limits. Please note that there is a trade-off here: the effect of this series is that the maximum resolution will be limited to 800x600. If user space would appropriately validate atomic states, lower bpp could still support higher resolutions. But converting color formats on the fly isn't free. I recently did some simple measurements in a different context and converting from 32 bpp to 16 bpp took 3 times as long as memcpy'ing the raw pixels. See the cover letter of [1]. AFAICT cirrus is currently paying CPU overhead for display resolution. [1] https://patchwork.freedesktop.org/series/146722/#rev1 > > take care, > Gerd > > PS: https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/ > still applies of course. It's been 10 years since you wrote that. So maybe it's time to re-consider cirrus' exceptions and just go for a 'dumb implementation'. Anyone can easily switch to better alternatives. Best regards Thomas >
On Wed, Mar 26, 2025 at 01:30:13PM +0100, Thomas Zimmermann wrote: > Hi, > > first of all, what about the other patches? > > - Patch 1 is a bugfix. > - Patch 4 depends on this one. > - Patch 2 should be given consideration. Looks reasonable to me. Don't feel like giving a reviewed-by due to not following drm development very closely any more, so Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > Second, because there is no way to communicate the hardware constrains > > of the cirrus. userspace can query the formats, and userspace can query > > the resolutions, but there is no way to tell userspace that not all > > combinations are valid and that you have to go for the DRM_FORMAT_RGB565 > > format if you want higher resolutions. > > The viable strategy for user space is to allocate a variety of different > configs and check them one by one, thus filtering out the ones that work. Last time I checked (which has been a few years) alot of existing software didn't do that. Maybe that changed with atomic becoming more mature though. > > Essentially the format conversations allows the driver to hide the > > oddities of the prehistoric hardware from userspace, so things are > > more smooth when running wayland on the cirrus. > > I'm aware of the situation. We've had similar discussions about other > low-end hardware, but generally went with the hardware limits. > > Please note that there is a trade-off here: the effect of this series is > that the maximum resolution will be limited to 800x600. Ah, ok, this is how you deal with the problem, go with the max resolution the cirrus can support at 32 bpp. Could be more explicit in the commit message, especially the 800x600 limit, there is a high chance people search for that when their display resolution changes after a kernel update. > If user space would appropriately validate atomic states, lower bpp > could still support higher resolutions. But converting color formats > on the fly isn't free. I recently did some simple measurements in a > different context and converting from 32 bpp to 16 bpp took 3 times as > long as memcpy'ing the raw pixels. Ouch. That is alot. > > PS: https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/ > > still applies of course. > > It's been 10 years since you wrote that. So maybe it's time to re-consider > cirrus' exceptions and just go for a 'dumb implementation'. Anyone can > easily switch to better alternatives. Fair enough. With an improved commit message: Acked-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
Hi Am 26.03.25 um 14:04 schrieb Gerd Hoffmann: > On Wed, Mar 26, 2025 at 01:30:13PM +0100, Thomas Zimmermann wrote: >> Hi, >> >> first of all, what about the other patches? >> >> - Patch 1 is a bugfix. >> - Patch 4 depends on this one. >> - Patch 2 should be given consideration. > Looks reasonable to me. Don't feel like giving a reviewed-by due to not > following drm development very closely any more, so > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > >>> Second, because there is no way to communicate the hardware constrains >>> of the cirrus. userspace can query the formats, and userspace can query >>> the resolutions, but there is no way to tell userspace that not all >>> combinations are valid and that you have to go for the DRM_FORMAT_RGB565 >>> format if you want higher resolutions. >> The viable strategy for user space is to allocate a variety of different >> configs and check them one by one, thus filtering out the ones that work. > Last time I checked (which has been a few years) alot of existing > software didn't do that. Maybe that changed with atomic becoming > more mature though. > >>> Essentially the format conversations allows the driver to hide the >>> oddities of the prehistoric hardware from userspace, so things are >>> more smooth when running wayland on the cirrus. >> I'm aware of the situation. We've had similar discussions about other >> low-end hardware, but generally went with the hardware limits. >> >> Please note that there is a trade-off here: the effect of this series is >> that the maximum resolution will be limited to 800x600. > Ah, ok, this is how you deal with the problem, go with the max > resolution the cirrus can support at 32 bpp. > > Could be more explicit in the commit message, especially the 800x600 > limit, there is a high chance people search for that when their display > resolution changes after a kernel update. > >> If user space would appropriately validate atomic states, lower bpp >> could still support higher resolutions. But converting color formats >> on the fly isn't free. I recently did some simple measurements in a >> different context and converting from 32 bpp to 16 bpp took 3 times as >> long as memcpy'ing the raw pixels. > Ouch. That is alot. > >>> PS: https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/ >>> still applies of course. >> It's been 10 years since you wrote that. So maybe it's time to re-consider >> cirrus' exceptions and just go for a 'dumb implementation'. Anyone can >> easily switch to better alternatives. > Fair enough. > > With an improved commit message: > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Thanks for reconsidering. I'll send out an updated series with an expanded commit message. Best regards Thomas > > take care, > Gerd >
diff --git a/drivers/gpu/drm/tiny/cirrus-qemu.c b/drivers/gpu/drm/tiny/cirrus-qemu.c index 0306433ec117..76744394e2a3 100644 --- a/drivers/gpu/drm/tiny/cirrus-qemu.c +++ b/drivers/gpu/drm/tiny/cirrus-qemu.c @@ -72,10 +72,6 @@ struct cirrus_device { struct cirrus_primary_plane_state { struct drm_shadow_plane_state base; - - /* HW scanout buffer */ - const struct drm_format_info *format; - unsigned int pitch; }; static inline struct cirrus_primary_plane_state * @@ -144,37 +140,6 @@ static void wreg_hdr(struct cirrus_device *cirrus, u8 val) iowrite8(val, cirrus->mmio + VGA_DAC_MASK); } -static const struct drm_format_info *cirrus_convert_to(struct drm_framebuffer *fb) -{ - if (fb->format->format == DRM_FORMAT_XRGB8888 && fb->pitches[0] > CIRRUS_MAX_PITCH) { - if (fb->width * 3 <= CIRRUS_MAX_PITCH) - /* convert from XR24 to RG24 */ - return drm_format_info(DRM_FORMAT_RGB888); - else - /* convert from XR24 to RG16 */ - return drm_format_info(DRM_FORMAT_RGB565); - } - return NULL; -} - -static const struct drm_format_info *cirrus_format(struct drm_framebuffer *fb) -{ - const struct drm_format_info *format = cirrus_convert_to(fb); - - if (format) - return format; - return fb->format; -} - -static int cirrus_pitch(struct drm_framebuffer *fb) -{ - const struct drm_format_info *format = cirrus_convert_to(fb); - - if (format) - return drm_format_info_min_pitch(format, 0, fb->width); - return fb->pitches[0]; -} - static void cirrus_set_start_address(struct cirrus_device *cirrus, u32 offset) { u32 addr; @@ -341,13 +306,10 @@ static int cirrus_primary_plane_helper_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) { struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); - struct cirrus_primary_plane_state *new_primary_plane_state = - to_cirrus_primary_plane_state(new_plane_state); struct drm_framebuffer *fb = new_plane_state->fb; struct drm_crtc *new_crtc = new_plane_state->crtc; struct drm_crtc_state *new_crtc_state = NULL; int ret; - unsigned int pitch; if (new_crtc) new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc); @@ -361,17 +323,12 @@ static int cirrus_primary_plane_helper_atomic_check(struct drm_plane *plane, else if (!new_plane_state->visible) return 0; - pitch = cirrus_pitch(fb); - /* validate size constraints */ - if (pitch > CIRRUS_MAX_PITCH) + if (fb->pitches[0] > CIRRUS_MAX_PITCH) return -EINVAL; - else if (pitch * fb->height > CIRRUS_VRAM_SIZE) + else if (fb->pitches[0] > CIRRUS_VRAM_SIZE / fb->height) return -EINVAL; - new_primary_plane_state->format = cirrus_format(fb); - new_primary_plane_state->pitch = pitch; - return 0; } @@ -380,15 +337,10 @@ static void cirrus_primary_plane_helper_atomic_update(struct drm_plane *plane, { struct cirrus_device *cirrus = to_cirrus(plane->dev); struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); - struct cirrus_primary_plane_state *primary_plane_state = - to_cirrus_primary_plane_state(plane_state); struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; - const struct drm_format_info *format = primary_plane_state->format; - unsigned int pitch = primary_plane_state->pitch; struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); - struct cirrus_primary_plane_state *old_primary_plane_state = - to_cirrus_primary_plane_state(old_plane_state); + struct drm_framebuffer *old_fb = old_plane_state->fb; struct iosys_map vaddr = IOSYS_MAP_INIT_VADDR_IOMEM(cirrus->vram); struct drm_atomic_helper_damage_iter iter; struct drm_rect damage; @@ -400,18 +352,17 @@ static void cirrus_primary_plane_helper_atomic_update(struct drm_plane *plane, if (!drm_dev_enter(&cirrus->dev, &idx)) return; - if (old_primary_plane_state->format != format) - cirrus_format_set(cirrus, format); - if (old_primary_plane_state->pitch != pitch) - cirrus_pitch_set(cirrus, pitch); + if (!old_fb || old_fb->format != fb->format) + cirrus_format_set(cirrus, fb->format); + if (!old_fb || old_fb->pitches[0] != fb->pitches[0]) + cirrus_pitch_set(cirrus, fb->pitches[0]); drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state); drm_atomic_for_each_plane_damage(&iter, &damage) { - unsigned int offset = drm_fb_clip_offset(pitch, format, &damage); + unsigned int offset = drm_fb_clip_offset(fb->pitches[0], fb->format, &damage); struct iosys_map dst = IOSYS_MAP_INIT_OFFSET(&vaddr, offset); - drm_fb_blit(&dst, &pitch, format->format, shadow_plane_state->data, fb, - &damage, &shadow_plane_state->fmtcnv_state); + drm_fb_memcpy(&dst, fb->pitches, shadow_plane_state->data, fb, &damage); } drm_dev_exit(idx); @@ -427,8 +378,6 @@ static struct drm_plane_state * cirrus_primary_plane_atomic_duplicate_state(struct drm_plane *plane) { struct drm_plane_state *plane_state = plane->state; - struct cirrus_primary_plane_state *primary_plane_state = - to_cirrus_primary_plane_state(plane_state); struct cirrus_primary_plane_state *new_primary_plane_state; struct drm_shadow_plane_state *new_shadow_plane_state; @@ -441,8 +390,6 @@ cirrus_primary_plane_atomic_duplicate_state(struct drm_plane *plane) new_shadow_plane_state = &new_primary_plane_state->base; __drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state); - new_primary_plane_state->format = primary_plane_state->format; - new_primary_plane_state->pitch = primary_plane_state->pitch; return &new_shadow_plane_state->base; }
Remove internal adjustments to framebuffer format from cirrus-qemu driver. The driver did this to support higher resolutions by reducing the per-pixel memory consumption. DRM has a policy of exporting formats as they are implemented in hardware. So avoid internal adjustments if possible. Also remove the call to drm_fb_blit() from cirrus-qemu. The helper is useful if source and destination format are not known beforehand. This is not the case for cirrus-qemu. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/tiny/cirrus-qemu.c | 71 ++++-------------------------- 1 file changed, 9 insertions(+), 62 deletions(-)