Re: [PATCH v3 02/15] drm/i915: Don't try to use the hardware frame counter with i965gm TV output

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 27, 2018 at 10:05:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> On i965gm the hardware frame counter does not work when
> the TV encoder is active. So let's not try to consult
> the hardware frame counter in that case. Instead we'll
> fall back to the timestamp based guesstimation method
> used on gen2.
> 
> Note that the pipe timings generated by the TV encoder
> are also rather peculiar. Apparently the pipe wants to
> run at a much higher speed (related to the oversample
> clock somehow it seems) but during the vertical active
> period the TV encoder stalls the pipe every few lines
> to keep its speed in check. But once the vertical
> blanking period is reached the pipe gets to run at full
> speed. This means our vblank timestamp estimates are
> suspect. Fixing all that would require quite a bit
> more work. This simple fix at least avoids the nasty
> vblank timeouts that are happening currently.
> 
> Curiously the frame counter works just fine on i945gm
> and gm45. I don't really understand what kind of mishap
> occurred with the hardware design on i965gm. Sadly
> I wasn't able to find any chicken bits etc. that would
> fix the frame counter :(
> 
> v2: Move the zero vs. non-zero hw counter value handling
>     into i915_get_vblank_counter() (Daniel)
>     Use the per-crtc maximum exclusively, leaving the
>     per-device maximum at zero
> v3: max_vblank_count not populated yet in intel_enable_pipe()
>     use intel_crtc_max_vblank_count() instead
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Fixes: 51e31d49c890 ("drm/i915: Use generic vblank wait")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93782
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> fix#	_slub_broken.S
  ^ some remnant

Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 27 ++++++++++------
>  drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++-----
>  2 files changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d447d7d508f4..ab2d4eefef18 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -822,11 +822,26 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>  static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	const struct drm_display_mode *mode = &vblank->hwmode;
>  	i915_reg_t high_frame, low_frame;
>  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -	const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
>  	unsigned long irqflags;
>  
> +	/*
> +	 * On i965gm TV output the frame counter only works up to
> +	 * the point when we enable the TV encoder. After that the
> +	 * frame counter ceases to work and reads zero. We need a
> +	 * vblank wait before enabling the TV encoder and so we
> +	 * have to enable vblank interrupts while the frame counter
> +	 * is still in a working state. However the core vblank code
> +	 * does not like us returning non-zero frame counter values
> +	 * when we've told it that we don't have a working frame
> +	 * counter. Thus we must stop non-zero values leaking out.
> +	 */
> +	if (!vblank->max_vblank_count)
> +		return 0;
> +
>  	htotal = mode->crtc_htotal;
>  	hsync_start = mode->crtc_hsync_start;
>  	vbl_start = mode->crtc_vblank_start;
> @@ -4836,16 +4851,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		rps->pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC;
>  
> -	if (IS_GEN2(dev_priv)) {
> -		/* Gen2 doesn't have a hardware frame counter */
> -		dev->max_vblank_count = 0;
> -	} else if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5) {
> -		dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
> +	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
>  		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
> -	} else {
> +	else if (INTEL_GEN(dev_priv) >= 3)
>  		dev->driver->get_vblank_counter = i915_get_vblank_counter;
> -		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> -	}
>  
>  	/*
>  	 * Opt out of the vblank disable timer on everything except gen2.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9f4e22b2a4e..cb13eff78ad9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1754,6 +1754,35 @@ enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc)
>  		return crtc->pipe;
>  }
>  
> +static u32 intel_crtc_max_vblank_count(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> +	/*
> +	 * On i965gm the hardware frame counter reads
> +	 * zero when the TV encoder is enabled :(
> +	 */
> +	if (IS_I965GM(dev_priv) &&
> +	    (crtc_state->output_types & BIT(INTEL_OUTPUT_TVOUT)))
> +		return 0;
> +
> +	if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
> +		return 0xffffffff; /* full 32 bit counter */
> +	else if (INTEL_GEN(dev_priv) >= 3)
> +		return 0xffffff; /* only 24 bits of frame count */
> +	else
> +		return 0; /* Gen2 doesn't have a hardware frame counter */
> +}
> +
> +static void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +
> +	drm_crtc_set_max_vblank_count(&crtc->base,
> +				      intel_crtc_max_vblank_count(crtc_state));
> +	drm_crtc_vblank_on(&crtc->base);
> +}
> +
>  static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> @@ -1806,7 +1835,7 @@ static void intel_enable_pipe(const struct intel_crtc_state *new_crtc_state)
>  	 * when it's derived from the timestamps. So let's wait for the
>  	 * pipe to start properly before we call drm_crtc_vblank_on()
>  	 */
> -	if (dev_priv->drm.max_vblank_count == 0)
> +	if (intel_crtc_max_vblank_count(new_crtc_state) == 0)
>  		intel_wait_for_pipe_scanline_moving(crtc);
>  }
>  
> @@ -5617,7 +5646,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>  		ironlake_pch_enable(old_intel_state, pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  
> @@ -5774,7 +5803,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  		intel_ddi_set_vc_payload_alloc(pipe_config, true);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  
> @@ -6114,7 +6143,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_enable_pipe(pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  }
> @@ -6173,7 +6202,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_enable_pipe(pipe_config);
>  
>  	assert_vblank_disabled(crtc);
> -	drm_crtc_vblank_on(crtc);
> +	intel_crtc_vblank_on(pipe_config);
>  
>  	intel_encoders_enable(crtc, pipe_config, old_state);
>  }
> @@ -12653,8 +12682,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(&crtc->base)];
>  
> -	if (!dev->max_vblank_count)
> +	if (!vblank->max_vblank_count)
>  		return (u32)drm_crtc_accurate_vblank_count(&crtc->base);
>  
>  	return dev->driver->get_vblank_counter(dev, crtc->pipe);
> @@ -15736,10 +15766,12 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	 * waits, so we need vblank interrupts restored beforehand.
>  	 */
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +
>  		drm_crtc_vblank_reset(&crtc->base);
>  
> -		if (crtc->base.state->active)
> -			drm_crtc_vblank_on(&crtc->base);
> +		if (crtc_state->base.active)
> +			intel_crtc_vblank_on(crtc_state);
>  	}
>  
>  	intel_sanitize_plane_mapping(dev_priv);
> -- 
> 2.18.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux