On Tue, Aug 21, 2018 at 8:43 PM, John Stultz john.stultz@linaro.org wrote:
On Tue, Aug 21, 2018 at 7:59 AM, Noralf Trønnes noralf@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/frameb... and then puts it into the private_handle_t: https://android.googlesource.com/device/linaro/hikey/+/master/gralloc/frameb...
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