On Tue, Jul 14, 2020 at 08:41:58AM +0200, Thomas Zimmermann wrote:
Hi
Am 13.07.20 um 18:21 schrieb Daniel Vetter:
On Fri, Jul 10, 2020 at 08:28:16AM +0200, Thomas Zimmermann wrote:
Hi
Am 09.07.20 um 21:30 schrieb Sam Ravnborg:
Mark reported that sparc64 would panic while booting using qemu. Mark bisected this to a patch that introduced generic fbdev emulation to the bochs DRM driver. Mark pointed out that a similar bug was fixed before where the sys helpers was replaced by cfb helpers.
The culprint here is that the framebuffer reside in IO memory which requires SPARC ASI_PHYS (physical) loads and stores.
The current bohcs DRM driver uses a shadow buffer. So all copying to the framebuffer happens in drm_fb_helper_dirty_blit_real().
The fix is to replace the memcpy with memcpy_toio() from io.h.
memcpy_toio() uses writeb() where the original fbdev code used sbus_memcpy_toio(). The latter uses sbus_writeb().
The difference between writeb() and sbus_memcpy_toio() is that writeb() writes bytes in little-endian, where sbus_writeb() writes bytes in big-endian. As endian does not matter for byte writes they are the same. So we can safely use memcpy_toio() here.
For many architectures memcpy_toio() is a simple memcpy(). One sideeffect that is unknow is if this has any impact on other architectures. So far the analysis tells that this change is OK for other arch's. but testing would be good.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reported-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk Tested-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk Cc: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Gerd Hoffmann kraxel@redhat.com Cc: "David S. Miller" davem@davemloft.net Cc: sparclinux@vger.kernel.org
So this actually is a problem in practice. Do you know how userspace handles this?
For this patch
Acked-by: Thomas Zimmermann tzimmermann@suse.de
but I'd like to have someone with more architecture expertise ack this as well.
Best regards Thomas
drivers/gpu/drm/drm_fb_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 5609e164805f..4d05b0ab1592 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -399,7 +399,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, unsigned int y;
for (y = clip->y1; y < clip->y2; y++) {
memcpy(dst, src, len);
memcpy_toio(dst, src, len);I don't think we can do this unconditionally, there's fbdev-helper drivers using shmem helpers, and for shmem memcpy_toio is wrong. We need a switch to fix this properly I think.
I once has a patch set for this problem, but it didn't make it. [1]
Buffers can move between I/O and system memory, so a simple flag would not work. I'd propose this
bool drm_gem_is_iomem(struct drm_gem_object *obj) { if (obj->funcs && obj->funcs->is_iomem) return obj->funcs->is_iomem(obj); return false; }
Most GEM implmentations wouldn't bother, but VRAM helpers could set the is_iomem function and return the current state. Fbdev helpers can then pick the correct memcpy_*() function.
Hm wasn't the (long term at least) idea to add the is_iomem flag to the vmap functions? is_iomem is kinda only well-defined if there's a vmap of the buffer around (which also pins it), or in general when the buffer is pinned. Outside of that an ->is_iomem function doesn't make much sense. -Daniel
Best regards Thomas
[1] https://lore.kernel.org/dri-devel/20191106093121.21762-1-tzimmermann@suse.de...
What Dave Airlie mentioned is just about memcpy_toio vs the sparc bus version, for which we don't have any drivers really. But I do think we need to differentiate between memcpy and memcpy_tio. That's what this entire annoying _cfb_ vs _sys_ business is all about, and also what gem vram helpers have to deal with. -Daniel
src += fb->pitches[0]; dst += fb->pitches[0];}
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer