| Message ID | 1481840990-31191-9-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > From: Peter Antoine <peter.antoine@intel.com> > > This patch will allow for getparams to return the status of the HuC. > As the HuC has to be validated by the GuC this patch uses the validated > status to show when the HuC is loaded and ready for use. You cannot use > the loaded status as with the GuC as the HuC is verified after it is > loaded and is not usable until it is verified. > > v2: removed the forewakes as the registers are already force-woken. > (T.Ursulin) > v4: rebased. > v5: rebased on top of drm-tip. > v6: rebased. Removed any reference to intel_huc.h > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > Remove intel_is_huc_valid() since it is used only in one place. > Put the case of I915_PARAM_HAS_HUC() in the right place. > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > include/uapi/drm/i915_drm.h | 1 + > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 85a47c2..0bc016d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -49,6 +49,7 @@ > #include "i915_trace.h" > #include "i915_vgpu.h" > #include "intel_drv.h" > +#include "intel_uc.h" > > static struct drm_driver driver; > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > case I915_PARAM_MIN_EU_IN_POOL: > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > break; > + case I915_PARAM_HUC_STATUS: > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; Same question as last time: does the device need to be awake? We know is one of the GT power wells, so presumably we need an rpm_get/rpm_put as well to access the register. -Chris
On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > > From: Peter Antoine <peter.antoine@intel.com> > > > > This patch will allow for getparams to return the status of the HuC. > > As the HuC has to be validated by the GuC this patch uses the validated > > status to show when the HuC is loaded and ready for use. You cannot use > > the loaded status as with the GuC as the HuC is verified after it is > > loaded and is not usable until it is verified. > > > > v2: removed the forewakes as the registers are already force-woken. > > (T.Ursulin) > > v4: rebased. > > v5: rebased on top of drm-tip. > > v6: rebased. Removed any reference to intel_huc.h > > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > > Remove intel_is_huc_valid() since it is used only in one place. > > Put the case of I915_PARAM_HAS_HUC() in the right place. > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > > include/uapi/drm/i915_drm.h | 1 + > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 85a47c2..0bc016d 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -49,6 +49,7 @@ > > #include "i915_trace.h" > > #include "i915_vgpu.h" > > #include "intel_drv.h" > > +#include "intel_uc.h" > > > > static struct drm_driver driver; > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > case I915_PARAM_MIN_EU_IN_POOL: > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > > break; > > + case I915_PARAM_HUC_STATUS: > > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > > Same question as last time: does the device need to be awake? We know is > one of the GT power wells, so presumably we need an rpm_get/rpm_put as > well to access the register. > -Chris I get: [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 PRE 24704 [ 1588.571285] [drm:intel_runtime_suspend [i915]] Suspending device [ 1588.575768] [drm:intel_runtime_suspend [i915]] Device suspended [ 1588.577156] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 [ 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device consistently from: value = I915_READ(HUC_STATUS2); DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); i915_pm_ops.runtime_suspend(dev_priv->drm.dev); value = I915_READ(HUC_STATUS2); DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); i915_pm_ops.runtime_resume(dev_priv->drm.dev); > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > From: Peter Antoine <peter.antoine@intel.com> > > This patch will allow for getparams to return the status of the HuC. > As the HuC has to be validated by the GuC this patch uses the validated > status to show when the HuC is loaded and ready for use. You cannot use > the loaded status as with the GuC as the HuC is verified after it is > loaded and is not usable until it is verified. > > v2: removed the forewakes as the registers are already force-woken. > (T.Ursulin) > v4: rebased. > v5: rebased on top of drm-tip. > v6: rebased. Removed any reference to intel_huc.h > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > Remove intel_is_huc_valid() since it is used only in one place. > Put the case of I915_PARAM_HAS_HUC() in the right place. > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> You've retained my rb without asking me. With the changes you've made and confirmation that MEDIA FW that I915_READ() assumes: Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > include/uapi/drm/i915_drm.h | 1 + > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 85a47c2..0bc016d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -49,6 +49,7 @@ > #include "i915_trace.h" > #include "i915_vgpu.h" > #include "intel_drv.h" > +#include "intel_uc.h" > > static struct drm_driver driver; > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > case I915_PARAM_MIN_EU_IN_POOL: > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > break; > + case I915_PARAM_HUC_STATUS: > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > + break; > case I915_PARAM_MMAP_GTT_VERSION: > /* Though we've started our numbering from 1, and so class all > * earlier versions as 0, in effect their value is undefined as > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c > index d8c5266..b06a613 100644 > --- a/drivers/gpu/drm/i915/intel_huc_loader.c > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c > @@ -288,4 +288,3 @@ void intel_huc_fini(struct drm_device *dev) > > huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE; > } > - > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index da32c2f..57093b4 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -395,6 +395,7 @@ typedef struct drm_i915_irq_wait { > * priorities and the driver will attempt to execute batches in priority order. > */ > #define I915_PARAM_HAS_SCHEDULER 41 > +#define I915_PARAM_HUC_STATUS 42 > > typedef struct drm_i915_getparam { > __s32 param; > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > > > From: Peter Antoine <peter.antoine@intel.com> > > > > > > This patch will allow for getparams to return the status of the HuC. > > > As the HuC has to be validated by the GuC this patch uses the validated > > > status to show when the HuC is loaded and ready for use. You cannot use > > > the loaded status as with the GuC as the HuC is verified after it is > > > loaded and is not usable until it is verified. > > > > > > v2: removed the forewakes as the registers are already force-woken. > > > (T.Ursulin) > > > v4: rebased. > > > v5: rebased on top of drm-tip. > > > v6: rebased. Removed any reference to intel_huc.h > > > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > > > Remove intel_is_huc_valid() since it is used only in one place. > > > Put the case of I915_PARAM_HAS_HUC() in the right place. > > > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > > > include/uapi/drm/i915_drm.h | 1 + > > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 85a47c2..0bc016d 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -49,6 +49,7 @@ > > > #include "i915_trace.h" > > > #include "i915_vgpu.h" > > > #include "intel_drv.h" > > > +#include "intel_uc.h" > > > > > > static struct drm_driver driver; > > > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > case I915_PARAM_MIN_EU_IN_POOL: > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > > > break; > > > + case I915_PARAM_HUC_STATUS: > > > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > > > > Same question as last time: does the device need to be awake? We know is > > one of the GT power wells, so presumably we need an rpm_get/rpm_put as > > well to access the register. > > -Chris > > I get: > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 PRE 24704 > [ 1588.571285] [drm:intel_runtime_suspend [i915]] Suspending device > [ 1588.575768] [drm:intel_runtime_suspend [i915]] Device suspended > [ 1588.577156] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 > [ 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device > > consistently from: > > value = I915_READ(HUC_STATUS2); > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); > > value = I915_READ(HUC_STATUS2); > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); > i915_pm_ops.runtime_resume(dev_priv->drm.dev); Also do the test with i915.mmio_debug=9999 -Chris
On Fri, Dec 16, 2016 at 04:12:36PM +0000, Chris Wilson wrote: > On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: > > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: > > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > > > > From: Peter Antoine <peter.antoine@intel.com> > > > > > > > > This patch will allow for getparams to return the status of the HuC. > > > > As the HuC has to be validated by the GuC this patch uses the validated > > > > status to show when the HuC is loaded and ready for use. You cannot use > > > > the loaded status as with the GuC as the HuC is verified after it is > > > > loaded and is not usable until it is verified. > > > > > > > > v2: removed the forewakes as the registers are already force-woken. > > > > (T.Ursulin) > > > > v4: rebased. > > > > v5: rebased on top of drm-tip. > > > > v6: rebased. Removed any reference to intel_huc.h > > > > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > > > > Remove intel_is_huc_valid() since it is used only in one place. > > > > Put the case of I915_PARAM_HAS_HUC() in the right place. > > > > > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > > > > include/uapi/drm/i915_drm.h | 1 + > > > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index 85a47c2..0bc016d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -49,6 +49,7 @@ > > > > #include "i915_trace.h" > > > > #include "i915_vgpu.h" > > > > #include "intel_drv.h" > > > > +#include "intel_uc.h" > > > > > > > > static struct drm_driver driver; > > > > > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > > case I915_PARAM_MIN_EU_IN_POOL: > > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > > > > break; > > > > + case I915_PARAM_HUC_STATUS: > > > > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > > > > > > Same question as last time: does the device need to be awake? We know is > > > one of the GT power wells, so presumably we need an rpm_get/rpm_put as > > > well to access the register. > > > -Chris > > > > I get: > > > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 PRE 24704 > > [ 1588.571285] [drm:intel_runtime_suspend [i915]] Suspending device > > [ 1588.575768] [drm:intel_runtime_suspend [i915]] Device suspended > > [ 1588.577156] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 > > [ 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device > > > > consistently from: > > > > value = I915_READ(HUC_STATUS2); > > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); > > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); > > > > value = I915_READ(HUC_STATUS2); > > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); > > i915_pm_ops.runtime_resume(dev_priv->drm.dev); > > Also do the test with i915.mmio_debug=9999 > -Chris Same effect. Works.
On Fri, Dec 16, 2016 at 05:21:38PM +0100, Arkadiusz Hiler wrote: > On Fri, Dec 16, 2016 at 04:12:36PM +0000, Chris Wilson wrote: > > On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: > > > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: > > > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > > > > > From: Peter Antoine <peter.antoine@intel.com> > > > > > > > > > > This patch will allow for getparams to return the status of the HuC. > > > > > As the HuC has to be validated by the GuC this patch uses the validated > > > > > status to show when the HuC is loaded and ready for use. You cannot use > > > > > the loaded status as with the GuC as the HuC is verified after it is > > > > > loaded and is not usable until it is verified. > > > > > > > > > > v2: removed the forewakes as the registers are already force-woken. > > > > > (T.Ursulin) > > > > > v4: rebased. > > > > > v5: rebased on top of drm-tip. > > > > > v6: rebased. Removed any reference to intel_huc.h > > > > > v7: rebased. Rename I915_PARAM_HAS_HUC to I915_PARAM_HUC_STATUS. > > > > > Remove intel_is_huc_valid() since it is used only in one place. > > > > > Put the case of I915_PARAM_HAS_HUC() in the right place. > > > > > > > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > > > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > > > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > > > > > include/uapi/drm/i915_drm.h | 1 + > > > > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 85a47c2..0bc016d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -49,6 +49,7 @@ > > > > > #include "i915_trace.h" > > > > > #include "i915_vgpu.h" > > > > > #include "intel_drv.h" > > > > > +#include "intel_uc.h" > > > > > > > > > > static struct drm_driver driver; > > > > > > > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > > > case I915_PARAM_MIN_EU_IN_POOL: > > > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > > > > > break; > > > > > + case I915_PARAM_HUC_STATUS: > > > > > + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > > > > > > > > Same question as last time: does the device need to be awake? We know is > > > > one of the GT power wells, so presumably we need an rpm_get/rpm_put as > > > > well to access the register. > > > > -Chris > > > > > > I get: > > > > > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 PRE 24704 > > > [ 1588.571285] [drm:intel_runtime_suspend [i915]] Suspending device > > > [ 1588.575768] [drm:intel_runtime_suspend [i915]] Device suspended > > > [ 1588.577156] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 > > > [ 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device > > > > > > consistently from: > > > > > > value = I915_READ(HUC_STATUS2); > > > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); > > > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); > > > > > > value = I915_READ(HUC_STATUS2); > > > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); > > > i915_pm_ops.runtime_resume(dev_priv->drm.dev); > > > > Also do the test with i915.mmio_debug=9999 > > -Chris > > Same effect. Works. Ok, then just mark up that we don't need rpm here so that we don't freak out in future scans for mmio access outside of rpm. -Chris
>-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Friday, December 16, 2016 8:31 AM >To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to >getparams > >On Fri, Dec 16, 2016 at 05:21:38PM +0100, Arkadiusz Hiler wrote: >> On Fri, Dec 16, 2016 at 04:12:36PM +0000, Chris Wilson wrote: >> > On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: >> > > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: >> > > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: >> > > > > From: Peter Antoine <peter.antoine@intel.com> >> > > > > >> > > > > This patch will allow for getparams to return the status of the HuC. >> > > > > As the HuC has to be validated by the GuC this patch uses the >> > > > > validated status to show when the HuC is loaded and ready for >> > > > > use. You cannot use the loaded status as with the GuC as the >> > > > > HuC is verified after it is loaded and is not usable until it is verified. >> > > > > >> > > > > v2: removed the forewakes as the registers are already force-woken. >> > > > > (T.Ursulin) >> > > > > v4: rebased. >> > > > > v5: rebased on top of drm-tip. >> > > > > v6: rebased. Removed any reference to intel_huc.h >> > > > > v7: rebased. Rename I915_PARAM_HAS_HUC to >I915_PARAM_HUC_STATUS. >> > > > > Remove intel_is_huc_valid() since it is used only in one place. >> > > > > Put the case of I915_PARAM_HAS_HUC() in the right place. >> > > > > >> > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> >> > > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> > > > > --- >> > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ >> > > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - >> > > > > include/uapi/drm/i915_drm.h | 1 + >> > > > > 3 files changed, 5 insertions(+), 1 deletion(-) >> > > > > >> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c >> > > > > b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..0bc016d >> > > > > 100644 >> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c >> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c >> > > > > @@ -49,6 +49,7 @@ >> > > > > #include "i915_trace.h" >> > > > > #include "i915_vgpu.h" >> > > > > #include "intel_drv.h" >> > > > > +#include "intel_uc.h" >> > > > > >> > > > > static struct drm_driver driver; >> > > > > >> > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device >*dev, void *data, >> > > > > case I915_PARAM_MIN_EU_IN_POOL: >> > > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; >> > > > > break; >> > > > > + case I915_PARAM_HUC_STATUS: >> > > > > + value = I915_READ(HUC_STATUS2) & >HUC_FW_VERIFIED; >> > > > >> > > > Same question as last time: does the device need to be awake? We >> > > > know is one of the GT power wells, so presumably we need an >> > > > rpm_get/rpm_put as well to access the register. >> > > > -Chris >> > > >> > > I get: >> > > >> > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 >> > > PRE 24704 [ 1588.571285] [drm:intel_runtime_suspend [i915]] >> > > Suspending device [ 1588.575768] [drm:intel_runtime_suspend >> > > [i915]] Device suspended [ 1588.577156] >> > > [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 [ >> > > 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device >> > > >> > > consistently from: >> > > >> > > value = I915_READ(HUC_STATUS2); >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); >> > > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); >> > > >> > > value = I915_READ(HUC_STATUS2); >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); >> > > i915_pm_ops.runtime_resume(dev_priv->drm.dev); >> > >> > Also do the test with i915.mmio_debug=9999 -Chris >> >> Same effect. Works. Thanks Arek for confirming. >Ok, then just mark up that we don't need rpm here so that we don't freak out in >future scans for mmio access outside of rpm. >-Chris Chris, v2 as changed by Tvrtko suggests that forcewakes are removed since the register is force waken. Are you suggesting that adding that rpm not being required in the commit message will make things much more clearer? Anusha > >-- >Chris Wilson, Intel Open Source Technology Centre
On Fri, Dec 16, 2016 at 06:31:46PM +0000, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > >Sent: Friday, December 16, 2016 8:31 AM > >To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com> > >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > >gfx@lists.freedesktop.org > >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to > >getparams > > > >On Fri, Dec 16, 2016 at 05:21:38PM +0100, Arkadiusz Hiler wrote: > >> On Fri, Dec 16, 2016 at 04:12:36PM +0000, Chris Wilson wrote: > >> > On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: > >> > > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: > >> > > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: > >> > > > > From: Peter Antoine <peter.antoine@intel.com> > >> > > > > > >> > > > > This patch will allow for getparams to return the status of the HuC. > >> > > > > As the HuC has to be validated by the GuC this patch uses the > >> > > > > validated status to show when the HuC is loaded and ready for > >> > > > > use. You cannot use the loaded status as with the GuC as the > >> > > > > HuC is verified after it is loaded and is not usable until it is verified. > >> > > > > > >> > > > > v2: removed the forewakes as the registers are already force-woken. > >> > > > > (T.Ursulin) > >> > > > > v4: rebased. > >> > > > > v5: rebased on top of drm-tip. > >> > > > > v6: rebased. Removed any reference to intel_huc.h > >> > > > > v7: rebased. Rename I915_PARAM_HAS_HUC to > >I915_PARAM_HUC_STATUS. > >> > > > > Remove intel_is_huc_valid() since it is used only in one place. > >> > > > > Put the case of I915_PARAM_HAS_HUC() in the right place. > >> > > > > > >> > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > >> > > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > >> > > > > --- > >> > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ > >> > > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - > >> > > > > include/uapi/drm/i915_drm.h | 1 + > >> > > > > 3 files changed, 5 insertions(+), 1 deletion(-) > >> > > > > > >> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > >> > > > > b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..0bc016d > >> > > > > 100644 > >> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > >> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > >> > > > > @@ -49,6 +49,7 @@ > >> > > > > #include "i915_trace.h" > >> > > > > #include "i915_vgpu.h" > >> > > > > #include "intel_drv.h" > >> > > > > +#include "intel_uc.h" > >> > > > > > >> > > > > static struct drm_driver driver; > >> > > > > > >> > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device > >*dev, void *data, > >> > > > > case I915_PARAM_MIN_EU_IN_POOL: > >> > > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; > >> > > > > break; > >> > > > > + case I915_PARAM_HUC_STATUS: > >> > > > > + value = I915_READ(HUC_STATUS2) & > >HUC_FW_VERIFIED; > >> > > > > >> > > > Same question as last time: does the device need to be awake? We > >> > > > know is one of the GT power wells, so presumably we need an > >> > > > rpm_get/rpm_put as well to access the register. > >> > > > -Chris > >> > > > >> > > I get: > >> > > > >> > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 > >> > > PRE 24704 [ 1588.571285] [drm:intel_runtime_suspend [i915]] > >> > > Suspending device [ 1588.575768] [drm:intel_runtime_suspend > >> > > [i915]] Device suspended [ 1588.577156] > >> > > [drm:i915_huc_load_status_info [i915]] HUC_STATUS2 POST 24704 [ > >> > > 1588.578259] [drm:intel_runtime_resume [i915]] Resuming device > >> > > > >> > > consistently from: > >> > > > >> > > value = I915_READ(HUC_STATUS2); > >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); > >> > > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); > >> > > > >> > > value = I915_READ(HUC_STATUS2); > >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); > >> > > i915_pm_ops.runtime_resume(dev_priv->drm.dev); > >> > > >> > Also do the test with i915.mmio_debug=9999 -Chris > >> > >> Same effect. Works. > Thanks Arek for confirming. > > >Ok, then just mark up that we don't need rpm here so that we don't freak out in > >future scans for mmio access outside of rpm. > >-Chris > Chris, v2 as changed by Tvrtko suggests that forcewakes are removed since the register is force waken. Are you suggesting that adding that rpm not being required in the commit message will make things much more clearer? No, if you are eschewing taking rpm around mmio access, I want that commented upon in the code so that it is visible the next time we do an audit for rpm abuse/misuse. -Chris
>-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Friday, December 16, 2016 10:47 AM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >Cc: Hiler, Arkadiusz <arkadiusz.hiler@intel.com>; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to >getparams > >On Fri, Dec 16, 2016 at 06:31:46PM +0000, Srivatsa, Anusha wrote: >> >> >> >-----Original Message----- >> >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >> >Sent: Friday, December 16, 2016 8:31 AM >> >To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com> >> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >> >gfx@lists.freedesktop.org >> >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC >> >status to getparams >> > >> >On Fri, Dec 16, 2016 at 05:21:38PM +0100, Arkadiusz Hiler wrote: >> >> On Fri, Dec 16, 2016 at 04:12:36PM +0000, Chris Wilson wrote: >> >> > On Fri, Dec 16, 2016 at 03:43:46PM +0100, Arkadiusz Hiler wrote: >> >> > > On Thu, Dec 15, 2016 at 10:42:53PM +0000, Chris Wilson wrote: >> >> > > > On Thu, Dec 15, 2016 at 02:29:50PM -0800, anushasr wrote: >> >> > > > > From: Peter Antoine <peter.antoine@intel.com> >> >> > > > > >> >> > > > > This patch will allow for getparams to return the status of the HuC. >> >> > > > > As the HuC has to be validated by the GuC this patch uses >> >> > > > > the validated status to show when the HuC is loaded and >> >> > > > > ready for use. You cannot use the loaded status as with the >> >> > > > > GuC as the HuC is verified after it is loaded and is not usable until it is >verified. >> >> > > > > >> >> > > > > v2: removed the forewakes as the registers are already force-woken. >> >> > > > > (T.Ursulin) >> >> > > > > v4: rebased. >> >> > > > > v5: rebased on top of drm-tip. >> >> > > > > v6: rebased. Removed any reference to intel_huc.h >> >> > > > > v7: rebased. Rename I915_PARAM_HAS_HUC to >> >I915_PARAM_HUC_STATUS. >> >> > > > > Remove intel_is_huc_valid() since it is used only in one place. >> >> > > > > Put the case of I915_PARAM_HAS_HUC() in the right place. >> >> > > > > >> >> > > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com> >> >> > > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> >> > > > > --- >> >> > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++++ >> >> > > > > drivers/gpu/drm/i915/intel_huc_loader.c | 1 - >> >> > > > > include/uapi/drm/i915_drm.h | 1 + >> >> > > > > 3 files changed, 5 insertions(+), 1 deletion(-) >> >> > > > > >> >> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c >> >> > > > > b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..0bc016d >> >> > > > > 100644 >> >> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c >> >> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c >> >> > > > > @@ -49,6 +49,7 @@ >> >> > > > > #include "i915_trace.h" >> >> > > > > #include "i915_vgpu.h" >> >> > > > > #include "intel_drv.h" >> >> > > > > +#include "intel_uc.h" >> >> > > > > >> >> > > > > static struct drm_driver driver; >> >> > > > > >> >> > > > > @@ -315,6 +316,9 @@ static int i915_getparam(struct >> >> > > > > drm_device >> >*dev, void *data, >> >> > > > > case I915_PARAM_MIN_EU_IN_POOL: >> >> > > > > value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; >> >> > > > > break; >> >> > > > > + case I915_PARAM_HUC_STATUS: >> >> > > > > + value = I915_READ(HUC_STATUS2) & >> >HUC_FW_VERIFIED; >> >> > > > >> >> > > > Same question as last time: does the device need to be awake? >> >> > > > We know is one of the GT power wells, so presumably we need >> >> > > > an rpm_get/rpm_put as well to access the register. >> >> > > > -Chris >> >> > > >> >> > > I get: >> >> > > >> >> > > [ 1588.570174] [drm:i915_huc_load_status_info [i915]] >> >> > > HUC_STATUS2 PRE 24704 [ 1588.571285] >> >> > > [drm:intel_runtime_suspend [i915]] Suspending device [ >> >> > > 1588.575768] [drm:intel_runtime_suspend [i915]] Device >> >> > > suspended [ 1588.577156] [drm:i915_huc_load_status_info [i915]] >> >> > > HUC_STATUS2 POST 24704 [ 1588.578259] [drm:intel_runtime_resume >> >> > > [i915]] Resuming device >> >> > > >> >> > > consistently from: >> >> > > >> >> > > value = I915_READ(HUC_STATUS2); >> >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 PRE %d\n", value); >> >> > > i915_pm_ops.runtime_suspend(dev_priv->drm.dev); >> >> > > >> >> > > value = I915_READ(HUC_STATUS2); >> >> > > DRM_DEBUG_DRIVER("HUC_STATUS2 POST %d\n", value); >> >> > > i915_pm_ops.runtime_resume(dev_priv->drm.dev); >> >> > >> >> > Also do the test with i915.mmio_debug=9999 -Chris >> >> >> >> Same effect. Works. >> Thanks Arek for confirming. >> >> >Ok, then just mark up that we don't need rpm here so that we don't >> >freak out in future scans for mmio access outside of rpm. >> >-Chris >> Chris, v2 as changed by Tvrtko suggests that forcewakes are removed since the >register is force waken. Are you suggesting that adding that rpm not being >required in the commit message will make things much more clearer? > >No, if you are eschewing taking rpm around mmio access, I want that >commented upon in the code so that it is visible the next time we do an audit for >rpm abuse/misuse. Ah! Got it. Thanks. Cheers, Anusha >-Chris > >-- >Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..0bc016d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -49,6 +49,7 @@ #include "i915_trace.h" #include "i915_vgpu.h" #include "intel_drv.h" +#include "intel_uc.h" static struct drm_driver driver; @@ -315,6 +316,9 @@ static int i915_getparam(struct drm_device *dev, void *data, case I915_PARAM_MIN_EU_IN_POOL: value = INTEL_INFO(dev_priv)->sseu.min_eu_in_pool; break; + case I915_PARAM_HUC_STATUS: + value = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; + break; case I915_PARAM_MMAP_GTT_VERSION: /* Though we've started our numbering from 1, and so class all * earlier versions as 0, in effect their value is undefined as diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c index d8c5266..b06a613 100644 --- a/drivers/gpu/drm/i915/intel_huc_loader.c +++ b/drivers/gpu/drm/i915/intel_huc_loader.c @@ -288,4 +288,3 @@ void intel_huc_fini(struct drm_device *dev) huc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE; } - diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index da32c2f..57093b4 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -395,6 +395,7 @@ typedef struct drm_i915_irq_wait { * priorities and the driver will attempt to execute batches in priority order. */ #define I915_PARAM_HAS_SCHEDULER 41 +#define I915_PARAM_HUC_STATUS 42 typedef struct drm_i915_getparam { __s32 param;