Re: [Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm
Quoting John Harrison (2019-01-10 01:10:09) > On 1/9/2019 16:24, John Harrison wrote: > > On 1/7/2019 03:54, Chris Wilson wrote: > >> Frequently, we use intel_runtime_pm_get/_put around a small block. > >> Formalise that usage by providing a macro to define such a block with an > >> automatic closure to scope the intel_runtime_pm wakeref to that block, > >> i.e. macro abuse smelling of python. > >> > >> Signed-off-by: Chris Wilson > >> Cc: Jani Nikula > >> --- > >> +#define with_intel_runtime_pm(i915, wf) \ > >> + for (wf = intel_runtime_pm_get(i915); wf; \ > >> + intel_runtime_pm_put(i915, wf), wf = 0) > >> + > >> +#define with_intel_runtime_pm_if_in_use(i915, wf) \ > >> + for (wf = intel_runtime_pm_get_if_in_use(i915); wf; \ > >> + intel_runtime_pm_put(i915, wf), wf = 0) > >> + > > This is a potential change in behaviour. Previously the simple 'get' > > version would unconditionally execute the wrapped code. Whereas now, > > if the get function fails for some reason and returns zero, the > > wrapped code will be skipped. Currently, the get() function can't > > return zero - it returns -1 in the case of the tracking code failing > > to allocate or similar. But is that guaranteed to be the case > > forevermore? It would be a better match for the original behaviour if > > the 'for' loop of the 'get' version was unconditional and only the > > 'get_if_in_use' version could skip. E.g. something like: > > for (intel_wakeref_t loop = -1, wf = intel_runtime_pm_get(i915) ; > > loop; intel_runtime_pm_put(i915, wf), wf = loop = 0) > > > > Although that does mean the wf becomes local to the loop. On the other > > hand, I'm also not sure why it needs to be external anyway? If it is > > guaranteed to be zero on exit and any value on entry is overwritten, > > then why have it external at all? Would it not be neater/smaller > > source to get rid of all the local instantiations? > > > > John. > > > Doh. Not sure why I was thinking C99 extensions were valid in the > kernel. I can't think of an alternative way to fix the above issues > without making the macro truly hideous. So maybe it's not enough of a > worry to worry about. Using C99 would be a nice improvement for a lot of our macros, and I hope it comes to pass. Yes, the whole reason we return -1 on tracking-failure-but-rpm-success is so that we keep 0 as meaning rpm-failure so that the different cases are identifiable required for the markup and cookie tracking. So using -1 here just falls out of the general case. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm
On 1/9/2019 16:24, John Harrison wrote: On 1/7/2019 03:54, Chris Wilson wrote: Frequently, we use intel_runtime_pm_get/_put around a small block. Formalise that usage by providing a macro to define such a block with an automatic closure to scope the intel_runtime_pm wakeref to that block, i.e. macro abuse smelling of python. Signed-off-by: Chris Wilson Cc: Jani Nikula --- +#define with_intel_runtime_pm(i915, wf) \ + for (wf = intel_runtime_pm_get(i915); wf; \ + intel_runtime_pm_put(i915, wf), wf = 0) + +#define with_intel_runtime_pm_if_in_use(i915, wf) \ + for (wf = intel_runtime_pm_get_if_in_use(i915); wf; \ + intel_runtime_pm_put(i915, wf), wf = 0) + This is a potential change in behaviour. Previously the simple 'get' version would unconditionally execute the wrapped code. Whereas now, if the get function fails for some reason and returns zero, the wrapped code will be skipped. Currently, the get() function can't return zero - it returns -1 in the case of the tracking code failing to allocate or similar. But is that guaranteed to be the case forevermore? It would be a better match for the original behaviour if the 'for' loop of the 'get' version was unconditional and only the 'get_if_in_use' version could skip. E.g. something like: for (intel_wakeref_t loop = -1, wf = intel_runtime_pm_get(i915) ; loop; intel_runtime_pm_put(i915, wf), wf = loop = 0) Although that does mean the wf becomes local to the loop. On the other hand, I'm also not sure why it needs to be external anyway? If it is guaranteed to be zero on exit and any value on entry is overwritten, then why have it external at all? Would it not be neater/smaller source to get rid of all the local instantiations? John. Doh. Not sure why I was thinking C99 extensions were valid in the kernel. I can't think of an alternative way to fix the above issues without making the macro truly hideous. So maybe it's not enough of a worry to worry about. John. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm
On 1/7/2019 03:54, Chris Wilson wrote: Frequently, we use intel_runtime_pm_get/_put around a small block. Formalise that usage by providing a macro to define such a block with an automatic closure to scope the intel_runtime_pm wakeref to that block, i.e. macro abuse smelling of python. Signed-off-by: Chris Wilson Cc: Jani Nikula --- +#define with_intel_runtime_pm(i915, wf) \ + for (wf = intel_runtime_pm_get(i915); wf; \ +intel_runtime_pm_put(i915, wf), wf = 0) + +#define with_intel_runtime_pm_if_in_use(i915, wf) \ + for (wf = intel_runtime_pm_get_if_in_use(i915); wf; \ +intel_runtime_pm_put(i915, wf), wf = 0) + This is a potential change in behaviour. Previously the simple 'get' version would unconditionally execute the wrapped code. Whereas now, if the get function fails for some reason and returns zero, the wrapped code will be skipped. Currently, the get() function can't return zero - it returns -1 in the case of the tracking code failing to allocate or similar. But is that guaranteed to be the case forevermore? It would be a better match for the original behaviour if the 'for' loop of the 'get' version was unconditional and only the 'get_if_in_use' version could skip. E.g. something like: for (intel_wakeref_t loop = -1, wf = intel_runtime_pm_get(i915) ; loop; intel_runtime_pm_put(i915, wf), wf = loop = 0) Although that does mean the wf becomes local to the loop. On the other hand, I'm also not sure why it needs to be external anyway? If it is guaranteed to be zero on exit and any value on entry is overwritten, then why have it external at all? Would it not be neater/smaller source to get rid of all the local instantiations? John. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm
Chris Wilson writes:
> Frequently, we use intel_runtime_pm_get/_put around a small block.
> Formalise that usage by providing a macro to define such a block with an
> automatic closure to scope the intel_runtime_pm wakeref to that block,
> i.e. macro abuse smelling of python.
>
> Signed-off-by: Chris Wilson
> Cc: Jani Nikula
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 162 --
> drivers/gpu/drm/i915/i915_gem.c | 10 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++-
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 51 +++---
> drivers/gpu/drm/i915/i915_pmu.c | 7 +-
> drivers/gpu/drm/i915/i915_sysfs.c | 7 +-
> drivers/gpu/drm/i915/intel_drv.h | 8 +
> drivers/gpu/drm/i915/intel_guc_log.c | 26 ++-
> drivers/gpu/drm/i915/intel_huc.c | 7 +-
> drivers/gpu/drm/i915/intel_panel.c| 18 +-
> drivers/gpu/drm/i915/intel_uncore.c | 30 ++--
> drivers/gpu/drm/i915/selftests/i915_gem.c | 34 ++--
> .../gpu/drm/i915/selftests/i915_gem_context.c | 12 +-
> .../gpu/drm/i915/selftests/i915_gem_object.c | 11 +-
> .../drm/i915/selftests/intel_workarounds.c| 28 +--
> 15 files changed, 203 insertions(+), 231 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d667b05e7ca4..1521e08861d1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -953,9 +953,9 @@ static int i915_gpu_info_open(struct inode *inode, struct
> file *file)
> struct i915_gpu_state *gpu;
> intel_wakeref_t wakeref;
>
> - wakeref = intel_runtime_pm_get(i915);
> - gpu = i915_capture_gpu_state(i915);
> - intel_runtime_pm_put(i915, wakeref);
> + gpu = NULL;
> + with_intel_runtime_pm(i915, wakeref)
> + gpu = i915_capture_gpu_state(i915);
> if (IS_ERR(gpu))
> return PTR_ERR(gpu);
>
> @@ -1287,17 +1287,15 @@ static int i915_hangcheck_info(struct seq_file *m,
> void *unused)
> return 0;
> }
>
> - wakeref = intel_runtime_pm_get(dev_priv);
> + with_intel_runtime_pm(dev_priv, wakeref) {
> + for_each_engine(engine, dev_priv, id) {
> + acthd[id] = intel_engine_get_active_head(engine);
> + seqno[id] = intel_engine_get_seqno(engine);
> + }
>
> - for_each_engine(engine, dev_priv, id) {
> - acthd[id] = intel_engine_get_active_head(engine);
> - seqno[id] = intel_engine_get_seqno(engine);
> + intel_engine_get_instdone(dev_priv->engine[RCS], );
> }
>
> - intel_engine_get_instdone(dev_priv->engine[RCS], );
> -
> - intel_runtime_pm_put(dev_priv, wakeref);
> -
> if (timer_pending(_priv->gpu_error.hangcheck_work.timer))
> seq_printf(m, "Hangcheck active, timer fires in %dms\n",
>
> jiffies_to_msecs(dev_priv->gpu_error.hangcheck_work.timer.expires -
> @@ -1573,18 +1571,16 @@ static int i915_drpc_info(struct seq_file *m, void
> *unused)
> {
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> intel_wakeref_t wakeref;
> - int err;
> -
> - wakeref = intel_runtime_pm_get(dev_priv);
> -
> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> - err = vlv_drpc_info(m);
> - else if (INTEL_GEN(dev_priv) >= 6)
> - err = gen6_drpc_info(m);
> - else
> - err = ironlake_drpc_info(m);
> + int err = -ENODEV;
>
> - intel_runtime_pm_put(dev_priv, wakeref);
> + with_intel_runtime_pm(dev_priv, wakeref) {
> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + err = vlv_drpc_info(m);
> + else if (INTEL_GEN(dev_priv) >= 6)
> + err = gen6_drpc_info(m);
> + else
> + err = ironlake_drpc_info(m);
> + }
>
> return err;
> }
> @@ -2068,8 +2064,7 @@ static int i915_rps_boost_info(struct seq_file *m, void
> *data)
> intel_wakeref_t wakeref;
> struct drm_file *file;
>
> - wakeref = intel_runtime_pm_get_if_in_use(dev_priv);
> - if (wakeref) {
> + with_intel_runtime_pm_if_in_use(dev_priv, wakeref) {
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> mutex_lock(_priv->pcu_lock);
> act_freq = vlv_punit_read(dev_priv,
> @@ -2080,7 +2075,6 @@ static int i915_rps_boost_info(struct seq_file *m, void
> *data)
> act_freq = intel_get_cagf(dev_priv,
> I915_READ(GEN6_RPSTAT1));
> }
> - intel_runtime_pm_put(dev_priv, wakeref);
> }
>
> seq_printf(m, "RPS enabled? %d\n", rps->enabled);
> @@ -2172,9 +2166,8 @@ static int i915_huc_load_status_info(struct seq_file
> *m, void
[Intel-gfx] [PATCH 17/46] drm/i915: Syntatic sugar for using intel_runtime_pm
Frequently, we use intel_runtime_pm_get/_put around a small block.
Formalise that usage by providing a macro to define such a block with an
automatic closure to scope the intel_runtime_pm wakeref to that block,
i.e. macro abuse smelling of python.
Signed-off-by: Chris Wilson
Cc: Jani Nikula
---
drivers/gpu/drm/i915/i915_debugfs.c | 162 --
drivers/gpu/drm/i915/i915_gem.c | 10 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++-
drivers/gpu/drm/i915/i915_gem_shrinker.c | 51 +++---
drivers/gpu/drm/i915/i915_pmu.c | 7 +-
drivers/gpu/drm/i915/i915_sysfs.c | 7 +-
drivers/gpu/drm/i915/intel_drv.h | 8 +
drivers/gpu/drm/i915/intel_guc_log.c | 26 ++-
drivers/gpu/drm/i915/intel_huc.c | 7 +-
drivers/gpu/drm/i915/intel_panel.c| 18 +-
drivers/gpu/drm/i915/intel_uncore.c | 30 ++--
drivers/gpu/drm/i915/selftests/i915_gem.c | 34 ++--
.../gpu/drm/i915/selftests/i915_gem_context.c | 12 +-
.../gpu/drm/i915/selftests/i915_gem_object.c | 11 +-
.../drm/i915/selftests/intel_workarounds.c| 28 +--
15 files changed, 203 insertions(+), 231 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index d667b05e7ca4..1521e08861d1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -953,9 +953,9 @@ static int i915_gpu_info_open(struct inode *inode, struct
file *file)
struct i915_gpu_state *gpu;
intel_wakeref_t wakeref;
- wakeref = intel_runtime_pm_get(i915);
- gpu = i915_capture_gpu_state(i915);
- intel_runtime_pm_put(i915, wakeref);
+ gpu = NULL;
+ with_intel_runtime_pm(i915, wakeref)
+ gpu = i915_capture_gpu_state(i915);
if (IS_ERR(gpu))
return PTR_ERR(gpu);
@@ -1287,17 +1287,15 @@ static int i915_hangcheck_info(struct seq_file *m, void
*unused)
return 0;
}
- wakeref = intel_runtime_pm_get(dev_priv);
+ with_intel_runtime_pm(dev_priv, wakeref) {
+ for_each_engine(engine, dev_priv, id) {
+ acthd[id] = intel_engine_get_active_head(engine);
+ seqno[id] = intel_engine_get_seqno(engine);
+ }
- for_each_engine(engine, dev_priv, id) {
- acthd[id] = intel_engine_get_active_head(engine);
- seqno[id] = intel_engine_get_seqno(engine);
+ intel_engine_get_instdone(dev_priv->engine[RCS], );
}
- intel_engine_get_instdone(dev_priv->engine[RCS], );
-
- intel_runtime_pm_put(dev_priv, wakeref);
-
if (timer_pending(_priv->gpu_error.hangcheck_work.timer))
seq_printf(m, "Hangcheck active, timer fires in %dms\n",
jiffies_to_msecs(dev_priv->gpu_error.hangcheck_work.timer.expires -
@@ -1573,18 +1571,16 @@ static int i915_drpc_info(struct seq_file *m, void
*unused)
{
struct drm_i915_private *dev_priv = node_to_i915(m->private);
intel_wakeref_t wakeref;
- int err;
-
- wakeref = intel_runtime_pm_get(dev_priv);
-
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
- err = vlv_drpc_info(m);
- else if (INTEL_GEN(dev_priv) >= 6)
- err = gen6_drpc_info(m);
- else
- err = ironlake_drpc_info(m);
+ int err = -ENODEV;
- intel_runtime_pm_put(dev_priv, wakeref);
+ with_intel_runtime_pm(dev_priv, wakeref) {
+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ err = vlv_drpc_info(m);
+ else if (INTEL_GEN(dev_priv) >= 6)
+ err = gen6_drpc_info(m);
+ else
+ err = ironlake_drpc_info(m);
+ }
return err;
}
@@ -2068,8 +2064,7 @@ static int i915_rps_boost_info(struct seq_file *m, void
*data)
intel_wakeref_t wakeref;
struct drm_file *file;
- wakeref = intel_runtime_pm_get_if_in_use(dev_priv);
- if (wakeref) {
+ with_intel_runtime_pm_if_in_use(dev_priv, wakeref) {
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
mutex_lock(_priv->pcu_lock);
act_freq = vlv_punit_read(dev_priv,
@@ -2080,7 +2075,6 @@ static int i915_rps_boost_info(struct seq_file *m, void
*data)
act_freq = intel_get_cagf(dev_priv,
I915_READ(GEN6_RPSTAT1));
}
- intel_runtime_pm_put(dev_priv, wakeref);
}
seq_printf(m, "RPS enabled? %d\n", rps->enabled);
@@ -2172,9 +2166,8 @@ static int i915_huc_load_status_info(struct seq_file *m,
void *data)
p = drm_seq_file_printer(m);
intel_uc_fw_dump(_priv->huc.fw, );
- wakeref =
