[Openchrome-devel] [RFC 1/21] Add VIA DRM driver

[James Simmons]

> > commit 1fcf23d361375645d586756d126b436796ba4fba
> > Author: James Simmons <jsimmons at infradead.org>
> > Date:   Sat Jun 8 09:31:57 2013 -0400
> >
> >     via: New KMS ioctls and hardware to support.
> >
> >     Add new VIA pci ids to support newer hardware. Cleanup userspace
> >     api structs to remove kernel types and add the new KMS ioctls we
> >     will be supporting.
> 
> why remove all the __u32? seems to just undo
> 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae
> 
> and definitely shouldn't be in this patch.

Attempting to sync my userland header in xorg with what is used in the 
kernel. Since we have BSD users I attempted to play nice. This weekend
I played with a new header format that should satisfy everyone.
 
> Also I don't think its really necessary to add all the pci ids there,
> just put them in the driver.

Would you be okay with the pci ids being in via_drm.h instead? I have to 
look but if I remember correctly some of those pci ids are for the north 
bridge bus which is used to detect how much VRAM we have.

> >
> >  #define DRM_IOCTL_VIA_ALLOCMEM   DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_ALLOCMEM, drm_via_mem_t)
> >  #define DRM_IOCTL_VIA_FREEMEM    DRM_IOW( DRM_COMMAND_BASE + DRM_VIA_FREEMEM, drm_via_mem_t)
> > @@ -86,6 +89,7 @@
> >  #define DRM_IOCTL_VIA_FB_INIT    DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_FB_INIT, drm_via_fb_t)
> >  #define DRM_IOCTL_VIA_MAP_INIT   DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_MAP_INIT, drm_via_init_t)
> >  #define DRM_IOCTL_VIA_DEC_FUTEX   DRM_IOW( DRM_COMMAND_BASE + DRM_VIA_DEC_FUTEX, drm_via_futex_t)
> > +#define DRM_IOCTL_VIA_OLD_GEM_CREATE  DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_OLD_GEM_CREATE, struct drm_via_gem_create)
> >  #define DRM_IOCTL_VIA_DMA_INIT   DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_DMA_INIT, drm_via_dma_init_t)
> >  #define DRM_IOCTL_VIA_CMDBUFFER          DRM_IOW( DRM_COMMAND_BASE + DRM_VIA_CMDBUFFER, drm_via_cmdbuffer_t)
> >  #define DRM_IOCTL_VIA_FLUSH      DRM_IO(  DRM_COMMAND_BASE + DRM_VIA_FLUSH)
> > @@ -96,6 +100,13 @@
> >  #define DRM_IOCTL_VIA_DMA_BLIT    DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_DMA_BLIT, drm_via_dmablit_t)
> >  #define DRM_IOCTL_VIA_BLIT_SYNC   DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_BLIT_SYNC, drm_via_blitsync_t)
> >
> > +/* KMS ioctls */
> > +#define DRM_IOCTL_VIA_GETPARAM    DRM_IOR(DRM_COMMAND_BASE + DRM_VIA_GETPARAM, struct drm_via_param)
> > +#define DRM_IOCTL_VIA_SETPARAM    DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_SETPARAM, struct drm_via_param)
> > +#define DRM_IOCTL_VIA_GEM_CREATE  DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
> > +#define DRM_IOCTL_VIA_GEM_WAIT    DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_GEM_WAIT, struct drm_via_gem_wait)
> > +#define DRM_IOCTL_VIA_GEM_STATE   DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_GEM_STATE, struct drm_via_gem_create)
> 
> why does gem_state take a gem_create struct? is it the same info it returns?

Yes that same info is returned. Prehaps struct drm_via_gem_create is a 
poor name. Maybe struct drm_via_gem_object ?

> >  typedef struct drm_via_dmablit {
> > -       __u32 num_lines;
> > -       __u32 line_length;
> > +       uint32_t num_lines;
> > +       uint32_t line_length;
> >
> > -       __u32 fb_addr;
> > -       __u32 fb_stride;
> > +       uint32_t fb_addr;
> > +       uint32_t fb_stride;
> >
> >         unsigned char *mem_addr;
> > -       __u32 mem_stride;
> > +       uint32_t  mem_stride;
> >
> > -       __u32 flags;
> > +       int bounce_buffer;
> 
> ^ totally wtf here? no explains.

Leftovers from older work that doesn't exist anymore. My bad.
 
> >         int to_fb;
> >
> >         drm_via_blitsync_t sync;
> >  } drm_via_dmablit_t;
> >
> > -struct via_file_private {
> > -       struct list_head obj_list;
> > +/* Ioctl to query kernel params:
> > + */
> > +#define VIA_PARAM_CHIPSET_ID           0
> > +#define VIA_PARAM_REVISION_ID          1
> > +
> > +struct drm_via_param {
> > +       uint64_t param;
> > +       uint64_t value;
> > +};
> > +
> > +struct drm_via_gem_create {
> > +       /**
> > +        * Requested size for the object.
> > +        *
> > +        * The (page-aligned) allocated size for the object will be returned.
> > +        */
> > +       uint64_t size;
> > +
> > +       /*
> > +        * Place the memory at the proper byte alignment.
> > +        */
> > +       uint32_t alignment;
> > +
> > +       /**
> > +        * Format of data i.e tile pitch, for linear it is zero
> > +        */
> > +       uint32_t pitch;
> > +
> > +       /**
> > +        * Give hints where to allocate this object.
> > +        */
> > +       uint32_t domains;
> > +
> > +       /**
> > +        * chmod values applied to a buffer.
> > +        */
> > +       uint32_t mode_t;
> > +
> > +       /**
> > +        * Offset to start of memory region.
> > +        */
> > +       uint64_t offset;
> > +
> > +       /**
> > +        * Returned handle need to mmap the buffer.
> > +        */
> > +       uint64_t map_handle;
> > +
> > +       /**
> > +        * Returned handle for the object.
> > +        *
> > +        * Object handles are nonzero.
> > +        */
> > +       uint32_t handle;
> > +
> > +       /**
> > +        * Padding for future expansion.
> > +        */
> > +       uint32_t pad1;
> > +       uint64_t pad2;
> > +       uint64_t pad3;
> > +       uint64_t pad4;
> 
> 
> Don't bother with padding here, if you need to create another ioctl,
> create another ioctl.

Would you mind if I keep one pad field to make it 64 bit align?
 
> This is just my initial interface pass, because mostly everything else
> can be cleaned up in tree,
> the ABI not so much.
> 
> Dave.
> Dave.
> _______________________________________________
> Openchrome-devel mailing list
> Openchrome-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/openchrome-devel
>