From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 3/3] drm/i915: Add boot paramter to control rps boost at boot time. Date: Thu, 13 Mar 2014 20:16:29 +0200 Message-ID: <20140313181628.GO20292@intel.com> References: <531718EF.3060201@intel.com> <1394726418-10831-1-git-send-email-deepak.s@linux.intel.com> <1394726418-10831-4-git-send-email-deepak.s@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id C32A0FAA7D for ; Thu, 13 Mar 2014 11:24:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1394726418-10831-4-git-send-email-deepak.s@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: deepak.s@linux.intel.com Cc: Deepak S , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 13, 2014 at 09:30:18PM +0530, deepak.s@linux.intel.com wrote: > From: Deepak S > = > We are adding a module paramter to control rps boost. By default, we > enable the boost for better performace. Based on the need (perf/power) > we can either enable/disable. > = > v2: Addressed rps default comment (Jani) > = > Signed-off-by: Deepak S > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 16 +++++++++++++++- > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > 3 files changed, 21 insertions(+), 1 deletion(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 607042b..7808319 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2106,6 +2106,7 @@ struct i915_params { > int panel_use_ssc; > int vbt_sdvo_panel_type; > int enable_rc6; > + int enable_rps_boost; Should be bool like Jani said. And then it should be thrown somewhere somewhere at the end of the structure next to the other bools. > int enable_fbc; > int enable_ppgtt; > int enable_psr; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_= gem.c > index 92b0b41..23a4700 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1002,6 +1002,17 @@ static bool can_wait_boost(struct drm_i915_file_pr= ivate *file_priv) > return !atomic_xchg(&file_priv->rps_wait_boost, true); > } > = > +static int intel_enable_rps_boost(struct drm_device *dev) > +{ > + /* No RPS Boost before Ironlake */ This comment is still wrong. I'd just drop it, everyone should know what the gen check below means. > + if (INTEL_INFO(dev)->gen < 6) > + return 0; > + > + /* Respect the kernel parameter if it is set */ This comment too seems rather obvious. I'd drop it as well. > + return i915.enable_rps_boost; > + > +} This function is still just a wrapper for i915.enable_rps_boost since __wait_seqno() already does the gen check. You could just check i915.enable_rps_boost directly in __wait_seqno(). The other option is to just drop the gen check from __wait_seqno() and just let this function take care of it. Hmm. Yeah that might be the nicest choice in fact. > + > /** > * __wait_seqno - wait until execution of seqno has finished > * @ring: the ring expected to report seqno > @@ -1042,8 +1053,11 @@ static int __wait_seqno(struct intel_ring_buffer *= ring, u32 seqno, > = > timeout_expire =3D timeout ? jiffies + timespec_to_jiffies_timeout(time= out) : 0; > = > - if (INTEL_INFO(dev)->gen >=3D 6 && can_wait_boost(file_priv)) { > + if (INTEL_INFO(dev)->gen >=3D 6 && can_wait_boost(file_priv) && = > + intel_enable_rps_boost(ring->dev)) { Indentation is quite wrong. There's also trailing whitespace around these parts. Please run patches through checkpatch.pl before submitting. > + = > gen6_rps_boost(dev_priv); > + > if (file_priv) > mod_delayed_work(dev_priv->wq, > &file_priv->mm.idle_work, > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i9= 15_params.c > index a66ffb6..2d207e3 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -34,6 +34,7 @@ struct i915_params i915 __read_mostly =3D { > .panel_use_ssc =3D -1, > .vbt_sdvo_panel_type =3D -1, > .enable_rc6 =3D -1, > + .enable_rps_boost =3D 1, true > .enable_fbc =3D -1, > .enable_hangcheck =3D true, > .enable_ppgtt =3D -1, > @@ -78,6 +79,10 @@ MODULE_PARM_DESC(enable_rc6, > "For example, 3 would enable rc6 and deep rc6, and 7 would enable every= thing. " > "default: -1 (use per-chip default)"); > = > +module_param_named(enable_rps_boost, i915.enable_rps_boost, int, 0600); bool > +MODULE_PARM_DESC(enable_rps_boost, > + "Enable/Disable boost RPS frequency (default: enabled (1))"); default: true > + > module_param_named(enable_fbc, i915.enable_fbc, int, 0600); > MODULE_PARM_DESC(enable_fbc, > "Enable frame buffer compression for power savings " > -- = > 1.8.4.2 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC