[PATCH 0/4] Fix more fallout from reverting atomic hw readout.

[PATCH 0/4] Fix more fallout from reverting atomic hw readout.

[Maarten Lankhorst]

Commit f662af8c5c1619 reverts
"drm/i915: Read hw state into an atomic state struct, v2."
but it doesn't take into account other changes that were done in that time.
Before I continue on the atomic fixes I want to fix the fallout first,
and some of the reasons I identified that could cause a failure for atomic
modeset.

When that's fixed I'll look at committing the atomic hw readout patch
again, since it will be needed for converting to full atomic.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929

Maarten Lankhorst (4):
  drm/i915: Do not use atomic modesets in hw readout.
  drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip
  drm/i915: Set hwmode during readout.
  drm/i915: Only enable cursor if it can be enabled.

 drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 45 deletions(-)

-- 
2.1.0

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

[PATCH 1/4] drm/i915: Do not use atomic modesets in hw readout.

[Maarten Lankhorst]

This should fix fallout caused by making intel_crtc_control
and update_dpms atomic, which became a problem after reverting the
atomic hw readout patch.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 75 +++++++++++++++---------------------
 1 file changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5cc2263db199..7abaffeda7ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6187,31 +6187,35 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_display_power_domain domain;
+	unsigned long domains;
+
+	if (!intel_crtc->active)
+		return;
+
+	intel_crtc_disable_planes(crtc);
+	dev_priv->display.crtc_disable(crtc);
+
+	domains = intel_crtc->enabled_power_domains;
+	for_each_power_domain(domain, domains)
+		intel_display_power_put(dev_priv, domain);
+	intel_crtc->enabled_power_domains = 0;
+}
+
 /*
  * turn all crtc's off, but do not adjust state
  * This has to be paired with a call to intel_modeset_setup_hw_state.
  */
 void intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *crtc;
 
-	for_each_crtc(dev, crtc) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-		enum intel_display_power_domain domain;
-		unsigned long domains;
-
-		if (!intel_crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
-
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
-	}
+	for_each_crtc(dev, crtc)
+		intel_crtc_disable_noatomic(crtc);
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
@@ -15120,7 +15124,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
 	u32 reg;
+	bool enable;
 
 	/* Clear any frame start delays used for debugging left by the BIOS */
 	reg = PIPECONF(crtc->config->cpu_transcoder);
@@ -15137,7 +15143,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	 * disable the crtc (and hence change the state) if it is wrong. Note
 	 * that gen4+ has a fixed plane -> pipe mapping.  */
 	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
-		struct intel_connector *connector;
 		bool plane;
 
 		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
@@ -15149,29 +15154,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		plane = crtc->plane;
 		to_intel_plane_state(crtc->base.primary->state)->visible = true;
 		crtc->plane = !plane;
-		intel_crtc_control(&crtc->base, false);
+		intel_crtc_disable_noatomic(&crtc->base);
 		crtc->plane = plane;
-
-		/* ... and break all links. */
-		for_each_intel_connector(dev, connector) {
-			if (connector->encoder->base.crtc != &crtc->base)
-				continue;
-
-			connector->base.dpms = DRM_MODE_DPMS_OFF;
-			connector->base.encoder = NULL;
-		}
-		/* multiple connectors may have the same encoder:
-		 *  handle them and break crtc link separately */
-		for_each_intel_connector(dev, connector)
-			if (connector->encoder->base.crtc == &crtc->base) {
-				connector->encoder->base.crtc = NULL;
-				connector->encoder->connectors_active = false;
-			}
-
-		WARN_ON(crtc->active);
-		crtc->base.state->enable = false;
-		crtc->base.state->active = false;
-		crtc->base.enabled = false;
 	}
 
 	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
@@ -15185,13 +15169,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
-	intel_crtc_update_dpms(&crtc->base);
+	enable = false;
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
+		enable |= encoder->connectors_active;
+
+	if (!enable)
+		intel_crtc_disable_noatomic(&crtc->base);
 
 	if (crtc->active != crtc->base.state->active) {
-		struct intel_encoder *encoder;
 
 		/* This can happen either due to bugs in the get_hw_state
-		 * functions or because the pipe is force-enabled due to the
+		 * functions or because of calls to intel_crtc_disable_noatomic,
+		 * or because the pipe is force-enabled due to the
 		 * pipe A quirk. */
 		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
 			      crtc->base.base.id,
-- 
2.1.0

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

Re: [PATCH 1/4] drm/i915: Do not use atomic modesets in hw readout.

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 11:15:39AM +0200, Maarten Lankhorst wrote:
> This should fix fallout caused by making intel_crtc_control
> and update_dpms atomic, which became a problem after reverting the
> atomic hw readout patch.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

So far my 830 and gen4 seem happy with this.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 75 +++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5cc2263db199..7abaffeda7ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6187,31 +6187,35 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum intel_display_power_domain domain;
> +	unsigned long domains;
> +
> +	if (!intel_crtc->active)
> +		return;
> +
> +	intel_crtc_disable_planes(crtc);
> +	dev_priv->display.crtc_disable(crtc);
> +
> +	domains = intel_crtc->enabled_power_domains;
> +	for_each_power_domain(domain, domains)
> +		intel_display_power_put(dev_priv, domain);
> +	intel_crtc->enabled_power_domains = 0;
> +}
> +
>  /*
>   * turn all crtc's off, but do not adjust state
>   * This has to be paired with a call to intel_modeset_setup_hw_state.
>   */
>  void intel_display_suspend(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *crtc;
>  
> -	for_each_crtc(dev, crtc) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -		enum intel_display_power_domain domain;
> -		unsigned long domains;
> -
> -		if (!intel_crtc->active)
> -			continue;
> -
> -		intel_crtc_disable_planes(crtc);
> -		dev_priv->display.crtc_disable(crtc);
> -
> -		domains = intel_crtc->enabled_power_domains;
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_put(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = 0;
> -	}
> +	for_each_crtc(dev, crtc)
> +		intel_crtc_disable_noatomic(crtc);
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> @@ -15120,7 +15124,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *encoder;
>  	u32 reg;
> +	bool enable;
>  
>  	/* Clear any frame start delays used for debugging left by the BIOS */
>  	reg = PIPECONF(crtc->config->cpu_transcoder);
> @@ -15137,7 +15143,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
>  	 * that gen4+ has a fixed plane -> pipe mapping.  */
>  	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
> -		struct intel_connector *connector;
>  		bool plane;
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
> @@ -15149,29 +15154,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		plane = crtc->plane;
>  		to_intel_plane_state(crtc->base.primary->state)->visible = true;
>  		crtc->plane = !plane;
> -		intel_crtc_control(&crtc->base, false);
> +		intel_crtc_disable_noatomic(&crtc->base);
>  		crtc->plane = plane;
> -
> -		/* ... and break all links. */
> -		for_each_intel_connector(dev, connector) {
> -			if (connector->encoder->base.crtc != &crtc->base)
> -				continue;
> -
> -			connector->base.dpms = DRM_MODE_DPMS_OFF;
> -			connector->base.encoder = NULL;
> -		}
> -		/* multiple connectors may have the same encoder:
> -		 *  handle them and break crtc link separately */
> -		for_each_intel_connector(dev, connector)
> -			if (connector->encoder->base.crtc == &crtc->base) {
> -				connector->encoder->base.crtc = NULL;
> -				connector->encoder->connectors_active = false;
> -			}
> -
> -		WARN_ON(crtc->active);
> -		crtc->base.state->enable = false;
> -		crtc->base.state->active = false;
> -		crtc->base.enabled = false;
>  	}
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> @@ -15185,13 +15169,18 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  	/* Adjust the state of the output pipe according to whether we
>  	 * have active connectors/encoders. */
> -	intel_crtc_update_dpms(&crtc->base);
> +	enable = false;
> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
> +		enable |= encoder->connectors_active;
> +
> +	if (!enable)
> +		intel_crtc_disable_noatomic(&crtc->base);
>  
>  	if (crtc->active != crtc->base.state->active) {
> -		struct intel_encoder *encoder;
>  
>  		/* This can happen either due to bugs in the get_hw_state
> -		 * functions or because the pipe is force-enabled due to the
> +		 * functions or because of calls to intel_crtc_disable_noatomic,
> +		 * or because the pipe is force-enabled due to the
>  		 * pipe A quirk. */
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
>  			      crtc->base.base.id,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Maarten Lankhorst]

