On Thu, Nov 3, 2011 at 6:22 PM, Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> wrote:
> From: Alan Cox <alan@xxxxxxxxxxxxxxx>
>
> Not really a nice way to split this up further for submission. This
> provides all the DRM interfacing logic, the headers and relevant glue.
I've started merging it, and my main review focus is as always the
userspace interfaces, which we are now setting in stone for ever (or
5-10 years whichever is shorter).
So generally we need to provide a userspace interface via ioctls, we
do this with a shared header file that goes in include/drm/ with the
Kbuild bits
Now I'm assuming psb_drm.h is meant to be this file? but as-is its not
really what I'd expect,
If you look at radeon you'll see defines for device specific ioctls,
same for i915, now I note these tables start at 0 and work their way
up,
Now if I understand the holes in this are due to some old userspace
code that is probably broken anyway,
It might be worth renaming psb_drm.h to gma500_drm.h to reflect the
overall driver name and then maybe start with a clean interface, I'm
willing to listen to why this is a bad plan, but I'd rather not push
psb_drm.h into kernel header packages and libdrm if there is a
conflicting one in existance (or conflicting 6).
some more comments below,
> +#define PSB_GPU_ACCESS_READ (1ULL << 32)
> +#define PSB_GPU_ACCESS_WRITE (1ULL << 33)
> +#define PSB_GPU_ACCESS_MASK (PSB_GPU_ACCESS_READ | PSB_GPU_ACCESS_WRITE)
> +
> +#define PSB_BO_FLAG_COMMAND (1ULL << 52)
Any reason it start at 52?
> +struct drm_psb_mode_operation_arg {
> + u32 obj_id;
> + u16 operation;
> + struct drm_mode_modeinfo mode;
> + void *data;
> +};
No void * in ioctl args, no u16 in ioctls args, not really sure what a
mode "operation" is here either. Try and design the ioctls to avoid
compat wrapper requirements.
Maybe move the operation flags up here.
> +
> +struct drm_psb_stolen_memory_arg {
> + u32 base;
> + u32 size;
> +};
Why does someone care about this? is it a workaround for lack of
decent memory management?
> +/* Controlling the kernel modesetting buffers */
> +
> +#define DRM_PSB_SIZES 0x07
> +#define DRM_PSB_FUSE_REG 0x08
> +#define DRM_PSB_DC_STATE 0x0A
> +#define DRM_PSB_ADB 0x0B
> +#define DRM_PSB_MODE_OPERATION 0x0C
> +#define DRM_PSB_STOLEN_MEMORY 0x0D
> +#define DRM_PSB_REGISTER_RW 0x0E
Direct read/writing of registers is not something, the regs being hit
here seem like workarounds for not having good overlay support in the
kernel perhaps,
can the new plane stuff help workaround this?
> +#define DRM_PSB_GEM_CREATE 0x10
> +#define DRM_PSB_2D_OP 0x11 /* Will be merged later */
> +#define DRM_PSB_GEM_MMAP 0x12
> +#define DRM_PSB_DPST 0x1B
> +#define DRM_PSB_GAMMA 0x1C
> +#define DRM_PSB_DPST_BL 0x1D
> +#define DRM_PSB_GET_PIPE_FROM_CRTC_ID 0x1F
If these are compat with somewhere else please state where the master
database is kept so future people can avoid collisions with closed
source drivers.
> +
> +#define PSB_MODE_OPERATION_MODE_VALID 0x01
> +#define PSB_MODE_OPERATION_SET_DC_BASE 0x02
> +
> +struct drm_psb_get_pipe_from_crtc_id_arg {
> + /** ID of CRTC being requested **/
> + u32 crtc_id;
> +
> + /** pipe of requested CRTC **/
> + u32 pipe;
> +};
I'm happy that we can fix all these problems incrementally with
patches on top of these, as I take the notion that the userspace
ioctls aren't set in stone until Linus merges and does a release
containing them.
So you can probably considered these patches merged at least, I'll
just keep them in a topic branch which I'll stick into the drm-next
queue.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
