| Message ID | 20170214161541.26781-4-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote: > Let intel_guc_init() focus on determining and fetching the correct > firmware. > > This patch introduces intel_sanitize_uc_params() that is called from > intel_uc_init(). > > Then, if we have GuC, we can call intel_guc_init() conditionally and we > do not have to do internal checks. > > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/intel_guc_loader.c | 15 +-------------- > drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++++++++++++++- > 3 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 6d0798e..e520895 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv) > > i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores); > DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores)); > + > + intel_uc_sanitize_params(dev_priv); > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index f5efe28..db5713c 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > } > > /** > - * intel_guc_init() - define parameters and fetch firmware > + * intel_guc_init() - determine and fetch firmware Again, can we have function name that corresponds to its purpose. Comment alone is not sufficient > * @dev_priv: i915 device private > * > * Called early during driver load, but after GEM is initialised. > @@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; > const char *fw_path; > > - if (!HAS_GUC(dev_priv)) { > - i915.enable_guc_loading = 0; > - i915.enable_guc_submission = 0; > - } else { > - /* A negative value means "use platform default" */ > - if (i915.enable_guc_loading < 0) > - i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv); > - if (i915.enable_guc_submission < 0) > - i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv); > - } > - > if (!HAS_GUC_UCODE(dev_priv)) { > fw_path = NULL; > } else if (IS_SKYLAKE(dev_priv)) { > @@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv) > guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; > > /* Early (and silent) return if GuC loading is disabled */ I think this comment was related to the line that you just deleted > - if (!i915.enable_guc_loading) > - return; > if (fw_path == NULL) > return; > if (*fw_path == '\0') > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index d9d0566..d1ca41d 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -25,6 +25,24 @@ > #include "i915_drv.h" > #include "intel_uc.h" > > +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv) > +{ > + if (!HAS_GUC(dev_priv)) { Maybe we should warn user if specified explicit guc params need to be nuked? > + i915.enable_guc_loading = 0; > + i915.enable_guc_submission = 0; > + } else { > + /* A negative value means "use platform default" */ > + if (i915.enable_guc_loading < 0) > + i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv); > + if (i915.enable_guc_submission < 0) > + i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv); > + } > + > + /* can't enable guc submission without guc loaded */ > + if (!i915.enable_guc_loading) > + i915.enable_guc_submission = 0; > +} > + > void intel_uc_init_early(struct drm_i915_private *dev_priv) > { > mutex_init(&dev_priv->guc.send_mutex); > @@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) > > void intel_uc_init(struct drm_i915_private *dev_priv) > { > - intel_huc_init(dev_priv); > + if (!i915.enable_guc_loading) > + return; > + > + if (HAS_HUC_UCODE(dev_priv)) Hmm, if I recall correctly, same condition is checked in huc_init()... Btw, what approach is more preferred? check before call or inside? Regards, Michal > + intel_huc_init(dev_priv); > + > intel_guc_init(dev_priv); > } > > -- > 2.9.3 >
On ti, 2017-02-14 at 20:37 +0100, Michal Wajdeczko wrote: > On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote: > > > > Let intel_guc_init() focus on determining and fetching the correct > > firmware. > > > > This patch introduces intel_sanitize_uc_params() that is called from > > intel_uc_init(). > > > > Then, if we have GuC, we can call intel_guc_init() conditionally and we > > do not have to do internal checks. > > > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> <SNIP> > > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > > } > > > > /** > > - * intel_guc_init() - define parameters and fetch firmware > > + * intel_guc_init() - determine and fetch firmware > > Again, can we have function name that corresponds to its purpose. > Comment alone is not sufficient I second Michal here, the naming of the functions is rather poor. Something like intel_guc_fetch_firmware would be more descriptive. Making it overly generic when we don't have other use yet, will cause more churn when we get more use (because fortune telling was never our thing). I'd rather see rather standalone functions, which are then be called from central point where more can be added, without changing the existing ones. So this applies to intel_uc_init, too. It'll be easier to add these generic functions iff we start repeating calls to a bunch of functions, and we can then see patterns emerging. Regards, Joonas
On Wed, Feb 15, 2017 at 02:48:59PM +0200, Joonas Lahtinen wrote: > On ti, 2017-02-14 at 20:37 +0100, Michal Wajdeczko wrote: > > On Tue, Feb 14, 2017 at 05:15:39PM +0100, Arkadiusz Hiler wrote: > > > > > > Let intel_guc_init() focus on determining and fetching the correct > > > firmware. > > > > > > This patch introduces intel_sanitize_uc_params() that is called from > > > intel_uc_init(). > > > > > > Then, if we have GuC, we can call intel_guc_init() conditionally and we > > > do not have to do internal checks. > > > > > > Cc: Michal Winiarski <michal.winiarski@intel.com> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > <SNIP> > > > > @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > > > } > > > > > > /** > > > - * intel_guc_init() - define parameters and fetch firmware > > > + * intel_guc_init() - determine and fetch firmware > > > > Again, can we have function name that corresponds to its purpose. > > Comment alone is not sufficient > > I second Michal here, the naming of the functions is rather poor. > Something like intel_guc_fetch_firmware would be more descriptive. > > Making it overly generic when we don't have other use yet, will cause > more churn when we get more use (because fortune telling was never our > thing). I'd rather see rather standalone functions, which are then be > called from central point where more can be added, without changing the > existing ones. So this applies to intel_uc_init, too. > > It'll be easier to add these generic functions iff we start repeating > calls to a bunch of functions, and we can then see patterns emerging. So, couple of possibilities I see: First: a) rename intel_?uc_init to intel_?uc_fw_fetch b) embed dev_priv in intel_uc_fw c) make them take intel_uc_fw struct only intel_uc_fw_ corresponds to struct we have, so it seems better fitting than what Joonas proposed. now we have intel_uc_fw_fetch that does the actual fetching, which is confusing, but we can rename it to something like intel_uc_fw_perform_fetch or something similar. Second: Go with what we have and rename it intel_?uc_fw_init as Michal proposed. What do you think?
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6d0798e..e520895 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -994,6 +994,8 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv) i915.semaphores = intel_sanitize_semaphores(dev_priv, i915.semaphores); DRM_DEBUG_DRIVER("use GPU sempahores? %s\n", yesno(i915.semaphores)); + + intel_uc_sanitize_params(dev_priv); } /** diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index f5efe28..db5713c 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -726,7 +726,7 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, } /** - * intel_guc_init() - define parameters and fetch firmware + * intel_guc_init() - determine and fetch firmware * @dev_priv: i915 device private * * Called early during driver load, but after GEM is initialised. @@ -739,17 +739,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv) struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; const char *fw_path; - if (!HAS_GUC(dev_priv)) { - i915.enable_guc_loading = 0; - i915.enable_guc_submission = 0; - } else { - /* A negative value means "use platform default" */ - if (i915.enable_guc_loading < 0) - i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv); - if (i915.enable_guc_submission < 0) - i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv); - } - if (!HAS_GUC_UCODE(dev_priv)) { fw_path = NULL; } else if (IS_SKYLAKE(dev_priv)) { @@ -773,8 +762,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv) guc_fw->load_status = INTEL_UC_FIRMWARE_NONE; /* Early (and silent) return if GuC loading is disabled */ - if (!i915.enable_guc_loading) - return; if (fw_path == NULL) return; if (*fw_path == '\0') diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index d9d0566..d1ca41d 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -25,6 +25,24 @@ #include "i915_drv.h" #include "intel_uc.h" +void intel_uc_sanitize_params(struct drm_i915_private *dev_priv) +{ + if (!HAS_GUC(dev_priv)) { + i915.enable_guc_loading = 0; + i915.enable_guc_submission = 0; + } else { + /* A negative value means "use platform default" */ + if (i915.enable_guc_loading < 0) + i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv); + if (i915.enable_guc_submission < 0) + i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv); + } + + /* can't enable guc submission without guc loaded */ + if (!i915.enable_guc_loading) + i915.enable_guc_submission = 0; +} + void intel_uc_init_early(struct drm_i915_private *dev_priv) { mutex_init(&dev_priv->guc.send_mutex); @@ -32,7 +50,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) void intel_uc_init(struct drm_i915_private *dev_priv) { - intel_huc_init(dev_priv); + if (!i915.enable_guc_loading) + return; + + if (HAS_HUC_UCODE(dev_priv)) + intel_huc_init(dev_priv); + intel_guc_init(dev_priv); }
Let intel_guc_init() focus on determining and fetching the correct firmware. This patch introduces intel_sanitize_uc_params() that is called from intel_uc_init(). Then, if we have GuC, we can call intel_guc_init() conditionally and we do not have to do internal checks. Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_guc_loader.c | 15 +-------------- drivers/gpu/drm/i915/intel_uc.c | 25 ++++++++++++++++++++++++- 3 files changed, 27 insertions(+), 15 deletions(-)