On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Sat, Sep 17, 2011 at 04:32:09PM -0500, Rob Clark wrote:
From: Rob Clark rob@ti.com
A DRM display driver for TI OMAP platform. Similar to omapfb (fbdev) and omap_vout (v4l2 display) drivers in the past, this driver uses the DSS2 driver to access the display hardware, including support for HDMI, DVI, and various types of LCD panels. And it implements GEM support for buffer allocation (for KMS as well as offscreen buffers used by the xf86-video-omap userspace xorg driver).
The driver maps CRTCs to overlays, encoders to overlay-managers, and connectors to dssdev's. Note that this arrangement might change slightly when support for drm_plane overlays is added.
For GEM support, non-scanout buffers are using the shmem backed pages provided by GEM core (In drm_gem_object_init()). In the case of scanout buffers, which need to be physically contiguous, those are allocated with CMA and use drm_gem_private_object_init().
See userspace xorg driver: git://github.com/robclark/xf86-video-omap.git
Refer to this link for CMA (Continuous Memory Allocator): http://lkml.org/lkml/2011/8/19/302
Links to previous versions of the patch: v1: http://lwn.net/Articles/458137/
History:
v2: replace omap_vram with CMA for scanout buffer allocation, remove unneeded functions, use dma_addr_t for physical addresses, error handling cleanup, refactor attach/detach pages into common drm functions, split non-userspace-facing API into omap_priv.h, remove plugin API
v1: original
This looks good. A few minors things to add to the TODO to be looked at before moving the driver out of staging:
- review the dss/kms interface mismatch issues you've noted in the code
- review the sync object implementation when an actual gpu side user
(not just pageflip) shows up. Also a minor comment on your ioctl interface, see below. A haven't looked too closely at the fbdev stuff, simply because that's way outside my expertise. I don't think there's anything in there that can't be fixed up in staging.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (for -staging)
Thanks Daniel
[snip]
On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter daniel@ffwll.ch wrote:
+struct drm_omap_gem_cpu_fini {
uint32_t handle; /* buffer handle (in) */uint32_t op; /* mask of omap_gem_op (in) *//* TODO maybe here we pass down info about what regions are touched* by sw so we can be clever about cache ops? For now a placeholder,* set to zero and we just do full buffer flush..*/uint32_t nregions;+};
Note that you cannot extend ioctl structures in an easy way with the drm ioctl infrastructure. So I think you need at least a pointer here, too.
oh, hrm.. it did look like drm_ioctl() code was handling the case where userspace size was smaller than current kernel side size, ie. the 'if (drv_size > asize)' stuff.. or am I missing something else?
If so, opinions on adding a padding field to the end vs ptr?
+struct drm_omap_gem_info {
uint32_t handle; /* buffer handle (in) */uint32_t pad;uint64_t offset; /* out */+};
+#define DRM_OMAP_GET_PARAM 0x00 +#define DRM_OMAP_SET_PARAM 0x01 +#define DRM_OMAP_GET_BASE 0x02
Unused ;-)
well, err, placeholder ;-)
I hope it is tolerable to leave a placeholder for this.. otherwise it becomes a royal PITA if the ioctl nr for the plugin part is moving upwards every time I add a new ioctl.. although I could change that to a comment about the skipped ioctl cmd nr if that is better
BR, -R
+#define DRM_OMAP_GEM_NEW 0x03 +#define DRM_OMAP_GEM_CPU_PREP 0x04 +#define DRM_OMAP_GEM_CPU_FINI 0x05 +#define DRM_OMAP_GEM_INFO 0x06 +#define DRM_OMAP_NUM_IOCTLS 0x07