| Message ID | 1465382507-23085-12-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 01:41:46PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Move the encoder cloning check to happen earlier in the modeset. The > main benefit will be that the debug output from a failed modeset will > be less confusing as output_types can not indicate an invalid > configuration during the later computation stages. > > For instance, what happened to me was kms_setmode was attempting one > of its invalid cloning checks during which it asked for DP+VGA cloning > on HSW. In this case the DP .compute_config() was executed after > the FDI .compute_config() leaving the DP link clock (1.62 in this case) > in port_clock, and then later the FDI BW computation tried to use that > as the FDI link clock (which should always be 2.7). 1.62 x 2 wasn't > enough for the mode it was trying to use, and so it ended up rejecting > the modeset, not because of an invalid cloning configuration, but > because of supposedly running out of FDI bandwidth. Took me a while > to figure out what had actually happened. Did it reject the 1.62 link clock when you simply removed the check_encoder_cloning()? Just checking... :) -Chris
On Wed, Jun 08, 2016 at 02:15:25PM +0100, Chris Wilson wrote: > On Wed, Jun 08, 2016 at 01:41:46PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Move the encoder cloning check to happen earlier in the modeset. The > > main benefit will be that the debug output from a failed modeset will > > be less confusing as output_types can not indicate an invalid > > configuration during the later computation stages. > > > > For instance, what happened to me was kms_setmode was attempting one > > of its invalid cloning checks during which it asked for DP+VGA cloning > > on HSW. In this case the DP .compute_config() was executed after > > the FDI .compute_config() leaving the DP link clock (1.62 in this case) > > in port_clock, and then later the FDI BW computation tried to use that > > as the FDI link clock (which should always be 2.7). 1.62 x 2 wasn't > > enough for the mode it was trying to use, and so it ended up rejecting > > the modeset, not because of an invalid cloning configuration, but > > because of supposedly running out of FDI bandwidth. Took me a while > > to figure out what had actually happened. > > Did it reject the 1.62 link clock when you simply removed the > check_encoder_cloning()? Just checking... :) Nope. The rejection only happened due to exceeding the max fdi lane count. I think I had a warn in there somewhere when I originally submitted the fdi==port_clock patch, but that apparently didn't survive into the version that eventually got merged.
Op 08-06-16 om 15:27 schreef Ville Syrjälä: > On Wed, Jun 08, 2016 at 02:15:25PM +0100, Chris Wilson wrote: >> On Wed, Jun 08, 2016 at 01:41:46PM +0300, ville.syrjala@linux.intel.com wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> Move the encoder cloning check to happen earlier in the modeset. The >>> main benefit will be that the debug output from a failed modeset will >>> be less confusing as output_types can not indicate an invalid >>> configuration during the later computation stages. >>> >>> For instance, what happened to me was kms_setmode was attempting one >>> of its invalid cloning checks during which it asked for DP+VGA cloning >>> on HSW. In this case the DP .compute_config() was executed after >>> the FDI .compute_config() leaving the DP link clock (1.62 in this case) >>> in port_clock, and then later the FDI BW computation tried to use that >>> as the FDI link clock (which should always be 2.7). 1.62 x 2 wasn't >>> enough for the mode it was trying to use, and so it ended up rejecting >>> the modeset, not because of an invalid cloning configuration, but >>> because of supposedly running out of FDI bandwidth. Took me a while >>> to figure out what had actually happened. >> Did it reject the 1.62 link clock when you simply removed the >> check_encoder_cloning()? Just checking... :) > Nope. The rejection only happened due to exceeding the max fdi lane > count. I think I had a warn in there somewhere when I originally > submitted the fdi==port_clock patch, but that apparently didn't > survive into the version that eventually got merged. > For patch 1-11: Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3a0b590c4885..442ed6320082 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11997,26 +11997,6 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state, return true; } -static bool check_encoder_cloning(struct drm_atomic_state *state, - struct intel_crtc *crtc) -{ - struct intel_encoder *encoder; - struct drm_connector *connector; - struct drm_connector_state *connector_state; - int i; - - for_each_connector_in_state(state, connector, connector_state, i) { - if (connector_state->crtc != &crtc->base) - continue; - - encoder = to_intel_encoder(connector_state->best_encoder); - if (!check_single_encoder_cloning(state, crtc, encoder)) - return false; - } - - return true; -} - static int intel_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) { @@ -12029,11 +12009,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, int ret; bool mode_changed = needs_modeset(crtc_state); - if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { - DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); - return -EINVAL; - } - if (mode_changed && !crtc_state->active) pipe_config->update_wm_post = true; @@ -12472,6 +12447,11 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, encoder = to_intel_encoder(connector_state->best_encoder); + if (!check_single_encoder_cloning(state, to_intel_crtc(crtc), encoder)) { + DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); + goto fail; + } + /* * Determine output_types before calling the .compute_config() * hooks so that the hooks can use this information safely.