[PATCH 1/5] drm/i915: Store and print dmc firmware version

[PATCH 1/5] drm/i915: Store and print dmc firmware version

[Mika Kuoppala]

Parse csr/dmc firmware version and augment debug message
by printing it.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 2 ++
 drivers/gpu/drm/i915/intel_csr.c | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3bf8a9b..17e8b25 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -755,6 +755,8 @@ struct intel_csr {
 	const char *fw_path;
 	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
+	uint16_t dmc_ver_major;
+	uint16_t dmc_ver_minor;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b69264d..58edc3f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -377,11 +377,16 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	dmc_payload = csr->dmc_payload;
 	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
+	csr->dmc_ver_major = dmc_header->header_ver;
+	csr->dmc_ver_minor = ((dmc_header->fw_version & 0xffff0000) >> 16) * 10
+		+ (dmc_header->fw_version & 0x0000ffff);
+
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
 	fw_loaded = true;
 
-	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
+	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
+		      csr->dmc_ver_major, csr->dmc_ver_minor);
 out:
 	if (fw_loaded)
 		intel_runtime_pm_put(dev_priv);
-- 
2.1.4

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

[PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Mika Kuoppala]

If csr/dmc firmware is known to be outdated, notify
user.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 58edc3f..73807c3 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -45,6 +45,9 @@
 
 MODULE_FIRMWARE(I915_CSR_SKL);
 
+#define RECOMMENDED_FW_MAJOR		1
+#define RECOMMENDED_FW_MINOR		21
+
 /*
 * SKL CSR registers for DC5 and DC6
 */
@@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
 		      csr->dmc_ver_major, csr->dmc_ver_minor);
+
+	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
+	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
+		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
+			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
+
 out:
 	if (fw_loaded)
 		intel_runtime_pm_put(dev_priv);
-- 
2.1.4

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

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Jani Nikula]

On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> If csr/dmc firmware is known to be outdated, notify
> user.

What would break if we requested a firmware version that works? Or we've
made it so that we only request the major version because there's not
supposed to be changes like this between minor versions...?

BR,
Jani.



>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 58edc3f..73807c3 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -45,6 +45,9 @@
>  
>  MODULE_FIRMWARE(I915_CSR_SKL);
>  
> +#define RECOMMENDED_FW_MAJOR		1
> +#define RECOMMENDED_FW_MINOR		21
> +
>  /*
>  * SKL CSR registers for DC5 and DC6
>  */
> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>  		      csr->dmc_ver_major, csr->dmc_ver_minor);
> +
> +	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
> +	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
> +		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
> +			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
> +
>  out:
>  	if (fw_loaded)
>  		intel_runtime_pm_put(dev_priv);
> -- 
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Mika Kuoppala]

Jani Nikula <jani.nikula@linux.intel.com> writes:

> On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>> If csr/dmc firmware is known to be outdated, notify
>> user.
>
> What would break if we requested a firmware version that works? Or we've
> made it so that we only request the major version because there's not
> supposed to be changes like this between minor versions...?
>

I guess the question is more of a what should we do
if there is only outdated (known bad) firmware available.

Refuse to load and limb onwards, or return with error code
on driver init.

Latter would force firmware and version to be mandatory and the
version to be tightly coupled to kernel driver version.

-Mika

> BR,
> Jani.
>
>
>
>>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 58edc3f..73807c3 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -45,6 +45,9 @@
>>  
>>  MODULE_FIRMWARE(I915_CSR_SKL);
>>  
>> +#define RECOMMENDED_FW_MAJOR		1
>> +#define RECOMMENDED_FW_MINOR		21
>> +
>>  /*
>>  * SKL CSR registers for DC5 and DC6
>>  */
>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>  
>>  	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>  		      csr->dmc_ver_major, csr->dmc_ver_minor);
>> +
>> +	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
>> +	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
>> +		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
>> +			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
>> +
>>  out:
>>  	if (fw_loaded)
>>  		intel_runtime_pm_put(dev_priv);
>> -- 
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Animesh Manna]

On 9/21/2015 2:00 PM, Mika Kuoppala wrote:
> Jani Nikula <jani.nikula@linux.intel.com> writes:
>
>> On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>>> If csr/dmc firmware is known to be outdated, notify
>>> user.
>> What would break if we requested a firmware version that works? Or we've
>> made it so that we only request the major version because there's not
>> supposed to be changes like this between minor versions...?
>>
> I guess the question is more of a what should we do
> if there is only outdated (known bad) firmware available.
>
> Refuse to load and limb onwards, or return with error code
> on driver init.
>
> Latter would force firmware and version to be mandatory and the
> version to be tightly coupled to kernel driver version.

A softlink is used to use recommended firmware for dmc and the same information is published through 01.org for the firmware user.
Imo, we should not have this kind of hack in code which will change over time and this is responsibility of repo-owner to link correct recommended firmware for new kernel update.

-Animesh

> -Mika
>
>> BR,
>> Jani.
>>
>>
>>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>> index 58edc3f..73807c3 100644
>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>> @@ -45,6 +45,9 @@
>>>   
>>>   MODULE_FIRMWARE(I915_CSR_SKL);
>>>   
>>> +#define RECOMMENDED_FW_MAJOR		1
>>> +#define RECOMMENDED_FW_MINOR		21
>>> +
>>>   /*
>>>   * SKL CSR registers for DC5 and DC6
>>>   */
>>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>   
>>>   	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>>   		      csr->dmc_ver_major, csr->dmc_ver_minor);
>>> +
>>> +	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
>>> +	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
>>> +		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
>>> +			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
>>> +
>>>   out:
>>>   	if (fw_loaded)
>>>   		intel_runtime_pm_put(dev_priv);
>>> -- 
>>> 2.1.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Mika Kuoppala]

Animesh Manna <animesh.manna@intel.com> writes:

> On 9/21/2015 2:00 PM, Mika Kuoppala wrote:
>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>>
>>> On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>>>> If csr/dmc firmware is known to be outdated, notify
>>>> user.
>>> What would break if we requested a firmware version that works? Or we've
>>> made it so that we only request the major version because there's not
>>> supposed to be changes like this between minor versions...?
>>>
>> I guess the question is more of a what should we do
>> if there is only outdated (known bad) firmware available.
>>
>> Refuse to load and limb onwards, or return with error code
>> on driver init.
>>
>> Latter would force firmware and version to be mandatory and the
>> version to be tightly coupled to kernel driver version.
>
> A softlink is used to use recommended firmware for dmc and the same information is published through 01.org for the firmware user.
> Imo, we should not have this kind of hack in code which will change over time and this is responsibility of repo-owner to link correct recommended firmware for new kernel update.
>

On machines that had 1.19 symlinked, in filesystem, execlist submission
sometimes broke due to interrupt delivery problem. To reach a conclusion
that it was csr firmware, before 1.21 was out, took quite amount of work.

I bet there are still machines with 1.19 only, and we get to 
wade through error states trying to connect the dots.

The dmc/csr firmware is part of our driver functionality. Apparently
it is very tightly coupled to our driver functionality as it can
break things outside of its own domain.

And currently it is loosely coupled black box with our driver,
through symlink, accepting any version that happens to be in customers filesystem.

So we recommend latest in website and end up in a situation
that user gets what happens to be in filesystem. Even a known
broken version? And we will keep debugging these problems caused by
broken version? I don't want any more dimensions in our triaging
space, the distributio/firmware version dimension.

Symlink also means that bisectability is very close to worthless on these
kind of bugs. Both in our machines and also on customers. We have
loosely coupled, black box entity, affecting our driver depending
on customers filesystem. Symlink threw that valuable tool out, and
we gained what?

So we are left with triaging. Which is true detective work as there are
no traces of firmware versions nor loading success/fails on
logs/error states.

From where I look at, the version blacklist is not a hack. It is a cure.

-Mika

> -Animesh
>
>> -Mika
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>>> index 58edc3f..73807c3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>>> @@ -45,6 +45,9 @@
>>>>   
>>>>   MODULE_FIRMWARE(I915_CSR_SKL);
>>>>   
>>>> +#define RECOMMENDED_FW_MAJOR		1
>>>> +#define RECOMMENDED_FW_MINOR		21
>>>> +
>>>>   /*
>>>>   * SKL CSR registers for DC5 and DC6
>>>>   */
>>>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>>   
>>>>   	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>>>   		      csr->dmc_ver_major, csr->dmc_ver_minor);
>>>> +
>>>> +	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
>>>> +	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
>>>> +		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
>>>> +			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
>>>> +
>>>>   out:
>>>>   	if (fw_loaded)
>>>>   		intel_runtime_pm_put(dev_priv);
>>>> -- 
>>>> 2.1.4
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> -- 
>>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Animesh Manna]

