From: Sujaritha <sujaritha.sundaresan@intel.com>
To: Sagar Arun Kamble <sagar.a.kamble@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module
Date: Wed, 4 Oct 2017 17:26:21 -0700 [thread overview]
Message-ID: <896a8aba-df4c-9357-4c4b-76e7546aa988@intel.com> (raw)
In-Reply-To: <16be1dd0-7030-bb8f-bd71-529c9d72788c@intel.com>
On 10/03/2017 11:45 PM, Sagar Arun Kamble wrote:
>
> Subject is missing "parameter" in the end. Either keep module
> parameter or i915_modparams.
Will fix the subject line.
>
>
> On 10/4/2017 4:26 AM, Sujaritha Sundaresan wrote:
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission".
>> Whenever we need i915_modparams.enable_guc_submission=1, we also need
>> enable_guc_loading=1.
>> We also need enable_guc_loading=1 when we want to verify the HuC,
>> which is every time we have a HuC (but all platforms with HuC have a
>> GuC and viceversa).
>>
>> v2: Clarifying the commit message (Anusha)
>>
>> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating message unification into a separate patch
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Need to change the order of the tag to comply with new convention.
> Applies to all patches.
> should be as below as per chronology
> S-o-b:
> Cc:
> R-b:
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 11 +++++--
>> drivers/gpu/drm/i915/i915_drv.h | 9 ++++--
>> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>> drivers/gpu/drm/i915/i915_irq.c | 2 +-
>> drivers/gpu/drm/i915/i915_params.c | 5 ----
>> drivers/gpu/drm/i915/i915_params.h | 1 -
>> drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++--
>> drivers/gpu/drm/i915/intel_huc.c | 4 ++-
>> drivers/gpu/drm/i915/intel_uc.c | 51
>> +++++++++++++++++----------------
>> drivers/gpu/drm/i915/intel_uc.h | 2 +-
>> drivers/gpu/drm/i915/intel_uncore.c | 4 +--
>> 12 files changed, 52 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 53e40dd..4fde4b2 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2336,8 +2336,10 @@ static int i915_huc_load_status_info(struct
>> seq_file *m, void *data)
>> struct drm_i915_private *dev_priv = node_to_i915(m->private);
>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>> - if (!HAS_HUC_UCODE(dev_priv))
>> + if (!HAS_GUC(dev_priv)){
>> + seq_puts(m, "not supported\n");
>> return 0;
>> + }
>> seq_puts(m, "HuC firmware status:\n");
>> seq_printf(m, "\tpath: %s\n", huc_fw->path);
>> @@ -2369,8 +2371,11 @@ static int i915_guc_load_status_info(struct
>> seq_file *m, void *data)
>> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> u32 tmp, i;
>> - if (!HAS_GUC_UCODE(dev_priv))
>> + if (!HAS_GUC(dev_priv)){
>> + seq_puts(m, "not supported\n");
>> return 0;
>> +
>> + }
>> seq_printf(m, "GuC firmware status:\n");
>> seq_printf(m, "\tpath: %s\n",
>> @@ -2465,7 +2470,7 @@ static bool check_guc_submission(struct
>> seq_file *m)
>> if (!guc->execbuf_client) {
>> seq_printf(m, "GuC submission %s\n",
>> - HAS_GUC_SCHED(dev_priv) ?
>> + HAS_GUC(dev_priv) ?
>> "disabled" :
>> "not supported");
>> return false;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 61a4be9..6479b72 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3141,9 +3141,12 @@ static inline unsigned int
>> i915_sg_segment_size(void)
>> */
>> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc)
>> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
>> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
>> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv))
>> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
>> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
>> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL)
> Is this typo? Should be (dev_priv)->huc.fw.path
Yes this is a typo, I will fix this in the revision.
>> +
>> +#define NEEDS_GUC_LOADING(dev_priv) \
>> + (HAS_GUC(dev_priv) && \
>> + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv)))
>> #define HAS_RESOURCE_STREAMER(dev_priv)
>> ((dev_priv)->info.has_resource_streamer)
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 921ee36..0890341 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct
>> drm_i915_private *i915,
>> * present or not in use we still need a small bias as ring
>> wraparound
>> * at offset 0 sometimes hangs. No idea why.
>> */
>> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
>> + if (NEEDS_GUC_LOADING(dev_priv))
>> ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
>> else
>> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 64d7852..a32935a 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3292,7 +3292,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private
>> *dev_priv)
>> * currently don't have any bits spare to pass in this upper
>> * restriction!
>> */
>> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
>> + if (NEEDS_GUC_LOADING(dev_priv)) {
>> ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>> ggtt->mappable_end = min(ggtt->mappable_end,
>> ggtt->base.total);
>> }
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 6a07ef3..60cdf0d 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3956,7 +3956,7 @@ void intel_irq_init(struct drm_i915_private
>> *dev_priv)
>> for (i = 0; i < MAX_L3_SLICES; ++i)
>> dev_priv->l3_parity.remap_info[i] = NULL;
>> - if (HAS_GUC_SCHED(dev_priv))
>> + if (NEEDS_GUC_LOADING(dev_priv))
>> dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
>> /* Let's track the enabled rps events */
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index ec65341..30344e1 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -63,7 +63,6 @@ struct i915_params i915_modparams __read_mostly = {
>> .verbose_state_checks = 1,
>> .nuclear_pageflip = 0,
>> .edp_vswing = 0,
>> - .enable_guc_loading = 0,
>> .enable_guc_submission = 0,
>> .guc_log_level = -1,
>> .guc_firmware_path = NULL,
>> @@ -201,10 +200,6 @@ struct i915_params i915_modparams __read_mostly = {
>> "(0=use value from vbt [default], 1=low power swing(200mV),"
>> "2=default swing(400mV))");
>> -i915_param_named_unsafe(enable_guc_loading, int, 0400,
>> - "Enable GuC firmware loading "
>> - "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> -
>> i915_param_named_unsafe(enable_guc_submission, int, 0400,
>> "Enable GuC submission "
>> "(-1=auto, 0=never [default], 1=if available, 2=required)");
>> diff --git a/drivers/gpu/drm/i915/i915_params.h
>> b/drivers/gpu/drm/i915/i915_params.h
>> index a2cbb47..b26df06 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,7 +44,6 @@
>> func(int, disable_power_well); \
>> func(int, enable_ips); \
>> func(int, invert_brightness); \
>> - func(int, enable_guc_loading); \
>> func(int, enable_guc_submission); \
>> func(int, guc_log_level); \
>> func(char *, guc_firmware_path); \
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index c9e25be..4210680 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -382,7 +382,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
>> *
>> * Return: zero when we know firmware, non-zero in other case
> Need to update the comment.
Will do.
>> */
>> -int intel_guc_select_fw(struct intel_guc *guc)
>> +void intel_guc_select_fw(struct intel_guc *guc)
>> {
>> struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> @@ -412,8 +412,9 @@ int intel_guc_select_fw(struct intel_guc *guc)
>> guc->fw.major_ver_wanted = GLK_FW_MAJOR;
>> guc->fw.minor_ver_wanted = GLK_FW_MINOR;
>> } else {
>> - DRM_ERROR("No GuC firmware known for platform with GuC!\n");
>> - return -ENOENT;
>> + if(HAS_GUC(dev_priv))
>> + DRM_ERROR("NO GUC FW konown for a platform with GuC!\n");
>> + return;
>> }
>> return 0;
> returning value is not correct
Will fix the return value in the revised patch.
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 6e1779b..ee9e786 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -176,7 +176,9 @@ void intel_huc_select_fw(struct intel_huc *huc)
>> huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
>> huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
>> } else {
>> - DRM_ERROR("No HuC firmware known for platform with HuC!\n");
>> + /* For now, everything with a GuC also has a HuC */
>> + if (HAS_GUC(dev_priv))
>> + DRM_ERROR("No HuC FW known for a platform with HuC!\n");
>> return;
>> }
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9018540..4290b35 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -63,35 +63,31 @@ static int __intel_uc_reset_hw(struct
>> drm_i915_private *dev_priv)
>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> {
>> if (!HAS_GUC(dev_priv)) {
>> - if (i915_modparams.enable_guc_loading > 0 ||
>> - i915_modparams.enable_guc_submission > 0)
>> + if (i915_modparams.enable_guc_submission > 0)
>> DRM_INFO("Ignoring GuC options, no hardware\n");
>> - i915_modparams.enable_guc_loading = 0;
>> i915_modparams.enable_guc_submission = 0;
>> return;
>> }
>> - /* A negative value means "use platform default" */
>> - if (i915_modparams.enable_guc_loading < 0)
>> - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
>> /* Verify firmware version */
>> - if (i915_modparams.enable_guc_loading) {
>> - if (HAS_HUC_UCODE(dev_priv))
>> - intel_huc_select_fw(&dev_priv->huc);
>> -
>> - if (intel_guc_select_fw(&dev_priv->guc))
>> - i915_modparams.enable_guc_loading = 0;
>> + if (!HAS_GUC(dev_priv)){
>> + if (i915_modparams.enable_guc_submission > 0){
>> + DRM_INFO("Ignoring GuC submission enable enable, no FW\n");
>> + i915_modparams.enable_guc_submission = 0;
>> + return;
>> + }
>> }
> This is same !HAS_GUC check as the one at the beginning of the function.
Will fix this error.
>> /* Can't enable guc submission without guc loaded */
> Remove this comment.
Will do
>> - if (!i915_modparams.enable_guc_loading)
>> + if (!i915_modparams.enable_guc_submission < 0)
> This is incorrect. Add ( braces to specify correct condition. Also
> missing { } till return.
>> i915_modparams.enable_guc_submission = 0;
>> + return;
>> /* A negative value means "use platform default" */
>> if (i915_modparams.enable_guc_submission < 0)
>> - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
>> + i915_modparams.enable_guc_submission = 1;
> This is wrong. This should be HAS_GUC.
Will fix this.
>> }
>> static void gen8_guc_raise_irq(struct intel_guc *guc)
>> @@ -106,6 +102,8 @@ void intel_uc_init_early(struct drm_i915_private
>> *dev_priv)
>> struct intel_guc *guc = &dev_priv->guc;
>> intel_guc_ct_init_early(&guc->ct);
>> + intel_guc_select_fw(&dev_priv->guc);
>> + intel_huc_select_fw(&dev_priv->huc);
> Need to rebase these w.r.t Michal's latest GuC code reorg series -
> https://patchwork.freedesktop.org/series/31340/
Will rebase the revised patches.
>> mutex_init(&guc->send_mutex);
>> guc->send = intel_guc_send_nop;
>> @@ -258,6 +256,8 @@ void intel_uc_init_fw(struct drm_i915_private
>> *dev_priv)
>> void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>> {
>> + if (!HAS_GUC(dev_priv))
>> + return;
>> __intel_uc_fw_fini(&dev_priv->guc.fw);
>> __intel_uc_fw_fini(&dev_priv->huc.fw);
>> }
>> @@ -333,7 +333,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> struct intel_guc *guc = &dev_priv->guc;
>> int ret, attempts;
>> - if (!i915_modparams.enable_guc_loading)
>> + if (!NEEDS_GUC_LOADING(dev_priv))
>> return 0;
>> guc_disable_communication(guc);
>> @@ -423,19 +423,20 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> i915_ggtt_disable_guc(dev_priv);
>> DRM_ERROR("GuC init failed\n");
>> - if (i915_modparams.enable_guc_loading > 1 ||
>> - i915_modparams.enable_guc_submission > 1)
>> + if (i915_modparams.enable_guc_submission > 1){
>> + DRM_NOTE("GuC is required, so marking the GPU as wedged\n");
>> ret = -EIO;
>> - else
>> - ret = 0;
>> +
>> + }
>> - if (i915_modparams.enable_guc_submission) {
>> - i915_modparams.enable_guc_submission = 0;
>> - DRM_NOTE("Falling back from GuC submission to execlist
>> mode\n");
>> + else if (i915_modparams.enable_guc_submission == 1){
>> + DRM_NOTE("Falling back from GuC submission to execlist
>> mode\n");
>> + i915_modparams.enable_guc_submission = 0;
>> + ret = 0;
>> }
>> - i915_modparams.enable_guc_loading = 0;
>> - DRM_NOTE("GuC firmware loading disabled\n");
>> + else
>> + ret = 0;
>> return ret;
>> }
>> @@ -444,7 +445,7 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> {
>> guc_free_load_err_log(&dev_priv->guc);
>> - if (!i915_modparams.enable_guc_loading)
>> + if (!NEEDS_GUC_LOADING(dev_priv))
>> return;
>> if (i915_modparams.enable_guc_submission)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 7703c9a..58d787e 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -223,7 +223,7 @@ static inline void intel_guc_notify(struct
>> intel_guc *guc)
>> }
>> /* intel_guc_loader.c */
>> -int intel_guc_select_fw(struct intel_guc *guc);
>> +void intel_guc_select_fw(struct intel_guc *guc);
>> int intel_guc_init_hw(struct intel_guc *guc);
>> int intel_guc_suspend(struct drm_i915_private *dev_priv);
>> int intel_guc_resume(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index b3c3f94..0164f41 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1786,12 +1786,10 @@ int intel_guc_reset(struct drm_i915_private
>> *dev_priv)
>> {
>> int ret;
>> - if (!HAS_GUC(dev_priv))
>> - return -EINVAL;
>> + GEM_BUG_ON(!HAS_GUC(dev_priv));
>> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
>> - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> return ret;
>> }
>
Thanks for the review.
Regards,
Sujaritha
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-10-05 0:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-03 22:56 [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module Sujaritha Sundaresan
2017-10-04 6:45 ` Sagar Arun Kamble
2017-10-05 0:26 ` Sujaritha [this message]
2017-10-04 12:00 ` Michal Wajdeczko
2017-10-04 13:07 ` Joonas Lahtinen
2017-11-02 23:52 ` Rodrigo Vivi
2017-11-03 0:03 ` Chris Wilson
2017-11-03 8:36 ` Joonas Lahtinen
2017-11-03 17:08 ` Rodrigo Vivi
2017-11-06 10:24 ` Patch tag ordering (Was: Re: [PATCH v5 2/5] drm/i915/guc : Removing i915_modparams.enable_guc_loading module) Joonas Lahtinen
2017-11-06 11:41 ` Jani Nikula
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=896a8aba-df4c-9357-4c4b-76e7546aa988@intel.com \
--to=sujaritha.sundaresan@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=sagar.a.kamble@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.