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

Sunil Kamath
2015-08-04T11:24:32Z

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>