From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915/chv: Use 16 and 32 for low and high drain latency precision. Date: Wed, 22 Oct 2014 09:57:06 +0300 Message-ID: <20141022065706.GO4284@intel.com> References: <1413558308-1669-1-git-send-email-rodrigo.vivi@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 E084089951 for ; Tue, 21 Oct 2014 23:57:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1413558308-1669-1-git-send-email-rodrigo.vivi@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Rodrigo Vivi Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Oct 17, 2014 at 08:05:08AM -0700, Rodrigo Vivi wrote: > Current chv spec teels we can only use either 16 or 32 bits as precision. > = > Although in the past VLV went from 16/32 to 32/64 and spec might not be u= pdated, > these precision values brings stability and fixes some issues Wayne was f= acing. Lies, damned lies, specs! Well in this case I guess the spec might be correct since it helps Wayne. > = > Cc: Wayne Boyer > Cc: Ville Syrj=E4l=E4 > Signed-off-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/i915_reg.h | 13 +++++++------ > drivers/gpu/drm/i915/intel_pm.c | 40 +++++++++++++++++++++++++----------= ----- > 2 files changed, 32 insertions(+), 21 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_= reg.h > index 6db369a..dcd5650 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4054,17 +4054,18 @@ enum punit_power_well { > #define DSPFW_PLANEA_WM1_HI_MASK (1<<0) > = > /* drain latency register values*/ > +#define DRAIN_LATENCY_PRECISION_16 16 > #define DRAIN_LATENCY_PRECISION_32 32 > #define DRAIN_LATENCY_PRECISION_64 64 While you're poking at this stuff, could I trouble you to remove these defines and just use the values directly? Could be a separate patch if you prefer. > #define VLV_DDL(pipe) (VLV_DISPLAY_BASE + 0x70050 + 4 * (pipe)) > -#define DDL_CURSOR_PRECISION_64 (1<<31) > -#define DDL_CURSOR_PRECISION_32 (0<<31) > +#define DDL_CURSOR_PRECISION_HIGH (1<<31) > +#define DDL_CURSOR_PRECISION_LOW (0<<31) > #define DDL_CURSOR_SHIFT 24 > -#define DDL_SPRITE_PRECISION_64(sprite) (1<<(15+8*(sprite))) > -#define DDL_SPRITE_PRECISION_32(sprite) (0<<(15+8*(sprite))) > +#define DDL_SPRITE_PRECISION_HIGH(sprite) (1<<(15+8*(sprite))) > +#define DDL_SPRITE_PRECISION_LOW(sprite) (0<<(15+8*(sprite))) > #define DDL_SPRITE_SHIFT(sprite) (8+8*(sprite)) > -#define DDL_PLANE_PRECISION_64 (1<<7) > -#define DDL_PLANE_PRECISION_32 (0<<7) > +#define DDL_PLANE_PRECISION_HIGH (1<<7) > +#define DDL_PLANE_PRECISION_LOW (0<<7) > #define DDL_PLANE_SHIFT 0 > #define DRAIN_LATENCY_MASK 0x7f > = > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index daa99e7..50c3512 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1345,6 +1345,7 @@ static bool vlv_compute_drain_latency(struct drm_cr= tc *crtc, > int *prec_mult, > int *drain_latency) > { > + struct drm_device *dev =3D crtc->dev; > int entries; > int clock =3D to_intel_crtc(crtc)->config.adjusted_mode.crtc_clock; > = > @@ -1355,8 +1356,12 @@ static bool vlv_compute_drain_latency(struct drm_c= rtc *crtc, > return false; > = > entries =3D DIV_ROUND_UP(clock, 1000) * pixel_size; > - *prec_mult =3D (entries > 128) ? DRAIN_LATENCY_PRECISION_64 : > - DRAIN_LATENCY_PRECISION_32; > + if (IS_CHERRYVIEW(dev)) > + *prec_mult =3D (entries > 128) ? DRAIN_LATENCY_PRECISION_32 : > + DRAIN_LATENCY_PRECISION_16; > + else > + *prec_mult =3D (entries > 128) ? DRAIN_LATENCY_PRECISION_64 : > + DRAIN_LATENCY_PRECISION_32; > *drain_latency =3D (64 * (*prec_mult) * 4) / entries; > = > if (*drain_latency > DRAIN_LATENCY_MASK) > @@ -1375,15 +1380,18 @@ static bool vlv_compute_drain_latency(struct drm_= crtc *crtc, > = > static void vlv_update_drain_latency(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv =3D crtc->dev->dev_private; > + struct drm_device *dev =3D crtc->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > int pixel_size; > int drain_latency; > enum pipe pipe =3D intel_crtc->pipe; > int plane_prec, prec_mult, plane_dl; > + int high_precision =3D IS_CHERRYVIEW(dev) ? DRAIN_LATENCY_PRECISION_32 : > + DRAIN_LATENCY_PRECISION_64; Maybe const just help the reader remember that it's a constant. > = > - plane_dl =3D I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_64 | > - DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_64 | > + plane_dl =3D I915_READ(VLV_DDL(pipe)) & ~(DDL_PLANE_PRECISION_HIGH | > + DRAIN_LATENCY_MASK | DDL_CURSOR_PRECISION_HIGH | > (DRAIN_LATENCY_MASK << DDL_CURSOR_SHIFT)); > = > if (!intel_crtc_active(crtc)) { > @@ -1394,9 +1402,9 @@ static void vlv_update_drain_latency(struct drm_crt= c *crtc) > /* Primary plane Drain Latency */ > pixel_size =3D crtc->primary->fb->bits_per_pixel / 8; /* BPP */ > if (vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_late= ncy)) { > - plane_prec =3D (prec_mult =3D=3D DRAIN_LATENCY_PRECISION_64) ? > - DDL_PLANE_PRECISION_64 : > - DDL_PLANE_PRECISION_32; > + plane_prec =3D (prec_mult =3D=3D high_precision) ? > + DDL_PLANE_PRECISION_HIGH : > + DDL_PLANE_PRECISION_LOW; > plane_dl |=3D plane_prec | drain_latency; > } > = > @@ -1408,9 +1416,9 @@ static void vlv_update_drain_latency(struct drm_crt= c *crtc) > /* Program cursor DL only if it is enabled */ > if (intel_crtc->cursor_base && > vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_late= ncy)) { > - plane_prec =3D (prec_mult =3D=3D DRAIN_LATENCY_PRECISION_64) ? > - DDL_CURSOR_PRECISION_64 : > - DDL_CURSOR_PRECISION_32; > + plane_prec =3D (prec_mult =3D=3D high_precision) ? > + DDL_CURSOR_PRECISION_HIGH : > + DDL_CURSOR_PRECISION_LOW; > plane_dl |=3D plane_prec | (drain_latency << DDL_CURSOR_SHIFT); > } > = > @@ -1578,15 +1586,17 @@ static void valleyview_update_sprite_wm(struct dr= m_plane *plane, > int plane_prec; > int sprite_dl; > int prec_mult; > + int high_precision =3D IS_CHERRYVIEW(dev) ? DRAIN_LATENCY_PRECISION_32 : > + DRAIN_LATENCY_PRECISION_64; Could also be const. Otherwise looks good, so: Reviewed-by: Ville Syrj=E4l=E4 > = > - sprite_dl =3D I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_64(spri= te) | > + sprite_dl =3D I915_READ(VLV_DDL(pipe)) & ~(DDL_SPRITE_PRECISION_HIGH(sp= rite) | > (DRAIN_LATENCY_MASK << DDL_SPRITE_SHIFT(sprite))); > = > if (enabled && vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, > &drain_latency)) { > - plane_prec =3D (prec_mult =3D=3D DRAIN_LATENCY_PRECISION_64) ? > - DDL_SPRITE_PRECISION_64(sprite) : > - DDL_SPRITE_PRECISION_32(sprite); > + plane_prec =3D (prec_mult =3D=3D high_precision) ? > + DDL_SPRITE_PRECISION_HIGH(sprite) : > + DDL_SPRITE_PRECISION_LOW(sprite); > sprite_dl |=3D plane_prec | > (drain_latency << DDL_SPRITE_SHIFT(sprite)); > } > -- = > 1.9.3 -- = Ville Syrj=E4l=E4 Intel OTC