On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> Trying to have subject_verb_object ordering and more descriptive names,
> the intel_huc_init() and intel_guc_init() functions are renamed:
>
> * `intel_guc` is the subject, so those functions now take intel_guc
> structure, instead of the dev_priv
> * fetch is the verb
> * fw is the object which better describes the function's role
>
> Same change is done for the huc counterpart.
>
> Also we bulk call both functions from higher-level intel_uc_fetch_fw:
> * intel being the subject (taking the dev_priv as param)
> * fetch being the verb
> * uc_fw being the subject
>
> v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
<SNIP>
> @@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> if (ret)
> goto cleanup_irq;
>
> - intel_huc_init(dev_priv);
> - intel_guc_init(dev_priv);
> + intel_uc_fetch_fw(dev_priv);
intel_uc_init fits this context. (See below)
> /**
> - * intel_guc_init() - define parameters and fetch firmware
> - * @dev_priv: i915 device private
> + * intel_guc_fetch_fw() - define parameters and fetch firmware
> + * @guc: intel_guc struct
> *
> * Called early during driver load, but after GEM is initialised.
> *
> * The firmware will be transferred to the GuC's memory later,
> * when intel_guc_init_hw() is called.
> */
"define parameters" is little vague, maybe "select and fetch firmware"?
> -void intel_guc_init(struct drm_i915_private *dev_priv)
> +void intel_guc_fetch_fw(struct intel_guc *guc)
> {
> - struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> const char *fw_path;
>
> if (!HAS_GUC(dev_priv)) {
This parameter dance needs to be moved away from guc_fetch_fw function,
into intel_sanitize_options (I'm pretty sure I've mentioned this
earlier).
> @@ -751,23 +751,23 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> fw_path = NULL;
> } else if (IS_SKYLAKE(dev_priv)) {
> fw_path = I915_SKL_GUC_UCODE;
> - guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> - guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> + guc->fw.major_ver_wanted = SKL_FW_MAJOR;
> + guc->fw.minor_ver_wanted = SKL_FW_MINOR;
> } else if (IS_BROXTON(dev_priv)) {
> fw_path = I915_BXT_GUC_UCODE;
> - guc_fw->major_ver_wanted = BXT_FW_MAJOR;
> - guc_fw->minor_ver_wanted = BXT_FW_MINOR;
> + guc->fw.major_ver_wanted = BXT_FW_MAJOR;
> + guc->fw.minor_ver_wanted = BXT_FW_MINOR;
> } else if (IS_KABYLAKE(dev_priv)) {
> fw_path = I915_KBL_GUC_UCODE;
> - guc_fw->major_ver_wanted = KBL_FW_MAJOR;
> - guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> + guc->fw.major_ver_wanted = KBL_FW_MAJOR;
> + guc->fw.minor_ver_wanted = KBL_FW_MINOR;
> } else {
> fw_path = ""; /* unknown device */
> }
>
> - guc_fw->path = fw_path;
> - guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> - guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> + guc->fw.path = fw_path;
Just get rid of fw_path variable and assign directly, also hoist the
warning to the else branch, which can then do "return;"
> + guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> + guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
Hoist this assignment before the if block, so no need to special for
the early return from else branch.
<SNIP>
> /**
> - * intel_huc_init() - initiate HuC firmware loading request
> - * @dev_priv: the drm_i915_private device
> + * intel_huc_fetch_fw() - initiate HuC firmware loading request
Correct this commit too to be more descriptive.
> + * @huc: intel_huc struct
> *
> * Called early during driver load, but after GEM is initialised. The loading
> * will continue only when driver explicitly specify firmware name and version.
> @@ -152,42 +152,41 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> *
> * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
> */
> -void intel_huc_init(struct drm_i915_private *dev_priv)
> +void intel_huc_fetch_fw(struct intel_huc *huc)
> {
> - struct intel_huc *huc = &dev_priv->huc;
> - struct intel_uc_fw *huc_fw = &huc->fw;
> + struct drm_i915_private *dev_priv = huc_to_i915(huc);
> const char *fw_path = NULL;
Similarly arrange to get rid of fw_path here.
<SNIP>
> @@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> mutex_init(&dev_priv->guc.send_mutex);
> }
>
> +void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
This function might be worth calling intel_uc_init (See above), if the
need comes to add other stuff. But either way.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
