Skip to contentSkip to conversation

drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

PATCH In review Oded Gabbay 2 participants 10 messages Dec 29, 2014

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

This patch moves the copy_to_user() and copy_from_user() calls from the different ioctl functions in amdkfd to the general kfd_ioctl() function, as this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
 1 file changed, 117 insertions(+), 117 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +117 -117
index 7d4974b..5460ad2 100644
@@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep)
 	return 0;
 }
 
-static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
+					void *data)
 {
-	struct kfd_ioctl_get_version_args args;
+	struct kfd_ioctl_get_version_args *args = data;
 	int err = 0;
 
-	args.major_version = KFD_IOCTL_MAJOR_VERSION;
-	args.minor_version = KFD_IOCTL_MINOR_VERSION;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		err = -EFAULT;
+	args->major_version = KFD_IOCTL_MAJOR_VERSION;
+	args->minor_version = KFD_IOCTL_MINOR_VERSION;
 
 	return err;
 }
@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties,
 	return 0;
 }
 
-static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
+					void *data)
 {
-	struct kfd_ioctl_create_queue_args args;
+	struct kfd_ioctl_create_queue_args *args = data;
 	struct kfd_dev *dev;
 	int err = 0;
 	unsigned int queue_id;
@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
 	memset(&q_properties, 0, sizeof(struct queue_properties));
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
 	pr_debug("kfd: creating queue ioctl\n");
 
-	err = set_queue_properties_from_user(&q_properties, &args);
+	err = set_queue_properties_from_user(&q_properties, args);
 	if (err)
 		return err;
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
 	pdd = kfd_bind_process_to_device(dev, p);
 	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
 		goto err_bind_process;
 	}
 
@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 	if (err != 0)
 		goto err_create_queue;
 
-	args.queue_id = queue_id;
+	args->queue_id = queue_id;
 
 	/* Return gpu_id as doorbell offset for mmap usage */
-	args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
-
-	if (copy_to_user(arg, &args, sizeof(args))) {
-		err = -EFAULT;
-		goto err_copy_args_out;
-	}
+	args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
 
 	mutex_unlock(&p->mutex);
 
-	pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
+	pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
 
 	pr_debug("ring buffer address == 0x%016llX\n",
-			args.ring_base_address);
+			args->ring_base_address);
 
 	pr_debug("read ptr address    == 0x%016llX\n",
-			args.read_pointer_address);
+			args->read_pointer_address);
 
 	pr_debug("write ptr address   == 0x%016llX\n",
-			args.write_pointer_address);
+			args->write_pointer_address);
 
 	return 0;
 
-err_copy_args_out:
-	pqm_destroy_queue(&p->pqm, queue_id);
 err_create_queue:
 err_bind_process:
 	mutex_unlock(&p->mutex);
@@ -297,99 +284,90 @@ err_bind_process:
 }
 
 static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
 {
 	int retval;
-	struct kfd_ioctl_destroy_queue_args args;
-
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
+	struct kfd_ioctl_destroy_queue_args *args = data;
 
 	pr_debug("kfd: destroying queue id %d for PASID %d\n",
-				args.queue_id,
+				args->queue_id,
 				p->pasid);
 
 	mutex_lock(&p->mutex);
 
-	retval = pqm_destroy_queue(&p->pqm, args.queue_id);
+	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
 
 	mutex_unlock(&p->mutex);
 	return retval;
 }
 
 static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
 {
 	int retval;
-	struct kfd_ioctl_update_queue_args args;
+	struct kfd_ioctl_update_queue_args *args = data;
 	struct queue_properties properties;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
+	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
 		pr_err("kfd: queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
 		return -EINVAL;
 	}
 
-	if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
+	if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
 		pr_err("kfd: queue priority must be between 0 to KFD_MAX_QUEUE_PRIORITY\n");
 		return -EINVAL;
 	}
 
-	if ((args.ring_base_address) &&
+	if ((args->ring_base_address) &&
 		(!access_ok(VERIFY_WRITE,
-			(const void __user *) args.ring_base_address,
+			(const void __user *) args->ring_base_address,
 			sizeof(uint64_t)))) {
 		pr_err("kfd: can't access ring base address\n");
 		return -EFAULT;
 	}
 
-	if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
+	if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
 		pr_err("kfd: ring size must be a power of 2 or 0\n");
 		return -EINVAL;
 	}
 
-	properties.queue_address = args.ring_base_address;
-	properties.queue_size = args.ring_size;
-	properties.queue_percent = args.queue_percentage;
-	properties.priority = args.queue_priority;
+	properties.queue_address = args->ring_base_address;
+	properties.queue_size = args->ring_size;
+	properties.queue_percent = args->queue_percentage;
+	properties.priority = args->queue_priority;
 
 	pr_debug("kfd: updating queue id %d for PASID %d\n",
-			args.queue_id, p->pasid);
+			args->queue_id, p->pasid);
 
 	mutex_lock(&p->mutex);
 
-	retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
+	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
 
 	mutex_unlock(&p->mutex);
 
 	return retval;
 }
 
-static long kfd_ioctl_set_memory_policy(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_set_memory_policy(struct file *filep,
+					struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_set_memory_policy_args args;
+	struct kfd_ioctl_set_memory_policy_args *args = data;
 	struct kfd_dev *dev;
 	int err = 0;
 	struct kfd_process_device *pdd;
 	enum cache_policy default_policy, alternate_policy;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
 		return -EINVAL;
 	}
 
-	if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
 		return -EINVAL;
 	}
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
 
 	pdd = kfd_bind_process_to_device(dev, p);
 	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
 		goto out;
 	}
 
-	default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+	default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
 			 ? cache_policy_coherent : cache_policy_noncoherent;
 
 	alternate_policy =
-		(args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+		(args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
 		   ? cache_policy_coherent : cache_policy_noncoherent;
 
 	if (!dev->dqm->set_cache_memory_policy(dev->dqm,
 				&pdd->qpd,
 				default_policy,
 				alternate_policy,
-				(void __user *)args.alternate_aperture_base,
-				args.alternate_aperture_size))
+				(void __user *)args->alternate_aperture_base,
+				args->alternate_aperture_size))
 		err = -EINVAL;
 
 out:
@@ -422,53 +400,44 @@ out:
 	return err;
 }
 
-static long kfd_ioctl_get_clock_counters(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_get_clock_counters(struct file *filep,
+				struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_get_clock_counters_args args;
+	struct kfd_ioctl_get_clock_counters_args *args = data;
 	struct kfd_dev *dev;
 	struct timespec time;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
 	/* Reading GPU clock counter from KGD */
-	args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
+	args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
 
 	/* No access to rdtsc. Using raw monotonic time */
 	getrawmonotonic(&time);
-	args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
 
 	get_monotonic_boottime(&time);
-	args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
 
 	/* Since the counter is in nano-seconds we use 1GHz frequency */
-	args.system_clock_freq = 1000000000;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
+	args->system_clock_freq = 1000000000;
 
 	return 0;
 }
 
 
 static int kfd_ioctl_get_process_apertures(struct file *filp,
-				struct kfd_process *p, void __user *arg)
+				struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_get_process_apertures_args args;
+	struct kfd_ioctl_get_process_apertures_args *args = data;
 	struct kfd_process_device_apertures *pAperture;
 	struct kfd_process_device *pdd;
 
 	dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	args.num_of_nodes = 0;
+	args->num_of_nodes = 0;
 
 	mutex_lock(&p->mutex);
 
@@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 		/* Run over all pdd of the process */
 		pdd = kfd_get_first_process_device_data(p);
 		do {
-			pAperture = &args.process_apertures[args.num_of_nodes];
+			pAperture =
+				&args->process_apertures[args->num_of_nodes];
 			pAperture->gpu_id = pdd->dev->id;
 			pAperture->lds_base = pdd->lds_base;
 			pAperture->lds_limit = pdd->lds_limit;
@@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 			pAperture->scratch_limit = pdd->scratch_limit;
 
 			dev_dbg(kfd_device,
-				"node id %u\n", args.num_of_nodes);
+				"node id %u\n", args->num_of_nodes);
 			dev_dbg(kfd_device,
 				"gpu id %u\n", pdd->dev->id);
 			dev_dbg(kfd_device,
@@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 			dev_dbg(kfd_device,
 				"scratch_limit %llX\n", pdd->scratch_limit);
 
-			args.num_of_nodes++;
+			args->num_of_nodes++;
 		} while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
-				(args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
+				(args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
 	}
 
 	mutex_unlock(&p->mutex);
 
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
-
 	return 0;
 }
 
 static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	struct kfd_process *process;
-	long err = -EINVAL;
+	char stack_kdata[128];
+	char *kdata = NULL;
+	unsigned int usize, asize;
+	int retcode = -EINVAL;
 
 	dev_dbg(kfd_device,
 		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	if (IS_ERR(process))
 		return PTR_ERR(process);
 
+	if (cmd & (IOC_IN | IOC_OUT)) {
+		if (asize <= sizeof(stack_kdata)) {
+			kdata = stack_kdata;
+		} else {
+			kdata = kmalloc(asize, GFP_KERNEL);
+			if (!kdata) {
+				retcode = -ENOMEM;
+				goto err_i1;
+			}
+		}
+		if (asize > usize)
+			memset(kdata + usize, 0, asize - usize);
+	}
+
+	if (cmd & IOC_IN) {
+		if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
+			retcode = -EFAULT;
+			goto err_i1;
+		}
+	} else if (cmd & IOC_OUT) {
+		memset(kdata, 0, usize);
+	}
+
+
 	switch (cmd) {
 	case KFD_IOC_GET_VERSION:
-		err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
+		retcode = kfd_ioctl_get_version(filep, process, kdata);
 		break;
 	case KFD_IOC_CREATE_QUEUE:
-		err = kfd_ioctl_create_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_create_queue(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_DESTROY_QUEUE:
-		err = kfd_ioctl_destroy_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_destroy_queue(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_SET_MEMORY_POLICY:
-		err = kfd_ioctl_set_memory_policy(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_set_memory_policy(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_GET_CLOCK_COUNTERS:
-		err = kfd_ioctl_get_clock_counters(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_clock_counters(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_GET_PROCESS_APERTURES:
-		err = kfd_ioctl_get_process_apertures(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_process_apertures(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_UPDATE_QUEUE:
-		err = kfd_ioctl_update_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_update_queue(filep, process,
+						kdata);
 		break;
 
 	default:
-		dev_err(kfd_device,
+		dev_dbg(kfd_device,
 			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
 			cmd, arg);
-		err = -EINVAL;
+		retcode = -EINVAL;
 		break;
 	}
 
-	if (err < 0)
-		dev_err(kfd_device,
-			"ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
-			err, cmd, _IOC_NR(cmd));
+	if (cmd & IOC_OUT)
+		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
+			retcode = -EFAULT;
 
-	return err;
+err_i1:
+	if (kdata != stack_kdata)
+		kfree(kdata);
+
+	if (retcode)
+		dev_dbg(kfd_device, "ret = %d\n", retcode);
+
+	return retcode;
 }
 
 static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

PATCH 2/3 drm/amdkfd: reformat IOCTL definitions to drm-style

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the drm ioctls definition style.

---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
 include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
 2 files changed, 30 insertions(+), 22 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +8 -7
index 5460ad2..390385f 100644
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 
 
 	switch (cmd) {
-	case KFD_IOC_GET_VERSION:
+	case AMDKFD_IOC_GET_VERSION:
 		retcode = kfd_ioctl_get_version(filep, process, kdata);
 		break;
-	case KFD_IOC_CREATE_QUEUE:
+
+	case AMDKFD_IOC_CREATE_QUEUE:
 		retcode = kfd_ioctl_create_queue(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_DESTROY_QUEUE:
+	case AMDKFD_IOC_DESTROY_QUEUE:
 		retcode = kfd_ioctl_destroy_queue(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_SET_MEMORY_POLICY:
+	case AMDKFD_IOC_SET_MEMORY_POLICY:
 		retcode = kfd_ioctl_set_memory_policy(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_GET_CLOCK_COUNTERS:
+	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
 		retcode = kfd_ioctl_get_clock_counters(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_GET_PROCESS_APERTURES:
+	case AMDKFD_IOC_GET_PROCESS_APERTURES:
 		retcode = kfd_ioctl_get_process_apertures(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_UPDATE_QUEUE:
+	case AMDKFD_IOC_UPDATE_QUEUE:
 		retcode = kfd_ioctl_update_queue(filep, process,
 						kdata);
 		break;
include/uapi/linux/kfd_ioctl.h +22 -15
index 7acef41..05b53f6 100644
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
 	uint32_t pad;
 };
 
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)			_IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)		_IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)		_IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)		_IOWR(AMDKFD_IOCTL_BASE, nr, type)
 
-#define KFD_IOC_GET_VERSION \
-		_IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION			\
+		AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
 
-#define KFD_IOC_CREATE_QUEUE \
-		_IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE			\
+		AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
 
-#define KFD_IOC_DESTROY_QUEUE \
-	_IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE		\
+		AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
 
-#define KFD_IOC_SET_MEMORY_POLICY \
-	_IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY		\
+		AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
 
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-	_IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS		\
+		AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
 
-#define KFD_IOC_GET_PROCESS_APERTURES \
-	_IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
+#define AMDKFD_IOC_GET_PROCESS_APERTURES	\
+		AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
 
-#define KFD_IOC_UPDATE_QUEUE \
-	_IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
+#define AMDKFD_IOC_UPDATE_QUEUE			\
+		AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END			0x08
 
 #endif
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 4 lines

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the drm ioctls definition style.

You are aware that this is a kernel API breakage?

Quoted 95 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
  include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
  2 files changed, 30 insertions(+), 22 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +8 -7
index 5460ad2..390385f 100644
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 
 
  	switch (cmd) {
-	case KFD_IOC_GET_VERSION:
+	case AMDKFD_IOC_GET_VERSION:
  		retcode = kfd_ioctl_get_version(filep, process, kdata);
  		break;
-	case KFD_IOC_CREATE_QUEUE:
+
+	case AMDKFD_IOC_CREATE_QUEUE:
  		retcode = kfd_ioctl_create_queue(filep, process,
  						kdata);
  		break;
 
-	case KFD_IOC_DESTROY_QUEUE:
+	case AMDKFD_IOC_DESTROY_QUEUE:
  		retcode = kfd_ioctl_destroy_queue(filep, process,
  						kdata);
  		break;
 
-	case KFD_IOC_SET_MEMORY_POLICY:
+	case AMDKFD_IOC_SET_MEMORY_POLICY:
  		retcode = kfd_ioctl_set_memory_policy(filep, process,
  						kdata);
  		break;
 
-	case KFD_IOC_GET_CLOCK_COUNTERS:
+	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
  		retcode = kfd_ioctl_get_clock_counters(filep, process,
  						kdata);
  		break;
 
-	case KFD_IOC_GET_PROCESS_APERTURES:
+	case AMDKFD_IOC_GET_PROCESS_APERTURES:
  		retcode = kfd_ioctl_get_process_apertures(filep, process,
  						kdata);
  		break;
 
-	case KFD_IOC_UPDATE_QUEUE:
+	case AMDKFD_IOC_UPDATE_QUEUE:
  		retcode = kfd_ioctl_update_queue(filep, process,
  						kdata);
  		break;
include/uapi/linux/kfd_ioctl.h +15 -13
index 7acef41..05b53f6 100644
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
  	uint32_t pad;
  };
 
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)			_IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)		_IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)		_IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)		_IOWR(AMDKFD_IOCTL_BASE, nr, type)
 
-#define KFD_IOC_GET_VERSION \
-		_IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION			\
+		AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
 
-#define KFD_IOC_CREATE_QUEUE \
-		_IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE			\
+		AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
 
-#define KFD_IOC_DESTROY_QUEUE \
-	_IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE		\
+		AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
 
-#define KFD_IOC_SET_MEMORY_POLICY \
-	_IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY		\
+		AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
 
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-	_IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS		\
+		AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
 
-#define KFD_IOC_GET_PROCESS_APERTURES \
-	_IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)

include/uapi/linux/kfd_ioctl.h struct kfd_ioctl_get_process_apertures_args { · line 159

Quoted 10 lines
include/uapi/linux/kfd_ioctl.h +7 -2
+#define AMDKFD_IOC_GET_PROCESS_APERTURES	\
+		AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
 
-#define KFD_IOC_UPDATE_QUEUE \
-	_IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
+#define AMDKFD_IOC_UPDATE_QUEUE			\
+		AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END			0x08

If you rename everything to AMDKFD_* you probably want to do so as well for KFD_COMMAND_(START|END).

Regards,
Christian.

include/uapi/linux/kfd_ioctl.h struct kfd_ioctl_get_process_apertures_args { · line 160

Quoted 2 lines
include/uapi/linux/kfd_ioctl.h +0 -0
  #endif

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

Quoted 7 lines · Christian König

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 4 lines

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the drm ioctls definition style.

You are aware that this is a kernel API breakage?

Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ? and I want this patch-set to be inserted to 3.19 so there won't be API breakage afterwards.

Quoted 88 lines
Quoted 88 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
  include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
  2 files changed, 30 insertions(+), 22 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +8 -7
index 5460ad2..390385f 100644
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int
 cmd, unsigned long arg)
      switch (cmd) {
-    case KFD_IOC_GET_VERSION:
+    case AMDKFD_IOC_GET_VERSION:
          retcode = kfd_ioctl_get_version(filep, process, kdata);
          break;
-    case KFD_IOC_CREATE_QUEUE:
+
+    case AMDKFD_IOC_CREATE_QUEUE:
          retcode = kfd_ioctl_create_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_DESTROY_QUEUE:
+    case AMDKFD_IOC_DESTROY_QUEUE:
          retcode = kfd_ioctl_destroy_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_SET_MEMORY_POLICY:
+    case AMDKFD_IOC_SET_MEMORY_POLICY:
          retcode = kfd_ioctl_set_memory_policy(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_CLOCK_COUNTERS:
+    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
          retcode = kfd_ioctl_get_clock_counters(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_PROCESS_APERTURES:
+    case AMDKFD_IOC_GET_PROCESS_APERTURES:
          retcode = kfd_ioctl_get_process_apertures(filep, process,
                          kdata);
          break;
-    case KFD_IOC_UPDATE_QUEUE:
+    case AMDKFD_IOC_UPDATE_QUEUE:
          retcode = kfd_ioctl_update_queue(filep, process,
                          kdata);
          break;
include/uapi/linux/kfd_ioctl.h +17 -15
index 7acef41..05b53f6 100644
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
      uint32_t pad;
  };
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, type)
-#define KFD_IOC_GET_VERSION \
-        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION            \
+        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
-#define KFD_IOC_CREATE_QUEUE \
-        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE            \
+        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
-#define KFD_IOC_DESTROY_QUEUE \
-    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE        \
+        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
-#define KFD_IOC_SET_MEMORY_POLICY \
-    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY        \
+        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
+        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
-#define KFD_IOC_GET_PROCESS_APERTURES \
-    _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
+#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
+        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
-#define KFD_IOC_UPDATE_QUEUE \
-    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
Quoted 8 lines

include/uapi/linux/kfd_ioctl.h struct kfd_ioctl_get_process_apertures_args { · line 159

Quoted 5 lines
include/uapi/linux/kfd_ioctl.h +5 -0
+#define AMDKFD_IOC_UPDATE_QUEUE            \
+        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END            0x08

If you rename everything to AMDKFD_* you probably want to do so as well for KFD_COMMAND_(START|END).

Thanks, I'll do that.

Oded

Quoted 6 lines

Regards,
Christian.

Quoted 1 line
include/uapi/linux/kfd_ioctl.h +0 -0
  #endif

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#4 lore raw

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

Am 29.12.2014 um 14:17 schrieb Oded Gabbay:

Quoted 11 lines · Christian König
Quoted 7 lines

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 4 lines

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the drm ioctls definition style.

You are aware that this is a kernel API breakage?

Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ?

Strictly speaking only if you fix a bug with it, but since we are still pretty early in the cycle it's ok with we (especially since it looks like a valid cleanup). I'm just not sure what Dave/Linus think of this.

Christian.

Quoted 103 lines

and I want this patch-set to be inserted to 3.19 so there won't be API breakage afterwards.

Quoted 92 lines
Quoted 92 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
  include/uapi/linux/kfd_ioctl.h           | 37 
+++++++++++++++++++-------------
  2 files changed, 30 insertions(+), 22 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +0 -0
index 5460ad2..390385f 100644
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, 

unsigned int
cmd, unsigned long arg)

      switch (cmd) {
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +8 -7
-    case KFD_IOC_GET_VERSION:
+    case AMDKFD_IOC_GET_VERSION:
          retcode = kfd_ioctl_get_version(filep, process, kdata);
          break;
-    case KFD_IOC_CREATE_QUEUE:
+
+    case AMDKFD_IOC_CREATE_QUEUE:
          retcode = kfd_ioctl_create_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_DESTROY_QUEUE:
+    case AMDKFD_IOC_DESTROY_QUEUE:
          retcode = kfd_ioctl_destroy_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_SET_MEMORY_POLICY:
+    case AMDKFD_IOC_SET_MEMORY_POLICY:
          retcode = kfd_ioctl_set_memory_policy(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_CLOCK_COUNTERS:
+    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
          retcode = kfd_ioctl_get_clock_counters(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_PROCESS_APERTURES:
+    case AMDKFD_IOC_GET_PROCESS_APERTURES:
          retcode = kfd_ioctl_get_process_apertures(filep, process,
                          kdata);
          break;
-    case KFD_IOC_UPDATE_QUEUE:
+    case AMDKFD_IOC_UPDATE_QUEUE:
          retcode = kfd_ioctl_update_queue(filep, process,
                          kdata);
          break;
include/uapi/linux/kfd_ioctl.h +17 -15
index 7acef41..05b53f6 100644
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
      uint32_t pad;
  };
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, 
 type)
-#define KFD_IOC_GET_VERSION \
-        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION            \
+        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
-#define KFD_IOC_CREATE_QUEUE \
-        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE            \
+        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
-#define KFD_IOC_DESTROY_QUEUE \
-    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE        \
+        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
-#define KFD_IOC_SET_MEMORY_POLICY \
-    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY        \
+        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
+        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
-#define KFD_IOC_GET_PROCESS_APERTURES \
-    _IOR(KFD_IOC_MAGIC, 6, struct 
 kfd_ioctl_get_process_apertures_args)
+#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
+        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
-#define KFD_IOC_UPDATE_QUEUE \
-    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
Quoted 8 lines

include/uapi/linux/kfd_ioctl.h struct kfd_ioctl_get_process_apertures_args { · line 159

Quoted 5 lines
include/uapi/linux/kfd_ioctl.h +5 -0
+#define AMDKFD_IOC_UPDATE_QUEUE            \
+        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END            0x08

If you rename everything to AMDKFD_* you probably want to do so as well for KFD_COMMAND_(START|END).

Quoted 9 lines

Thanks, I'll do that.

Oded

Quoted 6 lines

Regards,
Christian.

Quoted 1 line
include/uapi/linux/kfd_ioctl.h +0 -0
  #endif

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#5 lore raw

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

Quoted 10 lines · Christian König

Am 29.12.2014 um 14:17 schrieb Oded Gabbay:

Quoted 9 lines · Christian König
Quoted 6 lines

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 4 lines

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the drm ioctls definition style.

Quoted 8 lines
Quoted 2 lines
Quoted 1 line

You are aware that this is a kernel API breakage?

Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ?

Strictly speaking only if you fix a bug with it, but since we are still pretty early in the cycle it's ok with we (especially since it looks like a valid cleanup). I'm just not sure what Dave/Linus think of this.

Christian.

Well, Dave said I should remove the error prints from the ioctl function and generally make it similar to drm ioctl handling. And as you said, this looks like a valid cleanup so I assume it would be ok from his POV to merge it for -rc2/3.

Oded

Quoted 104 lines
Quoted 104 lines

and I want this patch-set to be inserted to 3.19 so there won't be API breakage afterwards.

Quoted 89 lines
Quoted 89 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
  include/uapi/linux/kfd_ioctl.h           | 37
+++++++++++++++++++-------------
  2 files changed, 30 insertions(+), 22 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +8 -7
index 5460ad2..390385f 100644
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int
 cmd, unsigned long arg)
      switch (cmd) {
-    case KFD_IOC_GET_VERSION:
+    case AMDKFD_IOC_GET_VERSION:
          retcode = kfd_ioctl_get_version(filep, process, kdata);
          break;
-    case KFD_IOC_CREATE_QUEUE:
+
+    case AMDKFD_IOC_CREATE_QUEUE:
          retcode = kfd_ioctl_create_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_DESTROY_QUEUE:
+    case AMDKFD_IOC_DESTROY_QUEUE:
          retcode = kfd_ioctl_destroy_queue(filep, process,
                          kdata);
          break;
-    case KFD_IOC_SET_MEMORY_POLICY:
+    case AMDKFD_IOC_SET_MEMORY_POLICY:
          retcode = kfd_ioctl_set_memory_policy(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_CLOCK_COUNTERS:
+    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
          retcode = kfd_ioctl_get_clock_counters(filep, process,
                          kdata);
          break;
-    case KFD_IOC_GET_PROCESS_APERTURES:
+    case AMDKFD_IOC_GET_PROCESS_APERTURES:
          retcode = kfd_ioctl_get_process_apertures(filep, process,
                          kdata);
          break;
-    case KFD_IOC_UPDATE_QUEUE:
+    case AMDKFD_IOC_UPDATE_QUEUE:
          retcode = kfd_ioctl_update_queue(filep, process,
                          kdata);
          break;
include/uapi/linux/kfd_ioctl.h +17 -15
index 7acef41..05b53f6 100644
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
      uint32_t pad;
  };
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, type)
-#define KFD_IOC_GET_VERSION \
-        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION            \
+        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
-#define KFD_IOC_CREATE_QUEUE \
-        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE            \
+        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
-#define KFD_IOC_DESTROY_QUEUE \
-    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE        \
+        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
-#define KFD_IOC_SET_MEMORY_POLICY \
-    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY        \
+        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
+        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
-#define KFD_IOC_GET_PROCESS_APERTURES \
-    _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
+#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
+        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
-#define KFD_IOC_UPDATE_QUEUE \
-    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
Quoted 8 lines

include/uapi/linux/kfd_ioctl.h struct kfd_ioctl_get_process_apertures_args { · line 159

Quoted 5 lines
include/uapi/linux/kfd_ioctl.h +5 -0
+#define AMDKFD_IOC_UPDATE_QUEUE            \
+        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END            0x08

If you rename everything to AMDKFD_* you probably want to do so as well for KFD_COMMAND_(START|END).

Thanks, I'll do that.

Quoted 8 lines
Quoted 7 lines

Oded

Quoted 6 lines

Regards,
Christian.

Quoted 1 line
include/uapi/linux/kfd_ioctl.h +0 -0
  #endif

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#6 lore raw

PATCH 3/3 drm/amdkfd: rewrite kfd_ioctl() according to drm_ioctl()

In reply to Oded Gabbay

This patch changes kfd_ioctl() to be very similar to drm_ioctl().

The patch defines an array of amdkfd_ioctls, which maps IOCTL definition to the ioctl function.

The kfd_ioctl() uses that mapping to call the appropriate ioctl function, through a function pointer.

This patch also declares a new typedef for the ioctl function pointer.

---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 116 ++++++++++++++++++-------------
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  18 +++++
 2 files changed, 86 insertions(+), 48 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +68 -48
index 390385f..06693fa 100644
@@ -483,21 +483,79 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 	return 0;
 }
 
+#define AMDKFD_IOCTL_DEF(ioctl, _func, _flags) \
+	[_IOC_NR(ioctl)] = {.cmd = ioctl, .func = _func, .flags = _flags, .cmd_drv = 0, .name = #ioctl}
+
+/** Ioctl table */
+static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_VERSION,
+			kfd_ioctl_get_version, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_CREATE_QUEUE,
+			kfd_ioctl_create_queue, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_DESTROY_QUEUE,
+			kfd_ioctl_destroy_queue, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_SET_MEMORY_POLICY,
+			kfd_ioctl_set_memory_policy, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_CLOCK_COUNTERS,
+			kfd_ioctl_get_clock_counters, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_PROCESS_APERTURES,
+			kfd_ioctl_get_process_apertures, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_UPDATE_QUEUE,
+			kfd_ioctl_update_queue, 0),
+};
+
+#define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
+
 static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	struct kfd_process *process;
+	amdkfd_ioctl_t *func;
+	const struct amdkfd_ioctl_desc *ioctl = NULL;
+	unsigned int nr = _IOC_NR(cmd);
 	char stack_kdata[128];
 	char *kdata = NULL;
 	unsigned int usize, asize;
 	int retcode = -EINVAL;
 
-	dev_dbg(kfd_device,
-		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
-		cmd, _IOC_NR(cmd), arg);
+	if (nr >= AMDKFD_CORE_IOCTL_COUNT)
+		goto err_i1;
+
+	if ((nr >= KFD_COMMAND_START) && (nr < KFD_COMMAND_END)) {
+		u32 amdkfd_size;
+
+		ioctl = &amdkfd_ioctls[nr];
+
+		amdkfd_size = _IOC_SIZE(ioctl->cmd);
+		usize = asize = _IOC_SIZE(cmd);
+		if (amdkfd_size > asize)
+			asize = amdkfd_size;
+
+		cmd = ioctl->cmd;
+	} else
+		goto err_i1;
+
+	dev_dbg(kfd_device, "ioctl cmd 0x%x (#%d), arg 0x%lx\n", cmd, nr, arg);
 
 	process = kfd_get_process(current);
-	if (IS_ERR(process))
-		return PTR_ERR(process);
+	if (IS_ERR(process)) {
+		dev_dbg(kfd_device, "no process\n");
+		goto err_i1;
+	}
+
+	/* Do not trust userspace, use our own definition */
+	func = ioctl->func;
+
+	if (unlikely(!func)) {
+		dev_dbg(kfd_device, "no function\n");
+		retcode = -EINVAL;
+		goto err_i1;
+	}
 
 	if (cmd & (IOC_IN | IOC_OUT)) {
 		if (asize <= sizeof(stack_kdata)) {
@@ -522,55 +580,17 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		memset(kdata, 0, usize);
 	}
 
-
-	switch (cmd) {
-	case AMDKFD_IOC_GET_VERSION:
-		retcode = kfd_ioctl_get_version(filep, process, kdata);
-		break;
-
-	case AMDKFD_IOC_CREATE_QUEUE:
-		retcode = kfd_ioctl_create_queue(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_DESTROY_QUEUE:
-		retcode = kfd_ioctl_destroy_queue(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_SET_MEMORY_POLICY:
-		retcode = kfd_ioctl_set_memory_policy(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
-		retcode = kfd_ioctl_get_clock_counters(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_GET_PROCESS_APERTURES:
-		retcode = kfd_ioctl_get_process_apertures(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_UPDATE_QUEUE:
-		retcode = kfd_ioctl_update_queue(filep, process,
-						kdata);
-		break;
-
-	default:
-		dev_dbg(kfd_device,
-			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
-			cmd, arg);
-		retcode = -EINVAL;
-		break;
-	}
+	retcode = func(filep, process, kdata);
 
 	if (cmd & IOC_OUT)
 		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
 			retcode = -EFAULT;
 
 err_i1:
+	if (!ioctl)
+		dev_dbg(kfd_device, "invalid ioctl: pid=%d, cmd=0x%02x, nr=0x%02x\n",
+			  task_pid_nr(current), cmd, nr);
+
 	if (kdata != stack_kdata)
 		kfree(kdata);
 
drivers/gpu/drm/amd/amdkfd/kfd_priv.h +18 -0
index f9fb81e..a5edb29 100644
@@ -463,6 +463,24 @@ struct kfd_process {
 	bool is_32bit_user_mode;
 };
 
+/**
+ * Ioctl function type.
+ *
+ * \param filep pointer to file structure.
+ * \param p amdkfd process pointer.
+ * \param data pointer to arg that was copied from user.
+ */
+typedef int amdkfd_ioctl_t(struct file *filep, struct kfd_process *p,
+				void *data);
+
+struct amdkfd_ioctl_desc {
+	unsigned int cmd;
+	int flags;
+	amdkfd_ioctl_t *func;
+	unsigned int cmd_drv;
+	const char *name;
+};
+
 void kfd_process_create_wq(void);
 void kfd_process_destroy_wq(void);
 struct kfd_process *kfd_create_process(const struct task_struct *);
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#7 lore raw
In reply to Oded Gabbay

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 7 lines

This patch moves the copy_to_user() and copy_from_user() calls from the different ioctl functions in amdkfd to the general kfd_ioctl() function, as this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

In general sounds like a good idea to me and the patch is "Reviewed-by: Christian König <[email protected]>" for now.

What really questions me is why we need all this code duplication and can't reuse the DRM infrastructure for this. But that's more a problem in the long term.

Regards,
Christian.

Quoted 476 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
  1 file changed, 117 insertions(+), 117 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +110 -117
index 7d4974b..5460ad2 100644
@@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep)
  	return 0;
  }
 
-static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
+					void *data)
  {
-	struct kfd_ioctl_get_version_args args;
+	struct kfd_ioctl_get_version_args *args = data;
  	int err = 0;
 
-	args.major_version = KFD_IOCTL_MAJOR_VERSION;
-	args.minor_version = KFD_IOCTL_MINOR_VERSION;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		err = -EFAULT;
+	args->major_version = KFD_IOCTL_MAJOR_VERSION;
+	args->minor_version = KFD_IOCTL_MINOR_VERSION;
 
  	return err;
  }
@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties,
  	return 0;
  }
 
-static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
+					void *data)
  {
-	struct kfd_ioctl_create_queue_args args;
+	struct kfd_ioctl_create_queue_args *args = data;
  	struct kfd_dev *dev;
  	int err = 0;
  	unsigned int queue_id;
@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
  	memset(&q_properties, 0, sizeof(struct queue_properties));
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
  	pr_debug("kfd: creating queue ioctl\n");
 
-	err = set_queue_properties_from_user(&q_properties, &args);
+	err = set_queue_properties_from_user(&q_properties, args);
  	if (err)
  		return err;
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
  	if (dev == NULL)
  		return -EINVAL;
 
@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
  	pdd = kfd_bind_process_to_device(dev, p);
  	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
  		goto err_bind_process;
  	}
 
@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
  	if (err != 0)
  		goto err_create_queue;
 
-	args.queue_id = queue_id;
+	args->queue_id = queue_id;
 
  	/* Return gpu_id as doorbell offset for mmap usage */
-	args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
-
-	if (copy_to_user(arg, &args, sizeof(args))) {
-		err = -EFAULT;
-		goto err_copy_args_out;
-	}
+	args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
 
  	mutex_unlock(&p->mutex);
 
-	pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
+	pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
 
  	pr_debug("ring buffer address == 0x%016llX\n",
-			args.ring_base_address);
+			args->ring_base_address);
 
  	pr_debug("read ptr address    == 0x%016llX\n",
-			args.read_pointer_address);
+			args->read_pointer_address);
 
  	pr_debug("write ptr address   == 0x%016llX\n",
-			args.write_pointer_address);
+			args->write_pointer_address);
 
  	return 0;
 
-err_copy_args_out:
-	pqm_destroy_queue(&p->pqm, queue_id);
  err_create_queue:
  err_bind_process:
  	mutex_unlock(&p->mutex);
@@ -297,99 +284,90 @@ err_bind_process:
  }
 
  static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
  {
  	int retval;
-	struct kfd_ioctl_destroy_queue_args args;
-
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
+	struct kfd_ioctl_destroy_queue_args *args = data;
 
  	pr_debug("kfd: destroying queue id %d for PASID %d\n",
-				args.queue_id,
+				args->queue_id,
  				p->pasid);
 
  	mutex_lock(&p->mutex);
 
-	retval = pqm_destroy_queue(&p->pqm, args.queue_id);
+	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
 
  	mutex_unlock(&p->mutex);
  	return retval;
  }
 
  static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
  {
  	int retval;
-	struct kfd_ioctl_update_queue_args args;
+	struct kfd_ioctl_update_queue_args *args = data;
  	struct queue_properties properties;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
+	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
  		pr_err("kfd: queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
  		return -EINVAL;
  	}
 
-	if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
+	if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
  		pr_err("kfd: queue priority must be between 0 to KFD_MAX_QUEUE_PRIORITY\n");
  		return -EINVAL;
  	}
 
-	if ((args.ring_base_address) &&
+	if ((args->ring_base_address) &&
  		(!access_ok(VERIFY_WRITE,
-			(const void __user *) args.ring_base_address,
+			(const void __user *) args->ring_base_address,
  			sizeof(uint64_t)))) {
  		pr_err("kfd: can't access ring base address\n");
  		return -EFAULT;
  	}
 
-	if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
+	if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
  		pr_err("kfd: ring size must be a power of 2 or 0\n");
  		return -EINVAL;
  	}
 
-	properties.queue_address = args.ring_base_address;
-	properties.queue_size = args.ring_size;
-	properties.queue_percent = args.queue_percentage;
-	properties.priority = args.queue_priority;
+	properties.queue_address = args->ring_base_address;
+	properties.queue_size = args->ring_size;
+	properties.queue_percent = args->queue_percentage;
+	properties.priority = args->queue_priority;
 
  	pr_debug("kfd: updating queue id %d for PASID %d\n",
-			args.queue_id, p->pasid);
+			args->queue_id, p->pasid);
 
  	mutex_lock(&p->mutex);
 
-	retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
+	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
 
  	mutex_unlock(&p->mutex);
 
  	return retval;
  }
 
-static long kfd_ioctl_set_memory_policy(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_set_memory_policy(struct file *filep,
+					struct kfd_process *p, void *data)
  {
-	struct kfd_ioctl_set_memory_policy_args args;
+	struct kfd_ioctl_set_memory_policy_args *args = data;
  	struct kfd_dev *dev;
  	int err = 0;
  	struct kfd_process_device *pdd;
  	enum cache_policy default_policy, alternate_policy;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
  		return -EINVAL;
  	}
 
-	if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
  		return -EINVAL;
  	}
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
  	if (dev == NULL)
  		return -EINVAL;
 
@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
 
  	pdd = kfd_bind_process_to_device(dev, p);
  	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
  		goto out;
  	}
 
-	default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+	default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
  			 ? cache_policy_coherent : cache_policy_noncoherent;
 
  	alternate_policy =
-		(args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+		(args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
  		   ? cache_policy_coherent : cache_policy_noncoherent;
 
  	if (!dev->dqm->set_cache_memory_policy(dev->dqm,
  				&pdd->qpd,
  				default_policy,
  				alternate_policy,
-				(void __user *)args.alternate_aperture_base,
-				args.alternate_aperture_size))
+				(void __user *)args->alternate_aperture_base,
+				args->alternate_aperture_size))
  		err = -EINVAL;
 
  out:
@@ -422,53 +400,44 @@ out:
  	return err;
  }
 
