Hi Eric and all,
this batch prevents setting modes one shouldn't set, adds precise vblank timestamping for interlaced video modes, and one fix for vblank en/disable during crtc en/disable.
All successfully tested, also with timing measurement equipment, on the RPi 2B.
Thanks, -mario
We already don't expose such modes to userspace, but make sure userspace can't sneak some interlaced mode in.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_dpi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 275fedb..1e1f6b8 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -340,9 +340,20 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder) } }
+static bool vc4_dpi_encoder_mode_fixup(struct drm_encoder *encoder, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) + return false; + + return true; +} + static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = { .disable = vc4_dpi_encoder_disable, .enable = vc4_dpi_encoder_enable, + .mode_fixup = vc4_dpi_encoder_mode_fixup, };
static const struct of_device_id vc4_dpi_dt_match[] = {
We must not apply CRTC_INTERLACE_HALVE_V to interlaced modes during mode enumeration, as drm_helper_probe_single_connector_modes does, so wrap it and reset the effect of CRTC_INTERLACE_HALVE_V on affected interlaced modes.
Also mode_fixup interlaced modes passed in from user space.
This fixes the vblank timestamping constants and entries in the mode->crtc_xxx fields needed for precise vblank timestamping.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_crtc.c | 18 ++++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 8fc2b73..a479d3d 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -532,6 +532,23 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); }
+static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + /* + * Interlaced video modes got CRTC_INTERLACE_HALVE_V applied when + * coming from user space. We don't want this, as it screws up + * vblank timestamping, so fix it up. + */ + drm_mode_set_crtcinfo(adjusted_mode, 0); + + DRM_DEBUG_KMS("[CRTC:%d] adjusted_mode :\n", crtc->base.id); + drm_mode_debug_printmodeline(adjusted_mode); + + return true; +} + static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -819,6 +836,7 @@ static const struct drm_crtc_helper_funcs vc4_crtc_helper_funcs = { .mode_set_nofb = vc4_crtc_mode_set_nofb, .disable = vc4_crtc_disable, .enable = vc4_crtc_enable, + .mode_fixup = vc4_crtc_mode_fixup, .atomic_check = vc4_crtc_atomic_check, .atomic_flush = vc4_crtc_atomic_flush, }; diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 4452f36..68ad106 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -208,10 +208,35 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) return ret; }
+/* + * drm_helper_probe_single_connector_modes() applies drm_mode_set_crtcinfo to + * all modes with flag CRTC_INTERLACE_HALVE_V. We don't want this, as it + * screws up vblank timestamping for interlaced modes, so fix it up. + */ +static int vc4_hdmi_connector_probe_modes(struct drm_connector *connector, + uint32_t maxX, uint32_t maxY) +{ + struct drm_display_mode *mode; + int count; + + count = drm_helper_probe_single_connector_modes(connector, maxX, maxY); + if (count == 0) + return 0; + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed adapted modes :\n", + connector->base.id, connector->name); + list_for_each_entry(mode, &connector->modes, head) { + drm_mode_set_crtcinfo(mode, 0); + drm_mode_debug_printmodeline(mode); + } + + return count; +} + static const struct drm_connector_funcs vc4_hdmi_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms, .detect = vc4_hdmi_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, + .fill_modes = vc4_hdmi_connector_probe_modes, .destroy = vc4_hdmi_connector_destroy, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, @@ -246,7 +271,7 @@ static struct drm_connector *vc4_hdmi_connector_init(struct drm_device *dev, connector->polled = (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT);
- connector->interlace_allowed = 0; + connector->interlace_allowed = 1; connector->doublescan_allowed = 0;
drm_mode_connector_attach_encoder(connector, encoder);
We can't handle doublescan modes at the moment, so if userspace tries to set one, reject the mode set.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_crtc.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index a479d3d..2bfa247 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -536,6 +536,13 @@ static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode) { + /* Do not allow doublescan modes from user space */ + if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN) { + DRM_DEBUG_KMS("[CRTC:%d] Doublescan mode rejected.\n", + crtc->base.id); + return false; + } + /* * Interlaced video modes got CRTC_INTERLACE_HALVE_V applied when * coming from user space. We don't want this, as it screws up
On top of the interlaced video mode fix and with some additional adjustments, this now works well. It has almost the same accuracy as on regular progressive scan modes.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_crtc.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 2bfa247..7ffdad5 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -163,14 +163,6 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id, int vblank_lines; int ret = 0;
- /* - * XXX Doesn't work well in interlaced mode yet, partially due - * to problems in vc4 kms or drm core interlaced mode handling, - * so disable for now in interlaced mode. - */ - if (mode->flags & DRM_MODE_FLAG_INTERLACE) - return ret; - /* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
/* Get optional system timestamp before query. */ @@ -191,10 +183,15 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
/* Vertical position of hvs composed scanline. */ *vpos = VC4_GET_FIELD(val, SCALER_DISPSTATX_LINE); + *hpos = 0; + + if (mode->flags & DRM_MODE_FLAG_INTERLACE) { + *vpos /= 2;
- /* No hpos info available. */ - if (hpos) - *hpos = 0; + /* Use hpos to correct for field offset in interlaced mode. */ + if (VC4_GET_FIELD(val, SCALER_DISPSTATX_FRAME_COUNT) % 2) + *hpos += mode->crtc_htotal / 2; + }
/* This is the offset we need for translating hvs -> pv scanout pos. */ fifo_lines = vc4_crtc->cob_size / mode->crtc_hdisplay; @@ -217,8 +214,6 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id, * position of the PV. */ *vpos -= fifo_lines + 1; - if (mode->flags & DRM_MODE_FLAG_INTERLACE) - *vpos /= 2;
ret |= DRM_SCANOUTPOS_ACCURATE; return ret;
Add missing drm_crtc_vblank_on/off() calls so vblank irq handling/updating/timestamping never runs with a crtc shut down or during its shutdown/startup, as that causes large jumps in vblank count and trouble for compositors.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Eric Anholt eric@anholt.net --- drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 7ffdad5..2682f07 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -475,6 +475,9 @@ static void vc4_crtc_disable(struct drm_crtc *crtc) int ret; require_hvs_enabled(dev);
+ /* Disable vblank irq handling before crtc is disabled. */ + drm_crtc_vblank_off(crtc); + CRTC_WRITE(PV_V_CONTROL, CRTC_READ(PV_V_CONTROL) & ~PV_VCONTROL_VIDEN); ret = wait_for(!(CRTC_READ(PV_V_CONTROL) & PV_VCONTROL_VIDEN), 1); @@ -525,6 +528,9 @@ static void vc4_crtc_enable(struct drm_crtc *crtc) /* Turn on the pixel valve, which will emit the vstart signal. */ CRTC_WRITE(PV_V_CONTROL, CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); + + /* Enable vblank irq handling after crtc is started. */ + drm_crtc_vblank_on(crtc); }
static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc,
Mario Kleiner mario.kleiner.de@gmail.com writes:
Hi Eric and all,
this batch prevents setting modes one shouldn't set, adds precise vblank timestamping for interlaced video modes, and one fix for vblank en/disable during crtc en/disable.
All successfully tested, also with timing measurement equipment, on the RPi 2B.
This all seems pretty reasonable to me. My question for modesetting gurus would be: Is mode_fixup the place to be filtering out unsupportable modes from the user (or their displays)?
On Thu, Jul 21, 2016 at 12:58:16PM -0700, Eric Anholt wrote:
Mario Kleiner mario.kleiner.de@gmail.com writes:
Hi Eric and all,
this batch prevents setting modes one shouldn't set, adds precise vblank timestamping for interlaced video modes, and one fix for vblank en/disable during crtc en/disable.
All successfully tested, also with timing measurement equipment, on the RPi 2B.
This all seems pretty reasonable to me. My question for modesetting gurus would be: Is mode_fixup the place to be filtering out unsupportable modes from the user (or their displays)?
Yup. And I wanted to forever add some glue so that drivers could share the same validation logic between mode_fixup (for modesets) and mode_valid (for probing). But never came up with a helper scaffolding that looked satisfactory. The icky bit is that mode_fixup wants an adjusted mode, and we'd need to fake that somehow to reuse the same code in mode_valid. It's even worse for atomic, where atomic_check (the new mode_fixup) gets an entire atomic state. But I still think for simpler hw (anything which doesn't have funny crtc/encoder specific restrictions) we should be able to create something that just works. -Daniel
Mario Kleiner mario.kleiner.de@gmail.com writes:
Hi Eric and all,
this batch prevents setting modes one shouldn't set, adds precise vblank timestamping for interlaced video modes, and one fix for vblank en/disable during crtc en/disable.
All successfully tested, also with timing measurement equipment, on the RPi 2B.
I finally got around to making new -next branches, and pulled all of these to drm-vc4-next. Thanks!
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Eric Anholt -
Mario Kleiner