| Message ID | 1382445473-17558-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 22, 2013 at 02:37:53PM +0200, Daniel Vetter wrote: > We expect this bit to be always set when possible, but some BIOSes are > lazy and don't do this. The result is a pile of WARNs and unhappy fdi > link training code ... > > v2: It's actually the inverse: The BIOS sets this bit when it's not > strictly needed. This should be cleaned up in the > global_modeset_resources callback, but we've failed to look at the > active bit. Which means this won't fire (and so clean up BIOS state) > when enabling pipe B or C for the first time. > > v3: Wrap lines. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70507 > Tested-by: Jan-Michael Brummer <jan.brummer@tabos.org> > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cfe9e709..3569db6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc) > FDI_FE_ERRC_ENABLE); > } > > -static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc) > +static bool pipe_has_enabled_pch(struct intel_crtc *crtc) > { > - return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder; > + return crtc->base.enabled && crtc->active && > + crtc->config.has_pch_encoder; I'm thinking the crtc->base.enabled check is actually pointless. AFAICS we should never get here with crtc->base.enabled==false and crtc->active==true. We anyway re-enable the bifurcation bit when we do the mode_set. Actually that in itself could be a maybe a bug. We'd turn off the bifurcation bit when both pipes B and C are disabled based on prepare_pipes, but we only do the mode_set based on modeset_pipes. But currently we are saved by the fact that those two bitmasks are the same. Another potential bug I found is that we always set the bifurcation bit in mode_set, even if we're not using FDI. So if we have eg. pipe B enabled w/ FDI using 4 lanes, and then we enable pipe C w/ EDP, we'd still flip the bifurcation bit in pipe C's mode_set and destroy pipe B's output. The fix would be to call ivybridge_update_fdi_bc_bifurcation() only when has_pch_encoder==true. > } > > static void ivb_modeset_global_resources(struct drm_device *dev) > -- > 1.8.4.rc3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 23, 2013 at 12:58 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > I'm thinking the crtc->base.enabled check is actually pointless. > AFAICS we should never get here with crtc->base.enabled==false and > crtc->active==true Hm yeah. I was kinda shooting for the minimal thing here. > We anyway re-enable the bifurcation bit when we do the mode_set. > Actually that in itself could be a maybe a bug. We'd turn off the > bifurcation bit when both pipes B and C are disabled based on > prepare_pipes, but we only do the mode_set based on modeset_pipes. > But currently we are saved by the fact that those two bitmasks are > the same. The current logic (after this patch) is to - always clear the bit when both pipes are off. - always set if if we either enable pipe B or C and we'd need it to allow the other pipe to be lit up. If pipe B uses 4 lanes then we can't light pipe C up anymore, so we don't set the bit. There is a potential issue though with all pipes disabled with dpms off now. I think conceptually I need to precompute the desired state of the bifurcate bit in the compute config stage and then only set it here. > Another potential bug I found is that we always set the bifurcation > bit in mode_set, even if we're not using FDI. So if we have eg. > pipe B enabled w/ FDI using 4 lanes, and then we enable pipe C w/ EDP, > we'd still flip the bifurcation bit in pipe C's mode_set and destroy pipe > B's output. The fix would be to call ivybridge_update_fdi_bc_bifurcation() > only when has_pch_encoder==true. has_pch_encoder should only be true if we have fdi_lanes > 0. So we shouldn't actually enable this stuff (and iirc I've tested the edp on pipe C with pipe B using 4 lanes). The actual configuration checks all happen in the pipe configuration computation stage. I'll rework the patch a bit (but that'll take a while). Thanks for the review, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cfe9e709..3569db6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2421,9 +2421,10 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc) FDI_FE_ERRC_ENABLE); } -static bool pipe_has_enabled_pch(struct intel_crtc *intel_crtc) +static bool pipe_has_enabled_pch(struct intel_crtc *crtc) { - return intel_crtc->base.enabled && intel_crtc->config.has_pch_encoder; + return crtc->base.enabled && crtc->active && + crtc->config.has_pch_encoder; } static void ivb_modeset_global_resources(struct drm_device *dev)