-static long kfd_ioctl_get_clock_counters(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_get_clock_counters(struct file *filep,
+				struct kfd_process *p, void *data)
  {
-	struct kfd_ioctl_get_clock_counters_args args;
+	struct kfd_ioctl_get_clock_counters_args *args = data;
  	struct kfd_dev *dev;
  	struct timespec time;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
  	if (dev == NULL)
  		return -EINVAL;
 
  	/* Reading GPU clock counter from KGD */
-	args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
+	args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
 
  	/* No access to rdtsc. Using raw monotonic time */
  	getrawmonotonic(&time);
-	args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
 
  	get_monotonic_boottime(&time);
-	args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
 
  	/* Since the counter is in nano-seconds we use 1GHz frequency */
-	args.system_clock_freq = 1000000000;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
+	args->system_clock_freq = 1000000000;
 
  	return 0;
  }
 
 
  static int kfd_ioctl_get_process_apertures(struct file *filp,
-				struct kfd_process *p, void __user *arg)
+				struct kfd_process *p, void *data)
  {
-	struct kfd_ioctl_get_process_apertures_args args;
+	struct kfd_ioctl_get_process_apertures_args *args = data;
  	struct kfd_process_device_apertures *pAperture;
  	struct kfd_process_device *pdd;
 
  	dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	args.num_of_nodes = 0;
+	args->num_of_nodes = 0;
 
  	mutex_lock(&p->mutex);
 
@@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
  		/* Run over all pdd of the process */
  		pdd = kfd_get_first_process_device_data(p);
  		do {
-			pAperture = &args.process_apertures[args.num_of_nodes];
+			pAperture =
+				&args->process_apertures[args->num_of_nodes];
  			pAperture->gpu_id = pdd->dev->id;
  			pAperture->lds_base = pdd->lds_base;
  			pAperture->lds_limit = pdd->lds_limit;
@@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
  			pAperture->scratch_limit = pdd->scratch_limit;
 
  			dev_dbg(kfd_device,
-				"node id %u\n", args.num_of_nodes);
+				"node id %u\n", args->num_of_nodes);
  			dev_dbg(kfd_device,
  				"gpu id %u\n", pdd->dev->id);
  			dev_dbg(kfd_device,
@@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
  			dev_dbg(kfd_device,
  				"scratch_limit %llX\n", pdd->scratch_limit);
 
-			args.num_of_nodes++;
+			args->num_of_nodes++;
  		} while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
-				(args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
+				(args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
  	}
 
  	mutex_unlock(&p->mutex);
 
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
-
  	return 0;
  }
 
  static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
  {
  	struct kfd_process *process;
-	long err = -EINVAL;
+	char stack_kdata[128];
+	char *kdata = NULL;
+	unsigned int usize, asize;
+	int retcode = -EINVAL;
 
  	dev_dbg(kfd_device,
  		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
  	if (IS_ERR(process))
  		return PTR_ERR(process);
 
+	if (cmd & (IOC_IN | IOC_OUT)) {
+		if (asize <= sizeof(stack_kdata)) {
+			kdata = stack_kdata;
+		} else {
+			kdata = kmalloc(asize, GFP_KERNEL);
+			if (!kdata) {
+				retcode = -ENOMEM;
+				goto err_i1;
+			}
+		}
+		if (asize > usize)
+			memset(kdata + usize, 0, asize - usize);
+	}
+
+	if (cmd & IOC_IN) {
+		if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
+			retcode = -EFAULT;
+			goto err_i1;
+		}
+	} else if (cmd & IOC_OUT) {
+		memset(kdata, 0, usize);
+	}
+
+
  	switch (cmd) {
  	case KFD_IOC_GET_VERSION:
-		err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
+		retcode = kfd_ioctl_get_version(filep, process, kdata);
  		break;
  	case KFD_IOC_CREATE_QUEUE:
-		err = kfd_ioctl_create_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_create_queue(filep, process,
+						kdata);
  		break;
 
  	case KFD_IOC_DESTROY_QUEUE:
-		err = kfd_ioctl_destroy_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_destroy_queue(filep, process,
+						kdata);
  		break;
 
  	case KFD_IOC_SET_MEMORY_POLICY:
-		err = kfd_ioctl_set_memory_policy(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_set_memory_policy(filep, process,
+						kdata);
  		break;
 
  	case KFD_IOC_GET_CLOCK_COUNTERS:
-		err = kfd_ioctl_get_clock_counters(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_clock_counters(filep, process,
+						kdata);
  		break;
 
  	case KFD_IOC_GET_PROCESS_APERTURES:
-		err = kfd_ioctl_get_process_apertures(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_process_apertures(filep, process,
+						kdata);
  		break;
 
  	case KFD_IOC_UPDATE_QUEUE:
-		err = kfd_ioctl_update_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_update_queue(filep, process,
+						kdata);
  		break;
 
  	default:
-		dev_err(kfd_device,
+		dev_dbg(kfd_device,
  			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
  			cmd, arg);
-		err = -EINVAL;
+		retcode = -EINVAL;
  		break;
  	}
 
-	if (err < 0)
-		dev_err(kfd_device,
-			"ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
-			err, cmd, _IOC_NR(cmd));
+	if (cmd & IOC_OUT)
+		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
+			retcode = -EFAULT;
 
-	return err;
+err_i1:

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) · line 582

Quoted 10 lines
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +7 -0
+	if (kdata != stack_kdata)
+		kfree(kdata);
+
+	if (retcode)
+		dev_dbg(kfd_device, "ret = %d\n", retcode);
+
+	return retcode;
  }
 
  static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#8 lore raw
Quoted 7 lines · Christian König

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 6 lines

This patch moves the copy_to_user() and copy_from_user() calls from the different ioctl functions in amdkfd to the general kfd_ioctl() function, as this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

Quoted 8 lines
Quoted 1 line

In general sounds like a good idea to me and the patch is "Reviewed-by:
Christian König <[email protected]>" for now.

What really questions me is why we need all this code duplication and can't reuse the DRM infrastructure for this. But that's more a problem in the long term.

Do you mean registering as DRM IOCTL ?
Or do you mean something else ?

If it is the former, than I think the main problem is that we use different devices (/dev/kfd vs. /dev/dri/)

If it is the latter, could you give me more specifics ?

Oded

Quoted 416 lines

Regards,
Christian.

Quoted 413 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
  1 file changed, 117 insertions(+), 117 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +111 -117
index 7d4974b..5460ad2 100644
@@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file
 *filep)
      return 0;
  }
-static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
-                    void __user *arg)
+static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
+                    void *data)
  {
-    struct kfd_ioctl_get_version_args args;
+    struct kfd_ioctl_get_version_args *args = data;
      int err = 0;
-    args.major_version = KFD_IOCTL_MAJOR_VERSION;
-    args.minor_version = KFD_IOCTL_MINOR_VERSION;
-
-    if (copy_to_user(arg, &args, sizeof(args)))
-        err = -EFAULT;
+    args->major_version = KFD_IOCTL_MAJOR_VERSION;
+    args->minor_version = KFD_IOCTL_MINOR_VERSION;
      return err;
  }
@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct
 queue_properties *q_properties,
      return 0;
  }
-static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
-                    void __user *arg)
+static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
+                    void *data)
  {
-    struct kfd_ioctl_create_queue_args args;
+    struct kfd_ioctl_create_queue_args *args = data;
      struct kfd_dev *dev;
      int err = 0;
      unsigned int queue_id;
@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep,
 struct kfd_process *p,
      memset(&q_properties, 0, sizeof(struct queue_properties));
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
      pr_debug("kfd: creating queue ioctl\n");
-    err = set_queue_properties_from_user(&q_properties, &args);
+    err = set_queue_properties_from_user(&q_properties, args);
      if (err)
          return err;
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep,
 struct kfd_process *p,
      pdd = kfd_bind_process_to_device(dev, p);
      if (IS_ERR(pdd)) {
-        err = PTR_ERR(pdd);
+        err = -ESRCH;
          goto err_bind_process;
      }
@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep,
 struct kfd_process *p,
      if (err != 0)
          goto err_create_queue;
-    args.queue_id = queue_id;
+    args->queue_id = queue_id;
      /* Return gpu_id as doorbell offset for mmap usage */
-    args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
-
-    if (copy_to_user(arg, &args, sizeof(args))) {
-        err = -EFAULT;
-        goto err_copy_args_out;
-    }
+    args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
      mutex_unlock(&p->mutex);
-    pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
+    pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
      pr_debug("ring buffer address == 0x%016llX\n",
-            args.ring_base_address);
+            args->ring_base_address);
      pr_debug("read ptr address    == 0x%016llX\n",
-            args.read_pointer_address);
+            args->read_pointer_address);
      pr_debug("write ptr address   == 0x%016llX\n",
-            args.write_pointer_address);
+            args->write_pointer_address);
      return 0;
-err_copy_args_out:
-    pqm_destroy_queue(&p->pqm, queue_id);
  err_create_queue:
  err_bind_process:
      mutex_unlock(&p->mutex);
@@ -297,99 +284,90 @@ err_bind_process:
  }
  static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
-                    void __user *arg)
+                    void *data)
  {
      int retval;
-    struct kfd_ioctl_destroy_queue_args args;
-
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
+    struct kfd_ioctl_destroy_queue_args *args = data;
      pr_debug("kfd: destroying queue id %d for PASID %d\n",
-                args.queue_id,
+                args->queue_id,
                  p->pasid);
      mutex_lock(&p->mutex);
-    retval = pqm_destroy_queue(&p->pqm, args.queue_id);
+    retval = pqm_destroy_queue(&p->pqm, args->queue_id);
      mutex_unlock(&p->mutex);
      return retval;
  }
  static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
-                    void __user *arg)
+                    void *data)
  {
      int retval;
-    struct kfd_ioctl_update_queue_args args;
+    struct kfd_ioctl_update_queue_args *args = data;
      struct queue_properties properties;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
+    if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
          pr_err("kfd: queue percentage must be between 0 to
 KFD_MAX_QUEUE_PERCENTAGE\n");
          return -EINVAL;
      }
-    if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
+    if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
          pr_err("kfd: queue priority must be between 0 to
 KFD_MAX_QUEUE_PRIORITY\n");
          return -EINVAL;
      }
-    if ((args.ring_base_address) &&
+    if ((args->ring_base_address) &&
          (!access_ok(VERIFY_WRITE,
-            (const void __user *) args.ring_base_address,
+            (const void __user *) args->ring_base_address,
              sizeof(uint64_t)))) {
          pr_err("kfd: can't access ring base address\n");
          return -EFAULT;
      }
-    if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
+    if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
          pr_err("kfd: ring size must be a power of 2 or 0\n");
          return -EINVAL;
      }
-    properties.queue_address = args.ring_base_address;
-    properties.queue_size = args.ring_size;
-    properties.queue_percent = args.queue_percentage;
-    properties.priority = args.queue_priority;
+    properties.queue_address = args->ring_base_address;
+    properties.queue_size = args->ring_size;
+    properties.queue_percent = args->queue_percentage;
+    properties.priority = args->queue_priority;
      pr_debug("kfd: updating queue id %d for PASID %d\n",
-            args.queue_id, p->pasid);
+            args->queue_id, p->pasid);
      mutex_lock(&p->mutex);
-    retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
+    retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
      mutex_unlock(&p->mutex);
      return retval;
  }
-static long kfd_ioctl_set_memory_policy(struct file *filep,
-                struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_set_memory_policy(struct file *filep,
+                    struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_set_memory_policy_args args;
+    struct kfd_ioctl_set_memory_policy_args *args = data;
      struct kfd_dev *dev;
      int err = 0;
      struct kfd_process_device *pdd;
      enum cache_policy default_policy, alternate_policy;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
-        && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+    if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
+        && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
          return -EINVAL;
      }
-    if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
-        && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+    if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
+        && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
          return -EINVAL;
      }
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
      pdd = kfd_bind_process_to_device(dev, p);
      if (IS_ERR(pdd)) {
-        err = PTR_ERR(pdd);
+        err = -ESRCH;
          goto out;
      }
-    default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+    default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
               ? cache_policy_coherent : cache_policy_noncoherent;
      alternate_policy =
-        (args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+        (args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
             ? cache_policy_coherent : cache_policy_noncoherent;
      if (!dev->dqm->set_cache_memory_policy(dev->dqm,
                  &pdd->qpd,
                  default_policy,
                  alternate_policy,
-                (void __user *)args.alternate_aperture_base,
-                args.alternate_aperture_size))
+                (void __user *)args->alternate_aperture_base,
+                args->alternate_aperture_size))
          err = -EINVAL;
  out:
@@ -422,53 +400,44 @@ out:
      return err;
  }
-static long kfd_ioctl_get_clock_counters(struct file *filep,
-                struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_get_clock_counters(struct file *filep,
+                struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_get_clock_counters_args args;
+    struct kfd_ioctl_get_clock_counters_args *args = data;
      struct kfd_dev *dev;
      struct timespec time;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
      /* Reading GPU clock counter from KGD */
-    args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
+    args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
      /* No access to rdtsc. Using raw monotonic time */
      getrawmonotonic(&time);
-    args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
+    args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
      get_monotonic_boottime(&time);
-    args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
+    args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
      /* Since the counter is in nano-seconds we use 1GHz frequency */
-    args.system_clock_freq = 1000000000;
-
-    if (copy_to_user(arg, &args, sizeof(args)))
-        return -EFAULT;
+    args->system_clock_freq = 1000000000;
      return 0;
  }
  static int kfd_ioctl_get_process_apertures(struct file *filp,
-                struct kfd_process *p, void __user *arg)
+                struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_get_process_apertures_args args;
+    struct kfd_ioctl_get_process_apertures_args *args = data;
      struct kfd_process_device_apertures *pAperture;
      struct kfd_process_device *pdd;
      dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    args.num_of_nodes = 0;
+    args->num_of_nodes = 0;
      mutex_lock(&p->mutex);
@@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
          /* Run over all pdd of the process */
          pdd = kfd_get_first_process_device_data(p);
          do {
-            pAperture = &args.process_apertures[args.num_of_nodes];
+            pAperture =
+                &args->process_apertures[args->num_of_nodes];
              pAperture->gpu_id = pdd->dev->id;
              pAperture->lds_base = pdd->lds_base;
              pAperture->lds_limit = pdd->lds_limit;
@@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
              pAperture->scratch_limit = pdd->scratch_limit;
              dev_dbg(kfd_device,
-                "node id %u\n", args.num_of_nodes);
+                "node id %u\n", args->num_of_nodes);
              dev_dbg(kfd_device,
                  "gpu id %u\n", pdd->dev->id);
              dev_dbg(kfd_device,
@@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file
 *filp,
              dev_dbg(kfd_device,
                  "scratch_limit %llX\n", pdd->scratch_limit);
-            args.num_of_nodes++;
+            args->num_of_nodes++;
          } while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
-                (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
+                (args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
      }
      mutex_unlock(&p->mutex);
-    if (copy_to_user(arg, &args, sizeof(args)))
-        return -EFAULT;
-
      return 0;
  }
  static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
  {
      struct kfd_process *process;
-    long err = -EINVAL;
+    char stack_kdata[128];
+    char *kdata = NULL;
+    unsigned int usize, asize;
+    int retcode = -EINVAL;
      dev_dbg(kfd_device,
          "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int
 cmd, unsigned long arg)
      if (IS_ERR(process))
          return PTR_ERR(process);
+    if (cmd & (IOC_IN | IOC_OUT)) {
+        if (asize <= sizeof(stack_kdata)) {
+            kdata = stack_kdata;
+        } else {
+            kdata = kmalloc(asize, GFP_KERNEL);
+            if (!kdata) {
+                retcode = -ENOMEM;
+                goto err_i1;
+            }
+        }
+        if (asize > usize)
+            memset(kdata + usize, 0, asize - usize);
+    }
+
+    if (cmd & IOC_IN) {
+        if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
+            retcode = -EFAULT;
+            goto err_i1;
+        }
+    } else if (cmd & IOC_OUT) {
+        memset(kdata, 0, usize);
+    }
+
+
      switch (cmd) {
      case KFD_IOC_GET_VERSION:
-        err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
+        retcode = kfd_ioctl_get_version(filep, process, kdata);
          break;
      case KFD_IOC_CREATE_QUEUE:
-        err = kfd_ioctl_create_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_create_queue(filep, process,
+                        kdata);
          break;
      case KFD_IOC_DESTROY_QUEUE:
-        err = kfd_ioctl_destroy_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_destroy_queue(filep, process,
+                        kdata);
          break;
      case KFD_IOC_SET_MEMORY_POLICY:
-        err = kfd_ioctl_set_memory_policy(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_set_memory_policy(filep, process,
+                        kdata);
          break;
      case KFD_IOC_GET_CLOCK_COUNTERS:
-        err = kfd_ioctl_get_clock_counters(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_get_clock_counters(filep, process,
+                        kdata);
          break;
      case KFD_IOC_GET_PROCESS_APERTURES:
-        err = kfd_ioctl_get_process_apertures(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_get_process_apertures(filep, process,
+                        kdata);
          break;
      case KFD_IOC_UPDATE_QUEUE:
-        err = kfd_ioctl_update_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_update_queue(filep, process,
+                        kdata);
          break;
      default:
-        dev_err(kfd_device,
+        dev_dbg(kfd_device,
              "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
              cmd, arg);
-        err = -EINVAL;
+        retcode = -EINVAL;
          break;
      }
-    if (err < 0)
-        dev_err(kfd_device,
-            "ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
-            err, cmd, _IOC_NR(cmd));
+    if (cmd & IOC_OUT)
+        if (copy_to_user((void __user *)arg, kdata, usize) != 0)
+            retcode = -EFAULT;
-    return err;
+err_i1:
+    if (kdata != stack_kdata)
Quoted 9 lines

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) · line 582

Quoted 8 lines
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +6 -0
+        kfree(kdata);
+
+    if (retcode)
+        dev_dbg(kfd_device, "ret = %d\n", retcode);
+
+    return retcode;
  }
  static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#9 lore raw

Am 29.12.2014 um 14:22 schrieb Oded Gabbay:

Quoted 15 lines · Christian König
Quoted 12 lines

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:

Quoted 7 lines

This patch moves the copy_to_user() and copy_from_user() calls from the different ioctl functions in amdkfd to the general kfd_ioctl() function, as this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

In general sounds like a good idea to me and the patch is "Reviewed-by:
Christian König <[email protected]>" for now.

Quoted 7 lines
Quoted 3 lines

What really questions me is why we need all this code duplication and can't reuse the DRM infrastructure for this. But that's more a problem in the long term.

Do you mean registering as DRM IOCTL ?
Or do you mean something else ?

If it is the former, than I think the main problem is that we use different devices (/dev/kfd vs. /dev/dri/)

Ah, yes of course. I simply keep forgetting that we have two device nodes for the same physical hardware.

Christian.

Quoted 456 lines

If it is the latter, could you give me more specifics ?

Oded

Quoted 451 lines

Regards,
Christian.

Quoted 447 lines
---
  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 
+++++++++++++++----------------
  1 file changed, 117 insertions(+), 117 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +0 -0
index 7d4974b..5460ad2 100644
@@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, 

struct file
*filep)

      return 0;

}
-static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +1 -1
-                    void __user *arg)
+static int kfd_ioctl_get_version(struct file *filep, struct 

kfd_process *p,

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +7 -9
+                    void *data)
  {
-    struct kfd_ioctl_get_version_args args;
+    struct kfd_ioctl_get_version_args *args = data;
      int err = 0;
-    args.major_version = KFD_IOCTL_MAJOR_VERSION;
-    args.minor_version = KFD_IOCTL_MINOR_VERSION;
-
-    if (copy_to_user(arg, &args, sizeof(args)))
-        err = -EFAULT;
+    args->major_version = KFD_IOCTL_MAJOR_VERSION;
+    args->minor_version = KFD_IOCTL_MINOR_VERSION;
      return err;
  }
@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct
 queue_properties *q_properties,
      return 0;
  }
-static long kfd_ioctl_create_queue(struct file *filep, struct 
 kfd_process *p,
-                    void __user *arg)
+static int kfd_ioctl_create_queue(struct file *filep, struct 
 kfd_process *p,
+                    void *data)
  {
-    struct kfd_ioctl_create_queue_args args;
+    struct kfd_ioctl_create_queue_args *args = data;
      struct kfd_dev *dev;
      int err = 0;
      unsigned int queue_id;
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +52 -79
@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file 
 *filep,
 struct kfd_process *p,
      memset(&q_properties, 0, sizeof(struct queue_properties));
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
      pr_debug("kfd: creating queue ioctl\n");
-    err = set_queue_properties_from_user(&q_properties, &args);
+    err = set_queue_properties_from_user(&q_properties, args);
      if (err)
          return err;
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file 
 *filep,
 struct kfd_process *p,
      pdd = kfd_bind_process_to_device(dev, p);
      if (IS_ERR(pdd)) {
-        err = PTR_ERR(pdd);
+        err = -ESRCH;
          goto err_bind_process;
      }
@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file 
 *filep,
 struct kfd_process *p,
      if (err != 0)
          goto err_create_queue;
-    args.queue_id = queue_id;
+    args->queue_id = queue_id;
      /* Return gpu_id as doorbell offset for mmap usage */
-    args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
-
-    if (copy_to_user(arg, &args, sizeof(args))) {
-        err = -EFAULT;
-        goto err_copy_args_out;
-    }
+    args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
      mutex_unlock(&p->mutex);
-    pr_debug("kfd: queue id %d was created successfully\n", 
 args.queue_id);
+    pr_debug("kfd: queue id %d was created successfully\n", 
 args->queue_id);
      pr_debug("ring buffer address == 0x%016llX\n",
-            args.ring_base_address);
+            args->ring_base_address);
      pr_debug("read ptr address    == 0x%016llX\n",
-            args.read_pointer_address);
+            args->read_pointer_address);
      pr_debug("write ptr address   == 0x%016llX\n",
-            args.write_pointer_address);
+            args->write_pointer_address);
      return 0;
-err_copy_args_out:
-    pqm_destroy_queue(&p->pqm, queue_id);
  err_create_queue:
  err_bind_process:
      mutex_unlock(&p->mutex);
@@ -297,99 +284,90 @@ err_bind_process:
  }
  static int kfd_ioctl_destroy_queue(struct file *filp, struct 
 kfd_process *p,
-                    void __user *arg)
+                    void *data)
  {
      int retval;
-    struct kfd_ioctl_destroy_queue_args args;
-
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
+    struct kfd_ioctl_destroy_queue_args *args = data;
      pr_debug("kfd: destroying queue id %d for PASID %d\n",
-                args.queue_id,
+                args->queue_id,
                  p->pasid);
      mutex_lock(&p->mutex);
-    retval = pqm_destroy_queue(&p->pqm, args.queue_id);
+    retval = pqm_destroy_queue(&p->pqm, args->queue_id);
      mutex_unlock(&p->mutex);
      return retval;
  }
  static int kfd_ioctl_update_queue(struct file *filp, struct 
 kfd_process *p,
-                    void __user *arg)
+                    void *data)
  {
      int retval;
-    struct kfd_ioctl_update_queue_args args;
+    struct kfd_ioctl_update_queue_args *args = data;
      struct queue_properties properties;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
+    if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
          pr_err("kfd: queue percentage must be between 0 to
 KFD_MAX_QUEUE_PERCENTAGE\n");
          return -EINVAL;
      }
-    if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
+    if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
          pr_err("kfd: queue priority must be between 0 to
 KFD_MAX_QUEUE_PRIORITY\n");
          return -EINVAL;
      }
-    if ((args.ring_base_address) &&
+    if ((args->ring_base_address) &&
          (!access_ok(VERIFY_WRITE,
-            (const void __user *) args.ring_base_address,
+            (const void __user *) args->ring_base_address,
              sizeof(uint64_t)))) {
          pr_err("kfd: can't access ring base address\n");
          return -EFAULT;
      }
-    if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
+    if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
          pr_err("kfd: ring size must be a power of 2 or 0\n");
          return -EINVAL;
      }
-    properties.queue_address = args.ring_base_address;
-    properties.queue_size = args.ring_size;
-    properties.queue_percent = args.queue_percentage;
-    properties.priority = args.queue_priority;
+    properties.queue_address = args->ring_base_address;
+    properties.queue_size = args->ring_size;
+    properties.queue_percent = args->queue_percentage;
+    properties.priority = args->queue_priority;
      pr_debug("kfd: updating queue id %d for PASID %d\n",
-            args.queue_id, p->pasid);
+            args->queue_id, p->pasid);
      mutex_lock(&p->mutex);
