All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] SKL watermark fixes for !fbcon
@ 2016-06-09 22:14 Matt Roper
  2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-09 22:14 UTC (permalink / raw)
  To: intel-gfx

There are a handful of watermark bugs that are only triggered (or more easily
triggered) when running without fbcon.  Thanks Tvrtko for pointing these out.

I do still see some WARN()'s from other parts of the display code when
launching X in a non-fbcon setup, so there may be other bugfixes needed in
other parts of the code as well.

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Matt Roper (3):
  drm/i915/gen9: Initialize intel_state->active_crtcs during WM
    sanitization
  drm/i915/gen9: Compute data rates for all planes on first commit
  drm/i915/gen9: Drop invalid WARN() during data rate calculation

 drivers/gpu/drm/i915/intel_pm.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

-- 
2.1.4

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

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

* [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
@ 2016-06-09 22:14 ` Matt Roper
  2016-06-13  8:59   ` Maarten Lankhorst
  2016-06-13 14:49   ` Daniel Vetter
  2016-06-09 22:14 ` [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit Matt Roper
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-09 22:14 UTC (permalink / raw)
  To: intel-gfx

intel_state->active_crtcs is usually only initialized when doing a
modeset.  During our first atomic commit after boot, we're effectively
faking a modeset to sanitize the DDB/wm setup, so ensure that this field
gets initialized before use.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 658a756..0cd38ca 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	 * pretend that all pipes switched active status so that we'll
 	 * ensure a full DDB recompute.
 	 */
-	if (dev_priv->wm.distrust_bios_wm)
+	if (dev_priv->wm.distrust_bios_wm) {
 		intel_state->active_pipe_changes = ~0;
 
+		/*
+		 * We usually only initialize intel_state->active_crtcs if we
+		 * we're doing a modeset; make sure this field is always
+		 * initialized during the sanitization process that happens
+		 * on the first commit too.
+		 */
+		intel_state->active_crtcs = dev_priv->active_crtcs;
+	}
+
 	/*
 	 * If the modeset changes which CRTC's are active, we need to
 	 * recompute the DDB allocation for *all* active pipes, even
-- 
2.1.4

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

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

* [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
  2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
@ 2016-06-09 22:14 ` Matt Roper
  2016-06-13  9:04   ` Maarten Lankhorst
  2016-06-13 14:50   ` Daniel Vetter
  2016-06-09 22:14 ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-09 22:14 UTC (permalink / raw)
  To: intel-gfx

When we sanitize our DDB and watermark info during the first atomic
commit, we need to calculate the total data rate.  Since we haven't
explicitly added the planes for each CRTC to our atomic state, the total
data rate calculation will try to use the cached values from a previous
commit (which are 0 since there was no previous commit); this result is
incorrect if we inherited any active planes from the BIOS.

During our very first atomic commit, we need to explicitly add all
active planes to the atomic state to ensure that valid data rate values
are calculated for them.  Subsequent commits will then have valid cached
values to fall back on.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0cd38ca..ba08639 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
 
+		/*
+		 * If this is our first commit after hw readout, we don't have
+		 * valid data rate values cached.  Add all planes to ensure we
+		 * calculate a valid data rate.
+		 */
+		if (dev_priv->wm.distrust_bios_wm) {
+			ret = drm_atomic_add_affected_planes(state,
+							     &intel_crtc->base);
+			if (ret)
+				return ret;
+		}
+
 		ret = skl_allocate_pipe_ddb(cstate, ddb);
 		if (ret)
 			return ret;
-- 
2.1.4

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

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

* [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
  2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
  2016-06-09 22:14 ` [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit Matt Roper
@ 2016-06-09 22:14 ` Matt Roper
  2016-06-13  9:33   ` Maarten Lankhorst
  2016-06-10  6:24 ` ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Matt Roper @ 2016-06-09 22:14 UTC (permalink / raw)
  To: intel-gfx

It's possible to have a non-zero plane mask and still wind up with a
total data rate of zero.  There are two cases where this can happen:

 * planes are active (from the KMS point of view), but are
   all fully clipped (positioned offscreen)
 * the only active plane on a CRTC is the cursor (which is handled
   independently and not counted into the general data rate computations

These are both valid display setups (although unusual), so we need to
drop the WARN().

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ba08639..2bd089e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3107,8 +3107,6 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 		total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id];
 	}
 
-	WARN_ON(cstate->plane_mask && total_data_rate == 0);
-
 	return total_data_rate;
 }
 
-- 
2.1.4

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

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

* ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
                   ` (2 preceding siblings ...)
  2016-06-09 22:14 ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
@ 2016-06-10  6:24 ` Patchwork
  2016-06-10 14:20   ` Matt Roper
  2016-06-10  7:28 ` [PATCH 0/3] " Jani Nikula
  2016-06-10  8:48 ` Tvrtko Ursulin
  5 siblings, 1 reply; 21+ messages in thread
From: Patchwork @ 2016-06-10  6:24 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

== Series Details ==

Series: SKL watermark fixes for !fbcon
URL   : https://patchwork.freedesktop.org/series/8513/
State : failure

== Summary ==

Series 8513v1 SKL watermark fixes for !fbcon
http://patchwork.freedesktop.org/api/1.0/series/8513/revisions/1/mbox

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)

ro-bdw-i5-5250u  total:213  pass:197  dwarn:3   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:177  pass:120  dwarn:2   dfail:2   fail:1   skip:51 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
fi-hsw-i7-4770k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1154/

b373842 drm-intel-nightly: 2016y-06m-09d-16h-49m-09s UTC integration manifest
7db1532 drm/i915/gen9: Drop invalid WARN() during data rate calculation
46946f4 drm/i915/gen9: Compute data rates for all planes on first commit
c90f98f drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization

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

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

* Re: [PATCH 0/3] SKL watermark fixes for !fbcon
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
                   ` (3 preceding siblings ...)
  2016-06-10  6:24 ` ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon Patchwork
@ 2016-06-10  7:28 ` Jani Nikula
  2016-06-10 14:17   ` Matt Roper
  2016-06-10  8:48 ` Tvrtko Ursulin
  5 siblings, 1 reply; 21+ messages in thread
From: Jani Nikula @ 2016-06-10  7:28 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

On Fri, 10 Jun 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> There are a handful of watermark bugs that are only triggered (or more easily
> triggered) when running without fbcon.  Thanks Tvrtko for pointing these out.
>
> I do still see some WARN()'s from other parts of the display code when
> launching X in a non-fbcon setup, so there may be other bugfixes needed in
> other parts of the code as well.

Should some or all of these be cc: stable?

BR,
Jani.

>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>
> Matt Roper (3):
>   drm/i915/gen9: Initialize intel_state->active_crtcs during WM
>     sanitization
>   drm/i915/gen9: Compute data rates for all planes on first commit
>   drm/i915/gen9: Drop invalid WARN() during data rate calculation
>
>  drivers/gpu/drm/i915/intel_pm.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)

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

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

* Re: [PATCH 0/3] SKL watermark fixes for !fbcon
  2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
                   ` (4 preceding siblings ...)
  2016-06-10  7:28 ` [PATCH 0/3] " Jani Nikula
@ 2016-06-10  8:48 ` Tvrtko Ursulin
  5 siblings, 0 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-10  8:48 UTC (permalink / raw)
  To: Matt Roper, intel-gfx


Hi,

On 09/06/16 23:14, Matt Roper wrote:
> There are a handful of watermark bugs that are only triggered (or more easily
> triggered) when running without fbcon.  Thanks Tvrtko for pointing these out.
>
> I do still see some WARN()'s from other parts of the display code when
> launching X in a non-fbcon setup, so there may be other bugfixes needed in
> other parts of the code as well.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>
> Matt Roper (3):
>    drm/i915/gen9: Initialize intel_state->active_crtcs during WM
>      sanitization
>    drm/i915/gen9: Compute data rates for all planes on first commit
>    drm/i915/gen9: Drop invalid WARN() during data rate calculation
>
>   drivers/gpu/drm/i915/intel_pm.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)

Thanks Matt, this fixes the issue on my SKL!

For the series,

Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

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

* Re: [PATCH 0/3] SKL watermark fixes for !fbcon
  2016-06-10  7:28 ` [PATCH 0/3] " Jani Nikula
@ 2016-06-10 14:17   ` Matt Roper
  0 siblings, 0 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-10 14:17 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Fri, Jun 10, 2016 at 10:28:13AM +0300, Jani Nikula wrote:
> On Fri, 10 Jun 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> > There are a handful of watermark bugs that are only triggered (or more easily
> > triggered) when running without fbcon.  Thanks Tvrtko for pointing these out.
> >
> > I do still see some WARN()'s from other parts of the display code when
> > launching X in a non-fbcon setup, so there may be other bugfixes needed in
> > other parts of the code as well.
> 
> Should some or all of these be cc: stable?

I don't think so; these are all followups to the SKL atomic wm work that
isn't going to land until 4.8.


Matt

> 
> BR,
> Jani.
> 
> >
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >
> > Matt Roper (3):
> >   drm/i915/gen9: Initialize intel_state->active_crtcs during WM
> >     sanitization
> >   drm/i915/gen9: Compute data rates for all planes on first commit
> >   drm/i915/gen9: Drop invalid WARN() during data rate calculation
> >
> >  drivers/gpu/drm/i915/intel_pm.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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

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

* Re: ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon
  2016-06-10  6:24 ` ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon Patchwork
@ 2016-06-10 14:20   ` Matt Roper
  0 siblings, 0 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-10 14:20 UTC (permalink / raw)
  To: intel-gfx

On Fri, Jun 10, 2016 at 06:24:43AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: SKL watermark fixes for !fbcon
> URL   : https://patchwork.freedesktop.org/series/8513/
> State : failure
> 
> == Summary ==
> 
> Series 8513v1 SKL watermark fixes for !fbcon
> http://patchwork.freedesktop.org/api/1.0/series/8513/revisions/1/mbox
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-cmd:
>                 pass       -> FAIL       (ro-byt-n2820)

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

> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (ro-bdw-i7-5600u)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 skip       -> DMESG-WARN (ro-bdw-i5-5250u)

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

> 
> ro-bdw-i5-5250u  total:213  pass:197  dwarn:3   dfail:0   fail:0   skip:13 
> ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
> ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
> ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
> ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
> ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
> ro-ilk-i7-620lm  total:177  pass:120  dwarn:2   dfail:2   fail:1   skip:51 
> ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
> ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
> ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
> ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
> fi-hsw-i7-4770k failed to connect after reboot
> ro-bdw-i7-5557U failed to connect after reboot
> 
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1154/
> 
> b373842 drm-intel-nightly: 2016y-06m-09d-16h-49m-09s UTC integration manifest
> 7db1532 drm/i915/gen9: Drop invalid WARN() during data rate calculation
> 46946f4 drm/i915/gen9: Compute data rates for all planes on first commit
> c90f98f drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
> 

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

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

* Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
@ 2016-06-13  8:59   ` Maarten Lankhorst
  2016-06-13  9:02     ` Maarten Lankhorst
  2016-06-13 14:49   ` Daniel Vetter
  1 sibling, 1 reply; 21+ messages in thread
From: Maarten Lankhorst @ 2016-06-13  8:59 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 10-06-16 om 00:14 schreef Matt Roper:
> intel_state->active_crtcs is usually only initialized when doing a
> modeset.  During our first atomic commit after boot, we're effectively
> faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> gets initialized before use.
>
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
Ah right. Code assumes that it's valid.

However this still leaves open a hole in case a modeset changes active_crtcs during the first commit.

intel_state->active_crtcs is set at the same time intel_state->modeset is, so
resend with if (!intel_state->modeset) intel_state->active_crtcs = dev_priv->active_crtcs; ?

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

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

* Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-13  8:59   ` Maarten Lankhorst
@ 2016-06-13  9:02     ` Maarten Lankhorst
  0 siblings, 0 replies; 21+ messages in thread
From: Maarten Lankhorst @ 2016-06-13  9:02 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 13-06-16 om 10:59 schreef Maarten Lankhorst:
> Op 10-06-16 om 00:14 schreef Matt Roper:
>> intel_state->active_crtcs is usually only initialized when doing a
>> modeset.  During our first atomic commit after boot, we're effectively
>> faking a modeset to sanitize the DDB/wm setup, so ensure that this field
>> gets initialized before use.
>>
>> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
> Ah right. Code assumes that it's valid.
>
> However this still leaves open a hole in case a modeset changes active_crtcs during the first commit.
>
> intel_state->active_crtcs is set at the same time intel_state->modeset is, so
> resend with if (!intel_state->modeset) intel_state->active_crtcs = dev_priv->active_crtcs; ?
Hm you need connection_mutex lock for that one, so need to lock that first before assigning active_crtcs.
You can grab that lock unconditionally, since we already hold it in case of modeset..

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

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

* Re: [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit
  2016-06-09 22:14 ` [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit Matt Roper
@ 2016-06-13  9:04   ` Maarten Lankhorst
  2016-06-13 14:50   ` Daniel Vetter
  1 sibling, 0 replies; 21+ messages in thread
From: Maarten Lankhorst @ 2016-06-13  9:04 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 10-06-16 om 00:14 schreef Matt Roper:
> When we sanitize our DDB and watermark info during the first atomic
> commit, we need to calculate the total data rate.  Since we haven't
> explicitly added the planes for each CRTC to our atomic state, the total
> data rate calculation will try to use the cached values from a previous
> commit (which are 0 since there was no previous commit); this result is
> incorrect if we inherited any active planes from the BIOS.
>
> During our very first atomic commit, we need to explicitly add all
> active planes to the atomic state to ensure that valid data rate values
> are calculated for them.  Subsequent commits will then have valid cached
> values to fall back on.
>
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation
  2016-06-09 22:14 ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
@ 2016-06-13  9:33   ` Maarten Lankhorst
  2016-06-13  9:38     ` [PATCH i-g-t] kms_universal_plane: Add testcase for when no plane is visible on the crtc Maarten Lankhorst
  2016-06-17 21:25     ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
  0 siblings, 2 replies; 21+ messages in thread
From: Maarten Lankhorst @ 2016-06-13  9:33 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 10-06-16 om 00:14 schreef Matt Roper:
> It's possible to have a non-zero plane mask and still wind up with a
> total data rate of zero.  There are two cases where this can happen:
>
>  * planes are active (from the KMS point of view), but are
>    all fully clipped (positioned offscreen)
>  * the only active plane on a CRTC is the cursor (which is handled
>    independently and not counted into the general data rate computations
>
> These are both valid display setups (although unusual), so we need to
> drop the WARN().
Is there a testcase that triggers this warn? Something for kms_universal_planes perhaps?

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_universal_planes.cursor-only-pipe-* (will reply here as a patch)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH i-g-t] kms_universal_plane: Add testcase for when no plane is visible on the crtc.
  2016-06-13  9:33   ` Maarten Lankhorst
@ 2016-06-13  9:38     ` Maarten Lankhorst
  2016-06-17 21:25     ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
  1 sibling, 0 replies; 21+ messages in thread
From: Maarten Lankhorst @ 2016-06-13  9:38 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

It's completely valid to have a crtc with no active planes, or
only with planes that are invisible. The cursor-only-pipe-* test
will try to set the crtc in a configuration where only the cursor is
visible, then hide the cursor at negative coordinates so no plane is
visible, and finally disables the cursor altogether.

This triggers a WARN_ON in the kernel:
WARN_ON(cstate->plane_mask && total_data_rate == 0);

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
index b06b51e6934c..30590ebd6a4b 100644
--- a/tests/kms_universal_plane.c
+++ b/tests/kms_universal_plane.c
@@ -739,6 +739,59 @@ gen9_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
 }
 
 static void
+cursor_only_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary, *cursor;
+	drmModeModeInfo *mode;
+	struct igt_fb background_fb, cursor_fb;
+
+	igt_assert(data->display.has_universal_planes);
+	igt_skip_on(pipe >= display->n_pipes);
+
+	igt_output_set_pipe(output, pipe);
+	mode = igt_output_get_mode(output);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR);
+	if (!primary || !cursor)
+		igt_skip("Primary and/or cursor are unavailable\n");
+
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    false,
+			    0.0, 0.0, 0.0,
+			    &background_fb);
+
+	igt_create_color_fb(data->drm_fd, 64, 64,
+			    DRM_FORMAT_ARGB8888,
+			    false, 0xff, 0xff, 0xff,
+			    &cursor_fb);
+
+	igt_plane_set_fb(primary, &background_fb);
+	igt_display_commit2(display, COMMIT_LEGACY);
+
+	/* Test with only cursor as active plane. */
+	igt_plane_set_fb(primary, NULL);
+	igt_plane_set_position(cursor, 100, 100);
+	igt_plane_set_fb(cursor, &cursor_fb);
+	igt_display_commit2(display, COMMIT_UNIVERSAL);
+
+	/* Test with cursor as the only plane, hidden. */
+	igt_plane_set_position(cursor, -64, -64);
+	igt_display_commit2(display, COMMIT_UNIVERSAL);
+
+	/* Test with 0 active planes. */
+	igt_plane_set_fb(cursor, NULL);
+	igt_display_commit2(display, COMMIT_UNIVERSAL);
+
+	igt_remove_fb(data->drm_fd, &background_fb);
+	igt_remove_fb(data->drm_fd, &cursor_fb);
+
+	igt_output_set_pipe(output, PIPE_ANY);
+}
+
+static void
 run_tests_for_pipe(data_t *data, enum pipe pipe)
 {
 	igt_output_t *output;
@@ -767,6 +820,11 @@ run_tests_for_pipe(data_t *data, enum pipe pipe)
 		      kmstest_pipe_name(pipe))
 		for_each_connected_output(&data->display, output)
 			gen9_test_pipe(data, pipe, output);
+
+	igt_subtest_f("cursor-only-pipe-%s",
+		      kmstest_pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			cursor_only_test_pipe(data, pipe, output);
 }
 
 static data_t data;

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

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

* Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
  2016-06-13  8:59   ` Maarten Lankhorst
@ 2016-06-13 14:49   ` Daniel Vetter
  2016-06-17 21:00     ` Matt Roper
  1 sibling, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2016-06-13 14:49 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote:
> intel_state->active_crtcs is usually only initialized when doing a
> modeset.  During our first atomic commit after boot, we're effectively
> faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> gets initialized before use.
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 658a756..0cd38ca 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	 * pretend that all pipes switched active status so that we'll
>  	 * ensure a full DDB recompute.
>  	 */
> -	if (dev_priv->wm.distrust_bios_wm)
> +	if (dev_priv->wm.distrust_bios_wm) {
>  		intel_state->active_pipe_changes = ~0;
>  
> +		/*
> +		 * We usually only initialize intel_state->active_crtcs if we
> +		 * we're doing a modeset; make sure this field is always
> +		 * initialized during the sanitization process that happens
> +		 * on the first commit too.
> +		 */
> +		intel_state->active_crtcs = dev_priv->active_crtcs;
> +	}

Can't we move input sanitization out of this? Imo mixing up atomic
check/compute config code with hw state restoring is way too fragile.

Also, why exactly do we have active_crtcs? Seems to just be duplicated
state that can get out of sync, we have too many of those already ...
-Daniel

> +
>  	/*
>  	 * If the modeset changes which CRTC's are active, we need to
>  	 * recompute the DDB allocation for *all* active pipes, even
> -- 
> 2.1.4
> 
> _______________________________________________
> 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

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

* Re: [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit
  2016-06-09 22:14 ` [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit Matt Roper
  2016-06-13  9:04   ` Maarten Lankhorst
@ 2016-06-13 14:50   ` Daniel Vetter
  2016-06-17 21:03     ` Matt Roper
  1 sibling, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2016-06-13 14:50 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote:
> When we sanitize our DDB and watermark info during the first atomic
> commit, we need to calculate the total data rate.  Since we haven't
> explicitly added the planes for each CRTC to our atomic state, the total
> data rate calculation will try to use the cached values from a previous
> commit (which are 0 since there was no previous commit); this result is
> incorrect if we inherited any active planes from the BIOS.
> 
> During our very first atomic commit, we need to explicitly add all
> active planes to the atomic state to ensure that valid data rate values
> are calculated for them.  Subsequent commits will then have valid cached
> values to fall back on.
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0cd38ca..ba08639 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  		if (IS_ERR(cstate))
>  			return PTR_ERR(cstate);
>  
> +		/*
> +		 * If this is our first commit after hw readout, we don't have
> +		 * valid data rate values cached.  Add all planes to ensure we
> +		 * calculate a valid data rate.
> +		 */
> +		if (dev_priv->wm.distrust_bios_wm) {
> +			ret = drm_atomic_add_affected_planes(state,
> +							     &intel_crtc->base);

Again, imo should be pulled out. Other wm code probably has the exact same
bug.
-Daniel

> +			if (ret)
> +				return ret;
> +		}
> +
>  		ret = skl_allocate_pipe_ddb(cstate, ddb);
>  		if (ret)
>  			return ret;
> -- 
> 2.1.4
> 
> _______________________________________________
> 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

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

* Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-13 14:49   ` Daniel Vetter
@ 2016-06-17 21:00     ` Matt Roper
  2016-06-20 12:44       ` Daniel Vetter
  0 siblings, 1 reply; 21+ messages in thread
From: Matt Roper @ 2016-06-17 21:00 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Mon, Jun 13, 2016 at 04:49:49PM +0200, Daniel Vetter wrote:
> On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote:
> > intel_state->active_crtcs is usually only initialized when doing a
> > modeset.  During our first atomic commit after boot, we're effectively
> > faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> > gets initialized before use.
> > 
> > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 658a756..0cd38ca 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> >  	 * pretend that all pipes switched active status so that we'll
> >  	 * ensure a full DDB recompute.
> >  	 */
> > -	if (dev_priv->wm.distrust_bios_wm)
> > +	if (dev_priv->wm.distrust_bios_wm) {
> >  		intel_state->active_pipe_changes = ~0;
> >  
> > +		/*
> > +		 * We usually only initialize intel_state->active_crtcs if we
> > +		 * we're doing a modeset; make sure this field is always
> > +		 * initialized during the sanitization process that happens
> > +		 * on the first commit too.
> > +		 */
> > +		intel_state->active_crtcs = dev_priv->active_crtcs;
> > +	}
> 
> Can't we move input sanitization out of this? Imo mixing up atomic
> check/compute config code with hw state restoring is way too fragile.

Handling this at readout time is tricky since we don't actually
reconstruct things like framebuffers until much later in the process.
Also, if I remember correctly, we don't actually read out sufficient
hardware state on any platform right now (e.g., non-primary planes
aren't read, gen9 plane scalers and such are never read, etc.).  So to
truly sanitize properly/safely, we need to run through a real atomic
commit to make sure that our sanitized values actually match how the
hardware is programmed.  I decided to do the sanitization steps as part
of the first real commit rather than trigger an extra "modeset" on
driver boot, but I can look at the other approach if you think it's
better.

Long term we should probably try harder to read out more complete
hardware state, but for now I figured it was better to get a short-term
fix since there are real regressions (as reported by Tvrtko) and I might
not have time to do a more complicated hw-readout rework series for a
little while.

> 
> Also, why exactly do we have active_crtcs? Seems to just be duplicated
> state that can get out of sync, we have too many of those already ...
> -Daniel

I think it would be harder to reconstruct this information without the
active_crtcs field.  You'd have to grab the CRTC state (and related
locks) for CRTC's that weren't actually part of the current commit.
Technically we have to do that anyway on gen9 for DDB allocation
reasons, but I don't think that's the case for other platforms iirc.


Matt

> 
> > +
> >  	/*
> >  	 * If the modeset changes which CRTC's are active, we need to
> >  	 * recompute the DDB allocation for *all* active pipes, even
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > 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

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

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

* Re: [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit
  2016-06-13 14:50   ` Daniel Vetter
@ 2016-06-17 21:03     ` Matt Roper
  2016-06-20 12:47       ` Daniel Vetter
  0 siblings, 1 reply; 21+ messages in thread
From: Matt Roper @ 2016-06-17 21:03 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Mon, Jun 13, 2016 at 04:50:31PM +0200, Daniel Vetter wrote:
> On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote:
> > When we sanitize our DDB and watermark info during the first atomic
> > commit, we need to calculate the total data rate.  Since we haven't
> > explicitly added the planes for each CRTC to our atomic state, the total
> > data rate calculation will try to use the cached values from a previous
> > commit (which are 0 since there was no previous commit); this result is
> > incorrect if we inherited any active planes from the BIOS.
> > 
> > During our very first atomic commit, we need to explicitly add all
> > active planes to the atomic state to ensure that valid data rate values
> > are calculated for them.  Subsequent commits will then have valid cached
> > values to fall back on.
> > 
> > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0cd38ca..ba08639 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> >  		if (IS_ERR(cstate))
> >  			return PTR_ERR(cstate);
> >  
> > +		/*
> > +		 * If this is our first commit after hw readout, we don't have
> > +		 * valid data rate values cached.  Add all planes to ensure we
> > +		 * calculate a valid data rate.
> > +		 */
> > +		if (dev_priv->wm.distrust_bios_wm) {
> > +			ret = drm_atomic_add_affected_planes(state,
> > +							     &intel_crtc->base);
> 
> Again, imo should be pulled out. Other wm code probably has the exact same
> bug.
> -Daniel

By "pulled out" do you mean ensure the initial data rates are calculated
at readout time?  As I mentioned on the other email, we don't actually
read out non-primary plane states, plane scaler states, etc., so we
don't actually have all the information we need to handle this at
readout time; out hardware and software states aren't truly in sync
until the first atomic commit happens.

We should get better at reading out more hardware state, but that's a
bit more involved and I've been short on upstream development time
lately so I wanted to make sure I had a timely fix for the regressions
Tvrtko reported.


Matt

> 
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> >  		ret = skl_allocate_pipe_ddb(cstate, ddb);
> >  		if (ret)
> >  			return ret;
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > 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

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

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

* Re: [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation
  2016-06-13  9:33   ` Maarten Lankhorst
  2016-06-13  9:38     ` [PATCH i-g-t] kms_universal_plane: Add testcase for when no plane is visible on the crtc Maarten Lankhorst
@ 2016-06-17 21:25     ` Matt Roper
  1 sibling, 0 replies; 21+ messages in thread
From: Matt Roper @ 2016-06-17 21:25 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Mon, Jun 13, 2016 at 11:33:50AM +0200, Maarten Lankhorst wrote:
> Op 10-06-16 om 00:14 schreef Matt Roper:
> > It's possible to have a non-zero plane mask and still wind up with a
> > total data rate of zero.  There are two cases where this can happen:
> >
> >  * planes are active (from the KMS point of view), but are
> >    all fully clipped (positioned offscreen)
> >  * the only active plane on a CRTC is the cursor (which is handled
> >    independently and not counted into the general data rate computations
> >
> > These are both valid display setups (although unusual), so we need to
> > drop the WARN().
> Is there a testcase that triggers this warn? Something for
> kms_universal_planes perhaps?

I do have the start of a new IGT testcase that generates hundreds of
random display configurations (for the purposes of searching for
watermark corner cases); that test triggers this WARN(), but I haven't
posted it yet.

But having extra subtests in the universal plane test sounds good too;
thanks!


Matt

> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: kms_universal_planes.cursor-only-pipe-* (will reply here as a patch)

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

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

* Re: [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization
  2016-06-17 21:00     ` Matt Roper
@ 2016-06-20 12:44       ` Daniel Vetter
  0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2016-06-20 12:44 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Jun 17, 2016 at 02:00:02PM -0700, Matt Roper wrote:
> On Mon, Jun 13, 2016 at 04:49:49PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote:
> > > intel_state->active_crtcs is usually only initialized when doing a
> > > modeset.  During our first atomic commit after boot, we're effectively
> > > faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> > > gets initialized before use.
> > > 
> > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 658a756..0cd38ca 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > >  	 * pretend that all pipes switched active status so that we'll
> > >  	 * ensure a full DDB recompute.
> > >  	 */
> > > -	if (dev_priv->wm.distrust_bios_wm)
> > > +	if (dev_priv->wm.distrust_bios_wm) {
> > >  		intel_state->active_pipe_changes = ~0;
> > >  
> > > +		/*
> > > +		 * We usually only initialize intel_state->active_crtcs if we
> > > +		 * we're doing a modeset; make sure this field is always
> > > +		 * initialized during the sanitization process that happens
> > > +		 * on the first commit too.
> > > +		 */
> > > +		intel_state->active_crtcs = dev_priv->active_crtcs;
> > > +	}
> > 
> > Can't we move input sanitization out of this? Imo mixing up atomic
> > check/compute config code with hw state restoring is way too fragile.
> 
> Handling this at readout time is tricky since we don't actually
> reconstruct things like framebuffers until much later in the process.
> Also, if I remember correctly, we don't actually read out sufficient
> hardware state on any platform right now (e.g., non-primary planes
> aren't read, gen9 plane scalers and such are never read, etc.).  So to
> truly sanitize properly/safely, we need to run through a real atomic
> commit to make sure that our sanitized values actually match how the
> hardware is programmed.  I decided to do the sanitization steps as part
> of the first real commit rather than trigger an extra "modeset" on
> driver boot, but I can look at the other approach if you think it's
> better.
> 
> Long term we should probably try harder to read out more complete
> hardware state, but for now I figured it was better to get a short-term
> fix since there are real regressions (as reported by Tvrtko) and I might
> not have time to do a more complicated hw-readout rework series for a
> little while.

I'm just unhappy with gunking up the atomic check/commit paths. Could we
stuff this into the plane state readout/fixup paths instead? That also has
the benefit of keeping all the readout logic more together in one place.

> > Also, why exactly do we have active_crtcs? Seems to just be duplicated
> > state that can get out of sync, we have too many of those already ...
> > -Daniel
> 
> I think it would be harder to reconstruct this information without the
> active_crtcs field.  You'd have to grab the CRTC state (and related
> locks) for CRTC's that weren't actually part of the current commit.
> Technically we have to do that anyway on gen9 for DDB allocation
> reasons, but I don't think that's the case for other platforms iirc.

Yeah, if the use case is strictly read-only (like we have active_planes
on the crtc) then I think it's ok. Might even be something the core
helpers could/should track for us in drm_atomic_state. But for that we
first need to create a real drm_device->mode_config->state pointer. Seems
a bit excessive for just active_crtcs.
-Daniel

> 
> 
> Matt
> 
> > 
> > > +
> > >  	/*
> > >  	 * If the modeset changes which CRTC's are active, we need to
> > >  	 * recompute the DDB allocation for *all* active pipes, even
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

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

* Re: [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit
  2016-06-17 21:03     ` Matt Roper
@ 2016-06-20 12:47       ` Daniel Vetter
  0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2016-06-20 12:47 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Jun 17, 2016 at 02:03:07PM -0700, Matt Roper wrote:
> On Mon, Jun 13, 2016 at 04:50:31PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote:
> > > When we sanitize our DDB and watermark info during the first atomic
> > > commit, we need to calculate the total data rate.  Since we haven't
> > > explicitly added the planes for each CRTC to our atomic state, the total
> > > data rate calculation will try to use the cached values from a previous
> > > commit (which are 0 since there was no previous commit); this result is
> > > incorrect if we inherited any active planes from the BIOS.
> > > 
> > > During our very first atomic commit, we need to explicitly add all
> > > active planes to the atomic state to ensure that valid data rate values
> > > are calculated for them.  Subsequent commits will then have valid cached
> > > values to fall back on.
> > > 
> > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 0cd38ca..ba08639 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > >  		if (IS_ERR(cstate))
> > >  			return PTR_ERR(cstate);
> > >  
> > > +		/*
> > > +		 * If this is our first commit after hw readout, we don't have
> > > +		 * valid data rate values cached.  Add all planes to ensure we
> > > +		 * calculate a valid data rate.
> > > +		 */
> > > +		if (dev_priv->wm.distrust_bios_wm) {
> > > +			ret = drm_atomic_add_affected_planes(state,
> > > +							     &intel_crtc->base);
> > 
> > Again, imo should be pulled out. Other wm code probably has the exact same
> > bug.
> > -Daniel
> 
> By "pulled out" do you mean ensure the initial data rates are calculated
> at readout time?  As I mentioned on the other email, we don't actually
> read out non-primary plane states, plane scaler states, etc., so we
> don't actually have all the information we need to handle this at
> readout time; out hardware and software states aren't truly in sync
> until the first atomic commit happens.
> 
> We should get better at reading out more hardware state, but that's a
> bit more involved and I've been short on upstream development time
> lately so I wanted to make sure I had a timely fix for the regressions
> Tvrtko reported.

One more I forgot in the other reply: We force-disable non-primary planes.
Hence wm state for those are kinda irrelevant. Again I think it's best to
group all this together with the plane readout/fb reconstruction code, so
that we have all the plane readout logic in one place. I know that we
already need to have the split between modeset readoud and plane
reconstruction, and that's imo bad enough. If possible I'd like if we
don't have to split it further into a separate plane/wm fixup path that's
hiding in the atomic_check code somewhere.
-Daniel

> 
> 
> Matt
> 
> > 
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +
> > >  		ret = skl_allocate_pipe_ddb(cstate, ddb);
> > >  		if (ret)
> > >  			return ret;
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

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

end of thread, other threads:[~2016-06-20 12:47 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-09 22:14 [PATCH 0/3] SKL watermark fixes for !fbcon Matt Roper
2016-06-09 22:14 ` [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization Matt Roper
2016-06-13  8:59   ` Maarten Lankhorst
2016-06-13  9:02     ` Maarten Lankhorst
2016-06-13 14:49   ` Daniel Vetter
2016-06-17 21:00     ` Matt Roper
2016-06-20 12:44       ` Daniel Vetter
2016-06-09 22:14 ` [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit Matt Roper
2016-06-13  9:04   ` Maarten Lankhorst
2016-06-13 14:50   ` Daniel Vetter
2016-06-17 21:03     ` Matt Roper
2016-06-20 12:47       ` Daniel Vetter
2016-06-09 22:14 ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
2016-06-13  9:33   ` Maarten Lankhorst
2016-06-13  9:38     ` [PATCH i-g-t] kms_universal_plane: Add testcase for when no plane is visible on the crtc Maarten Lankhorst
2016-06-17 21:25     ` [PATCH 3/3] drm/i915/gen9: Drop invalid WARN() during data rate calculation Matt Roper
2016-06-10  6:24 ` ✗ Ro.CI.BAT: failure for SKL watermark fixes for !fbcon Patchwork
2016-06-10 14:20   ` Matt Roper
2016-06-10  7:28 ` [PATCH 0/3] " Jani Nikula
2016-06-10 14:17   ` Matt Roper
2016-06-10  8:48 ` Tvrtko Ursulin

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.