Re: [PATCH] drm/i915/chv: Use 16 and 32 for low and high drain latency precision.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 27, 2014 at 03:20:28PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 02:17:34PM +0200, Ville Syrjälä wrote:
> > On Wed, Oct 22, 2014 at 09:57:06AM +0300, Ville Syrjälä wrote:
> > > 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 updated,
> > > > these precision values brings stability and fixes some issues Wayne was facing.
> > > 
> > > Lies, damned lies, specs!
> > > 
> > > Well in this case I guess the spec might be correct since it helps
> > > Wayne.
> > > 
> > > > 
> > > > Cc: Wayne Boyer <wayne.boyer@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  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_crtc *crtc,
> > > >  				      int *prec_mult,
> > > >  				      int *drain_latency)
> > > >  {
> > > > +	struct drm_device *dev = crtc->dev;
> > > >  	int entries;
> > > >  	int clock = to_intel_crtc(crtc)->config.adjusted_mode.crtc_clock;
> > > >  
> > > > @@ -1355,8 +1356,12 @@ static bool vlv_compute_drain_latency(struct drm_crtc *crtc,
> > > >  		return false;
> > > >  
> > > >  	entries = DIV_ROUND_UP(clock, 1000) * pixel_size;
> > > > -	*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_64 :
> > > > -				       DRAIN_LATENCY_PRECISION_32;
> > > > +	if (IS_CHERRYVIEW(dev))
> > > > +		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_32 :
> > > > +					       DRAIN_LATENCY_PRECISION_16;
> > > > +	else
> > > > +		*prec_mult = (entries > 128) ? DRAIN_LATENCY_PRECISION_64 :
> > > > +					       DRAIN_LATENCY_PRECISION_32;
> > 
> > Actually let me take that r-b back a bit. That 128 needs to be changed for CHV
> > as well.
> 
> Well it's already backed into history, so no taking back. Bad enough to
> warrant a revert or fixup patch ok?

A fixup is perfectly fine here. With the error this way we just end up
not taking full advantage of the available 7 bits. So things should
still work just fine, albeit not exactly optimally.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx