Den 21.08.2018 08.44, skrev John Stultz:
On Tue, Jul 3, 2018 at 9:03 AM, Noralf Trønnes noralf@tronnes.org 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@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
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