[PATCH] drm/drm_fb_helper: fix fbdev with sparc64

Fri Jul 24 06:56:20 UTC 2020

Hi

Am 24.07.20 um 06:53 schrieb Dave Airlie:
> On Tue, 14 Jul 2020 at 18:56, Thomas Zimmermann <tzimmermann at 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 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?
>>
> 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 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/20200724/dcc1d0a7/attachment.sig>