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@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
