Re: [PATCH 2/3] Define fb_open_adj_file()

[Daniel Vetter]

On Mon, May 23, 2016 at 12:48:52PM +0200, 'Max Staudt wrote:
> From: Max Staudt <mstaudt@xxxxxxx>
> 
> This callback from fb_open() allows a fbdev driver to adjust things such
> as file->f_mapping to better represent the internal structures.
> 
> This is needed to allow TTM drivers using ttm_fbdev_mmap() to properly
> set file->f_mapping to TTM's address_space from bo->bdev->dev_mapping,
> as is done form /dev/drm/card* in drm_open().
> 
> Currently, bochsdrmfb is the only driver requiring this, and this
> patch is a prerequisite to implement this in bochsdrmfb.
> 
> Signed-off-by: Max Staudt <mstaudt@xxxxxxx>

Do we _really_ care about fbdev mmap support so much that we want to add
more hacks all over the place (in each driver) to make it work? Given that
fbdev is officially in the "no more drivers" territory, I'm not sure we
want this either.

The trouble is that fbdev mmap and kms dumb buffer mmap have different
semantics and so every driver needs to implement both if they're special
in any kind of way.

If we can't say no to fbdev mmap then I think we should implement
something that works for all drivers. I'm thinking of allocating just a
pile of pages in the fbdev emulation, and then copying them over using the
defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
Or at least something along those lines. Of couse just opt-in, in case the
driver can do a more traditional mmio fbdev mmapping.
-Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c | 13 +++++++++++++
>  include/linux/fb.h               |  7 +++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 913c496..ff5000e 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1471,6 +1471,19 @@ __releases(&info->lock)
>  	if (info->fbdefio)
>  		fb_deferred_io_open(info, inode, file);
>  #endif
> +	if (info->fbops->fb_open_adj_file) {
> +		res = info->fbops->fb_open_adj_file(info, file);
> +		if (res) {
> +			/* If we got here, we also want to undo anything
> +			 * that fbops->fb_open() may have done.
> +			 */
> +			if (info->fbops->fb_release)
> +				info->fbops->fb_release(info,1);
> +
> +			module_put(info->fbops->owner);
> +			goto out;
> +		}
> +	}
>  out:
>  	mutex_unlock(&info->lock);
>  	if (res)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index dfe8835..4131496 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -320,6 +320,13 @@ struct fb_ops {
>  	/* called at KDB enter and leave time to prepare the console */
>  	int (*fb_debug_enter)(struct fb_info *info);
>  	int (*fb_debug_leave)(struct fb_info *info);
> +
> +	/* Called after fb_open() to adjust mappings when using
> +	 * complex backends such as TTM.
> +	 * For example, the bochsdrmfb driver needs to adjust
> +	 * file->f_mapping so it can use ttm_fbdev_mmap().
> +	 */
> +	int (*fb_open_adj_file)(struct fb_info *info, struct file *file);
>  };
>  
>  #ifdef CONFIG_FB_TILEBLITTING
> -- 
> 2.6.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch