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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 24, 2018 at 11:16:05AM +0200, Daniel Vetter wrote:
> On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote:
> > +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.

Looking at the cma conversion patch, this stuff here is actually required.
Or we'll break cma. I think your TODO comments need to be adjusted to
reflect that.

There's also the problem of how to handle the wc vs cached memory issue,
we might need more flags for 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