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

Submitted by Edward O'Callaghan on Sept. 27, 2016, 12:47 p.m.

Details

Message ID 1474980460-8743-3-git-send-email-funfunctor@folklore1984.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Edward O'Callaghan Sept. 27, 2016, 12:47 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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
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
Better would be to add them to a bitmask header and then use REG_GET_FIELD() so it's nice and clean looking.


Tom
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@lists.freedesktop.org> on behalf of
> Christian König <deathsimple@vodafone.de>
> *Sent:* Tuesday, September 27, 2016 08:52
> *To:* Edward O'Callaghan; amd-gfx@lists.freedesktop.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@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 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>