| Message ID | 1382711810-25881-5-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/25 Imre Deak <imre.deak@intel.com>: > Similarly rename the other related functions in the power domain > interface. > > Higher level driver code calling these functions knows only about power > domains, not the underlying power wells which may be different on > different platforms. Also these functions really init/cleanup/resume > power domains and only through that all related power wells, so rename > them accordingly. > > Signed-off-by: Imre Deak <imre.deak@intel.com> I agree with the "_domains" rename, I think it's an improvement, but since you're already renaming things, I have to drop my bikeshed: we have intel_init_power_{well,domains} and i915_init_power_{well,domains}. IMHO this is really super annoyingly confusing, because they sound like they do the same thing. I know it's not your fault, but while you're at it, could you please propose names to unconfuse this? i915_init_power_well takes care of initializing the structs and pointers, while intel_init_power_well is the only one that touches hardware. A possible suggestion: - i915_init_power_well becomes intel_init_power_domains (just because I don't like the "i915_" prefix, since the PM code uses "intel_" for everything). - i915_remove_power_well becomes intel_remove_power_domains (to match the one above) - intel_init_power_well becomes intel_init_power_domains_hardware or intel_init_power_wells (since on the HW these things are actually called "power wells") or intel_init_hw_power_wells (to combine both suggestions) Thanks, Paulo > --- > drivers/gpu/drm/i915/i915_dma.c | 8 ++++---- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 6 +++--- > drivers/gpu/drm/i915/intel_pm.c | 10 +++++----- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index b722b35..75f02e4 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > if (ret) > goto cleanup_gem_stolen; > > - intel_init_power_well(dev); > + intel_init_power_domains(dev); > > /* Important: The output setup functions called by modeset_init need > * working irqs for e.g. gmbus and dp aux transfers. */ > @@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > } > > if (HAS_POWER_WELL(dev)) > - i915_init_power_well(dev); > + i915_init_power_domains(dev); > > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > ret = i915_load_modeset_init(dev); > @@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > out_power_well: > if (HAS_POWER_WELL(dev)) > - i915_remove_power_well(dev); > + i915_remove_power_domains(dev); > drm_vblank_cleanup(dev); > out_gem_unload: > if (dev_priv->mm.inactive_shrinker.scan_objects) > @@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev) > * the power well is not enabled, so just enable it in case > * we're going to unload/reload. */ > intel_display_set_init_power(dev, true); > - i915_remove_power_well(dev); > + i915_remove_power_domains(dev); > } > > i915_teardown_sysfs(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 770c9f8..e4474c6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > mutex_unlock(&dev->struct_mutex); > } > > - intel_init_power_well(dev); > + intel_init_power_domains(dev); > > i915_restore_state(dev); > intel_opregion_setup(dev); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bf4394a..ae6d362 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev); > void intel_update_fbc(struct drm_device *dev); > void intel_gpu_ips_init(struct drm_i915_private *dev_priv); > void intel_gpu_ips_teardown(void); > -int i915_init_power_well(struct drm_device *dev); > -void i915_remove_power_well(struct drm_device *dev); > +int i915_init_power_domains(struct drm_device *dev); > +void i915_remove_power_domains(struct drm_device *dev); > bool intel_display_power_enabled(struct drm_device *dev, > enum intel_display_power_domain domain); > void intel_display_power_get(struct drm_device *dev, > enum intel_display_power_domain domain); > void intel_display_power_put(struct drm_device *dev, > enum intel_display_power_domain domain); > -void intel_init_power_well(struct drm_device *dev); > +void intel_init_power_domains(struct drm_device *dev); > void intel_set_power_well(struct drm_device *dev, bool enable); > void intel_enable_gt_powersave(struct drm_device *dev); > void intel_disable_gt_powersave(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4f84a4b..9afec23 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5697,7 +5697,7 @@ void i915_release_power_well(void) > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > > -int i915_init_power_well(struct drm_device *dev) > +int i915_init_power_domains(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct i915_power_domains *power_domains = &dev_priv->power_domains; > @@ -5712,12 +5712,12 @@ int i915_init_power_well(struct drm_device *dev) > return 0; > } > > -void i915_remove_power_well(struct drm_device *dev) > +void i915_remove_power_domains(struct drm_device *dev) > { > hsw_pwr = NULL; > } > > -static void intel_resume_power_well(struct drm_device *dev) > +static void intel_resume_power_domains(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct i915_power_domains *power_domains = &dev_priv->power_domains; > @@ -5740,7 +5740,7 @@ static void intel_resume_power_well(struct drm_device *dev) > * to be enabled, and it will only be disabled if none of the registers is > * requesting it to be enabled. > */ > -void intel_init_power_well(struct drm_device *dev) > +void intel_init_power_domains(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -5749,7 +5749,7 @@ void intel_init_power_well(struct drm_device *dev) > > /* For now, we need the power well to be always enabled. */ > intel_display_set_init_power(dev, true); > - intel_resume_power_well(dev); > + intel_resume_power_domains(dev); > > /* We're taking over the BIOS, so clear any requests made by it since > * the driver is in charge now. */ > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 25, 2013 at 06:10:31PM -0200, Paulo Zanoni wrote: > 2013/10/25 Imre Deak <imre.deak@intel.com>: > > Similarly rename the other related functions in the power domain > > interface. > > > > Higher level driver code calling these functions knows only about power > > domains, not the underlying power wells which may be different on > > different platforms. Also these functions really init/cleanup/resume > > power domains and only through that all related power wells, so rename > > them accordingly. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > I agree with the "_domains" rename, I think it's an improvement, but > since you're already renaming things, I have to drop my bikeshed: we > have intel_init_power_{well,domains} and > i915_init_power_{well,domains}. IMHO this is really super annoyingly > confusing, because they sound like they do the same thing. I know it's > not your fault, but while you're at it, could you please propose names > to unconfuse this? > > i915_init_power_well takes care of initializing the structs and > pointers, while intel_init_power_well is the only one that touches > hardware. A possible suggestion: > > - i915_init_power_well becomes intel_init_power_domains (just because > I don't like the "i915_" prefix, since the PM code uses "intel_" for > everything). > - i915_remove_power_well becomes intel_remove_power_domains (to match > the one above) > - intel_init_power_well becomes intel_init_power_domains_hardware or > intel_init_power_wells (since on the HW these things are actually > called "power wells") or intel_init_hw_power_wells (to combine both > suggestions) The pattern we have thus far is $prefix_$block_init and $prefix_$block_init_hw. Occasionally the $block is at the end, but for cases where we have both an init and an init_hw function I think putting the init[_hw] at the end to make it stick out more is better. Just my 2 bikesheds ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index b722b35..75f02e4 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev) if (ret) goto cleanup_gem_stolen; - intel_init_power_well(dev); + intel_init_power_domains(dev); /* Important: The output setup functions called by modeset_init need * working irqs for e.g. gmbus and dp aux transfers. */ @@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) } if (HAS_POWER_WELL(dev)) - i915_init_power_well(dev); + i915_init_power_domains(dev); if (drm_core_check_feature(dev, DRIVER_MODESET)) { ret = i915_load_modeset_init(dev); @@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) out_power_well: if (HAS_POWER_WELL(dev)) - i915_remove_power_well(dev); + i915_remove_power_domains(dev); drm_vblank_cleanup(dev); out_gem_unload: if (dev_priv->mm.inactive_shrinker.scan_objects) @@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev) * the power well is not enabled, so just enable it in case * we're going to unload/reload. */ intel_display_set_init_power(dev, true); - i915_remove_power_well(dev); + i915_remove_power_domains(dev); } i915_teardown_sysfs(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 770c9f8..e4474c6 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) mutex_unlock(&dev->struct_mutex); } - intel_init_power_well(dev); + intel_init_power_domains(dev); i915_restore_state(dev); intel_opregion_setup(dev); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bf4394a..ae6d362 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev); void intel_update_fbc(struct drm_device *dev); void intel_gpu_ips_init(struct drm_i915_private *dev_priv); void intel_gpu_ips_teardown(void); -int i915_init_power_well(struct drm_device *dev); -void i915_remove_power_well(struct drm_device *dev); +int i915_init_power_domains(struct drm_device *dev); +void i915_remove_power_domains(struct drm_device *dev); bool intel_display_power_enabled(struct drm_device *dev, enum intel_display_power_domain domain); void intel_display_power_get(struct drm_device *dev, enum intel_display_power_domain domain); void intel_display_power_put(struct drm_device *dev, enum intel_display_power_domain domain); -void intel_init_power_well(struct drm_device *dev); +void intel_init_power_domains(struct drm_device *dev); void intel_set_power_well(struct drm_device *dev, bool enable); void intel_enable_gt_powersave(struct drm_device *dev); void intel_disable_gt_powersave(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4f84a4b..9afec23 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5697,7 +5697,7 @@ void i915_release_power_well(void) } EXPORT_SYMBOL_GPL(i915_release_power_well); -int i915_init_power_well(struct drm_device *dev) +int i915_init_power_domains(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct i915_power_domains *power_domains = &dev_priv->power_domains; @@ -5712,12 +5712,12 @@ int i915_init_power_well(struct drm_device *dev) return 0; } -void i915_remove_power_well(struct drm_device *dev) +void i915_remove_power_domains(struct drm_device *dev) { hsw_pwr = NULL; } -static void intel_resume_power_well(struct drm_device *dev) +static void intel_resume_power_domains(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct i915_power_domains *power_domains = &dev_priv->power_domains; @@ -5740,7 +5740,7 @@ static void intel_resume_power_well(struct drm_device *dev) * to be enabled, and it will only be disabled if none of the registers is * requesting it to be enabled. */ -void intel_init_power_well(struct drm_device *dev) +void intel_init_power_domains(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -5749,7 +5749,7 @@ void intel_init_power_well(struct drm_device *dev) /* For now, we need the power well to be always enabled. */ intel_display_set_init_power(dev, true); - intel_resume_power_well(dev); + intel_resume_power_domains(dev); /* We're taking over the BIOS, so clear any requests made by it since * the driver is in charge now. */
Similarly rename the other related functions in the power domain interface. Higher level driver code calling these functions knows only about power domains, not the underlying power wells which may be different on different platforms. Also these functions really init/cleanup/resume power domains and only through that all related power wells, so rename them accordingly. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 8 ++++---- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 6 +++--- drivers/gpu/drm/i915/intel_pm.c | 10 +++++----- 4 files changed, 13 insertions(+), 13 deletions(-)