Re: [PATCH] drm/i915: Push vblank enable/disable past encoder->enable/disable

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 07 Jan 2015, Daniel Vetter <daniel.vetter@ffwll.ch> 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@ffwll.ch>
> 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@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx