diff mbox

[v2,16/17] drm/i915: Use crtc->hwmode for vblanks.

Message ID 1431548627-2527-17-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst May 13, 2015, 8:23 p.m. UTC
intel_crtc->config will be removed eventually, so use crtc->hwmode.
drm_atomic_helper_update_legacy_modeset_state updates hwmode,
but crtc->active will eventually be gone too. Set dotclock to zero
to indicate the crtc is inactive.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 13 ++++++-------
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Daniel Vetter May 18, 2015, 3:49 p.m. UTC | #1
On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote:
> intel_crtc->config will be removed eventually, so use crtc->hwmode.
> drm_atomic_helper_update_legacy_modeset_state updates hwmode,
> but crtc->active will eventually be gone too. Set dotclock to zero
> to indicate the crtc is inactive.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think adding a code comment to our assignment of crtc->hw_mode that we
need this for i915_get_vblank_timestamp (and only for that) would be
really good. Especially since I can't find it with a quick grep, at least
in current upstream ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 13 ++++++-------
>  drivers/gpu/drm/i915/intel_display.c |  3 +++
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ca457317a8ac..9359ea2399f1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
>  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -	const struct drm_display_mode *mode =
> -		&intel_crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
>  
>  	htotal = mode->crtc_htotal;
>  	hsync_start = mode->crtc_hsync_start;
> @@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *mode = &crtc->base.hwmode;
>  	enum pipe pipe = crtc->pipe;
>  	int position, vtotal;
>  
> @@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
> +	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
>  	int position;
>  	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
>  	bool in_vbl = true;
>  	int ret = 0;
>  	unsigned long irqflags;
>  
> -	if (!intel_crtc->active) {
> +	if (WARN_ON(!mode->crtc_clock)) {
>  		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
>  				 "pipe %c\n", pipe_name(pipe));
>  		return 0;
> @@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  		return -EINVAL;
>  	}
>  
> -	if (!crtc->state->active) {
> +	if (!crtc->hwmode.crtc_clock) {
>  		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
>  		return -EBUSY;
>  	}
> @@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
>  						     vblank_time, flags,
>  						     crtc,
> -						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
> +						     &crtc->hwmode);
>  }
>  
>  static bool intel_hpd_irq_event(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9316b0be4f5b..5063ac3d028d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
>  
>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
> +
> +		if (!crtc->state->active)
> +			crtc->hwmode.crtc_clock = 0;
>  	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä May 18, 2015, 4:28 p.m. UTC | #2
On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote:
> On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote:
> > intel_crtc->config will be removed eventually, so use crtc->hwmode.
> > drm_atomic_helper_update_legacy_modeset_state updates hwmode,
> > but crtc->active will eventually be gone too. Set dotclock to zero
> > to indicate the crtc is inactive.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> I think adding a code comment to our assignment of crtc->hw_mode that we
> need this for i915_get_vblank_timestamp (and only for that) would be
> really good. Especially since I can't find it with a quick grep, at least
> in current upstream ;-)

I don't particularly like resurrecting this zombie. Why we can't just use
crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)?

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c      | 13 ++++++-------
> >  drivers/gpu/drm/i915/intel_display.c |  3 +++
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index ca457317a8ac..9359ea2399f1 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> >  	struct intel_crtc *intel_crtc =
> >  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > -	const struct drm_display_mode *mode =
> > -		&intel_crtc->config->base.adjusted_mode;
> > +	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
> >  
> >  	htotal = mode->crtc_htotal;
> >  	hsync_start = mode->crtc_hsync_start;
> > @@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
> > +	const struct drm_display_mode *mode = &crtc->base.hwmode;
> >  	enum pipe pipe = crtc->pipe;
> >  	int position, vtotal;
> >  
> > @@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
> > +	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
> >  	int position;
> >  	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
> >  	bool in_vbl = true;
> >  	int ret = 0;
> >  	unsigned long irqflags;
> >  
> > -	if (!intel_crtc->active) {
> > +	if (WARN_ON(!mode->crtc_clock)) {
> >  		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
> >  				 "pipe %c\n", pipe_name(pipe));
> >  		return 0;
> > @@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (!crtc->state->active) {
> > +	if (!crtc->hwmode.crtc_clock) {
> >  		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
> >  		return -EBUSY;
> >  	}
> > @@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
> >  	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> >  						     vblank_time, flags,
> >  						     crtc,
> > -						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
> > +						     &crtc->hwmode);
> >  }
> >  
> >  static bool intel_hpd_irq_event(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9316b0be4f5b..5063ac3d028d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
> >  		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> >  
> >  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
> > +
> > +		if (!crtc->state->active)
> > +			crtc->hwmode.crtc_clock = 0;
> >  	}
> >  
> >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst May 19, 2015, 6:10 a.m. UTC | #3
Op 18-05-15 om 18:28 schreef Ville Syrjälä:
> On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote:
>> On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote:
>>> intel_crtc->config will be removed eventually, so use crtc->hwmode.
>>> drm_atomic_helper_update_legacy_modeset_state updates hwmode,
>>> but crtc->active will eventually be gone too. Set dotclock to zero
>>> to indicate the crtc is inactive.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> I think adding a code comment to our assignment of crtc->hw_mode that we
>> need this for i915_get_vblank_timestamp (and only for that) would be
>> really good. Especially since I can't find it with a quick grep, at least
>> in current upstream ;-)
> I don't particularly like resurrecting this zombie. Why we can't just use
> crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)?
>
Because we want to get rid of intel_crtc->config, and if drm_atomic_swap_state
is moved to be done before any call to then crtc->state->adjusted_mode will not
be in sync with the hw state, and any wai tfor vblank will produce funny results.

Since I don't think you should want to pass a state to vblank you would have to use
some crtc local variable somewhere, in this case I chose to use hwmode for that.

~Maarten
Daniel Vetter May 19, 2015, 8:16 a.m. UTC | #4
On Tue, May 19, 2015 at 08:10:46AM +0200, Maarten Lankhorst wrote:
> Op 18-05-15 om 18:28 schreef Ville Syrjälä:
> > On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote:
> >> On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote:
> >>> intel_crtc->config will be removed eventually, so use crtc->hwmode.
> >>> drm_atomic_helper_update_legacy_modeset_state updates hwmode,
> >>> but crtc->active will eventually be gone too. Set dotclock to zero
> >>> to indicate the crtc is inactive.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> I think adding a code comment to our assignment of crtc->hw_mode that we
> >> need this for i915_get_vblank_timestamp (and only for that) would be
> >> really good. Especially since I can't find it with a quick grep, at least
> >> in current upstream ;-)
> > I don't particularly like resurrecting this zombie. Why we can't just use
> > crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)?
> >
> Because we want to get rid of intel_crtc->config, and if drm_atomic_swap_state
> is moved to be done before any call to then crtc->state->adjusted_mode will not
> be in sync with the hw state, and any wai tfor vblank will produce funny results.
> 
> Since I don't think you should want to pass a state to vblank you would have to use
> some crtc local variable somewhere, in this case I chose to use hwmode for that.

I guess plan be could be to the required values (and only those we need,
not the entire mode struct) in struct intel_crtc. Not sure whether that's
worth the bother.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ca457317a8ac..9359ea2399f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -564,8 +564,7 @@  static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-	const struct drm_display_mode *mode =
-		&intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;
@@ -620,7 +619,7 @@  static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &crtc->base.hwmode;
 	enum pipe pipe = crtc->pipe;
 	int position, vtotal;
 
@@ -647,14 +646,14 @@  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 	int position;
 	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!intel_crtc->active) {
+	if (WARN_ON(!mode->crtc_clock)) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
 				 "pipe %c\n", pipe_name(pipe));
 		return 0;
@@ -796,7 +795,7 @@  static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 
-	if (!crtc->state->active) {
+	if (!crtc->hwmode.crtc_clock) {
 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}
@@ -805,7 +804,7 @@  static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
 						     vblank_time, flags,
 						     crtc,
-						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
+						     &crtc->hwmode);
 }
 
 static bool intel_hpd_irq_event(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9316b0be4f5b..5063ac3d028d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11467,6 +11467,9 @@  intel_modeset_update_state(struct drm_atomic_state *state)
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
 
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+
+		if (!crtc->state->active)
+			crtc->hwmode.crtc_clock = 0;
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {