[PATCH] drm/drm_fb_helper: fix fbdev with sparc64

[Thomas Zimmermann]

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 at ravnborg.org>
>>> Reported-by: Mark Cave-Ayland <mark.cave-ayland at ilande.co.uk>
>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland at ilande.co.uk>
>>> Cc: Mark Cave-Ayland <mark.cave-ayland at ilande.co.uk>
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>> Cc: "David S. Miller" <davem at davemloft.net>
>>> Cc: sparclinux at 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 at 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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200714/18e5f6ff/attachment.sig>