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

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

On Mon, 16 Feb 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 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?

https://bugs.freedesktop.org/show_bug.cgi?id=89108

and probably

http://mid.gmane.org/20150131211609.GA3710@yulia-desktop


BR,
Jani.


>
> 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

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