On 10/8/2015 5:53 PM, Mika Kuoppala wrote:
> Animesh Manna <animesh.manna@intel.com> writes:
>
>> On 9/21/2015 2:00 PM, Mika Kuoppala wrote:
>>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>>>
>>>> On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>>>>> If csr/dmc firmware is known to be outdated, notify
>>>>> user.
>>>> What would break if we requested a firmware version that works? Or we've
>>>> made it so that we only request the major version because there's not
>>>> supposed to be changes like this between minor versions...?
>>>>
>>> I guess the question is more of a what should we do
>>> if there is only outdated (known bad) firmware available.
>>>
>>> Refuse to load and limb onwards, or return with error code
>>> on driver init.
>>>
>>> Latter would force firmware and version to be mandatory and the
>>> version to be tightly coupled to kernel driver version.
>> A softlink is used to use recommended firmware for dmc and the same information is published through 01.org for the firmware user.
>> Imo, we should not have this kind of hack in code which will change over time and this is responsibility of repo-owner to link correct recommended firmware for new kernel update.
>>
> On machines that had 1.19 symlinked, in filesystem, execlist submission
> sometimes broke due to interrupt delivery problem. To reach a conclusion
> that it was csr firmware, before 1.21 was out, took quite amount of work.
>
> I bet there are still machines with 1.19 only, and we get to
> wade through error states trying to connect the dots.
>
> The dmc/csr firmware is part of our driver functionality. Apparently
> it is very tightly coupled to our driver functionality as it can
> break things outside of its own domain.
>
> And currently it is loosely coupled black box with our driver,
> through symlink, accepting any version that happens to be in customers filesystem.
>
> So we recommend latest in website and end up in a situation
> that user gets what happens to be in filesystem. Even a known
> broken version? And we will keep debugging these problems caused by
> broken version? I don't want any more dimensions in our triaging
> space, the distributio/firmware version dimension.
>
> Symlink also means that bisectability is very close to worthless on these
> kind of bugs. Both in our machines and also on customers. We have
> loosely coupled, black box entity, affecting our driver depending
> on customers filesystem. Symlink threw that valuable tool out, and
> we gained what?
>
> So we are left with triaging. Which is true detective work as there are
> no traces of firmware versions nor loading success/fails on
> logs/error states.
>
>  From where I look at, the version blacklist is not a hack. It is a cure.

I completely understand your concern and we discussed a lot on same during firmware naming
convention and finally decided to have symlink.

If we really want to tightly couple firmware and driver then imo putting exact firmware name
will be better option.

Next I saw your subsequent patch where you are not loading the firmware if it is older than 1.21.
http://lists.freedesktop.org/archives/intel-gfx/2015-September/076422.html
Curious to know the gpu-hang issue present for any version less than 1.21.

-Animesh


>
> -Mika
>
>> -Animesh
>>
>>> -Mika
>>>
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>>
>>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/intel_csr.c | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>>>> index 58edc3f..73807c3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_csr.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>>>>> @@ -45,6 +45,9 @@
>>>>>    
>>>>>    MODULE_FIRMWARE(I915_CSR_SKL);
>>>>>    
>>>>> +#define RECOMMENDED_FW_MAJOR		1
>>>>> +#define RECOMMENDED_FW_MINOR		21
>>>>> +
>>>>>    /*
>>>>>    * SKL CSR registers for DC5 and DC6
>>>>>    */
>>>>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>>>    
>>>>>    	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>>>>    		      csr->dmc_ver_major, csr->dmc_ver_minor);
>>>>> +
>>>>> +	if (csr->dmc_ver_major < RECOMMENDED_FW_MAJOR ||
>>>>> +	    csr->dmc_ver_minor < RECOMMENDED_FW_MINOR)
>>>>> +		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
>>>>> +			 RECOMMENDED_FW_MAJOR, RECOMMENDED_FW_MINOR);
>>>>> +
>>>>>    out:
>>>>>    	if (fw_loaded)
>>>>>    		intel_runtime_pm_put(dev_priv);
>>>>> -- 
>>>>> 2.1.4
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> -- 
>>>> Jani Nikula, Intel Open Source Technology Center
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Dave Gordon]

On 08/10/15 15:45, Animesh Manna wrote:
>
> On 10/8/2015 5:53 PM, Mika Kuoppala wrote:
>> Animesh Manna <animesh.manna@intel.com> writes:
>>
>>> On 9/21/2015 2:00 PM, Mika Kuoppala wrote:
>>>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>>>>
>>>>> On Fri, 18 Sep 2015, Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>>>> wrote:
>>>>>> If csr/dmc firmware is known to be outdated, notify
>>>>>> user.
>>>>> What would break if we requested a firmware version that works? Or
>>>>> we've
>>>>> made it so that we only request the major version because there's not
>>>>> supposed to be changes like this between minor versions...?
>>>>>
>>>> I guess the question is more of a what should we do
>>>> if there is only outdated (known bad) firmware available.
>>>>
>>>> Refuse to load and limb onwards, or return with error code
>>>> on driver init.
>>>>
>>>> Latter would force firmware and version to be mandatory and the
>>>> version to be tightly coupled to kernel driver version.
>>> A softlink is used to use recommended firmware for dmc and the same
>>> information is published through 01.org for the firmware user.
>>> Imo, we should not have this kind of hack in code which will change
>>> over time and this is responsibility of repo-owner to link correct
>>> recommended firmware for new kernel update.
>>>
>> On machines that had 1.19 symlinked, in filesystem, execlist submission
>> sometimes broke due to interrupt delivery problem. To reach a conclusion
>> that it was csr firmware, before 1.21 was out, took quite amount of work.
>>
>> I bet there are still machines with 1.19 only, and we get to
>> wade through error states trying to connect the dots.
>>
>> The dmc/csr firmware is part of our driver functionality. Apparently
>> it is very tightly coupled to our driver functionality as it can
>> break things outside of its own domain.
>>
>> And currently it is loosely coupled black box with our driver,
>> through symlink, accepting any version that happens to be in customers
>> filesystem.
>>
>> So we recommend latest in website and end up in a situation
>> that user gets what happens to be in filesystem. Even a known
>> broken version? And we will keep debugging these problems caused by
>> broken version? I don't want any more dimensions in our triaging
>> space, the distributio/firmware version dimension.
>>
>> Symlink also means that bisectability is very close to worthless on these
>> kind of bugs. Both in our machines and also on customers. We have
>> loosely coupled, black box entity, affecting our driver depending
>> on customers filesystem. Symlink threw that valuable tool out, and
>> we gained what?
>>
>> So we are left with triaging. Which is true detective work as there are
>> no traces of firmware versions nor loading success/fails on
>> logs/error states.
>>
>>  From where I look at, the version blacklist is not a hack. It is a cure.
>
> I completely understand your concern and we discussed a lot on same
> during firmware naming
> convention and finally decided to have symlink.
>
> If we really want to tightly couple firmware and driver then imo putting
> exact firmware name
> will be better option.
>
> Next I saw your subsequent patch where you are not loading the firmware
> if it is older than 1.21.
> http://lists.freedesktop.org/archives/intel-gfx/2015-September/076422.html
> Curious to know the gpu-hang issue present for any version less than 1.21.
>
> -Animesh

The GuC loader always had this sort of functionality, so the driver can 
be built to know that anything older than a specific minor version is bogus.

The proposed unified loader therefore tested (=major, >=minor) criteria 
for each of the various chunks of uC device firmware being loaded.

.Dave.

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

Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Marc Herbert]

> On machines that had 1.19 symlinked, in filesystem, execlist submission
> sometimes broke due to interrupt delivery problem. To reach a conclusion
> that it was csr firmware, before 1.21 was out, took quite amount of work.
>
> I bet there are still machines with 1.19 only, and we get to
> wade through error states trying to connect the dots.
>
> [...]
> So we are left with triaging. Which is true detective work as there are
> no traces of firmware versions nor loading success/fails on
> logs/error states.
>

I think this "Finished loading" log statement below should not just include the version
numbers, its level should also be increased to something like "INFO" so 
the DMC version always makes it to the logs. Collecting the error state is
less obvious and less usual than collecting logs; some users don't even know
about it.

>>>>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>>>
>>>>>    	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>>>>    		      csr->dmc_ver_major, csr->dmc_ver_minor);


    ---------

To be honest (and likely off-topic), I even think request_firmware() should log the
resolved symlink by default. Quick & dirty proof of concept below to illustrate.

The kernel is normally quick enough to squeal "TAINTED!". On the other hand
request_firmware() loads random binaries without even saying anywhere it did.
Rationale?

Marc

--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -344,17 +345,24 @@ static int fw_get_filesystem_firmware(struct device *device,
 		else
 			break;
 	}
-	__putname(path);
 
 	if (!rc) {
-		dev_dbg(device, "firmware: direct-loading firmware %s\n",
-			buf->fw_id);
+		// fallback on symlink in case lookup goes wrong
+		const char *resolved_sym = path;
+
+		struct path dp;
+		if (!kern_path(path, LOOKUP_FOLLOW, &dp))
+			resolved_sym = dp.dentry->d_name.name;
+
+		dev_info(device, "firmware: direct-loading firmware %s\n",
+			 resolved_sym);
 		mutex_lock(&fw_lock);
 		set_bit(FW_STATUS_DONE, &buf->status);
 		complete_all(&buf->completion);
 		mutex_unlock(&fw_lock);
 	}
 
+	__putname(path);
 	return rc;
 }
 

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

[PATCH 2/5] drm/i915/skl: Refuse to load outdated dmc firmware

[Mika Kuoppala]

There is known issue on GT interrupt delivery with DC6 and
firmwares <1.21. There is a suspicion that this causes
spurious gpu hangs on driver init and with some workloads,
as upgrading the firmware to 1.21 makes these problems
disappear.

As of now the current version included in distribution
firmware packages is very like to be 1.19. Play it safe and
refuse to load a firmware version that may affect gpu
side stability.

v2: Refuse to load fw instead of notifying the user

Cc: Animesh Manna <animesh.manna@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
References: https://01.org/linuxgraphics/downloads/skldmcver121
Testcase: igt/gem_exec_nop
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 308330b..edd1017 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -47,6 +47,9 @@
 MODULE_FIRMWARE(I915_CSR_SKL);
 MODULE_FIRMWARE(I915_CSR_BXT);
 
+#define SKL_REQUIRED_FW_MAJOR	1
+#define SKL_REQUIRED_FW_MINOR	21
+
 /*
 * SKL CSR registers for DC5 and DC6
 */
@@ -394,6 +397,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	csr->dmc_ver_minor = ((dmc_header->fw_version & 0xffff0000) >> 16) * 10
 		+ (dmc_header->fw_version & 0x0000ffff);
 
+	if (IS_SKYLAKE(dev) && (csr->dmc_ver_major < SKL_REQUIRED_FW_MAJOR ||
+				csr->dmc_ver_minor < SKL_REQUIRED_FW_MINOR)) {
+		DRM_INFO("Outdated dmc firmware found, please upgrade to %u.%u or newer\n",
+			 SKL_REQUIRED_FW_MAJOR, SKL_REQUIRED_FW_MINOR);
+			goto out;
+	}
+
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
 	fw_loaded = true;
-- 
2.1.4

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

[PATCH 3/5] drm/i915: Add dmc firmware version to error state

[Mika Kuoppala]

We have had one case where buggy csr/dmc firmware version influenced
gt side and caused a hang. Add dmc firmware version to error state.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3379f9c..fef708d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -366,6 +366,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	err_printf(m, "Suspend count: %u\n", error->suspend_count);
 	err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device);
 	err_printf(m, "IOMMU enabled?: %d\n", error->iommu);
+	err_printf(m, "DMC fw: %u.%u\n", dev_priv->csr.dmc_ver_major,
+		   dev_priv->csr.dmc_ver_minor);
 	err_printf(m, "EIR: 0x%08x\n", error->eir);
 	err_printf(m, "IER: 0x%08x\n", error->ier);
 	if (INTEL_INFO(dev)->gen >= 8) {
-- 
2.1.4

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

[PATCH 4/5] drm/i915: Add pci device revision to error state

[Mika Kuoppala]

We have codepaths that branch using the revision of the device.
Add pci revision to error state.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index fef708d..a29377e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -364,7 +364,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	}
 	err_printf(m, "Reset count: %u\n", error->reset_count);
 	err_printf(m, "Suspend count: %u\n", error->suspend_count);
-	err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device);
+	err_printf(m, "PCI ID: 0x%04x (rev %02x)\n", dev->pdev->device,
+		   dev->pdev->revision);
 	err_printf(m, "IOMMU enabled?: %d\n", error->iommu);
 	err_printf(m, "DMC fw: %u.%u\n", dev_priv->csr.dmc_ver_major,
 		   dev_priv->csr.dmc_ver_minor);
-- 
2.1.4

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

[PATCH 5/5] drm/i915: Add dmc firmware debugfs status entry

[Mika Kuoppala]

Add debugfs entry for csr/dmc fw to inspect firmware
loading status and version.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h     |  5 +++++
 drivers/gpu/drm/i915/intel_csr.c    |  3 ---
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72ae347..4a798a6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2509,6 +2509,37 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_dmc_load_status_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_i915_private *dev_priv = node->minor->dev->dev_private;
+	struct intel_csr *csr = &dev_priv->csr;
+	uint32_t state;
+	const char * const state_str[] = { "uninitialized",
+					   "loaded",
+					   "failed",
+					   "unknown" };
+
+	seq_puts(m, "DMC firmware status:\n");
+
+	mutex_lock(&dev_priv->csr_lock);
+
+	seq_printf(m, "\tpath: %s\n", csr->fw_path);
+	seq_printf(m, "\tfw_ver: %u.%u\n", csr->dmc_ver_major,
+		   csr->dmc_ver_minor);
+	seq_printf(m, "\tsize: %u bytes\n", csr->dmc_fw_size * 4);
+	state = (uint32_t)csr->state <= 3 ? csr->state : 3;
+	seq_printf(m, "\tstate: %s\n", state_str[state]);
+
+	seq_printf(m, "\tprogram base: 0x%08x\n", I915_READ(CSR_PROGRAM_BASE));
+	seq_printf(m, "\tssp base: 0x%08x\n", I915_READ(CSR_SSP_BASE));
+	seq_printf(m, "\thtp: 0x%08x\n", I915_READ(CSR_HTP_SKL));
+
+	mutex_unlock(&dev_priv->csr_lock);
+
+	return 0;
+}
+
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5173,6 +5204,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_guc_info", i915_guc_info, 0},
 	{"i915_guc_load_status", i915_guc_load_status_info, 0},
 	{"i915_guc_log_dump", i915_guc_log_dump, 0},
+	{"i915_dmc_load_status", i915_dmc_load_status_info, 0},
 	{"i915_frequency_info", i915_frequency_info, 0},
 	{"i915_hangcheck_info", i915_hangcheck_info, 0},
 	{"i915_drpc_info", i915_drpc_info, 0},
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 67bf205..cd040ff 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7977,4 +7977,9 @@ enum skl_disp_power_wells {
 #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
 #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
 
+/* DMC/CSR firmware */
+#define CSR_PROGRAM_BASE		0x80000
+#define CSR_SSP_BASE			0x8F074
+#define CSR_HTP_SKL			0x8F004
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 73807c3..876c839 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -51,11 +51,8 @@ MODULE_FIRMWARE(I915_CSR_SKL);
 /*
 * SKL CSR registers for DC5 and DC6
 */
-#define CSR_PROGRAM_BASE		0x80000
 #define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
 #define CSR_HTP_ADDR_SKL		0x00500034
-#define CSR_SSP_BASE			0x8F074
-#define CSR_HTP_SKL			0x8F004
 #define CSR_LAST_WRITE			0x8F034
 #define CSR_LAST_WRITE_VALUE		0xc003b400
 /* MMIO address range for CSR program (0x80000 - 0x82FFF) */
-- 
2.1.4

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

Re: [PATCH 5/5] drm/i915: Add dmc firmware debugfs status entry

[Animesh Manna]

On 9/18/2015 8:47 PM, Mika Kuoppala wrote:
> Add debugfs entry for csr/dmc fw to inspect firmware
> loading status and version.
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h     |  5 +++++
>   drivers/gpu/drm/i915/intel_csr.c    |  3 ---
>   3 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 72ae347..4a798a6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2509,6 +2509,37 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
>   	return 0;
>   }
>   
> +static int i915_dmc_load_status_info(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_i915_private *dev_priv = node->minor->dev->dev_private;
> +	struct intel_csr *csr = &dev_priv->csr;
> +	uint32_t state;
> +	const char * const state_str[] = { "uninitialized",
> +					   "loaded",
> +					   "failed",
> +					   "unknown" };
> +
> +	seq_puts(m, "DMC firmware status:\n");
> +
> +	mutex_lock(&dev_priv->csr_lock);

DMC-redesign series is floated for review where csr_lock is removed completely, not sure do we need mutex lock here.

> +
> +	seq_printf(m, "\tpath: %s\n", csr->fw_path);
> +	seq_printf(m, "\tfw_ver: %u.%u\n", csr->dmc_ver_major,
> +		   csr->dmc_ver_minor);
> +	seq_printf(m, "\tsize: %u bytes\n", csr->dmc_fw_size * 4);
> +	state = (uint32_t)csr->state <= 3 ? csr->state : 3;
> +	seq_printf(m, "\tstate: %s\n", state_str[state]);
> +
> +	seq_printf(m, "\tprogram base: 0x%08x\n", I915_READ(CSR_PROGRAM_BASE));
> +	seq_printf(m, "\tssp base: 0x%08x\n", I915_READ(CSR_SSP_BASE));
> +	seq_printf(m, "\thtp: 0x%08x\n", I915_READ(CSR_HTP_SKL));

Better print dmc firmware loading related info if firmware is loaded successfully.
In failure case, only firmware not loaded msg with path I feel is sufficient.

-Animesh

> +
> +	mutex_unlock(&dev_priv->csr_lock);
> +
> +	return 0;
> +}
> +
>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>   {
>   	struct drm_info_node *node = m->private;
> @@ -5173,6 +5204,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_guc_info", i915_guc_info, 0},
>   	{"i915_guc_load_status", i915_guc_load_status_info, 0},
>   	{"i915_guc_log_dump", i915_guc_log_dump, 0},
> +	{"i915_dmc_load_status", i915_dmc_load_status_info, 0},
>   	{"i915_frequency_info", i915_frequency_info, 0},
>   	{"i915_hangcheck_info", i915_hangcheck_info, 0},
>   	{"i915_drpc_info", i915_drpc_info, 0},
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 67bf205..cd040ff 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7977,4 +7977,9 @@ enum skl_disp_power_wells {
>   #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
>   #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
>   
> +/* DMC/CSR firmware */
> +#define CSR_PROGRAM_BASE		0x80000
> +#define CSR_SSP_BASE			0x8F074
> +#define CSR_HTP_SKL			0x8F004
> +
>   #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 73807c3..876c839 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -51,11 +51,8 @@ MODULE_FIRMWARE(I915_CSR_SKL);
>   /*
>   * SKL CSR registers for DC5 and DC6
>   */
> -#define CSR_PROGRAM_BASE		0x80000
>   #define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
>   #define CSR_HTP_ADDR_SKL		0x00500034
> -#define CSR_SSP_BASE			0x8F074
> -#define CSR_HTP_SKL			0x8F004
>   #define CSR_LAST_WRITE			0x8F034
>   #define CSR_LAST_WRITE_VALUE		0xc003b400
>   /* MMIO address range for CSR program (0x80000 - 0x82FFF) */

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

Re: [PATCH 5/5] drm/i915: Add dmc firmware debugfs status entry

[Mika Kuoppala]

Animesh Manna <animesh.manna@intel.com> writes:

> On 9/18/2015 8:47 PM, Mika Kuoppala wrote:
>> Add debugfs entry for csr/dmc fw to inspect firmware
>> loading status and version.
>>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_reg.h     |  5 +++++
>>   drivers/gpu/drm/i915/intel_csr.c    |  3 ---
>>   3 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 72ae347..4a798a6 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2509,6 +2509,37 @@ static int i915_guc_log_dump(struct seq_file *m, void *data)
>>   	return 0;
>>   }
>>   
>> +static int i915_dmc_load_status_info(struct seq_file *m, void *data)
>> +{
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_i915_private *dev_priv = node->minor->dev->dev_private;
>> +	struct intel_csr *csr = &dev_priv->csr;
>> +	uint32_t state;
>> +	const char * const state_str[] = { "uninitialized",
>> +					   "loaded",
>> +					   "failed",
>> +					   "unknown" };
>> +
>> +	seq_puts(m, "DMC firmware status:\n");
>> +
>> +	mutex_lock(&dev_priv->csr_lock);
>
> DMC-redesign series is floated for review where csr_lock is removed completely, not sure do we need mutex lock here.
>
>> +
>> +	seq_printf(m, "\tpath: %s\n", csr->fw_path);
>> +	seq_printf(m, "\tfw_ver: %u.%u\n", csr->dmc_ver_major,
>> +		   csr->dmc_ver_minor);
>> +	seq_printf(m, "\tsize: %u bytes\n", csr->dmc_fw_size * 4);
>> +	state = (uint32_t)csr->state <= 3 ? csr->state : 3;
>> +	seq_printf(m, "\tstate: %s\n", state_str[state]);
>> +
>> +	seq_printf(m, "\tprogram base: 0x%08x\n", I915_READ(CSR_PROGRAM_BASE));
>> +	seq_printf(m, "\tssp base: 0x%08x\n", I915_READ(CSR_SSP_BASE));
>> +	seq_printf(m, "\thtp: 0x%08x\n", I915_READ(CSR_HTP_SKL));
>
> Better print dmc firmware loading related info if firmware is loaded successfully.
> In failure case, only firmware not loaded msg with path I feel is sufficient.
>

The loader peeks these registers to determine if the loading was
succesful. If we toggle the register output based on the final state,
we lose debuggability in a case where the loading fails. As we wont
get to read the registers through this debugfs entry.

If the firmware has been loaded successfully, there is
little value in these registers as they are asserted
into known values in the code. 

The debugfs entry was made to sched some light if
we encounter a case where firmware didn't load succesfully.
I guess the real question is should we keep this or drop it
as unnecessary?

-Mika

> -Animesh
>
>> +
>> +	mutex_unlock(&dev_priv->csr_lock);
>> +
>> +	return 0;
>> +}
>> +
>>   static int i915_edp_psr_status(struct seq_file *m, void *data)
>>   {
>>   	struct drm_info_node *node = m->private;
>> @@ -5173,6 +5204,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>>   	{"i915_guc_info", i915_guc_info, 0},
>>   	{"i915_guc_load_status", i915_guc_load_status_info, 0},
>>   	{"i915_guc_log_dump", i915_guc_log_dump, 0},
>> +	{"i915_dmc_load_status", i915_dmc_load_status_info, 0},
>>   	{"i915_frequency_info", i915_frequency_info, 0},
>>   	{"i915_hangcheck_info", i915_hangcheck_info, 0},
>>   	{"i915_drpc_info", i915_drpc_info, 0},
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 67bf205..cd040ff 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7977,4 +7977,9 @@ enum skl_disp_power_wells {
>>   #define GEN9_VEBOX_MOCS_0	0xcb00	/* Video MOCS base register*/
>>   #define GEN9_BLT_MOCS_0		0xcc00	/* Blitter MOCS base register*/
>>   
>> +/* DMC/CSR firmware */
>> +#define CSR_PROGRAM_BASE		0x80000
>> +#define CSR_SSP_BASE			0x8F074
>> +#define CSR_HTP_SKL			0x8F004
>> +
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index 73807c3..876c839 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -51,11 +51,8 @@ MODULE_FIRMWARE(I915_CSR_SKL);
>>   /*
>>   * SKL CSR registers for DC5 and DC6
>>   */
>> -#define CSR_PROGRAM_BASE		0x80000
>>   #define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
>>   #define CSR_HTP_ADDR_SKL		0x00500034
>> -#define CSR_SSP_BASE			0x8F074
>> -#define CSR_HTP_SKL			0x8F004
>>   #define CSR_LAST_WRITE			0x8F034
>>   #define CSR_LAST_WRITE_VALUE		0xc003b400
>>   /* MMIO address range for CSR program (0x80000 - 0x82FFF) */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Store and print dmc firmware version

[Animesh Manna]

On 9/18/2015 8:47 PM, Mika Kuoppala wrote:
> Parse csr/dmc firmware version and augment debug message
> by printing it.
>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h  | 2 ++
>   drivers/gpu/drm/i915/intel_csr.c | 7 ++++++-
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b..17e8b25 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -755,6 +755,8 @@ struct intel_csr {
>   	const char *fw_path;
>   	uint32_t *dmc_payload;
>   	uint32_t dmc_fw_size;
> +	uint16_t dmc_ver_major;
> +	uint16_t dmc_ver_minor;
>   	uint32_t mmio_count;
>   	uint32_t mmioaddr[8];
>   	uint32_t mmiodata[8];
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index b69264d..58edc3f 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -377,11 +377,16 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	dmc_payload = csr->dmc_payload;
>   	memcpy(dmc_payload, &fw->data[readcount], nbytes);
>   
> +	csr->dmc_ver_major = dmc_header->header_ver;
> +	csr->dmc_ver_minor = ((dmc_header->fw_version & 0xffff0000) >> 16) * 10
> +		+ (dmc_header->fw_version & 0x0000ffff);
> +

I am not able to locate the the way major and minor version is derived, is it present in bspec?

-Animesh

>   	/* load csr program during system boot, as needed for DC states */
>   	intel_csr_load_program(dev);
>   	fw_loaded = true;
>   
> -	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> +	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
> +		      csr->dmc_ver_major, csr->dmc_ver_minor);
>   out:
>   	if (fw_loaded)
>   		intel_runtime_pm_put(dev_priv);

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

Re: [PATCH 1/5] drm/i915: Store and print dmc firmware version

[Damien Lespiau]

On Fri, Sep 18, 2015 at 06:17:05PM +0300, Mika Kuoppala wrote:
> Parse csr/dmc firmware version and augment debug message
> by printing it.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

FWIW I did something similar in the past, but that contribution was
denied. I also had the DC states entry counts, which sounds like
something super useful to have:

  http://lists.freedesktop.org/archives/intel-gfx/2015-June/thread.html

The DMC firmware version decoding was different in my patch and I'm sure
it worked then. Maye the header has changed :(

Feel free to use any part of that series with your authorship.

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

Re: [PATCH 1/5] drm/i915: Store and print dmc firmware version

[Damien Lespiau]

On Thu, Oct 08, 2015 at 12:03:30PM +0100, Damien Lespiau wrote:
> The DMC firmware version decoding was different in my patch and I'm sure
> it worked then. Maye the header has changed :(

By the way, if this is indeed the case, could you fix
intel_firmware_decode as well?

  http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tools/intel_firmware_decode.c

Thanks,

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

Re: [PATCH 1/5] drm/i915: Store and print dmc firmware version

[Mika Kuoppala]

Damien Lespiau <damien.lespiau@intel.com> writes:

> On Thu, Oct 08, 2015 at 12:03:30PM +0100, Damien Lespiau wrote:
>> The DMC firmware version decoding was different in my patch and I'm sure
>> it worked then. Maye the header has changed :(
>
> By the way, if this is indeed the case, could you fix
> intel_firmware_decode as well?
>
>   http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tools/intel_firmware_decode.c
>

Well this seems to be somewhat convoluted. The CSS header has a version
and then each binary package has their own. The fw version in binary
package seems to code only the minor part like in my patches.

I went through various firmware images and these seems to be in sync
currently so that version in CSS follows the version in dmc header,
albeit with different encoding.

There is not much info on bspec but I guess we should just
use the version provided in CSS header.

-Mika

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