[Intel-gfx] [PATCH 5/9] drm/fb-helper: Add generic fbdev emulation .fb_probe function
Daniel Vetter
daniel at ffwll.ch
Thu May 24 09:16:05 UTC 2018
On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:
> This is the first step in getting generic fbdev emulation.
> A drm_fb_helper_funcs.fb_probe function is added which uses the
> DRM client API to get a framebuffer backed by a dumb buffer.
>
> A transitional hack for tinydrm is needed in order to switch over all
> CMA helper drivers in a later patch. This hack will be removed when
> tinydrm moves to vmalloc buffers.
>
> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_fb_helper.h | 26 +++++++
> 2 files changed, 190 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 2ee1eaa66188..444c2b4040ea 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -30,11 +30,13 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/console.h>
> +#include <linux/dma-buf.h>
> #include <linux/kernel.h>
> #include <linux/sysrq.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> #include <drm/drmP.h>
> +#include <drm/drm_client.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_crtc_helper.h>
> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_fb_helper_output_poll_changed);
>
> +/* @user: 1=userspace, 0=fbcon */
> +static int drm_fbdev_fb_open(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + if (!try_module_get(fb_helper->dev->driver->fops->owner))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int drm_fbdev_fb_release(struct fb_info *info, int user)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + module_put(fb_helper->dev->driver->fops->owner);
> +
> + return 0;
> +}
Hm, I thought earlier versions of your patch series had this separately,
for everyone. What's the reasons for merging this into the fb_probe
implementation.
> +
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
> + * unregister_framebuffer() or fb_release().
> + */
> +static void drm_fbdev_fb_destroy(struct fb_info *info)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct fb_ops *fbops = NULL;
> +
> + DRM_DEBUG("\n");
> +
> + if (fb_helper->fbdev->fbdefio) {
> + fb_deferred_io_cleanup(fb_helper->fbdev);
> + fbops = fb_helper->fbdev->fbops;
> + }
> +
> + drm_fb_helper_fini(fb_helper);
> + drm_client_framebuffer_delete(fb_helper->buffer);
> + drm_client_free(fb_helper->client);
> + kfree(fb_helper);
> + kfree(fbops);
> +}
Hm, if we go with the idea that drm_clients could auto-unregister through
a callback, then I expect this will lead to some control inversion. But we
can fix that later on.
> +
> +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 fb_ops drm_fbdev_fb_ops = {
> + /* No need to set owner, this module is already pinned by the driver. */
I'd still set it, means less thinking since more obviously correct.
> + DRM_FB_HELPER_DEFAULT_OPS,
> + .fb_open = drm_fbdev_fb_open,
> + .fb_release = drm_fbdev_fb_release,
> + .fb_destroy = drm_fbdev_fb_destroy,
> + .fb_mmap = drm_fbdev_fb_mmap,
> + .fb_read = drm_fb_helper_sys_read,
> + .fb_write = drm_fb_helper_sys_write,
> + .fb_fillrect = drm_fb_helper_sys_fillrect,
> + .fb_copyarea = drm_fb_helper_sys_copyarea,
> + .fb_imageblit = drm_fb_helper_sys_imageblit,
Hm, some drivers require the cfb versions of these. In practice I guess
there's not much of a difference really, at least on x86 and arm.
We might want to document that though.
> +};
> +
> +static struct fb_deferred_io drm_fbdev_defio = {
> + .delay = HZ / 20,
> + .deferred_io = drm_fb_helper_deferred_io,
> +};
> +
> +/*
> + * TODO: Remove this when tinydrm is converted to vmalloc buffers.
> + */
> +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;
> +}
> +
Needs kerneldoc here.
> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + struct drm_client_dev *client = fb_helper->client;
> + struct drm_client_buffer *buffer;
> + struct drm_framebuffer *fb;
> + struct fb_info *fbi;
> + u32 format;
> + int ret;
> +
> + 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);
> + buffer = drm_client_framebuffer_create(client, sizes->surface_width,
> + sizes->surface_height, format);
> + if (IS_ERR(buffer))
> + return PTR_ERR(buffer);
> +
> + fb_helper->buffer = buffer;
> + fb_helper->fb = buffer->fb;
> + fb = buffer->fb;
> +
> + fbi = drm_fb_helper_alloc_fbi(fb_helper);
> + if (IS_ERR(fbi)) {
> + ret = PTR_ERR(fbi);
> + goto err_free_buffer;
> + }
> +
> + fbi->par = fb_helper;
> + fbi->fbops = &drm_fbdev_fb_ops;
> + fbi->screen_size = fb->height * fb->pitches[0];
> + fbi->fix.smem_len = fbi->screen_size;
> + fbi->screen_buffer = buffer->vaddr;
> + strcpy(fbi->fix.id, "DRM emulated");
> +
> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
> + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height);
> +
> + if (fb->funcs->dirty) {
> + struct fb_ops *fbops;
> +
> + /*
> + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per
> + * instance version is necessary.
> + */
> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> + if (!fbops) {
> + ret = -ENOMEM;
> + goto err_fb_info_destroy;
> + }
> +
> + *fbops = *fbi->fbops;
> + fbi->fbops = fbops;
> +
> + fbi->fbdefio = &drm_fbdev_defio;
> +
> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
> + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr));
> +
> + fb_deferred_io_init(fbi);
> +
> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */
> + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> + }
Ugh. Yeah defio and generic allocator through dumb buffers don't mix well.
The only true generic solution for this would be to give userspace (and
only userspace, for fbcon we can intercept everything) a staging buffer,
and then upload things using the dirty callback.
But that introduces another copy step, so isn't cool.
I think a check for is_vmalloc_addr and if that's false, not doing any of
the defio mmap setup would be good. Until we have a better idea. And yes
that would need to be done after tinydrm is moved over.
> +
> + return 0;
> +
> +err_fb_info_destroy:
> + drm_fb_helper_fini(fb_helper);
> +err_free_buffer:
> + drm_client_framebuffer_delete(buffer);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_generic_probe);
> +
> /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT)
> * but the module doesn't depend on any fb console symbols. At least
> * attempt to load fbcon to avoid leaving the system without a usable console.
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index b069433e7fc1..bb38469a9502 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -30,6 +30,8 @@
> #ifndef DRM_FB_HELPER_H
> #define DRM_FB_HELPER_H
>
> +struct drm_client_buffer;
> +struct drm_client_dev;
> struct drm_fb_helper;
>
> #include <drm/drm_crtc.h>
> @@ -232,6 +234,20 @@ struct drm_fb_helper {
> * See also: @deferred_setup
> */
> int preferred_bpp;
> +
> + /**
> + * @client:
> + *
> + * DRM client used by the generic fbdev emulation.
> + */
> + struct drm_client_dev *client;
> +
> + /**
> + * @buffer:
> + *
> + * Framebuffer used by the generic fbdev emulation.
> + */
> + struct drm_client_buffer *buffer;
> };
>
> /**
> @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev);
>
> void drm_fb_helper_lastclose(struct drm_device *dev);
> void drm_fb_helper_output_poll_changed(struct drm_device *dev);
> +
> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_surface_size *sizes);
> #else
> static inline void drm_fb_helper_prepare(struct drm_device *dev,
> struct drm_fb_helper *helper,
> @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
> {
> }
>
> +static inline int
> +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + return 0;
> +}
Ok, I think this patch looks ok. With the missing kerneldoc added (which
also explains the current limitations) this is
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> +
> #endif
>
> static inline int
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list