Re: [PATCH v2 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This removes another couple of hacks from intel_crtc->atomic, and
> creates proper atomic state for it.

Perhaps you could be more verbose about the hacks being removed.

> Changes since v1:
> - Rebase on top of wm changes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++------------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
>  3 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index c4eadbc928b7..3e390db9d22b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
> +	crtc_state->visible_changed = false;
> +	crtc_state->wm_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 54e4f04bb427..356e3a9e1741 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  static void intel_post_plane_update(struct intel_crtc *crtc)
>  {
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
> -	if (atomic->disable_cxsr)
> -		crtc->wm.cxsr_allowed = true;
> +	crtc->wm.cxsr_allowed = true;
>  
> -	if (crtc->atomic.update_wm_post)
> +	if (pipe_config->wm_changed)
>  		intel_update_watermarks(&crtc->base);
>  
>  	if (atomic->update_fbc)
> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
>  
> -	if (atomic->disable_cxsr) {
> +	if (pipe_config->visible_changed) {
>  		crtc->wm.cxsr_allowed = false;
>  		intel_set_memory_cxsr(dev_priv, false);
>  	}
> +
> +	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> +		intel_update_watermarks(&crtc->base);
>  }
>  
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> plane_mask)
> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
> *state)
>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  				    struct drm_plane_state *plane_state)
>  {
> +	struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
>  	struct drm_crtc *crtc = crtc_state->crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane = plane_state->plane;
> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  			 plane->base.id, was_visible, visible,
>  			 turn_off, turn_on, mode_changed);
>  
> -	if (turn_on) {
> -		intel_crtc->atomic.update_wm_pre = true;
> -		/* must disable cxsr around plane enable/disable */
> -		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			intel_crtc->atomic.disable_cxsr = true;
> -			/* to potentially re-enable cxsr */
> -			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_wm_post = true;
> -		}
> -	} else if (turn_off) {
> -		intel_crtc->atomic.update_wm_post = true;
> +	if (turn_on || turn_off) {
> +		pipe_config->wm_changed = true;
> +
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			pipe_config->visible_changed = true;
>  			if (is_crtc_enabled)
>  				intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.disable_cxsr = true;
>  		}

I'm a bit confused as to what the value of visible_changed is supposed to be. In
the comment in intel_drv.h you wrote "plane visibility changed". But that isn't
set if the cursor plane visibility changed. If the value of this is only
relevant for cxsr, maybe it make sense to still call it disable_cxsr?

Ander

> -	} else if (intel_wm_need_update(plane, plane_state)) {
> -		intel_crtc->atomic.update_wm_pre = true;
> +	} else if ((was_visible || visible) &&
> +		   intel_wm_need_update(plane, plane_state)) {
> +		pipe_config->wm_changed = true;
>  	}
>  
>  	if (visible || was_visible)
> @@ -11826,7 +11826,7 @@ static int intel_crtc_atomic_check(struct drm_crtc
> *crtc,
>  	}
>  
>  	if (mode_changed && !crtc_state->active)
> -		intel_crtc->atomic.update_wm_post = true;
> +		pipe_config->wm_changed = true;
>  
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
> @@ -13735,9 +13735,6 @@ static void intel_begin_crtc_commit(struct drm_crtc
> *crtc,
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
>  
> -	if (intel_crtc->atomic.update_wm_pre)
> -		intel_update_watermarks(crtc);
> -
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3b03074813e4..59367453d24b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -372,7 +372,9 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync
> mode.flags */
>  	unsigned long quirks;
>  
> -	bool update_pipe;
> +	bool update_pipe; /* can a fast modeset be performed? */
> +	bool visible_changed; /* plane visibility changed */
> +	bool wm_changed; /* wm changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -535,9 +537,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
>  	bool disable_ips;
> -	bool disable_cxsr;
>  	bool pre_disable_primary;
> -	bool update_wm_pre, update_wm_post;
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx