[PATCH] drm/i915/huc: Check HuC status in dedicated function

[PATCH] drm/i915/huc: Check HuC status in dedicated function

[Michal Wajdeczko]

We try to keep all HuC related code in dedicated file.
There is no need to peek HuC register directly during
handling getparam ioctl.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  |  6 +++---
 drivers/gpu/drm/i915/intel_huc.c | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_huc.h |  1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f03555e..a902e88 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -377,9 +377,9 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
 		value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool;
 		break;
 	case I915_PARAM_HUC_STATUS:
-		intel_runtime_pm_get(dev_priv);
-		value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
-		intel_runtime_pm_put(dev_priv);
+		value = intel_huc_check_status(&dev_priv->huc);
+		if (value < 0)
+			return value;
 		break;
 	case I915_PARAM_MMAP_GTT_VERSION:
 		/* Though we've started our numbering from 1, and so class all
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 1d6c47b..2912852 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -92,3 +92,28 @@ int intel_huc_auth(struct intel_huc *huc)
 	DRM_ERROR("HuC: Authentication failed %d\n", ret);
 	return ret;
 }
+
+/**
+ * intel_huc_check_status() - check HuC status
+ * @huc: intel_huc structure
+ *
+ * This function reads status register to verify if HuC
+ * firmware was successfully loaded.
+ *
+ * Returns positive value if HuC firmware is loaded and verified
+ * and -ENODEV if HuC is not present.
+ */
+int intel_huc_check_status(struct intel_huc *huc)
+{
+	struct drm_i915_private *dev_priv = huc_to_i915(huc);
+	u32 status;
+
+	if (!HAS_HUC(dev_priv))
+		return -ENODEV;
+
+	intel_runtime_pm_get(dev_priv);
+	status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
+	intel_runtime_pm_put(dev_priv);
+
+	return status;
+}
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index b185850..aa85490 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -37,6 +37,7 @@ struct intel_huc {
 
 void intel_huc_init_early(struct intel_huc *huc);
 int intel_huc_auth(struct intel_huc *huc);
+int intel_huc_check_status(struct intel_huc *huc);
 
 static inline int intel_huc_sanitize(struct intel_huc *huc)
 {
-- 
1.9.1

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

Re: [PATCH] drm/i915/huc: Check HuC status in dedicated function

[Michel Thierry]

On 14/03/18 13:04, Michal Wajdeczko wrote:
> We try to keep all HuC related code in dedicated file.
> There is no need to peek HuC register directly during
> handling getparam ioctl.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c  |  6 +++---
>   drivers/gpu/drm/i915/intel_huc.c | 25 +++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_huc.h |  1 +
>   3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f03555e..a902e88 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -377,9 +377,9 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   		value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool;
>   		break;
>   	case I915_PARAM_HUC_STATUS:
> -		intel_runtime_pm_get(dev_priv);
> -		value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> -		intel_runtime_pm_put(dev_priv);
> +		value = intel_huc_check_status(&dev_priv->huc);
> +		if (value < 0)
> +			return value;
>   		break;
>   	case I915_PARAM_MMAP_GTT_VERSION:
>   		/* Though we've started our numbering from 1, and so class all
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 1d6c47b..2912852 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -92,3 +92,28 @@ int intel_huc_auth(struct intel_huc *huc)
>   	DRM_ERROR("HuC: Authentication failed %d\n", ret);
>   	return ret;
>   }
> +
> +/**
> + * intel_huc_check_status() - check HuC status
> + * @huc: intel_huc structure
> + *
> + * This function reads status register to verify if HuC
> + * firmware was successfully loaded.
> + *
> + * Returns positive value if HuC firmware is loaded and verified
> + * and -ENODEV if HuC is not present.

Before if huc was not loaded, get_param would just return 0 and the 
ioctl call would be OK. Maybe there's a test somewhere that would break? 
(I'm not arguing -ENODEV is better).

Otherwise,

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> + */
> +int intel_huc_check_status(struct intel_huc *huc)
> +{
> +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
> +	u32 status;
> +
> +	if (!HAS_HUC(dev_priv))
> +		return -ENODEV;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> +	intel_runtime_pm_put(dev_priv);
> +
> +	return status;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> index b185850..aa85490 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -37,6 +37,7 @@ struct intel_huc {
>   
>   void intel_huc_init_early(struct intel_huc *huc);
>   int intel_huc_auth(struct intel_huc *huc);
> +int intel_huc_check_status(struct intel_huc *huc);
>   
>   static inline int intel_huc_sanitize(struct intel_huc *huc)
>   {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/huc: Check HuC status in dedicated function

[Patchwork]

== Series Details ==

Series: drm/i915/huc: Check HuC status in dedicated function
URL   : https://patchwork.freedesktop.org/series/39986/
State : success

== Summary ==

Series 39986v1 drm/i915/huc: Check HuC status in dedicated function
https://patchwork.freedesktop.org/api/1.0/series/39986/revisions/1/mbox/

---- Possible new issues:

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-skl-6770hq)

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:446s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:535s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:516s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:519s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:505s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:587s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:427s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:318s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:424s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:514s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:651s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:440s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:530s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:544s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:512s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:242  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:398s
Blacklisted hosts:
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:545s

86e964296fe8bc85fdb624fa75b4cd83fcfb58cd drm-tip: 2018y-03m-14d-17h-40m-20s UTC integration manifest
d9e8e4b60d8c drm/i915/huc: Check HuC status in dedicated function

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8353/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/huc: Check HuC status in dedicated function

[Michal Wajdeczko]

On Wed, 14 Mar 2018 21:17:29 +0100, Michel Thierry  
<michel.thierry@intel.com> wrote:

> On 14/03/18 13:04, Michal Wajdeczko wrote:
>> We try to keep all HuC related code in dedicated file.
>> There is no need to peek HuC register directly during
>> handling getparam ioctl.
>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c  |  6 +++---
>>   drivers/gpu/drm/i915/intel_huc.c | 25 +++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_huc.h |  1 +
>>   3 files changed, 29 insertions(+), 3 deletions(-)
>>  diff --git a/drivers/gpu/drm/i915/i915_drv.c  
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index f03555e..a902e88 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -377,9 +377,9 @@ static int i915_getparam_ioctl(struct drm_device  
>> *dev, void *data,
>>   		value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool;
>>   		break;
>>   	case I915_PARAM_HUC_STATUS:
>> -		intel_runtime_pm_get(dev_priv);
>> -		value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> -		intel_runtime_pm_put(dev_priv);
>> +		value = intel_huc_check_status(&dev_priv->huc);
>> +		if (value < 0)
>> +			return value;
>>   		break;
>>   	case I915_PARAM_MMAP_GTT_VERSION:
>>   		/* Though we've started our numbering from 1, and so class all
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 1d6c47b..2912852 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -92,3 +92,28 @@ int intel_huc_auth(struct intel_huc *huc)
>>   	DRM_ERROR("HuC: Authentication failed %d\n", ret);
>>   	return ret;
>>   }
>> +
>> +/**
>> + * intel_huc_check_status() - check HuC status
>> + * @huc: intel_huc structure
>> + *
>> + * This function reads status register to verify if HuC
>> + * firmware was successfully loaded.
>> + *
>> + * Returns positive value if HuC firmware is loaded and verified
>> + * and -ENODEV if HuC is not present.
>
> Before if huc was not loaded, get_param would just return 0 and the  
> ioctl call would be OK.

There is another potential problem, as in case HuC was loaded, this
getparam would return specific bit from register instead of predefined
value that shall indicate "loaded/verified" like "1".
What if this bit from register will be moved on future platforms?
Should we still return this old one?

> Maybe there's a test somewhere that would break?

I hope not, and given above concern, I assume we can still modify it.
Is there any documentation what to expect from this getparam?

> (I'm not arguing -ENODEV is better).

In all other places (like debugfs) we return -ENODEV if user wants
to access HuC data on platform without HuC, so I think we should be
consistent.

>
> Otherwise,
>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
>
>> + */
>> +int intel_huc_check_status(struct intel_huc *huc)
>> +{
>> +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
>> +	u32 status;
>> +
>> +	if (!HAS_HUC(dev_priv))
>> +		return -ENODEV;
>> +
>> +	intel_runtime_pm_get(dev_priv);
>> +	status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> +	intel_runtime_pm_put(dev_priv);
>> +
>> +	return status;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_huc.h  
>> b/drivers/gpu/drm/i915/intel_huc.h
>> index b185850..aa85490 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>> @@ -37,6 +37,7 @@ struct intel_huc {
>>     void intel_huc_init_early(struct intel_huc *huc);
>>   int intel_huc_auth(struct intel_huc *huc);
>> +int intel_huc_check_status(struct intel_huc *huc);
>>     static inline int intel_huc_sanitize(struct intel_huc *huc)
>>   {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/huc: Check HuC status in dedicated function

[Michel Thierry]

On 14/03/18 15:23, Michal Wajdeczko wrote:
> On Wed, 14 Mar 2018 21:17:29 +0100, Michel Thierry 
> <michel.thierry@intel.com> wrote:
> 
>> On 14/03/18 13:04, Michal Wajdeczko wrote:
>>> We try to keep all HuC related code in dedicated file.
>>> There is no need to peek HuC register directly during
>>> handling getparam ioctl.
>>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Michel Thierry <michel.thierry@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c  |  6 +++---
>>>   drivers/gpu/drm/i915/intel_huc.c | 25 +++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_huc.h |  1 +
>>>   3 files changed, 29 insertions(+), 3 deletions(-)
>>>  diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index f03555e..a902e88 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -377,9 +377,9 @@ static int i915_getparam_ioctl(struct drm_device 
>>> *dev, void *data,
>>>           value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool;
>>>           break;
>>>       case I915_PARAM_HUC_STATUS:
>>> -        intel_runtime_pm_get(dev_priv);
>>> -        value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>> -        intel_runtime_pm_put(dev_priv);
>>> +        value = intel_huc_check_status(&dev_priv->huc);
>>> +        if (value < 0)
>>> +            return value;
>>>           break;
>>>       case I915_PARAM_MMAP_GTT_VERSION:
>>>           /* Though we've started our numbering from 1, and so class all
>>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
>>> b/drivers/gpu/drm/i915/intel_huc.c
>>> index 1d6c47b..2912852 100644
>>> --- a/drivers/gpu/drm/i915/intel_huc.c
>>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>> @@ -92,3 +92,28 @@ int intel_huc_auth(struct intel_huc *huc)
>>>       DRM_ERROR("HuC: Authentication failed %d\n", ret);
>>>       return ret;
>>>   }
>>> +
>>> +/**
>>> + * intel_huc_check_status() - check HuC status
>>> + * @huc: intel_huc structure
>>> + *
>>> + * This function reads status register to verify if HuC
>>> + * firmware was successfully loaded.
>>> + *
>>> + * Returns positive value if HuC firmware is loaded and verified
>>> + * and -ENODEV if HuC is not present.
>>
>> Before if huc was not loaded, get_param would just return 0 and the 
>> ioctl call would be OK.
> 
> There is another potential problem, as in case HuC was loaded, this
> getparam would return specific bit from register instead of predefined
> value that shall indicate "loaded/verified" like "1".
> What if this bit from register will be moved on future platforms?

Really? ;)
I once heard someone claiming he had talked with the hw team and they've 
agreed not to randomly shuffle register specifications
(please laugh with me).

> Should we still return this old one?
> 
>> Maybe there's a test somewhere that would break?
> 
> I hope not, and given above concern, I assume we can still modify it.
> Is there any documentation what to expect from this getparam?
> 

And the media driver would survive the change [1] if that's what we care 
about.

But is the response from getparam ABI? I would say just 
drm_i915_getparam_t is.

[1] 
https://github.com/intel/media-driver/blob/c0321bc88e12247d21fa2d0b470cff841c3712ba/media_driver/linux/common/os/hwinfo_linux.c#L254

>> (I'm not arguing -ENODEV is better).
> 
> In all other places (like debugfs) we return -ENODEV if user wants
> to access HuC data on platform without HuC, so I think we should be
> consistent.
> 

sorry, a missing comma... I was agreeing with you:
 >> (I'm not arguing, -ENODEV is better).


>>
>> Otherwise,
>>
>> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
>>
>>> + */
>>> +int intel_huc_check_status(struct intel_huc *huc)
>>> +{
>>> +    struct drm_i915_private *dev_priv = huc_to_i915(huc);
>>> +    u32 status;
>>> +
>>> +    if (!HAS_HUC(dev_priv))
>>> +        return -ENODEV;
>>> +
>>> +    intel_runtime_pm_get(dev_priv);
>>> +    status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>> +    intel_runtime_pm_put(dev_priv);
>>> +
>>> +    return status;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/intel_huc.h 
>>> b/drivers/gpu/drm/i915/intel_huc.h
>>> index b185850..aa85490 100644
>>> --- a/drivers/gpu/drm/i915/intel_huc.h
>>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>>> @@ -37,6 +37,7 @@ struct intel_huc {
>>>     void intel_huc_init_early(struct intel_huc *huc);
>>>   int intel_huc_auth(struct intel_huc *huc);
>>> +int intel_huc_check_status(struct intel_huc *huc);
>>>     static inline int intel_huc_sanitize(struct intel_huc *huc)
>>>   {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915/huc: Check HuC status in dedicated function

[Patchwork]

== Series Details ==

Series: drm/i915/huc: Check HuC status in dedicated function
URL   : https://patchwork.freedesktop.org/series/39986/
State : failure

== Summary ==

---- Possible new issues:

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-draw-pwrite:
                pass       -> FAIL       (shard-apl)

---- Known issues:

Test drv_suspend:
        Subgroup debugfs-reader:
                pass       -> SKIP       (shard-snb) fdo#102365
Test gem_eio:
        Subgroup in-flight-external:
                incomplete -> PASS       (shard-apl) fdo#105341 +1
Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate:
                fail       -> PASS       (shard-hsw) fdo#100368 +1

fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-apl        total:3401 pass:1789 dwarn:1   dfail:0   fail:8   skip:1601 time:12464s
shard-hsw        total:3441 pass:1766 dwarn:1   dfail:0   fail:1   skip:1672 time:11873s
shard-snb        total:3441 pass:1354 dwarn:1   dfail:0   fail:3   skip:2083 time:7129s
Blacklisted hosts:
shard-kbl        total:3425 pass:1925 dwarn:1   dfail:0   fail:9   skip:1488 time:9137s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8353/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/huc: Check HuC status in dedicated function

[Chris Wilson]

Quoting Michel Thierry (2018-03-14 23:34:33)
> On 14/03/18 15:23, Michal Wajdeczko wrote:
> > On Wed, 14 Mar 2018 21:17:29 +0100, Michel Thierry 
> > <michel.thierry@intel.com> wrote:
> > 
> >> On 14/03/18 13:04, Michal Wajdeczko wrote:
> >>> We try to keep all HuC related code in dedicated file.
> >>> There is no need to peek HuC register directly during
> >>> handling getparam ioctl.
> >>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>> Cc: Michel Thierry <michel.thierry@intel.com>
> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_drv.c  |  6 +++---
> >>>   drivers/gpu/drm/i915/intel_huc.c | 25 +++++++++++++++++++++++++
> >>>   drivers/gpu/drm/i915/intel_huc.h |  1 +
> >>>   3 files changed, 29 insertions(+), 3 deletions(-)
> >>>  diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> >>> b/drivers/gpu/drm/i915/i915_drv.c
> >>> index f03555e..a902e88 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> @@ -377,9 +377,9 @@ static int i915_getparam_ioctl(struct drm_device 
> >>> *dev, void *data,
> >>>           value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool;
> >>>           break;
> >>>       case I915_PARAM_HUC_STATUS:
> >>> -        intel_runtime_pm_get(dev_priv);
> >>> -        value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> >>> -        intel_runtime_pm_put(dev_priv);
> >>> +        value = intel_huc_check_status(&dev_priv->huc);
> >>> +        if (value < 0)
> >>> +            return value;
> >>>           break;
> >>>       case I915_PARAM_MMAP_GTT_VERSION:
> >>>           /* Though we've started our numbering from 1, and so class all
> >>> diff --git a/drivers/gpu/drm/i915/intel_huc.c 
> >>> b/drivers/gpu/drm/i915/intel_huc.c
> >>> index 1d6c47b..2912852 100644
> >>> --- a/drivers/gpu/drm/i915/intel_huc.c
> >>> +++ b/drivers/gpu/drm/i915/intel_huc.c
> >>> @@ -92,3 +92,28 @@ int intel_huc_auth(struct intel_huc *huc)
> >>>       DRM_ERROR("HuC: Authentication failed %d\n", ret);
> >>>       return ret;
> >>>   }
> >>> +
> >>> +/**
> >>> + * intel_huc_check_status() - check HuC status
> >>> + * @huc: intel_huc structure
> >>> + *
> >>> + * This function reads status register to verify if HuC
> >>> + * firmware was successfully loaded.
> >>> + *
> >>> + * Returns positive value if HuC firmware is loaded and verified
> >>> + * and -ENODEV if HuC is not present.
> >>
> >> Before if huc was not loaded, get_param would just return 0 and the 
> >> ioctl call would be OK.
> > 
> > There is another potential problem, as in case HuC was loaded, this
> > getparam would return specific bit from register instead of predefined
> > value that shall indicate "loaded/verified" like "1".
> > What if this bit from register will be moved on future platforms?
> 
> Really? ;)
> I once heard someone claiming he had talked with the hw team and they've 
> agreed not to randomly shuffle register specifications
> (please laugh with me).
> 
> > Should we still return this old one?
> > 
> >> Maybe there's a test somewhere that would break?
> > 
> > I hope not, and given above concern, I assume we can still modify it.
> > Is there any documentation what to expect from this getparam?
> > 
> 
> And the media driver would survive the change [1] if that's what we care 
> about.
> 
> But is the response from getparam ABI? I would say just 
> drm_i915_getparam_t is.

So long as we obey the rule to not overwrite on error, we should be
within the existing ABI. Then we only have the argument over what errno
suits the ABI.

-ENODEV is what we have been using for a user call for a function not
supported by the HW, so fine by me.

Pushed, thanks for the patch and review.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx