Re: [Intel-gfx] [PATCH 04/15] drm/i915: Clean up cursor junk from intel_crtc

[Imre Deak]

On Mon, Mar 27, 2017 at 09:55:35PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Move cursor_base, cursor_cntl, and cursor_size from intel_crtc
> into intel_plane so that we don't need the crtc for cursor stuff
> so much.
> 
> Also entirely nuke cursor_addr which IMO doesn't provide any benefit
> since it's not actually used by the cursor code itself. I'm not 100%
> sure what the SKL+ DDB is code is after by looking at cursor_addr so
> I just make it do its checks unconditionally. If that's not correct
> then we should likely replace it with somehting like
> plane_state->visible.

Yes, AFAICS in case it's not visible the cursor DDB and WM will be still
computed (to fixed minimum and 0 accordingly) and programmed. Maybe
historical left-over? The code comment about this is also stale then.

> 
> Signed-off-by: Ville Syrjälä 

Reviewed-by: Imre Deak 

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 48 +-
>  drivers/gpu/drm/i915/intel_display.c | 80 
> +++-
>  drivers/gpu/drm/i915/intel_drv.h |  9 ++--
>  3 files changed, 48 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 316bc47a8eea..b9410cb845f3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3040,36 +3040,6 @@ static void intel_connector_info(struct seq_file *m,
>   intel_seq_print_mode(m, 2, mode);
>  }
>  
> -static bool cursor_active(struct drm_i915_private *dev_priv, int pipe)
> -{
> - u32 state;
> -
> - if (IS_I845G(dev_priv) || IS_I865G(dev_priv))
> - state = I915_READ(CURCNTR(PIPE_A)) & CURSOR_ENABLE;
> - else
> - state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
> -
> - return state;
> -}
> -
> -static bool cursor_position(struct drm_i915_private *dev_priv,
> - int pipe, int *x, int *y)
> -{
> - u32 pos;
> -
> - pos = I915_READ(CURPOS(pipe));
> -
> - *x = (pos >> CURSOR_X_SHIFT) & CURSOR_POS_MASK;
> - if (pos & (CURSOR_POS_SIGN << CURSOR_X_SHIFT))
> - *x = -*x;
> -
> - *y = (pos >> CURSOR_Y_SHIFT) & CURSOR_POS_MASK;
> - if (pos & (CURSOR_POS_SIGN << CURSOR_Y_SHIFT))
> - *y = -*y;
> -
> - return cursor_active(dev_priv, pipe);
> -}
> -
>  static const char *plane_type(enum drm_plane_type type)
>  {
>   switch (type) {
> @@ -3191,9 +3161,7 @@ static int i915_display_info(struct seq_file *m, void 
> *unused)
>   seq_printf(m, "CRTC info\n");
>   seq_printf(m, "-\n");
>   for_each_intel_crtc(dev, crtc) {
> - bool active;
>   struct intel_crtc_state *pipe_config;
> - int x, y;
>  
>   drm_modeset_lock(>base.mutex, NULL);
>   pipe_config = to_intel_crtc_state(crtc->base.state);
> @@ -3205,14 +3173,18 @@ static int i915_display_info(struct seq_file *m, void 
> *unused)
>  yesno(pipe_config->dither), pipe_config->pipe_bpp);
>  
>   if (pipe_config->base.active) {
> + struct intel_plane *cursor =
> + to_intel_plane(crtc->base.cursor);
> +
>   intel_crtc_info(m, crtc);
>  
> - active = cursor_position(dev_priv, crtc->pipe, , );
> - seq_printf(m, "\tcursor visible? %s, position (%d, %d), 
> size %dx%d, addr 0x%08x, active? %s\n",
> -yesno(crtc->cursor_base),
> -x, y, crtc->base.cursor->state->crtc_w,
> -crtc->base.cursor->state->crtc_h,
> -crtc->cursor_addr, yesno(active));
> + seq_printf(m, "\tcursor visible? %s, position (%d, %d), 
> size %dx%d, addr 0x%08x\n",
> +yesno(cursor->base.state->visible),
> +cursor->base.state->crtc_x,
> +cursor->base.state->crtc_y,
> +cursor->base.state->crtc_w,
> +cursor->base.state->crtc_h,
> +cursor->cursor.base);
>   intel_scaler_info(m, crtc);
>   intel_plane_info(m, crtc);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 5e04f64a0f76..1d55fac397ad 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9126,8 +9126,7 @@ static bool haswell_get_pipe_config(struct intel_crtc 
> *crtc,
>   return active;
>  }
>  
> -static u32 intel_cursor_base(struct intel_crtc *crtc,
> -  const struct intel_plane_state *plane_state)
> +static u32 intel_cursor_base(const struct 

[Intel-gfx] [PATCH 04/15] drm/i915: Clean up cursor junk from intel_crtc

[ville . syrjala]

From: Ville Syrjälä 

Move cursor_base, cursor_cntl, and cursor_size from intel_crtc
into intel_plane so that we don't need the crtc for cursor stuff
so much.

Also entirely nuke cursor_addr which IMO doesn't provide any benefit
since it's not actually used by the cursor code itself. I'm not 100%
sure what the SKL+ DDB is code is after by looking at cursor_addr so
I just make it do its checks unconditionally. If that's not correct
then we should likely replace it with somehting like
plane_state->visible.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 48 +-
 drivers/gpu/drm/i915/intel_display.c | 80 +++-
 drivers/gpu/drm/i915/intel_drv.h |  9 ++--
 3 files changed, 48 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 316bc47a8eea..b9410cb845f3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3040,36 +3040,6 @@ static void intel_connector_info(struct seq_file *m,
intel_seq_print_mode(m, 2, mode);
 }
 
-static bool cursor_active(struct drm_i915_private *dev_priv, int pipe)
-{
-   u32 state;
-
-   if (IS_I845G(dev_priv) || IS_I865G(dev_priv))
-   state = I915_READ(CURCNTR(PIPE_A)) & CURSOR_ENABLE;
-   else
-   state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
-
-   return state;
-}
-
-static bool cursor_position(struct drm_i915_private *dev_priv,
-   int pipe, int *x, int *y)
-{
-   u32 pos;
-
-   pos = I915_READ(CURPOS(pipe));
-
-   *x = (pos >> CURSOR_X_SHIFT) & CURSOR_POS_MASK;
-   if (pos & (CURSOR_POS_SIGN << CURSOR_X_SHIFT))
-   *x = -*x;
-
-   *y = (pos >> CURSOR_Y_SHIFT) & CURSOR_POS_MASK;
-   if (pos & (CURSOR_POS_SIGN << CURSOR_Y_SHIFT))
-   *y = -*y;
-
-   return cursor_active(dev_priv, pipe);
-}
-
 static const char *plane_type(enum drm_plane_type type)
 {
switch (type) {
@@ -3191,9 +3161,7 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
seq_printf(m, "CRTC info\n");
seq_printf(m, "-\n");
for_each_intel_crtc(dev, crtc) {
-   bool active;
struct intel_crtc_state *pipe_config;
-   int x, y;
 
drm_modeset_lock(>base.mutex, NULL);
pipe_config = to_intel_crtc_state(crtc->base.state);
@@ -3205,14 +3173,18 @@ static int i915_display_info(struct seq_file *m, void 
*unused)
   yesno(pipe_config->dither), pipe_config->pipe_bpp);
 
if (pipe_config->base.active) {
+   struct intel_plane *cursor =
+   to_intel_plane(crtc->base.cursor);
+
intel_crtc_info(m, crtc);
 
-   active = cursor_position(dev_priv, crtc->pipe, , );
-   seq_printf(m, "\tcursor visible? %s, position (%d, %d), 
size %dx%d, addr 0x%08x, active? %s\n",
-  yesno(crtc->cursor_base),
-  x, y, crtc->base.cursor->state->crtc_w,
-  crtc->base.cursor->state->crtc_h,
-  crtc->cursor_addr, yesno(active));
+   seq_printf(m, "\tcursor visible? %s, position (%d, %d), 
size %dx%d, addr 0x%08x\n",
+  yesno(cursor->base.state->visible),
+  cursor->base.state->crtc_x,
+  cursor->base.state->crtc_y,
+  cursor->base.state->crtc_w,
+  cursor->base.state->crtc_h,
+  cursor->cursor.base);
intel_scaler_info(m, crtc);
intel_plane_info(m, crtc);
}
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 5e04f64a0f76..1d55fac397ad 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9126,8 +9126,7 @@ static bool haswell_get_pipe_config(struct intel_crtc 
*crtc,
return active;
 }
 
-static u32 intel_cursor_base(struct intel_crtc *crtc,
-const struct intel_plane_state *plane_state)
+static u32 intel_cursor_base(const struct intel_plane_state *plane_state)
 {
struct drm_i915_private *dev_priv =
to_i915(plane_state->base.plane->dev);
@@ -9140,8 +9139,6 @@ static u32 intel_cursor_base(struct intel_crtc *crtc,
else
base = intel_plane_ggtt_offset(plane_state);
 
-   crtc->cursor_addr = base;
-
/* ILK+ do this automagically */
if (HAS_GMCH_DISPLAY(dev_priv) &&
plane_state->base.rotation & DRM_ROTATE_180)
@@ -9176,12