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

[Michel Thierry <michel.thierry@intel.com>]

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