| Message ID | 770f5f1c2dd99e4d6a314b70184e71b928a6d362.1556540890.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: the great header refactoring, part two | expand |
Quoting Jani Nikula (2019-04-29 13:29:37) > Commit 696173b064c6 ("drm/i915: extract intel_pm.h from intel_drv.h") > missed the declarations in i915_drv.h. Fwiw, I want to pull these along with gt powermanagement and rps into gt/intel_gt_pm.c and a few friends. Doesn't make much difference for this patch; just planned obsolescence. -Chris
On Mon, 29 Apr 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jani Nikula (2019-04-29 13:29:37) >> Commit 696173b064c6 ("drm/i915: extract intel_pm.h from intel_drv.h") >> missed the declarations in i915_drv.h. > > Fwiw, I want to pull these along with gt powermanagement and rps into > gt/intel_gt_pm.c and a few friends. > > Doesn't make much difference for this patch; just planned obsolescence. I'm fine either way, via this patch or directly. In general I like how it's easier to look at the new headers and wonder why on earth some functions are in the files they are, and try to come up with better division into files. --- I'm also trying to probe feedback on some style guidelines I might like to enforce in the future: 1) A file and the non-static functions in it should have the same prefix, i.e. intel_foo.c has functions prefixed intel_foo_*. 2) No file should have platform specific non-static functions, i.e. all the non-static functions should be intel_foo_* and this should internally split to platform_foo_* instead of leaving the if ladders or function pointer initializations to the callers. So, thoughts on naming the functions intel_gt_pm_* upon moving them? BR, Jani.
Quoting Jani Nikula (2019-04-29 16:03:33) > On Mon, 29 Apr 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Jani Nikula (2019-04-29 13:29:37) > >> Commit 696173b064c6 ("drm/i915: extract intel_pm.h from intel_drv.h") > >> missed the declarations in i915_drv.h. > > > > Fwiw, I want to pull these along with gt powermanagement and rps into > > gt/intel_gt_pm.c and a few friends. > > > > Doesn't make much difference for this patch; just planned obsolescence. > > I'm fine either way, via this patch or directly. > > In general I like how it's easier to look at the new headers and wonder > why on earth some functions are in the files they are, and try to come > up with better division into files. > > --- > > I'm also trying to probe feedback on some style guidelines I might like > to enforce in the future: > > 1) A file and the non-static functions in it should have the same > prefix, i.e. intel_foo.c has functions prefixed intel_foo_*. > > 2) No file should have platform specific non-static functions, i.e. all > the non-static functions should be intel_foo_* and this should > internally split to platform_foo_* instead of leaving the if ladders > or function pointer initializations to the callers. Agreed on these. GEM side has been moving to this direction slowly. > So, thoughts on naming the functions intel_gt_pm_* upon moving them? Sounds reasonable to me. Regards, Joonas
Quoting Joonas Lahtinen (2019-04-30 08:24:07) > Quoting Jani Nikula (2019-04-29 16:03:33) > > On Mon, 29 Apr 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Quoting Jani Nikula (2019-04-29 13:29:37) > > >> Commit 696173b064c6 ("drm/i915: extract intel_pm.h from intel_drv.h") > > >> missed the declarations in i915_drv.h. > > > > > > Fwiw, I want to pull these along with gt powermanagement and rps into > > > gt/intel_gt_pm.c and a few friends. > > > > > > Doesn't make much difference for this patch; just planned obsolescence. > > > > I'm fine either way, via this patch or directly. > > > > In general I like how it's easier to look at the new headers and wonder > > why on earth some functions are in the files they are, and try to come > > up with better division into files. > > > > --- > > > > I'm also trying to probe feedback on some style guidelines I might like > > to enforce in the future: > > > > 1) A file and the non-static functions in it should have the same > > prefix, i.e. intel_foo.c has functions prefixed intel_foo_*. > > > > 2) No file should have platform specific non-static functions, i.e. all > > the non-static functions should be intel_foo_* and this should > > internally split to platform_foo_* instead of leaving the if ladders > > or function pointer initializations to the callers. > > Agreed on these. GEM side has been moving to this direction slowly. > > > So, thoughts on naming the functions intel_gt_pm_* upon moving them? > > Sounds reasonable to me. And indeed the patches from last year where already making that transformation :) Next generation of patches are aiming to split up the different functions under the intel_gt_pm umbrella, but still following the same principle. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6cc749..16b2ce7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3370,19 +3370,6 @@ void icl_combo_phys_uninit(struct drm_i915_private *dev_priv); void cnl_combo_phys_init(struct drm_i915_private *dev_priv); void cnl_combo_phys_uninit(struct drm_i915_private *dev_priv); -int intel_gpu_freq(struct drm_i915_private *dev_priv, int val); -int intel_freq_opcode(struct drm_i915_private *dev_priv, int val); -u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, - const i915_reg_t reg); - -u32 intel_get_cagf(struct drm_i915_private *dev_priv, u32 rpstat1); - -static inline u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv, - const i915_reg_t reg) -{ - return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(dev_priv, reg), 1000); -} - #define __I915_REG_OP(op__, dev_priv__, ...) \ intel_uncore_##op__(&(dev_priv__)->uncore, __VA_ARGS__) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 35e5024..1ccda0e 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -9,8 +9,9 @@ #include "gt/intel_engine.h" -#include "i915_pmu.h" #include "i915_drv.h" +#include "i915_pmu.h" +#include "intel_pm.h" /* Frequency for the sampling timer for events which need it. */ #define FREQUENCY 200 diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 9bb3a15..3ef07b 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -30,9 +30,10 @@ #include <linux/stat.h> #include <linux/sysfs.h> +#include "i915_drv.h" #include "intel_drv.h" +#include "intel_pm.h" #include "intel_sideband.h" -#include "i915_drv.h" static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 350a671..7a7c5e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -9901,6 +9901,12 @@ u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, return mul_u64_u32_div(time_hw, mul, div); } +u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv, + i915_reg_t reg) +{ + return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(dev_priv, reg), 1000); +} + u32 intel_get_cagf(struct drm_i915_private *dev_priv, u32 rpstat) { u32 cagf; diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h index 674a3f..17339c 100644 --- a/drivers/gpu/drm/i915/intel_pm.h +++ b/drivers/gpu/drm/i915/intel_pm.h @@ -8,6 +8,8 @@ #include <linux/types.h> +#include "i915_reg.h" + struct drm_atomic_state; struct drm_device; struct drm_i915_private; @@ -68,4 +70,12 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc, void intel_init_ipc(struct drm_i915_private *dev_priv); void intel_enable_ipc(struct drm_i915_private *dev_priv); +int intel_gpu_freq(struct drm_i915_private *dev_priv, int val); +int intel_freq_opcode(struct drm_i915_private *dev_priv, int val); +u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, i915_reg_t reg); +u64 intel_rc6_residency_us(struct drm_i915_private *dev_priv, i915_reg_t reg); + +u32 intel_get_cagf(struct drm_i915_private *dev_priv, u32 rpstat1); + + #endif /* __INTEL_PM_H__ */
Commit 696173b064c6 ("drm/i915: extract intel_pm.h from intel_drv.h") missed the declarations in i915_drv.h. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 13 ------------- drivers/gpu/drm/i915/i915_pmu.c | 3 ++- drivers/gpu/drm/i915/i915_sysfs.c | 3 ++- drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ drivers/gpu/drm/i915/intel_pm.h | 10 ++++++++++ 5 files changed, 20 insertions(+), 15 deletions(-)