On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote:
> Let intel_guc_init() focus on determining and fetching the correct
> firmware.
>
> This patch introduces intel_sanitize_uc_params() that is called from
> intel_uc_init().
>
> Then, if we have GuC, we can call intel_guc_init() conditionally and we
> do not have to do internal checks.
>
> Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> drivers/gpu/drm/i915/intel_guc_loader.c | 15 +--------------
> drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++++++++++++++-
> 3 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6d0798e..e520895 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>
> i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores);
> DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores));
> +
> + intel_uc_sanitize_params(dev_priv);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f5efe28..db5713c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> }
>
> /**
> - * intel_guc_init() - define parameters and fetch firmware
> + * intel_guc_init() - determine and fetch firmware
Again, can we have function name that corresponds to its purpose.
Comment alone is not sufficient
> * @dev_priv: i915 device private
> *
> * Called early during driver load, but after GEM is initialised.
> @@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> const char *fw_path;
>
> - if (!HAS_GUC(dev_priv)) {
> - i915.enable_guc_loading = 0;
> - i915.enable_guc_submission = 0;
> - } else {
> - /* A negative value means "use platform default" */
> - if (i915.enable_guc_loading < 0)
> - i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> - if (i915.enable_guc_submission < 0)
> - i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> - }
> -
> if (!HAS_GUC_UCODE(dev_priv)) {
> fw_path = NULL;
> } else if (IS_SKYLAKE(dev_priv)) {
> @@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>
> /* Early (and silent) return if GuC loading is disabled */
I think this comment was related to the line that you just deleted
> - if (!i915.enable_guc_loading)
> - return;
> if (fw_path == NULL)
> return;
> if (*fw_path == '\0')
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d9d0566..d1ca41d 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,24 @@
> #include "i915_drv.h"
> #include "intel_uc.h"
>
> +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv)
> +{
> + if (!HAS_GUC(dev_priv)) {
Maybe we should warn user if specified explicit guc params need to be nuked?
> + i915.enable_guc_loading = 0;
> + i915.enable_guc_submission = 0;
> + } else {
> + /* A negative value means "use platform default" */
> + if (i915.enable_guc_loading < 0)
> + i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> + if (i915.enable_guc_submission < 0)
> + i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> + }
> +
> + /* can't enable guc submission without guc loaded */
> + if (!i915.enable_guc_loading)
> + i915.enable_guc_submission = 0;
> +}
> +
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> {
> mutex_init(&dev_priv->guc.send_mutex);
> @@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>
> void intel_uc_init(struct drm_i915_private *dev_priv)
> {
> - intel_huc_init(dev_priv);
> + if (!i915.enable_guc_loading)
> + return;
> +
> + if (HAS_HUC_UCODE(dev_priv))
Hmm, if I recall correctly, same condition is checked in huc_init()...
Btw, what approach is more preferred? check before call or inside?
Regards,
Michal
> + intel_huc_init(dev_priv);
> +
> intel_guc_init(dev_priv);
> }
>
> --
> 2.9.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
