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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > --- > > 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@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'. The whole mode->vrefesh story is a bit sad, and someone should really rework how it all works. > > > + 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@xxxxxxxxx> > > > + * > > + * 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx