On Mon, 09 Feb 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Mon, 09 Feb 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: >> On Mon, Feb 09, 2015 at 03:15:56PM +0200, Jani Nikula wrote: >>> On Wed, 26 Nov 2014, Paulo Zanoni <przanoni@xxxxxxxxx> wrote: >>> > 2014-11-24 13:54 GMT-02:00 Daniel Vetter <daniel.vetter@xxxxxxxx>: >>> >> Nothing in Bspec seems to indicate that we actually needs this, and it >>> >> looks like can't work since by this point the pipe is off and so >>> >> vblanks won't really happen any more. >>> >> >>> >> Note that Bspec mentions that it takes a vblank for this bit to >>> >> change, but _only_ when enabling. >>> >> >>> >> Dropping this code quenches an annoying backtrace introduced by the >>> >> more anal checking since >>> >> >>> >> commit 51e31d49c89055299e34b8f44d13f70e19aaaad1 >>> >> Author: Daniel Vetter <daniel.vetter@xxxxxxxx> >>> >> Date: Mon Sep 15 12:36:02 2014 +0200 >>> >> >>> >> drm/i915: Use generic vblank wait >>> >> >>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86095 >>> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> >>> >> --- >>> >> drivers/gpu/drm/i915/intel_dp.c | 17 +---------------- >>> >> 1 file changed, 1 insertion(+), 16 deletions(-) >>> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> >> index 46731da407c0..63fcdbf90652 100644 >>> >> --- a/drivers/gpu/drm/i915/intel_dp.c >>> >> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> >> @@ -3514,8 +3514,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) >>> >> enum port port = intel_dig_port->port; >>> >> struct drm_device *dev = intel_dig_port->base.base.dev; >>> >> struct drm_i915_private *dev_priv = dev->dev_private; >>> >> - struct intel_crtc *intel_crtc = >>> >> - to_intel_crtc(intel_dig_port->base.base.crtc); >>> >> uint32_t DP = intel_dp->DP; >>> >> >>> >> if (WARN_ON(HAS_DDI(dev))) >>> >> @@ -3540,8 +3538,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) >>> >> >>> >> if (HAS_PCH_IBX(dev) && >>> >> I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) { >>> >> - struct drm_crtc *crtc = intel_dig_port->base.base.crtc; >>> >> - >>> >> /* Hardware workaround: leaving our transcoder select >>> >> * set to transcoder B while it's off will prevent the >>> >> * corresponding HDMI output on transcoder A. >>> >> @@ -3552,18 +3548,7 @@ intel_dp_link_down(struct intel_dp *intel_dp) >>> >> */ >>> >> DP &= ~DP_PIPEB_SELECT; >>> >> I915_WRITE(intel_dp->output_reg, DP); >>> >> - >>> >> - /* Changes to enable or select take place the vblank >>> >> - * after being written. >>> >> - */ >>> >> - if (WARN_ON(crtc == NULL)) { >>> >> - /* We should never try to disable a port without a crtc >>> >> - * attached. For paranoia keep the code around for a >>> >> - * bit. */ >>> >> - POSTING_READ(intel_dp->output_reg); >>> >> - msleep(50); >>> >> - } else >>> >> - intel_wait_for_vblank(dev, intel_crtc->pipe); >>> > >>> > What I can guess is that we have the vblank wait here because the >>> > DP_PORT_EN bit is still enabled at this point. It would make some >>> > sense to have it if the pipe were not off... So removing the waits >>> > looks sane: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >>> > >>> > But when I read the spec, it makes me think that maybe doing the >>> > I915_WRITE above is also wrong, since the port is still enabled. Maybe >>> > we should only clear bit 30 in the same write as the one that clears >>> > bit 31? >>> >>> Ugh. So the spec says, "When disabling the port, software must >>> temporarily enable the port with transcoder select (bit #30) cleared to >>> ‘0’ after disabling the port." >>> >>> IIUC we should disable like we normally do, and do the w/a by enabling >>> and disabling the port with DP_PIPEB_SELECT cleared *after* that. I >>> think the current code is wrong, the patch is wrong, what Paulo suggests >>> is wrong, and also intel_disable_hdmi() is wrong. >> >> This code has been bugging me for a long time as well. IIRC I even had >> cooked up some patches to do the re-enable as you suggest since I >> read the spec the same way. But I never had enough time to test it. And >> in order to really test it I would first like to actually reproduce the >> problem that the workaround is supposed to fix. How else would you know >> if the workaround is correct after all. > > *sigh* an alternative is to apply Daniel's patch and add a comment > there's something fishy. I've done just that, with cc: stable for v3.19. I referenced this discussion from the commit message. Thanks for the patch, review, and bikeshedding. BR, Jani. > > Jani. > >> >> -- >> Ville Syrjälä >> Intel OTC > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
