Re: [Intel-gfx] [PATCH] drm/i915/huc: Check HuC status in dedicated function
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
> > 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
> >>> Cc: Michel Thierry
> >>> Cc: Rodrigo Vivi
> >>> Cc: Anusha Srivatsa
> >>> ---
> >>> 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
Re: [Intel-gfx] [PATCH] drm/i915/huc: Check HuC status in dedicated function
On 14/03/18 15:23, Michal Wajdeczko wrote:
On Wed, 14 Mar 2018 21:17:29 +0100, Michel Thierry
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
Cc: Michel Thierry
Cc: Rodrigo Vivi
Cc: Anusha Srivatsa
---
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
+ */
+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: [Intel-gfx] [PATCH] drm/i915/huc: Check HuC status in dedicated function
On Wed, 14 Mar 2018 21:17:29 +0100, Michel Thierry
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
Cc: Michel Thierry
Cc: Rodrigo Vivi
Cc: Anusha Srivatsa
---
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
+ */
+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: [Intel-gfx] [PATCH] drm/i915/huc: Check HuC status in dedicated function
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
Cc: Michel Thierry
Cc: Rodrigo Vivi
Cc: Anusha Srivatsa
---
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
+ */
+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
[Intel-gfx] [PATCH] drm/i915/huc: Check HuC status in dedicated function
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
Cc: Michel Thierry
Cc: Rodrigo Vivi
Cc: Anusha Srivatsa
---
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
