On Tue, Aug 21, 2018 at 6:03 PM, Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
>
> Den 21.08.2018 17.41, skrev Daniel Vetter:
>>
>> On Tue, Aug 21, 2018 at 04:59:46PM +0200, Noralf Trønnes wrote:
>>>
>>> Den 21.08.2018 10.44, skrev Daniel Vetter:
>>>>
>>>> On Mon, Aug 20, 2018 at 11:44:56PM -0700, John Stultz wrote:
>>>>>
>>>>> On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes <noralf@xxxxxxxxxxx>
>>>>> wrote:
>>>>>>
>>>>>> This switches the CMA helper drivers that use its fbdev emulation over
>>>>>> to the generic fbdev emulation. It's the first phase of using generic
>>>>>> fbdev. A later phase will use DRM client callbacks for the
>>>>>> lastclose/hotplug/remove callbacks.
>>>>>>
>>>>>> There are currently 2 fbdev init/fini functions:
>>>>>> - drm_fb_cma_fbdev_init/drm_fb_cma_fbdev_fini
>>>>>> - drm_fbdev_cma_init/drm_fbdev_cma_fini
>>>>>>
>>>>>> This is because the work on generic fbdev came up during a fbdev
>>>>>> refactoring and thus wasn't completed. No point in completing that
>>>>>> refactoring when drivers will soon move to
>>>>>> drm_fb_helper_generic_probe().
>>>>>>
>>>>>> tinydrm uses drm_fb_cma_fbdev_init_with_funcs().
>>>>>>
>>>>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
>>>>>> Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_fb_cma_helper.c | 360
>>>>>> +++++-------------------------------
>>>>>> include/drm/drm_fb_cma_helper.h | 3 -
>>>>>> 2 files changed, 42 insertions(+), 321 deletions(-)
>>>>>
>>>>> ...
>>>>>>
>>>>>> -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
>>>>>> - struct drm_gem_cma_object
>>>>>> *cma_obj)
>>>>>> -{
>>>>>> - struct fb_deferred_io *fbdefio;
>>>>>> - struct fb_ops *fbops;
>>>>>> -
>>>>>> - /*
>>>>>> - * Per device structures are needed because:
>>>>>> - * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
>>>>>> - * fbdefio: individual delays
>>>>>> - */
>>>>>> - fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>>>>>> - fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>>>>>> - if (!fbdefio || !fbops) {
>>>>>> - kfree(fbdefio);
>>>>>> - kfree(fbops);
>>>>>> - return -ENOMEM;
>>>>>> - }
>>>>>> -
>>>>>> - /* can't be offset from vaddr since dirty() uses cma_obj */
>>>>>> - fbi->screen_buffer = cma_obj->vaddr;
>>>>>> - /* fb_deferred_io_fault() needs a physical address */
>>>>>> - fbi->fix.smem_start =
>>>>>> page_to_phys(virt_to_page(fbi->screen_buffer));
>>>>>> -
>>>>>> - *fbops = *fbi->fbops;
>>>>>> - fbi->fbops = fbops;
>>>>>> -
>>>>>> - fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
>>>>>> - fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>>>>> - fbi->fbdefio = fbdefio;
>>>>>> - fb_deferred_io_init(fbi);
>>>>>> - fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
>>>>>> -
>>>>>> - return 0;
>>>>>> -}
>>>>>> -
>>>>>> -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
>>>>>> -{
>>>>>> - if (!fbi->fbdefio)
>>>>>> - return;
>>>>>> -
>>>>>> - fb_deferred_io_cleanup(fbi);
>>>>>> - kfree(fbi->fbdefio);
>>>>>> - kfree(fbi->fbops);
>>>>>> -}
>>>>>> -
>>>>>> -static int
>>>>>> -drm_fbdev_cma_create(struct drm_fb_helper *helper,
>>>>>> - struct drm_fb_helper_surface_size *sizes)
>>>>>> -{
>>>>>> - struct drm_fbdev_cma *fbdev_cma = to_fbdev_cma(helper);
>>>>>> - struct drm_device *dev = helper->dev;
>>>>>> - struct drm_gem_cma_object *obj;
>>>>>> - struct drm_framebuffer *fb;
>>>>>> - unsigned int bytes_per_pixel;
>>>>>> - unsigned long offset;
>>>>>> - struct fb_info *fbi;
>>>>>> - size_t size;
>>>>>> - int ret;
>>>>>> -
>>>>>> - DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n",
>>>>>> - sizes->surface_width, sizes->surface_height,
>>>>>> - sizes->surface_bpp);
>>>>>> -
>>>>>> - bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
>>>>>> - size = sizes->surface_width * sizes->surface_height *
>>>>>> bytes_per_pixel;
>>>>>> - obj = drm_gem_cma_create(dev, size);
>>>>>> - if (IS_ERR(obj))
>>>>>> - return -ENOMEM;
>>>>>> -
>>>>>> - fbi = drm_fb_helper_alloc_fbi(helper);
>>>>>> - if (IS_ERR(fbi)) {
>>>>>> - ret = PTR_ERR(fbi);
>>>>>> - goto err_gem_free_object;
>>>>>> - }
>>>>>> -
>>>>>> - fb = drm_gem_fbdev_fb_create(dev, sizes, 0, &obj->base,
>>>>>> - fbdev_cma->fb_funcs);
>>>>>> - if (IS_ERR(fb)) {
>>>>>> - dev_err(dev->dev, "Failed to allocate DRM
>>>>>> framebuffer.\n");
>>>>>> - ret = PTR_ERR(fb);
>>>>>> - goto err_fb_info_destroy;
>>>>>> - }
>>>>>> -
>>>>>> - helper->fb = fb;
>>>>>> -
>>>>>> - fbi->par = helper;
>>>>>> - fbi->flags = FBINFO_FLAG_DEFAULT;
>>>>>> - fbi->fbops = &drm_fbdev_cma_ops;
>>>>>> -
>>>>>> - drm_fb_helper_fill_fix(fbi, fb->pitches[0],
>>>>>> fb->format->depth);
>>>>>> - drm_fb_helper_fill_var(fbi, helper, sizes->fb_width,
>>>>>> sizes->fb_height);
>>>>>> -
>>>>>> - offset = fbi->var.xoffset * bytes_per_pixel;
>>>>>> - offset += fbi->var.yoffset * fb->pitches[0];
>>>>>> -
>>>>>> - dev->mode_config.fb_base = (resource_size_t)obj->paddr;
>>>>>> - fbi->screen_base = obj->vaddr + offset;
>>>>>> - fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
>>>>>
>>>>> Hey Noralf, all,
>>>>> I've been digging for a bit on the regression that this patch has
>>>>> tripped on the HiKey board as reported here:
>>>>> https://lkml.org/lkml/2018/8/16/81
>>>>>
>>>>> The first issue was that the kirin driver was setting
>>>>> mode_config.max_width/height = 2048, which was causing errors as the
>>>>> the requested resolution was 1920x2160 (due to surfaceflinger
>>>>> requesting y*2 for page flipping). This was relatively simple enough
>>>>> to figure out and fix, but bumping the values up on its own didn't
>>>>> seem to resolve the issue entirely, as I started to see hard hangs on
>>>>> bootup when userspace started using the emulated fbdev device.
>>>>>
>>>>> After confirming reverting the patch here worked around it, I started
>>>>> digging through what might be different, and I think I've found it.
>>>>> In the drm_fbdev_cma_create() function above, we set the
>>>>> fbi->fix.smem_start value, where as in drm_fb_helper_generic_probe(),
>>>>> which now replaces that, we don't set smem_start value at all.
>>>>>
>>>>> 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?
>>
>> Yup, most certainly is. As you analyzed in your other reply, smem_start
>> shouldn't matter at all for fbdev mmap support.
>>
>> And the only other place it's used is the ioctl.
>>
>> I'm pretty sure John owes us one, because the Oops he's seeing is in the
>> mali kernel driver for the mali userspace blob :-)
>
>
> Ah, so it makes sense after all.
>
> The change John did:
>
> fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
>
> is the conversions legal on all platforms? It doesn't have to be a legal
> value, but it can't be oops'ing or fail to build for some other user.
>
> If it's ok we can just add that line and be done with it.
Nope, very much not ok for anything using discontinuous buffers. For
that you need to point it at some remapping unit to make fbdev happy,
and only the driver knows about that.
Also, dri-devel officially doesn't care about blob userspace and
non-upstream kernel drivers :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
