public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled
@ 2015-05-07 21:19 Matt Roper
  2015-05-07 21:28 ` Matt Roper
  2015-05-11  6:11 ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled shuang.he
  0 siblings, 2 replies; 9+ messages in thread
From: Matt Roper @ 2015-05-07 21:19 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

With the recent modeset internal rework, we wind up setting crtc_state->enable
to false, but leave crtc_state->active as true following a
drmModeSetCrtc(fb=0), which is incorrect.  This mismatch gets caught by
drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane
updates while the CRTC is disabled) to fail.

Bisect points to

        commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
        Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
        Date:   Tue Apr 21 17:13:19 2015 +0300

            drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: igt/kms_universal_plane
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
My original expectation was that we'd need to be updating the in-flight CRTC
state rather than the committed state as I wound up doing here.  However it
looks like our legacy modesets aren't yet converted to the point where they do
state swaps, hence the need to update crtc->state directly.

Ander/Maarten, does this look like the right place to have 'active' updated for
the time being, or should it be moved elsewhere in the modeset pipeline?

 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c297cdc..be166cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6128,6 +6128,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 
 	drm_plane_helper_disable(crtc->primary);
 
+	crtc->state->active = false;
+
 	/* Update computed state. */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (!connector->encoder || !connector->encoder->crtc)
-- 
1.8.5.1

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

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

* Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled
  2015-05-07 21:19 [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled Matt Roper
@ 2015-05-07 21:28 ` Matt Roper
  2015-05-07 21:31   ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) Matt Roper
  2015-05-11  6:11 ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled shuang.he
  1 sibling, 1 reply; 9+ messages in thread
From: Matt Roper @ 2015-05-07 21:28 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

On Thu, May 07, 2015 at 02:19:07PM -0700, Matt Roper wrote:
> With the recent modeset internal rework, we wind up setting crtc_state->enable
> to false, but leave crtc_state->active as true following a
> drmModeSetCrtc(fb=0), which is incorrect.  This mismatch gets caught by
> drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane
> updates while the CRTC is disabled) to fail.
> 
> Bisect points to
> 
>         commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
>         Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>         Date:   Tue Apr 21 17:13:19 2015 +0300
> 
>             drm/i915: Use atomic helpers for computing changed flags
> 
> as the commit that actually triggers the regression.
> 
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: igt/kms_universal_plane
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> My original expectation was that we'd need to be updating the in-flight CRTC
> state rather than the committed state as I wound up doing here.  However it
> looks like our legacy modesets aren't yet converted to the point where they do
> state swaps, hence the need to update crtc->state directly.

Actually, it looks like I screwed up my working tree and lost the end of
Ander's patch series.  We actually *do* have proper CRTC state swapping
now (when his whole series is present), so updating in-flight state is
the right way to go; I'll send a v2 shortly that fixes that.


Matt

> 
> Ander/Maarten, does this look like the right place to have 'active' updated for
> the time being, or should it be moved elsewhere in the modeset pipeline?
> 
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c297cdc..be166cf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6128,6 +6128,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  
>  	drm_plane_helper_disable(crtc->primary);
>  
> +	crtc->state->active = false;
> +
>  	/* Update computed state. */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		if (!connector->encoder || !connector->encoder->crtc)
> -- 
> 1.8.5.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)
  2015-05-07 21:28 ` Matt Roper
@ 2015-05-07 21:31   ` Matt Roper
  2015-05-08  7:56     ` Ander Conselvan De Oliveira
  2015-05-09  5:08     ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) shuang.he
  0 siblings, 2 replies; 9+ messages in thread
From: Matt Roper @ 2015-05-07 21:31 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ander Conselvan de Oliveira

With the recent modeset internal rework, we wind up setting
crtc_state->enable to false, but leave crtc_state->active as true, which
is incorrect.  This mismatch gets caught by drm_atomic_crtc_check() and
causes subsequent atomic operations (such as plane updates while the
CRTC is disabled) to fail.

Bisect points to

        commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
        Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
        Date:   Tue Apr 21 17:13:19 2015 +0300

            drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

v2: Update to alter in-flight state rather than already-committed state
    (first version was accidentally based on a midpoint of Ander's
    modeset rework series, before his final patches that add proper
    state swapping to the legacy modeset path).

Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: igt/kms_universal_plane
Signed-off-by: Matt Roper <matthew.d.roper@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 c297cdc..981478a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			continue;
 
 		if (!crtc_state->enable) {
+			crtc_state->active = false;
 			intel_crtc_disable(crtc);
 		} else if (crtc->state->enable) {
 			intel_crtc_disable_planes(crtc);
-- 
1.8.5.1

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

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

* Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)
  2015-05-07 21:31   ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) Matt Roper
@ 2015-05-08  7:56     ` Ander Conselvan De Oliveira
  2015-05-08  8:17       ` Daniel Vetter
  2015-05-11  8:45       ` [PATCH] drm/i915: Keep crtc_state->active in sync with enable Maarten Lankhorst
  2015-05-09  5:08     ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) shuang.he
  1 sibling, 2 replies; 9+ messages in thread
From: Ander Conselvan De Oliveira @ 2015-05-08  7:56 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Thu, 2015-05-07 at 14:31 -0700, Matt Roper wrote:
> With the recent modeset internal rework, we wind up setting
> crtc_state->enable to false, but leave crtc_state->active as true, which
> is incorrect.  This mismatch gets caught by drm_atomic_crtc_check() and
> causes subsequent atomic operations (such as plane updates while the
> CRTC is disabled) to fail.
> 
> Bisect points to
> 
>         commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
>         Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>         Date:   Tue Apr 21 17:13:19 2015 +0300
> 
>             drm/i915: Use atomic helpers for computing changed flags
> 
> as the commit that actually triggers the regression.
> 
> v2: Update to alter in-flight state rather than already-committed state
>     (first version was accidentally based on a midpoint of Ander's
>     modeset rework series, before his final patches that add proper
>     state swapping to the legacy modeset path).
> 
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: igt/kms_universal_plane
> Signed-off-by: Matt Roper <matthew.d.roper@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 c297cdc..981478a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			continue;
>  
>  		if (!crtc_state->enable) {
> +			crtc_state->active = false;

I believe a more appropriate fix would be to set crtc_state->active when
we set crtc_state->enable to false in intel_modeset_stage_output_state()
and the HW state read out. But I think Maarten does that in his later
patch series, so as a stopgap solution this is fine.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


>  			intel_crtc_disable(crtc);
>  		} else if (crtc->state->enable) {
>  			intel_crtc_disable_planes(crtc);


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

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

* Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)
  2015-05-08  7:56     ` Ander Conselvan De Oliveira
@ 2015-05-08  8:17       ` Daniel Vetter
  2015-05-11  8:45       ` [PATCH] drm/i915: Keep crtc_state->active in sync with enable Maarten Lankhorst
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-05-08  8:17 UTC (permalink / raw)
  To: Ander Conselvan De Oliveira; +Cc: intel-gfx

On Fri, May 08, 2015 at 10:56:03AM +0300, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-05-07 at 14:31 -0700, Matt Roper wrote:
> > With the recent modeset internal rework, we wind up setting
> > crtc_state->enable to false, but leave crtc_state->active as true, which
> > is incorrect.  This mismatch gets caught by drm_atomic_crtc_check() and
> > causes subsequent atomic operations (such as plane updates while the
> > CRTC is disabled) to fail.
> > 
> > Bisect points to
> > 
> >         commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
> >         Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >         Date:   Tue Apr 21 17:13:19 2015 +0300
> > 
> >             drm/i915: Use atomic helpers for computing changed flags
> > 
> > as the commit that actually triggers the regression.
> > 
> > v2: Update to alter in-flight state rather than already-committed state
> >     (first version was accidentally based on a midpoint of Ander's
> >     modeset rework series, before his final patches that add proper
> >     state swapping to the legacy modeset path).
> > 
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Testcase: igt/kms_universal_plane
> > Signed-off-by: Matt Roper <matthew.d.roper@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 c297cdc..981478a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> >  			continue;
> >  
> >  		if (!crtc_state->enable) {
> > +			crtc_state->active = false;
> 
> I believe a more appropriate fix would be to set crtc_state->active when
> we set crtc_state->enable to false in intel_modeset_stage_output_state()
> and the HW state read out. But I think Maarten does that in his later
> patch series, so as a stopgap solution this is fine.
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)
  2015-05-07 21:31   ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) Matt Roper
  2015-05-08  7:56     ` Ander Conselvan De Oliveira
@ 2015-05-09  5:08     ` shuang.he
  1 sibling, 0 replies; 9+ messages in thread
From: shuang.he @ 2015-05-09  5:08 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, matthew.d.roper

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6352
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                 -1              314/314              313/314
IVB                                  338/338              338/338
BYT                                  286/286              286/286
BDW                                  320/320              320/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(2)PASS(1)      DMESG_WARN(2)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled
  2015-05-07 21:19 [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled Matt Roper
  2015-05-07 21:28 ` Matt Roper
@ 2015-05-11  6:11 ` shuang.he
  1 sibling, 0 replies; 9+ messages in thread
From: shuang.he @ 2015-05-11  6:11 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, matthew.d.roper

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6351
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                                  286/286              286/286
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH] drm/i915: Keep crtc_state->active in sync with enable.
  2015-05-08  7:56     ` Ander Conselvan De Oliveira
  2015-05-08  8:17       ` Daniel Vetter
@ 2015-05-11  8:45       ` Maarten Lankhorst
  2015-05-11  9:58         ` Daniel Vetter
  1 sibling, 1 reply; 9+ messages in thread
From: Maarten Lankhorst @ 2015-05-11  8:45 UTC (permalink / raw)
  To: Ander Conselvan De Oliveira, Matt Roper; +Cc: intel-gfx

With the recent modeset internal rework, we wind up setting crtc_state->enable
to false, but leave crtc_state->active as true following a
drmModeSetCrtc(fb=0), which is incorrect.  This mismatch gets caught by
drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane
updates while the CRTC is disabled) to fail.

Bisect points to

        commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
        Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
        Date:   Tue Apr 21 17:13:19 2015 +0300

            drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Reported-and-Tested-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
The original patch for this is wrong, we should keep enable and active in sync when changed.
Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)".

 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 22e6644f755e..e223209e2d81 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9868,7 +9868,7 @@ retry:
 		goto fail;
 	}
 
-	crtc_state->base.enable = true;
+	crtc_state->base.active = crtc_state->base.enable = true;
 
 	if (!mode)
 		mode = &load_detect_mode;
@@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		connector_state->best_encoder = NULL;
 		connector_state->crtc = NULL;
 
-		crtc_state->base.enable = false;
+		crtc_state->base.enable = crtc_state->base.active = false;
 
 		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
 						      0, 0);
@@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			continue;
 		}
 
-		crtc_state->base.enable = intel_crtc->new_enabled;
+		crtc_state->base.active = crtc_state->base.enable =
+			intel_crtc->new_enabled;
 
 		if (&intel_crtc->base == crtc)
 			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
@@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		bool has_connectors;
+
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret)
 			return ret;
 
-		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
+		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
+		if (has_connectors != crtc_state->enable)
+			crtc_state->enable =
+			crtc_state->active = has_connectors;
 	}
 
 	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
@@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 		WARN_ON(crtc->active);
 		crtc->base.state->enable = false;
+		crtc->base.state->active = false;
 		crtc->base.enabled = false;
 	}
 
@@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->active ? "enabled" : "disabled");
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		/* Because we only establish the connector -> encoder ->
@@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 								 crtc->config);
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		plane_state = to_intel_plane_state(primary->state);
-- 
2.1.0

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

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

* Re: [PATCH] drm/i915: Keep crtc_state->active in sync with enable.
  2015-05-11  8:45       ` [PATCH] drm/i915: Keep crtc_state->active in sync with enable Maarten Lankhorst
@ 2015-05-11  9:58         ` Daniel Vetter
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-05-11  9:58 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Mon, May 11, 2015 at 10:45:15AM +0200, Maarten Lankhorst wrote:
> With the recent modeset internal rework, we wind up setting crtc_state->enable
> to false, but leave crtc_state->active as true following a
> drmModeSetCrtc(fb=0), which is incorrect.  This mismatch gets caught by
> drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane
> updates while the CRTC is disabled) to fail.
> 
> Bisect points to
> 
>         commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
>         Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>         Date:   Tue Apr 21 17:13:19 2015 +0300
> 
>             drm/i915: Use atomic helpers for computing changed flags
> 
> as the commit that actually triggers the regression.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Reported-and-Tested-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Yeah this makes more sense, in the legacy codepaths we always implicitly
smash the dpms state (i.e. state->active) to the logical pipe state (i.e.
state->enable). Queued for -next, thanks for the patch.
> ---
> The original patch for this is wrong, we should keep enable and active in sync when changed.
> Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)".

I've squashed the revert right into this one, imo didn't make sense
separetely.
-Daniel

> 
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 22e6644f755e..e223209e2d81 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9868,7 +9868,7 @@ retry:
>  		goto fail;
>  	}
>  
> -	crtc_state->base.enable = true;
> +	crtc_state->base.active = crtc_state->base.enable = true;
>  
>  	if (!mode)
>  		mode = &load_detect_mode;
> @@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		connector_state->best_encoder = NULL;
>  		connector_state->crtc = NULL;
>  
> -		crtc_state->base.enable = false;
> +		crtc_state->base.enable = crtc_state->base.active = false;
>  
>  		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
>  						      0, 0);
> @@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  			continue;
>  		}
>  
> -		crtc_state->base.enable = intel_crtc->new_enabled;
> +		crtc_state->base.active = crtc_state->base.enable =
> +			intel_crtc->new_enabled;
>  
>  		if (&intel_crtc->base == crtc)
>  			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> @@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		bool has_connectors;
> +
>  		ret = drm_atomic_add_affected_connectors(state, crtc);
>  		if (ret)
>  			return ret;
>  
> -		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
> +		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
> +		if (has_connectors != crtc_state->enable)
> +			crtc_state->enable =
> +			crtc_state->active = has_connectors;
>  	}
>  
>  	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
> @@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  		WARN_ON(crtc->active);
>  		crtc->base.state->enable = false;
> +		crtc->base.state->active = false;
>  		crtc->base.enabled = false;
>  	}
>  
> @@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			      crtc->active ? "enabled" : "disabled");
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		/* Because we only establish the connector -> encoder ->
> @@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  								 crtc->config);
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		plane_state = to_intel_plane_state(primary->state);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2015-05-11  9:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-07 21:19 [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled Matt Roper
2015-05-07 21:28 ` Matt Roper
2015-05-07 21:31   ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) Matt Roper
2015-05-08  7:56     ` Ander Conselvan De Oliveira
2015-05-08  8:17       ` Daniel Vetter
2015-05-11  8:45       ` [PATCH] drm/i915: Keep crtc_state->active in sync with enable Maarten Lankhorst
2015-05-11  9:58         ` Daniel Vetter
2015-05-09  5:08     ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2) shuang.he
2015-05-11  6:11 ` [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled shuang.he

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox