All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout
@ 2015-09-10 15:59 ville.syrjala
  2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: ville.syrjala @ 2015-09-10 15:59 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The dotclock is often calculated in encoder .get_config(), so we
shouldn't copy the adjusted_mode to hwmode until we have read out the
dotclock.

Gets rid of some warnings like these:
[drm:drm_calc_timestamping_constants [drm]] *ERROR* crtc 21: Can't calculate constants, dotclock = 0!
[drm:i915_get_vblank_timestamp] crtc 0 is disabled

v2: Steal Maarten's idea to move crtc->mode etc. assignment too

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 57 +++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af48c1e..f5673c8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15094,33 +15094,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
-		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
-		if (crtc->base.state->active) {
-			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
-			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
-			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
-
-			/*
-			 * The initial mode needs to be set in order to keep
-			 * the atomic core happy. It wants a valid mode if the
-			 * crtc's enabled, so we do the above call.
-			 *
-			 * At this point some state updated by the connectors
-			 * in their ->detect() callback has not run yet, so
-			 * no recalculation can be done yet.
-			 *
-			 * Even if we could do a recalculation and modeset
-			 * right now it would cause a double modeset if
-			 * fbdev or userspace chooses a different initial mode.
-			 *
-			 * If that happens, someone indicated they wanted a
-			 * mode change, which means it's safe to do a full
-			 * recalculation.
-			 */
-			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
-		}
-
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
@@ -15180,6 +15153,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      connector->base.name,
 			      connector->base.encoder ? "enabled" : "disabled");
 	}
+
+	for_each_intel_crtc(dev, crtc) {
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
+
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			/*
+			 * The initial mode needs to be set in order to keep
+			 * the atomic core happy. It wants a valid mode if the
+			 * crtc's enabled, so we do the above call.
+			 *
+			 * At this point some state updated by the connectors
+			 * in their ->detect() callback has not run yet, so
+			 * no recalculation can be done yet.
+			 *
+			 * Even if we could do a recalculation and modeset
+			 * right now it would cause a double modeset if
+			 * fbdev or userspace chooses a different initial mode.
+			 *
+			 * If that happens, someone indicated they wanted a
+			 * mode change, which means it's safe to do a full
+			 * recalculation.
+			 */
+			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
+		}
+	}
 }
 
 /* Scan out the current hw modeset state,
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 19+ messages in thread

• * [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()
    2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
  @ 2015-09-10 15:59 ` ville.syrjala
    2015-09-14 11:48   ` Patrik Jakobsson
    2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
                     ` (2 subsequent siblings)
    3 siblings, 1 reply; 19+ messages in thread
  From: ville.syrjala @ 2015-09-10 15:59 UTC (permalink / raw)
    To: intel-gfx
  
  From: Ville Syrjälä <ville.syrjala@linux.intel.com>
  
  Move the sprite/cursor plane disabling to occur in intel_sanitize_crtc()
  where it belongs instead of doing it in intel_modeset_readout_hw_state().
  
  The plane disabling was first added in
  4cf0ebbd4fafbdf8e6431dbb315e5511c3efdc3b drm/i915: Rework plane readout.
  
  I got the idea from some patches from Partik and/or Maarten but those
  moved also the plane state readout to intel_sanitize_crtc() which isn't
  quite right in my opinion.
  
  Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
  Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
  Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
  ---
   drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++--------------------
   1 file changed, 20 insertions(+), 24 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
  index f5673c8..3707212 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -14878,9 +14878,19 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
   	/* restore vblank interrupts to correct state */
   	drm_crtc_vblank_reset(&crtc->base);
   	if (crtc->active) {
  +		struct intel_plane *plane;
  +
   		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
   		update_scanline_offset(crtc);
   		drm_crtc_vblank_on(&crtc->base);
  +
  +		/* Disable everything but the primary plane */
  +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
  +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
  +				continue;
  +
  +			plane->disable_plane(&plane->base, &crtc->base);
  +		}
   	}
   
   	/* We need to sanitize the plane -> pipe mapping first because this will
  @@ -15043,35 +15053,21 @@ void i915_redisable_vga(struct drm_device *dev)
   	i915_redisable_vga_power_on(dev);
   }
   
  -static bool primary_get_hw_state(struct intel_crtc *crtc)
  +static bool primary_get_hw_state(struct intel_plane *plane)
   {
  -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
   
  -	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
  +	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
   }
   
  -static void readout_plane_state(struct intel_crtc *crtc,
  -				struct intel_crtc_state *crtc_state)
  +/* FIXME read out full plane state for all planes */
  +static void readout_plane_state(struct intel_crtc *crtc)
   {
  -	struct intel_plane *p;
  -	struct intel_plane_state *plane_state;
  -	bool active = crtc_state->base.active;
  +	struct intel_plane_state *plane_state =
  +		to_intel_plane_state(crtc->base.primary->state);
   
  -	for_each_intel_plane(crtc->base.dev, p) {
  -		if (crtc->pipe != p->pipe)
  -			continue;
  -
  -		plane_state = to_intel_plane_state(p->base.state);
  -
  -		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
  -			plane_state->visible = primary_get_hw_state(crtc);
  -		else {
  -			if (active)
  -				p->disable_plane(&p->base, &crtc->base);
  -
  -			plane_state->visible = false;
  -		}
  -	}
  +	plane_state->visible =
  +		primary_get_hw_state(to_intel_plane(crtc->base.primary));
   }
   
   static void intel_modeset_readout_hw_state(struct drm_device *dev)
  @@ -15094,7 +15090,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
   		crtc->base.state->active = crtc->active;
   		crtc->base.enabled = crtc->active;
   
  -		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
  +		readout_plane_state(crtc);
   
   		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
   			      crtc->base.base.id,
  -- 
  2.4.6
  
  _______________________________________________
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
  
  ^ permalink raw reply related	[flat|nested] 19+ messages in thread

• • * Re: [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()
      2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
    @ 2015-09-14 11:48   ` Patrik Jakobsson
      0 siblings, 0 replies; 19+ messages in thread
    From: Patrik Jakobsson @ 2015-09-14 11:48 UTC (permalink / raw)
      To: ville.syrjala; +Cc: intel-gfx
    
    On Thu, Sep 10, 2015 at 06:59:08PM +0300, ville.syrjala@linux.intel.com wrote:
    > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > 
    > Move the sprite/cursor plane disabling to occur in intel_sanitize_crtc()
    > where it belongs instead of doing it in intel_modeset_readout_hw_state().
    > 
    > The plane disabling was first added in
    > 4cf0ebbd4fafbdf8e6431dbb315e5511c3efdc3b drm/i915: Rework plane readout.
    > 
    > I got the idea from some patches from Partik and/or Maarten but those
    > moved also the plane state readout to intel_sanitize_crtc() which isn't
    > quite right in my opinion.
    
    I agree, this is better.
    
    Probably related bug report:
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91910
    
    Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    
    > 
    > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > ---
    >  drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++--------------------
    >  1 file changed, 20 insertions(+), 24 deletions(-)
    > 
    > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > index f5673c8..3707212 100644
    > --- a/drivers/gpu/drm/i915/intel_display.c
    > +++ b/drivers/gpu/drm/i915/intel_display.c
    > @@ -14878,9 +14878,19 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
    >  	/* restore vblank interrupts to correct state */
    >  	drm_crtc_vblank_reset(&crtc->base);
    >  	if (crtc->active) {
    > +		struct intel_plane *plane;
    > +
    >  		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
    >  		update_scanline_offset(crtc);
    >  		drm_crtc_vblank_on(&crtc->base);
    > +
    > +		/* Disable everything but the primary plane */
    > +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
    > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
    > +				continue;
    > +
    > +			plane->disable_plane(&plane->base, &crtc->base);
    > +		}
    >  	}
    >  
    >  	/* We need to sanitize the plane -> pipe mapping first because this will
    > @@ -15043,35 +15053,21 @@ void i915_redisable_vga(struct drm_device *dev)
    >  	i915_redisable_vga_power_on(dev);
    >  }
    >  
    > -static bool primary_get_hw_state(struct intel_crtc *crtc)
    > +static bool primary_get_hw_state(struct intel_plane *plane)
    >  {
    > -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    >  
    > -	return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE);
    > +	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
    >  }
    >  
    > -static void readout_plane_state(struct intel_crtc *crtc,
    > -				struct intel_crtc_state *crtc_state)
    > +/* FIXME read out full plane state for all planes */
    > +static void readout_plane_state(struct intel_crtc *crtc)
    >  {
    > -	struct intel_plane *p;
    > -	struct intel_plane_state *plane_state;
    > -	bool active = crtc_state->base.active;
    > +	struct intel_plane_state *plane_state =
    > +		to_intel_plane_state(crtc->base.primary->state);
    >  
    > -	for_each_intel_plane(crtc->base.dev, p) {
    > -		if (crtc->pipe != p->pipe)
    > -			continue;
    > -
    > -		plane_state = to_intel_plane_state(p->base.state);
    > -
    > -		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
    > -			plane_state->visible = primary_get_hw_state(crtc);
    > -		else {
    > -			if (active)
    > -				p->disable_plane(&p->base, &crtc->base);
    > -
    > -			plane_state->visible = false;
    > -		}
    > -	}
    > +	plane_state->visible =
    > +		primary_get_hw_state(to_intel_plane(crtc->base.primary));
    >  }
    >  
    >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
    > @@ -15094,7 +15090,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
    >  		crtc->base.state->active = crtc->active;
    >  		crtc->base.enabled = crtc->active;
    >  
    > -		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
    > +		readout_plane_state(crtc);
    >  
    >  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
    >  			      crtc->base.base.id,
    > -- 
    > 2.4.6
    > 
    _______________________________________________
    Intel-gfx mailing list
    Intel-gfx@lists.freedesktop.org
    http://lists.freedesktop.org/mailman/listinfo/intel-gfx
    
    ^ permalink raw reply	[flat|nested] 19+ messages in thread

• * [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
    2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
    2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
  @ 2015-09-10 15:59 ` ville.syrjala
    2015-09-10 16:07   ` Daniel Vetter
    2015-09-14 12:10   ` Maarten Lankhorst
    2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
    2015-09-14 11:36 ` [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout Patrik Jakobsson
    3 siblings, 2 replies; 19+ messages in thread
  From: ville.syrjala @ 2015-09-10 15:59 UTC (permalink / raw)
    To: intel-gfx
  
  From: Ville Syrjälä <ville.syrjala@linux.intel.com>
  
  Add a .get_hw_state() method for planes, returning true or false
  depending on whether the plane is enabled. Use it to populate the
  plane state 'visible' during state readout.
  
  Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
  Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
  Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
  ---
   drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
   drivers/gpu/drm/i915/intel_drv.h     |  1 +
   drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
   3 files changed, 91 insertions(+), 14 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
  index 3707212..5fed120 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
   	POSTING_READ(reg);
   }
   
  +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
  +}
  +
   u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
   			      uint32_t pixel_format)
   {
  @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
   	POSTING_READ(PLANE_SURF(pipe, 0));
   }
   
  +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
  +}
  +
   /* Assume fb object is pinned & idle & fenced and just update base pointers */
   static int
   intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
  @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
   	}
   }
   
  +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
  +}
  +
   static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
   {
   	struct drm_device *dev = crtc->dev;
  @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
   	intel_crtc->cursor_base = base;
   }
   
  +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
  +}
  +
   /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
   static void intel_crtc_update_cursor(struct drm_crtc *crtc,
   				     bool on)
  @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
   	}
   	primary->pipe = pipe;
   	primary->plane = pipe;
  +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
  +		primary->plane = !pipe;
   	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
   	primary->check_plane = intel_check_primary_plane;
   	primary->commit_plane = intel_commit_primary_plane;
   	primary->disable_plane = intel_disable_primary_plane;
  -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
  -		primary->plane = !pipe;
  +
  +	if (INTEL_INFO(dev)->gen >= 9)
  +		primary->get_hw_state = skylake_primary_get_hw_state;
  +	else
  +		primary->get_hw_state = i9xx_primary_get_hw_state;
   
   	if (INTEL_INFO(dev)->gen >= 9) {
   		intel_primary_formats = skl_primary_formats;
  @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
   	cursor->commit_plane = intel_commit_cursor_plane;
   	cursor->disable_plane = intel_disable_cursor_plane;
   
  +	if (IS_845G(dev) || IS_I865G(dev))
  +		cursor->get_hw_state = i845_cursor_get_hw_state;
  +	else
  +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
  +
   	drm_universal_plane_init(dev, &cursor->base, 0,
   				 &intel_plane_funcs,
   				 intel_cursor_formats,
  @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
   
   		/* Disable everything but the primary plane */
   		for_each_intel_plane_on_crtc(dev, crtc, plane) {
  -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
  +			struct intel_plane_state *state =
  +				to_intel_plane_state(plane->base.state);
  +
  +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
  +			    !state->visible)
   				continue;
   
   			plane->disable_plane(&plane->base, &crtc->base);
  @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
   	i915_redisable_vga_power_on(dev);
   }
   
  -static bool primary_get_hw_state(struct intel_plane *plane)
  -{
  -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  -
  -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
  -}
  -
   /* FIXME read out full plane state for all planes */
   static void readout_plane_state(struct intel_crtc *crtc)
   {
  -	struct intel_plane_state *plane_state =
  -		to_intel_plane_state(crtc->base.primary->state);
  +	struct drm_device *dev = crtc->base.dev;
  +	struct intel_plane *plane;
   
  -	plane_state->visible =
  -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
  +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
  +		struct intel_plane_state *state =
  +			to_intel_plane_state(plane->base.state);
  +
  +		state->visible = plane->get_hw_state(plane);
  +	}
   }
   
   static void intel_modeset_readout_hw_state(struct drm_device *dev)
  diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
  index 46484e4..64d6d90 100644
  --- a/drivers/gpu/drm/i915/intel_drv.h
  +++ b/drivers/gpu/drm/i915/intel_drv.h
  @@ -628,6 +628,7 @@ struct intel_plane {
   			   struct intel_plane_state *state);
   	void (*commit_plane)(struct drm_plane *plane,
   			     struct intel_plane_state *state);
  +	bool (*get_hw_state)(struct intel_plane *plane);
   };
   
   struct intel_watermark_params {
  diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
  index ca7e264..8b2c744 100644
  --- a/drivers/gpu/drm/i915/intel_sprite.c
  +++ b/drivers/gpu/drm/i915/intel_sprite.c
  @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
   	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
   }
   
  +static bool
  +skl_plane_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(PLANE_CTL(plane->pipe,
  +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
  +}
  +
   static void
   chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
   {
  @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
   	POSTING_READ(SPSURF(pipe, plane));
   }
   
  +static bool
  +vlv_plane_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
  +}
  +
   static void
   ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
   		 struct drm_framebuffer *fb,
  @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
   	POSTING_READ(SPRSURF(pipe));
   }
   
  +static bool
  +ivb_plane_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
  +}
  +
   static void
   ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
   		 struct drm_framebuffer *fb,
  @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
   	POSTING_READ(DVSSURF(pipe));
   }
   
  +static bool
  +ilk_plane_get_hw_state(struct intel_plane *plane)
  +{
  +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
  +
  +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
  +}
  +
   static int
   intel_check_sprite_plane(struct drm_plane *plane,
   			 struct intel_crtc_state *crtc_state,
  @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
   		intel_plane->max_downscale = 16;
   		intel_plane->update_plane = ilk_update_plane;
   		intel_plane->disable_plane = ilk_disable_plane;
  +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
   
   		if (IS_GEN6(dev)) {
   			plane_formats = snb_plane_formats;
  @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
   		if (IS_VALLEYVIEW(dev)) {
   			intel_plane->update_plane = vlv_update_plane;
   			intel_plane->disable_plane = vlv_disable_plane;
  +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
   
   			plane_formats = vlv_plane_formats;
   			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
   		} else {
   			intel_plane->update_plane = ivb_update_plane;
   			intel_plane->disable_plane = ivb_disable_plane;
  +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
   
   			plane_formats = snb_plane_formats;
   			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
  @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
   		intel_plane->can_scale = true;
   		intel_plane->update_plane = skl_update_plane;
   		intel_plane->disable_plane = skl_disable_plane;
  +		intel_plane->get_hw_state = skl_plane_get_hw_state;
   		state->scaler_id = -1;
   
   		plane_formats = skl_plane_formats;
  -- 
  2.4.6
  
  _______________________________________________
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
  
  ^ permalink raw reply related	[flat|nested] 19+ messages in thread

• • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
      2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
    @ 2015-09-10 16:07   ` Daniel Vetter
      2015-09-10 16:13     ` Ville Syrjälä
      2015-09-14 12:10   ` Maarten Lankhorst
      1 sibling, 1 reply; 19+ messages in thread
    From: Daniel Vetter @ 2015-09-10 16:07 UTC (permalink / raw)
      To: ville.syrjala; +Cc: intel-gfx
    
    On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
    > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > 
    > Add a .get_hw_state() method for planes, returning true or false
    > depending on whether the plane is enabled. Use it to populate the
    > plane state 'visible' during state readout.
    > 
    > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    
    Do we really need this? The plane readout is fairly limited and we don't
    really care about recovering anything but the initial primary plane on
    driver load. Anything else can just be nuked if it misfits ...
    -Daniel
    
    > ---
    >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
    >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
    >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
    >  3 files changed, 91 insertions(+), 14 deletions(-)
    > 
    > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > index 3707212..5fed120 100644
    > --- a/drivers/gpu/drm/i915/intel_display.c
    > +++ b/drivers/gpu/drm/i915/intel_display.c
    > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
    >  	POSTING_READ(reg);
    >  }
    >  
    > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
    > +}
    > +
    >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
    >  			      uint32_t pixel_format)
    >  {
    > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
    >  	POSTING_READ(PLANE_SURF(pipe, 0));
    >  }
    >  
    > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
    > +}
    > +
    >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
    >  static int
    >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
    > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
    >  	}
    >  }
    >  
    > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
    > +}
    > +
    >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
    >  {
    >  	struct drm_device *dev = crtc->dev;
    > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
    >  	intel_crtc->cursor_base = base;
    >  }
    >  
    > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
    > +}
    > +
    >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
    >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
    >  				     bool on)
    > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
    >  	}
    >  	primary->pipe = pipe;
    >  	primary->plane = pipe;
    > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
    > +		primary->plane = !pipe;
    >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
    >  	primary->check_plane = intel_check_primary_plane;
    >  	primary->commit_plane = intel_commit_primary_plane;
    >  	primary->disable_plane = intel_disable_primary_plane;
    > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
    > -		primary->plane = !pipe;
    > +
    > +	if (INTEL_INFO(dev)->gen >= 9)
    > +		primary->get_hw_state = skylake_primary_get_hw_state;
    > +	else
    > +		primary->get_hw_state = i9xx_primary_get_hw_state;
    >  
    >  	if (INTEL_INFO(dev)->gen >= 9) {
    >  		intel_primary_formats = skl_primary_formats;
    > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
    >  	cursor->commit_plane = intel_commit_cursor_plane;
    >  	cursor->disable_plane = intel_disable_cursor_plane;
    >  
    > +	if (IS_845G(dev) || IS_I865G(dev))
    > +		cursor->get_hw_state = i845_cursor_get_hw_state;
    > +	else
    > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
    > +
    >  	drm_universal_plane_init(dev, &cursor->base, 0,
    >  				 &intel_plane_funcs,
    >  				 intel_cursor_formats,
    > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
    >  
    >  		/* Disable everything but the primary plane */
    >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
    > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
    > +			struct intel_plane_state *state =
    > +				to_intel_plane_state(plane->base.state);
    > +
    > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
    > +			    !state->visible)
    >  				continue;
    >  
    >  			plane->disable_plane(&plane->base, &crtc->base);
    > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
    >  	i915_redisable_vga_power_on(dev);
    >  }
    >  
    > -static bool primary_get_hw_state(struct intel_plane *plane)
    > -{
    > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > -
    > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
    > -}
    > -
    >  /* FIXME read out full plane state for all planes */
    >  static void readout_plane_state(struct intel_crtc *crtc)
    >  {
    > -	struct intel_plane_state *plane_state =
    > -		to_intel_plane_state(crtc->base.primary->state);
    > +	struct drm_device *dev = crtc->base.dev;
    > +	struct intel_plane *plane;
    >  
    > -	plane_state->visible =
    > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
    > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
    > +		struct intel_plane_state *state =
    > +			to_intel_plane_state(plane->base.state);
    > +
    > +		state->visible = plane->get_hw_state(plane);
    > +	}
    >  }
    >  
    >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
    > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
    > index 46484e4..64d6d90 100644
    > --- a/drivers/gpu/drm/i915/intel_drv.h
    > +++ b/drivers/gpu/drm/i915/intel_drv.h
    > @@ -628,6 +628,7 @@ struct intel_plane {
    >  			   struct intel_plane_state *state);
    >  	void (*commit_plane)(struct drm_plane *plane,
    >  			     struct intel_plane_state *state);
    > +	bool (*get_hw_state)(struct intel_plane *plane);
    >  };
    >  
    >  struct intel_watermark_params {
    > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
    > index ca7e264..8b2c744 100644
    > --- a/drivers/gpu/drm/i915/intel_sprite.c
    > +++ b/drivers/gpu/drm/i915/intel_sprite.c
    > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
    >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
    >  }
    >  
    > +static bool
    > +skl_plane_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(PLANE_CTL(plane->pipe,
    > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
    > +}
    > +
    >  static void
    >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
    >  {
    > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
    >  	POSTING_READ(SPSURF(pipe, plane));
    >  }
    >  
    > +static bool
    > +vlv_plane_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
    > +}
    > +
    >  static void
    >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
    >  		 struct drm_framebuffer *fb,
    > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
    >  	POSTING_READ(SPRSURF(pipe));
    >  }
    >  
    > +static bool
    > +ivb_plane_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
    > +}
    > +
    >  static void
    >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
    >  		 struct drm_framebuffer *fb,
    > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
    >  	POSTING_READ(DVSSURF(pipe));
    >  }
    >  
    > +static bool
    > +ilk_plane_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
    > +}
    > +
    >  static int
    >  intel_check_sprite_plane(struct drm_plane *plane,
    >  			 struct intel_crtc_state *crtc_state,
    > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
    >  		intel_plane->max_downscale = 16;
    >  		intel_plane->update_plane = ilk_update_plane;
    >  		intel_plane->disable_plane = ilk_disable_plane;
    > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
    >  
    >  		if (IS_GEN6(dev)) {
    >  			plane_formats = snb_plane_formats;
    > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
    >  		if (IS_VALLEYVIEW(dev)) {
    >  			intel_plane->update_plane = vlv_update_plane;
    >  			intel_plane->disable_plane = vlv_disable_plane;
    > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
    >  
    >  			plane_formats = vlv_plane_formats;
    >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
    >  		} else {
    >  			intel_plane->update_plane = ivb_update_plane;
    >  			intel_plane->disable_plane = ivb_disable_plane;
    > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
    >  
    >  			plane_formats = snb_plane_formats;
    >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
    > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
    >  		intel_plane->can_scale = true;
    >  		intel_plane->update_plane = skl_update_plane;
    >  		intel_plane->disable_plane = skl_disable_plane;
    > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
    >  		state->scaler_id = -1;
    >  
    >  		plane_formats = skl_plane_formats;
    > -- 
    > 2.4.6
    > 
    > _______________________________________________
    > 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
    
    ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
        2015-09-10 16:07   ` Daniel Vetter
      @ 2015-09-10 16:13     ` Ville Syrjälä
        2015-09-10 16:20       ` Ville Syrjälä
        0 siblings, 1 reply; 19+ messages in thread
      From: Ville Syrjälä @ 2015-09-10 16:13 UTC (permalink / raw)
        To: Daniel Vetter; +Cc: intel-gfx
      
      On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
      > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
      > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > > 
      > > Add a .get_hw_state() method for planes, returning true or false
      > > depending on whether the plane is enabled. Use it to populate the
      > > plane state 'visible' during state readout.
      > > 
      > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > 
      > Do we really need this? The plane readout is fairly limited and we don't
      > really care about recovering anything but the initial primary plane on
      > driver load. Anything else can just be nuked if it misfits ...
      
      At least avoids calling ->disable_plane for an already disabled plane.
      We could even put a WARN there to catch broken code.
      
      > -Daniel
      > 
      > > ---
      > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
      > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
      > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
      > >  3 files changed, 91 insertions(+), 14 deletions(-)
      > > 
      > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
      > > index 3707212..5fed120 100644
      > > --- a/drivers/gpu/drm/i915/intel_display.c
      > > +++ b/drivers/gpu/drm/i915/intel_display.c
      > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
      > >  	POSTING_READ(reg);
      > >  }
      > >  
      > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
      > > +}
      > > +
      > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
      > >  			      uint32_t pixel_format)
      > >  {
      > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
      > >  	POSTING_READ(PLANE_SURF(pipe, 0));
      > >  }
      > >  
      > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
      > > +}
      > > +
      > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
      > >  static int
      > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
      > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  	}
      > >  }
      > >  
      > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
      > > +}
      > > +
      > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  {
      > >  	struct drm_device *dev = crtc->dev;
      > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  	intel_crtc->cursor_base = base;
      > >  }
      > >  
      > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
      > > +}
      > > +
      > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
      > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
      > >  				     bool on)
      > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
      > >  	}
      > >  	primary->pipe = pipe;
      > >  	primary->plane = pipe;
      > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
      > > +		primary->plane = !pipe;
      > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
      > >  	primary->check_plane = intel_check_primary_plane;
      > >  	primary->commit_plane = intel_commit_primary_plane;
      > >  	primary->disable_plane = intel_disable_primary_plane;
      > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
      > > -		primary->plane = !pipe;
      > > +
      > > +	if (INTEL_INFO(dev)->gen >= 9)
      > > +		primary->get_hw_state = skylake_primary_get_hw_state;
      > > +	else
      > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
      > >  
      > >  	if (INTEL_INFO(dev)->gen >= 9) {
      > >  		intel_primary_formats = skl_primary_formats;
      > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
      > >  	cursor->commit_plane = intel_commit_cursor_plane;
      > >  	cursor->disable_plane = intel_disable_cursor_plane;
      > >  
      > > +	if (IS_845G(dev) || IS_I865G(dev))
      > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
      > > +	else
      > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
      > > +
      > >  	drm_universal_plane_init(dev, &cursor->base, 0,
      > >  				 &intel_plane_funcs,
      > >  				 intel_cursor_formats,
      > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
      > >  
      > >  		/* Disable everything but the primary plane */
      > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
      > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
      > > +			struct intel_plane_state *state =
      > > +				to_intel_plane_state(plane->base.state);
      > > +
      > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
      > > +			    !state->visible)
      > >  				continue;
      > >  
      > >  			plane->disable_plane(&plane->base, &crtc->base);
      > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
      > >  	i915_redisable_vga_power_on(dev);
      > >  }
      > >  
      > > -static bool primary_get_hw_state(struct intel_plane *plane)
      > > -{
      > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > -
      > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
      > > -}
      > > -
      > >  /* FIXME read out full plane state for all planes */
      > >  static void readout_plane_state(struct intel_crtc *crtc)
      > >  {
      > > -	struct intel_plane_state *plane_state =
      > > -		to_intel_plane_state(crtc->base.primary->state);
      > > +	struct drm_device *dev = crtc->base.dev;
      > > +	struct intel_plane *plane;
      > >  
      > > -	plane_state->visible =
      > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
      > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
      > > +		struct intel_plane_state *state =
      > > +			to_intel_plane_state(plane->base.state);
      > > +
      > > +		state->visible = plane->get_hw_state(plane);
      > > +	}
      > >  }
      > >  
      > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
      > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
      > > index 46484e4..64d6d90 100644
      > > --- a/drivers/gpu/drm/i915/intel_drv.h
      > > +++ b/drivers/gpu/drm/i915/intel_drv.h
      > > @@ -628,6 +628,7 @@ struct intel_plane {
      > >  			   struct intel_plane_state *state);
      > >  	void (*commit_plane)(struct drm_plane *plane,
      > >  			     struct intel_plane_state *state);
      > > +	bool (*get_hw_state)(struct intel_plane *plane);
      > >  };
      > >  
      > >  struct intel_watermark_params {
      > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
      > > index ca7e264..8b2c744 100644
      > > --- a/drivers/gpu/drm/i915/intel_sprite.c
      > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
      > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
      > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
      > >  }
      > >  
      > > +static bool
      > > +skl_plane_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(PLANE_CTL(plane->pipe,
      > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
      > > +}
      > > +
      > >  static void
      > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
      > >  {
      > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
      > >  	POSTING_READ(SPSURF(pipe, plane));
      > >  }
      > >  
      > > +static bool
      > > +vlv_plane_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
      > > +}
      > > +
      > >  static void
      > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
      > >  		 struct drm_framebuffer *fb,
      > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
      > >  	POSTING_READ(SPRSURF(pipe));
      > >  }
      > >  
      > > +static bool
      > > +ivb_plane_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
      > > +}
      > > +
      > >  static void
      > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
      > >  		 struct drm_framebuffer *fb,
      > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
      > >  	POSTING_READ(DVSSURF(pipe));
      > >  }
      > >  
      > > +static bool
      > > +ilk_plane_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
      > > +}
      > > +
      > >  static int
      > >  intel_check_sprite_plane(struct drm_plane *plane,
      > >  			 struct intel_crtc_state *crtc_state,
      > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
      > >  		intel_plane->max_downscale = 16;
      > >  		intel_plane->update_plane = ilk_update_plane;
      > >  		intel_plane->disable_plane = ilk_disable_plane;
      > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
      > >  
      > >  		if (IS_GEN6(dev)) {
      > >  			plane_formats = snb_plane_formats;
      > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
      > >  		if (IS_VALLEYVIEW(dev)) {
      > >  			intel_plane->update_plane = vlv_update_plane;
      > >  			intel_plane->disable_plane = vlv_disable_plane;
      > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
      > >  
      > >  			plane_formats = vlv_plane_formats;
      > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
      > >  		} else {
      > >  			intel_plane->update_plane = ivb_update_plane;
      > >  			intel_plane->disable_plane = ivb_disable_plane;
      > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
      > >  
      > >  			plane_formats = snb_plane_formats;
      > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
      > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
      > >  		intel_plane->can_scale = true;
      > >  		intel_plane->update_plane = skl_update_plane;
      > >  		intel_plane->disable_plane = skl_disable_plane;
      > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
      > >  		state->scaler_id = -1;
      > >  
      > >  		plane_formats = skl_plane_formats;
      > > -- 
      > > 2.4.6
      > > 
      > > _______________________________________________
      > > 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
      
      -- 
      Ville Syrjälä
      Intel OTC
      _______________________________________________
      Intel-gfx mailing list
      Intel-gfx@lists.freedesktop.org
      http://lists.freedesktop.org/mailman/listinfo/intel-gfx
      
      ^ permalink raw reply	[flat|nested] 19+ messages in thread

    • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
          2015-09-10 16:13     ` Ville Syrjälä
        @ 2015-09-10 16:20       ` Ville Syrjälä
          2015-09-10 16:30         ` Daniel Vetter
          0 siblings, 1 reply; 19+ messages in thread
        From: Ville Syrjälä @ 2015-09-10 16:20 UTC (permalink / raw)
          To: Daniel Vetter; +Cc: intel-gfx
        
        On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
        > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
        > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
        > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
        > > > 
        > > > Add a .get_hw_state() method for planes, returning true or false
        > > > depending on whether the plane is enabled. Use it to populate the
        > > > plane state 'visible' during state readout.
        > > > 
        > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
        > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
        > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
        > > 
        > > Do we really need this? The plane readout is fairly limited and we don't
        > > really care about recovering anything but the initial primary plane on
        > > driver load. Anything else can just be nuked if it misfits ...
        > 
        > At least avoids calling ->disable_plane for an already disabled plane.
        > We could even put a WARN there to catch broken code.
        
        Oh and we could also do something like
        for_each_plane()
        	assert(visible == get_hw_state);
        
        somewhere to perhaps catch even more crap.
        
        
        > 
        > > -Daniel
        > > 
        > > > ---
        > > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
        > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
        > > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
        > > >  3 files changed, 91 insertions(+), 14 deletions(-)
        > > > 
        > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
        > > > index 3707212..5fed120 100644
        > > > --- a/drivers/gpu/drm/i915/intel_display.c
        > > > +++ b/drivers/gpu/drm/i915/intel_display.c
        > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
        > > >  	POSTING_READ(reg);
        > > >  }
        > > >  
        > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
        > > > +}
        > > > +
        > > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
        > > >  			      uint32_t pixel_format)
        > > >  {
        > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
        > > >  	POSTING_READ(PLANE_SURF(pipe, 0));
        > > >  }
        > > >  
        > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
        > > > +}
        > > > +
        > > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
        > > >  static int
        > > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
        > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
        > > >  	}
        > > >  }
        > > >  
        > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
        > > > +}
        > > > +
        > > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
        > > >  {
        > > >  	struct drm_device *dev = crtc->dev;
        > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
        > > >  	intel_crtc->cursor_base = base;
        > > >  }
        > > >  
        > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
        > > > +}
        > > > +
        > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
        > > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
        > > >  				     bool on)
        > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
        > > >  	}
        > > >  	primary->pipe = pipe;
        > > >  	primary->plane = pipe;
        > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
        > > > +		primary->plane = !pipe;
        > > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
        > > >  	primary->check_plane = intel_check_primary_plane;
        > > >  	primary->commit_plane = intel_commit_primary_plane;
        > > >  	primary->disable_plane = intel_disable_primary_plane;
        > > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
        > > > -		primary->plane = !pipe;
        > > > +
        > > > +	if (INTEL_INFO(dev)->gen >= 9)
        > > > +		primary->get_hw_state = skylake_primary_get_hw_state;
        > > > +	else
        > > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
        > > >  
        > > >  	if (INTEL_INFO(dev)->gen >= 9) {
        > > >  		intel_primary_formats = skl_primary_formats;
        > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
        > > >  	cursor->commit_plane = intel_commit_cursor_plane;
        > > >  	cursor->disable_plane = intel_disable_cursor_plane;
        > > >  
        > > > +	if (IS_845G(dev) || IS_I865G(dev))
        > > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
        > > > +	else
        > > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
        > > > +
        > > >  	drm_universal_plane_init(dev, &cursor->base, 0,
        > > >  				 &intel_plane_funcs,
        > > >  				 intel_cursor_formats,
        > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
        > > >  
        > > >  		/* Disable everything but the primary plane */
        > > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
        > > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
        > > > +			struct intel_plane_state *state =
        > > > +				to_intel_plane_state(plane->base.state);
        > > > +
        > > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
        > > > +			    !state->visible)
        > > >  				continue;
        > > >  
        > > >  			plane->disable_plane(&plane->base, &crtc->base);
        > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
        > > >  	i915_redisable_vga_power_on(dev);
        > > >  }
        > > >  
        > > > -static bool primary_get_hw_state(struct intel_plane *plane)
        > > > -{
        > > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > -
        > > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
        > > > -}
        > > > -
        > > >  /* FIXME read out full plane state for all planes */
        > > >  static void readout_plane_state(struct intel_crtc *crtc)
        > > >  {
        > > > -	struct intel_plane_state *plane_state =
        > > > -		to_intel_plane_state(crtc->base.primary->state);
        > > > +	struct drm_device *dev = crtc->base.dev;
        > > > +	struct intel_plane *plane;
        > > >  
        > > > -	plane_state->visible =
        > > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
        > > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
        > > > +		struct intel_plane_state *state =
        > > > +			to_intel_plane_state(plane->base.state);
        > > > +
        > > > +		state->visible = plane->get_hw_state(plane);
        > > > +	}
        > > >  }
        > > >  
        > > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
        > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
        > > > index 46484e4..64d6d90 100644
        > > > --- a/drivers/gpu/drm/i915/intel_drv.h
        > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
        > > > @@ -628,6 +628,7 @@ struct intel_plane {
        > > >  			   struct intel_plane_state *state);
        > > >  	void (*commit_plane)(struct drm_plane *plane,
        > > >  			     struct intel_plane_state *state);
        > > > +	bool (*get_hw_state)(struct intel_plane *plane);
        > > >  };
        > > >  
        > > >  struct intel_watermark_params {
        > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
        > > > index ca7e264..8b2c744 100644
        > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
        > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
        > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
        > > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
        > > >  }
        > > >  
        > > > +static bool
        > > > +skl_plane_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(PLANE_CTL(plane->pipe,
        > > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
        > > > +}
        > > > +
        > > >  static void
        > > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
        > > >  {
        > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
        > > >  	POSTING_READ(SPSURF(pipe, plane));
        > > >  }
        > > >  
        > > > +static bool
        > > > +vlv_plane_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
        > > > +}
        > > > +
        > > >  static void
        > > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
        > > >  		 struct drm_framebuffer *fb,
        > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        > > >  	POSTING_READ(SPRSURF(pipe));
        > > >  }
        > > >  
        > > > +static bool
        > > > +ivb_plane_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
        > > > +}
        > > > +
        > > >  static void
        > > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
        > > >  		 struct drm_framebuffer *fb,
        > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        > > >  	POSTING_READ(DVSSURF(pipe));
        > > >  }
        > > >  
        > > > +static bool
        > > > +ilk_plane_get_hw_state(struct intel_plane *plane)
        > > > +{
        > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
        > > > +
        > > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
        > > > +}
        > > > +
        > > >  static int
        > > >  intel_check_sprite_plane(struct drm_plane *plane,
        > > >  			 struct intel_crtc_state *crtc_state,
        > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
        > > >  		intel_plane->max_downscale = 16;
        > > >  		intel_plane->update_plane = ilk_update_plane;
        > > >  		intel_plane->disable_plane = ilk_disable_plane;
        > > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
        > > >  
        > > >  		if (IS_GEN6(dev)) {
        > > >  			plane_formats = snb_plane_formats;
        > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
        > > >  		if (IS_VALLEYVIEW(dev)) {
        > > >  			intel_plane->update_plane = vlv_update_plane;
        > > >  			intel_plane->disable_plane = vlv_disable_plane;
        > > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
        > > >  
        > > >  			plane_formats = vlv_plane_formats;
        > > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
        > > >  		} else {
        > > >  			intel_plane->update_plane = ivb_update_plane;
        > > >  			intel_plane->disable_plane = ivb_disable_plane;
        > > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
        > > >  
        > > >  			plane_formats = snb_plane_formats;
        > > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
        > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
        > > >  		intel_plane->can_scale = true;
        > > >  		intel_plane->update_plane = skl_update_plane;
        > > >  		intel_plane->disable_plane = skl_disable_plane;
        > > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
        > > >  		state->scaler_id = -1;
        > > >  
        > > >  		plane_formats = skl_plane_formats;
        > > > -- 
        > > > 2.4.6
        > > > 
        > > > _______________________________________________
        > > > 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
        > 
        > -- 
        > Ville Syrjälä
        > Intel OTC
        
        -- 
        Ville Syrjälä
        Intel OTC
        _______________________________________________
        Intel-gfx mailing list
        Intel-gfx@lists.freedesktop.org
        http://lists.freedesktop.org/mailman/listinfo/intel-gfx
        
        ^ permalink raw reply	[flat|nested] 19+ messages in thread

      • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
            2015-09-10 16:20       ` Ville Syrjälä
          @ 2015-09-10 16:30         ` Daniel Vetter
            2015-09-10 16:36           ` Ville Syrjälä
            0 siblings, 1 reply; 19+ messages in thread
          From: Daniel Vetter @ 2015-09-10 16:30 UTC (permalink / raw)
            To: Ville Syrjälä; +Cc: intel-gfx
          
          On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
          > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
          > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
          > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
          > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
          > > > > 
          > > > > Add a .get_hw_state() method for planes, returning true or false
          > > > > depending on whether the plane is enabled. Use it to populate the
          > > > > plane state 'visible' during state readout.
          > > > > 
          > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
          > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
          > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
          > > > 
          > > > Do we really need this? The plane readout is fairly limited and we don't
          > > > really care about recovering anything but the initial primary plane on
          > > > driver load. Anything else can just be nuked if it misfits ...
          > > 
          > > At least avoids calling ->disable_plane for an already disabled plane.
          > > We could even put a WARN there to catch broken code.
          > 
          > Oh and we could also do something like
          > for_each_plane()
          > 	assert(visible == get_hw_state);
          > 
          > somewhere to perhaps catch even more crap.
          
          Yeah maybe, but I think that should be done as part of the patch series
          with this patch here. As-is I don't think it adds a lot really ...
          -Daniel
          
          > 
          > 
          > > 
          > > > -Daniel
          > > > 
          > > > > ---
          > > > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
          > > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
          > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
          > > > >  3 files changed, 91 insertions(+), 14 deletions(-)
          > > > > 
          > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
          > > > > index 3707212..5fed120 100644
          > > > > --- a/drivers/gpu/drm/i915/intel_display.c
          > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
          > > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
          > > > >  	POSTING_READ(reg);
          > > > >  }
          > > > >  
          > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
          > > > > +}
          > > > > +
          > > > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
          > > > >  			      uint32_t pixel_format)
          > > > >  {
          > > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
          > > > >  	POSTING_READ(PLANE_SURF(pipe, 0));
          > > > >  }
          > > > >  
          > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
          > > > > +}
          > > > > +
          > > > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
          > > > >  static int
          > > > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
          > > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
          > > > >  	}
          > > > >  }
          > > > >  
          > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
          > > > > +}
          > > > > +
          > > > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
          > > > >  {
          > > > >  	struct drm_device *dev = crtc->dev;
          > > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
          > > > >  	intel_crtc->cursor_base = base;
          > > > >  }
          > > > >  
          > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
          > > > > +}
          > > > > +
          > > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
          > > > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
          > > > >  				     bool on)
          > > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
          > > > >  	}
          > > > >  	primary->pipe = pipe;
          > > > >  	primary->plane = pipe;
          > > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
          > > > > +		primary->plane = !pipe;
          > > > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
          > > > >  	primary->check_plane = intel_check_primary_plane;
          > > > >  	primary->commit_plane = intel_commit_primary_plane;
          > > > >  	primary->disable_plane = intel_disable_primary_plane;
          > > > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
          > > > > -		primary->plane = !pipe;
          > > > > +
          > > > > +	if (INTEL_INFO(dev)->gen >= 9)
          > > > > +		primary->get_hw_state = skylake_primary_get_hw_state;
          > > > > +	else
          > > > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
          > > > >  
          > > > >  	if (INTEL_INFO(dev)->gen >= 9) {
          > > > >  		intel_primary_formats = skl_primary_formats;
          > > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
          > > > >  	cursor->commit_plane = intel_commit_cursor_plane;
          > > > >  	cursor->disable_plane = intel_disable_cursor_plane;
          > > > >  
          > > > > +	if (IS_845G(dev) || IS_I865G(dev))
          > > > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
          > > > > +	else
          > > > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
          > > > > +
          > > > >  	drm_universal_plane_init(dev, &cursor->base, 0,
          > > > >  				 &intel_plane_funcs,
          > > > >  				 intel_cursor_formats,
          > > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
          > > > >  
          > > > >  		/* Disable everything but the primary plane */
          > > > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
          > > > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
          > > > > +			struct intel_plane_state *state =
          > > > > +				to_intel_plane_state(plane->base.state);
          > > > > +
          > > > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
          > > > > +			    !state->visible)
          > > > >  				continue;
          > > > >  
          > > > >  			plane->disable_plane(&plane->base, &crtc->base);
          > > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
          > > > >  	i915_redisable_vga_power_on(dev);
          > > > >  }
          > > > >  
          > > > > -static bool primary_get_hw_state(struct intel_plane *plane)
          > > > > -{
          > > > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > -
          > > > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
          > > > > -}
          > > > > -
          > > > >  /* FIXME read out full plane state for all planes */
          > > > >  static void readout_plane_state(struct intel_crtc *crtc)
          > > > >  {
          > > > > -	struct intel_plane_state *plane_state =
          > > > > -		to_intel_plane_state(crtc->base.primary->state);
          > > > > +	struct drm_device *dev = crtc->base.dev;
          > > > > +	struct intel_plane *plane;
          > > > >  
          > > > > -	plane_state->visible =
          > > > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
          > > > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
          > > > > +		struct intel_plane_state *state =
          > > > > +			to_intel_plane_state(plane->base.state);
          > > > > +
          > > > > +		state->visible = plane->get_hw_state(plane);
          > > > > +	}
          > > > >  }
          > > > >  
          > > > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
          > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
          > > > > index 46484e4..64d6d90 100644
          > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
          > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
          > > > > @@ -628,6 +628,7 @@ struct intel_plane {
          > > > >  			   struct intel_plane_state *state);
          > > > >  	void (*commit_plane)(struct drm_plane *plane,
          > > > >  			     struct intel_plane_state *state);
          > > > > +	bool (*get_hw_state)(struct intel_plane *plane);
          > > > >  };
          > > > >  
          > > > >  struct intel_watermark_params {
          > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
          > > > > index ca7e264..8b2c744 100644
          > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
          > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
          > > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
          > > > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
          > > > >  }
          > > > >  
          > > > > +static bool
          > > > > +skl_plane_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(PLANE_CTL(plane->pipe,
          > > > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
          > > > > +}
          > > > > +
          > > > >  static void
          > > > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
          > > > >  {
          > > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
          > > > >  	POSTING_READ(SPSURF(pipe, plane));
          > > > >  }
          > > > >  
          > > > > +static bool
          > > > > +vlv_plane_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
          > > > > +}
          > > > > +
          > > > >  static void
          > > > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
          > > > >  		 struct drm_framebuffer *fb,
          > > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
          > > > >  	POSTING_READ(SPRSURF(pipe));
          > > > >  }
          > > > >  
          > > > > +static bool
          > > > > +ivb_plane_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
          > > > > +}
          > > > > +
          > > > >  static void
          > > > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
          > > > >  		 struct drm_framebuffer *fb,
          > > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
          > > > >  	POSTING_READ(DVSSURF(pipe));
          > > > >  }
          > > > >  
          > > > > +static bool
          > > > > +ilk_plane_get_hw_state(struct intel_plane *plane)
          > > > > +{
          > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
          > > > > +
          > > > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
          > > > > +}
          > > > > +
          > > > >  static int
          > > > >  intel_check_sprite_plane(struct drm_plane *plane,
          > > > >  			 struct intel_crtc_state *crtc_state,
          > > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
          > > > >  		intel_plane->max_downscale = 16;
          > > > >  		intel_plane->update_plane = ilk_update_plane;
          > > > >  		intel_plane->disable_plane = ilk_disable_plane;
          > > > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
          > > > >  
          > > > >  		if (IS_GEN6(dev)) {
          > > > >  			plane_formats = snb_plane_formats;
          > > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
          > > > >  		if (IS_VALLEYVIEW(dev)) {
          > > > >  			intel_plane->update_plane = vlv_update_plane;
          > > > >  			intel_plane->disable_plane = vlv_disable_plane;
          > > > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
          > > > >  
          > > > >  			plane_formats = vlv_plane_formats;
          > > > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
          > > > >  		} else {
          > > > >  			intel_plane->update_plane = ivb_update_plane;
          > > > >  			intel_plane->disable_plane = ivb_disable_plane;
          > > > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
          > > > >  
          > > > >  			plane_formats = snb_plane_formats;
          > > > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
          > > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
          > > > >  		intel_plane->can_scale = true;
          > > > >  		intel_plane->update_plane = skl_update_plane;
          > > > >  		intel_plane->disable_plane = skl_disable_plane;
          > > > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
          > > > >  		state->scaler_id = -1;
          > > > >  
          > > > >  		plane_formats = skl_plane_formats;
          > > > > -- 
          > > > > 2.4.6
          > > > > 
          > > > > _______________________________________________
          > > > > 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
          > > 
          > > -- 
          > > Ville Syrjälä
          > > Intel OTC
          > 
          > -- 
          > Ville Syrjälä
          > Intel OTC
          
          -- 
          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
          
          ^ permalink raw reply	[flat|nested] 19+ messages in thread

        • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
              2015-09-10 16:30         ` Daniel Vetter
            @ 2015-09-10 16:36           ` Ville Syrjälä
              2015-09-14  9:18             ` Daniel Vetter
              0 siblings, 1 reply; 19+ messages in thread
            From: Ville Syrjälä @ 2015-09-10 16:36 UTC (permalink / raw)
              To: Daniel Vetter; +Cc: intel-gfx
            
            On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote:
            > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
            > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
            > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
            > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
            > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
            > > > > > 
            > > > > > Add a .get_hw_state() method for planes, returning true or false
            > > > > > depending on whether the plane is enabled. Use it to populate the
            > > > > > plane state 'visible' during state readout.
            > > > > > 
            > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
            > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
            > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
            > > > > 
            > > > > Do we really need this? The plane readout is fairly limited and we don't
            > > > > really care about recovering anything but the initial primary plane on
            > > > > driver load. Anything else can just be nuked if it misfits ...
            > > > 
            > > > At least avoids calling ->disable_plane for an already disabled plane.
            > > > We could even put a WARN there to catch broken code.
            > > 
            > > Oh and we could also do something like
            > > for_each_plane()
            > > 	assert(visible == get_hw_state);
            > > 
            > > somewhere to perhaps catch even more crap.
            > 
            > Yeah maybe, but I think that should be done as part of the patch series
            > with this patch here. As-is I don't think it adds a lot really ...
            
            It would at least fix SKL/BXT primary readout, which I forgot to mention
            in the commit message naturally.
            
            > -Daniel
            > 
            > > 
            > > 
            > > > 
            > > > > -Daniel
            > > > > 
            > > > > > ---
            > > > > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
            > > > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
            > > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
            > > > > >  3 files changed, 91 insertions(+), 14 deletions(-)
            > > > > > 
            > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
            > > > > > index 3707212..5fed120 100644
            > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
            > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
            > > > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
            > > > > >  	POSTING_READ(reg);
            > > > > >  }
            > > > > >  
            > > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
            > > > > >  			      uint32_t pixel_format)
            > > > > >  {
            > > > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
            > > > > >  	POSTING_READ(PLANE_SURF(pipe, 0));
            > > > > >  }
            > > > > >  
            > > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
            > > > > >  static int
            > > > > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
            > > > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
            > > > > >  	}
            > > > > >  }
            > > > > >  
            > > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
            > > > > >  {
            > > > > >  	struct drm_device *dev = crtc->dev;
            > > > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
            > > > > >  	intel_crtc->cursor_base = base;
            > > > > >  }
            > > > > >  
            > > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
            > > > > > +}
            > > > > > +
            > > > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
            > > > > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
            > > > > >  				     bool on)
            > > > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
            > > > > >  	}
            > > > > >  	primary->pipe = pipe;
            > > > > >  	primary->plane = pipe;
            > > > > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
            > > > > > +		primary->plane = !pipe;
            > > > > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
            > > > > >  	primary->check_plane = intel_check_primary_plane;
            > > > > >  	primary->commit_plane = intel_commit_primary_plane;
            > > > > >  	primary->disable_plane = intel_disable_primary_plane;
            > > > > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
            > > > > > -		primary->plane = !pipe;
            > > > > > +
            > > > > > +	if (INTEL_INFO(dev)->gen >= 9)
            > > > > > +		primary->get_hw_state = skylake_primary_get_hw_state;
            > > > > > +	else
            > > > > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
            > > > > >  
            > > > > >  	if (INTEL_INFO(dev)->gen >= 9) {
            > > > > >  		intel_primary_formats = skl_primary_formats;
            > > > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
            > > > > >  	cursor->commit_plane = intel_commit_cursor_plane;
            > > > > >  	cursor->disable_plane = intel_disable_cursor_plane;
            > > > > >  
            > > > > > +	if (IS_845G(dev) || IS_I865G(dev))
            > > > > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
            > > > > > +	else
            > > > > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
            > > > > > +
            > > > > >  	drm_universal_plane_init(dev, &cursor->base, 0,
            > > > > >  				 &intel_plane_funcs,
            > > > > >  				 intel_cursor_formats,
            > > > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
            > > > > >  
            > > > > >  		/* Disable everything but the primary plane */
            > > > > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
            > > > > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
            > > > > > +			struct intel_plane_state *state =
            > > > > > +				to_intel_plane_state(plane->base.state);
            > > > > > +
            > > > > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
            > > > > > +			    !state->visible)
            > > > > >  				continue;
            > > > > >  
            > > > > >  			plane->disable_plane(&plane->base, &crtc->base);
            > > > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
            > > > > >  	i915_redisable_vga_power_on(dev);
            > > > > >  }
            > > > > >  
            > > > > > -static bool primary_get_hw_state(struct intel_plane *plane)
            > > > > > -{
            > > > > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > -
            > > > > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
            > > > > > -}
            > > > > > -
            > > > > >  /* FIXME read out full plane state for all planes */
            > > > > >  static void readout_plane_state(struct intel_crtc *crtc)
            > > > > >  {
            > > > > > -	struct intel_plane_state *plane_state =
            > > > > > -		to_intel_plane_state(crtc->base.primary->state);
            > > > > > +	struct drm_device *dev = crtc->base.dev;
            > > > > > +	struct intel_plane *plane;
            > > > > >  
            > > > > > -	plane_state->visible =
            > > > > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
            > > > > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
            > > > > > +		struct intel_plane_state *state =
            > > > > > +			to_intel_plane_state(plane->base.state);
            > > > > > +
            > > > > > +		state->visible = plane->get_hw_state(plane);
            > > > > > +	}
            > > > > >  }
            > > > > >  
            > > > > >  static void intel_modeset_readout_hw_state(struct drm_device *dev)
            > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
            > > > > > index 46484e4..64d6d90 100644
            > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
            > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
            > > > > > @@ -628,6 +628,7 @@ struct intel_plane {
            > > > > >  			   struct intel_plane_state *state);
            > > > > >  	void (*commit_plane)(struct drm_plane *plane,
            > > > > >  			     struct intel_plane_state *state);
            > > > > > +	bool (*get_hw_state)(struct intel_plane *plane);
            > > > > >  };
            > > > > >  
            > > > > >  struct intel_watermark_params {
            > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
            > > > > > index ca7e264..8b2c744 100644
            > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
            > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
            > > > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
            > > > > >  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false);
            > > > > >  }
            > > > > >  
            > > > > > +static bool
            > > > > > +skl_plane_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(PLANE_CTL(plane->pipe,
            > > > > > +				   plane->plane + 1)) & PLANE_CTL_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  static void
            > > > > >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
            > > > > >  {
            > > > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
            > > > > >  	POSTING_READ(SPSURF(pipe, plane));
            > > > > >  }
            > > > > >  
            > > > > > +static bool
            > > > > > +vlv_plane_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  static void
            > > > > >  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
            > > > > >  		 struct drm_framebuffer *fb,
            > > > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
            > > > > >  	POSTING_READ(SPRSURF(pipe));
            > > > > >  }
            > > > > >  
            > > > > > +static bool
            > > > > > +ivb_plane_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  static void
            > > > > >  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
            > > > > >  		 struct drm_framebuffer *fb,
            > > > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
            > > > > >  	POSTING_READ(DVSSURF(pipe));
            > > > > >  }
            > > > > >  
            > > > > > +static bool
            > > > > > +ilk_plane_get_hw_state(struct intel_plane *plane)
            > > > > > +{
            > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
            > > > > > +
            > > > > > +	return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE;
            > > > > > +}
            > > > > > +
            > > > > >  static int
            > > > > >  intel_check_sprite_plane(struct drm_plane *plane,
            > > > > >  			 struct intel_crtc_state *crtc_state,
            > > > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
            > > > > >  		intel_plane->max_downscale = 16;
            > > > > >  		intel_plane->update_plane = ilk_update_plane;
            > > > > >  		intel_plane->disable_plane = ilk_disable_plane;
            > > > > > +		intel_plane->get_hw_state = ilk_plane_get_hw_state;
            > > > > >  
            > > > > >  		if (IS_GEN6(dev)) {
            > > > > >  			plane_formats = snb_plane_formats;
            > > > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
            > > > > >  		if (IS_VALLEYVIEW(dev)) {
            > > > > >  			intel_plane->update_plane = vlv_update_plane;
            > > > > >  			intel_plane->disable_plane = vlv_disable_plane;
            > > > > > +			intel_plane->get_hw_state = vlv_plane_get_hw_state;
            > > > > >  
            > > > > >  			plane_formats = vlv_plane_formats;
            > > > > >  			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
            > > > > >  		} else {
            > > > > >  			intel_plane->update_plane = ivb_update_plane;
            > > > > >  			intel_plane->disable_plane = ivb_disable_plane;
            > > > > > +			intel_plane->get_hw_state = ivb_plane_get_hw_state;
            > > > > >  
            > > > > >  			plane_formats = snb_plane_formats;
            > > > > >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
            > > > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
            > > > > >  		intel_plane->can_scale = true;
            > > > > >  		intel_plane->update_plane = skl_update_plane;
            > > > > >  		intel_plane->disable_plane = skl_disable_plane;
            > > > > > +		intel_plane->get_hw_state = skl_plane_get_hw_state;
            > > > > >  		state->scaler_id = -1;
            > > > > >  
            > > > > >  		plane_formats = skl_plane_formats;
            > > > > > -- 
            > > > > > 2.4.6
            > > > > > 
            > > > > > _______________________________________________
            > > > > > 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
            > > > 
            > > > -- 
            > > > Ville Syrjälä
            > > > Intel OTC
            > > 
            > > -- 
            > > Ville Syrjälä
            > > Intel OTC
            > 
            > -- 
            > Daniel Vetter
            > Software Engineer, Intel Corporation
            > http://blog.ffwll.ch
            
            -- 
            Ville Syrjälä
            Intel OTC
            _______________________________________________
            Intel-gfx mailing list
            Intel-gfx@lists.freedesktop.org
            http://lists.freedesktop.org/mailman/listinfo/intel-gfx
            
            ^ permalink raw reply	[flat|nested] 19+ messages in thread

          • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
                2015-09-10 16:36           ` Ville Syrjälä
              @ 2015-09-14  9:18             ` Daniel Vetter
                0 siblings, 0 replies; 19+ messages in thread
              From: Daniel Vetter @ 2015-09-14  9:18 UTC (permalink / raw)
                To: Ville Syrjälä; +Cc: intel-gfx
              
              On Thu, Sep 10, 2015 at 07:36:39PM +0300, Ville Syrjälä wrote:
              > On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote:
              > > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
              > > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
              > > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
              > > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
              > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
              > > > > > > 
              > > > > > > Add a .get_hw_state() method for planes, returning true or false
              > > > > > > depending on whether the plane is enabled. Use it to populate the
              > > > > > > plane state 'visible' during state readout.
              > > > > > > 
              > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
              > > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
              > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
              > > > > > 
              > > > > > Do we really need this? The plane readout is fairly limited and we don't
              > > > > > really care about recovering anything but the initial primary plane on
              > > > > > driver load. Anything else can just be nuked if it misfits ...
              > > > > 
              > > > > At least avoids calling ->disable_plane for an already disabled plane.
              > > > > We could even put a WARN there to catch broken code.
              > > > 
              > > > Oh and we could also do something like
              > > > for_each_plane()
              > > > 	assert(visible == get_hw_state);
              > > > 
              > > > somewhere to perhaps catch even more crap.
              > > 
              > > Yeah maybe, but I think that should be done as part of the patch series
              > > with this patch here. As-is I don't think it adds a lot really ...
              > 
              > It would at least fix SKL/BXT primary readout, which I forgot to mention
              > in the commit message naturally.
              
              Yeah, please hide the bugfix less ;-) Adding ->get_hw_state shouldn't be
              the subject either, it should be something like "Add plane hw state
              readouf for skl" and the commit message then explains why you added a new
              vfunc.
              
              And we should have some idea what to do about the get_initial_plane_config
              hook - having two very similar hooks is a bit confusing imo.
              -Daniel
              -- 
              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
              
              ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
      2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
      2015-09-10 16:07   ` Daniel Vetter
    @ 2015-09-14 12:10   ` Maarten Lankhorst
      2015-09-14 13:15     ` Ville Syrjälä
      1 sibling, 1 reply; 19+ messages in thread
    From: Maarten Lankhorst @ 2015-09-14 12:10 UTC (permalink / raw)
      To: ville.syrjala, intel-gfx
    
    Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
    > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
    >
    > Add a .get_hw_state() method for planes, returning true or false
    > depending on whether the plane is enabled. Use it to populate the
    > plane state 'visible' during state readout.
    >
    > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > ---
    >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
    >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
    >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
    >  3 files changed, 91 insertions(+), 14 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > index 3707212..5fed120 100644
    > --- a/drivers/gpu/drm/i915/intel_display.c
    > +++ b/drivers/gpu/drm/i915/intel_display.c
    > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
    >  	POSTING_READ(reg);
    >  }
    >  
    > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
    > +}
    > +
    >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
    >  			      uint32_t pixel_format)
    >  {
    > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
    >  	POSTING_READ(PLANE_SURF(pipe, 0));
    >  }
    >  
    > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
    > +}
    > +
    >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
    >  static int
    >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
    > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
    >  	}
    >  }
    >  
    > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
    > +}
    > +
    >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
    >  {
    >  	struct drm_device *dev = crtc->dev;
    > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
    >  	intel_crtc->cursor_base = base;
    >  }
    >  
    > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
    > +{
    > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > +
    > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
    > +}
    > +
    >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
    >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
    >  				     bool on)
    > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
    >  	}
    >  	primary->pipe = pipe;
    >  	primary->plane = pipe;
    > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
    > +		primary->plane = !pipe;
    >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
    >  	primary->check_plane = intel_check_primary_plane;
    >  	primary->commit_plane = intel_commit_primary_plane;
    >  	primary->disable_plane = intel_disable_primary_plane;
    > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
    > -		primary->plane = !pipe;
    > +
    > +	if (INTEL_INFO(dev)->gen >= 9)
    > +		primary->get_hw_state = skylake_primary_get_hw_state;
    > +	else
    > +		primary->get_hw_state = i9xx_primary_get_hw_state;
    >  
    >  	if (INTEL_INFO(dev)->gen >= 9) {
    >  		intel_primary_formats = skl_primary_formats;
    > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
    >  	cursor->commit_plane = intel_commit_cursor_plane;
    >  	cursor->disable_plane = intel_disable_cursor_plane;
    >  
    > +	if (IS_845G(dev) || IS_I865G(dev))
    > +		cursor->get_hw_state = i845_cursor_get_hw_state;
    > +	else
    > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
    > +
    >  	drm_universal_plane_init(dev, &cursor->base, 0,
    >  				 &intel_plane_funcs,
    >  				 intel_cursor_formats,
    > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
    >  
    >  		/* Disable everything but the primary plane */
    >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
    > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
    > +			struct intel_plane_state *state =
    > +				to_intel_plane_state(plane->base.state);
    > +
    > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
    > +			    !state->visible)
    >  				continue;
    >  
    >  			plane->disable_plane(&plane->base, &crtc->base);
    > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
    >  	i915_redisable_vga_power_on(dev);
    >  }
    >  
    > -static bool primary_get_hw_state(struct intel_plane *plane)
    > -{
    > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
    > -
    > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
    > -}
    > -
    >  /* FIXME read out full plane state for all planes */
    >  static void readout_plane_state(struct intel_crtc *crtc)
    >  {
    > -	struct intel_plane_state *plane_state =
    > -		to_intel_plane_state(crtc->base.primary->state);
    > +	struct drm_device *dev = crtc->base.dev;
    > +	struct intel_plane *plane;
    >  
    > -	plane_state->visible =
    > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
    > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
    > +		struct intel_plane_state *state =
    > +			to_intel_plane_state(plane->base.state);
    > +
    > +		state->visible = plane->get_hw_state(plane);
    > +	}
    >  }
    >
    I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state.
    I don't think you need it here, blindly calling disable_plane would be good enough if it's done
    after setting up mode.
    
    ~Maarten
    _______________________________________________
    Intel-gfx mailing list
    Intel-gfx@lists.freedesktop.org
    http://lists.freedesktop.org/mailman/listinfo/intel-gfx
    
    ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • • * Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
        2015-09-14 12:10   ` Maarten Lankhorst
      @ 2015-09-14 13:15     ` Ville Syrjälä
        0 siblings, 0 replies; 19+ messages in thread
      From: Ville Syrjälä @ 2015-09-14 13:15 UTC (permalink / raw)
        To: Maarten Lankhorst; +Cc: intel-gfx
      
      On Mon, Sep 14, 2015 at 02:10:20PM +0200, Maarten Lankhorst wrote:
      > Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
      > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > >
      > > Add a .get_hw_state() method for planes, returning true or false
      > > depending on whether the plane is enabled. Use it to populate the
      > > plane state 'visible' during state readout.
      > >
      > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > > ---
      > >  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++--------
      > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
      > >  drivers/gpu/drm/i915/intel_sprite.c  | 37 ++++++++++++++++++++
      > >  3 files changed, 91 insertions(+), 14 deletions(-)
      > >
      > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
      > > index 3707212..5fed120 100644
      > > --- a/drivers/gpu/drm/i915/intel_display.c
      > > +++ b/drivers/gpu/drm/i915/intel_display.c
      > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
      > >  	POSTING_READ(reg);
      > >  }
      > >  
      > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE;
      > > +}
      > > +
      > >  u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
      > >  			      uint32_t pixel_format)
      > >  {
      > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
      > >  	POSTING_READ(PLANE_SURF(pipe, 0));
      > >  }
      > >  
      > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE;
      > > +}
      > > +
      > >  /* Assume fb object is pinned & idle & fenced and just update base pointers */
      > >  static int
      > >  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
      > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  	}
      > >  }
      > >  
      > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(_CURACNTR) & CURSOR_ENABLE;
      > > +}
      > > +
      > >  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  {
      > >  	struct drm_device *dev = crtc->dev;
      > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
      > >  	intel_crtc->cursor_base = base;
      > >  }
      > >  
      > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane)
      > > +{
      > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > +
      > > +	return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE;
      > > +}
      > > +
      > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
      > >  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
      > >  				     bool on)
      > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
      > >  	}
      > >  	primary->pipe = pipe;
      > >  	primary->plane = pipe;
      > > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
      > > +		primary->plane = !pipe;
      > >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
      > >  	primary->check_plane = intel_check_primary_plane;
      > >  	primary->commit_plane = intel_commit_primary_plane;
      > >  	primary->disable_plane = intel_disable_primary_plane;
      > > -	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
      > > -		primary->plane = !pipe;
      > > +
      > > +	if (INTEL_INFO(dev)->gen >= 9)
      > > +		primary->get_hw_state = skylake_primary_get_hw_state;
      > > +	else
      > > +		primary->get_hw_state = i9xx_primary_get_hw_state;
      > >  
      > >  	if (INTEL_INFO(dev)->gen >= 9) {
      > >  		intel_primary_formats = skl_primary_formats;
      > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
      > >  	cursor->commit_plane = intel_commit_cursor_plane;
      > >  	cursor->disable_plane = intel_disable_cursor_plane;
      > >  
      > > +	if (IS_845G(dev) || IS_I865G(dev))
      > > +		cursor->get_hw_state = i845_cursor_get_hw_state;
      > > +	else
      > > +		cursor->get_hw_state = i9xx_cursor_get_hw_state;
      > > +
      > >  	drm_universal_plane_init(dev, &cursor->base, 0,
      > >  				 &intel_plane_funcs,
      > >  				 intel_cursor_formats,
      > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
      > >  
      > >  		/* Disable everything but the primary plane */
      > >  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
      > > -			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
      > > +			struct intel_plane_state *state =
      > > +				to_intel_plane_state(plane->base.state);
      > > +
      > > +			if (plane->base.type == DRM_PLANE_TYPE_PRIMARY ||
      > > +			    !state->visible)
      > >  				continue;
      > >  
      > >  			plane->disable_plane(&plane->base, &crtc->base);
      > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev)
      > >  	i915_redisable_vga_power_on(dev);
      > >  }
      > >  
      > > -static bool primary_get_hw_state(struct intel_plane *plane)
      > > -{
      > > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
      > > -
      > > -	return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE;
      > > -}
      > > -
      > >  /* FIXME read out full plane state for all planes */
      > >  static void readout_plane_state(struct intel_crtc *crtc)
      > >  {
      > > -	struct intel_plane_state *plane_state =
      > > -		to_intel_plane_state(crtc->base.primary->state);
      > > +	struct drm_device *dev = crtc->base.dev;
      > > +	struct intel_plane *plane;
      > >  
      > > -	plane_state->visible =
      > > -		primary_get_hw_state(to_intel_plane(crtc->base.primary));
      > > +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
      > > +		struct intel_plane_state *state =
      > > +			to_intel_plane_state(plane->base.state);
      > > +
      > > +		state->visible = plane->get_hw_state(plane);
      > > +	}
      > >  }
      > >
      > I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state.
      
      That would be inconsistent with existing practices. .get_hw_state() != .get_config()
      
      -- 
      Ville Syrjälä
      Intel OTC
      _______________________________________________
      Intel-gfx mailing list
      Intel-gfx@lists.freedesktop.org
      http://lists.freedesktop.org/mailman/listinfo/intel-gfx
      
      ^ permalink raw reply	[flat|nested] 19+ messages in thread

• * [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
    2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
    2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
    2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
  @ 2015-09-10 15:59 ` ville.syrjala
    2015-09-14 11:57   ` Maarten Lankhorst
    2015-09-14 12:04   ` Patrik Jakobsson
    2015-09-14 11:36 ` [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout Patrik Jakobsson
    3 siblings, 2 replies; 19+ messages in thread
  From: ville.syrjala @ 2015-09-10 15:59 UTC (permalink / raw)
    To: intel-gfx
  
  From: Ville Syrjälä <ville.syrjala@linux.intel.com>
  
  intel_modeset_readout_hw_state() seems like the more appropriate place
  for populating the scanline_offset and timestamping constants than
  intel_sanitize_crtc() since they are basically part of the state we
  read out.
  
  Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
  Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
  Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
  ---
   drivers/gpu/drm/i915/intel_display.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
  index 5fed120..88d9466 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
   	if (crtc->active) {
   		struct intel_plane *plane;
   
  -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
  -		update_scanline_offset(crtc);
   		drm_crtc_vblank_on(&crtc->base);
   
   		/* Disable everything but the primary plane */
  @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
   			 * recalculation.
   			 */
   			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
  +
  +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
  +			update_scanline_offset(crtc);
   		}
   	}
   }
  -- 
  2.4.6
  
  _______________________________________________
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
  
  ^ permalink raw reply related	[flat|nested] 19+ messages in thread

• • * Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
      2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
    @ 2015-09-14 11:57   ` Maarten Lankhorst
      2015-09-14 14:23     ` Ville Syrjälä
      2015-09-14 12:04   ` Patrik Jakobsson
      1 sibling, 1 reply; 19+ messages in thread
    From: Maarten Lankhorst @ 2015-09-14 11:57 UTC (permalink / raw)
      To: ville.syrjala, intel-gfx
    
    Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
    > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
    >
    > intel_modeset_readout_hw_state() seems like the more appropriate place
    > for populating the scanline_offset and timestamping constants than
    > intel_sanitize_crtc() since they are basically part of the state we
    > read out.
    >
    > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > ---
    >  drivers/gpu/drm/i915/intel_display.c | 5 +++--
    >  1 file changed, 3 insertions(+), 2 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > index 5fed120..88d9466 100644
    > --- a/drivers/gpu/drm/i915/intel_display.c
    > +++ b/drivers/gpu/drm/i915/intel_display.c
    > @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
    >  	if (crtc->active) {
    >  		struct intel_plane *plane;
    >  
    > -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
    > -		update_scanline_offset(crtc);
    >  		drm_crtc_vblank_on(&crtc->base);
    >  
    >  		/* Disable everything but the primary plane */
    > @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
    >  			 * recalculation.
    >  			 */
    >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
    > +
    > +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
    > +			update_scanline_offset(crtc);
    >  		}
    >  	}
    >  }
    Can you move drm_vblank_reset and drm_vblank_on too?
    _______________________________________________
    Intel-gfx mailing list
    Intel-gfx@lists.freedesktop.org
    http://lists.freedesktop.org/mailman/listinfo/intel-gfx
    
    ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • • * Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
        2015-09-14 11:57   ` Maarten Lankhorst
      @ 2015-09-14 14:23     ` Ville Syrjälä
        2015-09-21 12:25       ` Maarten Lankhorst
        0 siblings, 1 reply; 19+ messages in thread
      From: Ville Syrjälä @ 2015-09-14 14:23 UTC (permalink / raw)
        To: Maarten Lankhorst; +Cc: intel-gfx
      
      On Mon, Sep 14, 2015 at 01:57:45PM +0200, Maarten Lankhorst wrote:
      > Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
      > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > >
      > > intel_modeset_readout_hw_state() seems like the more appropriate place
      > > for populating the scanline_offset and timestamping constants than
      > > intel_sanitize_crtc() since they are basically part of the state we
      > > read out.
      > >
      > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > > ---
      > >  drivers/gpu/drm/i915/intel_display.c | 5 +++--
      > >  1 file changed, 3 insertions(+), 2 deletions(-)
      > >
      > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
      > > index 5fed120..88d9466 100644
      > > --- a/drivers/gpu/drm/i915/intel_display.c
      > > +++ b/drivers/gpu/drm/i915/intel_display.c
      > > @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
      > >  	if (crtc->active) {
      > >  		struct intel_plane *plane;
      > >  
      > > -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
      > > -		update_scanline_offset(crtc);
      > >  		drm_crtc_vblank_on(&crtc->base);
      > >  
      > >  		/* Disable everything but the primary plane */
      > > @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
      > >  			 * recalculation.
      > >  			 */
      > >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
      > > +
      > > +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
      > > +			update_scanline_offset(crtc);
      > >  		}
      > >  	}
      > >  }
      > Can you move drm_vblank_reset and drm_vblank_on too?
      
      I'm not sure I really want to move those. They can actually modify
      the hardware state, so I don't think they really belong in
      intel_modeset_readout_hw_state(). intel_sanitize_crtc() feels like
      a better fit.
      
      -- 
      Ville Syrjälä
      Intel OTC
      _______________________________________________
      Intel-gfx mailing list
      Intel-gfx@lists.freedesktop.org
      http://lists.freedesktop.org/mailman/listinfo/intel-gfx
      
      ^ permalink raw reply	[flat|nested] 19+ messages in thread

    • • * Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
          2015-09-14 14:23     ` Ville Syrjälä
        @ 2015-09-21 12:25       ` Maarten Lankhorst
          0 siblings, 0 replies; 19+ messages in thread
        From: Maarten Lankhorst @ 2015-09-21 12:25 UTC (permalink / raw)
          To: Ville Syrjälä; +Cc: intel-gfx
        
        Hey,
        
        Op 14-09-15 om 16:23 schreef Ville Syrjälä:
        > On Mon, Sep 14, 2015 at 01:57:45PM +0200, Maarten Lankhorst wrote:
        >> Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com:
        >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
        >>>
        >>> intel_modeset_readout_hw_state() seems like the more appropriate place
        >>> for populating the scanline_offset and timestamping constants than
        >>> intel_sanitize_crtc() since they are basically part of the state we
        >>> read out.
        >>>
        >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
        >>> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
        >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
        >>> ---
        >>>  drivers/gpu/drm/i915/intel_display.c | 5 +++--
        >>>  1 file changed, 3 insertions(+), 2 deletions(-)
        >>>
        >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
        >>> index 5fed120..88d9466 100644
        >>> --- a/drivers/gpu/drm/i915/intel_display.c
        >>> +++ b/drivers/gpu/drm/i915/intel_display.c
        >>> @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
        >>>  	if (crtc->active) {
        >>>  		struct intel_plane *plane;
        >>>  
        >>> -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
        >>> -		update_scanline_offset(crtc);
        >>>  		drm_crtc_vblank_on(&crtc->base);
        >>>  
        >>>  		/* Disable everything but the primary plane */
        >>> @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
        >>>  			 * recalculation.
        >>>  			 */
        >>>  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
        >>> +
        >>> +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
        >>> +			update_scanline_offset(crtc);
        >>>  		}
        >>>  	}
        >>>  }
        >> Can you move drm_vblank_reset and drm_vblank_on too?
        > I'm not sure I really want to move those. They can actually modify
        > the hardware state, so I don't think they really belong in
        > intel_modeset_readout_hw_state(). intel_sanitize_crtc() feels like
        > a better fit.
        Well the changes are all useful and they're blocking other work. So I think it's better these get in as is.
        
        Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
        
        _______________________________________________
        Intel-gfx mailing list
        Intel-gfx@lists.freedesktop.org
        http://lists.freedesktop.org/mailman/listinfo/intel-gfx
        
        ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • * Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
      2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
      2015-09-14 11:57   ` Maarten Lankhorst
    @ 2015-09-14 12:04   ` Patrik Jakobsson
      2015-09-23  8:33     ` Daniel Vetter
      1 sibling, 1 reply; 19+ messages in thread
    From: Patrik Jakobsson @ 2015-09-14 12:04 UTC (permalink / raw)
      To: ville.syrjala; +Cc: intel-gfx
    
    On Thu, Sep 10, 2015 at 06:59:10PM +0300, ville.syrjala@linux.intel.com wrote:
    > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
    > 
    > intel_modeset_readout_hw_state() seems like the more appropriate place
    > for populating the scanline_offset and timestamping constants than
    > intel_sanitize_crtc() since they are basically part of the state we
    > read out.
    > 
    > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    
    Makes sense to move it.
    
    Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
    > ---
    >  drivers/gpu/drm/i915/intel_display.c | 5 +++--
    >  1 file changed, 3 insertions(+), 2 deletions(-)
    > 
    > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
    > index 5fed120..88d9466 100644
    > --- a/drivers/gpu/drm/i915/intel_display.c
    > +++ b/drivers/gpu/drm/i915/intel_display.c
    > @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
    >  	if (crtc->active) {
    >  		struct intel_plane *plane;
    >  
    > -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
    > -		update_scanline_offset(crtc);
    >  		drm_crtc_vblank_on(&crtc->base);
    >  
    >  		/* Disable everything but the primary plane */
    > @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
    >  			 * recalculation.
    >  			 */
    >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
    > +
    > +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
    > +			update_scanline_offset(crtc);
    >  		}
    >  	}
    >  }
    > -- 
    > 2.4.6
    > 
    _______________________________________________
    Intel-gfx mailing list
    Intel-gfx@lists.freedesktop.org
    http://lists.freedesktop.org/mailman/listinfo/intel-gfx
    
    ^ permalink raw reply	[flat|nested] 19+ messages in thread

  • • * Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()
        2015-09-14 12:04   ` Patrik Jakobsson
      @ 2015-09-23  8:33     ` Daniel Vetter
        0 siblings, 0 replies; 19+ messages in thread
      From: Daniel Vetter @ 2015-09-23  8:33 UTC (permalink / raw)
        To: ville.syrjala, intel-gfx, Maarten Lankhorst
      
      On Mon, Sep 14, 2015 at 02:04:31PM +0200, Patrik Jakobsson wrote:
      > On Thu, Sep 10, 2015 at 06:59:10PM +0300, ville.syrjala@linux.intel.com wrote:
      > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > > 
      > > intel_modeset_readout_hw_state() seems like the more appropriate place
      > > for populating the scanline_offset and timestamping constants than
      > > intel_sanitize_crtc() since they are basically part of the state we
      > > read out.
      > > 
      > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
      > 
      > Makes sense to move it.
      > 
      > Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      
      Merged all but patch 3 (since that one seems to still be contended) to
      dinq.
      
      Thanks, Daniel
      
      > > ---
      > >  drivers/gpu/drm/i915/intel_display.c | 5 +++--
      > >  1 file changed, 3 insertions(+), 2 deletions(-)
      > > 
      > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
      > > index 5fed120..88d9466 100644
      > > --- a/drivers/gpu/drm/i915/intel_display.c
      > > +++ b/drivers/gpu/drm/i915/intel_display.c
      > > @@ -14918,8 +14918,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
      > >  	if (crtc->active) {
      > >  		struct intel_plane *plane;
      > >  
      > > -		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
      > > -		update_scanline_offset(crtc);
      > >  		drm_crtc_vblank_on(&crtc->base);
      > >  
      > >  		/* Disable everything but the primary plane */
      > > @@ -15216,6 +15214,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
      > >  			 * recalculation.
      > >  			 */
      > >  			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
      > > +
      > > +			drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
      > > +			update_scanline_offset(crtc);
      > >  		}
      > >  	}
      > >  }
      > > -- 
      > > 2.4.6
      > > 
      > _______________________________________________
      > 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
      
      ^ permalink raw reply	[flat|nested] 19+ messages in thread

