| Message ID | 20170503072638.16826-6-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 03, 2017 at 09:26:38AM +0200, Daniel Vetter wrote: > In the previous patch we've implemented hwmode tracking a la i915 for > the vblank timestamp calculations. But that was just the basic > semantics, i915 has some nice sanity checks to make sure we keep > getting this right. Move them over too. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_irq.c | 8 +++++++- > drivers/gpu/drm/i915/i915_irq.c | 10 ++++++---- > drivers/gpu/drm/i915/intel_display.c | 11 ++--------- > 3 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 89f0928b042a..942183a2aa3c 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -775,8 +775,10 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, > /* If mode timing undefined, just return as no-op: > * Happens during initial modesetting of a crtc. > */ > - if (mode->crtc_clock == 0) { > + if (WARN_ON(mode->crtc_clock == 0)) { > DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > + WARN_ON(drm_drv_uses_atomic_modeset(dev)); I would make these _ONCE() otherwise the machine might end up practically dead. > + > return false; > } > > @@ -1338,6 +1340,10 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) > send_vblank_event(dev, e, seq, &now); > } > spin_unlock_irqrestore(&dev->event_lock, irqflags); > + > + /* Will be reset by the modeset helpers when re-enabling the crtc by > + * calling drm_calc_timestamping_constants(). */ > + vblank->hwmode.crtc_clock = 0; > } > EXPORT_SYMBOL(drm_crtc_vblank_off); Shouldn't we do this in drm_crtc_vblank_reset() as well? Hmm. Except we call that after drm_calc_timestamping_constants(). I guess we should be able to move the reset() into intel_modeset_readout_hw_state(). And possibly move the vblank_on() call as well? > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 837e5bc2019a..ff18b3fc00d2 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -720,9 +720,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe) > struct drm_i915_private *dev_priv = to_i915(dev); > i915_reg_t high_frame, low_frame; > u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; > - struct intel_crtc *intel_crtc = intel_get_crtc_for_pipe(dev_priv, > - pipe); > - const struct drm_display_mode *mode = &intel_crtc->base.hwmode; > + const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode; > unsigned long irqflags; > > htotal = mode->crtc_htotal; > @@ -779,13 +777,17 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > - const struct drm_display_mode *mode = &crtc->base.hwmode; > + const struct drm_display_mode *mode; > + struct drm_vblank_crtc *vblank; > enum pipe pipe = crtc->pipe; > int position, vtotal; > > if (!crtc->active) > return -1; > > + vblank = &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)]; > + mode = &vblank->hwmode; > + > vtotal = mode->crtc_vtotal; > if (mode->flags & DRM_MODE_FLAG_INTERLACE) > vtotal /= 2; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 85b9e2f521a0..a190973daeee 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11443,12 +11443,6 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state); > > - /* Update hwmode for vblank functions */ > - if (new_crtc_state->active) > - crtc->hwmode = new_crtc_state->adjusted_mode; > - else > - crtc->hwmode.crtc_clock = 0; > - > /* > * Update legacy state to satisfy fbc code. This can > * be removed when fbc uses the atomic state. > @@ -15425,8 +15419,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > to_intel_crtc_state(crtc->base.state); > int pixclk = 0; > > - crtc->base.hwmode = crtc_state->base.adjusted_mode; > - > memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > if (crtc_state->base.active) { > intel_mode_from_pipe_config(&crtc->base.mode, crtc_state); > @@ -15456,7 +15448,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) > pixclk = DIV_ROUND_UP(pixclk * 100, 95); > > - drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); > + drm_calc_timestamping_constants(&crtc->base, > + &crtc_state->base.adjusted_mode); > update_scanline_offset(crtc); > } > > -- > 2.11.0
On Wed, May 03, 2017 at 05:09:08PM +0300, Ville Syrjälä wrote: > On Wed, May 03, 2017 at 09:26:38AM +0200, Daniel Vetter wrote: > > In the previous patch we've implemented hwmode tracking a la i915 for > > the vblank timestamp calculations. But that was just the basic > > semantics, i915 has some nice sanity checks to make sure we keep > > getting this right. Move them over too. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_irq.c | 8 +++++++- > > drivers/gpu/drm/i915/i915_irq.c | 10 ++++++---- > > drivers/gpu/drm/i915/intel_display.c | 11 ++--------- > > 3 files changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 89f0928b042a..942183a2aa3c 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -775,8 +775,10 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, > > /* If mode timing undefined, just return as no-op: > > * Happens during initial modesetting of a crtc. > > */ > > - if (mode->crtc_clock == 0) { > > + if (WARN_ON(mode->crtc_clock == 0)) { > > DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > > + WARN_ON(drm_drv_uses_atomic_modeset(dev)); > > I would make these _ONCE() otherwise the machine might end up > practically dead. Will do. > > + > > return false; > > } > > > > @@ -1338,6 +1340,10 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) > > send_vblank_event(dev, e, seq, &now); > > } > > spin_unlock_irqrestore(&dev->event_lock, irqflags); > > + > > + /* Will be reset by the modeset helpers when re-enabling the crtc by > > + * calling drm_calc_timestamping_constants(). */ > > + vblank->hwmode.crtc_clock = 0; > > } > > EXPORT_SYMBOL(drm_crtc_vblank_off); > > Shouldn't we do this in drm_crtc_vblank_reset() as well? > > Hmm. Except we call that after drm_calc_timestamping_constants(). I > guess we should be able to move the reset() into > intel_modeset_readout_hw_state(). And possibly move the vblank_on() > call as well? Yeah, it'd be nice to clean this stuff up some more, but there's also the problem that legacy and new drivers callc drm_calc_timestamping_constants at opposite ends of the modeset sequence. Doing more here is a bunch more work, maybe for the next patche series ... I don't think we need to call it in _reset, at least at boot-up it should be 0 already. And for s/r we already shut down the pipe on suspend, so it's gone through this here. With the _ONCE nit address (and the build breakage I've introduced in this version fixed), ack from you on the entire series? Thanks, Daniel
On Thu, May 04, 2017 at 03:20:22PM +0200, Daniel Vetter wrote: > On Wed, May 03, 2017 at 05:09:08PM +0300, Ville Syrjälä wrote: > > On Wed, May 03, 2017 at 09:26:38AM +0200, Daniel Vetter wrote: > > > In the previous patch we've implemented hwmode tracking a la i915 for > > > the vblank timestamp calculations. But that was just the basic > > > semantics, i915 has some nice sanity checks to make sure we keep > > > getting this right. Move them over too. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_irq.c | 8 +++++++- > > > drivers/gpu/drm/i915/i915_irq.c | 10 ++++++---- > > > drivers/gpu/drm/i915/intel_display.c | 11 ++--------- > > > 3 files changed, 15 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > > index 89f0928b042a..942183a2aa3c 100644 > > > --- a/drivers/gpu/drm/drm_irq.c > > > +++ b/drivers/gpu/drm/drm_irq.c > > > @@ -775,8 +775,10 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, > > > /* If mode timing undefined, just return as no-op: > > > * Happens during initial modesetting of a crtc. > > > */ > > > - if (mode->crtc_clock == 0) { > > > + if (WARN_ON(mode->crtc_clock == 0)) { > > > DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > > > + WARN_ON(drm_drv_uses_atomic_modeset(dev)); > > > > I would make these _ONCE() otherwise the machine might end up > > practically dead. > > Will do. > > > > + > > > return false; > > > } > > > > > > @@ -1338,6 +1340,10 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) > > > send_vblank_event(dev, e, seq, &now); > > > } > > > spin_unlock_irqrestore(&dev->event_lock, irqflags); > > > + > > > + /* Will be reset by the modeset helpers when re-enabling the crtc by > > > + * calling drm_calc_timestamping_constants(). */ > > > + vblank->hwmode.crtc_clock = 0; > > > } > > > EXPORT_SYMBOL(drm_crtc_vblank_off); > > > > Shouldn't we do this in drm_crtc_vblank_reset() as well? > > > > Hmm. Except we call that after drm_calc_timestamping_constants(). I > > guess we should be able to move the reset() into > > intel_modeset_readout_hw_state(). And possibly move the vblank_on() > > call as well? > > Yeah, it'd be nice to clean this stuff up some more, but there's also the > problem that legacy and new drivers callc drm_calc_timestamping_constants > at opposite ends of the modeset sequence. Doing more here is a bunch more > work, maybe for the next patche series ... > > I don't think we need to call it in _reset, at least at boot-up it should > be 0 already. And for s/r we already shut down the pipe on suspend, so > it's gone through this here. Hmm. Indeed that seems like it should cover it. > > With the _ONCE nit address (and the build breakage I've introduced in this > version fixed), ack from you on the entire series? Sure, for the series Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 89f0928b042a..942183a2aa3c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -775,8 +775,10 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, /* If mode timing undefined, just return as no-op: * Happens during initial modesetting of a crtc. */ - if (mode->crtc_clock == 0) { + if (WARN_ON(mode->crtc_clock == 0)) { DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); + WARN_ON(drm_drv_uses_atomic_modeset(dev)); + return false; } @@ -1338,6 +1340,10 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) send_vblank_event(dev, e, seq, &now); } spin_unlock_irqrestore(&dev->event_lock, irqflags); + + /* Will be reset by the modeset helpers when re-enabling the crtc by + * calling drm_calc_timestamping_constants(). */ + vblank->hwmode.crtc_clock = 0; } EXPORT_SYMBOL(drm_crtc_vblank_off); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 837e5bc2019a..ff18b3fc00d2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -720,9 +720,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe) struct drm_i915_private *dev_priv = to_i915(dev); i915_reg_t high_frame, low_frame; u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; - struct intel_crtc *intel_crtc = intel_get_crtc_for_pipe(dev_priv, - pipe); - const struct drm_display_mode *mode = &intel_crtc->base.hwmode; + const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode; unsigned long irqflags; htotal = mode->crtc_htotal; @@ -779,13 +777,17 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); - const struct drm_display_mode *mode = &crtc->base.hwmode; + const struct drm_display_mode *mode; + struct drm_vblank_crtc *vblank; enum pipe pipe = crtc->pipe; int position, vtotal; if (!crtc->active) return -1; + vblank = &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)]; + mode = &vblank->hwmode; + vtotal = mode->crtc_vtotal; if (mode->flags & DRM_MODE_FLAG_INTERLACE) vtotal /= 2; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 85b9e2f521a0..a190973daeee 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11443,12 +11443,6 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state); - /* Update hwmode for vblank functions */ - if (new_crtc_state->active) - crtc->hwmode = new_crtc_state->adjusted_mode; - else - crtc->hwmode.crtc_clock = 0; - /* * Update legacy state to satisfy fbc code. This can * be removed when fbc uses the atomic state. @@ -15425,8 +15419,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) to_intel_crtc_state(crtc->base.state); int pixclk = 0; - crtc->base.hwmode = crtc_state->base.adjusted_mode; - memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); if (crtc_state->base.active) { intel_mode_from_pipe_config(&crtc->base.mode, crtc_state); @@ -15456,7 +15448,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) pixclk = DIV_ROUND_UP(pixclk * 100, 95); - drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); + drm_calc_timestamping_constants(&crtc->base, + &crtc_state->base.adjusted_mode); update_scanline_offset(crtc); }