[PATCH 1/4] drm/i915: Assign hwmode after encoder state readout

[PATCH 1/4] drm/i915: Assign hwmode after encoder state readout

[ville.syrjala]

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

[PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()

[ville.syrjala]

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

Re: [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()

[Patrik Jakobsson]

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

[PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[ville.syrjala]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Daniel Vetter]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Ville Syrjälä]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Ville Syrjälä]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Daniel Vetter]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Ville Syrjälä]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Daniel Vetter]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Maarten Lankhorst]

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

Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Ville Syrjälä]

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

[PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[ville.syrjala]

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

Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[Maarten Lankhorst]

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

Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[Maarten Lankhorst]

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

Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[Patrik Jakobsson]

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

Re: [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state()

[Daniel Vetter]

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

Re: [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout

[Patrik Jakobsson]

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