On Mon, Mar 27, 2017 at 09:55:35PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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ä <ville.syrjala@xxxxxxxxxxxxxxx>
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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(&crtc->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, &x, &y);
> - 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 +9173,10 @@ static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> CURSOR_STRIDE(stride);
> }
>
> -static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
> +static void i845_update_cursor(struct intel_plane *plane, u32 base,
> const struct intel_plane_state *plane_state)
> {
> - 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 drm_i915_private *dev_priv = to_i915(plane->base.dev);
> uint32_t cntl = 0, size = 0;
>
> if (plane_state && plane_state->base.visible) {
> @@ -9192,32 +9187,32 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
> size = (height << 12) | width;
> }
>
> - if (intel_crtc->cursor_cntl != 0 &&
> - (intel_crtc->cursor_base != base ||
> - intel_crtc->cursor_size != size ||
> - intel_crtc->cursor_cntl != cntl)) {
> + if (plane->cursor.cntl != 0 &&
> + (plane->cursor.base != base ||
> + plane->cursor.size != size ||
> + plane->cursor.cntl != cntl)) {
> /* On these chipsets we can only modify the base/size/stride
> * whilst the cursor is disabled.
> */
> I915_WRITE_FW(CURCNTR(PIPE_A), 0);
> POSTING_READ_FW(CURCNTR(PIPE_A));
> - intel_crtc->cursor_cntl = 0;
> + plane->cursor.cntl = 0;
> }
>
> - if (intel_crtc->cursor_base != base) {
> + if (plane->cursor.base != base) {
> I915_WRITE_FW(CURBASE(PIPE_A), base);
> - intel_crtc->cursor_base = base;
> + plane->cursor.base = base;
> }
>
> - if (intel_crtc->cursor_size != size) {
> + if (plane->cursor.size != size) {
> I915_WRITE_FW(CURSIZE, size);
> - intel_crtc->cursor_size = size;
> + plane->cursor.size = size;
> }
>
> - if (intel_crtc->cursor_cntl != cntl) {
> + if (plane->cursor.cntl != cntl) {
> I915_WRITE_FW(CURCNTR(PIPE_A), cntl);
> POSTING_READ_FW(CURCNTR(PIPE_A));
> - intel_crtc->cursor_cntl = cntl;
> + plane->cursor.cntl = cntl;
> }
> }
>
> @@ -9257,39 +9252,36 @@ static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> return cntl;
> }
>
> -static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
> +
> +static void i9xx_update_cursor(struct intel_plane *plane, u32 base,
> const struct intel_plane_state *plane_state)
> {
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> uint32_t cntl = 0;
>
> if (plane_state && plane_state->base.visible)
> cntl = plane_state->ctl;
>
> - if (intel_crtc->cursor_cntl != cntl) {
> + if (plane->cursor.cntl != cntl) {
> I915_WRITE_FW(CURCNTR(pipe), cntl);
> POSTING_READ_FW(CURCNTR(pipe));
> - intel_crtc->cursor_cntl = cntl;
> + plane->cursor.cntl = cntl;
> }
>
> /* and commit changes on next vblank */
> I915_WRITE_FW(CURBASE(pipe), base);
> POSTING_READ_FW(CURBASE(pipe));
>
> - intel_crtc->cursor_base = base;
> + plane->cursor.base = base;
> }
>
> /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> -static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> +static void intel_crtc_update_cursor(struct intel_plane *plane,
> const struct intel_plane_state *plane_state)
> {
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> u32 pos = 0, base = 0;
> unsigned long irqflags;
>
> @@ -9309,9 +9301,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> }
> pos |= y << CURSOR_Y_SHIFT;
>
> - base = intel_cursor_base(intel_crtc, plane_state);
> - } else {
> - intel_crtc->cursor_addr = 0;
> + base = intel_cursor_base(plane_state);
> }
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> @@ -9319,9 +9309,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> I915_WRITE_FW(CURPOS(pipe), pos);
>
> if (IS_I845G(dev_priv) || IS_I865G(dev_priv))
> - i845_update_cursor(crtc, base, plane_state);
> + i845_update_cursor(plane, base, plane_state);
> else
> - i9xx_update_cursor(crtc, base, plane_state);
> + i9xx_update_cursor(plane, base, plane_state);
>
> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
> @@ -11882,7 +11872,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
> * allocation. In that case since the ddb allocation will be updated
> * once the plane becomes visible, we can skip this check
> */
> - if (intel_crtc->cursor_addr) {
> + if (1) {
> hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
>
> @@ -13756,7 +13746,7 @@ static void
> intel_disable_cursor_plane(struct intel_plane *plane,
> struct intel_crtc *crtc)
> {
> - intel_crtc_update_cursor(&crtc->base, NULL);
> + intel_crtc_update_cursor(plane, NULL);
> }
>
> static void
> @@ -13764,9 +13754,7 @@ intel_update_cursor_plane(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *state)
> {
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -
> - intel_crtc_update_cursor(&crtc->base, state);
> + intel_crtc_update_cursor(plane, state);
> }
>
> static struct intel_plane *
> @@ -13800,6 +13788,10 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> cursor->update_plane = intel_update_cursor_plane;
> cursor->disable_plane = intel_disable_cursor_plane;
>
> + cursor->cursor.base = ~0;
> + cursor->cursor.cntl = ~0;
> + cursor->cursor.size = ~0;
> +
> ret = drm_universal_plane_init(&dev_priv->drm, &cursor->base,
> 0, &intel_cursor_plane_funcs,
> intel_cursor_formats,
> @@ -13907,10 +13899,6 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
> intel_crtc->pipe = pipe;
> intel_crtc->plane = primary->plane;
>
> - intel_crtc->cursor_base = ~0;
> - intel_crtc->cursor_cntl = ~0;
> - intel_crtc->cursor_size = ~0;
> -
> /* initialize shared scalers */
> intel_crtc_init_scalers(intel_crtc, crtc_state);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a3de17f3058b..b3235de8263b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -760,11 +760,6 @@ struct intel_crtc {
> int adjusted_x;
> int adjusted_y;
>
> - uint32_t cursor_addr;
> - uint32_t cursor_cntl;
> - uint32_t cursor_size;
> - uint32_t cursor_base;
> -
> struct intel_crtc_state *config;
>
> /* global reset count when the last flip was submitted */
> @@ -805,6 +800,10 @@ struct intel_plane {
> int max_downscale;
> uint32_t frontbuffer_bit;
>
> + struct {
> + u32 base, cntl, size;
> + } cursor;
> +
> /*
> * NOTE: Do not place new plane state fields here (e.g., when adding
> * new plane properties). New runtime state should now be placed in
> --
> 2.10.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
