From mboxrd@z Thu Jan 1 00:00:00 1970 From: "S, Deepak" Subject: Re: [PATCH v2 3/3] drm/i915: Add boot paramter to control rps boost at boot time. Date: Fri, 14 Mar 2014 00:16:37 +0530 Message-ID: <5321FD0D.1020706@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> <20140313181628.GO20292@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" 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 D52B9FA7FE for ; Thu, 13 Mar 2014 11:46:53 -0700 (PDT) In-Reply-To: <20140313181628.GO20292@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: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= , deepak.s@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 3/13/2014 11:46 PM, Ville Syrj=E4l=E4 wrote: > 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. I will address this. >> 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_p= rivate *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. Ok >> + 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. Ok >> + 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. Agreed. Does not make sense to have multiple platform check's. >> + >> /** >> * __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(ti= meout) : 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/i= 915_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 eve= rything. " >> "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 >