[PATCH 3/4] drm: add libdrm_amdgpu (v4)
Emil Velikov
emil.l.velikov at gmail.com
Wed Apr 29 07:05:07 PDT 2015
Hi Alex,
On 24 April 2015 at 16:23, Alex Deucher <alexdeucher at gmail.com> wrote:
> This is the new ioctl wrapper used by the new admgpu driver.
> It's primarily used by xf86-video-amdgpu and mesa.
>
> v2: fix amdgpu_drm.h install
> v3: Integrate some of the sugestions from Emil:
> clean up Makefile.am, configure.ac
> capitalize header guards
> fix _FILE_OFFSET_BITS with config.h
> use drm_mmap/drm_munmap
> Remove unused ARRAY_SIZE macro
> use shared list implementation
> use shared math implementation
> use drmGetNodeTypeFromFd helper
Huge thanks for picking my suggestions. There are a couple which got
lost considering the size of the patch. Do let me know if I have
missed the plot and some of them are not applicable.
> v4: remove unused tiling defines
>
> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> ---
> Makefile.am | 5 +
> Makefile.sources | 1 +
> amdgpu/Makefile.am | 53 ++
> amdgpu/amdgpu.h | 1276 ++++++++++++++++++++++++++++++++++++++++++++
> amdgpu/amdgpu_bo.c | 626 ++++++++++++++++++++++
> amdgpu/amdgpu_cs.c | 981 ++++++++++++++++++++++++++++++++++
> amdgpu/amdgpu_device.c | 241 +++++++++
> amdgpu/amdgpu_gpu_info.c | 275 ++++++++++
> amdgpu/amdgpu_internal.h | 208 ++++++++
> amdgpu/amdgpu_vamgr.c | 169 ++++++
> amdgpu/libdrm_amdgpu.pc.in | 10 +
> amdgpu/util_hash.c | 382 +++++++++++++
> amdgpu/util_hash.h | 99 ++++
> amdgpu/util_hash_table.c | 257 +++++++++
> amdgpu/util_hash_table.h | 65 +++
> configure.ac | 19 +
> include/drm/amdgpu_drm.h | 580 ++++++++++++++++++++
> 17 files changed, 5247 insertions(+)
> create mode 100644 amdgpu/Makefile.am
> create mode 100644 amdgpu/amdgpu.h
> create mode 100644 amdgpu/amdgpu_bo.c
> create mode 100644 amdgpu/amdgpu_cs.c
> create mode 100644 amdgpu/amdgpu_device.c
> create mode 100644 amdgpu/amdgpu_gpu_info.c
> create mode 100644 amdgpu/amdgpu_internal.h
> create mode 100644 amdgpu/amdgpu_vamgr.c
> create mode 100644 amdgpu/libdrm_amdgpu.pc.in
> create mode 100644 amdgpu/util_hash.c
> create mode 100644 amdgpu/util_hash.h
> create mode 100644 amdgpu/util_hash_table.c
> create mode 100644 amdgpu/util_hash_table.h
> create mode 100644 include/drm/amdgpu_drm.h
>
> --- /dev/null
> +++ b/amdgpu/amdgpu_device.c
> +#define RENDERNODE_MINOR_MASK 0xff7f
> +
You can drop this if you go for my suggestions below.
> +static unsigned fd_hash(void *key)
> +{
> + int fd = PTR_TO_UINT(key);
> + struct stat stat;
> + fstat(fd, &stat);
> +
> + if (!S_ISCHR(stat.st_mode))
Afaict this cannot/should not happen - might as well drop it ?
> + return stat.st_dev ^ stat.st_ino;
> + else
> + return stat.st_dev ^ (stat.st_rdev & RENDERNODE_MINOR_MASK);
One cannot get the render/primary node relation by masking/oring the 7th bit.
You can get the primary device name (via drmGetPrimaryDeviceNameFromFd
) and fstat it.
> +}
> +
> +static int fd_compare(void *key1, void *key2)
> +{
> + int fd1 = PTR_TO_UINT(key1);
> + int fd2 = PTR_TO_UINT(key2);
> + struct stat stat1, stat2;
> + fstat(fd1, &stat1);
> + fstat(fd2, &stat2);
> +
> + if (!S_ISCHR(stat1.st_mode) || !S_ISCHR(stat2.st_mode))
As above.
> + return stat1.st_dev != stat2.st_dev ||
> + stat1.st_ino != stat2.st_ino;
> + else
> + return major(stat1.st_rdev) != major(stat2.st_rdev) ||
> + (minor(stat1.st_rdev) & RENDERNODE_MINOR_MASK) !=
> + (minor(stat2.st_rdev) & RENDERNODE_MINOR_MASK);
Similar to above - please compare the primary device name for both fds.
> +}
> +
> diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
> new file mode 100644
> index 0000000..477cfd8
> --- /dev/null
> +++ b/include/drm/amdgpu_drm.h
> +struct drm_amdgpu_ctx_in {
> + uint32_t op;
> + uint32_t flags;
> + uint32_t ctx_id;
> + uint32_t pad;
> +};
> +
> +/** The same structure is shared for input/output */
> +struct drm_amdgpu_gem_metadata {
> + uint32_t handle; /* GEM Object handle */
> + uint32_t op; /** Do we want get or set metadata */
> + struct {
> + uint64_t flags;
> + uint64_t tiling_info; /* family specific tiling info */
> + uint32_t data_size_bytes;
Considering that this is fed directly into the kernel we should
pad/align it - similar to drm_amdgpu_ctx_in above.
> + uint32_t data[64];
> + } data;
> +};
> +
> +struct drm_amdgpu_gem_mmap_in {
> + uint32_t handle; /** the GEM object handle */
Ditto.
> +};
> +
> +struct drm_amdgpu_wait_cs_in {
> + uint64_t handle;
> + uint64_t timeout;
> + uint32_t ip_type;
> + uint32_t ip_instance;
> + uint32_t ring;
Ditto.
> +};
> +
> +/* Input structure for the INFO ioctl */
> +struct drm_amdgpu_info {
> + /* Where the return value will be stored */
> + uint64_t return_pointer;
> + /* The size of the return value. Just like "size" in "snprintf",
> + * it limits how many bytes the kernel can write. */
> + uint32_t return_size;
> + /* The query request id. */
> + uint32_t query;
> +
> + union {
> + struct {
> + uint32_t id;
> + } mode_crtc;
> +
> + struct {
> + /** AMDGPU_HW_IP_* */
> + uint32_t type;
> + /**
> + * Index of the IP if there are more IPs of the same type.
> + * Ignored by AMDGPU_INFO_HW_IP_COUNT.
> + */
> + uint32_t ip_instance;
> + } query_hw_ip;
> +
> + struct {
> + uint32_t dword_offset;
> + uint32_t count; /* number of registers to read */
> + uint32_t instance;
> + uint32_t flags;
> + } read_mmr_reg;
> +
> + struct {
> + /** AMDGPU_INFO_FW_* */
> + uint32_t fw_type;
> + /** Index of the IP if there are more IPs of the same type. */
> + uint32_t ip_instance;
> + /**
> + * Index of the engine. Whether this is used depends
> + * on the firmware type. (e.g. MEC, SDMA)
> + */
> + uint32_t index;
> + } query_fw;
> + };
> +};
> +
Might need some padding, not sure though.
> +struct drm_amdgpu_info_gds {
> + /** GDS GFX partition size */
> + uint32_t gds_gfx_partition_size;
> + /** GDS compute partition size */
> + uint32_t compute_partition_size;
> + /** total GDS memory size */
> + uint32_t gds_total_size;
> + /** GWS size per GFX partition */
> + uint32_t gws_per_gfx_partition;
> + /** GSW size per compute partition */
> + uint32_t gws_per_compute_partition;
> + /** OA size per GFX partition */
> + uint32_t oa_per_gfx_partition;
> + /** OA size per compute partition */
> + uint32_t oa_per_compute_partition;
> +};
Seems unused - neither libdrm_amdgpu nor winsys/amdgpu use it.
Cheers
Emil
More information about the dri-devel
mailing list