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 --- 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); src += fb->pitches[0]; dst += fb->pitches[0]; }
Hi Sam,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.8-rc4 next-20200709] [cannot apply to drm/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sam-Ravnborg/drm-drm_fb_helper-fix-... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-s021-20200709 (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.2-37-gc9676a3b-dirty # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
sparse warnings: (new ones prefixed by >>)
drivers/gpu/drm/drm_fb_helper.c:386:29: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void volatile [noderef] __iomem * @@ got void *dst @@ drivers/gpu/drm/drm_fb_helper.c:386:29: sparse: expected void volatile [noderef] __iomem * drivers/gpu/drm/drm_fb_helper.c:386:29: sparse: got void *dst
vim +386 drivers/gpu/drm/drm_fb_helper.c
373 374 static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, 375 struct drm_clip_rect *clip) 376 { 377 struct drm_framebuffer *fb = fb_helper->fb; 378 unsigned int cpp = fb->format->cpp[0]; 379 size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; 380 void *src = fb_helper->fbdev->screen_buffer + offset; 381 void *dst = fb_helper->buffer->vaddr + offset; 382 size_t len = (clip->x2 - clip->x1) * cpp; 383 unsigned int y; 384 385 for (y = clip->y1; y < clip->y2; y++) {
386 memcpy_toio(dst, src, len);
387 src += fb->pitches[0]; 388 dst += fb->pitches[0]; 389 } 390 } 391
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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);
On 10/07/2020 07:28, Thomas Zimmermann wrote:
Hi Sam,
Thanks again for the patch. I've spotted some small typos that you may like to fix if you repost the patch:
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
Typo here: culprit
requires SPARC ASI_PHYS (physical) loads and stores.
The current bohcs DRM driver uses a shadow buffer.
And another here: bochs
So all copying to the framebuffer happens in drm_fb_helper_dirty_blit_real().
How about this as an alternative to the above paragraphs which might be a bit easier to read:
Recent kernels have been reported to panic using the bochs_drm framebuffer under qemu-system-sparc64 which was bisected to commit 7a0483ac4ffc "drm/bochs: switch to generic drm fbdev emulation". The backtrace indicates that the shadow framebuffer copy in drm_fb_helper_dirty_blit_real() is trying to access the real framebuffer using a virtual address rather than use an IO access typically implemented using a physical (ASI_PHYS) access on SPARC.
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
side-effect
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.
Agreed. All my testing is using the bochs_drm framebuffer under qemu-system-sparc64 (a sun4u machine) so it would be nice to get an ACK from Dave or someone else who can vouch for this on real hardware.
ATB,
Mark.
Acked-by: Thomas Zimmermann tzimmermann@suse.de
but I'd like to have someone with more architecture expertise ack this as well.
Agreed. All my testing is using the bochs_drm framebuffer under qemu-system-sparc64 (a sun4u machine) so it would be nice to get an ACK from Dave or someone else who can vouch for this on real hardware.
I'm not sure there exists real hardware that would come down this path, I believe the last sparc64 with a GPU is a mach64, or maybe an r128 card.
Otherwise I think using the memcpy io should be fine here on all architectures.
Dave.
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. -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
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
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.
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.
Sam
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
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.
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
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
Hi
Am 14.07.20 um 10:41 schrieb Daniel Vetter:
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.
Oh. From how I understood the original discussion, you shoot down the idea because sparse would not support it well?
The other idea was to add an additional vmap_iomem() helper that returns an__iomem pointer. Can we try that?
Best regards Thomas
-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
On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 14.07.20 um 10:41 schrieb Daniel Vetter:
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.
Oh. From how I understood the original discussion, you shoot down the idea because sparse would not support it well?
The other idea was to add an additional vmap_iomem() helper that returns an__iomem pointer. Can we try that?
Did we get anywhere with this yet?
Dave.
Hi Dave. On Fri, Jul 24, 2020 at 02:53:30PM +1000, Dave Airlie wrote:
On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 14.07.20 um 10:41 schrieb Daniel Vetter:
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.
Oh. From how I understood the original discussion, you shoot down the idea because sparse would not support it well?
The other idea was to add an additional vmap_iomem() helper that returns an__iomem pointer. Can we try that?
Did we get anywhere with this yet?
A few on the work I did so far. Using qemu the original reported bug was fixed only be replacing a memcpy with memcpy_toio. But this looks like only a half solution as we would still use the sys_* variants to copy data to the framebuffer, and tye do not cope with frambuffer in dedicated IO memory.
But I have not managed to get it work wiht qemu when using the cfb_* variants. I end up in a deadlock waiting for the console lock. So far my debuggin have not told me why I lock up the boot waiting for the console lock and I am stuck on that.
I could send the patch memcpy => memcpy_toio but I am afraid it may not work on real HW as we do not cover the sys_* => cfb_*
Sam
On Fri, Jul 24, 2020 at 8:24 AM Sam Ravnborg sam@ravnborg.org wrote:
Hi Dave. On Fri, Jul 24, 2020 at 02:53:30PM +1000, Dave Airlie wrote:
On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 14.07.20 um 10:41 schrieb Daniel Vetter:
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.
Oh. From how I understood the original discussion, you shoot down the idea because sparse would not support it well?
The other idea was to add an additional vmap_iomem() helper that returns an__iomem pointer. Can we try that?
Did we get anywhere with this yet?
A few on the work I did so far. Using qemu the original reported bug was fixed only be replacing a memcpy with memcpy_toio. But this looks like only a half solution as we would still use the sys_* variants to copy data to the framebuffer, and tye do not cope with frambuffer in dedicated IO memory.
But I have not managed to get it work wiht qemu when using the cfb_* variants. I end up in a deadlock waiting for the console lock. So far my debuggin have not told me why I lock up the boot waiting for the console lock and I am stuck on that.
I could send the patch memcpy => memcpy_toio but I am afraid it may not work on real HW as we do not cover the sys_* => cfb_*
Let's focus on the single memcpy_toio for the regression fix only for now, that seems to be enough to get the system back to booting. Note that we don't want the cfb helpers for the case where fbdev emulation has a staging buffer in system memory, i.e. when the memcpy_toio matters. So fixing that really should be all that's needed.
Fixing sys vs cfb is more a case of making it possible of other drivers to also use the generic fbdev support, so that we can support the case of framebuffer in io space without the dirtyfb staging upload buffer. So minimal regression fix, more feature work (like the sys vs cfb stuff) goes into -next. -Daniel
Hi
Am 24.07.20 um 06:53 schrieb Dave Airlie:
On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 14.07.20 um 10:41 schrieb Daniel Vetter:
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.
Oh. From how I understood the original discussion, you shoot down the idea because sparse would not support it well?
The other idea was to add an additional vmap_iomem() helper that returns an__iomem pointer. Can we try that?
Did we get anywhere with this yet?
Not yet. But I intend to work on it ASAP.
Best regards Thomas
Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie -
kernel test robot -
Mark Cave-Ayland -
Sam Ravnborg -
Thomas Zimmermann