On Wed, 07 Jan 2015, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> It is platform/output depenedent when exactly the pipe will start
> running. Sometimes we just need the (cpu) pipe enabled, in other cases
> the pch transcoder is enough and in yet other cases the (DP) port is
> sending the frame start signal.
>
> In a perfect world we'd put the drm_crtc_vblank_on call exactly where
> the pipe starts running, but due to cloning and similar things this
> will get messy. And the current approach of picking the most
> conservative place for all combinations also doesn't work since that
> results in legit vblank waits (in encoder->enable hooks, e.g. the 2
> vblank waits for sdvo) failing.
>
> Completely going back to the old world before
>
> commit 51e31d49c89055299e34b8f44d13f70e19aaaad1
> Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Date: Mon Sep 15 12:36:02 2014 +0200
>
> drm/i915: Use generic vblank wait# Please enter the commit message for your changes. Lines starting
>
> isn't great either since screaming when the vblank wait work because
> the pipe is off is kinda nice.
>
> Pick a compromise and move the drm_crtc_vblank_on right before the
> encoder->enable call. This is a lie on some outputs/platforms, but
> after the ->enable callback the pipe is guaranteed to run everywhere.
> So not that bad really. Suggested by Ville.
>
> v2: Same treatment for drm_crtc_vblank_off and encoder->disable: I've
> missed the ibx pipe B select w/a, which also has a vblank wait in the
> disable function (while the pipe is obviously still running).
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Should this be forwarded to stable 3.19?
BR,
Jani.
> ---
> drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a1dbe747a372..e224820ea5a4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4301,15 +4301,15 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> if (intel_crtc->config.has_pch_encoder)
> ironlake_pch_enable(crtc);
>
> + assert_vblank_disabled(crtc);
> + drm_crtc_vblank_on(crtc);
> +
> for_each_encoder_on_crtc(dev, crtc, encoder)
> encoder->enable(encoder);
>
> if (HAS_PCH_CPT(dev))
> cpt_verify_modeset(dev, intel_crtc->pipe);
>
> - assert_vblank_disabled(crtc);
> - drm_crtc_vblank_on(crtc);
> -
> intel_crtc_enable_planes(crtc);
> }
>
> @@ -4421,14 +4421,14 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> if (intel_crtc->config.dp_encoder_is_mst)
> intel_ddi_set_vc_payload_alloc(crtc, true);
>
> + assert_vblank_disabled(crtc);
> + drm_crtc_vblank_on(crtc);
> +
> for_each_encoder_on_crtc(dev, crtc, encoder) {
> encoder->enable(encoder);
> intel_opregion_notify_encoder(encoder, true);
> }
>
> - assert_vblank_disabled(crtc);
> - drm_crtc_vblank_on(crtc);
> -
> /* If we change the relative order between pipe/planes enabling, we need
> * to change the workaround. */
> haswell_mode_set_planes_workaround(intel_crtc);
> @@ -4479,12 +4479,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>
> intel_crtc_disable_planes(crtc);
>
> - drm_crtc_vblank_off(crtc);
> - assert_vblank_disabled(crtc);
> -
> for_each_encoder_on_crtc(dev, crtc, encoder)
> encoder->disable(encoder);
>
> + drm_crtc_vblank_off(crtc);
> + assert_vblank_disabled(crtc);
> +
> if (intel_crtc->config.has_pch_encoder)
> intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
>
> @@ -4544,14 +4544,14 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>
> intel_crtc_disable_planes(crtc);
>
> - drm_crtc_vblank_off(crtc);
> - assert_vblank_disabled(crtc);
> -
> for_each_encoder_on_crtc(dev, crtc, encoder) {
> intel_opregion_notify_encoder(encoder, false);
> encoder->disable(encoder);
> }
>
> + drm_crtc_vblank_off(crtc);
> + assert_vblank_disabled(crtc);
> +
> if (intel_crtc->config.has_pch_encoder)
> intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> false);
> @@ -5021,12 +5021,12 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> intel_update_watermarks(crtc);
> intel_enable_pipe(intel_crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->enable(encoder);
> -
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> + for_each_encoder_on_crtc(dev, crtc, encoder)
> + encoder->enable(encoder);
> +
> intel_crtc_enable_planes(crtc);
>
> /* Underruns don't raise interrupts, so check manually. */
> @@ -5082,12 +5082,12 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> intel_update_watermarks(crtc);
> intel_enable_pipe(intel_crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - encoder->enable(encoder);
> -
> assert_vblank_disabled(crtc);
> drm_crtc_vblank_on(crtc);
>
> + for_each_encoder_on_crtc(dev, crtc, encoder)
> + encoder->enable(encoder);
> +
> intel_crtc_enable_planes(crtc);
>
> /*
> @@ -5159,12 +5159,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> */
> intel_wait_for_vblank(dev, pipe);
>
> - drm_crtc_vblank_off(crtc);
> - assert_vblank_disabled(crtc);
> -
> for_each_encoder_on_crtc(dev, crtc, encoder)
> encoder->disable(encoder);
>
> + drm_crtc_vblank_off(crtc);
> + assert_vblank_disabled(crtc);
> +
> intel_disable_pipe(intel_crtc);
>
> i9xx_pfit_disable(intel_crtc);
> --
> 2.1.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
