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

[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 at 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch