[Intel-gfx] [PATCH 5/9] drm/fb-helper: Add generic fbdev emulation .fb_probe function
Noralf Trønnes
noralf at tronnes.org
Fri May 25 12:42:02 UTC 2018
Den 24.05.2018 11.16, skrev Daniel Vetter:
> 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.
This is only necessary when struct fb_ops is defined in a library where
the owner field is the library module and not the driver module.
I realised that with this generic version it's highly unlikely that we
get another library that defines struct fb_ops, so it's only needed here.
Noralf.
>> +
>> +/*
>> + * 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
More information about the Intel-gfx
mailing list