Re: [Intel-gfx] [PATCH 12/15] drm/i915: Two stage watermarks for g4x

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 09:34:42AM +0200, Maarten Lankhorst wrote:
> On 21-04-17 20:14, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > Implement proper two stage watermark programming for g4x. As with
> > other pre-SKL platforms, the watermark registers aren't double
> > buffered on g4x. Hence we must sequence the watermark update
> > carefully around plane updates.
> >
> > The code is quite heavily modelled on the VLV/CHV code, with some
> > fairly significant differences due to the different hardware
> > architecture:
> > * g4x doesn't use inverted watermark values
> > * CxSR actually affects the watermarks since it controls memory self
> >   refresh in addition to the max FIFO mode
> > * A further HPLL SR mode is possible with higher memory wakeup
> >   latency
> > * g4x has FBC2 and so it also has FBC watermarks
> > * max FIFO mode for primary plane only (cursor is allowed, sprite is not)
> > * g4x has no manual FIFO repartitioning
> > * some TLB miss related workarounds are needed for the watermarks
> >
> > Actually the hardware is quite similar to ILK+ in many ways. The
> > most visible differences are in the actual watermakr register
> > layout. ILK revamped that part quite heavily whereas g4x is still
> > using the layout inherited from earlier platforms.
> >
> > Note that we didn't previously enable the HPLL SR on g4x. So in order
> > to not introduce too many functional changes in this patch I've not
> > actually enabled it here either, even though the code is now fully
> > ready for it. We'll enable it separately later on.
> >
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  12 +-
> >  drivers/gpu/drm/i915/i915_drv.h  |  12 +
> >  drivers/gpu/drm/i915/intel_display.c |  25 +-
> >  drivers/gpu/drm/i915/intel_drv.h |  28 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 942 
> > +++
> >  5 files changed, 792 insertions(+), 227 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 870c470177b5..69550d31099e 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3898,6 +3898,8 @@ static void wm_latency_show(struct seq_file *m, const 
> > uint16_t wm[8])
> > num_levels = 3;
> > else if (IS_VALLEYVIEW(dev_priv))
> > num_levels = 1;
> > +   else if (IS_G4X(dev_priv))
> > +   num_levels = 3;
> > else
> > num_levels = ilk_wm_max_level(dev_priv) + 1;
> >  
> > @@ -3910,8 +3912,10 @@ static void wm_latency_show(struct seq_file *m, 
> > const uint16_t wm[8])
> >  * - WM1+ latency values in 0.5us units
> >  * - latencies are in us on gen9/vlv/chv
> >  */
> > -   if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) ||
> > -   IS_CHERRYVIEW(dev_priv))
> > +   if (INTEL_GEN(dev_priv) >= 9 ||
> > +   IS_VALLEYVIEW(dev_priv) ||
> > +   IS_CHERRYVIEW(dev_priv) ||
> > +   IS_G4X(dev_priv))
> > latency *= 10;
> > else if (level > 0)
> > latency *= 5;
> > @@ -3972,7 +3976,7 @@ static int pri_wm_latency_open(struct inode *inode, 
> > struct file *file)
> >  {
> > struct drm_i915_private *dev_priv = inode->i_private;
> >  
> > -   if (INTEL_GEN(dev_priv) < 5)
> > +   if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> > return -ENODEV;
> >  
> > return single_open(file, pri_wm_latency_show, dev_priv);
> > @@ -4014,6 +4018,8 @@ static ssize_t wm_latency_write(struct file *file, 
> > const char __user *ubuf,
> > num_levels = 3;
> > else if (IS_VALLEYVIEW(dev_priv))
> > num_levels = 1;
> > +   else if (IS_G4X(dev_priv))
> > +   num_levels = 3;
> > else
> > num_levels = ilk_wm_max_level(dev_priv) + 1;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 0a393fdc53d1..6df8bff7f5a7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1762,11 +1762,13 @@ struct ilk_wm_values {
> >  
> >  struct g4x_pipe_wm {
> > uint16_t plane[I915_MAX_PLANES];
> > +   uint16_t fbc;
> >  };
> >  
> >  struct g4x_sr_wm {
> > uint16_t plane;
> > uint16_t cursor;
> > +   uint16_t fbc;
> >  };
> >  
> >  struct vlv_wm_ddl_values {
> > @@ -1781,6 +1783,15 @@ struct vlv_wm_values {
> > bool cxsr;
> >  };
> >  
> > +struct g4x_wm_values {
> > +   struct g4x_pipe_wm pipe[2];
> > +   struct g4x_sr_wm sr;
> > +   struct g4x_sr_wm hpll;
> > +   bool cxsr;
> > +   bool hpll_en;
> > +   bool fbc_en;
> > +};
> > +
> >  struct skl_ddb_entry {
> > uint16_t start, end;/* in number of blocks, 'end' is exclusive */
> >  };
> > @@ -2410,6 +2421,7 @@ struct drm_i915_private {
> >

Re: [Intel-gfx] [PATCH 12/15] drm/i915: Two stage watermarks for g4x

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 09:34:42AM +0200, Maarten Lankhorst wrote:
> On 21-04-17 20:14, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > Implement proper two stage watermark programming for g4x. As with
> > other pre-SKL platforms, the watermark registers aren't double
> > buffered on g4x. Hence we must sequence the watermark update
> > carefully around plane updates.
> >
> > The code is quite heavily modelled on the VLV/CHV code, with some
> > fairly significant differences due to the different hardware
> > architecture:
> > * g4x doesn't use inverted watermark values
> > * CxSR actually affects the watermarks since it controls memory self
> >   refresh in addition to the max FIFO mode
> > * A further HPLL SR mode is possible with higher memory wakeup
> >   latency
> > * g4x has FBC2 and so it also has FBC watermarks
> > * max FIFO mode for primary plane only (cursor is allowed, sprite is not)
> > * g4x has no manual FIFO repartitioning
> > * some TLB miss related workarounds are needed for the watermarks
> >
> > Actually the hardware is quite similar to ILK+ in many ways. The
> > most visible differences are in the actual watermakr register
> > layout. ILK revamped that part quite heavily whereas g4x is still
> > using the layout inherited from earlier platforms.
> >
> > Note that we didn't previously enable the HPLL SR on g4x. So in order
> > to not introduce too many functional changes in this patch I've not
> > actually enabled it here either, even though the code is now fully
> > ready for it. We'll enable it separately later on.
> >
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  12 +-
> >  drivers/gpu/drm/i915/i915_drv.h  |  12 +
> >  drivers/gpu/drm/i915/intel_display.c |  25 +-
> >  drivers/gpu/drm/i915/intel_drv.h |  28 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 942 
> > +++
> >  5 files changed, 792 insertions(+), 227 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 870c470177b5..69550d31099e 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3898,6 +3898,8 @@ static void wm_latency_show(struct seq_file *m, const 
> > uint16_t wm[8])
> > num_levels = 3;
> > else if (IS_VALLEYVIEW(dev_priv))
> > num_levels = 1;
> > +   else if (IS_G4X(dev_priv))
> > +   num_levels = 3;
> > else
> > num_levels = ilk_wm_max_level(dev_priv) + 1;
> >  
> > @@ -3910,8 +3912,10 @@ static void wm_latency_show(struct seq_file *m, 
> > const uint16_t wm[8])
> >  * - WM1+ latency values in 0.5us units
> >  * - latencies are in us on gen9/vlv/chv
> >  */
> > -   if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) ||
> > -   IS_CHERRYVIEW(dev_priv))
> > +   if (INTEL_GEN(dev_priv) >= 9 ||
> > +   IS_VALLEYVIEW(dev_priv) ||
> > +   IS_CHERRYVIEW(dev_priv) ||
> > +   IS_G4X(dev_priv))
> > latency *= 10;
> > else if (level > 0)
> > latency *= 5;
> > @@ -3972,7 +3976,7 @@ static int pri_wm_latency_open(struct inode *inode, 
> > struct file *file)
> >  {
> > struct drm_i915_private *dev_priv = inode->i_private;
> >  
> > -   if (INTEL_GEN(dev_priv) < 5)
> > +   if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> > return -ENODEV;
> >  
> > return single_open(file, pri_wm_latency_show, dev_priv);
> > @@ -4014,6 +4018,8 @@ static ssize_t wm_latency_write(struct file *file, 
> > const char __user *ubuf,
> > num_levels = 3;
> > else if (IS_VALLEYVIEW(dev_priv))
> > num_levels = 1;
> > +   else if (IS_G4X(dev_priv))
> > +   num_levels = 3;
> > else
> > num_levels = ilk_wm_max_level(dev_priv) + 1;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 0a393fdc53d1..6df8bff7f5a7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1762,11 +1762,13 @@ struct ilk_wm_values {
> >  
> >  struct g4x_pipe_wm {
> > uint16_t plane[I915_MAX_PLANES];
> > +   uint16_t fbc;
> >  };
> >  
> >  struct g4x_sr_wm {
> > uint16_t plane;
> > uint16_t cursor;
> > +   uint16_t fbc;
> >  };
> >  
> >  struct vlv_wm_ddl_values {
> > @@ -1781,6 +1783,15 @@ struct vlv_wm_values {
> > bool cxsr;
> >  };
> >  
> > +struct g4x_wm_values {
> > +   struct g4x_pipe_wm pipe[2];
> > +   struct g4x_sr_wm sr;
> > +   struct g4x_sr_wm hpll;
> > +   bool cxsr;
> > +   bool hpll_en;
> > +   bool fbc_en;
> > +};
> > +
> >  struct skl_ddb_entry {
> > uint16_t start, end;/* in number of blocks, 'end' is exclusive */
> >  };
> > @@ -2410,6 +2421,7 @@ struct drm_i915_private {
> >

Re: [Intel-gfx] [PATCH 12/15] drm/i915: Two stage watermarks for g4x

[Maarten Lankhorst]

On 21-04-17 20:14, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
>
> Implement proper two stage watermark programming for g4x. As with
> other pre-SKL platforms, the watermark registers aren't double
> buffered on g4x. Hence we must sequence the watermark update
> carefully around plane updates.
>
> The code is quite heavily modelled on the VLV/CHV code, with some
> fairly significant differences due to the different hardware
> architecture:
> * g4x doesn't use inverted watermark values
> * CxSR actually affects the watermarks since it controls memory self
>   refresh in addition to the max FIFO mode
> * A further HPLL SR mode is possible with higher memory wakeup
>   latency
> * g4x has FBC2 and so it also has FBC watermarks
> * max FIFO mode for primary plane only (cursor is allowed, sprite is not)
> * g4x has no manual FIFO repartitioning
> * some TLB miss related workarounds are needed for the watermarks
>
> Actually the hardware is quite similar to ILK+ in many ways. The
> most visible differences are in the actual watermakr register
> layout. ILK revamped that part quite heavily whereas g4x is still
> using the layout inherited from earlier platforms.
>
> Note that we didn't previously enable the HPLL SR on g4x. So in order
> to not introduce too many functional changes in this patch I've not
> actually enabled it here either, even though the code is now fully
> ready for it. We'll enable it separately later on.
>
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  12 +-
>  drivers/gpu/drm/i915/i915_drv.h  |  12 +
>  drivers/gpu/drm/i915/intel_display.c |  25 +-
>  drivers/gpu/drm/i915/intel_drv.h |  28 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 942 
> +++
>  5 files changed, 792 insertions(+), 227 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 870c470177b5..69550d31099e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3898,6 +3898,8 @@ static void wm_latency_show(struct seq_file *m, const 
> uint16_t wm[8])
>   num_levels = 3;
>   else if (IS_VALLEYVIEW(dev_priv))
>   num_levels = 1;
> + else if (IS_G4X(dev_priv))
> + num_levels = 3;
>   else
>   num_levels = ilk_wm_max_level(dev_priv) + 1;
>  
> @@ -3910,8 +3912,10 @@ static void wm_latency_show(struct seq_file *m, const 
> uint16_t wm[8])
>* - WM1+ latency values in 0.5us units
>* - latencies are in us on gen9/vlv/chv
>*/
> - if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) ||
> - IS_CHERRYVIEW(dev_priv))
> + if (INTEL_GEN(dev_priv) >= 9 ||
> + IS_VALLEYVIEW(dev_priv) ||
> + IS_CHERRYVIEW(dev_priv) ||
> + IS_G4X(dev_priv))
>   latency *= 10;
>   else if (level > 0)
>   latency *= 5;
> @@ -3972,7 +3976,7 @@ static int pri_wm_latency_open(struct inode *inode, 
> struct file *file)
>  {
>   struct drm_i915_private *dev_priv = inode->i_private;
>  
> - if (INTEL_GEN(dev_priv) < 5)
> + if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
>   return -ENODEV;
>  
>   return single_open(file, pri_wm_latency_show, dev_priv);
> @@ -4014,6 +4018,8 @@ static ssize_t wm_latency_write(struct file *file, 
> const char __user *ubuf,
>   num_levels = 3;
>   else if (IS_VALLEYVIEW(dev_priv))
>   num_levels = 1;
> + else if (IS_G4X(dev_priv))
> + num_levels = 3;
>   else
>   num_levels = ilk_wm_max_level(dev_priv) + 1;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0a393fdc53d1..6df8bff7f5a7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1762,11 +1762,13 @@ struct ilk_wm_values {
>  
>  struct g4x_pipe_wm {
>   uint16_t plane[I915_MAX_PLANES];
> + uint16_t fbc;
>  };
>  
>  struct g4x_sr_wm {
>   uint16_t plane;
>   uint16_t cursor;
> + uint16_t fbc;
>  };
>  
>  struct vlv_wm_ddl_values {
> @@ -1781,6 +1783,15 @@ struct vlv_wm_values {
>   bool cxsr;
>  };
>  
> +struct g4x_wm_values {
> + struct g4x_pipe_wm pipe[2];
> + struct g4x_sr_wm sr;
> + struct g4x_sr_wm hpll;
> + bool cxsr;
> + bool hpll_en;
> + bool fbc_en;
> +};
> +
>  struct skl_ddb_entry {
>   uint16_t start, end;/* in number of blocks, 'end' is exclusive */
>  };
> @@ -2410,6 +2421,7 @@ struct drm_i915_private {
>   struct ilk_wm_values hw;
>   struct skl_wm_values skl_hw;
>   struct vlv_wm_values vlv;
> + struct g4x_wm_values g4x;
>   };
>  
>   uint8_t 

[Intel-gfx] [PATCH 12/15] drm/i915: Two stage watermarks for g4x

[ville . syrjala]

From: Ville Syrjälä 

Implement proper two stage watermark programming for g4x. As with
other pre-SKL platforms, the watermark registers aren't double
buffered on g4x. Hence we must sequence the watermark update
carefully around plane updates.

The code is quite heavily modelled on the VLV/CHV code, with some
fairly significant differences due to the different hardware
architecture:
* g4x doesn't use inverted watermark values
* CxSR actually affects the watermarks since it controls memory self
  refresh in addition to the max FIFO mode
* A further HPLL SR mode is possible with higher memory wakeup
  latency
* g4x has FBC2 and so it also has FBC watermarks
* max FIFO mode for primary plane only (cursor is allowed, sprite is not)
* g4x has no manual FIFO repartitioning
* some TLB miss related workarounds are needed for the watermarks

Actually the hardware is quite similar to ILK+ in many ways. The
most visible differences are in the actual watermakr register
layout. ILK revamped that part quite heavily whereas g4x is still
using the layout inherited from earlier platforms.

Note that we didn't previously enable the HPLL SR on g4x. So in order
to not introduce too many functional changes in this patch I've not
actually enabled it here either, even though the code is now fully
ready for it. We'll enable it separately later on.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  12 +-
 drivers/gpu/drm/i915/i915_drv.h  |  12 +
 drivers/gpu/drm/i915/intel_display.c |  25 +-
 drivers/gpu/drm/i915/intel_drv.h |  28 ++
 drivers/gpu/drm/i915/intel_pm.c  | 942 +++
 5 files changed, 792 insertions(+), 227 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 870c470177b5..69550d31099e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3898,6 +3898,8 @@ static void wm_latency_show(struct seq_file *m, const 
uint16_t wm[8])
num_levels = 3;
else if (IS_VALLEYVIEW(dev_priv))
num_levels = 1;
+   else if (IS_G4X(dev_priv))
+   num_levels = 3;
else
num_levels = ilk_wm_max_level(dev_priv) + 1;
 
@@ -3910,8 +3912,10 @@ static void wm_latency_show(struct seq_file *m, const 
uint16_t wm[8])
 * - WM1+ latency values in 0.5us units
 * - latencies are in us on gen9/vlv/chv
 */
-   if (INTEL_GEN(dev_priv) >= 9 || IS_VALLEYVIEW(dev_priv) ||
-   IS_CHERRYVIEW(dev_priv))
+   if (INTEL_GEN(dev_priv) >= 9 ||
+   IS_VALLEYVIEW(dev_priv) ||
+   IS_CHERRYVIEW(dev_priv) ||
+   IS_G4X(dev_priv))
latency *= 10;
else if (level > 0)
latency *= 5;
@@ -3972,7 +3976,7 @@ static int pri_wm_latency_open(struct inode *inode, 
struct file *file)
 {
struct drm_i915_private *dev_priv = inode->i_private;
 
-   if (INTEL_GEN(dev_priv) < 5)
+   if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
return -ENODEV;
 
return single_open(file, pri_wm_latency_show, dev_priv);
@@ -4014,6 +4018,8 @@ static ssize_t wm_latency_write(struct file *file, const 
char __user *ubuf,
num_levels = 3;
else if (IS_VALLEYVIEW(dev_priv))
num_levels = 1;
+   else if (IS_G4X(dev_priv))
+   num_levels = 3;
else
num_levels = ilk_wm_max_level(dev_priv) + 1;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0a393fdc53d1..6df8bff7f5a7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1762,11 +1762,13 @@ struct ilk_wm_values {
 
 struct g4x_pipe_wm {
uint16_t plane[I915_MAX_PLANES];
+   uint16_t fbc;
 };
 
 struct g4x_sr_wm {
uint16_t plane;
uint16_t cursor;
+   uint16_t fbc;
 };
 
 struct vlv_wm_ddl_values {
@@ -1781,6 +1783,15 @@ struct vlv_wm_values {
bool cxsr;
 };
 
+struct g4x_wm_values {
+   struct g4x_pipe_wm pipe[2];
+   struct g4x_sr_wm sr;
+   struct g4x_sr_wm hpll;
+   bool cxsr;
+   bool hpll_en;
+   bool fbc_en;
+};
+
 struct skl_ddb_entry {
uint16_t start, end;/* in number of blocks, 'end' is exclusive */
 };
@@ -2410,6 +2421,7 @@ struct drm_i915_private {
struct ilk_wm_values hw;
struct skl_wm_values skl_hw;
struct vlv_wm_values vlv;
+   struct g4x_wm_values g4x;
};
 
uint8_t max_level;
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 85b9e2f521a0..0f42263c3f76 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++