[18/18] drm/i915/gen9: Removed byte swapping for csr firmware.
diff mbox

Message ID DFCE9515A6E7D340BE366A94B943DE1310C9A40F@BGSMSX103.gar.corp.intel.com
State New
Headers show

Commit Message

vathsala nagaraju July 28, 2015, 8:08 a.m. UTC
Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>

It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>

Commit message: Removed byte swapping for csr firmware.

Commit message does not convey as to why the change was made.  "This change is needed as DMC firmware loading failed on skylake due  byte swap  done in the driver"


-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Animesh Manna

Sent: Sunday, July 26, 2015 12:31 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vatsala Nagaraju
Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>

Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>

---
 drivers/gpu/drm/i915/intel_csr.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

-- 
2.0.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Comments

Kamath, Sunil July 28, 2015, 11:09 a.m. UTC | #1
On Tuesday 28 July 2015 01:38 PM, Nagaraju, Vathsala wrote:
> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
> It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>
>
> Commit message: Removed byte swapping for csr firmware.
>
> Commit message does not convey as to why the change was made.  "This change is needed as DMC firmware loading failed on skylake due  byte swap  done in the driver"

yes. agree.

Same review comments as other patches. Bug fixing patches should go 
first in sequence and also commit message should contain relevant info 
on fix/patch.

This byte swapping is not needed at all.

Minimal things to take care.

- Sunil



>
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Animesh Manna
> Sent: Sunday, July 26, 2015 12:31 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vatsala Nagaraju
> Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.
>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_csr.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 1858f02..50779e0 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>   		return NULL;
>   	}
>   
> -	for (i = 0; i < dmc_header->fw_size; i++) {
> -		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> -		/*
> -		 * The firmware payload is an array of 32 bit words stored in
> -		 * little-endian format in the firmware image and programmed
> -		 * as 32 bit big-endian format to memory.
> -		 */
> -		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
> -	}
> -
> +	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
>   	return dmc_payload;
>   }
>
Kamath, Sunil July 30, 2015, 7:04 a.m. UTC | #2
On Tuesday 28 July 2015 04:39 PM, Sunil Kamath wrote:
> On Tuesday 28 July 2015 01:38 PM, Nagaraju, Vathsala wrote:
>> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
>> It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>
>>
>> Commit message: Removed byte swapping for csr firmware.
>>
>> Commit message does not convey as to why the change was made. "This 
>> change is needed as DMC firmware loading failed on skylake due  byte 
>> swap  done in the driver"
>
> yes. agree.
>
> Same review comments as other patches. Bug fixing patches should go 
> first in sequence and also commit message should contain relevant info 
> on fix/patch.
>
> This byte swapping is not needed at all.
>
> Minimal things to take care.
>
> - Sunil
>
>
>
>>
>>
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
>> Behalf Of Animesh Manna
>> Sent: Sunday, July 26, 2015 12:31 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Vatsala Nagaraju
>> Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte 
>> swapping for csr firmware.
>>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_csr.c | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c 
>> b/drivers/gpu/drm/i915/intel_csr.c
>> index 1858f02..50779e0 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct 
>> drm_i915_private *dev_priv,
>>           return NULL;
>>       }
>>   -    for (i = 0; i < dmc_header->fw_size; i++) {
>> -        uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>> -        /*
>> -         * The firmware payload is an array of 32 bit words stored in
>> -         * little-endian format in the firmware image and programmed
>> -         * as 32 bit big-endian format to memory.
>> -         */
>> -        dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
>> -    }
>> -
>> +    memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);

*4 missing for byte conversion here.
memcpy(dmc_payload, &fw->data[readcount], (dmc_header->fw_size)*4); ?
Still issue to be addressed here to confirm that fw is loaded fine.

- Sunil
>>       return dmc_payload;
>>   }
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1858f02..50779e0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -328,16 +328,7 @@  static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 	}
 
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
-	}
-
+	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
 	return dmc_payload;
 }