diff mbox

[v2,01/12] drm/i915: Make the force_thru workaround atomic.

Message ID 1438000541-3219-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst July 27, 2015, 12:35 p.m. UTC 
Set active_changed to force a modeset if the panel fitter's force
enabled.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 2 files changed, 27 insertions(+), 58 deletions(-)

Comments

Daniel Vetter July 27, 2015, 2:04 p.m. UTC | #1 
On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
> Set active_changed to force a modeset if the panel fitter's force
> enabled.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Hm, shouldn't our fancy fastset logic be able to detect that we've changed
pfit change here and force a full modeset? Or am I blind again?

Abusing active_changed for this feels a bit tricksy tbh, can't we use
mode_changed for this? mode_changed is kinda for "crtc configuration that
needs a full modeset changed", not just for modes. active_changed is
"enable/disable it, but strictly speaking no need to recompute stuff".

At least that's how the atomic helpers treat it.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  2 files changed, 27 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 23a69307e12e..22467adee958 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> +static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
>  	struct intel_crtc_state *pipe_config;
> +	struct drm_atomic_state *state;
> +	int ret = 0;
>  
>  	drm_modeset_lock_all(dev);
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    !pipe_config->pch_pfit.enabled) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = true;
> -
> -		intel_display_power_get(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> -
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
> -	drm_modeset_unlock_all(dev);
> -}
>  
> -static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
> -	struct intel_crtc_state *pipe_config;
> -
> -	drm_modeset_lock_all(dev);
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -	if (pipe_config->pch_pfit.force_thru) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = false;
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
> +	pipe_config = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto out;
> +	}
>  
> -		intel_display_power_put(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> +	pipe_config->pch_pfit.force_thru = enable;
> +	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +	    pipe_config->pch_pfit.enabled != enable)
> +		pipe_config->base.active_changed = true;
>  
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> -	}
> +	ret = drm_atomic_commit(state);
> +out:
>  	drm_modeset_unlock_all(dev);
> +	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> +	if (ret)
> +		drm_atomic_state_free(state);
>  }
>  
>  static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
> @@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
>  		if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, true);
>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		else if (IS_VALLEYVIEW(dev))
>  			vlv_undo_pipe_scramble_reset(dev, pipe);
>  		else if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, false);
>  
>  		hsw_enable_ips(crtc);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43b0f17ad1fa..7747520bf9f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	enum intel_dpll_id shared_dpll;
>  	uint32_t ddi_pll_sel;
> +	bool force_thru;
>  
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
> @@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	ddi_pll_sel = crtc_state->ddi_pll_sel;
> +	force_thru = crtc_state->pch_pfit.force_thru;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->ddi_pll_sel = ddi_pll_sel;
> +	crtc_state->pch_pfit.force_thru = force_thru;
>  }
>  
>  static int
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst July 28, 2015, 7:57 a.m. UTC | #2 
Op 27-07-15 om 16:04 schreef Daniel Vetter:
> On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
>> Set active_changed to force a modeset if the panel fitter's force
>> enabled.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Hm, shouldn't our fancy fastset logic be able to detect that we've changed
> pfit change here and force a full modeset? Or am I blind again?
>
> Abusing active_changed for this feels a bit tricksy tbh, can't we use
> mode_changed for this? mode_changed is kinda for "crtc configuration that
> needs a full modeset changed", not just for modes. active_changed is
> "enable/disable it, but strictly speaking no need to recompute stuff".
>
> At least that's how the atomic helpers treat it.
>
I think for !PIPE_A it's ok, but pipe_a + edp can use a different power well iirc.
I wasn't sure how that was treated so I went for active_changed instead of mode_changed.

~Maarten
Daniel Vetter July 28, 2015, 8:25 a.m. UTC | #3 
On Tue, Jul 28, 2015 at 09:57:37AM +0200, Maarten Lankhorst wrote:
> Op 27-07-15 om 16:04 schreef Daniel Vetter:
> > On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
> >> Set active_changed to force a modeset if the panel fitter's force
> >> enabled.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Hm, shouldn't our fancy fastset logic be able to detect that we've changed
> > pfit change here and force a full modeset? Or am I blind again?
> >
> > Abusing active_changed for this feels a bit tricksy tbh, can't we use
> > mode_changed for this? mode_changed is kinda for "crtc configuration that
> > needs a full modeset changed", not just for modes. active_changed is
> > "enable/disable it, but strictly speaking no need to recompute stuff".
> >
> > At least that's how the atomic helpers treat it.
> >
> I think for !PIPE_A it's ok, but pipe_a + edp can use a different power well iirc.
> I wasn't sure how that was treated so I went for active_changed instead of mode_changed.

Following up from our irc discussion: mode_changed is indeed not a good
idea since with the fastset tricks we might accidentally ellide the
modeset if intel_pipe_config_compare doesn't catch it (and right now it
won't). But I still think active_changed is abuse since active_changed
should _not_ result in state recomputation really, the entire point of
crtc_state->active is that you can always flip it and it will never fail
due to lack of resources.

But with your latest patches we have connector_changed to track routing
changes, and force_thru is very much a routing change. I think using that
would be best.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a69307e12e..22467adee958 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3645,74 +3645,40 @@  static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
 	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret = 0;
 
 	drm_modeset_lock_all(dev);
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    !pipe_config->pch_pfit.enabled) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = true;
-
-		intel_display_power_get(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
-
-		if (active)
-			intel_crtc_control(&crtc->base, true);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
-	drm_modeset_unlock_all(dev);
-}
 
-static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
-	struct intel_crtc_state *pipe_config;
-
-	drm_modeset_lock_all(dev);
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-	if (pipe_config->pch_pfit.force_thru) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = false;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+	pipe_config = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
 
-		intel_display_power_put(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+	pipe_config->pch_pfit.force_thru = enable;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    pipe_config->pch_pfit.enabled != enable)
+		pipe_config->base.active_changed = true;
 
-		if (active)
-			intel_crtc_control(&crtc->base, true);
-	}
+	ret = drm_atomic_commit(state);
+out:
 	drm_modeset_unlock_all(dev);
+	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+	if (ret)
+		drm_atomic_state_free(state);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
@@ -3732,7 +3698,7 @@  static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
 		if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -3844,7 +3810,7 @@  static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_VALLEYVIEW(dev))
 			vlv_undo_pipe_scramble_reset(dev, pipe);
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
 		hsw_enable_ips(crtc);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17ad1fa..7747520bf9f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12160,6 +12160,7 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	enum intel_dpll_id shared_dpll;
 	uint32_t ddi_pll_sel;
+	bool force_thru;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12171,6 +12172,7 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	ddi_pll_sel = crtc_state->ddi_pll_sel;
+	force_thru = crtc_state->pch_pfit.force_thru;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12179,6 +12181,7 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->ddi_pll_sel = ddi_pll_sel;
+	crtc_state->pch_pfit.force_thru = force_thru;
 }
 
 static int