Re: [PATCH 0/2] drm: add SimpleDRM driver

[Daniel Vetter]

On Thu, Aug 04, 2016 at 04:03:18PM +0200, Noralf Trønnes wrote:
> This patchset adds the simpledrm driver by David Herrmann based on a
> patchset[1] from 2014. That patchset also included patches for kicking
> out simpledrm by real drivers. I have stayed away from that since it
> involves another subsystem and I would probably be unable to answer any
> questions about the implementation.

Need David's input on this, but I think the force-removal of simpledrm
when other drivers take over is required. Otherwise hilarity can ensue.
If we leave this out for the inital merge then I think we need a really
big warning in Kconfig that if people aren't careful it could result in a
kaboom.

> I have done my best to bring simpledrm up to speed. However I was unable to
> loose drm_legacy_mmap() in sdrm_drm_mmap() since I don't understand much of
> the gem code.

You can just nuke drm_legacy_mmap and the if check, plus the drm_legacy.h
include. There should be no reason at all for that.

> I was left with some questions after doing this:
> 
> - Is there any reason why simpledrm can't use drm_gem_cma_helper?
>   One obvious difference is that of contiguous memory allocation:
> 
>   sdrm_gem_get_pages():
> 	obj->pages = drm_malloc_ab(num, sizeof(*obj->pages));
> 	for (i = 0; i < num; ++i) {
> 		obj->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
> 	obj->vmapping = vmap(obj->pages, num, 0, PAGE_KERNEL);
> 
>   drm_gem_cma_create():
>         cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
>                                       GFP_KERNEL | __GFP_NOWARN);

cma means contiguous memory allocator, that's pretty much the entire
point. Once you have that there's not much left really. Note that since
sdrm was submitted we gained some helpers for normal (shmem-backed) gem
buffers. I think we could extend those a bit to provide the basics for
dumb gem based buffers and use that in sdrm. Otoh there's probably only
udl which could benefit, so not sure this is worth it much. And mmap
helper, maybe.

> - Could we set this range to the actual width/height of the native framebuffer?
> 
>   sdrm_drm_modeset_init():
> 	ddev->mode_config.min_width = 1;
> 	ddev->mode_config.min_height = 1;
> 	ddev->mode_config.max_width = 8192;
> 	ddev->mode_config.max_height = 8192;

Probably a good idea if we don't support modeset changes with simpledrm.

> - Is there a usecase for offsetting the dumb buffer when blitted onto the
>   native buffer?
> 
>   sdrm_blit():
> 	/* get scanout offsets */
> 	xoff = 0;
> 	yoff = 0;
> 	if (sdrm->pipe.plane.fb == fb) {
> 		xoff = sdrm->pipe.crtc.x;
> 		yoff = sdrm->pipe.crtc.y;
> 	}

I don't think allowing the plane to be positioned is a good idea. And
since it's using the simple pipe helpers now that should be even
impossible.

> I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> framebuffer and producing this node:
> 
>         framebuffer@1e887000 {
>                 compatible = "simple-framebuffer";
>                 reg = <0x1e887000 0x36c600>;
>                 format = "r5g6b5";
>                 width = <1824>;
>                 height = <984>;
>                 stride = <3648>;
>                 status = "okay";
>         };
> 
> I have only tested with fbcon and modetest (XR24,RG16).

If you can, booting this on a vesa/uefi platform would be interesting too,
just to make sure.
-Daniel


> 
> 
> Noralf.
> 
> 
> Changes from previous version[2]:
> - Remove FB_SIMPLE=n dependency to avoid kconfig recursive error
> - Changed module name to match kconfig help text: sdrm -> simpledrm
> - Use drm_simple_display_pipe
> - Replace deprecated drm_platform_init()
> - sdrm_dumb_create(): drm_gem_object_unreference() -> *_unlocked()
> - sdrm_dumb_map_offset(): drm_gem_object_lookup() remove drm_device parameter
> - sdrm_drm_mmap() changes:
>   Remove struct_mutex locking
>   Add drm_vma_offset_{lock,unlock}_lookup()
>   drm_mmap() -> drm_legacy_mmap()
> - dma_buf_begin_cpu_access() doesn't require start and length anymore
> - Use drm_cvt_mode() instead of open coding a mode
> - Fix format conversion. In the intermediate step, store the 8/6/5 bit color
>   value in the upper part of the 16-bit color variable, not the lower.
> - Support clips == NULL in sdrm_dirty()
> - Set mode_config.preferred_depth
> - Attach mode_config.dirty_info_property to connector
> fbdev:
> - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
> - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
> - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2014-January/052584.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2014-January/052594.html
> 
> 
> Further history:
> 
> [PATCH v4 0/6] SimpleDRM Driver
> https://lists.freedesktop.org/archives/dri-devel/2013-September/044638.html
> 
> [PATCH v2 00/14] Platform Framebuffers and SimpleDRM
> https://lists.freedesktop.org/archives/dri-devel/2013-July/041090.html
> 
> [RFC 0/6] SimpleDRM Driver (was: dvbe driver)
> https://lists.freedesktop.org/archives/dri-devel/2013-June/040386.html
> 
> [PATCH 0/9] System Framebuffer Bus (sysfb)
> https://lists.freedesktop.org/archives/dri-devel/2013-February/035013.html
> 
> 
> Noralf Trønnes (2):
>   drm: add SimpleDRM driver
>   drm: simpledrm: add fbdev fallback support
> 
>  drivers/gpu/drm/Kconfig                      |   2 +
>  drivers/gpu/drm/Makefile                     |   1 +
>  drivers/gpu/drm/simpledrm/Kconfig            |  22 ++
>  drivers/gpu/drm/simpledrm/Makefile           |   5 +
>  drivers/gpu/drm/simpledrm/simpledrm.h        | 111 ++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_damage.c | 314 +++++++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_drv.c    | 298 +++++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_fbdev.c  | 160 ++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_gem.c    | 276 +++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_kms.c    | 288 ++++++++++++++++++++++++
>  10 files changed, 1477 insertions(+)
>  create mode 100644 drivers/gpu/drm/simpledrm/Kconfig
>  create mode 100644 drivers/gpu/drm/simpledrm/Makefile
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm.h
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_damage.c
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_drv.c
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_gem.c
>  create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_kms.c
> 
> --
> 2.8.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch