Re: [PATCH 08/12] gma500: Add the core DRM files and headers

[Alan Cox]

> 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

At the moment the only public API is the generic bits.

> Now I'm assuming psb_drm.h is meant to be this file? but as-is its not
> really what I'd expect,

Not yet.

> 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,

I'll move the ones we want to zero.

> Now if I understand the holes in this are due to some old userspace
> code that is probably broken anyway,

Yep.. but really I can kill them off. The total number of people affected
will be < 10.

> 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).

Last time I looked there were conflicts between the various IMG based
Intel ones let along with anything else - lost cause so we might as well
do it right.

We could use a new namespace that might be smarter in fact.

> > +#define PSB_BO_FLAG_COMMAND         (1ULL << 52)
> 
> Any reason it start at 52?

That may be obsolete

> 
> > +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.

Again I need to see if we can simply kill it off

> > +
> > +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?

A smart GMA500 aware server can allocate GEM objects from host memory or
from the stolen memory area. Being smart about it really means knowing
how much stolen memory is so it can see how best to use it.

> > +/* 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?

That can go - its basically left for debugging.
 
> 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.

Cool.

I'll take the secateurs to the interfaces.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/12] gma500: Add the core DRM files and headers

[Dave Airlie]

On Thu, Nov 3, 2011 at 6:22 PM, Alan Cox  wrote:
> From: Alan Cox 
>
> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel