Re: [PATCH] drm/i915: Avoid divbyzero in modesetting

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 22-02-16 om 15:27 schreef Tvrtko Ursulin:
> On 22/02/16 13:09, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 22-02-16 om 12:52 schreef Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Not sure if intel_wm_config->num_pipes_active is supposed to
>>> ever be zero when intel_update_watermarks gets called. But
>>> since it can be triggered in early platform bringup perhaps
>>> we want to guard against it rather than divide by zero.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index feb57598727a..2b7998889617 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -2831,8 +2831,13 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>>   		nth_active_pipe++;
>>>   	}
>>>   
>>> -	pipe_size = ddb_size / config->num_pipes_active;
>>> -	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
>>> +	if (WARN_ON(!config->num_pipes_active)) {
>>> +		pipe_size = 0;
>>> +		alloc->start = 0;
>>> +	} else {
>>> +		pipe_size = ddb_size / config->num_pipes_active;
>>> +		alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
>>> +	}
>>>   	alloc->end = alloc->start + pipe_size;
>>>   }
>>>   
>> How can this happen? It seems in that case cstate->base.active would be false for the current pipe,
>> and the code should bail early already..
> Don't know, but does it harm to guard against it? Helps early
> platform bringup a bit.
>
>  [drm:intel_modeset_readout_hw_state] [CRTC:21] hw state readout: enabled
>  [drm:intel_modeset_readout_hw_state] [CRTC:26] hw state readout: disabled
>  [drm:intel_modeset_readout_hw_state] [CRTC:31] hw state readout: disabled
>  [drm:intel_modeset_readout_hw_state] PORT PLL A hw state readout: crtc_mask 0x00000001, on 0
>  [drm:intel_modeset_readout_hw_state] PORT PLL B hw state readout: crtc_mask 0x00000000, on 0
>  [drm:intel_modeset_readout_hw_state] PORT PLL C hw state readout: crtc_mask 0x00000000, on 0
>  [drm:intel_modeset_readout_hw_state] [ENCODER:33:TMDS-33] hw state readout: disabled, pipe A
>  [drm:intel_modeset_readout_hw_state] [ENCODER:35:DP MST-35] hw state readout: disabled, pipe A
>  [drm:intel_modeset_readout_hw_state] [ENCODER:36:DP MST-36] hw state readout: disabled, pipe B
>  [drm:intel_modeset_readout_hw_state] [ENCODER:37:DP MST-37] hw state readout: disabled, pipe C
>  [drm:intel_modeset_readout_hw_state] [ENCODER:42:TMDS-42] hw state readout: disabled, pipe A
>  [drm:intel_modeset_readout_hw_state] [ENCODER:44:DP MST-44] hw state readout: disabled, pipe A
>  [drm:intel_modeset_readout_hw_state] [ENCODER:45:DP MST-45] hw state readout: disabled, pipe B
>  [drm:intel_modeset_readout_hw_state] [ENCODER:46:DP MST-46] hw state readout: disabled, pipe C
>  [drm:intel_modeset_readout_hw_state] [CONNECTOR:34:DP-1] hw state readout: disabled
>  [drm:intel_modeset_readout_hw_state] [CONNECTOR:40:HDMI-A-1] hw state readout: disabled
>  [drm:intel_modeset_readout_hw_state] [CONNECTOR:43:DP-2] hw state readout: disabled
>  [drm:intel_modeset_readout_hw_state] [CONNECTOR:47:HDMI-A-2] hw state readout: disabled
>  [drm:drm_calc_timestamping_constants [drm]] *ERROR* crtc 21: Can't calculate constants, dotclock = 0!
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:i915_get_vblank_timestamp] crtc 0 is disabled
>  [drm:intel_disable_pipe] disabling pipe A
>
Uh oh, we didn't update the atomic state which confused the wm stuff.

Does the below work?

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index deee56010eee..d9e0470419a1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6358,6 +6358,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 {
+	struct intel_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	enum intel_display_power_domain domain;
@@ -6378,6 +6379,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	dev_priv->display.crtc_disable(crtc);
 	intel_crtc->active = false;
 	intel_fbc_disable(intel_crtc);
+
+	DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was enabled, now disabled\n",
+		      crtc->base.id);
+
+	WARN_ON(drm_atomic_set_mode_for_crtc(crtc->state, NULL) < 0);
+	crtc->state->active = false;
+	crtc->enabled = false;
+	crtc->state->connector_mask = 0;
+	crtc->state->encoder_mask = 0;
+
+	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
+		encoder->base.crtc = NULL;
+
 	intel_update_watermarks(crtc);
 	intel_disable_shared_dpll(intel_crtc);
 
@@ -15526,38 +15540,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
-	if (!intel_crtc_has_encoders(crtc))
+	if (crtc->active && !intel_crtc_has_encoders(crtc))
 		intel_crtc_disable_noatomic(&crtc->base);
 
-	if (crtc->active != crtc->base.state->active) {
-		struct intel_encoder *encoder;
-
-		/* This can happen either due to bugs in the get_hw_state
-		 * functions or because of calls to intel_crtc_disable_noatomic,
-		 * or because the pipe is force-enabled due to the
-		 * pipe A quirk. */
-		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
-			      crtc->base.base.id,
-			      crtc->base.state->enable ? "enabled" : "disabled",
-			      crtc->active ? "enabled" : "disabled");
-
-		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
-		crtc->base.state->connector_mask = 0;
-		crtc->base.state->encoder_mask = 0;
-
-		/* Because we only establish the connector -> encoder ->
-		 * crtc links if something is active, this means the
-		 * crtc is now deactivated. Break the links. connector
-		 * -> encoder links are only establish when things are
-		 *  actually up, hence no need to break them. */
-		WARN_ON(crtc->active);
-
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
-			encoder->base.crtc = NULL;
-	}
-
 	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx