drm/drm_fb_helper: fix fbdev with sparc64

Commit Message

Sam Ravnborg July 9, 2020, 7:30 p.m. UTC

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 <[e-mail]>
Reported-by: Mark Cave-Ayland <[e-mail]>
Tested-by: Mark Cave-Ayland <[e-mail]>
Cc: Mark Cave-Ayland <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Gerd Hoffmann <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: [e-mail]
---
 drivers/gpu/drm/drm_fb_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

kernel test robot July 10, 2020, 1:36 a.m. UTC | #1

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-fbdev-with-sparc64/20200710-033231
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 <[e-mail]>


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/[e-mail]

Thomas Zimmermann July 10, 2020, 6:28 a.m. UTC | #2

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 <[e-mail]>
> Reported-by: Mark Cave-Ayland <[e-mail]>
> Tested-by: Mark Cave-Ayland <[e-mail]>
> Cc: Mark Cave-Ayland <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Gerd Hoffmann <[e-mail]>
> Cc: "David S. Miller" <[e-mail]>
> Cc: [e-mail]

So this actually is a problem in practice. Do you know how userspace
handles this?

For this patch

Acked-by: Thomas Zimmermann <[e-mail]>

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);
>  		src += fb->pitches[0];
>  		dst += fb->pitches[0];
>  	}
>

Mark Cave-Ayland July 10, 2020, 7:10 p.m. UTC | #3

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 <[e-mail]>
>> Reported-by: Mark Cave-Ayland <[e-mail]>
>> Tested-by: Mark Cave-Ayland <[e-mail]>
>> Cc: Mark Cave-Ayland <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: Gerd Hoffmann <[e-mail]>
>> Cc: "David S. Miller" <[e-mail]>
>> Cc: [e-mail]
> 
> So this actually is a problem in practice. Do you know how userspace
> handles this?
> 
> For this patch
> 
> Acked-by: Thomas Zimmermann <[e-mail]>
> 
> 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.

Dave Airlie July 10, 2020, 9:11 p.m. UTC | #4

> > Acked-by: Thomas Zimmermann <[e-mail]>
> >
> > 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.

Daniel Vetter July 13, 2020, 4:21 p.m. UTC | #5

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 <[e-mail]>
> > Reported-by: Mark Cave-Ayland <[e-mail]>
> > Tested-by: Mark Cave-Ayland <[e-mail]>
> > Cc: Mark Cave-Ayland <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: Gerd Hoffmann <[e-mail]>
> > Cc: "David S. Miller" <[e-mail]>
> > Cc: [e-mail]
> 
> So this actually is a problem in practice. Do you know how userspace
> handles this?
> 
> For this patch
> 
> Acked-by: Thomas Zimmermann <[e-mail]>
> 
> 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Sam Ravnborg July 13, 2020, 6:39 p.m. UTC | #6

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 <[e-mail]>
> > > Reported-by: Mark Cave-Ayland <[e-mail]>
> > > Tested-by: Mark Cave-Ayland <[e-mail]>
> > > Cc: Mark Cave-Ayland <[e-mail]>
> > > Cc: Thomas Zimmermann <[e-mail]>
> > > Cc: Gerd Hoffmann <[e-mail]>
> > > Cc: "David S. Miller" <[e-mail]>
> > > Cc: [e-mail]
> > 
> > So this actually is a problem in practice. Do you know how userspace
> > handles this?
> > 
> > For this patch
> > 
> > Acked-by: Thomas Zimmermann <[e-mail]>
> > 
> > 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

Daniel Vetter July 13, 2020, 7:32 p.m. UTC | #7

On Mon, Jul 13, 2020 at 8:39 PM Sam Ravnborg <[e-mail]> 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 <[e-mail]>
> > > > Reported-by: Mark Cave-Ayland <[e-mail]>
> > > > Tested-by: Mark Cave-Ayland <[e-mail]>
> > > > Cc: Mark Cave-Ayland <[e-mail]>
> > > > Cc: Thomas Zimmermann <[e-mail]>
> > > > Cc: Gerd Hoffmann <[e-mail]>
> > > > Cc: "David S. Miller" <[e-mail]>
> > > > Cc: [e-mail]
> > >
> > > So this actually is a problem in practice. Do you know how userspace
> > > handles this?
> > >
> > > For this patch
> > >
> > > Acked-by: Thomas Zimmermann <[e-mail]>
> > >
> > > 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

Thomas Zimmermann July 14, 2020, 6:41 a.m. UTC | #8

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 <[e-mail]>
>>> Reported-by: Mark Cave-Ayland <[e-mail]>
>>> Tested-by: Mark Cave-Ayland <[e-mail]>
>>> Cc: Mark Cave-Ayland <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: Gerd Hoffmann <[e-mail]>
>>> Cc: "David S. Miller" <[e-mail]>
>>> Cc: [e-mail]
>>
>> So this actually is a problem in practice. Do you know how userspace
>> handles this?
>>
>> For this patch
>>
>> Acked-by: Thomas Zimmermann <[e-mail]>
>>
>> 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/[e-mail]/

> 
> 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
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
>

Daniel Vetter July 14, 2020, 8:41 a.m. UTC | #9

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 <[e-mail]>
> >>> Reported-by: Mark Cave-Ayland <[e-mail]>
> >>> Tested-by: Mark Cave-Ayland <[e-mail]>
> >>> Cc: Mark Cave-Ayland <[e-mail]>
> >>> Cc: Thomas Zimmermann <[e-mail]>
> >>> Cc: Gerd Hoffmann <[e-mail]>
> >>> Cc: "David S. Miller" <[e-mail]>
> >>> Cc: [e-mail]
> >>
> >> So this actually is a problem in practice. Do you know how userspace
> >> handles this?
> >>
> >> For this patch
> >>
> >> Acked-by: Thomas Zimmermann <[e-mail]>
> >>
> >> 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/[e-mail]/
> 
> > 
> > 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
> >> [e-mail]
> >> 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
>

Thomas Zimmermann July 14, 2020, 8:56 a.m. UTC | #10

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 <[e-mail]>
>>>>> Reported-by: Mark Cave-Ayland <[e-mail]>
>>>>> Tested-by: Mark Cave-Ayland <[e-mail]>
>>>>> Cc: Mark Cave-Ayland <[e-mail]>
>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>> Cc: Gerd Hoffmann <[e-mail]>
>>>>> Cc: "David S. Miller" <[e-mail]>
>>>>> Cc: [e-mail]
>>>>
>>>> So this actually is a problem in practice. Do you know how userspace
>>>> handles this?
>>>>
>>>> For this patch
>>>>
>>>> Acked-by: Thomas Zimmermann <[e-mail]>
>>>>
>>>> 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/[e-mail]/
>>
>>>
>>> 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
>>>> [e-mail]
>>>> 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
>>
> 
> 
> 
>

Dave Airlie July 24, 2020, 4:53 a.m. UTC | #11

On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann <[e-mail]> 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 <[e-mail]>
> >>>>> Reported-by: Mark Cave-Ayland <[e-mail]>
> >>>>> Tested-by: Mark Cave-Ayland <[e-mail]>
> >>>>> Cc: Mark Cave-Ayland <[e-mail]>
> >>>>> Cc: Thomas Zimmermann <[e-mail]>
> >>>>> Cc: Gerd Hoffmann <[e-mail]>
> >>>>> Cc: "David S. Miller" <[e-mail]>
> >>>>> Cc: [e-mail]
> >>>>
> >>>> So this actually is a problem in practice. Do you know how userspace
> >>>> handles this?
> >>>>
> >>>> For this patch
> >>>>
> >>>> Acked-by: Thomas Zimmermann <[e-mail]>
> >>>>
> >>>> 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.

Sam Ravnborg July 24, 2020, 6:23 a.m. UTC | #12

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 <[e-mail]> 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 <[e-mail]>
> > >>>>> Reported-by: Mark Cave-Ayland <[e-mail]>
> > >>>>> Tested-by: Mark Cave-Ayland <[e-mail]>
> > >>>>> Cc: Mark Cave-Ayland <[e-mail]>
> > >>>>> Cc: Thomas Zimmermann <[e-mail]>
> > >>>>> Cc: Gerd Hoffmann <[e-mail]>
> > >>>>> Cc: "David S. Miller" <[e-mail]>
> > >>>>> Cc: [e-mail]
> > >>>>
> > >>>> So this actually is a problem in practice. Do you know how userspace
> > >>>> handles this?
> > >>>>
> > >>>> For this patch
> > >>>>
> > >>>> Acked-by: Thomas Zimmermann <[e-mail]>
> > >>>>
> > >>>> 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

Thomas Zimmermann July 24, 2020, 6:56 a.m. UTC | #13

Hi

Am 24.07.20 um 06:53 schrieb Dave Airlie:
> On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann <[e-mail]> 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 <[e-mail]>
>>>>>>> Reported-by: Mark Cave-Ayland <[e-mail]>
>>>>>>> Tested-by: Mark Cave-Ayland <[e-mail]>
>>>>>>> Cc: Mark Cave-Ayland <[e-mail]>
>>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>>> Cc: Gerd Hoffmann <[e-mail]>
>>>>>>> Cc: "David S. Miller" <[e-mail]>
>>>>>>> Cc: [e-mail]
>>>>>>
>>>>>> So this actually is a problem in practice. Do you know how userspace
>>>>>> handles this?
>>>>>>
>>>>>> For this patch
>>>>>>
>>>>>> Acked-by: Thomas Zimmermann <[e-mail]>
>>>>>>
>>>>>> 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Daniel Vetter July 24, 2020, 8:09 a.m. UTC | #14

On Fri, Jul 24, 2020 at 8:24 AM Sam Ravnborg <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > >>>>> Reported-by: Mark Cave-Ayland <[e-mail]>
> > > >>>>> Tested-by: Mark Cave-Ayland <[e-mail]>
> > > >>>>> Cc: Mark Cave-Ayland <[e-mail]>
> > > >>>>> Cc: Thomas Zimmermann <[e-mail]>
> > > >>>>> Cc: Gerd Hoffmann <[e-mail]>
> > > >>>>> Cc: "David S. Miller" <[e-mail]>
> > > >>>>> Cc: [e-mail]
> > > >>>>
> > > >>>> So this actually is a problem in practice. Do you know how userspace
> > > >>>> handles this?
> > > >>>>
> > > >>>> For this patch
> > > >>>>
> > > >>>> Acked-by: Thomas Zimmermann <[e-mail]>
> > > >>>>
> > > >>>> 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

Patch

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];
 	}