Re: [PATCH 1/8] drm/i915/guc: Make the GuC fw loading helper functions general

From: Michal Wajdeczko <michal.wajdeczko@intel.com>
To: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/8] drm/i915/guc: Make the GuC fw loading helper functions general
Date: Tue, 3 Jan 2017 15:15:17 +0100	[thread overview]
Message-ID: <20170103141517.GA89880@mwajdecz-MOBL1.ger.corp.intel.com> (raw)
In-Reply-To: <83F5C7385F545743AD4FB2A62F75B0730195EA9B@ORSMSX108.amr.corp.intel.com>

On Tue, Jan 03, 2017 at 01:07:14AM +0100, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Wajdeczko, Michal
> >Sent: Tuesday, December 27, 2016 9:28 AM
> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Alex Dai <yu.dai@intel.com>; Peter Antoine
> ><peter.antoine@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915/guc: Make the GuC fw loading
> >helper functions general
> >
> >On Thu, Dec 22, 2016 at 03:12:17PM -0800, Anusha Srivatsa wrote:
> >> From: Peter Antoine <peter.antoine@intel.com>
> >>
> >> Rename some of the GuC fw loading code to make them more general. We
> >> will utilise them for HuC loading as well.
> >>      s/intel_guc_fw/intel_uc_fw/g
> >>      s/GUC_FIRMWARE/UC_FIRMWARE/g
> >>
> >> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> >> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> >> same purpose.
> >>
> >> v2: rebased on top of nightly.
> >>     reapplied the search/replace as upstream code as changed.
> >> v3: rebased again on drm-nightly.
> >> v4: removed G from messages in shared fw fetch function.
> >> v5: rebased.
> >> v7: rebased.
> >> v8: rebased.
> >> v9: rebased.
> >> v10: rebased.
> >> v11: rebased.
> >> v12: rebased on top of drm-tip
> >> v13: rebased.Updated dev to dev_priv in intel_guc_setup(),
> >> guc_fw_getch() and intel_guc_init().
> >> v14: rebased. Remove uint32_t fw_type to patch 2. Add INTEL_ prefix
> >> for fields in enum intel_uc_fw_status. Remove uc_dev field since its
> >> never used.Rename uc_fw to just fw and guc_fw to fw to avoid redundency.
> >> v15: rebased. Remove sections of code that were commented and no
> >> longer required.
> >>
> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Signed-off-by: Alex Dai <yu.dai@intel.com>
> >> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c        |  12 +--
> >>  drivers/gpu/drm/i915/i915_guc_submission.c |   4 +-
> >>  drivers/gpu/drm/i915/intel_guc_loader.c    | 156 ++++++++++++++---------------
> >>  drivers/gpu/drm/i915/intel_uc.h            |  36 +++----
> >>  4 files changed, 104 insertions(+), 104 deletions(-)
> >>

... CUT ...

> >> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> >> b/drivers/gpu/drm/i915/intel_uc.h index 11f5608..893bcec 100644
> >> --- a/drivers/gpu/drm/i915/intel_uc.h
> >> +++ b/drivers/gpu/drm/i915/intel_uc.h
> >> @@ -91,28 +91,28 @@ struct i915_guc_client {
> >>  	uint64_t submissions[I915_NUM_ENGINES];  };
> >>
> >> -enum intel_guc_fw_status {
> >> -	GUC_FIRMWARE_FAIL = -1,
> >> -	GUC_FIRMWARE_NONE = 0,
> >> -	GUC_FIRMWARE_PENDING,
> >> -	GUC_FIRMWARE_SUCCESS
> >> +enum intel_uc_fw_status {
> >> +	INTEL_UC_FIRMWARE_FAIL = -1,
> >> +	INTEL_UC_FIRMWARE_NONE = 0,
> >> +	INTEL_UC_FIRMWARE_PENDING,
> >> +	INTEL_UC_FIRMWARE_SUCCESS
> >>  };
> >>
> >>  /*
> >>   * This structure encapsulates all the data needed during the process
> >>   * of fetching, caching, and loading the firmware image into the GuC.
> >>   */
> >> -struct intel_guc_fw {
> >> -	const char *			guc_fw_path;
> >> -	size_t				guc_fw_size;
> >> -	struct drm_i915_gem_object *	guc_fw_obj;
> >> -	enum intel_guc_fw_status	guc_fw_fetch_status;
> >> -	enum intel_guc_fw_status	guc_fw_load_status;
> >> -
> >> -	uint16_t			guc_fw_major_wanted;
> >> -	uint16_t			guc_fw_minor_wanted;
> >> -	uint16_t			guc_fw_major_found;
> >> -	uint16_t			guc_fw_minor_found;
> >> +struct intel_uc_fw {
> >> +	const char *uc_fw_path;
> >
> >Can we drop "uc_fw_" prefix also from path and obj members?
> 
> Michal, you think we can do this change as a part of the guc refactor effort which will happen post this series gets merged? Or do you feel its makes more sense to that we have this change in this series....
> 

IMHO we should avoid introducing changes that we already know they are bad and easy to fix.
Number of changes in names shall be minimized to avoid confusion and merge conflicts.
Also relaying on the future refactor effort (that has no known ETA) is not good option, as one can forget to fix it ;)

Thanks,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx