Re: [PATCH 04/35] drm/i915: get rid of primary_enabled and use atomic state

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 21, 2015 at 05:12:53PM +0300, Ander Conselvan de Oliveira wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Generally a bit too terse commmit message. E.g. here a short summary of
our irc discussion about why it should be ok to remove this feature is
definitely needed. I added that.

But I prefer more verbose commit messages in general.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c     |  2 +-
>  3 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index acf3494..40e3c62 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2236,11 +2236,7 @@ static void intel_enable_primary_hw_plane(struct drm_plane *plane,
>  
>  	/* If the pipe isn't enabled, we can't pump pixels and may hang */
>  	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> -
> -	if (intel_crtc->primary_enabled)
> -		return;
> -
> -	intel_crtc->primary_enabled = true;
> +	to_intel_plane_state(plane->state)->visible = true;
>  
>  	dev_priv->display.update_primary_plane(crtc, plane->fb,
>  					       crtc->x, crtc->y);
> @@ -2661,6 +2657,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *primary = crtc->primary;
> +	bool visible = to_intel_plane_state(primary->state)->visible;
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
> @@ -2668,7 +2666,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	u32 reg = DSPCNTR(plane);
>  	int pixel_size;
>  
> -	if (!intel_crtc->primary_enabled || !fb) {
> +	if (!visible || !fb) {
>  		I915_WRITE(reg, 0);
>  		if (INTEL_INFO(dev)->gen >= 4)
>  			I915_WRITE(DSPSURF(plane), 0);
> @@ -2790,6 +2788,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *primary = crtc->primary;
> +	bool visible = to_intel_plane_state(primary->state)->visible;
>  	struct drm_i915_gem_object *obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
> @@ -2797,7 +2797,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	u32 reg = DSPCNTR(plane);
>  	int pixel_size;
>  
> -	if (!intel_crtc->primary_enabled || !fb) {
> +	if (!visible || !fb) {
>  		I915_WRITE(reg, 0);
>  		I915_WRITE(DSPSURF(plane), 0);
>  		POSTING_READ(reg);
> @@ -2966,6 +2966,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *plane = crtc->primary;
> +	bool visible = to_intel_plane_state(plane->state)->visible;
>  	struct drm_i915_gem_object *obj;
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl, stride_div, stride;
> @@ -2973,9 +2975,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	unsigned int rotation;
>  	int x_offset, y_offset;
>  	unsigned long surf_addr;
> -	struct drm_plane *plane;
>  
> -	if (!intel_crtc->primary_enabled || !fb) {
> +	if (!visible || !fb) {
>  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  		I915_WRITE(PLANE_SURF(pipe, 0), 0);
>  		POSTING_READ(PLANE_CTL(pipe, 0));
> @@ -3035,7 +3036,6 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  
>  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>  
> -	plane = crtc->primary;
>  	rotation = plane->state->rotation;
>  	switch (rotation) {
>  	case BIT(DRM_ROTATE_90):
> @@ -4699,7 +4699,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	hsw_disable_ips(intel_crtc);
>  
>  	intel_crtc_dpms_overlay(intel_crtc, false);
> -	intel_crtc->primary_enabled = false;
>  	for_each_intel_plane(dev, intel_plane) {
>  		if (intel_plane->pipe == pipe) {
>  			struct drm_crtc *from = intel_plane->base.crtc;
> @@ -12803,6 +12802,9 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	} else if (config->fb_changed) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
>  		struct drm_plane *primary = set->crtc->primary;
> +		struct intel_plane_state *plane_state =
> +				to_intel_plane_state(primary->state);
> +		bool was_visible = plane_state->visible;
>  		int vdisplay, hdisplay;
>  
>  		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> @@ -12815,7 +12817,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		 * We need to make sure the primary plane is re-enabled if it
>  		 * has previously been turned off.
>  		 */
> -		if (!intel_crtc->primary_enabled && ret == 0) {
> +		plane_state = to_intel_plane_state(primary->state);
> +		if (ret == 0 && !was_visible && plane_state->visible) {
>  			WARN_ON(!intel_crtc->active);
>  			intel_enable_primary_hw_plane(set->crtc->primary, set->crtc);
>  		}
> @@ -13113,6 +13116,9 @@ intel_check_primary_plane(struct drm_plane *plane,
>  		return ret;
>  
>  	if (intel_crtc->active) {
> +		struct intel_plane_state *old_state =
> +			to_intel_plane_state(plane->state);
> +
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -13125,20 +13131,20 @@ intel_check_primary_plane(struct drm_plane *plane,
>  		 * one is done too late. We eventually need to unify
>  		 * this.
>  		 */
> -		if (intel_crtc->primary_enabled &&
> +		if (state->visible &&
>  		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
>  		    dev_priv->fbc.crtc == intel_crtc &&
>  		    state->base.rotation != BIT(DRM_ROTATE_0)) {
>  			intel_crtc->atomic.disable_fbc = true;
>  		}
>  
> -		if (state->visible) {
> +		if (state->visible && !old_state->visible) {
>  			/*
>  			 * BDW signals flip done immediately if the plane
>  			 * is disabled, even if the plane enable is already
>  			 * armed to occur at the next vblank :(
>  			 */
> -			if (IS_BROADWELL(dev) && !intel_crtc->primary_enabled)
> +			if (IS_BROADWELL(dev))
>  				intel_crtc->atomic.wait_vblank = true;
>  		}
>  
> @@ -13173,8 +13179,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	crtc->y = src->y1 >> 16;
>  
>  	if (intel_crtc->active) {
> -		intel_crtc->primary_enabled = state->visible;
> -
>  		if (state->visible)
>  			/* FIXME: kill this fastboot hack */
>  			intel_update_pipe_size(intel_crtc);
> @@ -13192,9 +13196,6 @@ intel_disable_primary_plane(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!force)
> -		to_intel_crtc(crtc)->primary_enabled = false;
> -
>  	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
>  }
>  
> @@ -14655,8 +14656,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 * Temporarily change the plane mapping and disable everything
>  		 * ...  */
>  		plane = crtc->plane;
> +		to_intel_plane_state(crtc->base.primary->state)->visible = true;
>  		crtc->plane = !plane;
> -		crtc->primary_enabled = true;
>  		dev_priv->display.crtc_disable(&crtc->base);
>  		crtc->plane = plane;
>  
> @@ -14833,6 +14834,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	int i;
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		struct drm_plane *primary = crtc->base.primary;
> +		struct intel_plane_state *plane_state;
> +
>  		memset(crtc->config, 0, sizeof(*crtc->config));
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
> @@ -14842,7 +14846,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		crtc->base.state->enable = crtc->active;
>  		crtc->base.enabled = crtc->active;
> -		crtc->primary_enabled = primary_get_hw_state(crtc);
> +
> +		plane_state = to_intel_plane_state(primary->state);
> +		plane_state->visible = primary_get_hw_state(crtc);
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  			      crtc->base.base.id,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 42548bc..cb0f29e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -510,7 +510,6 @@ struct intel_crtc {
>  	 */
>  	bool active;
>  	unsigned long enabled_power_domains;
> -	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;
>  	struct intel_overlay *overlay;
>  	struct intel_unpin_work *unpin_work;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 4165ce0..6abb834 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -457,7 +457,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
>  		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>  
>  		if (intel_crtc_active(tmp_crtc) &&
> -		    to_intel_crtc(tmp_crtc)->primary_enabled) {
> +		    to_intel_plane_state(tmp_crtc->primary->state)->visible) {
>  			if (one_pipe_only && crtc) {
>  				if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
>  					DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx