Re: [Intel-gfx] [PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Chris Wilson]

On Wed, Oct 05, 2016 at 06:44:04PM -0300, Paulo Zanoni wrote:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index dd15ae2..c580d3d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct
> > drm_plane *plane,
> >     struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > >base.crtc);
> >     struct drm_framebuffer *fb = plane_state->base.fb;
> >     const struct skl_wm_values *wm = _priv->wm.skl_results;
> > +   const struct skl_plane_wm *p_wm =
> > +   _state->wm.skl.optimal.planes[0];
> 
> I wish someone would do a patch to convert all these hardcoded values
> to PLANE_X, and convert "int"s  to "enum plane"s everywhere.

Note that this is not PLANE_A, but setting up a shorthand local for

const struct skl_plane_wm *p_wm = crtc_state->wm.skl.optimal.planes;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> Now that we've make skl_wm_levels make a little more sense, we can
> remove all of the redundant wm information. Up until now we'd been
> storing two copies of all of the skl watermarks: one being the
> skl_pipe_wm structs, the other being the global wm struct in
> drm_i915_private containing the raw register values. This is
> confusing
> and problematic, since it means we're prone to accidentally letting
> the
> two copies go out of sync. So, get rid of all of the functions
> responsible for computing the register values and just use a single
> helper, skl_write_wm_level(), to convert and write the new watermarks
> on
> the fly.

I like the direction of this patch too, but there are some small
possible problems. See below.


> 
> Signed-off-by: Lyude 
> Cc: Maarten Lankhorst 
> Cc: Ville Syrjälä 
> Cc: Matt Roper 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   2 -
>  drivers/gpu/drm/i915/intel_display.c |  14 ++-
>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c  | 203 -
> --
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>  5 files changed, 88 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 0f97d43..63519ac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1643,8 +1643,6 @@ struct skl_ddb_allocation {
>  struct skl_wm_values {
>   unsigned dirty_pipes;
>   struct skl_ddb_allocation ddb;
> - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
>  
>  struct skl_wm_level {
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index dd15ae2..c580d3d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>   struct drm_framebuffer *fb = plane_state->base.fb;
>   const struct skl_wm_values *wm = _priv->wm.skl_results;
> + const struct skl_plane_wm *p_wm =
> + _state->wm.skl.optimal.planes[0];

I wish someone would do a patch to convert all these hardcoded values
to PLANE_X, and convert "int"s  to "enum plane"s everywhere.


>   int pipe = intel_crtc->pipe;
>   u32 plane_ctl;
>   unsigned int rotation = plane_state->base.rotation;
> @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>   intel_crtc->adjusted_y = src_y;
>  
>   if (wm->dirty_pipes & drm_crtc_mask(_crtc->base))
> - skl_write_plane_wm(intel_crtc, wm, 0);
> + skl_write_plane_wm(intel_crtc, p_wm, >ddb, 0);
>  
>   I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>   I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> @@ -3448,6 +3450,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> + const struct skl_plane_wm *p_wm = 
> >wm.skl.optimal.planes[0];
>   int pipe = intel_crtc->pipe;
>  
>   /*
> @@ -3455,7 +3459,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>    * plane's visiblity isn't actually changing neither is its
> watermarks.
>    */
>   if (!crtc->primary->state->visible)
> - skl_write_plane_wm(intel_crtc, _priv-
> >wm.skl_results, 0);
> + skl_write_plane_wm(intel_crtc, p_wm,
> +    _priv->wm.skl_results.ddb,
> 0);
>  
>   I915_WRITE(PLANE_CTL(pipe, 0), 0);
>   I915_WRITE(PLANE_SURF(pipe, 0), 0);
> @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>   const struct skl_wm_values *wm = _priv->wm.skl_results;
> + const struct skl_plane_wm *p_wm =
> + >wm.skl.optimal.planes[PLANE_CURSOR];
>   int pipe = intel_crtc->pipe;
>   uint32_t cntl = 0;
>  
>   if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> - skl_write_cursor_wm(intel_crtc, wm);
> + skl_write_cursor_wm(intel_crtc, p_wm, >ddb);
>  
>   if (plane_state && plane_state->base.visible) {
>   cntl = MCURSOR_GAMMA_ENABLE;
> diff --git 

[Intel-gfx] [PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Lyude]

Now that we've make skl_wm_levels make a little more sense, we can
remove all of the redundant wm information. Up until now we'd been
storing two copies of all of the skl watermarks: one being the
skl_pipe_wm structs, the other being the global wm struct in
drm_i915_private containing the raw register values. This is confusing
and problematic, since it means we're prone to accidentally letting the
two copies go out of sync. So, get rid of all of the functions
responsible for computing the register values and just use a single
helper, skl_write_wm_level(), to convert and write the new watermarks on
the fly.

Signed-off-by: Lyude 
Cc: Maarten Lankhorst 
Cc: Ville Syrjälä 
Cc: Matt Roper 
---
 drivers/gpu/drm/i915/i915_drv.h  |   2 -
 drivers/gpu/drm/i915/intel_display.c |  14 ++-
 drivers/gpu/drm/i915/intel_drv.h |   6 +-
 drivers/gpu/drm/i915/intel_pm.c  | 203 ---
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 88 insertions(+), 145 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f97d43..63519ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1643,8 +1643,6 @@ struct skl_ddb_allocation {
 struct skl_wm_values {
unsigned dirty_pipes;
struct skl_ddb_allocation ddb;
-   uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
-   uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
 };
 
 struct skl_wm_level {
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index dd15ae2..c580d3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane 
*plane,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
struct drm_framebuffer *fb = plane_state->base.fb;
const struct skl_wm_values *wm = _priv->wm.skl_results;
+   const struct skl_plane_wm *p_wm =
+   _state->wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
u32 plane_ctl;
unsigned int rotation = plane_state->base.rotation;
@@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct drm_plane 
*plane,
intel_crtc->adjusted_y = src_y;
 
if (wm->dirty_pipes & drm_crtc_mask(_crtc->base))
-   skl_write_plane_wm(intel_crtc, wm, 0);
+   skl_write_plane_wm(intel_crtc, p_wm, >ddb, 0);
 
I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
@@ -3448,6 +3450,8 @@ static void skylake_disable_primary_plane(struct 
drm_plane *primary,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+   const struct skl_plane_wm *p_wm = >wm.skl.optimal.planes[0];
int pipe = intel_crtc->pipe;
 
/*
@@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct 
drm_plane *primary,
 * plane's visiblity isn't actually changing neither is its watermarks.
 */
if (!crtc->primary->state->visible)
-   skl_write_plane_wm(intel_crtc, _priv->wm.skl_results, 0);
+   skl_write_plane_wm(intel_crtc, p_wm,
+  _priv->wm.skl_results.ddb, 0);
 
I915_WRITE(PLANE_CTL(pipe, 0), 0);
I915_WRITE(PLANE_SURF(pipe, 0), 0);
@@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, 
u32 base,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
const struct skl_wm_values *wm = _priv->wm.skl_results;
+   const struct skl_plane_wm *p_wm =
+   >wm.skl.optimal.planes[PLANE_CURSOR];
int pipe = intel_crtc->pipe;
uint32_t cntl = 0;
 
if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-   skl_write_cursor_wm(intel_crtc, wm);
+   skl_write_cursor_wm(intel_crtc, p_wm, >ddb);
 
if (plane_state && plane_state->base.visible) {
cntl = MCURSOR_GAMMA_ENABLE;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d684f4f..958dc72 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct 
skl_ddb_allocation *old,
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 struct intel_crtc *intel_crtc);
 void