Den 29.03.2016 09:27, skrev Daniel Vetter:
On Fri, Mar 25, 2016 at 11:41:44AM +0100, Noralf Trønnes wrote:
Den 23.03.2016 18:28, skrev Daniel Vetter:
On Wed, Mar 23, 2016 at 06:07:56PM +0100, Noralf Trønnes wrote:
Den 18.03.2016 18:47, skrev Daniel Vetter:
On Thu, Mar 17, 2016 at 10:51:55PM +0100, Noralf Trønnes wrote:
Den 16.03.2016 16:11, skrev Daniel Vetter: > On Wed, Mar 16, 2016 at 02:34:15PM +0100, Noralf Trønnes wrote: >> tinydrm provides a very simplified view of DRM for displays that has >> onboard video memory and is connected through a slow bus like SPI/I2C. >> >> Signed-off-by: Noralf Trønnes noralf@tronnes.org > Yay, it finally happens! I already made a comment on the cover letter > about the fbdev stuff, I think that's the biggest part to split out from > tinydrm here. I'm not entirely sure a detailed code review makes sense > before that part is done (and hey we can start merging already), so just a > high level review for now:
[...]
> In the case of tinydrm I think that means we should have a bunch of new > drm helpers, or extensions for existing ones: > - fbdev deferred io support using ->dirtyfb in drm_fb_helper.c. Are you thinking something like this?
struct drm_fb_helper_funcs { int (*dirtyfb)(struct drm_fb_helper *fb_helper, struct drm_clip_rect *clip);
We already have a dirty_fb function in dev->mode_config->funcs->dirty_fb(). This is the official interface native drm/kms userspace is supposed to use to flush frontbuffer rendering. The xfree86-video-modesetting driver uses it.
I couldn't find this dirty_fb() function, but I assume you mean drm_framebuffer_funcs.dirty().
Yup.
};
struct drm_fb_helper { spinlock_t dirty_lock; struct drm_clip_rect *dirty_clip; };
Yeah, this part is needed for the delayed work for the fbdev helper. struct work dirty_fb_work; is missing.
This confuses me. If we have this then there's no need for a fb->funcs->dirty() call, the driver can just add a work function here instead.
Possible fb dirty() call chain: Calls to drm_fb_helper_sys_* or mmap page writes will schedule fb_info->deferred_work. The worker func fb_deferred_io_work() calls fb_info->fbdefio->deferred_io(). Then deferred_io() can call fb_helper->fb->funcs->dirty().
In my use-case this dirty() function would schedule a delayed_work to run immediately since it has already been deferred collecting changes. The regular drm side framebuffer dirty() collects damage and schedules the same worker to run deferred.
I don't see an easy way for a driver to set the dirty() function in drm_fb_cma_helper apart from doing this:
struct drm_fbdev_cma { struct drm_fb_helper fb_helper; struct drm_fb_cma *fb;
int (*dirty)(struct drm_framebuffer *framebuffer,struct drm_file *file_priv, unsigned flags,unsigned color, struct drm_clip_rect *clips,unsigned num_clips);Well my point is that drm core already has a canonical interface (drm_framebuffer_funcs.dirty) to flush out rendering. And it's supposed to be called from process context, and userspace is supposed to batch up dirty updates.
[...]
What I'd like is that the fbdev emulation uses exactly that interface, without requiring drivers to write any additional fbdev code (like qxl and udl currently have). Since the drm_framebuffer_funcs.dirty is already expected to run in process context I think the only bit we need is the deferred_work you already added in fbdev, so that we can schedule the driver's ->dirty() function.
There shouldn't be any need to have another ->dirty() function anywhere else.
I'll try and see if I can explain better with some code. First the drm_fb_helper part:
void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagelist) { struct drm_fb_helper *helper = info->par; unsigned long start, end, min, max; struct drm_clip_rect clip; struct page *page;
if (!helper->fb->funcs->dirty) return; spin_lock(&helper->dirty_lock); clip = *helper->dirty_clip; /* reset dirty_clip */ spin_unlock(&helper->dirty_lock); min = ULONG_MAX; max = 0; list_for_each_entry(page, pagelist, lru) { start = page->index << PAGE_SHIFT; end = start + PAGE_SIZE - 1; min = min(min, start); max = max(max, end); } if (min < max) { /* TODO merge with clip */ } helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1);} EXPORT_SYMBOL(drm_fb_helper_deferred_io);
/* Called by the drm_fb_helper_sys_*() functions */ static void drm_fb_helper_sys_deferred(struct fb_info *info, u32 x, u32 y, u32 width, u32 height) { struct drm_fb_helper *helper = info->par; struct drm_clip_rect clip;
if (!info->fbdefio) return; clip.x1 = x; clip.x2 = x + width -1; cliy.y1 = y; clip.y2 = y + height - 1; spin_lock(&helper->dirty_lock); /* TODO merge clip with helper->dirty_clip */ spin_unlock(&helper->dirty_lock); schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);}
So the question I have asked is this: How can the driver set the dirty() function within drm_fb_cma_helper?
Having looked at the code over and over again, I have a suggestion, but it assumes that it's allowed to change fb->funcs.
First the necessary drm_fb_cma_helper changes:
EXPORT_SYMBOL(drm_fb_cma_destroy); EXPORT_SYMBOL(drm_fb_cma_create_handle); EXPORT_SYMBOL(drm_fbdev_cma_create);
/* This is the drm_fbdev_cma_init() code with one change */ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, unsigned int preferred_bpp, unsigned int num_crtc, unsigned int max_conn_count, struct drm_framebuffer_funcs *funcs) { [...] drm_fb_helper_prepare(dev, helper, funcs); [...] }
struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev, unsigned int preferred_bpp, unsigned int num_crtc, unsigned int max_conn_count) { return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp, num_crtc, max_conn_count, &drm_fb_cma_helper_funcs); }
Now tinydrm should be able to do this:
static int tinydrm_fbdev_dirty(struct drm_framebuffer *fb, struct drm_file *file_priv, unsigned flags, unsigned color, struct drm_clip_rect *clips, unsigned num_clips) { struct drm_fb_helper *helper = info->par; struct tinydrm_device *tdev = helper->dev->dev_private; struct drm_framebuffer *fb = helper->fb; struct drm_gem_cma_object *cma_obj;
if (tdev->plane.fb != fb) return 0; cma_obj = drm_fb_cma_get_gem_obj(fb, 0); if (!cma_obj) { dev_err_once(info->dev, "Can't get cma_obj\n"); return -EINVAL; } return tdev->dirtyfb(fb, cma_obj->vaddr, flags, color, clips,num_clips); }
static struct drm_framebuffer_funcs tinydrm_fb_cma_funcs = { .destroy = drm_fb_cma_destroy, .create_handle = drm_fb_cma_create_handle, .dirty = tinydrm_fbdev_dirty, };
Ah, cma midlayer strikes again. We've had a similar discussion around the gem side when vc4 landed for similar reasons iirc - it's hard to overwrite specific helper functions to customize the behaviour.
I guess the simplest solution would be to export drm_fb_cma_destroy/create_handle functions, and then add a drm_fb_cma_create_with_funcs helper which you can use like this:
static int tinydrm_fbdev_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) { return drm_fb_cma_create_with_funcs(helper, sizes, tinydrm_fb_cma_funcs); }
Changing fb->funcs after drm_framebuffer_init is called is a no-go, since that function also registers the framebuffer and makes it userspace accessible. So after that point other threads can go&look at fb->funcs.
static int tinydrm_fbdev_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) { struct tinydrm_device *tdev = helper->dev->dev_private; struct fb_deferred_io *fbdefio; struct fb_ops *fbops; struct fb_info *info;
/* * A per device structure is needed for: * - fbops because fb_deferred_io_cleanup() clears fbops.fb_mmap * - fbdefio to get per device delay */ fbops = devm_kzalloc(helper->dev->dev, sizeof(*fbops), GFP_KERNEL); fbdefio = devm_kzalloc(helper->dev->dev, sizeof(*fbdefio),GFP_KERNEL); if (!fbops || !fbdefio) return -ENOMEM;
ret = drm_fbdev_cma_create(helper, sizes); if (ret) return ret; helper->fb->funcs = &tinydrm_fb_cma_funcs;Ok, first thing I've forgotten is that the fbdev cma helper doesn't call mode_config->funcs->fb_create and so gets the wrong fb->funcs. One thing I've pondered for different reasons lately is whether the fbdev emulation should grow a fake drm_file fpriv. With that we could just allocate a dumb buffer and pass it to ->fb_create (it takes a handle, which means we need a fake fpriv). I think that would fully demidlayer cma helpers. Or we create a new helper function to do ->fb_create but with gem objects instead of ids like what we've done for vc4. Or we just open-code it all.
Not sure which is better here.
info = fb_helper->fbdev; /* * fb_deferred_io requires a vmalloc address or a physical address. * drm_fbdev_cma_create() sets smem_start to the dma address whichis * a device address. This isn't always a physical address. */ info->smem_start = page_to_phys(virt_to_page(info->screen_buffer));
*fbops = *info->fbops; info->fbops = fbops; info->fbdefio = fbdefio; fbdefio->deferred_io = drm_fb_helper_deferred_io; fbdefio->delay = msecs_to_jiffies(tdev->deferred->defer_ms); /* delay=0 is turned into delay=HZ, so use 1 as a minimum */ if (!fbdefio->delay) fbdefio->delay = 1; fb_deferred_io_init(info); return 0;}
static const struct drm_fb_helper_funcs tinydrm_fb_helper_funcs = { .fb_probe = tinydrm_fbdev_create, };
int tinydrm_fbdev_init(struct tinydrm_device *tdev) { struct drm_device *dev = tdev->base; struct drm_fbdev_cma *cma;
cma = drm_fbdev_cma_init_with_funcs(dev, 16,dev->mode_config.num_crtc, dev->mode_config.num_connector, &tinydrm_fb_helper_funcs); if (IS_ERR(cma)) return PTR_ERR(cma);
I was wondering whether we couldn't avoid the _with_funcs here by setting up fbdefio iff ->dirty is set. Then you could add the generic defio setup code to drm_fbdev_cma_create after helper->fb is allocated, but only if helper->fb->funcs->dirty is set. Makes for a bit less boilerplate.
Or did I miss something?
I don't see how I can avoid drm_fbdev_cma_init_with_funcs():
static struct drm_framebuffer_funcs tinydrm_fb_cma_funcs = { .destroy = drm_fb_cma_destroy, .create_handle = drm_fb_cma_create_handle, .dirty = tinydrm_fbdev_dirty, };
static int tinydrm_fbdev_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) { return drm_fbdev_cma_create_with_funcs(helper, sizes, &tinydrm_fb_cma_funcs); }
static const struct drm_fb_helper_funcs tinydrm_fb_helper_funcs = { .fb_probe = tinydrm_fbdev_create, };
int tinydrm_fbdev_init(struct tinydrm_device *tdev) { struct drm_device *dev = tdev->base; struct drm_fbdev_cma *cma;
cma = drm_fbdev_cma_init_with_funcs(dev, 16, dev->mode_config.num_crtc, dev->mode_config.num_connector, &tinydrm_fb_helper_funcs); if (IS_ERR(cma)) return PTR_ERR(cma);
tdev->fbdev_cma = cma;
return 0; }
Thanks for your feedback so far Daniel, I quite like the direction this is taking. I'll try and implement it in a new version of the patchset.
Noralf.