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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Den 21.08.2018 08.44, skrev John Stultz:
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!

I don't understand how setting smem_start can fix anything.
Is that line the only thing you have added/changed?
AFAICT smem_start is only used as a fall back in fb_mmap() if
fb_ops->fb_mmap is not set (and for defio).

Have I understood it correctly that it is fbdev mmap that is broken?
fbcon is working correctly? If so, have you checked that returning
-ENODEV unconditionally in drm_fbdev_fb_mmap() to block it's usage
prevents the kernel from hanging?


This is fbdev mmap before the change:

static int
drm_fbdev_cma_create(struct drm_fb_helper *helper,
    struct drm_fb_helper_surface_size *sizes)
{
    bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
    size = sizes->surface_width * sizes->surface_height * bytes_per_pixel;

    [ offset is always zero because of drm_fb_helper_fill_var() ]

    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);
    fbi->screen_size = size;
    fbi->fix.smem_len = size;

static struct fb_ops drm_fbdev_cma_ops = {
    .fb_mmap    = drm_fb_cma_mmap,

static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
    [ info->device is drm_device->dev ]
    return dma_mmap_writecombine(info->device, vma, info->screen_base,
                     info->fix.smem_start, info->fix.smem_len);
}

#define dma_mmap_writecombine dma_mmap_wc


This is the current:

int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
                struct drm_fb_helper_surface_size *sizes)
{
    format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
    buffer = drm_client_framebuffer_create(client, sizes->surface_width,
                           sizes->surface_height, format);

    fb = buffer->fb;

    fbi->screen_size = fb->height * fb->pitches[0];
    fbi->fix.smem_len = fbi->screen_size;
    fbi->screen_buffer = buffer->vaddr;


static struct fb_ops drm_fbdev_fb_ops = {
    .fb_mmap    = drm_fbdev_fb_mmap,

static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
    struct drm_fb_helper *fb_helper = info->par;

    if (fb_helper->dev->driver->gem_prime_mmap)
        return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
    else
        return -ENODEV;
}

static struct drm_driver kirin_drm_driver = {
    .gem_prime_mmap        = drm_gem_cma_prime_mmap,

int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
               struct vm_area_struct *vma)
{
    struct drm_gem_cma_object *cma_obj;
    int ret;

    ret = drm_gem_mmap_obj(obj, obj->size, vma);
    if (ret < 0)
        return ret;

    cma_obj = to_drm_gem_cma_obj(obj);
    return drm_gem_cma_mmap_obj(cma_obj, vma);
}

static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
                struct vm_area_struct *vma)
{
    int ret;

    /*
     * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
     * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
     * the whole buffer.
     */
    vma->vm_flags &= ~VM_PFNMAP;
    vma->vm_pgoff = 0;

    ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
              cma_obj->paddr, vma->vm_end - vma->vm_start);
    if (ret)
        drm_gem_vm_close(vma);

    return ret;
}


I see one difference and that is: vma->vm_pgoff = 0;
I don't know if that can affect the outcome.


Noralf.

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?

thanks
-john


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux