Re: [PATCH 3/4] drm: add libdrm_amdgpu (v4)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

On 24 April 2015 at 16:23, Alex Deucher <alexdeucher@xxxxxxxxx> 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@xxxxxxx>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux