Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466479 > unrolled thread

Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and __u64 from <linux/types.h>"

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and  __u64 from <linux/types.h>" Mikko Rapeli <mikko.rapeli@iki.fi> - 2016-08-19 17:00 +0200
    Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and  __u64 from <linux/types.h>" Marek Olšák <maraeo@gmail.com> - 2016-08-19 17:30 +0200
      Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and  __u64 from <linux/types.h>" Daniel Vetter <daniel@ffwll.ch> - 2016-08-19 19:20 +0200
        Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and  __u64 from <linux/types.h>" Marek Olšák <maraeo@gmail.com> - 2016-08-19 19:30 +0200
      Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and  __u64 from <linux/types.h>" Daniel Vetter <daniel@ffwll.ch> - 2016-08-19 19:20 +0200

#1466479 — Re: [PATCH 1/2] Revert "include/uapi/drm/amdgpu_drm.h: use __u32 and __u64 from <linux/types.h>"

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;
> >  };
> >  /*
> 
> 

[toc] | [next] | [standalone]

#1466495

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

[toc] | [prev] | [next] | [standalone]

#1466566

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1466577

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

[toc] | [prev] | [next] | [standalone]

#1466574

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web