Re: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware

[Sunil Kamath <sunil.kamath@intel.com>]

On Monday 03 August 2015 09:55 PM, Animesh Manna wrote:
> This patch contains the changes to remove the byte
> swapping logic introduced with old dmc firmware.
> While debugging PC10 entry issue for skylake found
> with latest dmc firmware version 1.18 without byte
> swapping dmc is working fine and able to enter PC10.
>
> v1: Initial version.
>
> v2: Corrected firmware size during memcpy(). (Suggested by Sunil)
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> 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: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>   drivers/gpu/drm/i915/intel_csr.c | 16 ++++------------
>   2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b94ada9..9d0ee58 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -742,7 +742,7 @@ enum csr_state {
>   
>   struct intel_csr {
>   	const char *fw_path;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	uint32_t dmc_fw_size;
>   	uint32_t mmio_count;
>   	uint32_t mmioaddr[8];
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..ba1ae03 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -244,7 +244,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>   void intel_csr_load_program(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	__be32 *payload = dev_priv->csr.dmc_payload;
> +	u32 *payload = dev_priv->csr.dmc_payload;
>   	uint32_t i, fw_size;
>   
>   	if (!IS_GEN9(dev)) {
> @@ -256,7 +256,7 @@ void intel_csr_load_program(struct drm_device *dev)
>   	fw_size = dev_priv->csr.dmc_fw_size;
>   	for (i = 0; i < fw_size; i++)
>   		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> -			(u32 __force)payload[i]);
> +			payload[i]);
>   
>   	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>   		I915_WRITE(dev_priv->csr.mmioaddr[i],
> @@ -279,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	char substepping = intel_get_substepping(dev);
>   	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>   	uint32_t i;
> -	__be32 *dmc_payload;
> +	uint32_t *dmc_payload;
>   	bool fw_loaded = false;
>   
>   	if (!fw) {
> @@ -375,15 +375,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	}
>   
>   	dmc_payload = csr->dmc_payload;
> -	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] = cpu_to_be32(*tmp);
> -	}
> +	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>   
>   	/* load csr program during system boot, as needed for DC states */
>   	intel_csr_load_program(dev);
Thanks for addressing review comments.
Small change can be done in Gerrit message to generalize the issue to fw 
1.0+. Else fine.

Valid bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx