[PATCH] drm/drm_fb_helper: fix fbdev with sparc64

Tue Jul 14 08:56:20 UTC 2020

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 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.
> 
> 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 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
>>
> 
> 
> 
> 

-- 
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/f181a61d/attachment.sig>