On Mon, Jul 13, 2020 at 8:39 PM Sam Ravnborg sam@ravnborg.org wrote:
On Mon, Jul 13, 2020 at 06:21:59PM +0200, Daniel Vetter wrote:
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.
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.
I did some more digging - taking notes see where this gets us.
fbdev have a fb_memcpy_tofb macro used in fb_write() that is architecture dependent: __aarch64__ memcpy_toio __alpha__ memcpy_toio __arm__ memcpy_toio __hppa__ memcpy_toio __i386__ memcpy_toio __powerpc__ memcpy_toio __sh__ memcpy_toio __sparc__ sbus_memcpy_toio __x86_64__ memcpy_toio
Others memcpy
If we then take a closer look at memcpy_toio it is defined like this: alpha __raw (optimized based on size / alignment) arm optimised memcpy - same as memcpy arm64 __raw (optimized based on size / alignment) hexagon memcpy ia64 writeb which is little endian so the same as memcpy m68k memcpy mips memcpy parisc __raw (optimized based on size/alignment) s390 s390 copy operations sh direct copies (looks like __raw copies) sparc writeb sparc64 sparc64 copies x86_64 x86_64 optimies copies (movs assembler)
Others memcpy
Aside from the sbus_memcpy_toio I don't think any of this matters. fbdev is simply older than memcpy_toio I think, on modern architectures you should always use memcpy_toio if it's an __mmio pointer, and normal memcpy for normal kernel pointers. Same holds for simple stores vs write* and friends.
As already analyzed sbus_memcpy_toio and memcpy_toio for sparc64 is the same. So from the above we can see that fbdev code always uses memcpy_toio or something equivalent when copying to framebuffer.
The conclusions so far:
- Copying to a framebuffer is correct to use memcpy_toio
- fbdev could have the macro fb_memcpy_tofb replaced by a direct call to memcpy_toio - I think the fb_memcpy_tofb macro predates the common memcpy_toio macro which explains why this was not used
This does not touch the whole _cfb_ vs _sys_ business.
It's all about the _cfb_ vs _sys_ business, since this is driver specific. Either you need the __mmio functions, or the normal system memory functions.
In video/fbdev/ this is driver specific. So we could add a fbdev_use_iomem as you suggested on irc som days ago. This is not strictly necessary for the sparc64 fix but let me try to come up with a patch anyway.
We need it, to avoid upsetting all the other drivers. I guess once we have this flag we could make special versions for fb-helpers which call the _cfb_ vs _sys_ functions correctly, as an added bonus. But for the sparc regression fix we need it already, so that we can call memcpy on drm_framebuffer residing in system memory, and memcpy_toio for those in vram or what looks like vram at least. -Daniel