Use a full atomic call instead. intel_crtc_page_flip will still
have to live until async updates are allowed.

This doesn't seem to be a regression from the convert to atomic,
part 3 patch. During GPU reset it fixes the following warning:

 ------------[ cut here ]------------
WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
Modules linked in: i915
CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
 ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
 0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
 ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
Call Trace:
 [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
 [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
 [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
 [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
 [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
 [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
 [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
 [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
 [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
 [<ffffffff811f6001>] ? expand_files+0x261/0x270
 [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
 [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
 [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
---[ end trace 9ce834560085bd64 ]---

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7abaffeda7ce..cdf6549c8e74 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11618,8 +11618,35 @@ free_work:
 	kfree(work);
 
 	if (ret == -EIO) {
+		struct drm_atomic_state *state;
+		struct drm_plane_state *plane_state;
+
 out_hang:
-		ret = intel_plane_restore(primary);
+		state = drm_atomic_state_alloc(dev);
+		if (!state)
+			return -ENOMEM;
+		state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+
+retry:
+		plane_state = drm_atomic_get_plane_state(state, primary);
+		ret = PTR_ERR_OR_ZERO(plane_state);
+		if (!ret) {
+			drm_atomic_set_fb_for_plane(plane_state, fb);
+
+			ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+			if (!ret)
+				ret = drm_atomic_commit(state);
+		}
+
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(state->acquire_ctx);
+			drm_atomic_state_clear(state);
+			goto retry;
+		}
+
+		if (ret)
+			drm_atomic_state_free(state);
+
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
-- 
2.1.0

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
> Use a full atomic call instead. intel_crtc_page_flip will still
> have to live until async updates are allowed.
> 
> This doesn't seem to be a regression from the convert to atomic,
> part 3 patch. During GPU reset it fixes the following warning:
> 
>  ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
> Modules linked in: i915
> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
> Call Trace:
>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
> ---[ end trace 9ce834560085bd64 ]---
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abaffeda7ce..cdf6549c8e74 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11618,8 +11618,35 @@ free_work:
>  	kfree(work);
>  
>  	if (ret == -EIO) {
> +		struct drm_atomic_state *state;
> +		struct drm_plane_state *plane_state;
> +
>  out_hang:
> -		ret = intel_plane_restore(primary);

So what exactly is wrong with intel_plane_restore() (ie.
drm_plane_helper_update())? Shouldn't you fix that instead of spreading
the uglyness here?

> +		state = drm_atomic_state_alloc(dev);
> +		if (!state)
> +			return -ENOMEM;
> +		state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> +
> +retry:
> +		plane_state = drm_atomic_get_plane_state(state, primary);
> +		ret = PTR_ERR_OR_ZERO(plane_state);
> +		if (!ret) {
> +			drm_atomic_set_fb_for_plane(plane_state, fb);
> +
> +			ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +			if (!ret)
> +				ret = drm_atomic_commit(state);
> +		}
> +
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(state->acquire_ctx);
> +			drm_atomic_state_clear(state);
> +			goto retry;
> +		}
> +
> +		if (ret)
> +			drm_atomic_state_free(state);
> +
>  		if (ret == 0 && event) {
>  			spin_lock_irq(&dev->event_lock);
>  			drm_send_vblank_event(dev, pipe, event);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Maarten Lankhorst]

Op 12-06-15 om 12:16 schreef Ville Syrjälä:
> On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
>> Use a full atomic call instead. intel_crtc_page_flip will still
>> have to live until async updates are allowed.
>>
>> This doesn't seem to be a regression from the convert to atomic,
>> part 3 patch. During GPU reset it fixes the following warning:
>>
>>  ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
>> Modules linked in: i915
>> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
>> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
>>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
>>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
>>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
>> Call Trace:
>>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
>>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
>>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
>>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
>>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
>>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
>>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
>>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
>>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
>> ---[ end trace 9ce834560085bd64 ]---
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 7abaffeda7ce..cdf6549c8e74 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11618,8 +11618,35 @@ free_work:
>>  	kfree(work);
>>  
>>  	if (ret == -EIO) {
>> +		struct drm_atomic_state *state;
>> +		struct drm_plane_state *plane_state;
>> +
>>  out_hang:
>> -		ret = intel_plane_restore(primary);
> So what exactly is wrong with intel_plane_restore() (ie.
> drm_plane_helper_update())? Shouldn't you fix that instead of spreading
> the uglyness here?
>
intel_plane_restore uses the transitional helpers. This is currently used for
setting color key, enabling sprite planes after a modeset and this function.
- Color key will be made atomic by making ckey part of the plane state.
- Sprite plane updates after modeset will be made unnecessary by calling
  drm_atomic_helper_commit_planes_on_crtc after .crtc_enable.
- This function needs to update plane->fb, and intel_plane_restore doesn't provide that.

That leaves only this function calling intel_plane_restore, if we can get rid of it
there will be no need of transitional helper calls any more.

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 12:31:37PM +0200, Maarten Lankhorst wrote:
> Op 12-06-15 om 12:16 schreef Ville Syrjälä:
> > On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
> >> Use a full atomic call instead. intel_crtc_page_flip will still
> >> have to live until async updates are allowed.
> >>
> >> This doesn't seem to be a regression from the convert to atomic,
> >> part 3 patch. During GPU reset it fixes the following warning:
> >>
> >>  ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
> >> Modules linked in: i915
> >> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
> >> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
> >>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
> >>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
> >>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
> >> Call Trace:
> >>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
> >>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> >>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
> >>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
> >>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
> >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> >>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
> >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> >>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
> >>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
> >>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
> >>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
> >>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
> >> ---[ end trace 9ce834560085bd64 ]---
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 7abaffeda7ce..cdf6549c8e74 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -11618,8 +11618,35 @@ free_work:
> >>  	kfree(work);
> >>  
> >>  	if (ret == -EIO) {
> >> +		struct drm_atomic_state *state;
> >> +		struct drm_plane_state *plane_state;
> >> +
> >>  out_hang:
> >> -		ret = intel_plane_restore(primary);
> > So what exactly is wrong with intel_plane_restore() (ie.
> > drm_plane_helper_update())? Shouldn't you fix that instead of spreading
> > the uglyness here?
> >
> intel_plane_restore uses the transitional helpers. This is currently used for
> setting color key, enabling sprite planes after a modeset and this function.
> - Color key will be made atomic by making ckey part of the plane state.

Well what function do you plan to call there? The same should work here
no?

> - Sprite plane updates after modeset will be made unnecessary by calling
>   drm_atomic_helper_commit_planes_on_crtc after .crtc_enable.
> - This function needs to update plane->fb, and intel_plane_restore doesn't provide that.
> 
> That leaves only this function calling intel_plane_restore, if we can get rid of it
> there will be no need of transitional helper calls any more.

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 02:27:54PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2015 at 12:31:37PM +0200, Maarten Lankhorst wrote:
> > Op 12-06-15 om 12:16 schreef Ville Syrjälä:
> > > On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
> > >> Use a full atomic call instead. intel_crtc_page_flip will still
> > >> have to live until async updates are allowed.
> > >>
> > >> This doesn't seem to be a regression from the convert to atomic,
> > >> part 3 patch. During GPU reset it fixes the following warning:
> > >>
> > >>  ------------[ cut here ]------------
> > >> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
> > >> Modules linked in: i915
> > >> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
> > >> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
> > >>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
> > >>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
> > >>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
> > >> Call Trace:
> > >>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
> > >>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> > >>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
> > >>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
> > >>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
> > >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> > >>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
> > >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> > >>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
> > >>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
> > >>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
> > >>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
> > >>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
> > >> ---[ end trace 9ce834560085bd64 ]---
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
> > >>  1 file changed, 28 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >> index 7abaffeda7ce..cdf6549c8e74 100644
> > >> --- a/drivers/gpu/drm/i915/intel_display.c
> > >> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> @@ -11618,8 +11618,35 @@ free_work:
> > >>  	kfree(work);
> > >>  
> > >>  	if (ret == -EIO) {
> > >> +		struct drm_atomic_state *state;
> > >> +		struct drm_plane_state *plane_state;
> > >> +
> > >>  out_hang:
> > >> -		ret = intel_plane_restore(primary);
> > > So what exactly is wrong with intel_plane_restore() (ie.
> > > drm_plane_helper_update())? Shouldn't you fix that instead of spreading
> > > the uglyness here?
> > >
> > intel_plane_restore uses the transitional helpers. This is currently used for
> > setting color key, enabling sprite planes after a modeset and this function.
> > - Color key will be made atomic by making ckey part of the plane state.
> 
> Well what function do you plan to call there? The same should work here
> no?

To elaborate a bit more. My main gripe here is all this boilerplate we
need. I hope we're planning to abstract that away a bit so we don't have
to keep repeating it everywhere.

So maybe it could look something like this:

ret = prep_plane_update()
set stuff in the state
ret = check_and_commit()

That way the EDEADLOCK mess etc. would be hidden inside the
prep part.

In the meantime I'd at least stuff this thing into separate functions so
that the ugly if (!ret) stuff would go away.

> 
> > - Sprite plane updates after modeset will be made unnecessary by calling
> >   drm_atomic_helper_commit_planes_on_crtc after .crtc_enable.
> > - This function needs to update plane->fb, and intel_plane_restore doesn't provide that.
> > 
> > That leaves only this function calling intel_plane_restore, if we can get rid of it
> > there will be no need of transitional helper calls any more.
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Maarten Lankhorst]

Op 12-06-15 om 13:43 schreef Ville Syrjälä:
> On Fri, Jun 12, 2015 at 02:27:54PM +0300, Ville Syrjälä wrote:
>> On Fri, Jun 12, 2015 at 12:31:37PM +0200, Maarten Lankhorst wrote:
>>> Op 12-06-15 om 12:16 schreef Ville Syrjälä:
>>>> On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
>>>>> Use a full atomic call instead. intel_crtc_page_flip will still
>>>>> have to live until async updates are allowed.
>>>>>
>>>>> This doesn't seem to be a regression from the convert to atomic,
>>>>> part 3 patch. During GPU reset it fixes the following warning:
>>>>>
>>>>>  ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
>>>>> Modules linked in: i915
>>>>> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
>>>>> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
>>>>>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
>>>>>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
>>>>>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
>>>>> Call Trace:
>>>>>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
>>>>>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
>>>>>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
>>>>>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
>>>>>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
>>>>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>>>>>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
>>>>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>>>>>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
>>>>>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
>>>>>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
>>>>>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
>>>>>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
>>>>> ---[ end trace 9ce834560085bd64 ]---
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 7abaffeda7ce..cdf6549c8e74 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -11618,8 +11618,35 @@ free_work:
>>>>>  	kfree(work);
>>>>>  
>>>>>  	if (ret == -EIO) {
>>>>> +		struct drm_atomic_state *state;
>>>>> +		struct drm_plane_state *plane_state;
>>>>> +
>>>>>  out_hang:
>>>>> -		ret = intel_plane_restore(primary);
>>>> So what exactly is wrong with intel_plane_restore() (ie.
>>>> drm_plane_helper_update())? Shouldn't you fix that instead of spreading
>>>> the uglyness here?
>>>>
>>> intel_plane_restore uses the transitional helpers. This is currently used for
>>> setting color key, enabling sprite planes after a modeset and this function.
>>> - Color key will be made atomic by making ckey part of the plane state.
>> Well what function do you plan to call there? The same should work here
>> no?
> To elaborate a bit more. My main gripe here is all this boilerplate we
> need. I hope we're planning to abstract that away a bit so we don't have
> to keep repeating it everywhere.

Eventually color key should probably become a property, but how does this look?

Adding color key with this function should be easy.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7abaffeda7ce..cda85eca7174 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11431,6 +11431,53 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
 	spin_unlock(&dev->event_lock);
 }
 
+static int intel_update_plane_state(struct drm_plane *plane,
+				    struct drm_modeset_acquire_ctx *acquire_ctx,
+				    int (*update_fn)(struct drm_plane_state *, void *data),
+				    void *data)
+{
+	struct drm_atomic_state *state;
+	struct drm_plane_state *plane_state;
+	int ret;
+
+	state = drm_atomic_state_alloc(plane->dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = acquire_ctx;
+
+retry:
+	plane_state = drm_atomic_get_plane_state(state, plane);
+	ret = PTR_ERR_OR_ZERO(plane_state);
+	if (!ret)
+		ret = update_fn(plane_state, data);
+
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(state->acquire_ctx);
+		drm_atomic_state_clear(state);
+		goto retry;
+	}
+
+	if (ret)
+		drm_atomic_state_free(state);
+
+	return ret;
+}
+
+static int
+intel_crtc_page_flip_update_fb(struct drm_plane_state *plane_state, void *data)
+{
+	drm_atomic_set_fb_for_plane(plane_state, data);
+
+	if (WARN_ON(!plane_state->crtc))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int intel_crtc_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
@@ -11619,7 +11666,8 @@ free_work:
 
 	if (ret == -EIO) {
 out_hang:
-		ret = intel_plane_restore(primary);
+		ret = intel_update_plane_state(primary, drm_modeset_legacy_acquire_ctx(crtc), intel_crtc_page_flip_update_fb, fb);
+
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);

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

Re: [PATCH 2/4] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Daniel Vetter]

On Fri, Jun 12, 2015 at 03:17:04PM +0200, Maarten Lankhorst wrote:
> Op 12-06-15 om 13:43 schreef Ville Syrjälä:
> > On Fri, Jun 12, 2015 at 02:27:54PM +0300, Ville Syrjälä wrote:
> >> On Fri, Jun 12, 2015 at 12:31:37PM +0200, Maarten Lankhorst wrote:
> >>> Op 12-06-15 om 12:16 schreef Ville Syrjälä:
> >>>> On Fri, Jun 12, 2015 at 11:15:40AM +0200, Maarten Lankhorst wrote:
> >>>>> Use a full atomic call instead. intel_crtc_page_flip will still
> >>>>> have to live until async updates are allowed.
> >>>>>
> >>>>> This doesn't seem to be a regression from the convert to atomic,
> >>>>> part 3 patch. During GPU reset it fixes the following warning:
> >>>>>
> >>>>>  ------------[ cut here ]------------
> >>>>> WARNING: CPU: 0 PID: 752 at drivers/gpu/drm/drm_crtc.c:5337 drm_mode_page_flip_ioctl+0x27b/0x360()
> >>>>> Modules linked in: i915
> >>>>> CPU: 0 PID: 752 Comm: Xorg Not tainted 4.1.0-rc7-patser+ #4090
> >>>>> Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
> >>>>>  ffffffff81c90866 ffff8800d87c3ca8 ffffffff817f7d87 0000000080000001
> >>>>>  0000000000000000 ffff8800d87c3ce8 ffffffff81084955 ffff880000000000
> >>>>>  ffff8800d87c3dc0 ffff8800d93d1208 0000000000000000 ffff8800b7d1f3e0
> >>>>> Call Trace:
> >>>>>  [<ffffffff817f7d87>] dump_stack+0x4f/0x7b
> >>>>>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> >>>>>  [<ffffffff81084a35>] warn_slowpath_null+0x15/0x20
> >>>>>  [<ffffffff8146dffb>] drm_mode_page_flip_ioctl+0x27b/0x360
> >>>>>  [<ffffffff8145ccb0>] drm_ioctl+0x1a0/0x6a0
> >>>>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> >>>>>  [<ffffffff812e5540>] ? avc_has_perm+0x20/0x280
> >>>>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> >>>>>  [<ffffffff811ea0f8>] do_vfs_ioctl+0x2f8/0x530
> >>>>>  [<ffffffff811f6001>] ? expand_files+0x261/0x270
> >>>>>  [<ffffffff812e7c16>] ? selinux_file_ioctl+0x56/0x100
> >>>>>  [<ffffffff811ea3b1>] SyS_ioctl+0x81/0xa0
> >>>>>  [<ffffffff81801b97>] system_call_fastpath+0x12/0x6f
> >>>>> ---[ end trace 9ce834560085bd64 ]---
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 7abaffeda7ce..cdf6549c8e74 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -11618,8 +11618,35 @@ free_work:
> >>>>>  	kfree(work);
> >>>>>  
> >>>>>  	if (ret == -EIO) {
> >>>>> +		struct drm_atomic_state *state;
> >>>>> +		struct drm_plane_state *plane_state;
> >>>>> +
> >>>>>  out_hang:
> >>>>> -		ret = intel_plane_restore(primary);
> >>>> So what exactly is wrong with intel_plane_restore() (ie.
> >>>> drm_plane_helper_update())? Shouldn't you fix that instead of spreading
> >>>> the uglyness here?
> >>>>
> >>> intel_plane_restore uses the transitional helpers. This is currently used for
> >>> setting color key, enabling sprite planes after a modeset and this function.
> >>> - Color key will be made atomic by making ckey part of the plane state.
> >> Well what function do you plan to call there? The same should work here
> >> no?
> > To elaborate a bit more. My main gripe here is all this boilerplate we
> > need. I hope we're planning to abstract that away a bit so we don't have
> > to keep repeating it everywhere.
> 
> Eventually color key should probably become a property, but how does this look?
> 
> Adding color key with this function should be easy.

Yeah once everything goes properly through atomic props and helpers this
boilerplate state frobbing should go away. Until then we just have to bear
the cost of having tons of driver-specific logic all over the place in
i915. Note a whole lot we can do about that.

In the end I hope the only thing we really need to keep around is state
frobberry for the hw state readout (the force-restore should be doable
with drm atomic helpers to save/restore all atomic state) and in the load
detect code (that's truly driver private).
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abaffeda7ce..cda85eca7174 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11431,6 +11431,53 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
>  	spin_unlock(&dev->event_lock);
>  }
>  
> +static int intel_update_plane_state(struct drm_plane *plane,
> +				    struct drm_modeset_acquire_ctx *acquire_ctx,
> +				    int (*update_fn)(struct drm_plane_state *, void *data),
> +				    void *data)
> +{
> +	struct drm_atomic_state *state;
> +	struct drm_plane_state *plane_state;
> +	int ret;
> +
> +	state = drm_atomic_state_alloc(plane->dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = acquire_ctx;
> +
> +retry:
> +	plane_state = drm_atomic_get_plane_state(state, plane);
> +	ret = PTR_ERR_OR_ZERO(plane_state);
> +	if (!ret)
> +		ret = update_fn(plane_state, data);
> +
> +	if (!ret)
> +		ret = drm_atomic_commit(state);
> +
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(state->acquire_ctx);
> +		drm_atomic_state_clear(state);
> +		goto retry;
> +	}
> +
> +	if (ret)
> +		drm_atomic_state_free(state);
> +
> +	return ret;
> +}
> +
> +static int
> +intel_crtc_page_flip_update_fb(struct drm_plane_state *plane_state, void *data)
> +{
> +	drm_atomic_set_fb_for_plane(plane_state, data);
> +
> +	if (WARN_ON(!plane_state->crtc))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
> @@ -11619,7 +11666,8 @@ free_work:
>  
>  	if (ret == -EIO) {
>  out_hang:
> -		ret = intel_plane_restore(primary);
> +		ret = intel_update_plane_state(primary, drm_modeset_legacy_acquire_ctx(crtc), intel_crtc_page_flip_update_fb, fb);
> +
>  		if (ret == 0 && event) {
>  			spin_lock_irq(&dev->event_lock);
>  			drm_send_vblank_event(dev, pipe, event);
> 
> _______________________________________________
> 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

[PATCH 3/4] drm/i915: Set hwmode during readout.

[Maarten Lankhorst]

This was introduced after converting hw readout to atomic,
so it should have been part of the revert too.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cdf6549c8e74..14ccf49b9067 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15357,6 +15357,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		crtc->base.state->enable = crtc->active;
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 
 		plane_state = to_intel_plane_state(primary->state);
 		plane_state->visible = primary_get_hw_state(crtc);
-- 
2.1.0

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

Re: [PATCH 3/4] drm/i915: Set hwmode during readout.

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 11:15:41AM +0200, Maarten Lankhorst wrote:
> This was introduced after converting hw readout to atomic,
> so it should have been part of the revert too.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cdf6549c8e74..14ccf49b9067 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15357,6 +15357,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		crtc->base.state->enable = crtc->active;
>  		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
> +		crtc->base.hwmode = crtc->config->base.adjusted_mode;
>  
>  		plane_state = to_intel_plane_state(primary->state);
>  		plane_state->visible = primary_get_hw_state(crtc);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: Only enable cursor if it can be enabled.

[Maarten Lankhorst]

The cursor should only be enabled if it's visible. This fixes
igt/kms_cursor_crc, which may otherwise produce the following
warning:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 3425 at drivers/gpu/drm/i915/intel_display.c:9995 intel_crtc_update_cursor+0x14c/0x4d0 [i915]()
Missing switch case (0) in i9xx_update_cursor
Modules linked in: i915
CPU: 0 PID: 3425 Comm: kms_cursor_crc Tainted: G        W       4.1.0-rc7-patser+ #4079
Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
 ffffffffc01aad10 ffff8800b083faa8 ffffffff817f7827 0000000080000001
 ffff8800b083faf8 ffff8800b083fae8 ffffffff81084955 ffff8800b083fad8
 ffff8800c4931148 0000000001200000 ffff8800c48b0000 0000000000000000
Call Trace:
 [<ffffffff817f7827>] dump_stack+0x4f/0x7b
 [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
 [<ffffffff810849d1>] warn_slowpath_fmt+0x41/0x50
 [<ffffffffc0139f2c>] intel_crtc_update_cursor+0x14c/0x4d0 [i915]
 [<ffffffffc01497f4>] __intel_set_mode+0x6c4/0x750 [i915]
 [<ffffffffc0150143>] intel_crtc_set_config+0x473/0x5c0 [i915]
 [<ffffffff81467da9>] drm_mode_set_config_internal+0x69/0x120
 [<ffffffff8146c1b9>] drm_mode_setcrtc+0x189/0x540
 [<ffffffff8145c7e0>] drm_ioctl+0x1a0/0x6a0
 [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
 [<ffffffff811e9c28>] do_vfs_ioctl+0x2f8/0x530
 [<ffffffff810d0f7d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff812e7746>] ? selinux_file_ioctl+0x56/0x100
 [<ffffffff811e9ee1>] SyS_ioctl+0x81/0xa0
 [<ffffffff81801617>] system_call_fastpath+0x12/0x6f
---[ end trace abf0f71163290a96 ]---

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 14ccf49b9067..afe91a8f7e36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4748,7 +4748,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 
 	intel_enable_primary_hw_plane(crtc->primary, crtc);
 	intel_enable_sprite_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
+	if (to_intel_plane_state(crtc->cursor->state)->visible)
+		intel_crtc_update_cursor(crtc, true);
 
 	intel_post_enable_primary(crtc);
 
-- 
2.1.0

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

Re: [PATCH 4/4] drm/i915: Only enable cursor if it can be enabled.

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 11:15:42AM +0200, Maarten Lankhorst wrote:
> The cursor should only be enabled if it's visible. This fixes
> igt/kms_cursor_crc, which may otherwise produce the following
> warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3425 at drivers/gpu/drm/i915/intel_display.c:9995 intel_crtc_update_cursor+0x14c/0x4d0 [i915]()
> Missing switch case (0) in i9xx_update_cursor
> Modules linked in: i915
> CPU: 0 PID: 3425 Comm: kms_cursor_crc Tainted: G        W       4.1.0-rc7-patser+ #4079
> Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
>  ffffffffc01aad10 ffff8800b083faa8 ffffffff817f7827 0000000080000001
>  ffff8800b083faf8 ffff8800b083fae8 ffffffff81084955 ffff8800b083fad8
>  ffff8800c4931148 0000000001200000 ffff8800c48b0000 0000000000000000
> Call Trace:
>  [<ffffffff817f7827>] dump_stack+0x4f/0x7b
>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
>  [<ffffffff810849d1>] warn_slowpath_fmt+0x41/0x50
>  [<ffffffffc0139f2c>] intel_crtc_update_cursor+0x14c/0x4d0 [i915]
>  [<ffffffffc01497f4>] __intel_set_mode+0x6c4/0x750 [i915]
>  [<ffffffffc0150143>] intel_crtc_set_config+0x473/0x5c0 [i915]
>  [<ffffffff81467da9>] drm_mode_set_config_internal+0x69/0x120
>  [<ffffffff8146c1b9>] drm_mode_setcrtc+0x189/0x540
>  [<ffffffff8145c7e0>] drm_ioctl+0x1a0/0x6a0
>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>  [<ffffffff811e9c28>] do_vfs_ioctl+0x2f8/0x530
>  [<ffffffff810d0f7d>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff812e7746>] ? selinux_file_ioctl+0x56/0x100
>  [<ffffffff811e9ee1>] SyS_ioctl+0x81/0xa0
>  [<ffffffff81801617>] system_call_fastpath+0x12/0x6f
> ---[ end trace abf0f71163290a96 ]---
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 14ccf49b9067..afe91a8f7e36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4748,7 +4748,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  
>  	intel_enable_primary_hw_plane(crtc->primary, crtc);
>  	intel_enable_sprite_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> +	if (to_intel_plane_state(crtc->cursor->state)->visible)
> +		intel_crtc_update_cursor(crtc, true);

Can we actually trust it now? Last time I looked we couldn't, and
Daniel didn't want my fixes to make it so.

So if we can't trust 'visible' I suppose you should just do
something a bit more ugly and look at 'crtc_w' or something.

>  
>  	intel_post_enable_primary(crtc);
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: Only enable cursor if it can be enabled.

[Maarten Lankhorst]

Op 12-06-15 om 12:26 schreef Ville Syrjälä:
> On Fri, Jun 12, 2015 at 11:15:42AM +0200, Maarten Lankhorst wrote:
>> The cursor should only be enabled if it's visible. This fixes
>> igt/kms_cursor_crc, which may otherwise produce the following
>> warning:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 3425 at drivers/gpu/drm/i915/intel_display.c:9995 intel_crtc_update_cursor+0x14c/0x4d0 [i915]()
>> Missing switch case (0) in i9xx_update_cursor
>> Modules linked in: i915
>> CPU: 0 PID: 3425 Comm: kms_cursor_crc Tainted: G        W       4.1.0-rc7-patser+ #4079
>> Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
>>  ffffffffc01aad10 ffff8800b083faa8 ffffffff817f7827 0000000080000001
>>  ffff8800b083faf8 ffff8800b083fae8 ffffffff81084955 ffff8800b083fad8
>>  ffff8800c4931148 0000000001200000 ffff8800c48b0000 0000000000000000
>> Call Trace:
>>  [<ffffffff817f7827>] dump_stack+0x4f/0x7b
>>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
>>  [<ffffffff810849d1>] warn_slowpath_fmt+0x41/0x50
>>  [<ffffffffc0139f2c>] intel_crtc_update_cursor+0x14c/0x4d0 [i915]
>>  [<ffffffffc01497f4>] __intel_set_mode+0x6c4/0x750 [i915]
>>  [<ffffffffc0150143>] intel_crtc_set_config+0x473/0x5c0 [i915]
>>  [<ffffffff81467da9>] drm_mode_set_config_internal+0x69/0x120
>>  [<ffffffff8146c1b9>] drm_mode_setcrtc+0x189/0x540
>>  [<ffffffff8145c7e0>] drm_ioctl+0x1a0/0x6a0
>>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
>>  [<ffffffff811e9c28>] do_vfs_ioctl+0x2f8/0x530
>>  [<ffffffff810d0f7d>] ? trace_hardirqs_on+0xd/0x10
>>  [<ffffffff812e7746>] ? selinux_file_ioctl+0x56/0x100
>>  [<ffffffff811e9ee1>] SyS_ioctl+0x81/0xa0
>>  [<ffffffff81801617>] system_call_fastpath+0x12/0x6f
>> ---[ end trace abf0f71163290a96 ]---
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 14ccf49b9067..afe91a8f7e36 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4748,7 +4748,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>>  
>>  	intel_enable_primary_hw_plane(crtc->primary, crtc);
>>  	intel_enable_sprite_planes(crtc);
>> -	intel_crtc_update_cursor(crtc, true);
>> +	if (to_intel_plane_state(crtc->cursor->state)->visible)
>> +		intel_crtc_update_cursor(crtc, true);
> Can we actually trust it now? Last time I looked we couldn't, and
> Daniel didn't want my fixes to make it so.
>
> So if we can't trust 'visible' I suppose you should just do
> something a bit more ugly and look at 'crtc_w' or something.
>
We add all planes during a modeset, so state->visible should be sane.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Only enable cursor if it can be enabled.

[Ville Syrjälä]

On Fri, Jun 12, 2015 at 12:32:24PM +0200, Maarten Lankhorst wrote:
> Op 12-06-15 om 12:26 schreef Ville Syrjälä:
> > On Fri, Jun 12, 2015 at 11:15:42AM +0200, Maarten Lankhorst wrote:
> >> The cursor should only be enabled if it's visible. This fixes
> >> igt/kms_cursor_crc, which may otherwise produce the following
> >> warning:
> >>
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 3425 at drivers/gpu/drm/i915/intel_display.c:9995 intel_crtc_update_cursor+0x14c/0x4d0 [i915]()
> >> Missing switch case (0) in i9xx_update_cursor
> >> Modules linked in: i915
> >> CPU: 0 PID: 3425 Comm: kms_cursor_crc Tainted: G        W       4.1.0-rc7-patser+ #4079
> >> Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
> >>  ffffffffc01aad10 ffff8800b083faa8 ffffffff817f7827 0000000080000001
> >>  ffff8800b083faf8 ffff8800b083fae8 ffffffff81084955 ffff8800b083fad8
> >>  ffff8800c4931148 0000000001200000 ffff8800c48b0000 0000000000000000
> >> Call Trace:
> >>  [<ffffffff817f7827>] dump_stack+0x4f/0x7b
> >>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> >>  [<ffffffff810849d1>] warn_slowpath_fmt+0x41/0x50
> >>  [<ffffffffc0139f2c>] intel_crtc_update_cursor+0x14c/0x4d0 [i915]
> >>  [<ffffffffc01497f4>] __intel_set_mode+0x6c4/0x750 [i915]
> >>  [<ffffffffc0150143>] intel_crtc_set_config+0x473/0x5c0 [i915]
> >>  [<ffffffff81467da9>] drm_mode_set_config_internal+0x69/0x120
> >>  [<ffffffff8146c1b9>] drm_mode_setcrtc+0x189/0x540
> >>  [<ffffffff8145c7e0>] drm_ioctl+0x1a0/0x6a0
> >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> >>  [<ffffffff811e9c28>] do_vfs_ioctl+0x2f8/0x530
> >>  [<ffffffff810d0f7d>] ? trace_hardirqs_on+0xd/0x10
> >>  [<ffffffff812e7746>] ? selinux_file_ioctl+0x56/0x100
> >>  [<ffffffff811e9ee1>] SyS_ioctl+0x81/0xa0
> >>  [<ffffffff81801617>] system_call_fastpath+0x12/0x6f
> >> ---[ end trace abf0f71163290a96 ]---
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 14ccf49b9067..afe91a8f7e36 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4748,7 +4748,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> >>  
> >>  	intel_enable_primary_hw_plane(crtc->primary, crtc);
> >>  	intel_enable_sprite_planes(crtc);
> >> -	intel_crtc_update_cursor(crtc, true);
> >> +	if (to_intel_plane_state(crtc->cursor->state)->visible)
> >> +		intel_crtc_update_cursor(crtc, true);
> > Can we actually trust it now? Last time I looked we couldn't, and
> > Daniel didn't want my fixes to make it so.
> >
> > So if we can't trust 'visible' I suppose you should just do
> > something a bit more ugly and look at 'crtc_w' or something.
> >
> We add all planes during a modeset, so state->visible should be sane.

If we don't already have it, I'd like to see a test case that does:
- set a big mode
- set up all kinds of planes near the bottom right corner
- set a small mode

This should test that all active planes get clipped properly during the
modeset.

-- 
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: Only enable cursor if it can be enabled.

[Daniel Vetter]

On Fri, Jun 12, 2015 at 02:49:50PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2015 at 12:32:24PM +0200, Maarten Lankhorst wrote:
> > Op 12-06-15 om 12:26 schreef Ville Syrjälä:
> > > On Fri, Jun 12, 2015 at 11:15:42AM +0200, Maarten Lankhorst wrote:
> > >> The cursor should only be enabled if it's visible. This fixes
> > >> igt/kms_cursor_crc, which may otherwise produce the following
> > >> warning:
> > >>
> > >> ------------[ cut here ]------------
> > >> WARNING: CPU: 0 PID: 3425 at drivers/gpu/drm/i915/intel_display.c:9995 intel_crtc_update_cursor+0x14c/0x4d0 [i915]()
> > >> Missing switch case (0) in i9xx_update_cursor
> > >> Modules linked in: i915
> > >> CPU: 0 PID: 3425 Comm: kms_cursor_crc Tainted: G        W       4.1.0-rc7-patser+ #4079
> > >> Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
> > >>  ffffffffc01aad10 ffff8800b083faa8 ffffffff817f7827 0000000080000001
> > >>  ffff8800b083faf8 ffff8800b083fae8 ffffffff81084955 ffff8800b083fad8
> > >>  ffff8800c4931148 0000000001200000 ffff8800c48b0000 0000000000000000
> > >> Call Trace:
> > >>  [<ffffffff817f7827>] dump_stack+0x4f/0x7b
> > >>  [<ffffffff81084955>] warn_slowpath_common+0x85/0xc0
> > >>  [<ffffffff810849d1>] warn_slowpath_fmt+0x41/0x50
> > >>  [<ffffffffc0139f2c>] intel_crtc_update_cursor+0x14c/0x4d0 [i915]
> > >>  [<ffffffffc01497f4>] __intel_set_mode+0x6c4/0x750 [i915]
> > >>  [<ffffffffc0150143>] intel_crtc_set_config+0x473/0x5c0 [i915]
> > >>  [<ffffffff81467da9>] drm_mode_set_config_internal+0x69/0x120
> > >>  [<ffffffff8146c1b9>] drm_mode_setcrtc+0x189/0x540
> > >>  [<ffffffff8145c7e0>] drm_ioctl+0x1a0/0x6a0
> > >>  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
> > >>  [<ffffffff811e9c28>] do_vfs_ioctl+0x2f8/0x530
> > >>  [<ffffffff810d0f7d>] ? trace_hardirqs_on+0xd/0x10
> > >>  [<ffffffff812e7746>] ? selinux_file_ioctl+0x56/0x100
> > >>  [<ffffffff811e9ee1>] SyS_ioctl+0x81/0xa0
> > >>  [<ffffffff81801617>] system_call_fastpath+0x12/0x6f
> > >> ---[ end trace abf0f71163290a96 ]---
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >> index 14ccf49b9067..afe91a8f7e36 100644
> > >> --- a/drivers/gpu/drm/i915/intel_display.c
> > >> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> @@ -4748,7 +4748,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> > >>
> > >>   intel_enable_primary_hw_plane(crtc->primary, crtc);
> > >>   intel_enable_sprite_planes(crtc);
> > >> - intel_crtc_update_cursor(crtc, true);
> > >> + if (to_intel_plane_state(crtc->cursor->state)->visible)
> > >> + intel_crtc_update_cursor(crtc, true);
> > > Can we actually trust it now? Last time I looked we couldn't, and
> > > Daniel didn't want my fixes to make it so.
> > >
> > > So if we can't trust 'visible' I suppose you should just do
> > > something a bit more ugly and look at 'crtc_w' or something.
> > >
> > We add all planes during a modeset, so state->visible should be sane.
>
> If we don't already have it, I'd like to see a test case that does:
> - set a big mode
> - set up all kinds of planes near the bottom right corner
> - set a small mode
>
> This should test that all active planes get clipped properly during the
> modeset.

Yeah sounds like a great idea. Adding Annie to make sure we don't
forget to track this properly in Jira.
-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 0/4] Fix more fallout from reverting atomic hw readout.

[Jani Nikula]

On Fri, 12 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> Commit f662af8c5c1619 reverts
> "drm/i915: Read hw state into an atomic state struct, v2."
> but it doesn't take into account other changes that were done in that time.
> Before I continue on the atomic fixes I want to fix the fallout first,
> and some of the reasons I identified that could cause a failure for atomic
> modeset.
>
> When that's fixed I'll look at committing the atomic hw readout patch
> again, since it will be needed for converting to full atomic.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929

To recap, the atomic conversion part 2 series [1] broke things
[2][3][4], and the quick fix reverts [5] broke other things [6], and now
we have these patches to fix that.

I was resolved to either merge these fixes or purge the whole thing this
afternoon. On the one hand people have based their work on this and it
clearly now works with the test coverage we've had, on the other hand
I'm now less confident with the series overall, and Ville and Maarten
keep on debating about it.

I've ended up chickening out of this and compromising. I've moved
("demoted") Maarten's atomic series part 2 from drm-intel-next-queued to
a new topical branch topic/atomic-conversion, and added these four
patches on top. The topical branch merges to
drm-intel-nightly. Basically nightly now has these patches on top, but
the source of the commits is different.

Frankly what I'm doing is deferring this to Daniel who'll return next
week. He'll take over handling drm-intel-next-queue on his return
anyway, so given he'll have to deal with the rest of the fallout I think
it's only fair he can decide what to do. I would have dropped the whole
series and started over if I were in charge next week, but this will
probably cause less of a hickup right now.


BR,
Jani.


[1] http://mid.gmane.org/1433155811-9671-1-git-send-email-maarten.lankhorst@linux.intel.com
[2] https://bugs.freedesktop.org/show_bug.cgi?id=90868
[3] https://bugs.freedesktop.org/show_bug.cgi?id=90861
[4] https://bugs.freedesktop.org/show_bug.cgi?id=90874
[5] http://mid.gmane.org/1433924660-2228-1-git-send-email-maarten.lankhorst@linux.intel.com
[6] https://bugs.freedesktop.org/show_bug.cgi?id=90929


>
> Maarten Lankhorst (4):
>   drm/i915: Do not use atomic modesets in hw readout.
>   drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip
>   drm/i915: Set hwmode during readout.
>   drm/i915: Only enable cursor if it can be enabled.
>
>  drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++++++---------------
>  1 file changed, 63 insertions(+), 45 deletions(-)
>
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] Fix more fallout from reverting atomic hw readout.

[Maarten Lankhorst]

Hey,

Op 12-06-15 om 14:45 schreef Jani Nikula:
> On Fri, 12 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
>> Commit f662af8c5c1619 reverts
>> "drm/i915: Read hw state into an atomic state struct, v2."
>> but it doesn't take into account other changes that were done in that time.
>> Before I continue on the atomic fixes I want to fix the fallout first,
>> and some of the reasons I identified that could cause a failure for atomic
>> modeset.
>>
>> When that's fixed I'll look at committing the atomic hw readout patch
>> again, since it will be needed for converting to full atomic.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
> To recap, the atomic conversion part 2 series [1] broke things
> [2][3][4], and the quick fix reverts [5] broke other things [6], and now
> we have these patches to fix that.
>
> I was resolved to either merge these fixes or purge the whole thing this
> afternoon. On the one hand people have based their work on this and it
> clearly now works with the test coverage we've had, on the other hand
> I'm now less confident with the series overall, and Ville and Maarten
> keep on debating about it.
>
> I've ended up chickening out of this and compromising. I've moved
> ("demoted") Maarten's atomic series part 2 from drm-intel-next-queued to
> a new topical branch topic/atomic-conversion, and added these four
> patches on top. The topical branch merges to
> drm-intel-nightly. Basically nightly now has these patches on top, but
> the source of the commits is different.
We were discussing the intel_plane_restore patch, but that was to fix a hard hang on my broadwell machine, that occurs when it terminally wedges.
I picked the patch from my tree because I already had it and the warning made it looks like it would fix it. This was already broken before part 2 was applied.

It can safely be dropped without affecting anyone affected by the hangs, the other 3 patches should still be applied to the topic branch.

> Frankly what I'm doing is deferring this to Daniel who'll return next
> week. He'll take over handling drm-intel-next-queue on his return
> anyway, so given he'll have to deal with the rest of the fallout I think
> it's only fair he can decide what to do. I would have dropped the whole
> series and started over if I were in charge next week, but this will
> probably cause less of a hickup right now.
Sane decision, especially if he's back next monday. :-)
>
> BR,
> Jani.
>
>
> [1] http://mid.gmane.org/1433155811-9671-1-git-send-email-maarten.lankhorst@linux.intel.com
> [2] https://bugs.freedesktop.org/show_bug.cgi?id=90868
> [3] https://bugs.freedesktop.org/show_bug.cgi?id=90861
> [4] https://bugs.freedesktop.org/show_bug.cgi?id=90874
> [5] http://mid.gmane.org/1433924660-2228-1-git-send-email-maarten.lankhorst@linux.intel.com
> [6] https://bugs.freedesktop.org/show_bug.cgi?id=90929
>
>
>> Maarten Lankhorst (4):
>>   drm/i915: Do not use atomic modesets in hw readout.
>>   drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip
>>   drm/i915: Set hwmode during readout.
>>   drm/i915: Only enable cursor if it can be enabled.
>>
>>  drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++++++---------------
>>  1 file changed, 63 insertions(+), 45 deletions(-)
>>
>> -- 
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 0/4] Fix more fallout from reverting atomic hw readout.

[Daniel Vetter]

On Fri, Jun 12, 2015 at 03:10:53PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 12-06-15 om 14:45 schreef Jani Nikula:
> > On Fri, 12 Jun 2015, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> >> Commit f662af8c5c1619 reverts
> >> "drm/i915: Read hw state into an atomic state struct, v2."
> >> but it doesn't take into account other changes that were done in that time.
> >> Before I continue on the atomic fixes I want to fix the fallout first,
> >> and some of the reasons I identified that could cause a failure for atomic
> >> modeset.
> >>
> >> When that's fixed I'll look at committing the atomic hw readout patch
> >> again, since it will be needed for converting to full atomic.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90929
> > To recap, the atomic conversion part 2 series [1] broke things
> > [2][3][4], and the quick fix reverts [5] broke other things [6], and now
> > we have these patches to fix that.
> >
> > I was resolved to either merge these fixes or purge the whole thing this
> > afternoon. On the one hand people have based their work on this and it
> > clearly now works with the test coverage we've had, on the other hand
> > I'm now less confident with the series overall, and Ville and Maarten
> > keep on debating about it.
> >
> > I've ended up chickening out of this and compromising. I've moved
> > ("demoted") Maarten's atomic series part 2 from drm-intel-next-queued to
> > a new topical branch topic/atomic-conversion, and added these four
> > patches on top. The topical branch merges to
> > drm-intel-nightly. Basically nightly now has these patches on top, but
> > the source of the commits is different.
> We were discussing the intel_plane_restore patch, but that was to fix a
> hard hang on my broadwell machine, that occurs when it terminally
> wedges.  I picked the patch from my tree because I already had it and
> the warning
> made it looks like it would fix it. This was already broken before part
> 2 was applied.
> 
> It can safely be dropped without affecting anyone affected by the hangs,
> the other 3 patches should still be applied to the topic branch.
> 
> > Frankly what I'm doing is deferring this to Daniel who'll return next
> > week. He'll take over handling drm-intel-next-queue on his return
> > anyway, so given he'll have to deal with the rest of the fallout I think
> > it's only fair he can decide what to do. I would have dropped the whole
> > series and started over if I were in charge next week, but this will
> > probably cause less of a hickup right now.
> Sane decision, especially if he's back next monday. :-)

Just for the recorded I decided that nothing can hamper my vacation mood
and pulled branch in with a merge. More seriously I don't expect that this
will be easier as a branch, and we've far enough away from 4.3 that I'm
confident we can fix up anything horrible that rears its ugly head in
time.

I'll definitely get less adventurous as get move towards 4.2-rc5 ;-)
-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