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

Animesh Manna
2015-08-06T09:21:02Z

On 8/5/2015 2:31 PM, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 11:25:40AM +0530, Animesh Manna wrote:
>>
>> On 8/4/2015 9:16 AM, Nagaraju, Vathsala wrote:
>>> "This patch contains the changes to remove the byte swapping logic introduced with old dmc firmware."
>>> Which version of DMC need reversal logic? Atleast , 4,1.16,1.18 follow the same format.
>> Packaging of firmware binary completely changed after api version 1.0, so by old firmware I want to mean
>> the initial version before dmc 1.0.
> This kind of information really must be included in the commit message.
> Very likely someone with old firmware will bisect to this commit, and if
> you don't include that people need latest dmc firmware there will be
> confusion.
>
> Commit message _must_ be complete and contain everything that was
> discussed while reviewing and developing a patch.
>
> I added a note while merging the patch.
> -Daniel

Thanks Daniel. Sure, will follow your suggestion in future.

-Animesh

>>> -----Original Message-----
>>> From: Manna, Animesh
>>> Sent: Monday, August 3, 2015 9:56 PM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: Manna, Animesh; Vetter, Daniel; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala
>>> Subject: [SKL-DMC-BUGFIX 1/5] drm/i915/gen9: Removed byte swapping for csr firmware
>>>
>>> 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);
>>> --
>>> 2.0.2
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx