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

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx