[PATCH] RFCv2: omapdrm DRM/KMS driver for TI OMAP platforms

[Rob Clark]

On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Sat, Sep 17, 2011 at 04:32:09PM -0500, Rob Clark wrote:
>> From: Rob Clark <rob at 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 at ffwll.ch> (for -staging)
>

Thanks Daniel

[snip]

On Sun, Sep 18, 2011 at 5:23 AM, Daniel Vetter <daniel at 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