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

Jani Nikula
2015-02-23T12:56:20Z

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