| Message ID | 1481238139-9509-9-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 08, 2016 at 03:02:19PM -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 > > 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 | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 1 + > include/uapi/drm/i915_drm.h | 1 + > 4 files changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 85a47c2..6be06a27 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; > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > */ > value = 1; > break; > + case I915_PARAM_HAS_HUC: > + value = intel_is_huc_valid(dev_priv); > + break; Why did you put it here? It breaks the pattern of case statements. > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c > index 96fc727..6704cc8 100644 > --- a/drivers/gpu/drm/i915/intel_huc_loader.c > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c > @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) > huc_fw->fetch_status = UC_FIRMWARE_NONE; > } > > +/** > + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. > + * @dev_priv: drm device to check. > + * > + * This function will return true if the guc has been loaded and > + * has valid firmware. The simplest way of doing this is to check > + * if the HuC has been validated, if so it must have been loaded. > + */ > +int intel_is_huc_valid(struct drm_i915_private *dev_priv) bool > +{ > + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); (brackets (because (brackets))) But what I really wanted to ask... Does this register access require the device to be awake and powered? -Chris
On Thu, Dec 08, 2016 at 03:02:19PM -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 > > 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 | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 1 + > include/uapi/drm/i915_drm.h | 1 + > 4 files changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 85a47c2..6be06a27 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; > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > */ > value = 1; > break; > + case I915_PARAM_HAS_HUC: For me this param name does not match returned result which maybe misleading. Note that other HAS params return static driver/hw capability, not runtime. I guess PARAM_HUC_STATUS would be better (0=no huc, 1=pending, 2=ok, -1=failed) And you can cache huc status in intel_huc struct and make final modification in intel_huc_auth() function to avoid registry read (unless we want to detect later crash of the huc using this reg read) > + value = intel_is_huc_valid(dev_priv); > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c > index 96fc727..6704cc8 100644 > --- a/drivers/gpu/drm/i915/intel_huc_loader.c > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c > @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) > huc_fw->fetch_status = UC_FIRMWARE_NONE; > } > > +/** > + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. > + * @dev_priv: drm device to check. > + * > + * This function will return true if the guc has been loaded and Please change function return type to bool to match this description. > + * has valid firmware. The simplest way of doing this is to check > + * if the HuC has been validated, if so it must have been loaded. > + */ > +int intel_is_huc_valid(struct drm_i915_private *dev_priv) > +{ > + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); > +} > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 1db8bc2..ccd3f69 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -226,6 +226,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); > void intel_huc_init(struct drm_i915_private *dev_priv); > void intel_huc_fini(struct drm_device *dev); > int intel_huc_load(struct drm_i915_private *dev_priv); > +int intel_is_huc_valid(struct drm_i915_private *dev_priv); > > #endif > #endif > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index da32c2f..3e1964c 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_HAS_HUC 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 09, 2016 at 01:59:45PM +0100, Michal Wajdeczko wrote: > On Thu, Dec 08, 2016 at 03:02:19PM -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 > > > > 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 | 12 ++++++++++++ > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > include/uapi/drm/i915_drm.h | 1 + > > 4 files changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 85a47c2..6be06a27 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; > > > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > */ > > value = 1; > > break; > > + case I915_PARAM_HAS_HUC: > > For me this param name does not match returned result which maybe misleading. > Note that other HAS params return static driver/hw capability, not runtime. > > I guess PARAM_HUC_STATUS would be better (0=no huc, 1=pending, 2=ok, -1=failed) > And you can cache huc status in intel_huc struct and make final modification in > intel_huc_auth() function to avoid registry read (unless we want to detect later > crash of the huc using this reg read) Why should userspace care for those intermediary states? From what I know (docs and patches from the cover letter) userspace is interested only in being able to use HuC. If something is not working you have DebugFS for that exact purpose. As of cacheing - seems like a good idea to limit reg reads. > > + value = intel_is_huc_valid(dev_priv); > > + break; > > default: > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > return -EINVAL; > > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c > > index 96fc727..6704cc8 100644 > > --- a/drivers/gpu/drm/i915/intel_huc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c > > @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) > > huc_fw->fetch_status = UC_FIRMWARE_NONE; > > } > > > > +/** > > + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. > > + * @dev_priv: drm device to check. > > + * > > + * This function will return true if the guc has been loaded and > > Please change function return type to bool to match this description. > > > + * has valid firmware. The simplest way of doing this is to check > > + * if the HuC has been validated, if so it must have been loaded. > > + */ > > +int intel_is_huc_valid(struct drm_i915_private *dev_priv) > > +{ > > + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); > > +} > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index 1db8bc2..ccd3f69 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -226,6 +226,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); > > void intel_huc_init(struct drm_i915_private *dev_priv); > > void intel_huc_fini(struct drm_device *dev); > > int intel_huc_load(struct drm_i915_private *dev_priv); > > +int intel_is_huc_valid(struct drm_i915_private *dev_priv); > > > > #endif > > #endif > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index da32c2f..3e1964c 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_HAS_HUC 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 12, 2016 at 03:13:17PM +0100, Arkadiusz Hiler wrote: > On Fri, Dec 09, 2016 at 01:59:45PM +0100, Michal Wajdeczko wrote: > > On Thu, Dec 08, 2016 at 03:02:19PM -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 > > > > > > 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 | 12 ++++++++++++ > > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > > include/uapi/drm/i915_drm.h | 1 + > > > 4 files changed, 18 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 85a47c2..6be06a27 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; > > > > > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > */ > > > value = 1; > > > break; > > > + case I915_PARAM_HAS_HUC: > > > > For me this param name does not match returned result which maybe misleading. > > Note that other HAS params return static driver/hw capability, not runtime. > > > > I guess PARAM_HUC_STATUS would be better (0=no huc, 1=pending, 2=ok, -1=failed) > > And you can cache huc status in intel_huc struct and make final modification in > > intel_huc_auth() function to avoid registry read (unless we want to detect later > > crash of the huc using this reg read) > > Why should userspace care for those intermediary states? From what I > know (docs and patches from the cover letter) userspace is interested > only in being able to use HuC. If something is not working you have > DebugFS for that exact purpose. > > As of cacheing - seems like a good idea to limit reg reads. Is GETPARAM(HAS_HUC) a hot path? Should we even encourage it? Are there any other users of intel_huc_is_valid()? As for userspace simply asking where huc is enabled, we already have that in the ABI via the module parameter, so you need to justify why this is preferred (in addition to the available information). -Chris
On Mon, Dec 12, 2016 at 02:21:41PM +0000, Chris Wilson wrote: > On Mon, Dec 12, 2016 at 03:13:17PM +0100, Arkadiusz Hiler wrote: > > On Fri, Dec 09, 2016 at 01:59:45PM +0100, Michal Wajdeczko wrote: > > > On Thu, Dec 08, 2016 at 03:02:19PM -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 > > > > > > > > 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 | 12 ++++++++++++ > > > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > > > include/uapi/drm/i915_drm.h | 1 + > > > > 4 files changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index 85a47c2..6be06a27 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; > > > > > > > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > > > */ > > > > value = 1; > > > > break; > > > > + case I915_PARAM_HAS_HUC: > > > > > > For me this param name does not match returned result which maybe misleading. > > > Note that other HAS params return static driver/hw capability, not runtime. > > > > > > I guess PARAM_HUC_STATUS would be better (0=no huc, 1=pending, 2=ok, -1=failed) > > > And you can cache huc status in intel_huc struct and make final modification in > > > intel_huc_auth() function to avoid registry read (unless we want to detect later > > > crash of the huc using this reg read) > > > > Why should userspace care for those intermediary states? From what I > > know (docs and patches from the cover letter) userspace is interested > > only in being able to use HuC. If something is not working you have > > DebugFS for that exact purpose. > > > > As of cacheing - seems like a good idea to limit reg reads. > > Is GETPARAM(HAS_HUC) a hot path? Should we even encourage it? Actually... Good point. HAS_HUC is used once each time you initialise libva client. > Are there any other users of intel_huc_is_valid()? intel_guc_auth_huc() uses ret = wait_for((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) > 0, 50); So it can be reused there. > As for userspace simply asking where huc is enabled, we already have > that in the ABI via the module parameter, so you need to justify why > this is preferred (in addition to the available information). Yeah, we do change the values of module parameters. But a lot of them are uid 0 only and we have PARAMS for those. Do anything userspace use those actually? Do we plan to use them instead of the getparams since now on? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Dec 12, 2016 at 03:52:05PM +0100, Arkadiusz Hiler wrote: > On Mon, Dec 12, 2016 at 02:21:41PM +0000, Chris Wilson wrote: > > As for userspace simply asking where huc is enabled, we already have > > that in the ABI via the module parameter, so you need to justify why > > this is preferred (in addition to the available information). > > Yeah, we do change the values of module parameters. But a lot of them > are uid 0 only and we have PARAMS for those. > > Do anything userspace use those actually? Do we plan to use them instead > of the getparams since now on? I've done both from userspace... I don't really have a preference, once you have an fd, an ioctl/GETPARAM is trivial. But for quick querying and reporting of driver state, reading the module parameter is easier. So I have a tendency to use an ioctl in production code and module parameter in igt. (And I would say that for one-off validation purposes, i.e. did hte module actually enable huc?, just check the module parameter. If userspace is expected to interact and respond to the setting during its early probe, make it an ioctl so it fits in with the similar code also being run at that time.) It is just worth explaining the intended usecase in the cover note, so that the use can be sanity checked and reflected upon later if need be. -Chris
On Mon, Dec 12, 2016 at 03:17:16PM +0000, Chris Wilson wrote: > On Mon, Dec 12, 2016 at 03:52:05PM +0100, Arkadiusz Hiler wrote: > > On Mon, Dec 12, 2016 at 02:21:41PM +0000, Chris Wilson wrote: > > > As for userspace simply asking where huc is enabled, we already have > > > that in the ABI via the module parameter, so you need to justify why > > > this is preferred (in addition to the available information). > > > > Yeah, we do change the values of module parameters. But a lot of them > > are uid 0 only and we have PARAMS for those. > > > > Do anything userspace use those actually? Do we plan to use them instead > > of the getparams since now on? > > I've done both from userspace... I don't really have a preference, once > you have an fd, an ioctl/GETPARAM is trivial. But for quick querying and > reporting of driver state, reading the module parameter is easier. So I > have a tendency to use an ioctl in production code and module parameter > in igt. (And I would say that for one-off validation purposes, i.e. did > hte module actually enable huc?, just check the module parameter. Thank you for explaining it :) > If userspace is expected to interact and respond to the setting > during its early probe, make it an ioctl so it fits in with the > similar code also being run at that time.) Okay. Seems like the scenario Anusha's got. > It is just worth explaining the intended usecase in the cover note, so > that the use can be sanity checked and reflected upon later if need be. > -Chris The cover letter links patches from the libva which is pretty much a template usecase, but I agree, it wold be nice if it used words/pseudocode instead of portions of code from some other project. > -- > Chris Wilson, Intel Open Source Technology Centre
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Chris Wilson >Sent: Monday, December 12, 2016 7:17 AM >To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Michal Wajdeczko ><michal.wajdeczko@linux.intel.com> >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to >getparams > >On Mon, Dec 12, 2016 at 03:52:05PM +0100, Arkadiusz Hiler wrote: >> On Mon, Dec 12, 2016 at 02:21:41PM +0000, Chris Wilson wrote: >> > As for userspace simply asking where huc is enabled, we already have >> > that in the ABI via the module parameter, so you need to justify why >> > this is preferred (in addition to the available information). >> >> Yeah, we do change the values of module parameters. But a lot of them >> are uid 0 only and we have PARAMS for those. >> >> Do anything userspace use those actually? Do we plan to use them >> instead of the getparams since now on? > >I've done both from userspace... I don't really have a preference, once you have >an fd, an ioctl/GETPARAM is trivial. But for quick querying and reporting of driver >state, reading the module parameter is easier. So I have a tendency to use an >ioctl in production code and module parameter in igt. (And I would say that for >one-off validation purposes, i.e. did hte module actually enable huc?, just check >the module parameter. If userspace is expected to interact and respond to the >setting during its early probe, make it an ioctl so it fits in with the similar code >also being run at that time.) > >It is just worth explaining the intended usecase in the cover note, so that the use >can be sanity checked and reflected upon later if need be. >-Chris > Thanks for explaining Chris.... Anusha >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 08, 2016 at 11:55:34PM +0000, Chris Wilson wrote: > On Thu, Dec 08, 2016 at 03:02:19PM -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 > > > > 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 | 12 ++++++++++++ > > drivers/gpu/drm/i915/intel_uc.h | 1 + > > include/uapi/drm/i915_drm.h | 1 + > > 4 files changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 85a47c2..6be06a27 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; > > > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, > > */ > > value = 1; > > break; > > + case I915_PARAM_HAS_HUC: > > + value = intel_is_huc_valid(dev_priv); > > + break; > > Why did you put it here? It breaks the pattern of case statements. > > > default: > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > return -EINVAL; > > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c > > index 96fc727..6704cc8 100644 > > --- a/drivers/gpu/drm/i915/intel_huc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c > > @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) > > huc_fw->fetch_status = UC_FIRMWARE_NONE; > > } > > > > +/** > > + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. > > + * @dev_priv: drm device to check. > > + * > > + * This function will return true if the guc has been loaded and > > + * has valid firmware. The simplest way of doing this is to check > > + * if the HuC has been validated, if so it must have been loaded. > > + */ > > +int intel_is_huc_valid(struct drm_i915_private *dev_priv) > > bool > > > +{ > > + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); > > (brackets (because (brackets))) > > But what I really wanted to ask... Does this register access require the > device to be awake and powered? Just confirmed that this register indeed requires MEDIA FW. > -Chris
>-----Original Message----- >From: Hiler, Arkadiusz >Sent: Tuesday, December 13, 2016 1:41 AM >To: Chris Wilson <chris@chris-wilson.co.uk>; Srivatsa, Anusha ><anusha.srivatsa@intel.com>; intel-gfx@lists.freedesktop.org; Peter Antoine ><peter.antoine@intel.com> >Subject: Re: [Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to >getparams > >On Thu, Dec 08, 2016 at 11:55:34PM +0000, Chris Wilson wrote: >> On Thu, Dec 08, 2016 at 03:02:19PM -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 >> > >> > 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 | 12 ++++++++++++ >> > drivers/gpu/drm/i915/intel_uc.h | 1 + >> > include/uapi/drm/i915_drm.h | 1 + >> > 4 files changed, 18 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c >> > b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..6be06a27 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; >> > >> > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void >*data, >> > */ >> > value = 1; >> > break; >> > + case I915_PARAM_HAS_HUC: >> > + value = intel_is_huc_valid(dev_priv); >> > + break; >> >> Why did you put it here? It breaks the pattern of case statements. >> >> > default: >> > DRM_DEBUG("Unknown parameter %d\n", param->param); >> > return -EINVAL; >> > diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c >> > b/drivers/gpu/drm/i915/intel_huc_loader.c >> > index 96fc727..6704cc8 100644 >> > --- a/drivers/gpu/drm/i915/intel_huc_loader.c >> > +++ b/drivers/gpu/drm/i915/intel_huc_loader.c >> > @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) >> > huc_fw->fetch_status = UC_FIRMWARE_NONE; } >> > >> > +/** >> > + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. >> > + * @dev_priv: drm device to check. >> > + * >> > + * This function will return true if the guc has been loaded and >> > + * has valid firmware. The simplest way of doing this is to check >> > + * if the HuC has been validated, if so it must have been loaded. >> > + */ >> > +int intel_is_huc_valid(struct drm_i915_private *dev_priv) >> >> bool >> >> > +{ >> > + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); >> >> (brackets (because (brackets))) >> >> But what I really wanted to ask... Does this register access require >> the device to be awake and powered? > >Just confirmed that this register indeed requires MEDIA FW. Thanks Arek. >> -Chris > >-- >Cheers, >Arek
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 85a47c2..6be06a27 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; @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data, */ value = 1; break; + case I915_PARAM_HAS_HUC: + value = intel_is_huc_valid(dev_priv); + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c index 96fc727..6704cc8 100644 --- a/drivers/gpu/drm/i915/intel_huc_loader.c +++ b/drivers/gpu/drm/i915/intel_huc_loader.c @@ -289,3 +289,15 @@ void intel_huc_fini(struct drm_device *dev) huc_fw->fetch_status = UC_FIRMWARE_NONE; } +/** + * intel_is_huc_valid() - Check to see if the HuC is fully loaded. + * @dev_priv: drm device to check. + * + * This function will return true if the guc has been loaded and + * has valid firmware. The simplest way of doing this is to check + * if the HuC has been validated, if so it must have been loaded. + */ +int intel_is_huc_valid(struct drm_i915_private *dev_priv) +{ + return ((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) != 0); +} diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 1db8bc2..ccd3f69 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -226,6 +226,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val); void intel_huc_init(struct drm_i915_private *dev_priv); void intel_huc_fini(struct drm_device *dev); int intel_huc_load(struct drm_i915_private *dev_priv); +int intel_is_huc_valid(struct drm_i915_private *dev_priv); #endif #endif diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index da32c2f..3e1964c 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_HAS_HUC 42 typedef struct drm_i915_getparam { __s32 param;