Hi Tvrtko, Thank you for your valuable comments. We have gone through it. I'll be submitting revised patch-sets after incorporating all your review comments.
> On 21/09/2018 10:13, [email protected] wrote: > > From: Praveen Diwakar <[email protected]> > > > > High resoluton timer is used for this purpose. > > > > Debugfs is provided to enable/disable/update timer configuration > > > > Change-Id: I35d692c5afe962fcad4573185bc6f744487711d0 > > Signed-off-by: Praveen Diwakar <[email protected]> > > Signed-off-by: Yogesh Marathe <[email protected]> > > Signed-off-by: Aravindan Muthukumar <[email protected]> > > Signed-off-by: Kedar J Karanje <[email protected]> > > Signed-off-by: Ankit Navik <[email protected] > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 94 > ++++++++++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 2 files changed, 94 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index f9ce35d..81ba509 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -4740,6 +4740,97 @@ static const struct drm_info_list > i915_debugfs_list[] = { > > {"i915_drrs_status", i915_drrs_status, 0}, > > {"i915_rps_boost_info", i915_rps_boost_info, 0}, > > }; > > + > > +#define POLL_PERIOD_MS (1000 * 1000) > > +#define PENDING_REQ_0 0 /* No active request pending*/ > > +#define PENDING_REQ_3 3 /* Threshold value of 3 active request > pending*/ > > + /* Anything above this is considered as HIGH load > > + * context > > + */ > > + /* And less is considered as LOW load*/ > > + /* And equal is considered as mediaum load */ > > Wonky comments and some typos up to here. > > > + > > +static int predictive_load_enable; > > +static int predictive_load_timer_init; > > + > > +static enum hrtimer_restart predictive_load_cb(struct hrtimer > > +*hrtimer) { > > + struct drm_i915_private *dev_priv = > > + container_of(hrtimer, typeof(*dev_priv), > > + pred_timer); > > + enum intel_engine_id id; > > + struct intel_engine_cs *engine; > > Some unused's. > > > + struct i915_gem_context *ctx; > > + u64 req_pending; > > unsigned long, and also please try to order declaration so the right edge of > text > is moving in one direction only. > > > + > > + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { > > + > > + if (!ctx->name) > > + continue; > > What is this? > > > + > > + mutex_lock(&dev_priv->pred_mutex); > > Here the mutex bites you since you cannot sleep in the timer callback. > atomic_t would solve it. Or would a native unsigned int/long since lock to > read a > native word on x86 is not needed. > > > + req_pending = ctx->req_cnt; > > + mutex_unlock(&dev_priv->pred_mutex); > > + > > + if (req_pending == PENDING_REQ_0) > > + continue; > > + > > + if (req_pending > PENDING_REQ_3) > > + ctx->load_type = LOAD_TYPE_HIGH; > > + else if (req_pending == PENDING_REQ_3) > > + ctx->load_type = LOAD_TYPE_MEDIUM; > > + else if (req_pending < PENDING_REQ_3) > > Must be smaller if not greater or equal, but maybe the compiler does that for > you. > > > + ctx->load_type = LOAD_TYPE_LOW; > > + > > + i915_set_optimum_config(ctx->load_type, ctx, > KABYLAKE_GT3); > > Only KBL? Idea to put the table in dev_priv FTW! :) > > ctx->load_type used only as a temporary uncovered here? :) > > > + } > > + > > + hrtimer_forward_now(hrtimer, > > + > ns_to_ktime(predictive_load_enable*POLL_PERIOD_MS)); > > Or HRTIMER_NORESTART if disabled? Hard to call it, details.. > > > + > > + return HRTIMER_RESTART; > > +} > > + > > +static int > > +i915_predictive_load_get(void *data, u64 *val) { > > + struct drm_i915_private *dev_priv = data; > > + > > + *val = predictive_load_enable; > > + return 0; > > +} > > + > > +static int > > +i915_predictive_load_set(void *data, u64 val) { > > + struct drm_i915_private *dev_priv = data; > > + struct intel_device_info *info; > > + > > + info = mkwrite_device_info(dev_priv); > > Unused, why? > > > + > > + predictive_load_enable = val; > > + > > + if (predictive_load_enable) { > > + if (!predictive_load_timer_init) { > > + hrtimer_init(&dev_priv->pred_timer, > CLOCK_MONOTONIC, > > + HRTIMER_MODE_REL); > > + dev_priv->pred_timer.function = predictive_load_cb; > > + predictive_load_timer_init = 1; > > Move timer init to dev_priv setup. > > > + } > > + hrtimer_start(&dev_priv->pred_timer, > > + > ns_to_ktime(predictive_load_enable*POLL_PERIOD_MS), > > + HRTIMER_MODE_REL_PINNED); > > Two threads can race to here. > > Also you can give some slack to the timer since precision is not critical to > you > and can save you some CPU power. > > > + } else { > > + hrtimer_cancel(&dev_priv->pred_timer); > > + } > > + > > + return 0; > > +} > > + > > +DEFINE_SIMPLE_ATTRIBUTE(i915_predictive_load_ctl, > > + i915_predictive_load_get, i915_predictive_load_set, > > + "%llu\n"); > > + > > #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) > > > > static const struct i915_debugfs_files { @@ -4769,7 +4860,8 @@ > > static const struct i915_debugfs_files { > > {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops}, > > {"i915_ipc_status", &i915_ipc_status_fops}, > > {"i915_drrs_ctl", &i915_drrs_ctl_fops}, > > - {"i915_edp_psr_debug", &i915_edp_psr_debug_fops} > > + {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}, > > + {"i915_predictive_load_ctl", &i915_predictive_load_ctl} > > And if the feature is to become real one day, given it's nature, the knob > would > probably have to go to sysfs, not debugfs. > > > }; > > > > int i915_debugfs_register(struct drm_i915_private *dev_priv) diff > > --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index 137ec33..0505c47 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1552,6 +1552,7 @@ struct intel_cdclk_state { > > }; > > > > struct drm_i915_private { > > + struct hrtimer pred_timer; > > Surely not the first member! :) > > Regards, > > Tvrtko > > > struct drm_device drm; > > > > struct kmem_cache *objects; > > Regards, Ankit _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
