| Message ID | 20180703160354.59955-5-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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! 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
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@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! > > 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. -Daniel
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 >
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@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! >> >> 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? Noralf. [1] https://irclog.whitequark.org/linux-rockchip/2015-12-22
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@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! > > > > > > 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 :-) -Daniel
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@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! >>>> >>>> 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. Noralf.
On Tue, Aug 21, 2018 at 6:03 PM, Noralf Trønnes <noralf@tronnes.org> 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@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! >>>>> >>>>> 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
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/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? thanks -john
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/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
On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: > 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). Hey Noralf, Sorry, I know your probably sick of me. But I just wanted to circle around on this little bit. So part of the issue I found earlier, was that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support Surfaceflinger's request for page flipping. This is what makes the Y resolution 2160, which runs afoul of the new max_height check of 2048 in the generic code. I was checking with Xinliang, who know the kirin display hardware, about the max_height being set to 2048 to ensure bumping it up wasn't a problem, but he said 2048x2048 was unfortunately not arbitrary, and that was the hard limit of the display hardware. However, with overalloc, the 1920x2160 res fbdev should still be ok, as only 1920x1080 is actually displayed at one time. So it seems like we might need to multiply the max_height by the overalloc factor when we are checking it in drm_internal_framebuffer_create? Does that approach sound sane, or would folks prefer something different? thanks -john
On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@linaro.org> wrote: > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: >> 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). > > Hey Noralf, > Sorry, I know your probably sick of me. But I just wanted to circle > around on this little bit. So part of the issue I found earlier, was > that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > Surfaceflinger's request for page flipping. This is what makes the Y > resolution 2160, which runs afoul of the new max_height check of 2048 > in the generic code. > > I was checking with Xinliang, who know the kirin display hardware, > about the max_height being set to 2048 to ensure bumping it up wasn't > a problem, but he said 2048x2048 was unfortunately not arbitrary, and > that was the hard limit of the display hardware. However, with > overalloc, the 1920x2160 res fbdev should still be ok, as only > 1920x1080 is actually displayed at one time. > > So it seems like we might need to multiply the max_height by the > overalloc factor when we are checking it in > drm_internal_framebuffer_create? > > Does that approach sound sane, or would folks prefer something different? I guess we could simply not check against the height limit when allocating framebuffers. But we've done that for userspace buffers since forever (they just allocate 2 buffers for page-flipping), so I have no idea what would all break if we'd suddenly lift this restriction. And whether we'd lift it for fbdev only or for everyone doesn't really make much of a difference, since either this works, or it doesn't (across all chips). -Daniel
On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@linaro.org> wrote: >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: >>> 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). >> >> Hey Noralf, >> Sorry, I know your probably sick of me. But I just wanted to circle >> around on this little bit. So part of the issue I found earlier, was >> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >> Surfaceflinger's request for page flipping. This is what makes the Y >> resolution 2160, which runs afoul of the new max_height check of 2048 >> in the generic code. >> >> I was checking with Xinliang, who know the kirin display hardware, >> about the max_height being set to 2048 to ensure bumping it up wasn't >> a problem, but he said 2048x2048 was unfortunately not arbitrary, and >> that was the hard limit of the display hardware. However, with >> overalloc, the 1920x2160 res fbdev should still be ok, as only >> 1920x1080 is actually displayed at one time. >> >> So it seems like we might need to multiply the max_height by the >> overalloc factor when we are checking it in >> drm_internal_framebuffer_create? >> >> Does that approach sound sane, or would folks prefer something different? > > I guess we could simply not check against the height limit when > allocating framebuffers. But we've done that for userspace buffers > since forever (they just allocate 2 buffers for page-flipping), so I > have no idea what would all break if we'd suddenly lift this > restriction. And whether we'd lift it for fbdev only or for everyone > doesn't really make much of a difference, since either this works, or > it doesn't (across all chips). That feels a bit more risky then what I was thinking. What about something like (apologies, whitespace corrupted) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index fe7e545..0424a71 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1810,6 +1810,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int i; struct drm_fb_helper_surface_size sizes; int gamma_size = 0; + struct drm_mode_config *config; memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); sizes.surface_depth = 24; @@ -1910,6 +1911,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, sizes.surface_height *= drm_fbdev_overalloc; sizes.surface_height /= 100; + config = &fb_helper->client.dev->mode_config; + config->max_height *= drm_fbdev_overalloc; + config->max_height /= 100; + + /* push down into drivers */ ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); if (ret < 0) That way it only effects the fbdev + overalloc case? thanks -john
On Wed, Aug 22, 2018 at 11:21:11PM -0700, John Stultz wrote: > On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@linaro.org> wrote: > >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: > >>> 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). > >> > >> Hey Noralf, > >> Sorry, I know your probably sick of me. But I just wanted to circle > >> around on this little bit. So part of the issue I found earlier, was > >> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > >> Surfaceflinger's request for page flipping. This is what makes the Y > >> resolution 2160, which runs afoul of the new max_height check of 2048 > >> in the generic code. > >> > >> I was checking with Xinliang, who know the kirin display hardware, > >> about the max_height being set to 2048 to ensure bumping it up wasn't > >> a problem, but he said 2048x2048 was unfortunately not arbitrary, and > >> that was the hard limit of the display hardware. However, with > >> overalloc, the 1920x2160 res fbdev should still be ok, as only > >> 1920x1080 is actually displayed at one time. > >> > >> So it seems like we might need to multiply the max_height by the > >> overalloc factor when we are checking it in > >> drm_internal_framebuffer_create? > >> > >> Does that approach sound sane, or would folks prefer something different? > > > > I guess we could simply not check against the height limit when > > allocating framebuffers. But we've done that for userspace buffers > > since forever (they just allocate 2 buffers for page-flipping), so I > > have no idea what would all break if we'd suddenly lift this > > restriction. And whether we'd lift it for fbdev only or for everyone > > doesn't really make much of a difference, since either this works, or > > it doesn't (across all chips). > > That feels a bit more risky then what I was thinking. What about > something like (apologies, whitespace corrupted) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index fe7e545..0424a71 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1810,6 +1810,7 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > int i; > struct drm_fb_helper_surface_size sizes; > int gamma_size = 0; > + struct drm_mode_config *config; > > memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); > sizes.surface_depth = 24; > @@ -1910,6 +1911,11 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > sizes.surface_height *= drm_fbdev_overalloc; > sizes.surface_height /= 100; > > + config = &fb_helper->client.dev->mode_config; > + config->max_height *= drm_fbdev_overalloc; > + config->max_height /= 100; > + > + > /* push down into drivers */ > ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); > if (ret < 0) > > > That way it only effects the fbdev + overalloc case? Still changes it for everyone, not just fbdev, if you enable overalloc. You'd need to reset. Another, cleaner way to fix this would be to overallocate the buffer, but have the drm_framebuffer limited. But that means we need to change the fbdev scrolling logic. And the entire interface between fbdev helpers and drivers needs a rework, since atm the driver allocates the drm_framebuffer for you. That's what userspace can/will do in this case I guess. Has all the issues of scrolling by moving the drm_fb without hw knowledge. I guess maybe just dropping the max_height check in fbdev is ok, if we put a really big comment/FIXME there. Or maybe make it conditional on fbdev_overalloc being at the default value, that'd probably be better even. -Daniel
Hi John, On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote: > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz wrote: > > 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). > > Hey Noralf, > Sorry, I know your probably sick of me. But I just wanted to circle > around on this little bit. So part of the issue I found earlier, was > that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > Surfaceflinger's request for page flipping. Possibly slightly out of topic, but we're in 2018, is there any plan to make SurfaceFlinger move away from FBDEV ? > This is what makes the Y resolution 2160, which runs afoul of the new > max_height check of 2048 in the generic code. > > I was checking with Xinliang, who know the kirin display hardware, > about the max_height being set to 2048 to ensure bumping it up wasn't > a problem, but he said 2048x2048 was unfortunately not arbitrary, and > that was the hard limit of the display hardware. However, with > overalloc, the 1920x2160 res fbdev should still be ok, as only > 1920x1080 is actually displayed at one time. > > So it seems like we might need to multiply the max_height by the > overalloc factor when we are checking it in > drm_internal_framebuffer_create? > > Does that approach sound sane, or would folks prefer something different?
On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote: > Hi John, > > On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote: > > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz wrote: > > > 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). > > > > Hey Noralf, > > Sorry, I know your probably sick of me. But I just wanted to circle > > around on this little bit. So part of the issue I found earlier, was > > that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > > Surfaceflinger's request for page flipping. > > Possibly slightly out of topic, but we're in 2018, is there any plan to make > SurfaceFlinger move away from FBDEV ? Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or is this just an artifact of the mali blob hwcomposer backend? -Daniel > > > This is what makes the Y resolution 2160, which runs afoul of the new > > max_height check of 2048 in the generic code. > > > > I was checking with Xinliang, who know the kirin display hardware, > > about the max_height being set to 2048 to ensure bumping it up wasn't > > a problem, but he said 2048x2048 was unfortunately not arbitrary, and > > that was the hard limit of the display hardware. However, with > > overalloc, the 1920x2160 res fbdev should still be ok, as only > > 1920x1080 is actually displayed at one time. > > > > So it seems like we might need to multiply the max_height by the > > overalloc factor when we are checking it in > > drm_internal_framebuffer_create? > > > > Does that approach sound sane, or would folks prefer something different? > > -- > Regards, > > Laurent Pinchart > > >
Den 23.08.2018 09.37, skrev Daniel Vetter: > On Wed, Aug 22, 2018 at 11:21:11PM -0700, John Stultz wrote: >> On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@linaro.org> wrote: >>>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: >>>>> 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). >>>> Hey Noralf, >>>> Sorry, I know your probably sick of me. But I just wanted to circle >>>> around on this little bit. So part of the issue I found earlier, was >>>> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >>>> Surfaceflinger's request for page flipping. This is what makes the Y >>>> resolution 2160, which runs afoul of the new max_height check of 2048 >>>> in the generic code. >>>> >>>> I was checking with Xinliang, who know the kirin display hardware, >>>> about the max_height being set to 2048 to ensure bumping it up wasn't >>>> a problem, but he said 2048x2048 was unfortunately not arbitrary, and >>>> that was the hard limit of the display hardware. However, with >>>> overalloc, the 1920x2160 res fbdev should still be ok, as only >>>> 1920x1080 is actually displayed at one time. >>>> >>>> So it seems like we might need to multiply the max_height by the >>>> overalloc factor when we are checking it in >>>> drm_internal_framebuffer_create? >>>> >>>> Does that approach sound sane, or would folks prefer something different? >>> I guess we could simply not check against the height limit when >>> allocating framebuffers. But we've done that for userspace buffers >>> since forever (they just allocate 2 buffers for page-flipping), so I >>> have no idea what would all break if we'd suddenly lift this >>> restriction. And whether we'd lift it for fbdev only or for everyone >>> doesn't really make much of a difference, since either this works, or >>> it doesn't (across all chips). >> That feels a bit more risky then what I was thinking. What about >> something like (apologies, whitespace corrupted) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index fe7e545..0424a71 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1810,6 +1810,7 @@ static int drm_fb_helper_single_fb_probe(struct >> drm_fb_helper *fb_helper, >> int i; >> struct drm_fb_helper_surface_size sizes; >> int gamma_size = 0; >> + struct drm_mode_config *config; >> >> memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); >> sizes.surface_depth = 24; >> @@ -1910,6 +1911,11 @@ static int drm_fb_helper_single_fb_probe(struct >> drm_fb_helper *fb_helper, >> sizes.surface_height *= drm_fbdev_overalloc; >> sizes.surface_height /= 100; >> >> + config = &fb_helper->client.dev->mode_config; >> + config->max_height *= drm_fbdev_overalloc; >> + config->max_height /= 100; >> + >> + >> /* push down into drivers */ >> ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); >> if (ret < 0) >> >> >> That way it only effects the fbdev + overalloc case? > Still changes it for everyone, not just fbdev, if you enable overalloc. > You'd need to reset. > > Another, cleaner way to fix this would be to overallocate the buffer, but > have the drm_framebuffer limited. But that means we need to change the > fbdev scrolling logic. And the entire interface between fbdev helpers and > drivers needs a rework, since atm the driver allocates the drm_framebuffer > for you. That's what userspace can/will do in this case I guess. Has all > the issues of scrolling by moving the drm_fb without hw knowledge. > > I guess maybe just dropping the max_height check in fbdev is ok, if we put > a really big comment/FIXME there. Or maybe make it conditional on > fbdev_overalloc being at the default value, that'd probably be better > even. Maybe something like this could work: int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, struct drm_fb_helper_surface_size *sizes) { DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", sizes->surface_width, sizes->surface_height, sizes->surface_bpp); format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); if (drm_fbdev_overalloc > 100 && sizes->surface_height > fb_helper->client.dev->mode_config.max_height) { u32 divisor = drm_fbdev_overalloc / 100; buffer = drm_client_buffer_create(client, sizes->surface_width, sizes->surface_height, format); if (IS_ERR(buffer)) return PTR_ERR(buffer); ret = drm_client_buffer_addfb(buffer, sizes->surface_width, sizes->surface_height / divisor, format); if (ret) { drm_client_buffer_delete(buffer); return ret; } buffer->fb->height = sizes->surface_height; } else { buffer = drm_client_framebuffer_create(client, sizes->surface_width, sizes->surface_height, format); if (IS_ERR(buffer)) return PTR_ERR(buffer); } Noralf.
On Thu, Aug 23, 2018 at 6:49 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > > Den 23.08.2018 09.37, skrev Daniel Vetter: >> >> On Wed, Aug 22, 2018 at 11:21:11PM -0700, John Stultz wrote: >>> >>> On Wed, Aug 22, 2018 at 10:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> >>>> On Thu, Aug 23, 2018 at 6:14 AM, John Stultz <john.stultz@linaro.org> >>>> wrote: >>>>> >>>>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> >>>>> wrote: >>>>>> >>>>>> 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). >>>>> >>>>> Hey Noralf, >>>>> Sorry, I know your probably sick of me. But I just wanted to circle >>>>> around on this little bit. So part of the issue I found earlier, was >>>>> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >>>>> Surfaceflinger's request for page flipping. This is what makes the Y >>>>> resolution 2160, which runs afoul of the new max_height check of 2048 >>>>> in the generic code. >>>>> >>>>> I was checking with Xinliang, who know the kirin display hardware, >>>>> about the max_height being set to 2048 to ensure bumping it up wasn't >>>>> a problem, but he said 2048x2048 was unfortunately not arbitrary, and >>>>> that was the hard limit of the display hardware. However, with >>>>> overalloc, the 1920x2160 res fbdev should still be ok, as only >>>>> 1920x1080 is actually displayed at one time. >>>>> >>>>> So it seems like we might need to multiply the max_height by the >>>>> overalloc factor when we are checking it in >>>>> drm_internal_framebuffer_create? >>>>> >>>>> Does that approach sound sane, or would folks prefer something >>>>> different? >>>> >>>> I guess we could simply not check against the height limit when >>>> allocating framebuffers. But we've done that for userspace buffers >>>> since forever (they just allocate 2 buffers for page-flipping), so I >>>> have no idea what would all break if we'd suddenly lift this >>>> restriction. And whether we'd lift it for fbdev only or for everyone >>>> doesn't really make much of a difference, since either this works, or >>>> it doesn't (across all chips). >>> >>> That feels a bit more risky then what I was thinking. What about >>> something like (apologies, whitespace corrupted) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c >>> b/drivers/gpu/drm/drm_fb_helper.c >>> index fe7e545..0424a71 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -1810,6 +1810,7 @@ static int drm_fb_helper_single_fb_probe(struct >>> drm_fb_helper *fb_helper, >>> int i; >>> struct drm_fb_helper_surface_size sizes; >>> int gamma_size = 0; >>> + struct drm_mode_config *config; >>> >>> memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); >>> sizes.surface_depth = 24; >>> @@ -1910,6 +1911,11 @@ static int drm_fb_helper_single_fb_probe(struct >>> drm_fb_helper *fb_helper, >>> sizes.surface_height *= drm_fbdev_overalloc; >>> sizes.surface_height /= 100; >>> >>> + config = &fb_helper->client.dev->mode_config; >>> + config->max_height *= drm_fbdev_overalloc; >>> + config->max_height /= 100; >>> + >>> + >>> /* push down into drivers */ >>> ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); >>> if (ret < 0) >>> >>> >>> That way it only effects the fbdev + overalloc case? >> >> Still changes it for everyone, not just fbdev, if you enable overalloc. >> You'd need to reset. >> >> Another, cleaner way to fix this would be to overallocate the buffer, but >> have the drm_framebuffer limited. But that means we need to change the >> fbdev scrolling logic. And the entire interface between fbdev helpers and >> drivers needs a rework, since atm the driver allocates the drm_framebuffer >> for you. That's what userspace can/will do in this case I guess. Has all >> the issues of scrolling by moving the drm_fb without hw knowledge. >> >> I guess maybe just dropping the max_height check in fbdev is ok, if we put >> a really big comment/FIXME there. Or maybe make it conditional on >> fbdev_overalloc being at the default value, that'd probably be better >> even. > > > Maybe something like this could work: > > int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > struct drm_fb_helper_surface_size *sizes) > { > DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > sizes->surface_width, sizes->surface_height, > sizes->surface_bpp); > > format = drm_mode_legacy_fb_format(sizes->surface_bpp, > sizes->surface_depth); > > if (drm_fbdev_overalloc > 100 && > sizes->surface_height > > fb_helper->client.dev->mode_config.max_height) { > u32 divisor = drm_fbdev_overalloc / 100; > > buffer = drm_client_buffer_create(client, sizes->surface_width, > sizes->surface_height, format); > if (IS_ERR(buffer)) > return PTR_ERR(buffer); > > ret = drm_client_buffer_addfb(buffer, sizes->surface_width, > sizes->surface_height / divisor, format); Doesn't work, because the fbdev code checks against the framebuffer size when we "flip", i.e. when the viewport gets moved. And atm there's no support in the fbdev emulation code to re-create the drm_framebuffer for overallocated buffers. Hence why the proper solution is a pile more work than just this. -Daniel > if (ret) { > drm_client_buffer_delete(buffer); > return ret; > } > > buffer->fb->height = sizes->surface_height; > } else { > buffer = drm_client_framebuffer_create(client, sizes->surface_width, > sizes->surface_height, format); > if (IS_ERR(buffer)) > return PTR_ERR(buffer); > } > > > Noralf. >
On Wed, Aug 22, 2018 at 09:14:08PM -0700, John Stultz wrote: > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: > > 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). > > Hey Noralf, > Sorry, I know your probably sick of me. But I just wanted to circle > around on this little bit. So part of the issue I found earlier, was > that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > Surfaceflinger's request for page flipping. This is what makes the Y > resolution 2160, which runs afoul of the new max_height check of 2048 > in the generic code. > > I was checking with Xinliang, who know the kirin display hardware, > about the max_height being set to 2048 to ensure bumping it up wasn't > a problem, but he said 2048x2048 was unfortunately not arbitrary, and > that was the hard limit of the display hardware. However, with > overalloc, the 1920x2160 res fbdev should still be ok, as only > 1920x1080 is actually displayed at one time. I recently tried to clarify that max_width/height are simply the max framebuffer dimensions supported by the driver. So it's perfectly legal for a driver to declare max_height as something big that can't be scanned out in its entirety by a single plane. For i915 I'm currently working on bumping these limits to 32k-1 regardless of the hardware scanout limitations. So if you're already running with a framebuffer height >2048 and it works then it would seem to me that you could just bump this limit in the driver.
On Thu, Aug 23, 2018 at 12:46 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi John, > > On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote: >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz wrote: >> > 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). >> >> Hey Noralf, >> Sorry, I know your probably sick of me. But I just wanted to circle >> around on this little bit. So part of the issue I found earlier, was >> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >> Surfaceflinger's request for page flipping. > > Possibly slightly out of topic, but we're in 2018, is there any plan to make > SurfaceFlinger move away from FBDEV ? Yes. That is actually work-in-progress for both HiKey and HiKey960, switching to use the drm_hwcomposer once we can get upstream project into a sane state. thanks -john
On Thu, Aug 23, 2018 at 10:24 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Aug 22, 2018 at 09:14:08PM -0700, John Stultz wrote: >> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz <john.stultz@linaro.org> wrote: >> > 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). >> >> Hey Noralf, >> Sorry, I know your probably sick of me. But I just wanted to circle >> around on this little bit. So part of the issue I found earlier, was >> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >> Surfaceflinger's request for page flipping. This is what makes the Y >> resolution 2160, which runs afoul of the new max_height check of 2048 >> in the generic code. >> >> I was checking with Xinliang, who know the kirin display hardware, >> about the max_height being set to 2048 to ensure bumping it up wasn't >> a problem, but he said 2048x2048 was unfortunately not arbitrary, and >> that was the hard limit of the display hardware. However, with >> overalloc, the 1920x2160 res fbdev should still be ok, as only >> 1920x1080 is actually displayed at one time. > > I recently tried to clarify that max_width/height are simply the max > framebuffer dimensions supported by the driver. So it's perfectly legal > for a driver to declare max_height as something big that can't be > scanned out in its entirety by a single plane. For i915 I'm currently > working on bumping these limits to 32k-1 regardless of the hardware > scanout limitations. > > So if you're already running with a framebuffer height >2048 and it > works then it would seem to me that you could just bump this limit in > the driver. Ok. I'm fine with this as long as its not going to cause further trouble. thanks -john
On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote: >> Hi John, >> >> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote: >> > On Mon, Aug 20, 2018 at 11:44 PM, John Stultz wrote: >> > > 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). >> > >> > Hey Noralf, >> > Sorry, I know your probably sick of me. But I just wanted to circle >> > around on this little bit. So part of the issue I found earlier, was >> > that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support >> > Surfaceflinger's request for page flipping. >> >> Possibly slightly out of topic, but we're in 2018, is there any plan to make >> SurfaceFlinger move away from FBDEV ? > > Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or > is this just an artifact of the mali blob hwcomposer backend? Mostly its due to the simple fbdev being a legacy solution on android that works out of the box. I do suspect the android devs hope to retire it, which is why I'm working on getting things going w/ the drm_hwcomposer right now so we can get away from the fbdev. But in the meantime, keeping the fbdev method running is important so we have a functioning device for testing AOSP w/ mainline. thanks -john
Hi John, On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote: > On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote: > >> On Thursday, 23 August 2018 07:14:08 EEST John Stultz wrote: > >>> On Mon, Aug 20, 2018 at 11:44 PM, John Stultz wrote: > >>>> 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). > >>> > >>> Hey Noralf, > >>> > >>> Sorry, I know your probably sick of me. But I just wanted to circle > >>> > >>> around on this little bit. So part of the issue I found earlier, was > >>> that I'm running w/ CONFIG_DRM_FBDEV_OVERALLOC=200, to support > >>> Surfaceflinger's request for page flipping. > >> > >> Possibly slightly out of topic, but we're in 2018, is there any plan to > >> make SurfaceFlinger move away from FBDEV ? > > > > Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or > > is this just an artifact of the mali blob hwcomposer backend? > > Mostly its due to the simple fbdev being a legacy solution on android > that works out of the box. > I do suspect the android devs hope to retire it, which is why I'm > working on getting things going w/ the drm_hwcomposer right now so we > can get away from the fbdev. That would be good news. Are there many Android components other than vendor- specific hwcomposer implementations that still use fbdev ? > But in the meantime, keeping the fbdev method running is important so we > have a functioning device for testing AOSP w/ mainline.
On Thu, Aug 23, 2018 at 1:49 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote: >> On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote: >> >> Possibly slightly out of topic, but we're in 2018, is there any plan to >> >> make SurfaceFlinger move away from FBDEV ? >> > >> > Is surfaceflinger really using direct fbdev still (maybe for boot-up)? Or >> > is this just an artifact of the mali blob hwcomposer backend? >> >> Mostly its due to the simple fbdev being a legacy solution on android >> that works out of the box. >> I do suspect the android devs hope to retire it, which is why I'm >> working on getting things going w/ the drm_hwcomposer right now so we >> can get away from the fbdev. > > That would be good news. Are there many Android components other than vendor- > specific hwcomposer implementations that still use fbdev ? So yea, I can't really speak about what the various vendors are doing, as I don't really know, but I'm aware there are still a few (in some cases major) vendors who still use fbdev on their shipping devices with their custom hwcomposer code. Other then that, to my knowledge AOSP only has a default fallback hwcomposer that uses fbdev, which is what we've used here as we didn't want to take the vendor's proprietary hwcomposer blob. But again, moving to the drm_hwcomposer is the shiny bright future, as soon as a few remaining issues are sorted upstream. thanks -john
Hi John, On Friday, 24 August 2018 00:12:46 EEST John Stultz wrote: > On Thu, Aug 23, 2018 at 1:49 PM, Laurent Pinchart wrote: > > On Thursday, 23 August 2018 20:48:40 EEST John Stultz wrote: > >> On Thu, Aug 23, 2018 at 1:09 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>> On Thu, Aug 23, 2018 at 10:46:15AM +0300, Laurent Pinchart wrote: > >>>> Possibly slightly out of topic, but we're in 2018, is there any plan > >>>> to make SurfaceFlinger move away from FBDEV ? > >>> > >>> Is surfaceflinger really using direct fbdev still (maybe for boot-up)? > >>> Or is this just an artifact of the mali blob hwcomposer backend? > >> > >> Mostly its due to the simple fbdev being a legacy solution on android > >> that works out of the box. > >> I do suspect the android devs hope to retire it, which is why I'm > >> working on getting things going w/ the drm_hwcomposer right now so we > >> can get away from the fbdev. > > > > That would be good news. Are there many Android components other than > > vendor- specific hwcomposer implementations that still use fbdev ? > > So yea, I can't really speak about what the various vendors are doing, > as I don't really know, but I'm aware there are still a few (in some > cases major) vendors who still use fbdev on their shipping devices > with their custom hwcomposer code. > > Other then that, to my knowledge AOSP only has a default fallback > hwcomposer that uses fbdev, which is what we've used here as we didn't > want to take the vendor's proprietary hwcomposer blob. But again, > moving to the drm_hwcomposer is the shiny bright future, as soon as a > few remaining issues are sorted upstream. Last time I looked (and that was years ago) the init process also used fbdev to render the boot splash screen. Is it still the case ? If so is there any chance you could add a fix for that to your todo list ? :-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 186d00adfb5f..718c7f961d8a 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -18,6 +18,7 @@ */ #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_fb_helper.h> #include <drm/drm_framebuffer.h> #include <drm/drm_gem_cma_helper.h> @@ -26,11 +27,8 @@ #include <drm/drm_print.h> #include <linux/module.h> -#define DEFAULT_FBDEFIO_DELAY_MS 50 - struct drm_fbdev_cma { struct drm_fb_helper fb_helper; - const struct drm_framebuffer_funcs *fb_funcs; }; /** @@ -44,36 +42,6 @@ struct drm_fbdev_cma { * * An fbdev framebuffer backed by cma is also available by calling * drm_fb_cma_fbdev_init(). drm_fb_cma_fbdev_fini() tears it down. - * If the &drm_framebuffer_funcs.dirty callback is set, fb_deferred_io will be - * set up automatically. &drm_framebuffer_funcs.dirty is called by - * drm_fb_helper_deferred_io() in process context (&struct delayed_work). - * - * Example fbdev deferred io code:: - * - * static int driver_fb_dirty(struct drm_framebuffer *fb, - * struct drm_file *file_priv, - * unsigned flags, unsigned color, - * struct drm_clip_rect *clips, - * unsigned num_clips) - * { - * struct drm_gem_cma_object *cma = drm_fb_cma_get_gem_obj(fb, 0); - * ... push changes ... - * return 0; - * } - * - * static struct drm_framebuffer_funcs driver_fb_funcs = { - * .destroy = drm_gem_fb_destroy, - * .create_handle = drm_gem_fb_create_handle, - * .dirty = driver_fb_dirty, - * }; - * - * Initialize:: - * - * fbdev = drm_fb_cma_fbdev_init_with_funcs(dev, 16, - * dev->mode_config.num_crtc, - * dev->mode_config.num_connector, - * &driver_fb_funcs); - * */ static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper) @@ -131,153 +99,6 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, } EXPORT_SYMBOL_GPL(drm_fb_cma_get_gem_addr); -static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) -{ - return dma_mmap_writecombine(info->device, vma, info->screen_base, - info->fix.smem_start, info->fix.smem_len); -} - -static struct fb_ops drm_fbdev_cma_ops = { - .owner = THIS_MODULE, - DRM_FB_HELPER_DEFAULT_OPS, - .fb_fillrect = drm_fb_helper_sys_fillrect, - .fb_copyarea = drm_fb_helper_sys_copyarea, - .fb_imageblit = drm_fb_helper_sys_imageblit, - .fb_mmap = drm_fb_cma_mmap, -}; - -static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, - struct vm_area_struct *vma) -{ - fb_deferred_io_mmap(info, vma); - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); - - return 0; -} - -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); - fbi->screen_size = size; - fbi->fix.smem_len = size; - - if (fb->funcs->dirty) { - ret = drm_fbdev_cma_defio_init(fbi, obj); - if (ret) - goto err_cma_destroy; - } - - return 0; - -err_cma_destroy: - drm_framebuffer_remove(fb); -err_fb_info_destroy: - drm_fb_helper_fini(helper); -err_gem_free_object: - drm_gem_object_put_unlocked(&obj->base); - return ret; -} - -static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = { - .fb_probe = drm_fbdev_cma_create, -}; - /** * drm_fb_cma_fbdev_init_with_funcs() - Allocate and initialize fbdev emulation * @dev: DRM device @@ -295,53 +116,7 @@ int drm_fb_cma_fbdev_init_with_funcs(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count, const struct drm_framebuffer_funcs *funcs) { - struct drm_fbdev_cma *fbdev_cma; - struct drm_fb_helper *fb_helper; - int ret; - - if (!preferred_bpp) - preferred_bpp = dev->mode_config.preferred_depth; - if (!preferred_bpp) - preferred_bpp = 32; - - if (!max_conn_count) - max_conn_count = dev->mode_config.num_connector; - - fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL); - if (!fbdev_cma) - return -ENOMEM; - - fbdev_cma->fb_funcs = funcs; - fb_helper = &fbdev_cma->fb_helper; - - drm_fb_helper_prepare(dev, fb_helper, &drm_fb_cma_helper_funcs); - - ret = drm_fb_helper_init(dev, fb_helper, max_conn_count); - if (ret < 0) { - DRM_DEV_ERROR(dev->dev, "Failed to initialize fbdev helper.\n"); - goto err_free; - } - - ret = drm_fb_helper_single_add_all_connectors(fb_helper); - if (ret < 0) { - DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n"); - goto err_drm_fb_helper_fini; - } - - ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp); - if (ret < 0) { - DRM_DEV_ERROR(dev->dev, "Failed to set fbdev configuration.\n"); - goto err_drm_fb_helper_fini; - } - - return 0; - -err_drm_fb_helper_fini: - drm_fb_helper_fini(fb_helper); -err_free: - kfree(fbdev_cma); - - return ret; + return drm_fb_cma_fbdev_init(dev, preferred_bpp, max_conn_count); } EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs); @@ -359,8 +134,14 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init_with_funcs); int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count) { - return drm_fb_cma_fbdev_init_with_funcs(dev, preferred_bpp, - max_conn_count, NULL); + struct drm_fbdev_cma *fbdev_cma; + + /* dev->fb_helper will indirectly point to fbdev_cma after this call */ + fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count); + if (IS_ERR(fbdev_cma)) + return PTR_ERR(fbdev_cma); + + return 0; } EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init); @@ -370,87 +151,13 @@ EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_init); */ void drm_fb_cma_fbdev_fini(struct drm_device *dev) { - struct drm_fb_helper *fb_helper = dev->fb_helper; - - if (!fb_helper) - return; - - /* Unregister if it hasn't been done already */ - if (fb_helper->fbdev && fb_helper->fbdev->dev) - drm_fb_helper_unregister_fbi(fb_helper); - - if (fb_helper->fbdev) - drm_fbdev_cma_defio_fini(fb_helper->fbdev); - - if (fb_helper->fb) - drm_framebuffer_remove(fb_helper->fb); - - drm_fb_helper_fini(fb_helper); - kfree(to_fbdev_cma(fb_helper)); + if (dev->fb_helper) + drm_fbdev_cma_fini(to_fbdev_cma(dev->fb_helper)); } EXPORT_SYMBOL_GPL(drm_fb_cma_fbdev_fini); -/** - * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct - * @dev: DRM device - * @preferred_bpp: Preferred bits per pixel for the device - * @max_conn_count: Maximum number of connectors - * @funcs: fb helper functions, in particular a custom dirty() callback - * - * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR. - */ -struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, - unsigned int preferred_bpp, unsigned int max_conn_count, - const struct drm_framebuffer_funcs *funcs) -{ - struct drm_fbdev_cma *fbdev_cma; - struct drm_fb_helper *helper; - int ret; - - fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL); - if (!fbdev_cma) { - dev_err(dev->dev, "Failed to allocate drm fbdev.\n"); - return ERR_PTR(-ENOMEM); - } - fbdev_cma->fb_funcs = funcs; - - helper = &fbdev_cma->fb_helper; - - drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs); - - ret = drm_fb_helper_init(dev, helper, max_conn_count); - if (ret < 0) { - dev_err(dev->dev, "Failed to initialize drm fb helper.\n"); - goto err_free; - } - - ret = drm_fb_helper_single_add_all_connectors(helper); - if (ret < 0) { - dev_err(dev->dev, "Failed to add connectors.\n"); - goto err_drm_fb_helper_fini; - - } - - ret = drm_fb_helper_initial_config(helper, preferred_bpp); - if (ret < 0) { - dev_err(dev->dev, "Failed to set initial hw configuration.\n"); - goto err_drm_fb_helper_fini; - } - - return fbdev_cma; - -err_drm_fb_helper_fini: - drm_fb_helper_fini(helper); -err_free: - kfree(fbdev_cma); - - return ERR_PTR(ret); -} -EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs); - -static const struct drm_framebuffer_funcs drm_fb_cma_funcs = { - .destroy = drm_gem_fb_destroy, - .create_handle = drm_gem_fb_create_handle, +static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = { + .fb_probe = drm_fb_helper_generic_probe, }; /** @@ -464,9 +171,33 @@ static const struct drm_framebuffer_funcs drm_fb_cma_funcs = { struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count) { - return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, - max_conn_count, - &drm_fb_cma_funcs); + struct drm_fbdev_cma *fbdev_cma; + struct drm_fb_helper *fb_helper; + int ret; + + fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL); + if (!fbdev_cma) + return ERR_PTR(-ENOMEM); + + fb_helper = &fbdev_cma->fb_helper; + + ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL); + if (ret) + goto err_free; + + ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs, + preferred_bpp, max_conn_count); + if (ret) + goto err_client_put; + + return fbdev_cma; + +err_client_put: + drm_client_release(&fb_helper->client); +err_free: + kfree(fbdev_cma); + + return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); @@ -477,14 +208,7 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) { drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper); - if (fbdev_cma->fb_helper.fbdev) - drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev); - - if (fbdev_cma->fb_helper.fb) - drm_framebuffer_remove(fbdev_cma->fb_helper.fb); - - drm_fb_helper_fini(&fbdev_cma->fb_helper); - kfree(fbdev_cma); + /* All resources have now been freed by drm_fbdev_fb_destroy() */ } EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h index d532f88a8d55..a0546c3451f9 100644 --- a/include/drm/drm_fb_cma_helper.h +++ b/include/drm/drm_fb_cma_helper.h @@ -23,9 +23,6 @@ int drm_fb_cma_fbdev_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count); void drm_fb_cma_fbdev_fini(struct drm_device *dev); -struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, - unsigned int preferred_bpp, unsigned int max_conn_count, - const struct drm_framebuffer_funcs *funcs); struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int max_conn_count); void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);