| Message ID | 1471614628-11585-1-git-send-email-maraeo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 19.08.2016 um 15:50 schrieb Marek Olšák: > From: Marek Olšák <marek.olsak@amd.com> > > This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. > > See the comment in the code. Basically, don't do cleanups in this header. > > Signed-off-by: Marek Olšák <marek.olsak@amd.com> I completely agree with you that this was a bad move, but I fear that we will run into opposition with that. Adding Mikko Rapeli who made the reverted patch to comment. Regards, Christian. > --- > include/uapi/drm/amdgpu_drm.h | 295 +++++++++++++++++++++--------------------- > 1 file changed, 150 insertions(+), 145 deletions(-) > > diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h > index 462246a..b39e07c 100644 > --- a/include/uapi/drm/amdgpu_drm.h > +++ b/include/uapi/drm/amdgpu_drm.h > @@ -29,6 +29,11 @@ > * Keith Whitwell <keith@tungstengraphics.com> > */ > > +/* IT IS NOT ALLOWED TO CHANGE THIS HEADER WITHOUT DOING THE SAME IN LIBDRM !!! > + * THIS IS NOT A UAPI HEADER. IT IS ONLY A MIRROR OF ITS COUNTERPART IN LIBDRM. > + * USERSPACE SHOULD USE THE HEADERS FROM LIBDRM. NOT THIS ONE. > + */ > + > #ifndef __AMDGPU_DRM_H__ > #define __AMDGPU_DRM_H__ > > @@ -80,19 +85,19 @@ extern "C" { > > struct drm_amdgpu_gem_create_in { > /** the requested memory size */ > - __u64 bo_size; > + uint64_t bo_size; > /** physical start_addr alignment in bytes for some HW requirements */ > - __u64 alignment; > + uint64_t alignment; > /** the requested memory domains */ > - __u64 domains; > + uint64_t domains; > /** allocation flags */ > - __u64 domain_flags; > + uint64_t domain_flags; > }; > > struct drm_amdgpu_gem_create_out { > /** returned GEM object handle */ > - __u32 handle; > - __u32 _pad; > + uint32_t handle; > + uint32_t _pad; > }; > > union drm_amdgpu_gem_create { > @@ -109,28 +114,28 @@ union drm_amdgpu_gem_create { > > struct drm_amdgpu_bo_list_in { > /** Type of operation */ > - __u32 operation; > + uint32_t operation; > /** Handle of list or 0 if we want to create one */ > - __u32 list_handle; > + uint32_t list_handle; > /** Number of BOs in list */ > - __u32 bo_number; > + uint32_t bo_number; > /** Size of each element describing BO */ > - __u32 bo_info_size; > + uint32_t bo_info_size; > /** Pointer to array describing BOs */ > - __u64 bo_info_ptr; > + uint64_t bo_info_ptr; > }; > > struct drm_amdgpu_bo_list_entry { > /** Handle of BO */ > - __u32 bo_handle; > + uint32_t bo_handle; > /** New (if specified) BO priority to be used during migration */ > - __u32 bo_priority; > + uint32_t bo_priority; > }; > > struct drm_amdgpu_bo_list_out { > /** Handle of resource list */ > - __u32 list_handle; > - __u32 _pad; > + uint32_t list_handle; > + uint32_t _pad; > }; > > union drm_amdgpu_bo_list { > @@ -154,26 +159,26 @@ union drm_amdgpu_bo_list { > > struct drm_amdgpu_ctx_in { > /** AMDGPU_CTX_OP_* */ > - __u32 op; > + uint32_t op; > /** For future use, no flags defined so far */ > - __u32 flags; > - __u32 ctx_id; > - __u32 _pad; > + uint32_t flags; > + uint32_t ctx_id; > + uint32_t _pad; > }; > > union drm_amdgpu_ctx_out { > struct { > - __u32 ctx_id; > - __u32 _pad; > + uint32_t ctx_id; > + uint32_t _pad; > } alloc; > > struct { > /** For future use, no flags defined so far */ > - __u64 flags; > + uint64_t flags; > /** Number of resets caused by this context so far. */ > - __u32 hangs; > + uint32_t hangs; > /** Reset status since the last call of the ioctl. */ > - __u32 reset_status; > + uint32_t reset_status; > } state; > }; > > @@ -193,12 +198,12 @@ union drm_amdgpu_ctx { > #define AMDGPU_GEM_USERPTR_REGISTER (1 << 3) > > struct drm_amdgpu_gem_userptr { > - __u64 addr; > - __u64 size; > + uint64_t addr; > + uint64_t size; > /* AMDGPU_GEM_USERPTR_* */ > - __u32 flags; > + uint32_t flags; > /* Resulting GEM handle */ > - __u32 handle; > + uint32_t handle; > }; > > /* same meaning as the GB_TILE_MODE and GL_MACRO_TILE_MODE fields */ > @@ -230,28 +235,28 @@ struct drm_amdgpu_gem_userptr { > /** The same structure is shared for input/output */ > struct drm_amdgpu_gem_metadata { > /** GEM Object handle */ > - __u32 handle; > + uint32_t handle; > /** Do we want get or set metadata */ > - __u32 op; > + uint32_t op; > struct { > /** For future use, no flags defined so far */ > - __u64 flags; > + uint64_t flags; > /** family specific tiling info */ > - __u64 tiling_info; > - __u32 data_size_bytes; > - __u32 data[64]; > + uint64_t tiling_info; > + uint32_t data_size_bytes; > + uint32_t data[64]; > } data; > }; > > struct drm_amdgpu_gem_mmap_in { > /** the GEM object handle */ > - __u32 handle; > - __u32 _pad; > + uint32_t handle; > + uint32_t _pad; > }; > > struct drm_amdgpu_gem_mmap_out { > /** mmap offset from the vma offset manager */ > - __u64 addr_ptr; > + uint64_t addr_ptr; > }; > > union drm_amdgpu_gem_mmap { > @@ -261,18 +266,18 @@ union drm_amdgpu_gem_mmap { > > struct drm_amdgpu_gem_wait_idle_in { > /** GEM object handle */ > - __u32 handle; > + uint32_t handle; > /** For future use, no flags defined so far */ > - __u32 flags; > + uint32_t flags; > /** Absolute timeout to wait */ > - __u64 timeout; > + uint64_t timeout; > }; > > struct drm_amdgpu_gem_wait_idle_out { > /** BO status: 0 - BO is idle, 1 - BO is busy */ > - __u32 status; > + uint32_t status; > /** Returned current memory domain */ > - __u32 domain; > + uint32_t domain; > }; > > union drm_amdgpu_gem_wait_idle { > @@ -282,18 +287,18 @@ union drm_amdgpu_gem_wait_idle { > > struct drm_amdgpu_wait_cs_in { > /** Command submission handle */ > - __u64 handle; > + uint64_t handle; > /** Absolute timeout to wait */ > - __u64 timeout; > - __u32 ip_type; > - __u32 ip_instance; > - __u32 ring; > - __u32 ctx_id; > + uint64_t timeout; > + uint32_t ip_type; > + uint32_t ip_instance; > + uint32_t ring; > + uint32_t ctx_id; > }; > > struct drm_amdgpu_wait_cs_out { > /** CS status: 0 - CS completed, 1 - CS still busy */ > - __u64 status; > + uint64_t status; > }; > > union drm_amdgpu_wait_cs { > @@ -307,11 +312,11 @@ union drm_amdgpu_wait_cs { > /* Sets or returns a value associated with a buffer. */ > struct drm_amdgpu_gem_op { > /** GEM object handle */ > - __u32 handle; > + uint32_t handle; > /** AMDGPU_GEM_OP_* */ > - __u32 op; > + uint32_t op; > /** Input or return value */ > - __u64 value; > + uint64_t value; > }; > > #define AMDGPU_VA_OP_MAP 1 > @@ -330,18 +335,18 @@ struct drm_amdgpu_gem_op { > > struct drm_amdgpu_gem_va { > /** GEM object handle */ > - __u32 handle; > - __u32 _pad; > + uint32_t handle; > + uint32_t _pad; > /** AMDGPU_VA_OP_* */ > - __u32 operation; > + uint32_t operation; > /** AMDGPU_VM_PAGE_* */ > - __u32 flags; > + uint32_t flags; > /** va address to assign . Must be correctly aligned.*/ > - __u64 va_address; > + uint64_t va_address; > /** Specify offset inside of BO to assign. Must be correctly aligned.*/ > - __u64 offset_in_bo; > + uint64_t offset_in_bo; > /** Specify mapping size. Must be correctly aligned. */ > - __u64 map_size; > + uint64_t map_size; > }; > > #define AMDGPU_HW_IP_GFX 0 > @@ -358,24 +363,24 @@ struct drm_amdgpu_gem_va { > #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03 > > struct drm_amdgpu_cs_chunk { > - __u32 chunk_id; > - __u32 length_dw; > - __u64 chunk_data; > + uint32_t chunk_id; > + uint32_t length_dw; > + uint64_t chunk_data; > }; > > struct drm_amdgpu_cs_in { > /** Rendering context id */ > - __u32 ctx_id; > + uint32_t ctx_id; > /** Handle of resource list associated with CS */ > - __u32 bo_list_handle; > - __u32 num_chunks; > - __u32 _pad; > - /** this points to __u64 * which point to cs chunks */ > - __u64 chunks; > + uint32_t bo_list_handle; > + uint32_t num_chunks; > + uint32_t _pad; > + /** this points to uint64_t * which point to cs chunks */ > + uint64_t chunks; > }; > > struct drm_amdgpu_cs_out { > - __u64 handle; > + uint64_t handle; > }; > > union drm_amdgpu_cs { > @@ -392,32 +397,32 @@ union drm_amdgpu_cs { > #define AMDGPU_IB_FLAG_PREAMBLE (1<<1) > > struct drm_amdgpu_cs_chunk_ib { > - __u32 _pad; > + uint32_t _pad; > /** AMDGPU_IB_FLAG_* */ > - __u32 flags; > + uint32_t flags; > /** Virtual address to begin IB execution */ > - __u64 va_start; > + uint64_t va_start; > /** Size of submission */ > - __u32 ib_bytes; > + uint32_t ib_bytes; > /** HW IP to submit to */ > - __u32 ip_type; > + uint32_t ip_type; > /** HW IP index of the same type to submit to */ > - __u32 ip_instance; > + uint32_t ip_instance; > /** Ring index to submit to */ > - __u32 ring; > + uint32_t ring; > }; > > struct drm_amdgpu_cs_chunk_dep { > - __u32 ip_type; > - __u32 ip_instance; > - __u32 ring; > - __u32 ctx_id; > - __u64 handle; > + uint32_t ip_type; > + uint32_t ip_instance; > + uint32_t ring; > + uint32_t ctx_id; > + uint64_t handle; > }; > > struct drm_amdgpu_cs_chunk_fence { > - __u32 handle; > - __u32 offset; > + uint32_t handle; > + uint32_t offset; > }; > > struct drm_amdgpu_cs_chunk_data { > @@ -489,53 +494,53 @@ struct drm_amdgpu_cs_chunk_data { > > struct drm_amdgpu_query_fw { > /** AMDGPU_INFO_FW_* */ > - __u32 fw_type; > + uint32_t fw_type; > /** > * Index of the IP if there are more IPs of > * the same type. > */ > - __u32 ip_instance; > + uint32_t ip_instance; > /** > * Index of the engine. Whether this is used depends > * on the firmware type. (e.g. MEC, SDMA) > */ > - __u32 index; > - __u32 _pad; > + uint32_t index; > + uint32_t _pad; > }; > > /* Input structure for the INFO ioctl */ > struct drm_amdgpu_info { > /* Where the return value will be stored */ > - __u64 return_pointer; > + uint64_t return_pointer; > /* The size of the return value. Just like "size" in "snprintf", > * it limits how many bytes the kernel can write. */ > - __u32 return_size; > + uint32_t return_size; > /* The query request id. */ > - __u32 query; > + uint32_t query; > > union { > struct { > - __u32 id; > - __u32 _pad; > + uint32_t id; > + uint32_t _pad; > } mode_crtc; > > struct { > /** AMDGPU_HW_IP_* */ > - __u32 type; > + uint32_t type; > /** > * Index of the IP if there are more IPs of the same > * type. Ignored by AMDGPU_INFO_HW_IP_COUNT. > */ > - __u32 ip_instance; > + uint32_t ip_instance; > } query_hw_ip; > > struct { > - __u32 dword_offset; > + uint32_t dword_offset; > /** number of registers to read */ > - __u32 count; > - __u32 instance; > + uint32_t count; > + uint32_t instance; > /** For future use, no flags defined so far */ > - __u32 flags; > + uint32_t flags; > } read_mmr_reg; > > struct drm_amdgpu_query_fw query_fw; > @@ -544,31 +549,31 @@ struct drm_amdgpu_info { > > struct drm_amdgpu_info_gds { > /** GDS GFX partition size */ > - __u32 gds_gfx_partition_size; > + uint32_t gds_gfx_partition_size; > /** GDS compute partition size */ > - __u32 compute_partition_size; > + uint32_t compute_partition_size; > /** total GDS memory size */ > - __u32 gds_total_size; > + uint32_t gds_total_size; > /** GWS size per GFX partition */ > - __u32 gws_per_gfx_partition; > + uint32_t gws_per_gfx_partition; > /** GSW size per compute partition */ > - __u32 gws_per_compute_partition; > + uint32_t gws_per_compute_partition; > /** OA size per GFX partition */ > - __u32 oa_per_gfx_partition; > + uint32_t oa_per_gfx_partition; > /** OA size per compute partition */ > - __u32 oa_per_compute_partition; > - __u32 _pad; > + uint32_t oa_per_compute_partition; > + uint32_t _pad; > }; > > struct drm_amdgpu_info_vram_gtt { > - __u64 vram_size; > - __u64 vram_cpu_accessible_size; > - __u64 gtt_size; > + uint64_t vram_size; > + uint64_t vram_cpu_accessible_size; > + uint64_t gtt_size; > }; > > struct drm_amdgpu_info_firmware { > - __u32 ver; > - __u32 feature; > + uint32_t ver; > + uint32_t feature; > }; > > #define AMDGPU_VRAM_TYPE_UNKNOWN 0 > @@ -582,61 +587,61 @@ struct drm_amdgpu_info_firmware { > > struct drm_amdgpu_info_device { > /** PCI Device ID */ > - __u32 device_id; > + uint32_t device_id; > /** Internal chip revision: A0, A1, etc.) */ > - __u32 chip_rev; > - __u32 external_rev; > + uint32_t chip_rev; > + uint32_t external_rev; > /** Revision id in PCI Config space */ > - __u32 pci_rev; > - __u32 family; > - __u32 num_shader_engines; > - __u32 num_shader_arrays_per_engine; > + uint32_t pci_rev; > + uint32_t family; > + uint32_t num_shader_engines; > + uint32_t num_shader_arrays_per_engine; > /* in KHz */ > - __u32 gpu_counter_freq; > - __u64 max_engine_clock; > - __u64 max_memory_clock; > + uint32_t gpu_counter_freq; > + uint64_t max_engine_clock; > + uint64_t max_memory_clock; > /* cu information */ > - __u32 cu_active_number; > - __u32 cu_ao_mask; > - __u32 cu_bitmap[4][4]; > + uint32_t cu_active_number; > + uint32_t cu_ao_mask; > + uint32_t cu_bitmap[4][4]; > /** Render backend pipe mask. One render backend is CB+DB. */ > - __u32 enabled_rb_pipes_mask; > - __u32 num_rb_pipes; > - __u32 num_hw_gfx_contexts; > - __u32 _pad; > - __u64 ids_flags; > + uint32_t enabled_rb_pipes_mask; > + uint32_t num_rb_pipes; > + uint32_t num_hw_gfx_contexts; > + uint32_t _pad; > + uint64_t ids_flags; > /** Starting virtual address for UMDs. */ > - __u64 virtual_address_offset; > + uint64_t virtual_address_offset; > /** The maximum virtual address */ > - __u64 virtual_address_max; > + uint64_t virtual_address_max; > /** Required alignment of virtual addresses. */ > - __u32 virtual_address_alignment; > + uint32_t virtual_address_alignment; > /** Page table entry - fragment size */ > - __u32 pte_fragment_size; > - __u32 gart_page_size; > + uint32_t pte_fragment_size; > + uint32_t gart_page_size; > /** constant engine ram size*/ > - __u32 ce_ram_size; > + uint32_t ce_ram_size; > /** video memory type info*/ > - __u32 vram_type; > + uint32_t vram_type; > /** video memory bit width*/ > - __u32 vram_bit_width; > + uint32_t vram_bit_width; > /* vce harvesting instance */ > - __u32 vce_harvest_config; > + uint32_t vce_harvest_config; > }; > > struct drm_amdgpu_info_hw_ip { > /** Version of h/w IP */ > - __u32 hw_ip_version_major; > - __u32 hw_ip_version_minor; > + uint32_t hw_ip_version_major; > + uint32_t hw_ip_version_minor; > /** Capabilities */ > - __u64 capabilities_flags; > + uint64_t capabilities_flags; > /** command buffer address start alignment*/ > - __u32 ib_start_alignment; > + uint32_t ib_start_alignment; > /** command buffer size alignment*/ > - __u32 ib_size_alignment; > + uint32_t ib_size_alignment; > /** Bitmask of available rings. Bit 0 means ring 0, etc. */ > - __u32 available_rings; > - __u32 _pad; > + uint32_t available_rings; > + uint32_t _pad; > }; > > /*
On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian König wrote: > Am 19.08.2016 um 15:50 schrieb Marek Olšák: > >From: Marek Olšák <marek.olsak@amd.com> > > > >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. > > > >See the comment in the code. Basically, don't do cleanups in this header. > > > >Signed-off-by: Marek Olšák <marek.olsak@amd.com> > > I completely agree with you that this was a bad move, but I fear that we > will run into opposition with that. > > Adding Mikko Rapeli who made the reverted patch to comment. But this header is part of Linux kernel uapi. Remove it from there too then. -Mikko > Regards, > Christian. > > >--- > > include/uapi/drm/amdgpu_drm.h | 295 +++++++++++++++++++++--------------------- > > 1 file changed, 150 insertions(+), 145 deletions(-) > > > >diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h > >index 462246a..b39e07c 100644 > >--- a/include/uapi/drm/amdgpu_drm.h > >+++ b/include/uapi/drm/amdgpu_drm.h > >@@ -29,6 +29,11 @@ > > * Keith Whitwell <keith@tungstengraphics.com> > > */ > >+/* IT IS NOT ALLOWED TO CHANGE THIS HEADER WITHOUT DOING THE SAME IN LIBDRM !!! > >+ * THIS IS NOT A UAPI HEADER. IT IS ONLY A MIRROR OF ITS COUNTERPART IN LIBDRM. > >+ * USERSPACE SHOULD USE THE HEADERS FROM LIBDRM. NOT THIS ONE. > >+ */ > >+ > > #ifndef __AMDGPU_DRM_H__ > > #define __AMDGPU_DRM_H__ > >@@ -80,19 +85,19 @@ extern "C" { > > struct drm_amdgpu_gem_create_in { > > /** the requested memory size */ > >- __u64 bo_size; > >+ uint64_t bo_size; > > /** physical start_addr alignment in bytes for some HW requirements */ > >- __u64 alignment; > >+ uint64_t alignment; > > /** the requested memory domains */ > >- __u64 domains; > >+ uint64_t domains; > > /** allocation flags */ > >- __u64 domain_flags; > >+ uint64_t domain_flags; > > }; > > struct drm_amdgpu_gem_create_out { > > /** returned GEM object handle */ > >- __u32 handle; > >- __u32 _pad; > >+ uint32_t handle; > >+ uint32_t _pad; > > }; > > union drm_amdgpu_gem_create { > >@@ -109,28 +114,28 @@ union drm_amdgpu_gem_create { > > struct drm_amdgpu_bo_list_in { > > /** Type of operation */ > >- __u32 operation; > >+ uint32_t operation; > > /** Handle of list or 0 if we want to create one */ > >- __u32 list_handle; > >+ uint32_t list_handle; > > /** Number of BOs in list */ > >- __u32 bo_number; > >+ uint32_t bo_number; > > /** Size of each element describing BO */ > >- __u32 bo_info_size; > >+ uint32_t bo_info_size; > > /** Pointer to array describing BOs */ > >- __u64 bo_info_ptr; > >+ uint64_t bo_info_ptr; > > }; > > struct drm_amdgpu_bo_list_entry { > > /** Handle of BO */ > >- __u32 bo_handle; > >+ uint32_t bo_handle; > > /** New (if specified) BO priority to be used during migration */ > >- __u32 bo_priority; > >+ uint32_t bo_priority; > > }; > > struct drm_amdgpu_bo_list_out { > > /** Handle of resource list */ > >- __u32 list_handle; > >- __u32 _pad; > >+ uint32_t list_handle; > >+ uint32_t _pad; > > }; > > union drm_amdgpu_bo_list { > >@@ -154,26 +159,26 @@ union drm_amdgpu_bo_list { > > struct drm_amdgpu_ctx_in { > > /** AMDGPU_CTX_OP_* */ > >- __u32 op; > >+ uint32_t op; > > /** For future use, no flags defined so far */ > >- __u32 flags; > >- __u32 ctx_id; > >- __u32 _pad; > >+ uint32_t flags; > >+ uint32_t ctx_id; > >+ uint32_t _pad; > > }; > > union drm_amdgpu_ctx_out { > > struct { > >- __u32 ctx_id; > >- __u32 _pad; > >+ uint32_t ctx_id; > >+ uint32_t _pad; > > } alloc; > > struct { > > /** For future use, no flags defined so far */ > >- __u64 flags; > >+ uint64_t flags; > > /** Number of resets caused by this context so far. */ > >- __u32 hangs; > >+ uint32_t hangs; > > /** Reset status since the last call of the ioctl. */ > >- __u32 reset_status; > >+ uint32_t reset_status; > > } state; > > }; > >@@ -193,12 +198,12 @@ union drm_amdgpu_ctx { > > #define AMDGPU_GEM_USERPTR_REGISTER (1 << 3) > > struct drm_amdgpu_gem_userptr { > >- __u64 addr; > >- __u64 size; > >+ uint64_t addr; > >+ uint64_t size; > > /* AMDGPU_GEM_USERPTR_* */ > >- __u32 flags; > >+ uint32_t flags; > > /* Resulting GEM handle */ > >- __u32 handle; > >+ uint32_t handle; > > }; > > /* same meaning as the GB_TILE_MODE and GL_MACRO_TILE_MODE fields */ > >@@ -230,28 +235,28 @@ struct drm_amdgpu_gem_userptr { > > /** The same structure is shared for input/output */ > > struct drm_amdgpu_gem_metadata { > > /** GEM Object handle */ > >- __u32 handle; > >+ uint32_t handle; > > /** Do we want get or set metadata */ > >- __u32 op; > >+ uint32_t op; > > struct { > > /** For future use, no flags defined so far */ > >- __u64 flags; > >+ uint64_t flags; > > /** family specific tiling info */ > >- __u64 tiling_info; > >- __u32 data_size_bytes; > >- __u32 data[64]; > >+ uint64_t tiling_info; > >+ uint32_t data_size_bytes; > >+ uint32_t data[64]; > > } data; > > }; > > struct drm_amdgpu_gem_mmap_in { > > /** the GEM object handle */ > >- __u32 handle; > >- __u32 _pad; > >+ uint32_t handle; > >+ uint32_t _pad; > > }; > > struct drm_amdgpu_gem_mmap_out { > > /** mmap offset from the vma offset manager */ > >- __u64 addr_ptr; > >+ uint64_t addr_ptr; > > }; > > union drm_amdgpu_gem_mmap { > >@@ -261,18 +266,18 @@ union drm_amdgpu_gem_mmap { > > struct drm_amdgpu_gem_wait_idle_in { > > /** GEM object handle */ > >- __u32 handle; > >+ uint32_t handle; > > /** For future use, no flags defined so far */ > >- __u32 flags; > >+ uint32_t flags; > > /** Absolute timeout to wait */ > >- __u64 timeout; > >+ uint64_t timeout; > > }; > > struct drm_amdgpu_gem_wait_idle_out { > > /** BO status: 0 - BO is idle, 1 - BO is busy */ > >- __u32 status; > >+ uint32_t status; > > /** Returned current memory domain */ > >- __u32 domain; > >+ uint32_t domain; > > }; > > union drm_amdgpu_gem_wait_idle { > >@@ -282,18 +287,18 @@ union drm_amdgpu_gem_wait_idle { > > struct drm_amdgpu_wait_cs_in { > > /** Command submission handle */ > >- __u64 handle; > >+ uint64_t handle; > > /** Absolute timeout to wait */ > >- __u64 timeout; > >- __u32 ip_type; > >- __u32 ip_instance; > >- __u32 ring; > >- __u32 ctx_id; > >+ uint64_t timeout; > >+ uint32_t ip_type; > >+ uint32_t ip_instance; > >+ uint32_t ring; > >+ uint32_t ctx_id; > > }; > > struct drm_amdgpu_wait_cs_out { > > /** CS status: 0 - CS completed, 1 - CS still busy */ > >- __u64 status; > >+ uint64_t status; > > }; > > union drm_amdgpu_wait_cs { > >@@ -307,11 +312,11 @@ union drm_amdgpu_wait_cs { > > /* Sets or returns a value associated with a buffer. */ > > struct drm_amdgpu_gem_op { > > /** GEM object handle */ > >- __u32 handle; > >+ uint32_t handle; > > /** AMDGPU_GEM_OP_* */ > >- __u32 op; > >+ uint32_t op; > > /** Input or return value */ > >- __u64 value; > >+ uint64_t value; > > }; > > #define AMDGPU_VA_OP_MAP 1 > >@@ -330,18 +335,18 @@ struct drm_amdgpu_gem_op { > > struct drm_amdgpu_gem_va { > > /** GEM object handle */ > >- __u32 handle; > >- __u32 _pad; > >+ uint32_t handle; > >+ uint32_t _pad; > > /** AMDGPU_VA_OP_* */ > >- __u32 operation; > >+ uint32_t operation; > > /** AMDGPU_VM_PAGE_* */ > >- __u32 flags; > >+ uint32_t flags; > > /** va address to assign . Must be correctly aligned.*/ > >- __u64 va_address; > >+ uint64_t va_address; > > /** Specify offset inside of BO to assign. Must be correctly aligned.*/ > >- __u64 offset_in_bo; > >+ uint64_t offset_in_bo; > > /** Specify mapping size. Must be correctly aligned. */ > >- __u64 map_size; > >+ uint64_t map_size; > > }; > > #define AMDGPU_HW_IP_GFX 0 > >@@ -358,24 +363,24 @@ struct drm_amdgpu_gem_va { > > #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03 > > struct drm_amdgpu_cs_chunk { > >- __u32 chunk_id; > >- __u32 length_dw; > >- __u64 chunk_data; > >+ uint32_t chunk_id; > >+ uint32_t length_dw; > >+ uint64_t chunk_data; > > }; > > struct drm_amdgpu_cs_in { > > /** Rendering context id */ > >- __u32 ctx_id; > >+ uint32_t ctx_id; > > /** Handle of resource list associated with CS */ > >- __u32 bo_list_handle; > >- __u32 num_chunks; > >- __u32 _pad; > >- /** this points to __u64 * which point to cs chunks */ > >- __u64 chunks; > >+ uint32_t bo_list_handle; > >+ uint32_t num_chunks; > >+ uint32_t _pad; > >+ /** this points to uint64_t * which point to cs chunks */ > >+ uint64_t chunks; > > }; > > struct drm_amdgpu_cs_out { > >- __u64 handle; > >+ uint64_t handle; > > }; > > union drm_amdgpu_cs { > >@@ -392,32 +397,32 @@ union drm_amdgpu_cs { > > #define AMDGPU_IB_FLAG_PREAMBLE (1<<1) > > struct drm_amdgpu_cs_chunk_ib { > >- __u32 _pad; > >+ uint32_t _pad; > > /** AMDGPU_IB_FLAG_* */ > >- __u32 flags; > >+ uint32_t flags; > > /** Virtual address to begin IB execution */ > >- __u64 va_start; > >+ uint64_t va_start; > > /** Size of submission */ > >- __u32 ib_bytes; > >+ uint32_t ib_bytes; > > /** HW IP to submit to */ > >- __u32 ip_type; > >+ uint32_t ip_type; > > /** HW IP index of the same type to submit to */ > >- __u32 ip_instance; > >+ uint32_t ip_instance; > > /** Ring index to submit to */ > >- __u32 ring; > >+ uint32_t ring; > > }; > > struct drm_amdgpu_cs_chunk_dep { > >- __u32 ip_type; > >- __u32 ip_instance; > >- __u32 ring; > >- __u32 ctx_id; > >- __u64 handle; > >+ uint32_t ip_type; > >+ uint32_t ip_instance; > >+ uint32_t ring; > >+ uint32_t ctx_id; > >+ uint64_t handle; > > }; > > struct drm_amdgpu_cs_chunk_fence { > >- __u32 handle; > >- __u32 offset; > >+ uint32_t handle; > >+ uint32_t offset; > > }; > > struct drm_amdgpu_cs_chunk_data { > >@@ -489,53 +494,53 @@ struct drm_amdgpu_cs_chunk_data { > > struct drm_amdgpu_query_fw { > > /** AMDGPU_INFO_FW_* */ > >- __u32 fw_type; > >+ uint32_t fw_type; > > /** > > * Index of the IP if there are more IPs of > > * the same type. > > */ > >- __u32 ip_instance; > >+ uint32_t ip_instance; > > /** > > * Index of the engine. Whether this is used depends > > * on the firmware type. (e.g. MEC, SDMA) > > */ > >- __u32 index; > >- __u32 _pad; > >+ uint32_t index; > >+ uint32_t _pad; > > }; > > /* Input structure for the INFO ioctl */ > > struct drm_amdgpu_info { > > /* Where the return value will be stored */ > >- __u64 return_pointer; > >+ uint64_t return_pointer; > > /* The size of the return value. Just like "size" in "snprintf", > > * it limits how many bytes the kernel can write. */ > >- __u32 return_size; > >+ uint32_t return_size; > > /* The query request id. */ > >- __u32 query; > >+ uint32_t query; > > union { > > struct { > >- __u32 id; > >- __u32 _pad; > >+ uint32_t id; > >+ uint32_t _pad; > > } mode_crtc; > > struct { > > /** AMDGPU_HW_IP_* */ > >- __u32 type; > >+ uint32_t type; > > /** > > * Index of the IP if there are more IPs of the same > > * type. Ignored by AMDGPU_INFO_HW_IP_COUNT. > > */ > >- __u32 ip_instance; > >+ uint32_t ip_instance; > > } query_hw_ip; > > struct { > >- __u32 dword_offset; > >+ uint32_t dword_offset; > > /** number of registers to read */ > >- __u32 count; > >- __u32 instance; > >+ uint32_t count; > >+ uint32_t instance; > > /** For future use, no flags defined so far */ > >- __u32 flags; > >+ uint32_t flags; > > } read_mmr_reg; > > struct drm_amdgpu_query_fw query_fw; > >@@ -544,31 +549,31 @@ struct drm_amdgpu_info { > > struct drm_amdgpu_info_gds { > > /** GDS GFX partition size */ > >- __u32 gds_gfx_partition_size; > >+ uint32_t gds_gfx_partition_size; > > /** GDS compute partition size */ > >- __u32 compute_partition_size; > >+ uint32_t compute_partition_size; > > /** total GDS memory size */ > >- __u32 gds_total_size; > >+ uint32_t gds_total_size; > > /** GWS size per GFX partition */ > >- __u32 gws_per_gfx_partition; > >+ uint32_t gws_per_gfx_partition; > > /** GSW size per compute partition */ > >- __u32 gws_per_compute_partition; > >+ uint32_t gws_per_compute_partition; > > /** OA size per GFX partition */ > >- __u32 oa_per_gfx_partition; > >+ uint32_t oa_per_gfx_partition; > > /** OA size per compute partition */ > >- __u32 oa_per_compute_partition; > >- __u32 _pad; > >+ uint32_t oa_per_compute_partition; > >+ uint32_t _pad; > > }; > > struct drm_amdgpu_info_vram_gtt { > >- __u64 vram_size; > >- __u64 vram_cpu_accessible_size; > >- __u64 gtt_size; > >+ uint64_t vram_size; > >+ uint64_t vram_cpu_accessible_size; > >+ uint64_t gtt_size; > > }; > > struct drm_amdgpu_info_firmware { > >- __u32 ver; > >- __u32 feature; > >+ uint32_t ver; > >+ uint32_t feature; > > }; > > #define AMDGPU_VRAM_TYPE_UNKNOWN 0 > >@@ -582,61 +587,61 @@ struct drm_amdgpu_info_firmware { > > struct drm_amdgpu_info_device { > > /** PCI Device ID */ > >- __u32 device_id; > >+ uint32_t device_id; > > /** Internal chip revision: A0, A1, etc.) */ > >- __u32 chip_rev; > >- __u32 external_rev; > >+ uint32_t chip_rev; > >+ uint32_t external_rev; > > /** Revision id in PCI Config space */ > >- __u32 pci_rev; > >- __u32 family; > >- __u32 num_shader_engines; > >- __u32 num_shader_arrays_per_engine; > >+ uint32_t pci_rev; > >+ uint32_t family; > >+ uint32_t num_shader_engines; > >+ uint32_t num_shader_arrays_per_engine; > > /* in KHz */ > >- __u32 gpu_counter_freq; > >- __u64 max_engine_clock; > >- __u64 max_memory_clock; > >+ uint32_t gpu_counter_freq; > >+ uint64_t max_engine_clock; > >+ uint64_t max_memory_clock; > > /* cu information */ > >- __u32 cu_active_number; > >- __u32 cu_ao_mask; > >- __u32 cu_bitmap[4][4]; > >+ uint32_t cu_active_number; > >+ uint32_t cu_ao_mask; > >+ uint32_t cu_bitmap[4][4]; > > /** Render backend pipe mask. One render backend is CB+DB. */ > >- __u32 enabled_rb_pipes_mask; > >- __u32 num_rb_pipes; > >- __u32 num_hw_gfx_contexts; > >- __u32 _pad; > >- __u64 ids_flags; > >+ uint32_t enabled_rb_pipes_mask; > >+ uint32_t num_rb_pipes; > >+ uint32_t num_hw_gfx_contexts; > >+ uint32_t _pad; > >+ uint64_t ids_flags; > > /** Starting virtual address for UMDs. */ > >- __u64 virtual_address_offset; > >+ uint64_t virtual_address_offset; > > /** The maximum virtual address */ > >- __u64 virtual_address_max; > >+ uint64_t virtual_address_max; > > /** Required alignment of virtual addresses. */ > >- __u32 virtual_address_alignment; > >+ uint32_t virtual_address_alignment; > > /** Page table entry - fragment size */ > >- __u32 pte_fragment_size; > >- __u32 gart_page_size; > >+ uint32_t pte_fragment_size; > >+ uint32_t gart_page_size; > > /** constant engine ram size*/ > >- __u32 ce_ram_size; > >+ uint32_t ce_ram_size; > > /** video memory type info*/ > >- __u32 vram_type; > >+ uint32_t vram_type; > > /** video memory bit width*/ > >- __u32 vram_bit_width; > >+ uint32_t vram_bit_width; > > /* vce harvesting instance */ > >- __u32 vce_harvest_config; > >+ uint32_t vce_harvest_config; > > }; > > struct drm_amdgpu_info_hw_ip { > > /** Version of h/w IP */ > >- __u32 hw_ip_version_major; > >- __u32 hw_ip_version_minor; > >+ uint32_t hw_ip_version_major; > >+ uint32_t hw_ip_version_minor; > > /** Capabilities */ > >- __u64 capabilities_flags; > >+ uint64_t capabilities_flags; > > /** command buffer address start alignment*/ > >- __u32 ib_start_alignment; > >+ uint32_t ib_start_alignment; > > /** command buffer size alignment*/ > >- __u32 ib_size_alignment; > >+ uint32_t ib_size_alignment; > > /** Bitmask of available rings. Bit 0 means ring 0, etc. */ > >- __u32 available_rings; > >- __u32 _pad; > >+ uint32_t available_rings; > >+ uint32_t _pad; > > }; > > /* > >
On Fri, Aug 19, 2016 at 4:52 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: > On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian König wrote: >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >> >From: Marek Olšák <marek.olsak@amd.com> >> > >> >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >> > >> >See the comment in the code. Basically, don't do cleanups in this header. >> > >> >Signed-off-by: Marek Olšák <marek.olsak@amd.com> >> >> I completely agree with you that this was a bad move, but I fear that we >> will run into opposition with that. >> >> Adding Mikko Rapeli who made the reverted patch to comment. > > But this header is part of Linux kernel uapi. Remove it from there too then. That's a good idea, but it really is a uapi header in the sense that it defines the kernel driver interface for a specific kernel version. However, it is not a header that the userspace stack should include, because userspace should get it from libdrm. (it makes userspace more independent from the currently running kernel) Marek
On Fri, Aug 19, 2016 at 5:22 PM, Marek Olšák <maraeo@gmail.com> wrote: > On Fri, Aug 19, 2016 at 4:52 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: >> On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian König wrote: >>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>> >From: Marek Olšák <marek.olsak@amd.com> >>> > >>> >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>> > >>> >See the comment in the code. Basically, don't do cleanups in this header. >>> > >>> >Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>> >>> I completely agree with you that this was a bad move, but I fear that we >>> will run into opposition with that. >>> >>> Adding Mikko Rapeli who made the reverted patch to comment. >> >> But this header is part of Linux kernel uapi. Remove it from there too then. > > That's a good idea, but it really is a uapi header in the sense that > it defines the kernel driver interface for a specific kernel version. > However, it is not a header that the userspace stack should include, > because userspace should get it from libdrm. (it makes userspace more > independent from the currently running kernel) Please don't remove it from the kernel side if it's included in libdrm. Emil&I just spent a bit of time making sure those two sets of headers match when we produce them using the make headers_install target. drm is indeed special, as in our headers aren't shipped with the kernel-headers package but libdrm. But otherwise they are supposed to work exactly like any other uapi headers. No need to move them out of the uapi headers - I'd even say that would be bad since it removes the visibility and clear marker that this header must be considered uapi! Also the very loud comment at the top is definitely misleading, there's nothing that guarnatees that the libdrm and kernel copy are in sync when you build or run userspace. Also maybe for context, but what exactly is the problem with the __ types? Adding Emil. -Daniel
On Fri, Aug 19, 2016 at 7:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Aug 19, 2016 at 5:22 PM, Marek Olšák <maraeo@gmail.com> wrote: >> On Fri, Aug 19, 2016 at 4:52 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: >>> On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian König wrote: >>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>> >From: Marek Olšák <marek.olsak@amd.com> >>>> > >>>> >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>> > >>>> >See the comment in the code. Basically, don't do cleanups in this header. >>>> > >>>> >Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>> >>>> I completely agree with you that this was a bad move, but I fear that we >>>> will run into opposition with that. >>>> >>>> Adding Mikko Rapeli who made the reverted patch to comment. >>> >>> But this header is part of Linux kernel uapi. Remove it from there too then. >> >> That's a good idea, but it really is a uapi header in the sense that >> it defines the kernel driver interface for a specific kernel version. >> However, it is not a header that the userspace stack should include, >> because userspace should get it from libdrm. (it makes userspace more >> independent from the currently running kernel) > > Please don't remove it from the kernel side if it's included in > libdrm. Emil&I just spent a bit of time making sure those two sets of > headers match when we produce them using the make headers_install > target. > > drm is indeed special, as in our headers aren't shipped with the > kernel-headers package but libdrm. But otherwise they are supposed to > work exactly like any other uapi headers. No need to move them out of > the uapi headers - I'd even say that would be bad since it removes the > visibility and clear marker that this header must be considered uapi! > > Also the very loud comment at the top is definitely misleading, > there's nothing that guarnatees that the libdrm and kernel copy are in > sync when you build or run userspace. Also maybe for context, but what > exactly is the problem with the __ types? > > Adding Emil. Adding Emil for real ... -Daniel
On Fri, Aug 19, 2016 at 7:12 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Aug 19, 2016 at 7:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Fri, Aug 19, 2016 at 5:22 PM, Marek Olšák <maraeo@gmail.com> wrote: >>> On Fri, Aug 19, 2016 at 4:52 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: >>>> On Fri, Aug 19, 2016 at 04:26:40PM +0200, Christian König wrote: >>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>> >From: Marek Olšák <marek.olsak@amd.com> >>>>> > >>>>> >This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>> > >>>>> >See the comment in the code. Basically, don't do cleanups in this header. >>>>> > >>>>> >Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>>> >>>>> I completely agree with you that this was a bad move, but I fear that we >>>>> will run into opposition with that. >>>>> >>>>> Adding Mikko Rapeli who made the reverted patch to comment. >>>> >>>> But this header is part of Linux kernel uapi. Remove it from there too then. >>> >>> That's a good idea, but it really is a uapi header in the sense that >>> it defines the kernel driver interface for a specific kernel version. >>> However, it is not a header that the userspace stack should include, >>> because userspace should get it from libdrm. (it makes userspace more >>> independent from the currently running kernel) >> >> Please don't remove it from the kernel side if it's included in >> libdrm. Emil&I just spent a bit of time making sure those two sets of >> headers match when we produce them using the make headers_install >> target. >> >> drm is indeed special, as in our headers aren't shipped with the >> kernel-headers package but libdrm. But otherwise they are supposed to >> work exactly like any other uapi headers. No need to move them out of >> the uapi headers - I'd even say that would be bad since it removes the >> visibility and clear marker that this header must be considered uapi! >> >> Also the very loud comment at the top is definitely misleading, >> there's nothing that guarnatees that the libdrm and kernel copy are in >> sync when you build or run userspace. Also maybe for context, but what >> exactly is the problem with the __ types? >> >> Adding Emil. > > Adding Emil for real ... My understanding is that the problem was that userspace had to include stdint.h before including these uapi headers. That's not a problem for 2 reasons: 1) Userspace doesn't include these headers, but includes the libdrm headers instead. 2) The few userspace drivers and tools that include the libdrm headers can also include stdint.h. User-friendliness isn't required here. Marek
On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: > Am 19.08.2016 um 15:50 schrieb Marek Olšák: >> >> From: Marek Olšák <marek.olsak@amd.com> >> >> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >> >> See the comment in the code. Basically, don't do cleanups in this header. >> >> Signed-off-by: Marek Olšák <marek.olsak@amd.com> > > > I completely agree with you that this was a bad move, but I fear that we > will run into opposition with that. > Please check the facts before introducing RATHER ANNOYING AND HARD TO READ COMMENT IN ALL CAPS. Story time: I was dreaming of a day were we can stop installing these headers, thus making deprecation a bit easier process. Yet after failing to convince Dave and Daniel on a number of occasions I've accepted that those headers _are_ here to stay. And yes they _are_ the UAPI, even though no applications are meant to use them but the libdrm 'version'. Thus any changes to the libdrm ones should be a mirror of the ones here and libdrm should _not_ differ. But let's ignore all that and imagine that those headers are _not_ UAPI. That gives us even greater reason to _not_ use the uintx_t types but the kernel __uX ones. The series that did these changes had a fair few references why we want that. Yes, I can imagine that the situation isn't ideal, and/or not that clear. Then again a check with git log should have straightened things out. If not _please_ help us improve this (documentation and/or others). And last but not least, please share with up what inspired this - (build/runtime) regression, attempted sync with libdrm, other ? Thanks Emil
On Fri, Aug 19, 2016 at 6:54 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>> >>> From: Marek Olšák <marek.olsak@amd.com> >>> >>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>> >>> See the comment in the code. Basically, don't do cleanups in this header. >>> >>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >> >> >> I completely agree with you that this was a bad move, but I fear that we >> will run into opposition with that. >> > Please check the facts before introducing RATHER ANNOYING AND HARD TO > READ COMMENT IN ALL CAPS. > > Story time: > I was dreaming of a day were we can stop installing these headers, > thus making deprecation a bit easier process. > Yet after failing to convince Dave and Daniel on a number of occasions > I've accepted that those headers _are_ here to stay. And yes they > _are_ the UAPI, even though no applications are meant to use them but > the libdrm 'version'. > Thus any changes to the libdrm ones should be a mirror of the ones > here and libdrm should _not_ differ. > > But let's ignore all that and imagine that those headers are _not_ > UAPI. That gives us even greater reason to _not_ use the uintx_t types > but the kernel __uX ones. The series that did these changes had a fair > few references why we want that. tbh, I'm all in favor of making it easier to sync kernel headers to libdrm, etc. But kernel *does* have stdint types. Just (for some reason that completely baffles me) not in stdint.h so we can't include that from the uapi headers themselves. I'm not a huge fan of the uX/sX types when the rest of the world has moved on to stdint. I can *kinda* (barely) understand the argument for kernel using uX/sX in uapi (to avoid other userspace src files from needing to #include stdint.h first). But I think that is a bad argument (kernel should fix it's shit to be compatible with stdint.h, not other way around), and it doesn't even apply for drm uapi (where the consumer of the uapi is just libdrm_$drivername, not random other consumers) so we can take care to #include stdint.h first.. tl;dr: I wasn't a big fan of the conversion to uX/sX types.. I kinda see the argument in the general case (but I think it is bogus, and even if it was legit it doesn't apply to drm uapi) </rant> BR, -R > Yes, I can imagine that the situation isn't ideal, and/or not that > clear. Then again a check with git log should have straightened things > out. > If not _please_ help us improve this (documentation and/or others). > > > And last but not least, please share with up what inspired this - > (build/runtime) regression, attempted sync with libdrm, other ? > > Thanks > Emil > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Aug 19, 2016 at 6:32 PM, Rob Clark <robdclark@gmail.com> wrote: > tbh, I'm all in favor of making it easier to sync kernel headers to > libdrm, etc. > > But kernel *does* have stdint types. Just (for some reason that > completely baffles me) not in stdint.h so we can't include that from > the uapi headers themselves. I'm not a huge fan of the uX/sX types > when the rest of the world has moved on to stdint. I can *kinda* > (barely) understand the argument for kernel using uX/sX in uapi (to > avoid other userspace src files from needing to #include stdint.h > first). But I think that is a bad argument (kernel should fix it's > shit to be compatible with stdint.h, not other way around), and it > doesn't even apply for drm uapi (where the consumer of the uapi is > just libdrm_$drivername, not random other consumers) so we can take > care to #include stdint.h first.. > > tl;dr: I wasn't a big fan of the conversion to uX/sX types.. I kinda > see the argument in the general case (but I think it is bogus, and > even if it was legit it doesn't apply to drm uapi) > > </rant> > > BR, > -R > I believe that this patch is to simplify porting requirements by reducing system specific data types whenever possible. The C99 capable data types defined in stdbool.h, stdint.h and inttypes.h are supported by modern compilers. This means that anyone who uses gcc, clang, and Visual Studio 2013 ( or newer ) can compile these include files without any major hassles.
On Fri, Aug 19, 2016 at 9:23 PM, Ken Phillis Jr <kphillisjr@gmail.com> wrote: > On Fri, Aug 19, 2016 at 6:32 PM, Rob Clark <robdclark@gmail.com> wrote: >> >> tbh, I'm all in favor of making it easier to sync kernel headers to >> libdrm, etc. >> >> But kernel *does* have stdint types. Just (for some reason that >> completely baffles me) not in stdint.h so we can't include that from >> the uapi headers themselves. I'm not a huge fan of the uX/sX types >> when the rest of the world has moved on to stdint. I can *kinda* >> (barely) understand the argument for kernel using uX/sX in uapi (to >> avoid other userspace src files from needing to #include stdint.h >> first). But I think that is a bad argument (kernel should fix it's >> shit to be compatible with stdint.h, not other way around), and it >> doesn't even apply for drm uapi (where the consumer of the uapi is >> just libdrm_$drivername, not random other consumers) so we can take >> care to #include stdint.h first.. >> >> tl;dr: I wasn't a big fan of the conversion to uX/sX types.. I kinda >> see the argument in the general case (but I think it is bogus, and >> even if it was legit it doesn't apply to drm uapi) >> >> </rant> >> >> BR, >> -R > > > I believe that this patch is to simplify porting requirements by reducing > system > specific data types whenever possible. The C99 capable data types defined in > stdbool.h, stdint.h and inttypes.h are supported by modern compilers. This > means that anyone who uses gcc, clang, and Visual Studio 2013 ( or newer ) > can compile these include files without any major hassles. > perhaps, but if the target audience for driver specific APIs is libdrm/mesa, which already uses stdint types, then I fail to see the point.. It is a bit difference scenario for some of the core kms ioctls which are not driver specific.. but most of the driver specific gpu related ioctls tend to be rather complex and not intended for use by something other than libdrm/mesa. BR, -R
On Fri, Aug 19, 2016 at 8:46 PM, Rob Clark <robdclark@gmail.com> wrote: > perhaps, but if the target audience for driver specific APIs is > libdrm/mesa, which already uses stdint types, then I fail to see the > point.. > > It is a bit difference scenario for some of the core kms ioctls which > are not driver specific.. but most of the driver specific gpu related > ioctls tend to be rather complex and not intended for use by something > other than libdrm/mesa. > > BR, > -R > I personally do not see the reason to break user space API in the first place. I know for a fact that the include file ( linux/types.h ) mentioned by the patch supports C99 integer data types since Linux Kernel Version 2.2.0. This makes the patch that got reverted completely meaningless since the only reason to make this change in the first place would be if the developer intends on backporting the driver to kernels older than version 2.2.0. Also the only integer types that could pose problems is if __STRICT_ANSI__ is defined because this would cause uint64_t, and int64_t to not be defined.
On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>> >>> From: Marek Olšák <marek.olsak@amd.com> >>> >>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>> >>> See the comment in the code. Basically, don't do cleanups in this header. >>> >>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >> >> >> I completely agree with you that this was a bad move, but I fear that we >> will run into opposition with that. >> > Please check the facts before introducing RATHER ANNOYING AND HARD TO > READ COMMENT IN ALL CAPS. > > Story time: > I was dreaming of a day were we can stop installing these headers, > thus making deprecation a bit easier process. > Yet after failing to convince Dave and Daniel on a number of occasions > I've accepted that those headers _are_ here to stay. And yes they > _are_ the UAPI, even though no applications are meant to use them but > the libdrm 'version'. > Thus any changes to the libdrm ones should be a mirror of the ones > here and libdrm should _not_ differ. > > But let's ignore all that and imagine that those headers are _not_ > UAPI. That gives us even greater reason to _not_ use the uintx_t types > but the kernel __uX ones. The series that did these changes had a fair > few references why we want that. > > Yes, I can imagine that the situation isn't ideal, and/or not that > clear. Then again a check with git log should have straightened things > out. > If not _please_ help us improve this (documentation and/or others). > > > And last but not least, please share with up what inspired this - > (build/runtime) regression, attempted sync with libdrm, other ? Syncing with libdrm became difficult. I'd like the diff between kernel and libdrm to be as small as possible. We must take into account that the uapi headers can potentially be implemented by a different OS. That's why they are in libdrm and that's why nobody should make random changes to them in the kernel tree. Do not think like a kernel developer isolated in Linux and just consider the broader use case. If you do, you'll realize that it simply doesn't make sense to use the __uX types here. Marek
On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: > On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>> >>>> From: Marek Olšák <marek.olsak@amd.com> >>>> >>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>> >>>> See the comment in the code. Basically, don't do cleanups in this header. >>>> >>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>> >>> >>> I completely agree with you that this was a bad move, but I fear that we >>> will run into opposition with that. >>> >> Please check the facts before introducing RATHER ANNOYING AND HARD TO >> READ COMMENT IN ALL CAPS. >> >> Story time: >> I was dreaming of a day were we can stop installing these headers, >> thus making deprecation a bit easier process. >> Yet after failing to convince Dave and Daniel on a number of occasions >> I've accepted that those headers _are_ here to stay. And yes they >> _are_ the UAPI, even though no applications are meant to use them but >> the libdrm 'version'. >> Thus any changes to the libdrm ones should be a mirror of the ones >> here and libdrm should _not_ differ. >> >> But let's ignore all that and imagine that those headers are _not_ >> UAPI. That gives us even greater reason to _not_ use the uintx_t types >> but the kernel __uX ones. The series that did these changes had a fair >> few references why we want that. >> >> Yes, I can imagine that the situation isn't ideal, and/or not that >> clear. Then again a check with git log should have straightened things >> out. >> If not _please_ help us improve this (documentation and/or others). >> >> >> And last but not least, please share with up what inspired this - >> (build/runtime) regression, attempted sync with libdrm, other ? > > Syncing with libdrm became difficult. Actually it should be easier now. Perhaps the radeon one was always a good citizen, but sadly that was not the case for the rest. > I'd like the diff between kernel > and libdrm to be as small as possible. > I believe we all agree on this one :-) > We must take into account that the uapi headers can potentially be > implemented by a different OS. Agreed. Have you looked at the 'compatibility layer' in drm.h ? > That's why they are in libdrm and > that's why nobody should make random changes to them in the kernel > tree. Do not think like a kernel developer isolated in Linux and just > consider the broader use case. If you do, you'll realize that it > simply doesn't make sense to use the __uX types here. > Ftr, like Rob (and maybe others) I believe that using __uX (in the kernel) is a bit odd, and opting for the stdint.h types should happen. But until/if that happens we have to live with the __uX ones. That said, I have poked various BSD people on a number of occasions, (hopefully) inspiring them to upstream their changes in a compatible way. Thus the whole "don't think like a kernel developer" doesn't really apply here :-\ I'm simply one of the few fools^wpeople trying to make things OK for most (since one can never please everyone, all the time). IIRC the FreeBSD/DragonFly people had some issues with their compatibility layer since the kernel and userspace drm.h were divergent "by design" [1]. To make it even 'better' there's even two difference versions of drm.h in their kernel itself [2]. What I am for is a discussion how to resolve things. Although expect resistance if you're thinking about applying tape, in order to fix somethings that's 'broken' elsewhere. If you or any !Linux folks are around on XDC we should really sit down and untangle some/all of these issues. Thanks Emil [1] https://lists.freedesktop.org/archives/dri-devel/2016-May/107656.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-May/107726.html
On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: >> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>> >>>>> From: Marek Olšák <marek.olsak@amd.com> >>>>> >>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>> >>>>> See the comment in the code. Basically, don't do cleanups in this header. >>>>> >>>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>> >>>> >>>> I completely agree with you that this was a bad move, but I fear that we >>>> will run into opposition with that. >>>> >>> Please check the facts before introducing RATHER ANNOYING AND HARD TO >>> READ COMMENT IN ALL CAPS. >>> >>> Story time: >>> I was dreaming of a day were we can stop installing these headers, >>> thus making deprecation a bit easier process. >>> Yet after failing to convince Dave and Daniel on a number of occasions >>> I've accepted that those headers _are_ here to stay. And yes they >>> _are_ the UAPI, even though no applications are meant to use them but >>> the libdrm 'version'. >>> Thus any changes to the libdrm ones should be a mirror of the ones >>> here and libdrm should _not_ differ. >>> >>> But let's ignore all that and imagine that those headers are _not_ >>> UAPI. That gives us even greater reason to _not_ use the uintx_t types >>> but the kernel __uX ones. The series that did these changes had a fair >>> few references why we want that. >>> >>> Yes, I can imagine that the situation isn't ideal, and/or not that >>> clear. Then again a check with git log should have straightened things >>> out. >>> If not _please_ help us improve this (documentation and/or others). >>> >>> >>> And last but not least, please share with up what inspired this - >>> (build/runtime) regression, attempted sync with libdrm, other ? >> >> Syncing with libdrm became difficult. > Actually it should be easier now. Perhaps the radeon one was always a > good citizen, but sadly that was not the case for the rest. > >> I'd like the diff between kernel >> and libdrm to be as small as possible. >> > I believe we all agree on this one :-) > >> We must take into account that the uapi headers can potentially be >> implemented by a different OS. > Agreed. Have you looked at the 'compatibility layer' in drm.h ? > >> That's why they are in libdrm and >> that's why nobody should make random changes to them in the kernel >> tree. Do not think like a kernel developer isolated in Linux and just >> consider the broader use case. If you do, you'll realize that it >> simply doesn't make sense to use the __uX types here. >> > Ftr, like Rob (and maybe others) I believe that using __uX (in the > kernel) is a bit odd, and opting for the stdint.h types should happen. > But until/if that happens we have to live with the __uX ones. > > That said, I have poked various BSD people on a number of occasions, > (hopefully) inspiring them to upstream their changes in a compatible > way. Thus the whole "don't think like a kernel developer" doesn't > really apply here :-\ > > I'm simply one of the few fools^wpeople trying to make things OK for > most (since one can never please everyone, all the time). > > IIRC the FreeBSD/DragonFly people had some issues with their > compatibility layer since the kernel and userspace drm.h were > divergent "by design" [1]. To make it even 'better' there's even two > difference versions of drm.h in their kernel itself [2]. > > What I am for is a discussion how to resolve things. Although expect > resistance if you're thinking about applying tape, in order to fix > somethings that's 'broken' elsewhere. > > If you or any !Linux folks are around on XDC we should really sit down > and untangle some/all of these issues. It's not 100% certain but it looks like we won't be there. We need the uapi headers to be the same as libdrm ones to make syncing easier. There is not much else to discuss here really. We (AMD) are also the ones who have to work with these headers the most, not you, not Mikko. While I understand some people want to discuss this further, these patches must land first in order bring back the compatibility with libdrm. After that, we can discuss the possible solutions and everybody interested in a better solution *that will take libdrm into account* can join. For now, I have to expect that those discussions might also lead nowhere and I don't wanna be stuck with bad uapi headers in the kernel forever. Marek
On 20 August 2016 at 12:47, Marek Olšák <maraeo@gmail.com> wrote: > On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: >>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>>> >>>>>> From: Marek Olšák <marek.olsak@amd.com> >>>>>> >>>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>>> >>>>>> See the comment in the code. Basically, don't do cleanups in this header. >>>>>> >>>>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>>> >>>>> >>>>> I completely agree with you that this was a bad move, but I fear that we >>>>> will run into opposition with that. >>>>> >>>> Please check the facts before introducing RATHER ANNOYING AND HARD TO >>>> READ COMMENT IN ALL CAPS. >>>> >>>> Story time: >>>> I was dreaming of a day were we can stop installing these headers, >>>> thus making deprecation a bit easier process. >>>> Yet after failing to convince Dave and Daniel on a number of occasions >>>> I've accepted that those headers _are_ here to stay. And yes they >>>> _are_ the UAPI, even though no applications are meant to use them but >>>> the libdrm 'version'. >>>> Thus any changes to the libdrm ones should be a mirror of the ones >>>> here and libdrm should _not_ differ. >>>> >>>> But let's ignore all that and imagine that those headers are _not_ >>>> UAPI. That gives us even greater reason to _not_ use the uintx_t types >>>> but the kernel __uX ones. The series that did these changes had a fair >>>> few references why we want that. >>>> >>>> Yes, I can imagine that the situation isn't ideal, and/or not that >>>> clear. Then again a check with git log should have straightened things >>>> out. >>>> If not _please_ help us improve this (documentation and/or others). >>>> >>>> >>>> And last but not least, please share with up what inspired this - >>>> (build/runtime) regression, attempted sync with libdrm, other ? >>> >>> Syncing with libdrm became difficult. >> Actually it should be easier now. Perhaps the radeon one was always a >> good citizen, but sadly that was not the case for the rest. >> >>> I'd like the diff between kernel >>> and libdrm to be as small as possible. >>> >> I believe we all agree on this one :-) >> >>> We must take into account that the uapi headers can potentially be >>> implemented by a different OS. >> Agreed. Have you looked at the 'compatibility layer' in drm.h ? >> >>> That's why they are in libdrm and >>> that's why nobody should make random changes to them in the kernel >>> tree. Do not think like a kernel developer isolated in Linux and just >>> consider the broader use case. If you do, you'll realize that it >>> simply doesn't make sense to use the __uX types here. >>> >> Ftr, like Rob (and maybe others) I believe that using __uX (in the >> kernel) is a bit odd, and opting for the stdint.h types should happen. >> But until/if that happens we have to live with the __uX ones. >> >> That said, I have poked various BSD people on a number of occasions, >> (hopefully) inspiring them to upstream their changes in a compatible >> way. Thus the whole "don't think like a kernel developer" doesn't >> really apply here :-\ >> >> I'm simply one of the few fools^wpeople trying to make things OK for >> most (since one can never please everyone, all the time). >> >> IIRC the FreeBSD/DragonFly people had some issues with their >> compatibility layer since the kernel and userspace drm.h were >> divergent "by design" [1]. To make it even 'better' there's even two >> difference versions of drm.h in their kernel itself [2]. >> >> What I am for is a discussion how to resolve things. Although expect >> resistance if you're thinking about applying tape, in order to fix >> somethings that's 'broken' elsewhere. >> >> If you or any !Linux folks are around on XDC we should really sit down >> and untangle some/all of these issues. > > It's not 100% certain but it looks like we won't be there. > > We need the uapi headers to be the same as libdrm ones to make syncing > easier. There is not much else to discuss here really. We (AMD) are > also the ones who have to work with these headers the most, not you, not Mikko. > Agreed and agreed. > While I understand some people want to discuss this further, these > patches must land first in order bring back the compatibility with > libdrm. This is where the misunderstanding lies - there _must_ _not_ be compatible with the libdrm ones, but the other way around. Check the output of $ git log -p -- include/drm in libdrm. Pretty please ? > After that, we can discuss the possible solutions and > everybody interested in a better solution *that will take libdrm into > account* can join. For now, I have to expect that those discussions > might also lead nowhere and > I don't wanna be stuck with bad uapi > headers in the kernel forever. > As mentioned before - please clearly state what do you perceive as bad and/or why. Daniel, myself and Rob (to a point) have explained that things are not perfect as-is but they are definitely not bad or wrong. Thanks Emil
On Sat, Aug 20, 2016 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 20 August 2016 at 12:47, Marek Olšák <maraeo@gmail.com> wrote: >> On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: >>>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>>> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>>>> >>>>>>> From: Marek Olšák <marek.olsak@amd.com> >>>>>>> >>>>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>>>> >>>>>>> See the comment in the code. Basically, don't do cleanups in this header. >>>>>>> >>>>>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>>>> >>>>>> >>>>>> I completely agree with you that this was a bad move, but I fear that we >>>>>> will run into opposition with that. >>>>>> >>>>> Please check the facts before introducing RATHER ANNOYING AND HARD TO >>>>> READ COMMENT IN ALL CAPS. >>>>> >>>>> Story time: >>>>> I was dreaming of a day were we can stop installing these headers, >>>>> thus making deprecation a bit easier process. >>>>> Yet after failing to convince Dave and Daniel on a number of occasions >>>>> I've accepted that those headers _are_ here to stay. And yes they >>>>> _are_ the UAPI, even though no applications are meant to use them but >>>>> the libdrm 'version'. >>>>> Thus any changes to the libdrm ones should be a mirror of the ones >>>>> here and libdrm should _not_ differ. >>>>> >>>>> But let's ignore all that and imagine that those headers are _not_ >>>>> UAPI. That gives us even greater reason to _not_ use the uintx_t types >>>>> but the kernel __uX ones. The series that did these changes had a fair >>>>> few references why we want that. >>>>> >>>>> Yes, I can imagine that the situation isn't ideal, and/or not that >>>>> clear. Then again a check with git log should have straightened things >>>>> out. >>>>> If not _please_ help us improve this (documentation and/or others). >>>>> >>>>> >>>>> And last but not least, please share with up what inspired this - >>>>> (build/runtime) regression, attempted sync with libdrm, other ? >>>> >>>> Syncing with libdrm became difficult. >>> Actually it should be easier now. Perhaps the radeon one was always a >>> good citizen, but sadly that was not the case for the rest. >>> >>>> I'd like the diff between kernel >>>> and libdrm to be as small as possible. >>>> >>> I believe we all agree on this one :-) >>> >>>> We must take into account that the uapi headers can potentially be >>>> implemented by a different OS. >>> Agreed. Have you looked at the 'compatibility layer' in drm.h ? >>> >>>> That's why they are in libdrm and >>>> that's why nobody should make random changes to them in the kernel >>>> tree. Do not think like a kernel developer isolated in Linux and just >>>> consider the broader use case. If you do, you'll realize that it >>>> simply doesn't make sense to use the __uX types here. >>>> >>> Ftr, like Rob (and maybe others) I believe that using __uX (in the >>> kernel) is a bit odd, and opting for the stdint.h types should happen. >>> But until/if that happens we have to live with the __uX ones. >>> >>> That said, I have poked various BSD people on a number of occasions, >>> (hopefully) inspiring them to upstream their changes in a compatible >>> way. Thus the whole "don't think like a kernel developer" doesn't >>> really apply here :-\ >>> >>> I'm simply one of the few fools^wpeople trying to make things OK for >>> most (since one can never please everyone, all the time). >>> >>> IIRC the FreeBSD/DragonFly people had some issues with their >>> compatibility layer since the kernel and userspace drm.h were >>> divergent "by design" [1]. To make it even 'better' there's even two >>> difference versions of drm.h in their kernel itself [2]. >>> >>> What I am for is a discussion how to resolve things. Although expect >>> resistance if you're thinking about applying tape, in order to fix >>> somethings that's 'broken' elsewhere. >>> >>> If you or any !Linux folks are around on XDC we should really sit down >>> and untangle some/all of these issues. >> >> It's not 100% certain but it looks like we won't be there. >> >> We need the uapi headers to be the same as libdrm ones to make syncing >> easier. There is not much else to discuss here really. We (AMD) are >> also the ones who have to work with these headers the most, not you, not Mikko. >> > Agreed and agreed. > >> While I understand some people want to discuss this further, these >> patches must land first in order bring back the compatibility with >> libdrm. > This is where the misunderstanding lies - there _must_ _not_ be > compatible with the libdrm ones, but the other way around. Check the > output of $ git log -p -- include/drm in libdrm. Pretty please ? > >> After that, we can discuss the possible solutions and >> everybody interested in a better solution *that will take libdrm into >> account* can join. For now, I have to expect that those discussions >> might also lead nowhere and > >> I don't wanna be stuck with bad uapi >> headers in the kernel forever. >> > As mentioned before - please clearly state what do you perceive as bad > and/or why. Daniel, myself and Rob (to a point) have explained that > things are not perfect as-is but they are definitely not bad or wrong. The problem is the diff is different, which has been said many times. Marek
Cc'ing lkml too. On Fri, Aug 19, 2016 at 11:54:21PM +0100, Emil Velikov wrote: > Story time: > I was dreaming of a day were we can stop installing these headers, > thus making deprecation a bit easier process. > Yet after failing to convince Dave and Daniel on a number of occasions > I've accepted that those headers _are_ here to stay. And yes they > _are_ the UAPI, even though no applications are meant to use them but > the libdrm 'version'. > Thus any changes to the libdrm ones should be a mirror of the ones > here and libdrm should _not_ differ. Another day dream: Wouldn't it be nice if the uapi headers from Linux kernel would pass a simple quality check of compiling in userspace where they are meant to be used? Stand alone. Without magic tricks and additional libraries and their headers. Without glibc or any other libc implementation specific additions. The uapi headers define many parts of the Linux kernel API and ABI, and thus compiling them also without the 'official' GNU/Linux userspace libraries like glibc or libdrm does have some uses. For example API and ABI compatibility checks and API/ABI/system call fuzzers. Many headers required stdint.h types but Linux kernel headers do not define them in userspace, and then Linus has said that uapi headers should use the linux/types.h with double underscores. Thus my patches for fixing trivial compile errors turned into changing several stdint.h definitions to linux/types.h. Yes, there have been some regressions in this work but to err is human. What is the actual problem and how can we (yes, including me) try to solve it? -Mikko
Cc'ing lkml. On Fri, Aug 19, 2016 at 09:18:24PM -0500, Ken Phillis Jr wrote: > On Fri, Aug 19, 2016 at 8:46 PM, Rob Clark <robdclark@gmail.com> wrote: > > > perhaps, but if the target audience for driver specific APIs is > > libdrm/mesa, which already uses stdint types, then I fail to see the > > point.. > > > > It is a bit difference scenario for some of the core kms ioctls which > > are not driver specific.. but most of the driver specific gpu related > > ioctls tend to be rather complex and not intended for use by something > > other than libdrm/mesa. > > > > BR, > > -R > > > > I personally do not see the reason to break user space API in the first > place. > I know for a fact that the include file ( linux/types.h ) mentioned by the > patch > supports C99 integer data types since Linux Kernel Version 2.2.0. This makes > the patch that got reverted completely meaningless since the only reason to > make this change in the first place would be if the developer intends on > backporting the driver to kernels older than version 2.2.0. In userspace linux/types.h does not provide these integer data types to avoid conflict with libc ones. Thus several uapi headers fail to compile in userspace. Fix is to either include libc side stdint.h in userspace but that was rejected by kernel developers including Linus who are in favor of kernel specific double underscore types. -Mikko
Cc'ing lkml. On Sat, Aug 20, 2016 at 12:05:54PM +0200, Marek Olšák wrote: > On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: > >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: > >>> > >>> From: Marek Olšák <marek.olsak@amd.com> > >>> > >>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. > >>> > >>> See the comment in the code. Basically, don't do cleanups in this header. > >>> > >>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> > >> > >> > >> I completely agree with you that this was a bad move, but I fear that we > >> will run into opposition with that. > >> > > Please check the facts before introducing RATHER ANNOYING AND HARD TO > > READ COMMENT IN ALL CAPS. > > > > Story time: > > I was dreaming of a day were we can stop installing these headers, > > thus making deprecation a bit easier process. > > Yet after failing to convince Dave and Daniel on a number of occasions > > I've accepted that those headers _are_ here to stay. And yes they > > _are_ the UAPI, even though no applications are meant to use them but > > the libdrm 'version'. > > Thus any changes to the libdrm ones should be a mirror of the ones > > here and libdrm should _not_ differ. > > > > But let's ignore all that and imagine that those headers are _not_ > > UAPI. That gives us even greater reason to _not_ use the uintx_t types > > but the kernel __uX ones. The series that did these changes had a fair > > few references why we want that. > > > > Yes, I can imagine that the situation isn't ideal, and/or not that > > clear. Then again a check with git log should have straightened things > > out. > > If not _please_ help us improve this (documentation and/or others). > > > > > > And last but not least, please share with up what inspired this - > > (build/runtime) regression, attempted sync with libdrm, other ? > > Syncing with libdrm became difficult. I'd like the diff between kernel > and libdrm to be as small as possible. > > We must take into account that the uapi headers can potentially be > implemented by a different OS. That's why they are in libdrm and > that's why nobody should make random changes to them in the kernel > tree. Do not think like a kernel developer isolated in Linux and just > consider the broader use case. If you do, you'll realize that it > simply doesn't make sense to use the __uX types here. When libdrm is combined with Linux kernel, then libdrm should be using the uapi headers from the kernel. For various reasons there are embedded kernel header copies in libdrm, glibc and basically everywhere but there should not be need for that. What exact problems did you now encounter with libdrm? Did something fail to compile on Linux or other OS'es? -Mikko
On Sat, Aug 20, 2016 at 8:08 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: > Cc'ing lkml. > > On Sat, Aug 20, 2016 at 12:05:54PM +0200, Marek Olšák wrote: >> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> > On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >> >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >> >>> >> >>> From: Marek Olšák <marek.olsak@amd.com> >> >>> >> >>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >> >>> >> >>> See the comment in the code. Basically, don't do cleanups in this header. >> >>> >> >>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >> >> >> >> >> >> I completely agree with you that this was a bad move, but I fear that we >> >> will run into opposition with that. >> >> >> > Please check the facts before introducing RATHER ANNOYING AND HARD TO >> > READ COMMENT IN ALL CAPS. >> > >> > Story time: >> > I was dreaming of a day were we can stop installing these headers, >> > thus making deprecation a bit easier process. >> > Yet after failing to convince Dave and Daniel on a number of occasions >> > I've accepted that those headers _are_ here to stay. And yes they >> > _are_ the UAPI, even though no applications are meant to use them but >> > the libdrm 'version'. >> > Thus any changes to the libdrm ones should be a mirror of the ones >> > here and libdrm should _not_ differ. >> > >> > But let's ignore all that and imagine that those headers are _not_ >> > UAPI. That gives us even greater reason to _not_ use the uintx_t types >> > but the kernel __uX ones. The series that did these changes had a fair >> > few references why we want that. >> > >> > Yes, I can imagine that the situation isn't ideal, and/or not that >> > clear. Then again a check with git log should have straightened things >> > out. >> > If not _please_ help us improve this (documentation and/or others). >> > >> > >> > And last but not least, please share with up what inspired this - >> > (build/runtime) regression, attempted sync with libdrm, other ? >> >> Syncing with libdrm became difficult. I'd like the diff between kernel >> and libdrm to be as small as possible. >> >> We must take into account that the uapi headers can potentially be >> implemented by a different OS. That's why they are in libdrm and >> that's why nobody should make random changes to them in the kernel >> tree. Do not think like a kernel developer isolated in Linux and just >> consider the broader use case. If you do, you'll realize that it >> simply doesn't make sense to use the __uX types here. > > When libdrm is combined with Linux kernel, then libdrm should be using > the uapi headers from the kernel. For various reasons there are embedded > kernel header copies in libdrm, glibc and basically everywhere but there > should not be need for that. You quoted what I had written but you didn't read it. > > What exact problems did you now encounter with libdrm? Did something fail > to compile on Linux or other OS'es? Read the thread again. I described the problem clearly. Marek
On Sat, Aug 20, 2016 at 8:28 PM, Marek Olšák <maraeo@gmail.com> wrote: > On Sat, Aug 20, 2016 at 8:08 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: >> Cc'ing lkml. >> >> On Sat, Aug 20, 2016 at 12:05:54PM +0200, Marek Olšák wrote: >>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> > On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>> >> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>> >>> >>> >>> From: Marek Olšák <marek.olsak@amd.com> >>> >>> >>> >>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>> >>> >>> >>> See the comment in the code. Basically, don't do cleanups in this header. >>> >>> >>> >>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>> >> >>> >> >>> >> I completely agree with you that this was a bad move, but I fear that we >>> >> will run into opposition with that. >>> >> >>> > Please check the facts before introducing RATHER ANNOYING AND HARD TO >>> > READ COMMENT IN ALL CAPS. >>> > >>> > Story time: >>> > I was dreaming of a day were we can stop installing these headers, >>> > thus making deprecation a bit easier process. >>> > Yet after failing to convince Dave and Daniel on a number of occasions >>> > I've accepted that those headers _are_ here to stay. And yes they >>> > _are_ the UAPI, even though no applications are meant to use them but >>> > the libdrm 'version'. >>> > Thus any changes to the libdrm ones should be a mirror of the ones >>> > here and libdrm should _not_ differ. >>> > >>> > But let's ignore all that and imagine that those headers are _not_ >>> > UAPI. That gives us even greater reason to _not_ use the uintx_t types >>> > but the kernel __uX ones. The series that did these changes had a fair >>> > few references why we want that. >>> > >>> > Yes, I can imagine that the situation isn't ideal, and/or not that >>> > clear. Then again a check with git log should have straightened things >>> > out. >>> > If not _please_ help us improve this (documentation and/or others). >>> > >>> > >>> > And last but not least, please share with up what inspired this - >>> > (build/runtime) regression, attempted sync with libdrm, other ? >>> >>> Syncing with libdrm became difficult. I'd like the diff between kernel >>> and libdrm to be as small as possible. >>> >>> We must take into account that the uapi headers can potentially be >>> implemented by a different OS. That's why they are in libdrm and >>> that's why nobody should make random changes to them in the kernel >>> tree. Do not think like a kernel developer isolated in Linux and just >>> consider the broader use case. If you do, you'll realize that it >>> simply doesn't make sense to use the __uX types here. >> >> When libdrm is combined with Linux kernel, then libdrm should be using >> the uapi headers from the kernel. For various reasons there are embedded >> kernel header copies in libdrm, glibc and basically everywhere but there >> should not be need for that. > > You quoted what I had written but you didn't read it. Here's why libdrm can't use the uapi headers: It would break the very common use case and that is building the latest userspace driver stack on top of an old kernel. The userspace needs the latest headers, not the installed ones. Only then is it fully compatible both ways. Marek
On 20 August 2016 at 16:08, Marek Olšák <maraeo@gmail.com> wrote: > On Sat, Aug 20, 2016 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 20 August 2016 at 12:47, Marek Olšák <maraeo@gmail.com> wrote: >>> On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>> On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: >>>>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>>>> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>>>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>>>>> >>>>>>>> From: Marek Olšák <marek.olsak@amd.com> >>>>>>>> >>>>>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>>>>> >>>>>>>> See the comment in the code. Basically, don't do cleanups in this header. >>>>>>>> >>>>>>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>>>>> >>>>>>> >>>>>>> I completely agree with you that this was a bad move, but I fear that we >>>>>>> will run into opposition with that. >>>>>>> >>>>>> Please check the facts before introducing RATHER ANNOYING AND HARD TO >>>>>> READ COMMENT IN ALL CAPS. >>>>>> >>>>>> Story time: >>>>>> I was dreaming of a day were we can stop installing these headers, >>>>>> thus making deprecation a bit easier process. >>>>>> Yet after failing to convince Dave and Daniel on a number of occasions >>>>>> I've accepted that those headers _are_ here to stay. And yes they >>>>>> _are_ the UAPI, even though no applications are meant to use them but >>>>>> the libdrm 'version'. >>>>>> Thus any changes to the libdrm ones should be a mirror of the ones >>>>>> here and libdrm should _not_ differ. >>>>>> >>>>>> But let's ignore all that and imagine that those headers are _not_ >>>>>> UAPI. That gives us even greater reason to _not_ use the uintx_t types >>>>>> but the kernel __uX ones. The series that did these changes had a fair >>>>>> few references why we want that. >>>>>> >>>>>> Yes, I can imagine that the situation isn't ideal, and/or not that >>>>>> clear. Then again a check with git log should have straightened things >>>>>> out. >>>>>> If not _please_ help us improve this (documentation and/or others). >>>>>> >>>>>> >>>>>> And last but not least, please share with up what inspired this - >>>>>> (build/runtime) regression, attempted sync with libdrm, other ? >>>>> >>>>> Syncing with libdrm became difficult. >>>> Actually it should be easier now. Perhaps the radeon one was always a >>>> good citizen, but sadly that was not the case for the rest. >>>> >>>>> I'd like the diff between kernel >>>>> and libdrm to be as small as possible. >>>>> >>>> I believe we all agree on this one :-) >>>> >>>>> We must take into account that the uapi headers can potentially be >>>>> implemented by a different OS. >>>> Agreed. Have you looked at the 'compatibility layer' in drm.h ? >>>> >>>>> That's why they are in libdrm and >>>>> that's why nobody should make random changes to them in the kernel >>>>> tree. Do not think like a kernel developer isolated in Linux and just >>>>> consider the broader use case. If you do, you'll realize that it >>>>> simply doesn't make sense to use the __uX types here. >>>>> >>>> Ftr, like Rob (and maybe others) I believe that using __uX (in the >>>> kernel) is a bit odd, and opting for the stdint.h types should happen. >>>> But until/if that happens we have to live with the __uX ones. >>>> >>>> That said, I have poked various BSD people on a number of occasions, >>>> (hopefully) inspiring them to upstream their changes in a compatible >>>> way. Thus the whole "don't think like a kernel developer" doesn't >>>> really apply here :-\ >>>> >>>> I'm simply one of the few fools^wpeople trying to make things OK for >>>> most (since one can never please everyone, all the time). >>>> >>>> IIRC the FreeBSD/DragonFly people had some issues with their >>>> compatibility layer since the kernel and userspace drm.h were >>>> divergent "by design" [1]. To make it even 'better' there's even two >>>> difference versions of drm.h in their kernel itself [2]. >>>> >>>> What I am for is a discussion how to resolve things. Although expect >>>> resistance if you're thinking about applying tape, in order to fix >>>> somethings that's 'broken' elsewhere. >>>> >>>> If you or any !Linux folks are around on XDC we should really sit down >>>> and untangle some/all of these issues. >>> >>> It's not 100% certain but it looks like we won't be there. >>> >>> We need the uapi headers to be the same as libdrm ones to make syncing >>> easier. There is not much else to discuss here really. We (AMD) are >>> also the ones who have to work with these headers the most, not you, not Mikko. >>> >> Agreed and agreed. >> >>> While I understand some people want to discuss this further, these >>> patches must land first in order bring back the compatibility with >>> libdrm. >> This is where the misunderstanding lies - there _must_ _not_ be >> compatible with the libdrm ones, but the other way around. Check the >> output of $ git log -p -- include/drm in libdrm. Pretty please ? >> >>> After that, we can discuss the possible solutions and >>> everybody interested in a better solution *that will take libdrm into >>> account* can join. For now, I have to expect that those discussions >>> might also lead nowhere and >> >>> I don't wanna be stuck with bad uapi >>> headers in the kernel forever. >>> >> As mentioned before - please clearly state what do you perceive as bad >> and/or why. Daniel, myself and Rob (to a point) have explained that >> things are not perfect as-is but they are definitely not bad or wrong. > > The problem is the diff is different, which has been said many times. > I see two things, neither of which implies any problems. - "syncing became difficult" which should _not_ be the case if you're using make headers_install - unease about usage of __uX types and misdirected finger pointing about compatibility with other OS. All I can think of is that you (?) are porting some changes from the kernel to libdrm or vice-versa. In the latter case please _don't_ do that. Work with your changes in upstream kernel, then pull them down to libdrm with `make headers_install`. Thanks Emil
On Sat, Aug 20, 2016 at 1:58 PM, Mikko Rapeli <mikko.rapeli@iki.fi> wrote: > Cc'ing lkml too. > > On Fri, Aug 19, 2016 at 11:54:21PM +0100, Emil Velikov wrote: >> Story time: >> I was dreaming of a day were we can stop installing these headers, >> thus making deprecation a bit easier process. >> Yet after failing to convince Dave and Daniel on a number of occasions >> I've accepted that those headers _are_ here to stay. And yes they >> _are_ the UAPI, even though no applications are meant to use them but >> the libdrm 'version'. >> Thus any changes to the libdrm ones should be a mirror of the ones >> here and libdrm should _not_ differ. > > Another day dream: > > Wouldn't it be nice if the uapi headers from Linux kernel would pass > a simple quality check of compiling in userspace where they are meant to be > used? Stand alone. Without magic tricks and additional libraries and their > headers. Without glibc or any other libc implementation specific additions. > The uapi headers define many parts of the Linux kernel API and ABI, and thus > compiling them also without the 'official' GNU/Linux userspace libraries > like glibc or libdrm does have some uses. For example API and ABI > compatibility checks and API/ABI/system call fuzzers. > > Many headers required stdint.h types but Linux kernel headers do not > define them in userspace, and then Linus has said that uapi headers > should use the linux/types.h with double underscores. Thus my patches > for fixing trivial compile errors turned into changing several stdint.h > definitions to linux/types.h. The problem is, for the most part, the driver specific gpu related ioctl interfaces are not intended for general public consumption. They have one consumer, ie. libdrm_$drivername (or perhaps mesa directly). They are complex interfaces, because GPUs are complex. They are not intended to be used directly (or for the most part, even indirectly) by random userspace applications. And in fact the uapi headers exported from kernel are not actually ever used. (ie. libdrm_$drivername uses it's own copy internally within libdrm.) So Linus's argument against stdint types, as weak as it is, doesn't even apply for gpu driver specific ioctls. BR, -R > Yes, there have been some regressions in this work but to err is human. > What is the actual problem and how can we (yes, including me) try to > solve it? > > -Mikko > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 20.08.2016 um 19:58 schrieb Mikko Rapeli: > Cc'ing lkml too. > > On Fri, Aug 19, 2016 at 11:54:21PM +0100, Emil Velikov wrote: >> Story time: >> I was dreaming of a day were we can stop installing these headers, >> thus making deprecation a bit easier process. >> Yet after failing to convince Dave and Daniel on a number of occasions >> I've accepted that those headers _are_ here to stay. And yes they >> _are_ the UAPI, even though no applications are meant to use them but >> the libdrm 'version'. >> Thus any changes to the libdrm ones should be a mirror of the ones >> here and libdrm should _not_ differ. > Another day dream: > > Wouldn't it be nice if the uapi headers from Linux kernel would pass > a simple quality check of compiling in userspace where they are meant to be > used? libdrm has a whole bunch of unit tests exercising the kernel UAPI headers for both API and ABI compatibility. So to be honest I see your good intentions here, but no those checks are completely useless for us. Christian. > Stand alone. Without magic tricks and additional libraries and their > headers. Without glibc or any other libc implementation specific additions. > The uapi headers define many parts of the Linux kernel API and ABI, and thus > compiling them also without the 'official' GNU/Linux userspace libraries > like glibc or libdrm does have some uses. For example API and ABI > compatibility checks and API/ABI/system call fuzzers. > > Many headers required stdint.h types but Linux kernel headers do not > define them in userspace, and then Linus has said that uapi headers > should use the linux/types.h with double underscores. Thus my patches > for fixing trivial compile errors turned into changing several stdint.h > definitions to linux/types.h. > > Yes, there have been some regressions in this work but to err is human. > What is the actual problem and how can we (yes, including me) try to > solve it? > > -Mikko
On Sat, Aug 20, 2016 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> While I understand some people want to discuss this further, these >> patches must land first in order bring back the compatibility with >> libdrm. > This is where the misunderstanding lies - there _must_ _not_ be > compatible with the libdrm ones, but the other way around. Check the > output of $ git log -p -- include/drm in libdrm. Pretty please ? Yes, uapi needs to flow from the kernel. libdrm cannot be treated as the master copy for shared headers. I think the problem here was simply that a patch landed without the Ack from maintainers, or they didn't check things carefully enough. -Daniel
On Sat, Aug 20, 2016 at 8:58 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 20 August 2016 at 16:08, Marek Olšák <maraeo@gmail.com> wrote: >> On Sat, Aug 20, 2016 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> On 20 August 2016 at 12:47, Marek Olšák <maraeo@gmail.com> wrote: >>>> On Sat, Aug 20, 2016 at 1:08 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>>> On 20 August 2016 at 11:05, Marek Olšák <maraeo@gmail.com> wrote: >>>>>> On Sat, Aug 20, 2016 at 12:54 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>>>>> On 19 August 2016 at 15:26, Christian König <deathsimple@vodafone.de> wrote: >>>>>>>> Am 19.08.2016 um 15:50 schrieb Marek Olšák: >>>>>>>>> >>>>>>>>> From: Marek Olšák <marek.olsak@amd.com> >>>>>>>>> >>>>>>>>> This reverts commit 2ce9dde0d47f2f94ab25c73a30596a7328bcdf1f. >>>>>>>>> >>>>>>>>> See the comment in the code. Basically, don't do cleanups in this header. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Olšák <marek.olsak@amd.com> >>>>>>>> >>>>>>>> >>>>>>>> I completely agree with you that this was a bad move, but I fear that we >>>>>>>> will run into opposition with that. >>>>>>>> >>>>>>> Please check the facts before introducing RATHER ANNOYING AND HARD TO >>>>>>> READ COMMENT IN ALL CAPS. >>>>>>> >>>>>>> Story time: >>>>>>> I was dreaming of a day were we can stop installing these headers, >>>>>>> thus making deprecation a bit easier process. >>>>>>> Yet after failing to convince Dave and Daniel on a number of occasions >>>>>>> I've accepted that those headers _are_ here to stay. And yes they >>>>>>> _are_ the UAPI, even though no applications are meant to use them but >>>>>>> the libdrm 'version'. >>>>>>> Thus any changes to the libdrm ones should be a mirror of the ones >>>>>>> here and libdrm should _not_ differ. >>>>>>> >>>>>>> But let's ignore all that and imagine that those headers are _not_ >>>>>>> UAPI. That gives us even greater reason to _not_ use the uintx_t types >>>>>>> but the kernel __uX ones. The series that did these changes had a fair >>>>>>> few references why we want that. >>>>>>> >>>>>>> Yes, I can imagine that the situation isn't ideal, and/or not that >>>>>>> clear. Then again a check with git log should have straightened things >>>>>>> out. >>>>>>> If not _please_ help us improve this (documentation and/or others). >>>>>>> >>>>>>> >>>>>>> And last but not least, please share with up what inspired this - >>>>>>> (build/runtime) regression, attempted sync with libdrm, other ? >>>>>> >>>>>> Syncing with libdrm became difficult. >>>>> Actually it should be easier now. Perhaps the radeon one was always a >>>>> good citizen, but sadly that was not the case for the rest. >>>>> >>>>>> I'd like the diff between kernel >>>>>> and libdrm to be as small as possible. >>>>>> >>>>> I believe we all agree on this one :-) >>>>> >>>>>> We must take into account that the uapi headers can potentially be >>>>>> implemented by a different OS. >>>>> Agreed. Have you looked at the 'compatibility layer' in drm.h ? >>>>> >>>>>> That's why they are in libdrm and >>>>>> that's why nobody should make random changes to them in the kernel >>>>>> tree. Do not think like a kernel developer isolated in Linux and just >>>>>> consider the broader use case. If you do, you'll realize that it >>>>>> simply doesn't make sense to use the __uX types here. >>>>>> >>>>> Ftr, like Rob (and maybe others) I believe that using __uX (in the >>>>> kernel) is a bit odd, and opting for the stdint.h types should happen. >>>>> But until/if that happens we have to live with the __uX ones. >>>>> >>>>> That said, I have poked various BSD people on a number of occasions, >>>>> (hopefully) inspiring them to upstream their changes in a compatible >>>>> way. Thus the whole "don't think like a kernel developer" doesn't >>>>> really apply here :-\ >>>>> >>>>> I'm simply one of the few fools^wpeople trying to make things OK for >>>>> most (since one can never please everyone, all the time). >>>>> >>>>> IIRC the FreeBSD/DragonFly people had some issues with their >>>>> compatibility layer since the kernel and userspace drm.h were >>>>> divergent "by design" [1]. To make it even 'better' there's even two >>>>> difference versions of drm.h in their kernel itself [2]. >>>>> >>>>> What I am for is a discussion how to resolve things. Although expect >>>>> resistance if you're thinking about applying tape, in order to fix >>>>> somethings that's 'broken' elsewhere. >>>>> >>>>> If you or any !Linux folks are around on XDC we should really sit down >>>>> and untangle some/all of these issues. >>>> >>>> It's not 100% certain but it looks like we won't be there. >>>> >>>> We need the uapi headers to be the same as libdrm ones to make syncing >>>> easier. There is not much else to discuss here really. We (AMD) are >>>> also the ones who have to work with these headers the most, not you, not Mikko. >>>> >>> Agreed and agreed. >>> >>>> While I understand some people want to discuss this further, these >>>> patches must land first in order bring back the compatibility with >>>> libdrm. >>> This is where the misunderstanding lies - there _must_ _not_ be >>> compatible with the libdrm ones, but the other way around. Check the >>> output of $ git log -p -- include/drm in libdrm. Pretty please ? >>> >>>> After that, we can discuss the possible solutions and >>>> everybody interested in a better solution *that will take libdrm into >>>> account* can join. For now, I have to expect that those discussions >>>> might also lead nowhere and >>> >>>> I don't wanna be stuck with bad uapi >>>> headers in the kernel forever. >>>> >>> As mentioned before - please clearly state what do you perceive as bad >>> and/or why. Daniel, myself and Rob (to a point) have explained that >>> things are not perfect as-is but they are definitely not bad or wrong. >> >> The problem is the diff is different, which has been said many times. >> > I see two things, neither of which implies any problems. > - "syncing became difficult" which should _not_ be the case if you're > using make headers_install > - unease about usage of __uX types and misdirected finger pointing > about compatibility with other OS. > > All I can think of is that you (?) are porting some changes from the > kernel to libdrm or vice-versa. In the latter case please _don't_ do > that. Work with your changes in upstream kernel, then pull them down > to libdrm with `make headers_install`. Same here, I'm honestly not clear on what the problem is. uapi flows from the kernel, we _must_ change the kernel headers first before libdrm, and we can do that as long as we don't change the api or abi. Resyncing is trivial using make headers_install and the flow Emil laid out. Definitely _never_ apply uapi changes to libdrm first (or only, and iirc one of the radeon headers had done that). -Daniel
On Sat, 20 Aug 2016, Emil Velikov wrote: > > If you or any !Linux folks are around on XDC we should really sit down > and untangle some/all of these issues. > > Thanks > Emil > If there was to be some sort of BoF (doesn't have to be formal, and could be an after-hours event), I would investigate attending. ---- Randy
On Mon, 22 Aug 2016, Daniel Vetter wrote: > On Sat, Aug 20, 2016 at 8:58 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> >> All I can think of is that you (?) are porting some changes from the >> kernel to libdrm or vice-versa. In the latter case please _don't_ do >> that. Work with your changes in upstream kernel, then pull them down >> to libdrm with `make headers_install`. > > Same here, I'm honestly not clear on what the problem is. uapi flows > from the kernel, we _must_ change the kernel headers first before > libdrm, and we can do that as long as we don't change the api or abi. IMO, the problem is that they can be different. And any difference is an opportunity for problems. It wouldn't matter which is the master, so long both sides consume the same file. > > Resyncing is trivial using make headers_install and the flow Emil laid > out. Definitely _never_ apply uapi changes to libdrm first (or only, > and iirc one of the radeon headers had done that). > -Daniel In the Solaris space, there isn't a 'make headers_install' that is sufficient to resolve the discrepancy (the uapi files are not usable as is and need patching - might be the same for the BSD's as well). So this methodology might be sufficient for Linux, but will in some way fall short for others (yes, I am aware of this and take it on, but occasionally nice to note). Cheers! ---- Randy
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h index 462246a..b39e07c 100644 --- a/include/uapi/drm/amdgpu_drm.h +++ b/include/uapi/drm/amdgpu_drm.h @@ -29,6 +29,11 @@ * Keith Whitwell <keith@tungstengraphics.com> */ +/* IT IS NOT ALLOWED TO CHANGE THIS HEADER WITHOUT DOING THE SAME IN LIBDRM !!! + * THIS IS NOT A UAPI HEADER. IT IS ONLY A MIRROR OF ITS COUNTERPART IN LIBDRM. + * USERSPACE SHOULD USE THE HEADERS FROM LIBDRM. NOT THIS ONE. + */ + #ifndef __AMDGPU_DRM_H__ #define __AMDGPU_DRM_H__ @@ -80,19 +85,19 @@ extern "C" { struct drm_amdgpu_gem_create_in { /** the requested memory size */ - __u64 bo_size; + uint64_t bo_size; /** physical start_addr alignment in bytes for some HW requirements */ - __u64 alignment; + uint64_t alignment; /** the requested memory domains */ - __u64 domains; + uint64_t domains; /** allocation flags */ - __u64 domain_flags; + uint64_t domain_flags; }; struct drm_amdgpu_gem_create_out { /** returned GEM object handle */ - __u32 handle; - __u32 _pad; + uint32_t handle; + uint32_t _pad; }; union drm_amdgpu_gem_create { @@ -109,28 +114,28 @@ union drm_amdgpu_gem_create { struct drm_amdgpu_bo_list_in { /** Type of operation */ - __u32 operation; + uint32_t operation; /** Handle of list or 0 if we want to create one */ - __u32 list_handle; + uint32_t list_handle; /** Number of BOs in list */ - __u32 bo_number; + uint32_t bo_number; /** Size of each element describing BO */ - __u32 bo_info_size; + uint32_t bo_info_size; /** Pointer to array describing BOs */ - __u64 bo_info_ptr; + uint64_t bo_info_ptr; }; struct drm_amdgpu_bo_list_entry { /** Handle of BO */ - __u32 bo_handle; + uint32_t bo_handle; /** New (if specified) BO priority to be used during migration */ - __u32 bo_priority; + uint32_t bo_priority; }; struct drm_amdgpu_bo_list_out { /** Handle of resource list */ - __u32 list_handle; - __u32 _pad; + uint32_t list_handle; + uint32_t _pad; }; union drm_amdgpu_bo_list { @@ -154,26 +159,26 @@ union drm_amdgpu_bo_list { struct drm_amdgpu_ctx_in { /** AMDGPU_CTX_OP_* */ - __u32 op; + uint32_t op; /** For future use, no flags defined so far */ - __u32 flags; - __u32 ctx_id; - __u32 _pad; + uint32_t flags; + uint32_t ctx_id; + uint32_t _pad; }; union drm_amdgpu_ctx_out { struct { - __u32 ctx_id; - __u32 _pad; + uint32_t ctx_id; + uint32_t _pad; } alloc; struct { /** For future use, no flags defined so far */ - __u64 flags; + uint64_t flags; /** Number of resets caused by this context so far. */ - __u32 hangs; + uint32_t hangs; /** Reset status since the last call of the ioctl. */ - __u32 reset_status; + uint32_t reset_status; } state; }; @@ -193,12 +198,12 @@ union drm_amdgpu_ctx { #define AMDGPU_GEM_USERPTR_REGISTER (1 << 3) struct drm_amdgpu_gem_userptr { - __u64 addr; - __u64 size; + uint64_t addr; + uint64_t size; /* AMDGPU_GEM_USERPTR_* */ - __u32 flags; + uint32_t flags; /* Resulting GEM handle */ - __u32 handle; + uint32_t handle; }; /* same meaning as the GB_TILE_MODE and GL_MACRO_TILE_MODE fields */ @@ -230,28 +235,28 @@ struct drm_amdgpu_gem_userptr { /** The same structure is shared for input/output */ struct drm_amdgpu_gem_metadata { /** GEM Object handle */ - __u32 handle; + uint32_t handle; /** Do we want get or set metadata */ - __u32 op; + uint32_t op; struct { /** For future use, no flags defined so far */ - __u64 flags; + uint64_t flags; /** family specific tiling info */ - __u64 tiling_info; - __u32 data_size_bytes; - __u32 data[64]; + uint64_t tiling_info; + uint32_t data_size_bytes; + uint32_t data[64]; } data; }; struct drm_amdgpu_gem_mmap_in { /** the GEM object handle */ - __u32 handle; - __u32 _pad; + uint32_t handle; + uint32_t _pad; }; struct drm_amdgpu_gem_mmap_out { /** mmap offset from the vma offset manager */ - __u64 addr_ptr; + uint64_t addr_ptr; }; union drm_amdgpu_gem_mmap { @@ -261,18 +266,18 @@ union drm_amdgpu_gem_mmap { struct drm_amdgpu_gem_wait_idle_in { /** GEM object handle */ - __u32 handle; + uint32_t handle; /** For future use, no flags defined so far */ - __u32 flags; + uint32_t flags; /** Absolute timeout to wait */ - __u64 timeout; + uint64_t timeout; }; struct drm_amdgpu_gem_wait_idle_out { /** BO status: 0 - BO is idle, 1 - BO is busy */ - __u32 status; + uint32_t status; /** Returned current memory domain */ - __u32 domain; + uint32_t domain; }; union drm_amdgpu_gem_wait_idle { @@ -282,18 +287,18 @@ union drm_amdgpu_gem_wait_idle { struct drm_amdgpu_wait_cs_in { /** Command submission handle */ - __u64 handle; + uint64_t handle; /** Absolute timeout to wait */ - __u64 timeout; - __u32 ip_type; - __u32 ip_instance; - __u32 ring; - __u32 ctx_id; + uint64_t timeout; + uint32_t ip_type; + uint32_t ip_instance; + uint32_t ring; + uint32_t ctx_id; }; struct drm_amdgpu_wait_cs_out { /** CS status: 0 - CS completed, 1 - CS still busy */ - __u64 status; + uint64_t status; }; union drm_amdgpu_wait_cs { @@ -307,11 +312,11 @@ union drm_amdgpu_wait_cs { /* Sets or returns a value associated with a buffer. */ struct drm_amdgpu_gem_op { /** GEM object handle */ - __u32 handle; + uint32_t handle; /** AMDGPU_GEM_OP_* */ - __u32 op; + uint32_t op; /** Input or return value */ - __u64 value; + uint64_t value; }; #define AMDGPU_VA_OP_MAP 1 @@ -330,18 +335,18 @@ struct drm_amdgpu_gem_op { struct drm_amdgpu_gem_va { /** GEM object handle */ - __u32 handle; - __u32 _pad; + uint32_t handle; + uint32_t _pad; /** AMDGPU_VA_OP_* */ - __u32 operation; + uint32_t operation; /** AMDGPU_VM_PAGE_* */ - __u32 flags; + uint32_t flags; /** va address to assign . Must be correctly aligned.*/ - __u64 va_address; + uint64_t va_address; /** Specify offset inside of BO to assign. Must be correctly aligned.*/ - __u64 offset_in_bo; + uint64_t offset_in_bo; /** Specify mapping size. Must be correctly aligned. */ - __u64 map_size; + uint64_t map_size; }; #define AMDGPU_HW_IP_GFX 0 @@ -358,24 +363,24 @@ struct drm_amdgpu_gem_va { #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03 struct drm_amdgpu_cs_chunk { - __u32 chunk_id; - __u32 length_dw; - __u64 chunk_data; + uint32_t chunk_id; + uint32_t length_dw; + uint64_t chunk_data; }; struct drm_amdgpu_cs_in { /** Rendering context id */ - __u32 ctx_id; + uint32_t ctx_id; /** Handle of resource list associated with CS */ - __u32 bo_list_handle; - __u32 num_chunks; - __u32 _pad; - /** this points to __u64 * which point to cs chunks */ - __u64 chunks; + uint32_t bo_list_handle; + uint32_t num_chunks; + uint32_t _pad; + /** this points to uint64_t * which point to cs chunks */ + uint64_t chunks; }; struct drm_amdgpu_cs_out { - __u64 handle; + uint64_t handle; }; union drm_amdgpu_cs { @@ -392,32 +397,32 @@ union drm_amdgpu_cs { #define AMDGPU_IB_FLAG_PREAMBLE (1<<1) struct drm_amdgpu_cs_chunk_ib { - __u32 _pad; + uint32_t _pad; /** AMDGPU_IB_FLAG_* */ - __u32 flags; + uint32_t flags; /** Virtual address to begin IB execution */ - __u64 va_start; + uint64_t va_start; /** Size of submission */ - __u32 ib_bytes; + uint32_t ib_bytes; /** HW IP to submit to */ - __u32 ip_type; + uint32_t ip_type; /** HW IP index of the same type to submit to */ - __u32 ip_instance; + uint32_t ip_instance; /** Ring index to submit to */ - __u32 ring; + uint32_t ring; }; struct drm_amdgpu_cs_chunk_dep { - __u32 ip_type; - __u32 ip_instance; - __u32 ring; - __u32 ctx_id; - __u64 handle; + uint32_t ip_type; + uint32_t ip_instance; + uint32_t ring; + uint32_t ctx_id; + uint64_t handle; }; struct drm_amdgpu_cs_chunk_fence { - __u32 handle; - __u32 offset; + uint32_t handle; + uint32_t offset; }; struct drm_amdgpu_cs_chunk_data { @@ -489,53 +494,53 @@ struct drm_amdgpu_cs_chunk_data { struct drm_amdgpu_query_fw { /** AMDGPU_INFO_FW_* */ - __u32 fw_type; + uint32_t fw_type; /** * Index of the IP if there are more IPs of * the same type. */ - __u32 ip_instance; + uint32_t ip_instance; /** * Index of the engine. Whether this is used depends * on the firmware type. (e.g. MEC, SDMA) */ - __u32 index; - __u32 _pad; + uint32_t index; + uint32_t _pad; }; /* Input structure for the INFO ioctl */ struct drm_amdgpu_info { /* Where the return value will be stored */ - __u64 return_pointer; + uint64_t return_pointer; /* The size of the return value. Just like "size" in "snprintf", * it limits how many bytes the kernel can write. */ - __u32 return_size; + uint32_t return_size; /* The query request id. */ - __u32 query; + uint32_t query; union { struct { - __u32 id; - __u32 _pad; + uint32_t id; + uint32_t _pad; } mode_crtc; struct { /** AMDGPU_HW_IP_* */ - __u32 type; + uint32_t type; /** * Index of the IP if there are more IPs of the same * type. Ignored by AMDGPU_INFO_HW_IP_COUNT. */ - __u32 ip_instance; + uint32_t ip_instance; } query_hw_ip; struct { - __u32 dword_offset; + uint32_t dword_offset; /** number of registers to read */ - __u32 count; - __u32 instance; + uint32_t count; + uint32_t instance; /** For future use, no flags defined so far */ - __u32 flags; + uint32_t flags; } read_mmr_reg; struct drm_amdgpu_query_fw query_fw; @@ -544,31 +549,31 @@ struct drm_amdgpu_info { struct drm_amdgpu_info_gds { /** GDS GFX partition size */ - __u32 gds_gfx_partition_size; + uint32_t gds_gfx_partition_size; /** GDS compute partition size */ - __u32 compute_partition_size; + uint32_t compute_partition_size; /** total GDS memory size */ - __u32 gds_total_size; + uint32_t gds_total_size; /** GWS size per GFX partition */ - __u32 gws_per_gfx_partition; + uint32_t gws_per_gfx_partition; /** GSW size per compute partition */ - __u32 gws_per_compute_partition; + uint32_t gws_per_compute_partition; /** OA size per GFX partition */ - __u32 oa_per_gfx_partition; + uint32_t oa_per_gfx_partition; /** OA size per compute partition */ - __u32 oa_per_compute_partition; - __u32 _pad; + uint32_t oa_per_compute_partition; + uint32_t _pad; }; struct drm_amdgpu_info_vram_gtt { - __u64 vram_size; - __u64 vram_cpu_accessible_size; - __u64 gtt_size; + uint64_t vram_size; + uint64_t vram_cpu_accessible_size; + uint64_t gtt_size; }; struct drm_amdgpu_info_firmware { - __u32 ver; - __u32 feature; + uint32_t ver; + uint32_t feature; }; #define AMDGPU_VRAM_TYPE_UNKNOWN 0 @@ -582,61 +587,61 @@ struct drm_amdgpu_info_firmware { struct drm_amdgpu_info_device { /** PCI Device ID */ - __u32 device_id; + uint32_t device_id; /** Internal chip revision: A0, A1, etc.) */ - __u32 chip_rev; - __u32 external_rev; + uint32_t chip_rev; + uint32_t external_rev; /** Revision id in PCI Config space */ - __u32 pci_rev; - __u32 family; - __u32 num_shader_engines; - __u32 num_shader_arrays_per_engine; + uint32_t pci_rev; + uint32_t family; + uint32_t num_shader_engines; + uint32_t num_shader_arrays_per_engine; /* in KHz */ - __u32 gpu_counter_freq; - __u64 max_engine_clock; - __u64 max_memory_clock; + uint32_t gpu_counter_freq; + uint64_t max_engine_clock; + uint64_t max_memory_clock; /* cu information */ - __u32 cu_active_number; - __u32 cu_ao_mask; - __u32 cu_bitmap[4][4]; + uint32_t cu_active_number; + uint32_t cu_ao_mask; + uint32_t cu_bitmap[4][4]; /** Render backend pipe mask. One render backend is CB+DB. */ - __u32 enabled_rb_pipes_mask; - __u32 num_rb_pipes; - __u32 num_hw_gfx_contexts; - __u32 _pad; - __u64 ids_flags; + uint32_t enabled_rb_pipes_mask; + uint32_t num_rb_pipes; + uint32_t num_hw_gfx_contexts; + uint32_t _pad; + uint64_t ids_flags; /** Starting virtual address for UMDs. */ - __u64 virtual_address_offset; + uint64_t virtual_address_offset; /** The maximum virtual address */ - __u64 virtual_address_max; + uint64_t virtual_address_max; /** Required alignment of virtual addresses. */ - __u32 virtual_address_alignment; + uint32_t virtual_address_alignment; /** Page table entry - fragment size */ - __u32 pte_fragment_size; - __u32 gart_page_size; + uint32_t pte_fragment_size; + uint32_t gart_page_size; /** constant engine ram size*/ - __u32 ce_ram_size; + uint32_t ce_ram_size; /** video memory type info*/ - __u32 vram_type; + uint32_t vram_type; /** video memory bit width*/ - __u32 vram_bit_width; + uint32_t vram_bit_width; /* vce harvesting instance */ - __u32 vce_harvest_config; + uint32_t vce_harvest_config; }; struct drm_amdgpu_info_hw_ip { /** Version of h/w IP */ - __u32 hw_ip_version_major; - __u32 hw_ip_version_minor; + uint32_t hw_ip_version_major; + uint32_t hw_ip_version_minor; /** Capabilities */ - __u64 capabilities_flags; + uint64_t capabilities_flags; /** command buffer address start alignment*/ - __u32 ib_start_alignment; + uint32_t ib_start_alignment; /** command buffer size alignment*/ - __u32 ib_size_alignment; + uint32_t ib_size_alignment; /** Bitmask of available rings. Bit 0 means ring 0, etc. */ - __u32 available_rings; - __u32 _pad; + uint32_t available_rings; + uint32_t _pad; }; /*