On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Sat, Sep 17, 2011 at 04:32:09PM -0500, Rob Clark wrote: >> From: Rob Clark <rob@xxxxxx> >> >> 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@xxxxxxxx> (for -staging) > Thanks Daniel [snip] On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter <daniel@xxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
