[1/1] drm/i915/guc: s/intel_guc_fw_upload/intel_guc_init_hw/
diff mbox

Message ID 1519892298-32132-1-git-send-email-sagar.a.kamble@intel.com
State New
Headers show

Commit Message

sagar.a.kamble@intel.com March 1, 2018, 8:18 a.m. UTC
GuC and HuC get loaded from intel_uc_init_hw. HuC load function is
named intel_huc_init_hw, however GuC load function is still named in
old style as intel_guc_fw_upload. Update it and the function doc. for
both functions.
Move of GuC load function's def. & decl. to intel_guc.c|h seems necessary
as it is more about core GuC functionality and not so much about fw itself.
This can be done in later patch if needed.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc_fw.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_guc_fw.h |  2 +-
 drivers/gpu/drm/i915/intel_huc.c    |  8 ++++----
 drivers/gpu/drm/i915/intel_uc.c     |  2 +-
 4 files changed, 12 insertions(+), 12 deletions(-)

Comments

Michal Wajdeczko March 1, 2018, 10:06 a.m. UTC | #1
On Thu, 01 Mar 2018 09:18:18 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> GuC and HuC get loaded from intel_uc_init_hw. HuC load function is
> named intel_huc_init_hw, however GuC load function is still named in
> old style as intel_guc_fw_upload. Update it and the function doc. for
> both functions.
> Move of GuC load function's def. & decl. to intel_guc.c|h seems necessary
> as it is more about core GuC functionality and not so much about fw  
> itself.
> This can be done in later patch if needed.
>

Function intel_guc_fw_upload() was named this way on purpose to follow
object-verb naming pattern, where our object is GuC FW (hence file name
intel_guc_fw.*)

There was a plan to unify this approach with HuC but in the opposite way:
by moving HuC firmware selection code to intel_huc_fw.* but since only
one function will be left in intel_huc.c this action was deferred.

Note that there will be nothing wrong to call fw_upload functions from
our uc_init_hw function:

intel_uc_init_hw()
   intel_uc_reset()
   intel_huc_fw_upload()
   intel_guc_fw_upload()
   intel_guc_enable_comm()
   intel_huc_auth()


/Michal
sagar.a.kamble@intel.com March 1, 2018, 10:28 a.m. UTC | #2
On 3/1/2018 3:36 PM, Michal Wajdeczko wrote:
> On Thu, 01 Mar 2018 09:18:18 +0100, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> GuC and HuC get loaded from intel_uc_init_hw. HuC load function is
>> named intel_huc_init_hw, however GuC load function is still named in
>> old style as intel_guc_fw_upload. Update it and the function doc. for
>> both functions.
>> Move of GuC load function's def. & decl. to intel_guc.c|h seems 
>> necessary
>> as it is more about core GuC functionality and not so much about fw 
>> itself.
>> This can be done in later patch if needed.
>>
>
> Function intel_guc_fw_upload() was named this way on purpose to follow
> object-verb naming pattern, where our object is GuC FW (hence file name
> intel_guc_fw.*)
>
> There was a plan to unify this approach with HuC but in the opposite way:
> by moving HuC firmware selection code to intel_huc_fw.* but since only
> one function will be left in intel_huc.c this action was deferred.
>
Thanks for background on this.
> Note that there will be nothing wrong to call fw_upload functions from
> our uc_init_hw function:
>
> intel_uc_init_hw()
>   intel_uc_reset()
>   intel_huc_fw_upload()
Will just do HuC name change (s/intel_huc_init_hw/intel_huc_fw_upload/) 
and comments update. HuC related move can be done later.
Is that ok?
> intel_guc_fw_upload()
>   intel_guc_enable_comm()
>   intel_huc_auth()
>
>
> /Michal
Michal Wajdeczko March 1, 2018, 10:58 a.m. UTC | #3
On Thu, 01 Mar 2018 11:28:03 +0100, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 3/1/2018 3:36 PM, Michal Wajdeczko wrote:
>> On Thu, 01 Mar 2018 09:18:18 +0100, Sagar Arun Kamble  
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> GuC and HuC get loaded from intel_uc_init_hw. HuC load function is
>>> named intel_huc_init_hw, however GuC load function is still named in
>>> old style as intel_guc_fw_upload. Update it and the function doc. for
>>> both functions.
>>> Move of GuC load function's def. & decl. to intel_guc.c|h seems  
>>> necessary
>>> as it is more about core GuC functionality and not so much about fw  
>>> itself.
>>> This can be done in later patch if needed.
>>>
>>
>> Function intel_guc_fw_upload() was named this way on purpose to follow
>> object-verb naming pattern, where our object is GuC FW (hence file name
>> intel_guc_fw.*)
>>
>> There was a plan to unify this approach with HuC but in the opposite  
>> way:
>> by moving HuC firmware selection code to intel_huc_fw.* but since only
>> one function will be left in intel_huc.c this action was deferred.
>>
> Thanks for background on this.
>> Note that there will be nothing wrong to call fw_upload functions from
>> our uc_init_hw function:
>>
>> intel_uc_init_hw()
>>   intel_uc_reset()
>>   intel_huc_fw_upload()
> Will just do HuC name change (s/intel_huc_init_hw/intel_huc_fw_upload/)  
> and comments update. HuC related move can be done later.
> Is that ok?

Hmm, I've mixed feelings, as on one hand, this small step will unify
fw_upload calls, but at the same time it will break object-verb pattern
in intel_huc.* files ... so maybe we should do it only right?

>> intel_guc_fw_upload()
>>   intel_guc_enable_comm()
>>   intel_huc_auth()
>>
>>
>> /Michal

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c b/drivers/gpu/drm/i915/intel_guc_fw.c
index 3b09329..916dc64 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -269,19 +269,19 @@  static int guc_fw_xfer(struct intel_uc_fw *guc_fw, struct i915_vma *vma)
 }
 
 /**
- * intel_guc_fw_upload() - finish preparing the GuC for activity
+ * intel_guc_init_hw() - load GuC uCode to device
  * @guc: intel_guc structure
  *
- * Called during driver loading and also after a GPU reset.
+ * Called from intel_uc_init_hw() during driver load, resume from sleep and
+ * after a GPU reset.
  *
- * The main action required here it to load the GuC uCode into the device.
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_guc_init(), so here we need only check that
- * worked, and then transfer the image to the h/w.
+ * earlier call to intel_uc_init_fw(), so here we need to only check that
+ * fetch succeeded, and then transfer the image to the h/w.
  *
  * Return:	non-zero code on error
  */
-int intel_guc_fw_upload(struct intel_guc *guc)
+int intel_guc_init_hw(struct intel_guc *guc)
 {
 	return intel_uc_fw_upload(&guc->fw, guc_fw_xfer);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.h b/drivers/gpu/drm/i915/intel_guc_fw.h
index 4ec5d3d..6042fd0 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.h
+++ b/drivers/gpu/drm/i915/intel_guc_fw.h
@@ -28,6 +28,6 @@ 
 struct intel_guc;
 
 void intel_guc_fw_init_early(struct intel_guc *guc);
-int intel_guc_fw_upload(struct intel_guc *guc);
+int intel_guc_init_hw(struct intel_guc *guc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index ef9a05d..5e58c8c 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -171,12 +171,12 @@  static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma)
  * intel_huc_init_hw() - load HuC uCode to device
  * @huc: intel_huc structure
  *
- * Called from intel_uc_init_hw() during driver loading and also after a GPU
- * reset. Be note that HuC loading must be done before GuC loading.
+ * Called from intel_uc_init_hw() during driver load, resume from sleep and
+ * after a GPU reset. Note that HuC must be loaded before GuC.
  *
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_uc_init_fw(), so here we need only check that
- * is succeeded, and then transfer the image to the h/w.
+ * earlier call to intel_uc_init_fw(), so here we need to only check that
+ * fetch succeeded, and then transfer the image to the h/w.
  *
  */
 int intel_huc_init_hw(struct intel_huc *huc)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9f1bac6..69c8643 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -367,7 +367,7 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		}
 
 		intel_guc_init_params(guc);
-		ret = intel_guc_fw_upload(guc);
+		ret = intel_guc_init_hw(guc);
 		if (ret == 0 || ret != -EAGAIN)
 			break;