All of lore.kernel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.