Re: [PATCH 5/9] drm/fb-helper: Add generic fbdev emulation .fb_probe function

["Noralf Trønnes" <noralf@tronnes.org>]

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@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@ffwll.ch>
>
>> +
>>   #endif
>>   
>>   static inline int
>> -- 
>> 2.15.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx