[Intel-gfx] [PATCH v4 2/8] drm/i915/skl: Add DC5 Trigger Sequence.

[Imre Deak]

On Fri, 2015-04-17 at 11:29 +0530, Animesh Manna wrote:
> 
> On 4/16/2015 3:18 PM, Imre Deak wrote:
> > On to, 2015-04-16 at 12:25 +0300, Imre Deak wrote:
> >> On to, 2015-04-16 at 14:22 +0530, Animesh Manna wrote:
> >>>   [...]
> >>> @@ -223,11 +244,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   

To clarify, I meant to add at the top of this function:
bool fw_loaded = false;

> >>>   	if (!fw) {
> >>>   		i915_firmware_load_error_print(csr->fw_path, 0);
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);

... remove all the intel_csr_load_status_set(FW_FAILED) calls added in
this function

> >>>   		goto out;
> >>>   	}
> >>>   
> >>>   	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
> >>>   		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   
> >>> @@ -237,6 +260,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   		(css_header->header_len * 4)) {
> >>>   		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
> >>>   			(css_header->header_len * 4));
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	readcount += sizeof(struct intel_css_header);
> >>> @@ -248,6 +272,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   		(package_header->header_len * 4)) {
> >>>   		DRM_ERROR("Firmware has wrong package header length %u bytes\n",
> >>>   			(package_header->header_len * 4));
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	readcount += sizeof(struct intel_package_header);
> >>> @@ -268,6 +293,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	}
> >>>   	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
> >>>   		DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	readcount += dmc_offset;
> >>> @@ -277,6 +303,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
> >>>   		DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
> >>>   				(dmc_header->header_len));
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	readcount += sizeof(struct intel_dmc_header);
> >>> @@ -285,6 +312,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
> >>>   		DRM_ERROR("Firmware has wrong mmio count %u\n",
> >>>   						dmc_header->mmio_count);
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	csr->mmio_count = dmc_header->mmio_count;
> >>> @@ -293,6 +321,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   			dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
> >>>   			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
> >>>   						dmc_header->mmioaddr[i]);
> >>> +			intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   			goto out;
> >>>   		}
> >>>   		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
> >>> @@ -303,6 +332,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	nbytes = dmc_header->fw_size * 4;
> >>>   	if (nbytes > CSR_MAX_FW_SIZE) {
> >>>   		DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   	csr->dmc_fw_size = dmc_header->fw_size;
> >>> @@ -310,6 +340,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
> >>>   	if (!csr->dmc_payload) {
> >>>   		DRM_ERROR("Memory allocation failed for dmc payload\n");
> >>> +		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >>>   		goto out;
> >>>   	}
> >>>   
> >>> @@ -327,6 +358,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >>>   	/* load csr program during system boot, as needed for DC states */
> >>>   	intel_csr_load_program(dev);

... set fw_loaded = true; here

> >>>   out:
> > Also, since you have to anyway change the patch: could you replace all
> > the above intel_csr_load_status_set(dev_priv, FW_FAILED); with one here
> > using a fw_loaded bool that you set after intel_csr_load_program?
>
> Now we do not have fw_loaded bool in current patch, enum csr_state is
> used to capture the current state. Want me to add fw_loaded bool and
> call intel_csr_load_status_set(dev_priv, FW_FAILED) based on that?

... and add here
if (!fw_loaded)
	intel_csr_load_status_set(dev_priv, FW_FAILED);

to remove some duplication in the code.

--Imre