-    retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
+    retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
      mutex_unlock(&p->mutex);
      return retval;
  }
-static long kfd_ioctl_set_memory_policy(struct file *filep,
-                struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_set_memory_policy(struct file *filep,
+                    struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_set_memory_policy_args args;
+    struct kfd_ioctl_set_memory_policy_args *args = data;
      struct kfd_dev *dev;
      int err = 0;
      struct kfd_process_device *pdd;
      enum cache_policy default_policy, alternate_policy;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
-        && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+    if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
+        && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
          return -EINVAL;
      }
-    if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
-        && args.alternate_policy != 
 KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+    if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
+        && args->alternate_policy != 
 KFD_IOC_CACHE_POLICY_NONCOHERENT) {
          return -EINVAL;
      }
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct 
 file *filep,
      pdd = kfd_bind_process_to_device(dev, p);
      if (IS_ERR(pdd)) {
-        err = PTR_ERR(pdd);
+        err = -ESRCH;
          goto out;
      }
-    default_policy = (args.default_policy == 
 KFD_IOC_CACHE_POLICY_COHERENT)
+    default_policy = (args->default_policy == 
 KFD_IOC_CACHE_POLICY_COHERENT)
               ? cache_policy_coherent : cache_policy_noncoherent;
      alternate_policy =
-        (args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+        (args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
             ? cache_policy_coherent : cache_policy_noncoherent;
      if (!dev->dqm->set_cache_memory_policy(dev->dqm,
                  &pdd->qpd,
                  default_policy,
                  alternate_policy,
-                (void __user *)args.alternate_aperture_base,
-                args.alternate_aperture_size))
+                (void __user *)args->alternate_aperture_base,
+                args->alternate_aperture_size))
          err = -EINVAL;
  out:
@@ -422,53 +400,44 @@ out:
      return err;
  }
-static long kfd_ioctl_get_clock_counters(struct file *filep,
-                struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_get_clock_counters(struct file *filep,
+                struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_get_clock_counters_args args;
+    struct kfd_ioctl_get_clock_counters_args *args = data;
      struct kfd_dev *dev;
      struct timespec time;
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    dev = kfd_device_by_id(args.gpu_id);
+    dev = kfd_device_by_id(args->gpu_id);
      if (dev == NULL)
          return -EINVAL;
      /* Reading GPU clock counter from KGD */
-    args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
+    args->gpu_clock_counter = 
 kfd2kgd->get_gpu_clock_counter(dev->kgd);
      /* No access to rdtsc. Using raw monotonic time */
      getrawmonotonic(&time);
-    args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
+    args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
      get_monotonic_boottime(&time);
-    args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
+    args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
      /* Since the counter is in nano-seconds we use 1GHz frequency */
-    args.system_clock_freq = 1000000000;
-
-    if (copy_to_user(arg, &args, sizeof(args)))
-        return -EFAULT;
+    args->system_clock_freq = 1000000000;
      return 0;
  }
  static int kfd_ioctl_get_process_apertures(struct file *filp,
-                struct kfd_process *p, void __user *arg)
+                struct kfd_process *p, void *data)
  {
-    struct kfd_ioctl_get_process_apertures_args args;
+    struct kfd_ioctl_get_process_apertures_args *args = data;
      struct kfd_process_device_apertures *pAperture;
      struct kfd_process_device *pdd;
      dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
-    if (copy_from_user(&args, arg, sizeof(args)))
-        return -EFAULT;
-
-    args.num_of_nodes = 0;
+    args->num_of_nodes = 0;
      mutex_lock(&p->mutex);
@@ -477,7 +446,8 @@ static int 
 kfd_ioctl_get_process_apertures(struct file *filp,
          /* Run over all pdd of the process */
          pdd = kfd_get_first_process_device_data(p);
          do {
-            pAperture = &args.process_apertures[args.num_of_nodes];
+            pAperture =
+ &args->process_apertures[args->num_of_nodes];
              pAperture->gpu_id = pdd->dev->id;
              pAperture->lds_base = pdd->lds_base;
              pAperture->lds_limit = pdd->lds_limit;
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +1 -1
@@ -487,7 +457,7 @@ static int 
 kfd_ioctl_get_process_apertures(struct file *filp,
              pAperture->scratch_limit = pdd->scratch_limit;
              dev_dbg(kfd_device,
-                "node id %u\n", args.num_of_nodes);
+                "node id %u\n", args->num_of_nodes);
              dev_dbg(kfd_device,
                  "gpu id %u\n", pdd->dev->id);
              dev_dbg(kfd_device,
@@ -503,23 +473,23 @@ static int 

kfd_ioctl_get_process_apertures(struct file
*filp,

              dev_dbg(kfd_device,
                  "scratch_limit %llX\n", pdd->scratch_limit);
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +1 -1
-            args.num_of_nodes++;
+            args->num_of_nodes++;
          } while ((pdd = kfd_get_next_process_device_data(p, pdd)) 

!= NULL &&

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +1 -4
-                (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
+                (args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
      }
      mutex_unlock(&p->mutex);
-    if (copy_to_user(arg, &args, sizeof(args)))
-        return -EFAULT;
-
      return 0;
  }
  static long kfd_ioctl(struct file *filep, unsigned int cmd, 

unsigned long arg)

  {
      struct kfd_process *process;
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +4 -1
-    long err = -EINVAL;
+    char stack_kdata[128];
+    char *kdata = NULL;
+    unsigned int usize, asize;
+    int retcode = -EINVAL;
      dev_dbg(kfd_device,
          "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, 

unsigned int
cmd, unsigned long arg)

      if (IS_ERR(process))
          return PTR_ERR(process);
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +24 -1
+    if (cmd & (IOC_IN | IOC_OUT)) {
+        if (asize <= sizeof(stack_kdata)) {
+            kdata = stack_kdata;
+        } else {
+            kdata = kmalloc(asize, GFP_KERNEL);
+            if (!kdata) {
+                retcode = -ENOMEM;
+                goto err_i1;
+            }
+        }
+        if (asize > usize)
+            memset(kdata + usize, 0, asize - usize);
+    }
+
+    if (cmd & IOC_IN) {
+        if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
+            retcode = -EFAULT;
+            goto err_i1;
+        }
+    } else if (cmd & IOC_OUT) {
+        memset(kdata, 0, usize);
+    }
+
+
      switch (cmd) {
      case KFD_IOC_GET_VERSION:
-        err = kfd_ioctl_get_version(filep, process, (void __user 

*)arg);

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +26 -19
+        retcode = kfd_ioctl_get_version(filep, process, kdata);
          break;
      case KFD_IOC_CREATE_QUEUE:
-        err = kfd_ioctl_create_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_create_queue(filep, process,
+                        kdata);
          break;
      case KFD_IOC_DESTROY_QUEUE:
-        err = kfd_ioctl_destroy_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_destroy_queue(filep, process,
+                        kdata);
          break;
      case KFD_IOC_SET_MEMORY_POLICY:
-        err = kfd_ioctl_set_memory_policy(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_set_memory_policy(filep, process,
+                        kdata);
          break;
      case KFD_IOC_GET_CLOCK_COUNTERS:
-        err = kfd_ioctl_get_clock_counters(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_get_clock_counters(filep, process,
+                        kdata);
          break;
      case KFD_IOC_GET_PROCESS_APERTURES:
-        err = kfd_ioctl_get_process_apertures(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_get_process_apertures(filep, process,
+                        kdata);
          break;
      case KFD_IOC_UPDATE_QUEUE:
-        err = kfd_ioctl_update_queue(filep, process,
-                        (void __user *)arg);
+        retcode = kfd_ioctl_update_queue(filep, process,
+                        kdata);
          break;
      default:
-        dev_err(kfd_device,
+        dev_dbg(kfd_device,
              "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
              cmd, arg);
-        err = -EINVAL;
+        retcode = -EINVAL;
          break;
      }
-    if (err < 0)
-        dev_err(kfd_device,
-            "ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
-            err, cmd, _IOC_NR(cmd));
+    if (cmd & IOC_OUT)
+        if (copy_to_user((void __user *)arg, kdata, usize) != 0)
+            retcode = -EFAULT;
-    return err;
+err_i1:
+    if (kdata != stack_kdata)
+        kfree(kdata);
+
+    if (retcode)
+        dev_dbg(kfd_device, "ret = %d\n", retcode);
+
+    return retcode;
  }
  static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel