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

[Daniel Vetter]

On Sun, Sep 18, 2011 at 10:31:45AM -0500, Rob Clark wrote:
> 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:
> 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?

Oops, misconception on my side - I've had a year long typo in my i915
overlay code, but that was not about the size (as I've thought), but about
the ioctl number (which libdrm somehow ignores because it stitches stuff
together on my own). Indeed, upon reading that code you seem to be right.
There's the little funny caveat though that if userspace passes in less
data than the kernel expects, it's not being cleared. But that can be
fixed.

> 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 know, and I don't mind.  Because not having it is just a unnecessary
pain for you ;-) Maybe just add a comment.

> 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

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48