[Intel-gfx] [PATCH v5 4/8] drm/cma-helper: Use the generic fbdev emulation

[Daniel Vetter]

On Tue, Aug 21, 2018 at 8:43 PM, John Stultz <john.stultz at linaro.org> wrote:
> On Tue, Aug 21, 2018 at 7:59 AM, Noralf Trønnes <noralf at tronnes.org> wrote:
>> Den 21.08.2018 10.44, skrev Daniel Vetter:
>>> On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
>>>>
>>>> Since we don't have a drm_gem_cma_object reference in
>>>> drm_fb_helper_generic_probe(), I instead added:
>>>>     fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
>>>>
>>>> And that got things working!
>>>>
>>>> So yea, I wanted to figure out if we are just missing the line above
>>>> in drm_fb_helper_generic_probe()? Or if the kirin driver should be
>>>> setting the fix.smem_start in some other fashion?
>>>
>>> With the generic helpers we can't use the generic fb_mmap() implementation
>>> in fbdev/core/fbmem.c anymore. Instead Noralf added a generic fb_mmap
>>> fb_ops callback in drm_fbdev_fb_mmap, which you're supposed to use. Can
>>> you pls double-check that this is wired up correctly for kirin?
>>>
>>> At least I don't see any other place where we use smem_start in the fbdev
>>> code. It does get copied to userspace, but userspace should never use
>>> that.
>>
>>
>> I googled 'FBIOGET_FSCREENINFO smem_start' and came across info[1]
>> about the Mali blob using it and HiKey seems to have a Mali GPU:
>>
>> 00:17 <ssvb> BorgCuba:
>> the mali blob is supposed to open the framebuffer device,
>> get "smem_start" and try to map this memory via MALI_IOC_MEM_MAP_EXT ioctl
>>
>> Could this be the problem here?
>
> It does look like that's the case. Looking for smem_start usage, for
> Android its the gralloc code that fetches it:
>    https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#353
> and then puts it into the private_handle_t:
>    https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/framebuffer_device.cpp#384
>
> Which I assume then gets used later in the opaque mali blob (which
> then results in a similarly opaque hang).
>
> While I'm perfectly understanding that folks are not interested as its
> related to mali (which is fine, I can carry a patch - working to move
> away from fbdev emulation anyway), I am wondering if it might cause
> trouble for other users, as smem_start was previously set and now its
> not, so it seems likely to break userspace examples like:
>    https://www.linuxtv.org/downloads/v4l-dvb-apis-old/osd.html
>
> Or are those such old legacy they don't really count?

We've been pretty strict in drm about never ever exposing any physical
offsets to userspace. I'm mildly tempted to plug this even more if
possible, to stop more abuse. If you want mmap, fbdev has that. If you
want buffer sharing, use drm. Don't do buffer sharing by passing
physical addresses around in userspace, that idea is dead since about
8 years (back when we've done the initial dma-buf discussions).

So yeah, not going to care one bit here :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch