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().
};
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); };
Initially I used drm_fb_cma_helper.c with some added deferred code. This worked fine for fbcon, but the deferred mmap part didn't work well. For instance when using fbtest, I got short random horizontal lines on the display that didn't contain the latest pixels. I had to write several times to /dev/fb1 to trigger a display update to get all the previous pixels to go away and get the current image. Maybe it's some caching issue, I don't know. The Raspberry Pi doesn't support 16-bit SPI, so tinydrm does a byte swap to a new buffer before sending it using 8-bit. Maybe I need to call some kind of DMA sync function?
drm_fb_cma_helper is for creating drm_framebuffer backed by cma allocator objects. How you create drm_framebuffer is orthogonal to whether you have a ->dirty_fb hook (and hence needed defio support in fbdev) or not. E.g. maybe some SPI device has a dma engine, and hence you want to allocate drm_framebuffer using cma. On others with an i2c bus you want to just allocate kernel memory, since the cpu will copy the data anyway.
That's why I think we need to make sure this split is still maintained.
The dumb buffer uses drm_gem_cma_dumb_create() which is backed by cma, and that works just fine (I have only tested with David Herrmann's modeset[1]). A similar byte swapping happens here.
I also had to do this for the deferred io to work:
info->fix.smem_start = __pa(info->screen_base);
drm_fb_cma_helper assigns the dma address to smem_start, but at least on the Raspberry Pi this bus address can't be used by deferred_io (fb_deferred_io_fault()). And the ARM version of __pa states that it shouldn't be used by drivers, so when my vmalloc version worked, I went with that. But I see that there's a virt_to_phys() function that doesn't have that statement about not being used by drivers, so maybe this isn't a show stopper after all?
Any thoughts on this problem? I would rather have a cma backed fbdev framebuffer since that would give me the same type of memory both for fbdev and DRM.
Hm, tbh I have no clear idea who fbdev fb memory mapping workings. The above comments are more from a pov of a native kms userspace client. With fbdev as a clean helper sitting entirely on top of of kms interfaces, with no need to violate the layering for mmap support. There's some other thread going on (for the arc driver or whatever it was called) with the exact same problem. Might be good if you chat directly with Alexey Brodkin about this topic.
Thanks, that discussion gave me a solution. My problem goes away if I add this to fb_deferred_io_mmap():
vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
I have asked on the fbdev mailinglist about this and the physical address: Problems using fb_deferred_io with drm_fb_cma_helper http://marc.info/?l=linux-fbdev&m=145874714523971&w=2
Noralf.