Additional amdkfd cleanups for 4.9

Additional amdkfd cleanups for 4.9

[Edward O'Callaghan]

These additional minor changes apply on top of my previous set. Nothing
too interesting here just yet more clean ups to make way for future changes.

Please review,
Kind Regards,

Edward O'Callaghan (2):
 [PATCH 1/2] drm/amdkfd: Use kfd_vm_info struct to carry consts state
 [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry'
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/2] drm/amdkfd: Use kfd_vm_info struct to carry consts state

[Edward O'Callaghan]

Use a struct to carry the calculated const state inside the
main kfd_dev state to use where we need it. Minor cleanups
while we are here.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c                |  9 ++-------
 drivers/gpu/drm/amd/amdkfd/kfd_device.c                |  7 +++++++
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  | 13 ++++++-------
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h  |  3 ---
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h                  |  7 +++++++
 drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c |  2 +-
 6 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
index d5e19b5..2114c66 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
@@ -800,13 +800,8 @@ int dbgdev_wave_reset_wavefronts(struct kfd_dev *dev, struct kfd_process *p)
 	union GRBM_GFX_INDEX_BITS reg_gfx_index;
 	struct kfd_process_device *pdd;
 	struct dbg_wave_control_info wac_info;
-	int temp;
-	int first_vmid_to_scan = 8;
-	int last_vmid_to_scan = 15;
-
-	first_vmid_to_scan = ffs(dev->shared_resources.compute_vmid_bitmap) - 1;
-	temp = dev->shared_resources.compute_vmid_bitmap >> first_vmid_to_scan;
-	last_vmid_to_scan = first_vmid_to_scan + ffz(temp);
+	int first_vmid_to_scan = dev->vm_info.first_vmid_kfd;
+	int last_vmid_to_scan = dev->vm_info.last_vmid_kfd;
 
 	reg_sq_cmd.u32All = 0;
 	status = 0;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 3f95f7c..2417b44 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -223,9 +223,16 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
 			 const struct kgd2kfd_shared_resources *gpu_resources)
 {
 	unsigned int size;
+	unsigned int vmid_bitmap_kfd;
 
 	kfd->shared_resources = *gpu_resources;
 
+	vmid_bitmap_kfd = kfd->shared_resources.compute_vmid_bitmap;
+	kfd->vm_info.first_vmid_kfd = ffs(vmid_bitmap_kfd) - 1;
+	kfd->vm_info.last_vmid_kfd = fls(vmid_bitmap_kfd) - 1;
+	kfd->vm_info.vmid_num_kfd = 1 + kfd->vm_info.last_vmid_kfd
+				    - kfd->vm_info.first_vmid_kfd;
+
 	/* calculate max size of mqds needed for queues */
 	size = max_num_of_queues_per_device *
 			kfd->device_info->mqd_size_aligned;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index f49c551..f13058c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -100,11 +100,11 @@ static int allocate_vmid(struct device_queue_manager *dqm,
 	if (dqm->vmid_bitmap == 0)
 		return -ENOMEM;
 
-	bit = find_first_bit((unsigned long *)&dqm->vmid_bitmap, CIK_VMID_NUM);
+	bit = find_first_bit((unsigned long *)&dqm->vmid_bitmap,
+				dqm->dev->vm_info.vmid_num_kfd);
 	clear_bit(bit, (unsigned long *)&dqm->vmid_bitmap);
 
-	/* Kaveri kfd vmid's starts from vmid 8 */
-	allocated_vmid = bit + KFD_VMID_START_OFFSET;
+	allocated_vmid = bit + dqm->dev->vm_info.first_vmid_kfd;
 	pr_debug("kfd: vmid allocation %d\n", allocated_vmid);
 	qpd->vmid = allocated_vmid;
 	q->properties.vmid = allocated_vmid;
@@ -119,7 +119,7 @@ static void deallocate_vmid(struct device_queue_manager *dqm,
 				struct qcm_process_device *qpd,
 				struct queue *q)
 {
-	int bit = qpd->vmid - KFD_VMID_START_OFFSET;
+	int bit = qpd->vmid - dqm->dev->vm_info.first_vmid_kfd;
 
 	/* Release the vmid mapping */
 	set_pasid_vmid_mapping(dqm, 0, qpd->vmid);
@@ -570,7 +570,7 @@ static int initialize_nocpsch(struct device_queue_manager *dqm)
 	for (i = 0; i < get_pipes_num(dqm); i++)
 		dqm->allocated_queues[i] = (1 << QUEUES_PER_PIPE) - 1;
 
-	dqm->vmid_bitmap = (1 << VMID_PER_DEVICE) - 1;
+	dqm->vmid_bitmap = (1 << dqm->dev->vm_info.vmid_num_kfd) - 1;
 	dqm->sdma_bitmap = (1 << CIK_SDMA_QUEUES) - 1;
 
 	init_scheduler(dqm);
@@ -684,8 +684,7 @@ static int set_sched_resources(struct device_queue_manager *dqm)
 
 	queue_num = get_pipes_num_cpsch() * QUEUES_PER_PIPE;
 	queue_mask = (1 << queue_num) - 1;
-	res.vmid_mask = (1 << VMID_PER_DEVICE) - 1;
-	res.vmid_mask <<= KFD_VMID_START_OFFSET;
+	res.vmid_mask = dqm->dev->shared_resources.compute_vmid_bitmap;
 	res.queue_mask = queue_mask << (get_first_pipe(dqm) * QUEUES_PER_PIPE);
 	res.gws_mask = res.oac_mask = res.gds_heap_base =
 						res.gds_heap_size = 0;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
index a625b91..bdf9541 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -32,9 +32,6 @@
 #define QUEUE_PREEMPT_DEFAULT_TIMEOUT_MS	(500)
 #define QUEUES_PER_PIPE				(8)
 #define PIPE_PER_ME_CP_SCHEDULING		(3)
-#define CIK_VMID_NUM				(8)
-#define KFD_VMID_START_OFFSET			(8)
-#define VMID_PER_DEVICE				CIK_VMID_NUM
 #define KFD_DQM_FIRST_PIPE			(0)
 #define CIK_SDMA_QUEUES				(4)
 #define CIK_SDMA_QUEUES_PER_ENGINE		(2)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 4750cab..e824922 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -141,6 +141,12 @@ struct kfd_mem_obj {
 	uint32_t *cpu_ptr;
 };
 
+struct kfd_vmid_info {
+	uint32_t first_vmid_kfd;
+	uint32_t last_vmid_kfd;
+	uint32_t vmid_num_kfd;
+};
+
 struct kfd_dev {
 	struct kgd_dev *kgd;
 
@@ -165,6 +171,7 @@ struct kfd_dev {
 					   */
 
 	struct kgd2kfd_shared_resources shared_resources;
+	struct kfd_vmid_info vm_info;
 
 	const struct kfd2kgd_calls *kfd2kgd;
 	struct mutex doorbell_mutex;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
index e1fb40b..766312b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -208,7 +208,7 @@ int pqm_create_queue(struct process_queue_manager *pqm,
 	case KFD_QUEUE_TYPE_COMPUTE:
 		/* check if there is over subscription */
 		if ((sched_policy == KFD_SCHED_POLICY_HWS_NO_OVERSUBSCRIPTION) &&
-		((dev->dqm->processes_count >= VMID_PER_DEVICE) ||
+		((dev->dqm->processes_count >= dev->vm_info.vmid_num_kfd) ||
 		(dev->dqm->queue_count >= PIPE_PER_ME_CP_SCHEDULING * QUEUES_PER_PIPE))) {
 			pr_err("kfd: over-subscription is not allowed in radeon_kfd.sched_policy == 1\n");
 			retval = -EPERM;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

[Edward O'Callaghan]

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 15 +++++----------
 drivers/gpu/drm/amd/amdkfd/cik_int.h             | 20 ++++++++++++++++----
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
index 211fc48..1c47b9e 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
@@ -27,14 +27,12 @@
 static bool cik_event_interrupt_isr(struct kfd_dev *dev,
 					const uint32_t *ih_ring_entry)
 {
-	unsigned int pasid;
 	const struct cik_ih_ring_entry *ihre =
 			(const struct cik_ih_ring_entry *)ih_ring_entry;
 
-	pasid = (ihre->ring_id & 0xffff0000) >> 16;
 
 	/* Do not process in ISR, just request it to be forwarded to WQ. */
-	return (pasid != 0) &&
+	return (ihre->pasid != 0) &&
 		(ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
 		ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
 		ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
@@ -43,21 +41,18 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
 static void cik_event_interrupt_wq(struct kfd_dev *dev,
 					const uint32_t *ih_ring_entry)
 {
-	unsigned int pasid;
 	const struct cik_ih_ring_entry *ihre =
 			(const struct cik_ih_ring_entry *)ih_ring_entry;
 
-	pasid = (ihre->ring_id & 0xffff0000) >> 16;
-
-	if (pasid == 0)
+	if (ihre->pasid == 0)
 		return;
 
 	if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
-		kfd_signal_event_interrupt(pasid, 0, 0);
+		kfd_signal_event_interrupt(ihre->pasid, 0, 0);
 	else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
-		kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
+		kfd_signal_event_interrupt(ihre->pasid, ihre->data & 0xFF, 8);
 	else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
-		kfd_signal_hw_exception_event(pasid);
+		kfd_signal_hw_exception_event(ihre->pasid);
 }
 
 const struct kfd_event_interrupt_class event_interrupt_class_cik = {
diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
index 79a16d2..27d1ede 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
+++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
@@ -26,10 +26,22 @@
 #include <linux/types.h>
 
 struct cik_ih_ring_entry {
-	uint32_t source_id;
-	uint32_t data;
-	uint32_t ring_id;
-	uint32_t reserved;
+	uint32_t source_id:8;
+	uint32_t reserved1:8;
+	uint32_t reserved2:16;
+
+	uint32_t data:28;
+	uint32_t reserved3:4;
+
+	/* pipeid, meid and unused3 are officially called RINGID,
+	 * but for our purposes, they always decode into pipe and ME. */
+	uint32_t pipeid:2;
+	uint32_t meid:2;
+	uint32_t reserved4:4;
+	uint32_t vmid:8;
+	uint32_t pasid:16;
+
+	uint32_t reserved5;
 };
 
 #define CIK_INTSRC_DEQUEUE_COMPLETE	0xC6
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

[Christian König]

NAK, don't use bitfields to decode hardware values. They aren't portable.

Regards,
Christian.

Am 27.09.2016 um 14:47 schrieb Edward O'Callaghan:
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>   drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 15 +++++----------
>   drivers/gpu/drm/amd/amdkfd/cik_int.h             | 20 ++++++++++++++++----
>   2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> index 211fc48..1c47b9e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> @@ -27,14 +27,12 @@
>   static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>   					const uint32_t *ih_ring_entry)
>   {
> -	unsigned int pasid;
>   	const struct cik_ih_ring_entry *ihre =
>   			(const struct cik_ih_ring_entry *)ih_ring_entry;
>   
> -	pasid = (ihre->ring_id & 0xffff0000) >> 16;
>   
>   	/* Do not process in ISR, just request it to be forwarded to WQ. */
> -	return (pasid != 0) &&
> +	return (ihre->pasid != 0) &&
>   		(ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
>   		ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
>   		ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
> @@ -43,21 +41,18 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>   static void cik_event_interrupt_wq(struct kfd_dev *dev,
>   					const uint32_t *ih_ring_entry)
>   {
> -	unsigned int pasid;
>   	const struct cik_ih_ring_entry *ihre =
>   			(const struct cik_ih_ring_entry *)ih_ring_entry;
>   
> -	pasid = (ihre->ring_id & 0xffff0000) >> 16;
> -
> -	if (pasid == 0)
> +	if (ihre->pasid == 0)
>   		return;
>   
>   	if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
> -		kfd_signal_event_interrupt(pasid, 0, 0);
> +		kfd_signal_event_interrupt(ihre->pasid, 0, 0);
>   	else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
> -		kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
> +		kfd_signal_event_interrupt(ihre->pasid, ihre->data & 0xFF, 8);
>   	else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
> -		kfd_signal_hw_exception_event(pasid);
> +		kfd_signal_hw_exception_event(ihre->pasid);
>   }
>   
>   const struct kfd_event_interrupt_class event_interrupt_class_cik = {
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> index 79a16d2..27d1ede 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> @@ -26,10 +26,22 @@
>   #include <linux/types.h>
>   
>   struct cik_ih_ring_entry {
> -	uint32_t source_id;
> -	uint32_t data;
> -	uint32_t ring_id;
> -	uint32_t reserved;
> +	uint32_t source_id:8;
> +	uint32_t reserved1:8;
> +	uint32_t reserved2:16;
> +
> +	uint32_t data:28;
> +	uint32_t reserved3:4;
> +
> +	/* pipeid, meid and unused3 are officially called RINGID,
> +	 * but for our purposes, they always decode into pipe and ME. */
> +	uint32_t pipeid:2;
> +	uint32_t meid:2;
> +	uint32_t reserved4:4;
> +	uint32_t vmid:8;
> +	uint32_t pasid:16;
> +
> +	uint32_t reserved5;
>   };
>   
>   #define CIK_INTSRC_DEQUEUE_COMPLETE	0xC6


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

[Oded Gabbay]

Christian is correct. That is not portable to other architectures

Oded

On Tue, Sep 27, 2016 at 3:52 PM, Christian König
<deathsimple@vodafone.de> wrote:
> NAK, don't use bitfields to decode hardware values. They aren't portable.
>
> Regards,
> Christian.
>
>
> Am 27.09.2016 um 14:47 schrieb Edward O'Callaghan:
>>
>> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 15 +++++----------
>>   drivers/gpu/drm/amd/amdkfd/cik_int.h             | 20
>> ++++++++++++++++----
>>   2 files changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> index 211fc48..1c47b9e 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> @@ -27,14 +27,12 @@
>>   static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>>                                         const uint32_t *ih_ring_entry)
>>   {
>> -       unsigned int pasid;
>>         const struct cik_ih_ring_entry *ihre =
>>                         (const struct cik_ih_ring_entry *)ih_ring_entry;
>>   -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
>>         /* Do not process in ISR, just request it to be forwarded to WQ.
>> */
>> -       return (pasid != 0) &&
>> +       return (ihre->pasid != 0) &&
>>                 (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
>>                 ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
>>                 ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
>> @@ -43,21 +41,18 @@ static bool cik_event_interrupt_isr(struct kfd_dev
>> *dev,
>>   static void cik_event_interrupt_wq(struct kfd_dev *dev,
>>                                         const uint32_t *ih_ring_entry)
>>   {
>> -       unsigned int pasid;
>>         const struct cik_ih_ring_entry *ihre =
>>                         (const struct cik_ih_ring_entry *)ih_ring_entry;
>>   -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
>> -
>> -       if (pasid == 0)
>> +       if (ihre->pasid == 0)
>>                 return;
>>         if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
>> -               kfd_signal_event_interrupt(pasid, 0, 0);
>> +               kfd_signal_event_interrupt(ihre->pasid, 0, 0);
>>         else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
>> -               kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
>> +               kfd_signal_event_interrupt(ihre->pasid, ihre->data & 0xFF,
>> 8);
>>         else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
>> -               kfd_signal_hw_exception_event(pasid);
>> +               kfd_signal_hw_exception_event(ihre->pasid);
>>   }
>>     const struct kfd_event_interrupt_class event_interrupt_class_cik = {
>> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> b/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> index 79a16d2..27d1ede 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> @@ -26,10 +26,22 @@
>>   #include <linux/types.h>
>>     struct cik_ih_ring_entry {
>> -       uint32_t source_id;
>> -       uint32_t data;
>> -       uint32_t ring_id;
>> -       uint32_t reserved;
>> +       uint32_t source_id:8;
>> +       uint32_t reserved1:8;
>> +       uint32_t reserved2:16;
>> +
>> +       uint32_t data:28;
>> +       uint32_t reserved3:4;
>> +
>> +       /* pipeid, meid and unused3 are officially called RINGID,
>> +        * but for our purposes, they always decode into pipe and ME. */
>> +       uint32_t pipeid:2;
>> +       uint32_t meid:2;
>> +       uint32_t reserved4:4;
>> +       uint32_t vmid:8;
>> +       uint32_t pasid:16;
>> +
>> +       uint32_t reserved5;
>>   };
>>     #define CIK_INTSRC_DEQUEUE_COMPLETE 0xC6
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

[StDenis, Tom]

[-- Attachment #1.1: Type: text/plain, Size: 4596 bytes --]

Better would be to add them to a bitmask header and then use REG_GET_FIELD() so it's nice and clean looking.


Tom


________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Tuesday, September 27, 2016 08:52
To: Edward O'Callaghan; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

NAK, don't use bitfields to decode hardware values. They aren't portable.

Regards,
Christian.

Am 27.09.2016 um 14:47 schrieb Edward O'Callaghan:
> Signed-off-by: Edward O'Callaghan <funfunctor-dczkZgxz+BNUPWh3PAxdjQ@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 15 +++++----------
>   drivers/gpu/drm/amd/amdkfd/cik_int.h             | 20 ++++++++++++++++----
>   2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> index 211fc48..1c47b9e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> @@ -27,14 +27,12 @@
>   static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>                                        const uint32_t *ih_ring_entry)
>   {
> -     unsigned int pasid;
>        const struct cik_ih_ring_entry *ihre =
>                        (const struct cik_ih_ring_entry *)ih_ring_entry;
>
> -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
>
>        /* Do not process in ISR, just request it to be forwarded to WQ. */
> -     return (pasid != 0) &&
> +     return (ihre->pasid != 0) &&
>                (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
>                ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
>                ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
> @@ -43,21 +41,18 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>   static void cik_event_interrupt_wq(struct kfd_dev *dev,
>                                        const uint32_t *ih_ring_entry)
>   {
> -     unsigned int pasid;
>        const struct cik_ih_ring_entry *ihre =
>                        (const struct cik_ih_ring_entry *)ih_ring_entry;
>
> -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
> -
> -     if (pasid == 0)
> +     if (ihre->pasid == 0)
>                return;
>
>        if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
> -             kfd_signal_event_interrupt(pasid, 0, 0);
> +             kfd_signal_event_interrupt(ihre->pasid, 0, 0);
>        else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
> -             kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
> +             kfd_signal_event_interrupt(ihre->pasid, ihre->data & 0xFF, 8);
>        else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
> -             kfd_signal_hw_exception_event(pasid);
> +             kfd_signal_hw_exception_event(ihre->pasid);
>   }
>
>   const struct kfd_event_interrupt_class event_interrupt_class_cik = {
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> index 79a16d2..27d1ede 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> @@ -26,10 +26,22 @@
>   #include <linux/types.h>
>
>   struct cik_ih_ring_entry {
> -     uint32_t source_id;
> -     uint32_t data;
> -     uint32_t ring_id;
> -     uint32_t reserved;
> +     uint32_t source_id:8;
> +     uint32_t reserved1:8;
> +     uint32_t reserved2:16;
> +
> +     uint32_t data:28;
> +     uint32_t reserved3:4;
> +
> +     /* pipeid, meid and unused3 are officially called RINGID,
> +      * but for our purposes, they always decode into pipe and ME. */
> +     uint32_t pipeid:2;
> +     uint32_t meid:2;
> +     uint32_t reserved4:4;
> +     uint32_t vmid:8;
> +     uint32_t pasid:16;
> +
> +     uint32_t reserved5;
>   };
>
>   #define CIK_INTSRC_DEQUEUE_COMPLETE 0xC6


_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - lists.freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
To see the collection of prior postings to the list, visit the amd-gfx Archives. Using amd-gfx: To post a message to all the list members, send email ...




[-- Attachment #1.2: Type: text/html, Size: 10471 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in 'cik_ih_ring_entry' directly

[Edward O'Callaghan]

[-- Attachment #1.1.1: Type: text/plain, Size: 5250 bytes --]

Excellent point Christian, it did occur to me that endianness could be a
problem so will definitely fix this one up as Tom suggested.

Kind Regards,
Edward.

On 09/27/2016 11:02 PM, StDenis, Tom wrote:
> Better would be to add them to a bitmask header and then use
> REG_GET_FIELD() so it's nice and clean looking.
> 
> 
> Tom
> 
> 
> 
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of
> Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
> *Sent:* Tuesday, September 27, 2016 08:52
> *To:* Edward O'Callaghan; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* Re: [PATCH 2/2] drm/amdkfd: Decode bit fields in
> 'cik_ih_ring_entry' directly
>  
> NAK, don't use bitfields to decode hardware values. They aren't portable.
> 
> Regards,
> Christian.
> 
> Am 27.09.2016 um 14:47 schrieb Edward O'Callaghan:
>> Signed-off-by: Edward O'Callaghan <funfunctor-dczkZgxz+BNUPWh3PAxdjQ@public.gmane.org>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 15 +++++----------
>>   drivers/gpu/drm/amd/amdkfd/cik_int.h             | 20 ++++++++++++++++----
>>   2 files changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> index 211fc48..1c47b9e 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
>> @@ -27,14 +27,12 @@
>>   static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>>                                        const uint32_t *ih_ring_entry)
>>   {
>> -     unsigned int pasid;
>>        const struct cik_ih_ring_entry *ihre =
>>                        (const struct cik_ih_ring_entry *)ih_ring_entry;
>>   
>> -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
>>   
>>        /* Do not process in ISR, just request it to be forwarded to WQ. */
>> -     return (pasid != 0) &&
>> +     return (ihre->pasid != 0) &&
>>                (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
>>                ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
>>                ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
>> @@ -43,21 +41,18 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>>   static void cik_event_interrupt_wq(struct kfd_dev *dev,
>>                                        const uint32_t *ih_ring_entry)
>>   {
>> -     unsigned int pasid;
>>        const struct cik_ih_ring_entry *ihre =
>>                        (const struct cik_ih_ring_entry *)ih_ring_entry;
>>   
>> -     pasid = (ihre->ring_id & 0xffff0000) >> 16;
>> -
>> -     if (pasid == 0)
>> +     if (ihre->pasid == 0)
>>                return;
>>   
>>        if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
>> -             kfd_signal_event_interrupt(pasid, 0, 0);
>> +             kfd_signal_event_interrupt(ihre->pasid, 0, 0);
>>        else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
>> -             kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
>> +             kfd_signal_event_interrupt(ihre->pasid, ihre->data & 0xFF, 8);
>>        else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
>> -             kfd_signal_hw_exception_event(pasid);
>> +             kfd_signal_hw_exception_event(ihre->pasid);
>>   }
>>   
>>   const struct kfd_event_interrupt_class event_interrupt_class_cik = {
>> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> index 79a16d2..27d1ede 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
>> @@ -26,10 +26,22 @@
>>   #include <linux/types.h>
>>   
>>   struct cik_ih_ring_entry {
>> -     uint32_t source_id;
>> -     uint32_t data;
>> -     uint32_t ring_id;
>> -     uint32_t reserved;
>> +     uint32_t source_id:8;
>> +     uint32_t reserved1:8;
>> +     uint32_t reserved2:16;
>> +
>> +     uint32_t data:28;
>> +     uint32_t reserved3:4;
>> +
>> +     /* pipeid, meid and unused3 are officially called RINGID,
>> +      * but for our purposes, they always decode into pipe and ME. */
>> +     uint32_t pipeid:2;
>> +     uint32_t meid:2;
>> +     uint32_t reserved4:4;
>> +     uint32_t vmid:8;
>> +     uint32_t pasid:16;
>> +
>> +     uint32_t reserved5;
>>   };
>>   
>>   #define CIK_INTSRC_DEQUEUE_COMPLETE 0xC6
> 
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> amd-gfx Info Page - lists.freedesktop.org
> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
> lists.freedesktop.org
> To see the collection of prior postings to the list, visit the amd-gfx
> Archives. Using amd-gfx: To post a message to all the list members, send
> email ...
> 
> 
> 
> 
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdkfd: Use kfd_vm_info struct to carry consts state

[Oded Gabbay]

On Tue, Sep 27, 2016 at 3:47 PM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Use a struct to carry the calculated const state inside the
> main kfd_dev state to use where we need it. Minor cleanups
> while we are here.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c                |  9 ++-------
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c                |  7 +++++++
>  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  | 13 ++++++-------
>  drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h  |  3 ---
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h                  |  7 +++++++
>  drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c |  2 +-
>  6 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
> index d5e19b5..2114c66 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
> @@ -800,13 +800,8 @@ int dbgdev_wave_reset_wavefronts(struct kfd_dev *dev, struct kfd_process *p)
>         union GRBM_GFX_INDEX_BITS reg_gfx_index;
>         struct kfd_process_device *pdd;
>         struct dbg_wave_control_info wac_info;
> -       int temp;
> -       int first_vmid_to_scan = 8;
> -       int last_vmid_to_scan = 15;
> -
> -       first_vmid_to_scan = ffs(dev->shared_resources.compute_vmid_bitmap) - 1;
> -       temp = dev->shared_resources.compute_vmid_bitmap >> first_vmid_to_scan;
> -       last_vmid_to_scan = first_vmid_to_scan + ffz(temp);
> +       int first_vmid_to_scan = dev->vm_info.first_vmid_kfd;
> +       int last_vmid_to_scan = dev->vm_info.last_vmid_kfd;
>
>         reg_sq_cmd.u32All = 0;
>         status = 0;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 3f95f7c..2417b44 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -223,9 +223,16 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>                          const struct kgd2kfd_shared_resources *gpu_resources)
>  {
>         unsigned int size;
> +       unsigned int vmid_bitmap_kfd;
>
>         kfd->shared_resources = *gpu_resources;
>
> +       vmid_bitmap_kfd = kfd->shared_resources.compute_vmid_bitmap;
> +       kfd->vm_info.first_vmid_kfd = ffs(vmid_bitmap_kfd) - 1;
> +       kfd->vm_info.last_vmid_kfd = fls(vmid_bitmap_kfd) - 1;
> +       kfd->vm_info.vmid_num_kfd = 1 + kfd->vm_info.last_vmid_kfd
> +                                   - kfd->vm_info.first_vmid_kfd;
> +
>         /* calculate max size of mqds needed for queues */
>         size = max_num_of_queues_per_device *
>                         kfd->device_info->mqd_size_aligned;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index f49c551..f13058c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -100,11 +100,11 @@ static int allocate_vmid(struct device_queue_manager *dqm,
>         if (dqm->vmid_bitmap == 0)
>                 return -ENOMEM;
>
> -       bit = find_first_bit((unsigned long *)&dqm->vmid_bitmap, CIK_VMID_NUM);
> +       bit = find_first_bit((unsigned long *)&dqm->vmid_bitmap,
> +                               dqm->dev->vm_info.vmid_num_kfd);
>         clear_bit(bit, (unsigned long *)&dqm->vmid_bitmap);
>
> -       /* Kaveri kfd vmid's starts from vmid 8 */
> -       allocated_vmid = bit + KFD_VMID_START_OFFSET;
> +       allocated_vmid = bit + dqm->dev->vm_info.first_vmid_kfd;
>         pr_debug("kfd: vmid allocation %d\n", allocated_vmid);
>         qpd->vmid = allocated_vmid;
>         q->properties.vmid = allocated_vmid;
> @@ -119,7 +119,7 @@ static void deallocate_vmid(struct device_queue_manager *dqm,
>                                 struct qcm_process_device *qpd,
>                                 struct queue *q)
>  {
> -       int bit = qpd->vmid - KFD_VMID_START_OFFSET;
> +       int bit = qpd->vmid - dqm->dev->vm_info.first_vmid_kfd;
>
>         /* Release the vmid mapping */
>         set_pasid_vmid_mapping(dqm, 0, qpd->vmid);
> @@ -570,7 +570,7 @@ static int initialize_nocpsch(struct device_queue_manager *dqm)
>         for (i = 0; i < get_pipes_num(dqm); i++)
>                 dqm->allocated_queues[i] = (1 << QUEUES_PER_PIPE) - 1;
>
> -       dqm->vmid_bitmap = (1 << VMID_PER_DEVICE) - 1;
> +       dqm->vmid_bitmap = (1 << dqm->dev->vm_info.vmid_num_kfd) - 1;
>         dqm->sdma_bitmap = (1 << CIK_SDMA_QUEUES) - 1;
>
>         init_scheduler(dqm);
> @@ -684,8 +684,7 @@ static int set_sched_resources(struct device_queue_manager *dqm)
>
>         queue_num = get_pipes_num_cpsch() * QUEUES_PER_PIPE;
>         queue_mask = (1 << queue_num) - 1;
> -       res.vmid_mask = (1 << VMID_PER_DEVICE) - 1;
> -       res.vmid_mask <<= KFD_VMID_START_OFFSET;
> +       res.vmid_mask = dqm->dev->shared_resources.compute_vmid_bitmap;
>         res.queue_mask = queue_mask << (get_first_pipe(dqm) * QUEUES_PER_PIPE);
>         res.gws_mask = res.oac_mask = res.gds_heap_base =
>                                                 res.gds_heap_size = 0;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> index a625b91..bdf9541 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
> @@ -32,9 +32,6 @@
>  #define QUEUE_PREEMPT_DEFAULT_TIMEOUT_MS       (500)
>  #define QUEUES_PER_PIPE                                (8)
>  #define PIPE_PER_ME_CP_SCHEDULING              (3)
> -#define CIK_VMID_NUM                           (8)
> -#define KFD_VMID_START_OFFSET                  (8)
> -#define VMID_PER_DEVICE                                CIK_VMID_NUM
>  #define KFD_DQM_FIRST_PIPE                     (0)
>  #define CIK_SDMA_QUEUES                                (4)
>  #define CIK_SDMA_QUEUES_PER_ENGINE             (2)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4750cab..e824922 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -141,6 +141,12 @@ struct kfd_mem_obj {
>         uint32_t *cpu_ptr;
>  };
>
> +struct kfd_vmid_info {
> +       uint32_t first_vmid_kfd;
> +       uint32_t last_vmid_kfd;
> +       uint32_t vmid_num_kfd;

Could you please drop the "_kfd" suffix from the field names ? It is
redundant as the structure's name implies its kfd.

> +};
> +
>  struct kfd_dev {
>         struct kgd_dev *kgd;
>
> @@ -165,6 +171,7 @@ struct kfd_dev {
>                                            */
>
>         struct kgd2kfd_shared_resources shared_resources;
> +       struct kfd_vmid_info vm_info;
>
>         const struct kfd2kgd_calls *kfd2kgd;
>         struct mutex doorbell_mutex;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> index e1fb40b..766312b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -208,7 +208,7 @@ int pqm_create_queue(struct process_queue_manager *pqm,
>         case KFD_QUEUE_TYPE_COMPUTE:
>                 /* check if there is over subscription */
>                 if ((sched_policy == KFD_SCHED_POLICY_HWS_NO_OVERSUBSCRIPTION) &&
> -               ((dev->dqm->processes_count >= VMID_PER_DEVICE) ||
> +               ((dev->dqm->processes_count >= dev->vm_info.vmid_num_kfd) ||
>                 (dev->dqm->queue_count >= PIPE_PER_ME_CP_SCHEDULING * QUEUES_PER_PIPE))) {
>                         pr_err("kfd: over-subscription is not allowed in radeon_kfd.sched_policy == 1\n");
>                         retval = -EPERM;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

With the above comment fixed, this patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx