[PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Maarten Lankhorst]

We shouldn't inspect crtc->state, instead grab the crtc state.
At this point the hw state verifier should be able to run even if
crtc->state has been updated (which cannot currently happen).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 295e17d0f272..e5205d338e3c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5893,9 +5893,10 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 
 /* Cross check the actual hw state with our own modeset state tracking (and it's
  * internal consistency). */
-static void intel_connector_verify_state(struct intel_connector *connector)
+static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state)
 {
-	struct drm_crtc *crtc = connector->base.state->crtc;
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.base.id,
@@ -5903,15 +5904,14 @@ static void intel_connector_verify_state(struct intel_connector *connector)
 
 	if (connector->get_hw_state(connector)) {
 		struct intel_encoder *encoder = connector->encoder;
-		struct drm_connector_state *conn_state = connector->base.state;
 
-		I915_STATE_WARN(!crtc,
+		I915_STATE_WARN(!crtc_state,
 			 "connector enabled without attached crtc\n");
 
-		if (!crtc)
+		if (!crtc_state)
 			return;
 
-		I915_STATE_WARN(!crtc->state->active,
+		I915_STATE_WARN(!crtc_state->active,
 		      "connector is active, but attached crtc isn't\n");
 
 		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
@@ -5923,9 +5923,9 @@ static void intel_connector_verify_state(struct intel_connector *connector)
 		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
 			"attached encoder crtc differs from connector crtc\n");
 	} else {
-		I915_STATE_WARN(crtc && crtc->state->active,
+		I915_STATE_WARN(crtc_state && crtc_state->active,
 			"attached crtc is active, but connector isn't\n");
-		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
 			"best encoder set without crtc!\n");
 	}
 }
@@ -11859,11 +11859,15 @@ verify_connector_state(struct drm_device *dev,
 
 	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		struct drm_encoder *encoder = connector->encoder;
+		struct drm_crtc_state *crtc_state = NULL;
 
 		if (new_conn_state->crtc != crtc)
 			continue;
 
-		intel_connector_verify_state(to_intel_connector(connector));
+		if (crtc)
+			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
+
+		intel_connector_verify_state(crtc_state, new_conn_state);
 
 		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
 		     "connector's atomic encoder doesn't match legacy encoder\n");
@@ -11981,7 +11985,7 @@ verify_crtc_state(struct drm_crtc *crtc,
 
 	intel_pipe_config_sanity_check(dev_priv, pipe_config);
 
-	sw_config = to_intel_crtc_state(crtc->state);
+	sw_config = to_intel_crtc_state(new_crtc_state);
 	if (!intel_pipe_config_compare(dev_priv, sw_config,
 				       pipe_config, false)) {
 		I915_STATE_WARN(1, "pipe state doesn't match!\n");
-- 
2.11.0

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

[PATCH 2/2] drm/i915: Remove vma unpin in intel_plane_destroy

[Maarten Lankhorst]

commit a667fb402c1e856209bf9e77ba41fc1cf356b867
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Thu Dec 15 15:29:44 2016 +0100

    drm/i915: Disable all crtcs during driver unload, v2.

made sure that all crtc's are disabled on driver unload, but only the
following commit made sure all fb's are cleaned up correctly:

commit 9b2104f423de5c148749a07e8197dbab4c449877
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue Feb 21 14:51:40 2017 +0100

    drm/atomic: Make disable_all helper fully disable the crtc.

Finally remove this and add a WARN_ON when vma is set. It should
have been removed by intel_cleanup_plane_fb().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index cfb47293fd53..cd74a7dca3ce 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -102,23 +102,7 @@ void
 intel_plane_destroy_state(struct drm_plane *plane,
 			  struct drm_plane_state *state)
 {
-	struct i915_vma *vma;
-
-	vma = fetch_and_zero(&to_intel_plane_state(state)->vma);
-
-	/*
-	 * FIXME: Normally intel_cleanup_plane_fb handles destruction of vma.
-	 * We currently don't clear all planes during driver unload, so we have
-	 * to be able to unpin vma here for now.
-	 *
-	 * Normally this can only happen during unload when kmscon is disabled
-	 * and userspace doesn't attempt to set a framebuffer at all.
-	 */
-	if (vma) {
-		mutex_lock(&plane->dev->struct_mutex);
-		intel_unpin_fb_vma(vma);
-		mutex_unlock(&plane->dev->struct_mutex);
-	}
+	WARN_ON(to_intel_plane_state(state)->vma);
 
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
-- 
2.11.0

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

Re: [PATCH 2/2] drm/i915: Remove vma unpin in intel_plane_destroy

[Daniel Vetter]

On Thu, May 11, 2017 at 10:28:44AM +0200, Maarten Lankhorst wrote:
> commit a667fb402c1e856209bf9e77ba41fc1cf356b867
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Thu Dec 15 15:29:44 2016 +0100
> 
>     drm/i915: Disable all crtcs during driver unload, v2.
> 
> made sure that all crtc's are disabled on driver unload, but only the
> following commit made sure all fb's are cleaned up correctly:
> 
> commit 9b2104f423de5c148749a07e8197dbab4c449877
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Tue Feb 21 14:51:40 2017 +0100
> 
>     drm/atomic: Make disable_all helper fully disable the crtc.
> 
> Finally remove this and add a WARN_ON when vma is set. It should
> have been removed by intel_cleanup_plane_fb().
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index cfb47293fd53..cd74a7dca3ce 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -102,23 +102,7 @@ void
>  intel_plane_destroy_state(struct drm_plane *plane,
>  			  struct drm_plane_state *state)
>  {
> -	struct i915_vma *vma;
> -
> -	vma = fetch_and_zero(&to_intel_plane_state(state)->vma);
> -
> -	/*
> -	 * FIXME: Normally intel_cleanup_plane_fb handles destruction of vma.
> -	 * We currently don't clear all planes during driver unload, so we have
> -	 * to be able to unpin vma here for now.
> -	 *
> -	 * Normally this can only happen during unload when kmscon is disabled
> -	 * and userspace doesn't attempt to set a framebuffer at all.
> -	 */
> -	if (vma) {
> -		mutex_lock(&plane->dev->struct_mutex);
> -		intel_unpin_fb_vma(vma);
> -		mutex_unlock(&plane->dev->struct_mutex);
> -	}
> +	WARN_ON(to_intel_plane_state(state)->vma);
>  
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Fix hw state verifier access to crtc->state.
URL   : https://patchwork.freedesktop.org/series/24281/
State : success

== Summary ==

Series 24281v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24281/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:435s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:437s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:593s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:518s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:553s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:493s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:423s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:490s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:498s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:461s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:577s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:464s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:582s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:503s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:410s

4149c7b22164d81960dae0987f497725117b68e6 drm-tip: 2017y-05m-10d-18h-09m-15s UTC integration manifest
10cf0f5 drm/i915: Remove vma unpin in intel_plane_destroy
afc741a drm/i915: Fix hw state verifier access to crtc->state.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4670/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Daniel Vetter]

On Thu, May 11, 2017 at 10:28:43AM +0200, Maarten Lankhorst wrote:
> We shouldn't inspect crtc->state, instead grab the crtc state.
> At this point the hw state verifier should be able to run even if
> crtc->state has been updated (which cannot currently happen).
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

pll state checking still looks at ->state directly, we might want to port
that to the new private obj helpers perhaps, with the same new/old
iterators?

Just throwing around some ideas.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 295e17d0f272..e5205d338e3c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5893,9 +5893,10 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
>  
>  /* Cross check the actual hw state with our own modeset state tracking (and it's
>   * internal consistency). */
> -static void intel_connector_verify_state(struct intel_connector *connector)
> +static void intel_connector_verify_state(struct drm_crtc_state *crtc_state,
> +					 struct drm_connector_state *conn_state)
>  {
> -	struct drm_crtc *crtc = connector->base.state->crtc;
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.base.id,
> @@ -5903,15 +5904,14 @@ static void intel_connector_verify_state(struct intel_connector *connector)
>  
>  	if (connector->get_hw_state(connector)) {
>  		struct intel_encoder *encoder = connector->encoder;
> -		struct drm_connector_state *conn_state = connector->base.state;
>  
> -		I915_STATE_WARN(!crtc,
> +		I915_STATE_WARN(!crtc_state,
>  			 "connector enabled without attached crtc\n");
>  
> -		if (!crtc)
> +		if (!crtc_state)
>  			return;
>  
> -		I915_STATE_WARN(!crtc->state->active,
> +		I915_STATE_WARN(!crtc_state->active,
>  		      "connector is active, but attached crtc isn't\n");
>  
>  		if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> @@ -5923,9 +5923,9 @@ static void intel_connector_verify_state(struct intel_connector *connector)
>  		I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
>  			"attached encoder crtc differs from connector crtc\n");
>  	} else {
> -		I915_STATE_WARN(crtc && crtc->state->active,
> +		I915_STATE_WARN(crtc_state && crtc_state->active,
>  			"attached crtc is active, but connector isn't\n");
> -		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
> +		I915_STATE_WARN(!crtc_state && conn_state->best_encoder,
>  			"best encoder set without crtc!\n");
>  	}
>  }
> @@ -11859,11 +11859,15 @@ verify_connector_state(struct drm_device *dev,
>  
>  	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>  		struct drm_encoder *encoder = connector->encoder;
> +		struct drm_crtc_state *crtc_state = NULL;
>  
>  		if (new_conn_state->crtc != crtc)
>  			continue;
>  
> -		intel_connector_verify_state(to_intel_connector(connector));
> +		if (crtc)
> +			crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> +
> +		intel_connector_verify_state(crtc_state, new_conn_state);
>  
>  		I915_STATE_WARN(new_conn_state->best_encoder != encoder,
>  		     "connector's atomic encoder doesn't match legacy encoder\n");
> @@ -11981,7 +11985,7 @@ verify_crtc_state(struct drm_crtc *crtc,
>  
>  	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>  
> -	sw_config = to_intel_crtc_state(crtc->state);
> +	sw_config = to_intel_crtc_state(new_crtc_state);
>  	if (!intel_pipe_config_compare(dev_priv, sw_config,
>  				       pipe_config, false)) {
>  		I915_STATE_WARN(1, "pipe state doesn't match!\n");
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Maarten Lankhorst]

Op 11-05-17 om 11:23 schreef Daniel Vetter:
> On Thu, May 11, 2017 at 10:28:43AM +0200, Maarten Lankhorst wrote:
>> We shouldn't inspect crtc->state, instead grab the crtc state.
>> At this point the hw state verifier should be able to run even if
>> crtc->state has been updated (which cannot currently happen).
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> pll state checking still looks at ->state directly, we might want to port
> that to the new private obj helpers perhaps, with the same new/old
> iterators?
That might be an excellent idea to do in the future. :)

If I look at it though it's race safe in the current design,
but not necessarily against multiple nonblocking modesets,
which should probably be addressed at some point.

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

Re: [PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Daniel Vetter]

On Thu, May 11, 2017 at 11:41:22AM +0200, Maarten Lankhorst wrote:
> Op 11-05-17 om 11:23 schreef Daniel Vetter:
> > On Thu, May 11, 2017 at 10:28:43AM +0200, Maarten Lankhorst wrote:
> >> We shouldn't inspect crtc->state, instead grab the crtc state.
> >> At this point the hw state verifier should be able to run even if
> >> crtc->state has been updated (which cannot currently happen).
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > pll state checking still looks at ->state directly, we might want to port
> > that to the new private obj helpers perhaps, with the same new/old
> > iterators?
> That might be an excellent idea to do in the future. :)
> 
> If I look at it though it's race safe in the current design,
> but not necessarily against multiple nonblocking modesets,
> which should probably be addressed at some point.

I looked at this more from the pov of unifying state handling across all
blocks. If everything works roughly the same, it's much easier to
understand. And I do kinda like DK's private state stuff, that should help
in aligning the various internal bits we have (like shared dpll, wms, and
all that).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fix hw state verifier access to crtc->state.

[Maarten Lankhorst]

Op 15-05-17 om 15:52 schreef Daniel Vetter:
> On Thu, May 11, 2017 at 11:41:22AM +0200, Maarten Lankhorst wrote:
>> Op 11-05-17 om 11:23 schreef Daniel Vetter:
>>> On Thu, May 11, 2017 at 10:28:43AM +0200, Maarten Lankhorst wrote:
>>>> We shouldn't inspect crtc->state, instead grab the crtc state.
>>>> At this point the hw state verifier should be able to run even if
>>>> crtc->state has been updated (which cannot currently happen).
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> pll state checking still looks at ->state directly, we might want to port
>>> that to the new private obj helpers perhaps, with the same new/old
>>> iterators?
>> That might be an excellent idea to do in the future. :)
>>
>> If I look at it though it's race safe in the current design,
>> but not necessarily against multiple nonblocking modesets,
>> which should probably be addressed at some point.
> I looked at this more from the pov of unifying state handling across all
> blocks. If everything works roughly the same, it's much easier to
> understand. And I do kinda like DK's private state stuff, that should help
> in aligning the various internal bits we have (like shared dpll, wms, and
> all that).
> -Daniel

Agreed, it should be something for unification.

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