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@xxxxxxxxxxx>
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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
