Re: [RFC 1/5] drm: Add DRM support for tiny LCD displays

["Noralf Trønnes" <noralf@tronnes.org>]

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel