| Message ID | 20161215154708.31521-2-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote: > GuC historically has two "startup" functions called _init() and _setup() > > Then HuC came with it's _init() and _load(). > > To make naming more consistent this commit renames intel_guc_setup() to > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c > after all). Or init_hw as that is the initialisation phase it is called from. -Chris
On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote: > GuC historically has two "startup" functions called _init() and _setup() > > Then HuC came with it's _init() and _load(). > > To make naming more consistent this commit renames intel_guc_setup() to > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c > after all). > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------ > drivers/gpu/drm/i915/intel_uc.h | 2 +- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f86a71d9..6af4e85 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv) > intel_mocs_init_l3cc_table(dev_priv); > > /* We can't enable contexts until all firmware is loaded */ > - ret = intel_guc_setup(dev_priv); > + ret = intel_guc_load(dev_priv); > if (ret) > goto out; > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index 21db697..f8b28b1 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv) > } > > /** > - * intel_guc_setup() - finish preparing the GuC for activity > + * intel_guc_load() - finish preparing the GuC for activity > * @dev_priv: i915 device private > * > - * Called from gem_init_hw() during driver loading and also after a GPU reset. > + * Called during driver loading and also 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_guc_init(), so here we need only check that > + * worked, and then transfer the image to the h/w. > * > * Return: non-zero code on error > */ > -int intel_guc_setup(struct drm_i915_private *dev_priv) > +int intel_guc_load(struct drm_i915_private *dev_priv) Can we use this refactor effort to fix also inconsistency in params passed to functions that start with "intel_guc_" prefix ? Some require dev_priv, while other expect pointer to intel_guc struct. My preferrence would be latter syntax with *guc. Note that if required we can easily get dev_priv using guc_to_i915(). Other option, use "i915_guc" prefix and then pass dev_priv to distinguish between these two forms. Michal > { > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > const char *fw_path = guc_fw->guc_fw_path; > @@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv, > * Called early during driver load, but after GEM is initialised. > * > * The firmware will be transferred to the GuC's memory later, > - * when intel_guc_setup() is called. > + * when intel_guc_load() is called. > */ > void intel_guc_init(struct drm_i915_private *dev_priv) > { > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 11f5608..7222e6c 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val); > > /* intel_guc_loader.c */ > extern void intel_guc_init(struct drm_i915_private *dev_priv); > -extern int intel_guc_setup(struct drm_i915_private *dev_priv); > +extern int intel_guc_load(struct drm_i915_private *dev_priv); > extern void intel_guc_fini(struct drm_i915_private *dev_priv); > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 15, 2016 at 05:22:53PM +0100, Michal Wajdeczko wrote: > On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote: > > GuC historically has two "startup" functions called _init() and _setup() > > > > Then HuC came with it's _init() and _load(). > > > > To make naming more consistent this commit renames intel_guc_setup() to > > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c > > after all). > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 +- > > drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------ > > drivers/gpu/drm/i915/intel_uc.h | 2 +- > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index f86a71d9..6af4e85 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv) > > intel_mocs_init_l3cc_table(dev_priv); > > > > /* We can't enable contexts until all firmware is loaded */ > > - ret = intel_guc_setup(dev_priv); > > + ret = intel_guc_load(dev_priv); > > if (ret) > > goto out; > > > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > > index 21db697..f8b28b1 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > > @@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv) > > } > > > > /** > > - * intel_guc_setup() - finish preparing the GuC for activity > > + * intel_guc_load() - finish preparing the GuC for activity > > * @dev_priv: i915 device private > > * > > - * Called from gem_init_hw() during driver loading and also after a GPU reset. > > + * Called during driver loading and also 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_guc_init(), so here we need only check that > > + * worked, and then transfer the image to the h/w. > > * > > * Return: non-zero code on error > > */ > > -int intel_guc_setup(struct drm_i915_private *dev_priv) > > +int intel_guc_load(struct drm_i915_private *dev_priv) > > Can we use this refactor effort to fix also inconsistency in params > passed to functions that start with "intel_guc_" prefix ? > Some require dev_priv, while other expect pointer to intel_guc struct. > My preferrence would be latter syntax with *guc. > > Note that if required we can easily get dev_priv using guc_to_i915(). > > Other option, use "i915_guc" prefix and then pass dev_priv to distinguish > between these two forms. > Good idea. I'll include it in vol. 2. > Michal > > > { > > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; > > const char *fw_path = guc_fw->guc_fw_path; > > @@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv, > > * Called early during driver load, but after GEM is initialised. > > * > > * The firmware will be transferred to the GuC's memory later, > > - * when intel_guc_setup() is called. > > + * when intel_guc_load() is called. > > */ > > void intel_guc_init(struct drm_i915_private *dev_priv) > > { > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index 11f5608..7222e6c 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val); > > > > /* intel_guc_loader.c */ > > extern void intel_guc_init(struct drm_i915_private *dev_priv); > > -extern int intel_guc_setup(struct drm_i915_private *dev_priv); > > +extern int intel_guc_load(struct drm_i915_private *dev_priv); > > extern void intel_guc_fini(struct drm_i915_private *dev_priv); > > extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); > > extern int intel_guc_suspend(struct drm_i915_private *dev_priv); > > -- > > 2.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 15, 2016 at 03:57:01PM +0000, Chris Wilson wrote: > On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote: > > GuC historically has two "startup" functions called _init() and _setup() > > > > Then HuC came with it's _init() and _load(). > > > > To make naming more consistent this commit renames intel_guc_setup() to > > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c > > after all). > > Or init_hw as that is the initialisation phase it is called from. > -Chris Since it's intel_guc_loader.c I somehow prefer _load() here. But intel_uc_load() which, is introduced with the series and call intel_guc_load() can be renamed to intel_uc_init_hw() What do you think?
On Fri, Dec 16, 2016 at 12:47:26PM +0100, Arkadiusz Hiler wrote: > On Thu, Dec 15, 2016 at 03:57:01PM +0000, Chris Wilson wrote: > > On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote: > > > GuC historically has two "startup" functions called _init() and _setup() > > > > > > Then HuC came with it's _init() and _load(). > > > > > > To make naming more consistent this commit renames intel_guc_setup() to > > > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c > > > after all). > > > > Or init_hw as that is the initialisation phase it is called from. > > -Chris > > Since it's intel_guc_loader.c I somehow prefer _load() here. > > But intel_uc_load() which, is introduced with the series and call > intel_guc_load() can be renamed to intel_uc_init_hw() > > What do you think? We want to push the "phase verb" as far as it makes sense, especially along the chain i.e. driver -> subsystem -> subsubsystem -> ... Once we are in the handler, it should use the right functions named appropriate. I still think carrying the phase verb as far as possible is important (more important say for init_early) as that carries the information about the rest of the driver state and the limitations we must keep in mind. Good taste should prevail ofc; the actual work must be done by sensibly named functions. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f86a71d9..6af4e85 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv) intel_mocs_init_l3cc_table(dev_priv); /* We can't enable contexts until all firmware is loaded */ - ret = intel_guc_setup(dev_priv); + ret = intel_guc_load(dev_priv); if (ret) goto out; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 21db697..f8b28b1 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv) } /** - * intel_guc_setup() - finish preparing the GuC for activity + * intel_guc_load() - finish preparing the GuC for activity * @dev_priv: i915 device private * - * Called from gem_init_hw() during driver loading and also after a GPU reset. + * Called during driver loading and also 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_guc_init(), so here we need only check that + * worked, and then transfer the image to the h/w. * * Return: non-zero code on error */ -int intel_guc_setup(struct drm_i915_private *dev_priv) +int intel_guc_load(struct drm_i915_private *dev_priv) { struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw; const char *fw_path = guc_fw->guc_fw_path; @@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv, * Called early during driver load, but after GEM is initialised. * * The firmware will be transferred to the GuC's memory later, - * when intel_guc_setup() is called. + * when intel_guc_load() is called. */ void intel_guc_init(struct drm_i915_private *dev_priv) { diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 11f5608..7222e6c 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val); /* intel_guc_loader.c */ extern void intel_guc_init(struct drm_i915_private *dev_priv); -extern int intel_guc_setup(struct drm_i915_private *dev_priv); +extern int intel_guc_load(struct drm_i915_private *dev_priv); extern void intel_guc_fini(struct drm_i915_private *dev_priv); extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status); extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
GuC historically has two "startup" functions called _init() and _setup() Then HuC came with it's _init() and _load(). To make naming more consistent this commit renames intel_guc_setup() to intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c after all). Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------ drivers/gpu/drm/i915/intel_uc.h | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-)