[RFC 1/5] drm: Add DRM support for tiny LCD displays
Daniel Vetter
daniel at ffwll.ch
Fri Mar 18 17:47:33 UTC 2016
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:
> >
> >The big story in kms/drm in the past years is that we've rejecting
> >anything that remotely looks like a midlayer. Instead the preferred design
> >pattern is a library of helper functions to implement useful default
> >behaviour, or sometimes just building blocks for useful default behaviour.
> >And then build up real drivers using these pieces. The benefit of that is
> >two-fold:
> >- easier to share code with other drivers that only need part of the
> > behaviour (e.g. fbdev deferred io support).
> >- easier to adapt to special hw that needs exceptions since worst case you
> > can just copypaste an entire hook. Or implement the special case and
> > call the default helper for the normal cases.
> >
> >lwn has a good article on this pattern:
> >
> >https://lwn.net/Articles/336262/
>
> I was afraid you would say "midlayer" :-)
>
> How about creating macros like SIMPLE_DEV_PM_OPS and friends to simplify
> the drm_driver boilerplate and use that in the drivers?
>
> #define SET_DRM_DRIVER_GEM_CMA_OPS \
> .gem_free_object = drm_gem_cma_free_object, \
> .gem_vm_ops = &drm_gem_cma_vm_ops, \
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \
> .gem_prime_import = drm_gem_prime_import, \
> .gem_prime_export = drm_gem_prime_export, \
> .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, \
> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, \
> .gem_prime_vmap = drm_gem_cma_prime_vmap, \
> .gem_prime_vunmap = drm_gem_cma_prime_vunmap, \
> .gem_prime_mmap = drm_gem_cma_prime_mmap, \
> .dumb_create = drm_gem_cma_dumb_create, \
> .dumb_map_offset = drm_gem_cma_dumb_map_offset, \
> .dumb_destroy = drm_gem_dumb_destroy,
>
> #define TINYDRM_DRM_DRIVER(name_struct, name_str, desc_str, date_str) \
> static struct drm_driver name_struct = { \
> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME \
> | DRIVER_ATOMIC, \
> .load = tinydrm_load, \
> .unload = tinydrm_unload, \
> .lastclose = tinydrm_lastclose, \
> SET_DRM_DRIVER_GEM_CMA_OPS \
> .fops = &tinydrm_fops, \
> .name = name_str, \
> .desc = desc_str, \
> .date = date_str, \
> .major = 1, \
> .minor = 0, \
> }
Looks like a pretty sweet idea. Maybe even split it up into GEM_PRIME_OPS
and similar sub-groups, which you could roll out into lots of drivers.
>
> Now the driver can do this:
> TINYDRM_DRM_DRIVER(adafruit_tft, "adafruit-tft", Adafruit TFT", "20160317");
>
> In addition to that, the tinydrm specific parts that make up
> tinydrm_load/unload can be exported if someone wants it slightly different.
Just from your sketch here this sounds nifty.
> >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.
> };
>
> 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.
> Should I extend drm_fb_helper_sys_* or make it explicit with
> drm_fb_helper_sys_*_deferred functions?
Imo extend the existing helpers, adding deferred io is part of the reasons
we added them. Just add a check for mode_config->funcs->dirty_fb to
short-circuit all the deferred io flushing if the driver doesn't need it.
Same for setting up deferred io fbdev driver flags - just check whether
->dirty_fb is there, and only if that is set fill in inof->fbdefio.
> #ifdef CONFIG_FB_DEFERRED_IO
> /* Will just return if info->fbdefio is not set */
> void drm_fb_helper_sys_deferred(struct fb_info *info, u32 x, u32 y,
> u32 width, u32 height);
> #else
> static inline void drm_fb_helper_sys_deferred(struct fb_info *info, u32 x,
> u32 y,
> u32 width, u32 height)
> { }
> #endif
>
> void drm_fb_helper_sys_imageblit(struct fb_info *info,
> const struct fb_image *image)
> {
> sys_imageblit(info, image);
> drm_fb_helper_sys_deferred(info, image->dx, image->dy, image->width,
> image->height);
> }
>
> OR
>
> void drm_fb_helper_sys_imageblit_deferred(struct fb_info *info,
> const struct fb_image *image)
> {
> drm_fb_helper_sys_imageblit(info, image);
> drm_fb_helper_sys_deferred(info, image->dx, image->dy, image->width,
> image->height);
> }
>
>
> 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.
It would be really awesome if we could make this Just Work.
> I can however live with a vmalloc buffer, because the SPI subsystem uses
> the DMA streaming API and supports vmalloc buffers (spi_map_buf()).
> The vast majority of these displays are connected through SPI.
> Also the dma address at the head of the buffer isn't much use to me since
> almost all of these display controllers supports partial updates, so I
> can't use it much anyway (partial updates isn't implemented in the current
> code yet).
>
> [1] https://github.com/dvdhrm/docs/blob/master/drm-howto/modeset.c
Ah, I thought you've used vmalloc because your buses need cpu access
anyway and can't dma. I think we really need to look into making the fbdev
helpers work better - does fbdev allow us to implement a special mmap
handler? We could then write one specific to cma buffers to handle this, I
think that's also Alexey's plan now.
> >- Helper to generate a simple display pipeline with 1 plane, 1 crtc, 1
> > encoder pointing at a specific drm_connector. There's lots of other
> > simple hw that could use this. Maybe create a new
> > drm_simple_kms_helper.c for this.
> >- A helper to create a simple drm_connector from a drm_panel (the
> > get_modes hooks you have here), maybe also in drm_simple_kms_helper.c.
>
> How about this:
>
> struct drm_connector *drm_simple_kms_create_panel_connector(struct
> drm_device *dev,
> struct drm_panel *panel);
>
> int drm_simple_kms_create_pipeline(struct drm_device *dev,
> const struct drm_plane_helper_funcs *plane_helper_funcs,
> const uint32_t *plane_formats, unsigned int format_count,
> const struct drm_crtc_helper_funcs *crtc_helper_funcs,
> const struct drm_encoder_helper_funcs *encoder_helper_funcs,
> int encoder_type, struct drm_connector *connector);
>
> Or with DRM_MODE_ENCODER_NONE:
>
> int drm_simple_kms_create_pipeline(struct drm_device *dev,
> const struct drm_plane_helper_funcs *plane_helper_funcs,
> const uint32_t *plane_formats, unsigned int format_count,
> const struct drm_crtc_helper_funcs *crtc_helper_funcs,
> struct drm_connector *connector);
My idea was to go even more radical and nuke all the crtc/plane/encoder
stuff and all the vfuncs entirely:
struct drm_simple_display_pipe {
struct drm_crtc crtc;
struct drm_plane plane;
/* maybe future versions will want to allow connecting to
* drm_bridge, but drm_encoder already supports this. */
struct drm_encoder encoder;
struct drm_simple_display_pipe_funcs *funcs;
/* here a pointer, since we want flexibility to integrate with
* panels and whatever. */
struct drm_connector *connector;
/* anything else simple support needs */
};
struct drm_simple_display_pipe_funcs {
void (*enable)(struct drm_simple_display_pipe *pipe, struct
struct drm_crtc_state *crtc_state);
void (*disable)(struct drm_simple_kms_create_pipeline *pipe);
/* this would be a combination of dirty_fb + atomic_plane_update */
void (*plane_update)(struct drm_simple_display_pipe *pipe,
struct drm_plane_state *plane_state);
/* maybe more? I think this should be enough. */
}
Not entirely sure about the actual interface, but that's the rough idea.
Should be pretty quick to implement using the atomic helpers we have
already.
Then we'd just have
drm_simple_display_pipe_init(struct drm_simple_display_pipe *pipe,
struct drm_simple_display_pipe_funcs *funcs,
struct drm_connnector *connector);
Simple drivers could then embed their struct into whatever they already
have (maybe right into their overall dev_priv, together with the single
drm_connector). Simple support would provide hooks for all the other
things needed, implemented using the super-simple hooks above.
Note: The above is just typed out without even looking at the details of
your current tinydrm driver. So most likely needs to be adjusted. But the
goal would be to make things _really_ simple. Only flexibility I'd leave
intact is drm_connector, created by the driver directly. And maybe the
option to specify a drm_bridge chain. But we can add a new
_init_with_bridge for that.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list