[PATCH 13/15] drm/i915/tv: Generate better pipe timings for TV encoder

Imre Deak imre.deak at intel.com
Wed Jan 23 10:30:30 UTC 2019


On Tue, Jan 22, 2019 at 07:34:55PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 22, 2019 at 07:22:24PM +0200, Imre Deak wrote:
> > On Mon, Nov 12, 2018 at 06:59:58PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > To make vblank timestamps work better with the TV encoder let's
> > > scale the pipe timings such that the relationship between the
> > > TV active and TV blanking periods is mirrored in the
> > > corresponding pipe timings.
> > > 
> > > Note that in reality the pipe runs at a faster speed during the
> > > TV vblank, and correspondigly there are periods when the pipe
> > > is enitrely stopped. We pretend that this isn't the case and
> > > as such we incur some error in the vblank timestamps during
> > > the TV vblank. Further explanation of the issues in a big
> > > comment in the code.
> > > 
> > > This makes the vblank timestamps good enough to make
> > > i965gm (which doesn't have a working frame counter with
> > > the TV encoder) report correct frame numbers. Previously
> > > you could get all kinds of nonsense which resulted in
> > > eg. glxgears reporting that it's running at twice the
> > > actual framerate in most cases.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h |   1 +
> > >  drivers/gpu/drm/i915/intel_tv.c | 322 +++++++++++++++++++++++++++-----
> > >  2 files changed, 278 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index fe4b913e46ac..10813ae7bee7 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4848,6 +4848,7 @@ enum {
> > >  # define TV_OVERSAMPLE_NONE		(2 << 18)
> > >  /* Selects 8x oversampling */
> > >  # define TV_OVERSAMPLE_8X		(3 << 18)
> > > +# define TV_OVERSAMPLE_MASK		(3 << 18)
> > >  /* Selects progressive mode rather than interlaced */
> > >  # define TV_PROGRESSIVE			(1 << 17)
> > >  /* Sets the colorburst to PAL mode.  Required for non-M PAL modes. */
> > > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> > > index 216525dd144a..75126fce655d 100644
> > > --- a/drivers/gpu/drm/i915/intel_tv.c
> > > +++ b/drivers/gpu/drm/i915/intel_tv.c
> > > @@ -340,7 +340,6 @@ struct tv_mode {
> > >  	const struct video_levels *composite_levels, *svideo_levels;
> > >  	const struct color_conversion *composite_color, *svideo_color;
> > >  	const u32 *filter_table;
> > > -	u16 max_srcw;
> > >  };
> > >  
> > >  
> > > @@ -729,7 +728,6 @@ static const struct tv_mode tv_modes[] = {
> > >  		.burst_ena      = false,
> > >  
> > >  		.filter_table = filter_table,
> > > -		.max_srcw = 800
> > >  	},
> > >  	{
> > >  		.name       = "1080i at 50Hz",
> > > @@ -947,13 +945,183 @@ intel_tv_mode_vdisplay(const struct tv_mode *tv_mode)
> > >  		return 2 * (tv_mode->nbr_end + 1);
> > >  }
> > >  
> > > +static void
> > > +intel_tv_mode_to_mode(struct drm_display_mode *mode,
> > > +		      const struct tv_mode *tv_mode)
> > > +{
> > > +	mode->clock = tv_mode->clock /
> > > +		(tv_mode->oversample >> !tv_mode->progressive);
> > > +
> > > +	/*
> > > +	 * tv_mode horizontal timings:
> > > +	 *
> > > +	 * hsync_end
> > > +	 *    | hblank_end
> > > +	 *    |    | hblank_start
> > > +	 *    |    |       | htotal
> > > +	 *    |     _______    |
> > > +	 *     ____/       \___
> > > +	 * \__/                \
> > > +	 */
> > > +	mode->hdisplay =
> > > +		tv_mode->hblank_start - tv_mode->hblank_end;
> > > +	mode->hsync_start = mode->hdisplay +
> > > +		tv_mode->htotal - tv_mode->hblank_start;
> > > +	mode->hsync_end = mode->hsync_start +
> > > +		tv_mode->hsync_end;
> > > +	mode->htotal = tv_mode->htotal + 1;
> > > +
> > > +	/*
> > > +	 * tv_mode vertical timings:
> > > +	 *
> > > +	 * vsync_start
> > > +	 *    | vsync_end
> > > +	 *    |  | vi_end nbr_end
> > > +	 *    |  |    |       |
> > > +	 *    |  |     _______
> > > +	 * \__    ____/       \
> > > +	 *    \__/
> > > +	 */
> > > +	mode->vdisplay = intel_tv_mode_vdisplay(tv_mode);
> > > +	if (tv_mode->progressive) {
> > > +		mode->vsync_start = mode->vdisplay +
> > > +			tv_mode->vsync_start_f1 + 1;
> > > +		mode->vsync_end = mode->vsync_start +
> > > +			tv_mode->vsync_len;
> > > +		mode->vtotal = mode->vdisplay +
> > > +			tv_mode->vi_end_f1 + 1;
> > > +	} else {
> > > +		mode->vsync_start = mode->vdisplay +
> > > +			tv_mode->vsync_start_f1 + 1 +
> > > +			tv_mode->vsync_start_f2 + 1;
> > > +		mode->vsync_end = mode->vsync_start +
> > > +			2 * tv_mode->vsync_len;
> > > +		mode->vtotal = mode->vdisplay +
> > > +			tv_mode->vi_end_f1 + 1 +
> > > +			tv_mode->vi_end_f2 + 1;
> > > +	}
> > > +
> > > +	/* TV has it's own notion of sync and other mode flags, so clear them. */
> > > +	mode->flags = 0;
> > > +
> > > +	mode->vrefresh = 0;
> > 
> > Redundant line.
> 
> drm_mode_vrefresh() won't recompute the value if it's already
> set. So unless we clear it first this would effectively become
> 'mode->vrefresh = mode->vrefresh'.

Oops, correct.

> 
> The whole mode->vrefesh story is a bit sad, and someone should
> really rework how it all works.

Yea, would be nice.

> 
> > 
> > > +	mode->vrefresh = drm_mode_vrefresh(mode);
> > > +
> > > +	snprintf(mode->name, sizeof(mode->name),
> > > +		 "%dx%d%c (%s)",
> > > +		 mode->hdisplay, mode->vdisplay,
> > > +		 tv_mode->progressive ? 'p' : 'i',
> > > +		 tv_mode->name);
> > > +}
> > > +
> > > +static void intel_tv_scale_mode_horiz(struct drm_display_mode *mode,
> > > +				      int hdisplay, int left_margin,
> > > +				      int right_margin)
> > > +{
> > > +	int hsync_start = mode->hsync_start - mode->hdisplay + right_margin;
> > > +	int hsync_end = mode->hsync_end - mode->hdisplay + right_margin;
> > > +	int new_htotal = mode->htotal * hdisplay /
> > > +		(mode->hdisplay - left_margin - right_margin);
> > > +
> > > +	mode->clock = mode->clock * new_htotal / mode->htotal;
> > > +
> > > +	mode->hdisplay = hdisplay;
> > > +	mode->hsync_start = hdisplay + hsync_start * new_htotal / mode->htotal;
> > > +	mode->hsync_end = hdisplay + hsync_end * new_htotal / mode->htotal;
> > > +	mode->htotal = new_htotal;
> > > +}
> > > +
> > > +static void intel_tv_scale_mode_vert(struct drm_display_mode *mode,
> > > +				     int vdisplay, int top_margin,
> > > +				     int bottom_margin)
> > > +{
> > > +	int vsync_start = mode->vsync_start - mode->vdisplay + bottom_margin;
> > > +	int vsync_end = mode->vsync_end - mode->vdisplay + bottom_margin;
> > > +	int new_vtotal = mode->vtotal * vdisplay /
> > > +		(mode->vdisplay - top_margin - bottom_margin);
> > > +
> > > +	mode->clock = mode->clock * new_vtotal / mode->vtotal;
> > > +
> > > +	mode->vdisplay = vdisplay;
> > > +	mode->vsync_start = vdisplay + vsync_start * new_vtotal / mode->vtotal;
> > > +	mode->vsync_end = vdisplay + vsync_end * new_vtotal / mode->vtotal;
> > > +	mode->vtotal = new_vtotal;
> > > +}
> > > +
> > >  static void
> > >  intel_tv_get_config(struct intel_encoder *encoder,
> > >  		    struct intel_crtc_state *pipe_config)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	struct drm_display_mode *adjusted_mode =
> > > +		&pipe_config->base.adjusted_mode;
> > > +	struct drm_display_mode mode = {};
> > > +	u32 tv_ctl, hctl1, hctl3, vctl1, vctl2, tmp;
> > > +	struct tv_mode tv_mode = {};
> > > +	int hdisplay = adjusted_mode->crtc_hdisplay;
> > > +	int vdisplay = adjusted_mode->crtc_vdisplay;
> > > +	int xsize, ysize, xpos, ypos;
> > > +
> > >  	pipe_config->output_types |= BIT(INTEL_OUTPUT_TVOUT);
> > >  
> > > -	pipe_config->base.adjusted_mode.crtc_clock = pipe_config->port_clock;
> > > +	tv_ctl = I915_READ(TV_CTL);
> > > +	hctl1 = I915_READ(TV_H_CTL_1);
> > > +	hctl3 = I915_READ(TV_H_CTL_3);
> > > +	vctl1 = I915_READ(TV_V_CTL_1);
> > > +	vctl2 = I915_READ(TV_V_CTL_2);
> > > +
> > > +	tv_mode.htotal = (hctl1 & TV_HTOTAL_MASK) >> TV_HTOTAL_SHIFT;
> > > +	tv_mode.hsync_end = (hctl1 & TV_HSYNC_END_MASK) >> TV_HSYNC_END_SHIFT;
> > > +
> > > +	tv_mode.hblank_start = (hctl3 & TV_HBLANK_START_MASK) >> TV_HBLANK_START_SHIFT;
> > > +	tv_mode.hblank_end = (hctl3 & TV_HSYNC_END_MASK) >> TV_HBLANK_END_SHIFT;
> > > +
> > > +	tv_mode.nbr_end = (vctl1 & TV_NBR_END_MASK) >> TV_NBR_END_SHIFT;
> > > +	tv_mode.vi_end_f1 = (vctl1 & TV_VI_END_F1_MASK) >> TV_VI_END_F1_SHIFT;
> > > +	tv_mode.vi_end_f2 = (vctl1 & TV_VI_END_F2_MASK) >> TV_VI_END_F2_SHIFT;
> > > +
> > > +	tv_mode.vsync_len = (vctl2 & TV_VSYNC_LEN_MASK) >> TV_VSYNC_LEN_SHIFT;
> > > +	tv_mode.vsync_start_f1 = (vctl2 & TV_VSYNC_START_F1_MASK) >> TV_VSYNC_START_F1_SHIFT;
> > > +	tv_mode.vsync_start_f2 = (vctl2 & TV_VSYNC_START_F2_MASK) >> TV_VSYNC_START_F2_SHIFT;
> > > +
> > > +	tv_mode.clock = pipe_config->port_clock;
> > > +
> > > +	tv_mode.progressive = tv_ctl & TV_PROGRESSIVE;
> > > +
> > > +	switch (tv_ctl & TV_OVERSAMPLE_MASK) {
> > > +	case TV_OVERSAMPLE_8X:
> > > +		tv_mode.oversample = 8;
> > > +		break;
> > > +	case TV_OVERSAMPLE_4X:
> > > +		tv_mode.oversample = 4;
> > > +		break;
> > > +	case TV_OVERSAMPLE_2X:
> > > +		tv_mode.oversample = 2;
> > > +		break;
> > > +	default:
> > > +		tv_mode.oversample = 1;
> > > +		break;
> > > +	}
> > > +
> > > +	tmp = I915_READ(TV_WIN_POS);
> > > +	xpos = tmp >> 16;
> > > +	ypos = tmp & 0xffff;
> > > +
> > > +	tmp = I915_READ(TV_WIN_SIZE);
> > > +	xsize = tmp >> 16;
> > > +	ysize = tmp & 0xffff;
> > > +
> > > +	intel_tv_mode_to_mode(&mode, &tv_mode);
> > > +
> > > +	DRM_DEBUG_KMS("TV mode:\n");
> > > +	drm_mode_debug_printmodeline(&mode);
> > > +
> > > +	intel_tv_scale_mode_horiz(&mode, hdisplay,
> > > +				  xpos, mode.hdisplay - xsize - xpos);
> > > +	intel_tv_scale_mode_vert(&mode, vdisplay,
> > > +				 ypos, mode.vdisplay - ysize - ypos);
> > > +
> > > +	adjusted_mode->crtc_clock = mode.clock;
> > >  }
> > >  
> > >  static bool
> > > @@ -964,6 +1132,8 @@ intel_tv_compute_config(struct intel_encoder *encoder,
> > >  	const struct tv_mode *tv_mode = intel_tv_mode_find(conn_state);
> > >  	struct drm_display_mode *adjusted_mode =
> > >  		&pipe_config->base.adjusted_mode;
> > > +	int hdisplay = adjusted_mode->crtc_hdisplay;
> > > +	int vdisplay = adjusted_mode->crtc_vdisplay;
> > >  
> > >  	if (!tv_mode)
> > >  		return false;
> > > @@ -972,17 +1142,90 @@ intel_tv_compute_config(struct intel_encoder *encoder,
> > >  		return false;
> > >  
> > >  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> > > -	adjusted_mode->crtc_clock = tv_mode->clock;
> > > +
> > >  	DRM_DEBUG_KMS("forcing bpc to 8 for TV\n");
> > >  	pipe_config->pipe_bpp = 8*3;
> > >  
> > > -	/* TV has it's own notion of sync and other mode flags, so clear them. */
> > > -	adjusted_mode->flags = 0;
> > > +	pipe_config->port_clock = tv_mode->clock;
> > > +
> > > +	intel_tv_mode_to_mode(adjusted_mode, tv_mode);
> > > +
> > > +	DRM_DEBUG_KMS("TV mode:\n");
> > > +	drm_mode_debug_printmodeline(adjusted_mode);
> > >  
> > >  	/*
> > > -	 * FIXME: We don't check whether the input mode is actually what we want
> > > -	 * or whether userspace is doing something stupid.
> > > +	 * The pipe scanline counter behaviour looks as follows when
> > > +	 * using the TV encoder:
> > > +	 *
> > > +	 * time ->
> > > +	 *
> > > +	 * dsl=vtotal-1       |             |
> > > +	 *                   ||            ||
> > > +	 *               ___| |        ___| |
> > > +	 *              /     |       /     |
> > > +	 *             /      |      /      |
> > > +	 * dsl=0   ___/       |_____/       |
> > > +	 *        | | |  |  | |
> > > +	 *         ^ ^ ^   ^ ^
> > > +	 *         | | |   | pipe vblank/first part of tv vblank
> > > +	 *         | | |   bottom margin
> > > +	 *         | | active
> > > +	 *         | top margin
> > > +	 *         remainder of tv vblank
> > > +	 *
> > > +	 * When the TV encoder is used the pipe wants to run faster
> > > +	 * than expected rate. During the active portion the TV
> > > +	 * encoder stalls the pipe every few lines to keep it in
> > > +	 * check. When the TV encoder reaches the bottom margin the
> > > +	 * pipe simply stops. Once we reach the TV vblank the pipe is
> > > +	 * no longer stalled and it runs at the max rate (apparently
> > > +	 * oversample clock on gen3, cdclk on gen4). Once the pipe
> > > +	 * reaches the pipe vtotal the pipe stops for the remainder
> > > +	 * of the TV vblank/top margin. The pipe starts up again when
> > > +	 * the TV encoder exits the top margin.
> > > +	 *
> > > +	 * To avoid huge hassles for vblank timestamping we scale
> > > +	 * the pipe timings as if the pipe always runs at the average
> > > +	 * rate it maintains during the active period. This also
> > > +	 * gives us a reasonable guesstimate as to the pixel rate.
> > > +	 * Due to the variation in the actual pipe speed the scanline
> > > +	 * counter will give us slightly erroneous results during the
> > > +	 * TV vblank/margins. But since vtotal was selected such that
> > > +	 * it matches the average rate of the pipe during the active
> > > +	 * portion the error shouldn't cause any serious grief to
> > > +	 * vblank timestamps.
> > 
> > Nice rev. enging and description. Was thinking for a while what is the
> > max error the isssue you found adds wrt. the actual vblank timestamp,
> > after your change. AFAIU it's the duration of the flats in the curve,
> > that is
> > max_err=+(bottom margin+remainder of tv vblank+top margin).
> > 
> > Reviewed-by: Imre Deak <imre.deak at intel.com>
> > 
> > > +	 *
> > > +	 * For posterity here is the empirically derived formula
> > > +	 * that gives us the maximum length of the pipe vblank
> > > +	 * we can use without causing display corruption. Following
> > > +	 * this would allow us to have a ticking scanline counter
> > > +	 * everywhere except during the bottom margin (there the
> > > +	 * pipe always stops). Ie. this would eliminate the second
> > > +	 * flat portion of the above graph. However this would also
> > > +	 * complicate vblank timestamping as the pipe vtotal would
> > > +	 * no longer match the average rate the pipe runs at during
> > > +	 * the active portion. Hence following this formula seems
> > > +	 * more trouble that it's worth.
> > > +	 *
> > > +	 * if (IS_GEN4(dev_priv)) {
> > > +	 *	num = cdclk * (tv_mode->oversample >> !tv_mode->progressive);
> > > +	 *	den = tv_mode->clock;
> > > +	 * } else {
> > > +	 *	num = tv_mode->oversample >> !tv_mode->progressive;
> > > +	 *	den = 1;
> > > +	 * }
> > > +	 * max_pipe_vblank_len ~=
> > > +	 *	(num * tv_htotal * (tv_vblank_len + top_margin)) /
> > > +	 *	(den * pipe_htotal);
> > >  	 */
> > > +	intel_tv_scale_mode_horiz(adjusted_mode, hdisplay,
> > > +				  conn_state->tv.margins.left,
> > > +				  conn_state->tv.margins.right);
> > > +	intel_tv_scale_mode_vert(adjusted_mode, vdisplay,
> > > +				 conn_state->tv.margins.top,
> > > +				 conn_state->tv.margins.bottom);
> > > +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> > > +	adjusted_mode->name[0] = '\0';
> > >  
> > >  	return true;
> > >  }
> > > @@ -1411,52 +1654,41 @@ intel_tv_set_mode_type(struct drm_display_mode *mode,
> > >  static int
> > >  intel_tv_get_modes(struct drm_connector *connector)
> > >  {
> > > -	struct drm_display_mode *mode_ptr;
> > >  	const struct tv_mode *tv_mode = intel_tv_mode_find(connector->state);
> > > -	int j, count = 0;
> > > -	u64 tmp;
> > > +	int i, count = 0;
> > >  
> > > -	for (j = 0; j < ARRAY_SIZE(input_res_table);
> > > -	     j++) {
> > > -		const struct input_res *input = &input_res_table[j];
> > > -		unsigned int hactive_s = input->w;
> > > -		unsigned int vactive_s = input->h;
> > > -
> > > -		if (tv_mode->max_srcw && input->w > tv_mode->max_srcw)
> > > -			continue;
> > > +	for (i = 0; i < ARRAY_SIZE(input_res_table); i++) {
> > > +		const struct input_res *input = &input_res_table[i];
> > > +		struct drm_display_mode *mode;
> > >  
> > > -		if (input->w > 1024 && (!tv_mode->progressive
> > > -					&& !tv_mode->component_only))
> > > +		if (input->w > 1024 &&
> > > +		    !tv_mode->progressive &&
> > > +		    !tv_mode->component_only)
> > >  			continue;
> > >  
> > > -		mode_ptr = drm_mode_create(connector->dev);
> > > -		if (!mode_ptr)
> > > +		mode = drm_mode_create(connector->dev);
> > > +		if (!mode)
> > >  			continue;
> > >  
> > > -		mode_ptr->hdisplay = hactive_s;
> > > -		mode_ptr->hsync_start = hactive_s + 1;
> > > -		mode_ptr->hsync_end = hactive_s + 64;
> > > -		if (mode_ptr->hsync_end <= mode_ptr->hsync_start)
> > > -			mode_ptr->hsync_end = mode_ptr->hsync_start + 1;
> > > -		mode_ptr->htotal = hactive_s + 96;
> > > -
> > > -		mode_ptr->vdisplay = vactive_s;
> > > -		mode_ptr->vsync_start = vactive_s + 1;
> > > -		mode_ptr->vsync_end = vactive_s + 32;
> > > -		if (mode_ptr->vsync_end <= mode_ptr->vsync_start)
> > > -			mode_ptr->vsync_end = mode_ptr->vsync_start  + 1;
> > > -		mode_ptr->vtotal = vactive_s + 33;
> > > -
> > > -		tmp = mul_u32_u32(tv_mode->refresh, mode_ptr->vtotal);
> > > -		tmp *= mode_ptr->htotal;
> > > -		tmp = div_u64(tmp, 1000000);
> > > -		mode_ptr->clock = (int) tmp;
> > > -
> > > -		intel_tv_set_mode_type(mode_ptr, tv_mode);
> > > +		/*
> > > +		 * We take the TV mode and scale it to look
> > > +		 * like it had the expected h/vdisplay. This
> > > +		 * provides the most information to userspace
> > > +		 * about the actual timings of the mode. We
> > > +		 * do ignore the margins though.
> > > +		 */
> > > +		intel_tv_mode_to_mode(mode, tv_mode);
> > > +		if (count == 0) {
> > > +			DRM_DEBUG_KMS("TV mode:\n");
> > > +			drm_mode_debug_printmodeline(mode);
> > > +		}
> > > +		intel_tv_scale_mode_horiz(mode, input->w, 0, 0);
> > > +		intel_tv_scale_mode_vert(mode, input->h, 0, 0);
> > > +		intel_tv_set_mode_type(mode, tv_mode);
> > >  
> > > -		drm_mode_set_name(mode_ptr);
> > > +		drm_mode_set_name(mode);
> > >  
> > > -		drm_mode_probed_add(connector, mode_ptr);
> > > +		drm_mode_probed_add(connector, mode);
> > >  		count++;
> > >  	}
> > >  
> > > -- 
> > > 2.18.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel


More information about the dri-devel mailing list