[Intel-gfx] [PATCH 5/5] drm/vblank: Lock down vblank->hwmode more

[Daniel Vetter]

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.

v2:
- WARN_ON_ONCE to avoid excessive spam (Ville)
- Really only WARN on atomic drivers.

Cc: Ville Syrjälä 
Reviewed-by: Neil Armstrong 
Acked-by: Ville Syrjälä 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_irq.c|  6 ++
 drivers/gpu/drm/i915/i915_irq.c  | 10 ++
 drivers/gpu/drm/i915/intel_display.c | 11 ++-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 89f0928b042a..c7debaad67f8 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -777,6 +777,8 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct 
drm_device *dev,
 */
if (mode->crtc_clock == 0) {
DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe);
+   WARN_ON_ONCE(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, );
}
spin_unlock_irqrestore(>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 34b09edc18e4..5292fb1e5c53 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 = _crtc->base.hwmode;
+   const struct drm_display_mode *mode = >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 = >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 = >base.dev->vblank[drm_crtc_index(>base)];
+   mode = >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(>base.mode, 0, sizeof(crtc->base.mode));
if (crtc_state->base.active) {
intel_mode_from_pipe_config(>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(>base, 
>base.hwmode);
+   drm_calc_timestamping_constants(>base,
+   
_state->base.adjusted_mode);
update_scanline_offset(crtc);
}
 
-- 

Re: [Intel-gfx] [PATCH 5/5] drm/vblank: Lock down vblank->hwmode more

[Ville Syrjälä]

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ä 
> > > Reviewed-by: Neil Armstrong 
> > > Signed-off-by: Daniel Vetter 
> > > ---
> > >  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, );
> > >   }
> > >   spin_unlock_irqrestore(>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 Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/vblank: Lock down vblank->hwmode more

[Daniel Vetter]

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ä 
> > Reviewed-by: Neil Armstrong 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  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, );
> > }
> > spin_unlock_irqrestore(>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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/5] drm/vblank: Lock down vblank->hwmode more

[Ville Syrjälä]

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ä 
> Reviewed-by: Neil Armstrong 
> Signed-off-by: Daniel Vetter 
> ---
>  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, );
>   }
>   spin_unlock_irqrestore(>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 = _crtc->base.hwmode;
> + const struct drm_display_mode *mode = >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 = >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 = >base.dev->vblank[drm_crtc_index(>base)];
> + mode = >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(>base.mode, 0, sizeof(crtc->base.mode));
>   if (crtc_state->base.active) {
>   intel_mode_from_pipe_config(>base.mode, 
> crtc_state);
> @@ -15456,7 +15448,8 @@ 

[Intel-gfx] [PATCH 5/5] drm/vblank: Lock down vblank->hwmode more

[Daniel Vetter]

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ä 
Reviewed-by: Neil Armstrong 
Signed-off-by: Daniel Vetter 
---
 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));
+
return false;
}
 
@@ -1338,6 +1340,10 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
send_vblank_event(dev, e, seq, );
}
spin_unlock_irqrestore(>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 = _crtc->base.hwmode;
+   const struct drm_display_mode *mode = >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 = >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 = >base.dev->vblank[drm_crtc_index(>base)];
+   mode = >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(>base.mode, 0, sizeof(crtc->base.mode));
if (crtc_state->base.active) {
intel_mode_from_pipe_config(>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(>base, 
>base.hwmode);
+   drm_calc_timestamping_constants(>base,
+   
_state->base.adjusted_mode);
update_scanline_offset(crtc);