• * Re: [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout
    2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
                     ` (2 preceding siblings ...)
    2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
  @ 2015-09-14 11:36 ` Patrik Jakobsson
    3 siblings, 0 replies; 19+ messages in thread
  From: Patrik Jakobsson @ 2015-09-14 11:36 UTC (permalink / raw)
    To: ville.syrjala; +Cc: intel-gfx
  
  On Thu, Sep 10, 2015 at 06:59:07PM +0300, ville.syrjala@linux.intel.com wrote:
  > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
  > 
  > The dotclock is often calculated in encoder .get_config(), so we
  > shouldn't copy the adjusted_mode to hwmode until we have read out the
  > dotclock.
  > 
  > Gets rid of some warnings like these:
  > [drm:drm_calc_timestamping_constants [drm]] *ERROR* crtc 21: Can't calculate constants, dotclock = 0!
  > [drm:i915_get_vblank_timestamp] crtc 0 is disabled
  > 
  > v2: Steal Maarten's idea to move crtc->mode etc. assignment too
  
  Tested and looks good. Here's a bug fixed by this patch.
  
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91428
  
  Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
  > 
  > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
  > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
  > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
  > ---
  >  drivers/gpu/drm/i915/intel_display.c | 57 +++++++++++++++++++-----------------
  >  1 file changed, 30 insertions(+), 27 deletions(-)
  > 
  > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
  > index af48c1e..f5673c8 100644
  > --- a/drivers/gpu/drm/i915/intel_display.c
  > +++ b/drivers/gpu/drm/i915/intel_display.c
  > @@ -15094,33 +15094,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
  >  		crtc->base.state->active = crtc->active;
  >  		crtc->base.enabled = crtc->active;
  >  
  > -		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
  > -		if (crtc->base.state->active) {
  > -			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
  > -			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
  > -			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
  > -
  > -			/*
  > -			 * The initial mode needs to be set in order to keep
  > -			 * the atomic core happy. It wants a valid mode if the
  > -			 * crtc's enabled, so we do the above call.
  > -			 *
  > -			 * At this point some state updated by the connectors
  > -			 * in their ->detect() callback has not run yet, so
  > -			 * no recalculation can be done yet.
  > -			 *
  > -			 * Even if we could do a recalculation and modeset
  > -			 * right now it would cause a double modeset if
  > -			 * fbdev or userspace chooses a different initial mode.
  > -			 *
  > -			 * If that happens, someone indicated they wanted a
  > -			 * mode change, which means it's safe to do a full
  > -			 * recalculation.
  > -			 */
  > -			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
  > -		}
  > -
  > -		crtc->base.hwmode = crtc->config->base.adjusted_mode;
  >  		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
  >  
  >  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
  > @@ -15180,6 +15153,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
  >  			      connector->base.name,
  >  			      connector->base.encoder ? "enabled" : "disabled");
  >  	}
  > +
  > +	for_each_intel_crtc(dev, crtc) {
  > +		crtc->base.hwmode = crtc->config->base.adjusted_mode;
  > +
  > +		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
  > +		if (crtc->base.state->active) {
  > +			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
  > +			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
  > +			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
  > +
  > +			/*
  > +			 * The initial mode needs to be set in order to keep
  > +			 * the atomic core happy. It wants a valid mode if the
  > +			 * crtc's enabled, so we do the above call.
  > +			 *
  > +			 * At this point some state updated by the connectors
  > +			 * in their ->detect() callback has not run yet, so
  > +			 * no recalculation can be done yet.
  > +			 *
  > +			 * Even if we could do a recalculation and modeset
  > +			 * right now it would cause a double modeset if
  > +			 * fbdev or userspace chooses a different initial mode.
  > +			 *
  > +			 * If that happens, someone indicated they wanted a
  > +			 * mode change, which means it's safe to do a full
  > +			 * recalculation.
  > +			 */
  > +			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
  > +		}
  > +	}
  >  }
  >  
  >  /* Scan out the current hw modeset state,
  > -- 
  > 2.4.6
  > 
  _______________________________________________
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
  
  ^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2015-09-23  8:30 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
2015-09-14 11:48   ` Patrik Jakobsson
2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
2015-09-10 16:07   ` Daniel Vetter
2015-09-10 16:13     ` Ville Syrjälä
2015-09-10 16:20       ` Ville Syrjälä
2015-09-10 16:30         ` Daniel Vetter
2015-09-10 16:36           ` Ville Syrjälä
2015-09-14  9:18             ` Daniel Vetter
2015-09-14 12:10   ` Maarten Lankhorst
2015-09-14 13:15     ` Ville Syrjälä
2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
2015-09-14 11:57   ` Maarten Lankhorst
2015-09-14 14:23     ` Ville Syrjälä
2015-09-21 12:25       ` Maarten Lankhorst
2015-09-14 12:04   ` Patrik Jakobsson
2015-09-23  8:33     ` Daniel Vetter
2015-09-14 11:36 ` [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout Patrik Jakobsson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.