Re: [Intel-gfx] [PATCH v5 4/4] drm/i915/perf: add support for Coffeelake GT2

[Lionel Landwerlin]

On 04/09/17 10:25, Matthew Auld wrote:

On 30 August 2017 at 17:12, Lionel Landwerlin
 wrote:

Add the test configuration & timestamp frequency for Coffeelake GT2.

Signed-off-by: Lionel Landwerlin 

Do we not want to also disable the clock-ratio-change reports? Also
can we not get away with just making the condition INTEL_GEN(i915) >=
9?

Reviewed-by: Matthew Auld 


You're right!

Will send an update.

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

[Intel-gfx] ✗ Fi.CI.IGT: warning for drm/i915: Speed up DMC firmware loading (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Speed up DMC firmware loading (rev3)
URL   : https://patchwork.freedesktop.org/series/29688/
State : warning

== Summary ==

Test tools_test:
Subgroup tools_test:
pass   -> DMESG-WARN (shard-hsw)

shard-hswtotal:2265 pass:1232 dwarn:1   dfail:0   fail:16  skip:1016 
time:9668s

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5577/shards.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: warning for drm/i915: Speed up DMC firmware loading (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Speed up DMC firmware loading (rev2)
URL   : https://patchwork.freedesktop.org/series/29688/
State : warning

== Summary ==

Test kms_cursor_crc:
Subgroup cursor-256x256-offscreen:
pass   -> SKIP   (shard-hsw)
Test perf:
Subgroup blocking:
pass   -> FAIL   (shard-hsw) fdo#102252
Test kms_setmode:
Subgroup basic:
fail   -> PASS   (shard-hsw) fdo#99912

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

shard-hswtotal:2265 pass:1231 dwarn:1   dfail:0   fail:16  skip:1017 
time:9590s

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5576/shards.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Speed up DMC firmware loading (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Speed up DMC firmware loading (rev3)
URL   : https://patchwork.freedesktop.org/series/29688/
State : success

== Summary ==

Series 29688v3 drm/i915: Speed up DMC firmware loading
https://patchwork.freedesktop.org/api/1.0/series/29688/revisions/3/mbox/

Test kms_flip:
Subgroup basic-flip-vs-modeset:
skip   -> PASS   (fi-skl-x1585l) fdo#101781

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

fi-bdw-5557u total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:458s
fi-bdw-gvtdvmtotal:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:440s
fi-blb-e6850 total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  
time:356s
fi-bsw-n3050 total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  
time:562s
fi-bwr-2160  total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 
time:253s
fi-bxt-j4205 total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:520s
fi-byt-j1900 total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  
time:533s
fi-byt-n2820 total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  
time:518s
fi-cfl-s total:288  pass:250  dwarn:4   dfail:0   fail:0   skip:34  
time:471s
fi-elk-e7500 total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  
time:437s
fi-glk-2atotal:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:616s
fi-hsw-4770  total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:446s
fi-hsw-4770r total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:425s
fi-ilk-650   total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  
time:424s
fi-ivb-3520m total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:497s
fi-ivb-3770  total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:472s
fi-kbl-7500u total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  
time:510s
fi-kbl-7560u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:599s
fi-kbl-r total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:603s
fi-pnv-d510  total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  
time:530s
fi-skl-6260u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:469s
fi-skl-6700k total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:544s
fi-skl-6770hqtotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:517s
fi-skl-gvtdvmtotal:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  
time:445s
fi-skl-x1585ltotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:504s
fi-snb-2520m total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  
time:559s
fi-snb-2600  total:288  pass:249  dwarn:0   dfail:0   fail:1   skip:38  
time:407s

9dd459ef87a90495aac4cee73831f4cd694048ca drm-tip: 2017y-09m-04d-19h-10m-24s UTC 
integration manifest
292f6d3959ee drm/i915: Speed up DMC firmware loading

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5577/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Speed up DMC firmware loading

[David Weinehall]

On Mon, Sep 04, 2017 at 08:15:57PM +0100, Chris Wilson wrote:
> Quoting Chris Wilson (2017-09-04 20:14:32)
> > Quoting David Weinehall (2017-09-04 20:08:06)
> > > Currently we're doing:
> > > 
> > > 1. acquire lock
> > > 2. write word to hardware
> > > 3. release lock
> > > 4. repeat from 1
> > > 
> > > to load the DMC firmware. Due to the cost of acquiring/releasing a lock,
> > > and the size of the DMC firmware, this slows down DMC loading a lot.
> > > 
> > > This patch simply acquires the lock, writes the entire firmware,
> > > then releases the lock.  Testing shows resume speedups
> > > in the order of 10ms on platforms with DMC firmware (GEN9+).
> > > 
> > > v2: Per feedback from Chris & Ville there's no need to do the whole
> > > forcewake dance, so lose that bit (Chris, Ville)
> > > 
> > > v3: Actually send the new version of the patch...
> > > 
> > > Signed-off-by: David Weinehall 
> > > ---
> > >  drivers/gpu/drm/i915/intel_csr.c | 8 +++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_csr.c 
> > > b/drivers/gpu/drm/i915/intel_csr.c
> > > index 965988f79a55..28ea24932ef1 100644
> > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private 
> > > *dev_priv)
> > >  {
> > > u32 *payload = dev_priv->csr.dmc_payload;
> > > uint32_t i, fw_size;
> > > +   unsigned long flags;
> > >  
> > > if (!HAS_CSR(dev_priv)) {
> > > DRM_ERROR("No CSR support available for this platform\n");
> > > @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private 
> > > *dev_priv)
> > > }
> > >  
> > > fw_size = dev_priv->csr.dmc_fw_size;
> > > +   assert_rpm_wakelock_held(dev_priv);
> > > +   spin_lock_irqsave(_priv->uncore.lock, flags);
> > > +
> > > for (i = 0; i < fw_size; i++)
> > > -   I915_WRITE(CSR_PROGRAM(i), payload[i]);
> > > +   I915_WRITE_FW(CSR_PROGRAM(i), payload[i]);
> > > +
> > > +   spin_unlock_irqrestore(_priv->uncore.lock, flags);
> > 
> > Still would like to see the version without the uncore.lock. Afaict,
> > there isn't a requirement here -- unless you are serialising between
> > multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM.
> 
> You may also want to consider a preempt_disable around this block as
> well, the argument being that we want the writes tightly grouped.
> -Chris

OK, I'll spin a non-spinning v4 tomorrow :)


Kind regards, David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Speed up DMC firmware loading

[Chris Wilson]

Quoting Chris Wilson (2017-09-04 20:14:32)
> Quoting David Weinehall (2017-09-04 20:08:06)
> > Currently we're doing:
> > 
> > 1. acquire lock
> > 2. write word to hardware
> > 3. release lock
> > 4. repeat from 1
> > 
> > to load the DMC firmware. Due to the cost of acquiring/releasing a lock,
> > and the size of the DMC firmware, this slows down DMC loading a lot.
> > 
> > This patch simply acquires the lock, writes the entire firmware,
> > then releases the lock.  Testing shows resume speedups
> > in the order of 10ms on platforms with DMC firmware (GEN9+).
> > 
> > v2: Per feedback from Chris & Ville there's no need to do the whole
> > forcewake dance, so lose that bit (Chris, Ville)
> > 
> > v3: Actually send the new version of the patch...
> > 
> > Signed-off-by: David Weinehall 
> > ---
> >  drivers/gpu/drm/i915/intel_csr.c | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_csr.c 
> > b/drivers/gpu/drm/i915/intel_csr.c
> > index 965988f79a55..28ea24932ef1 100644
> > --- a/drivers/gpu/drm/i915/intel_csr.c
> > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private 
> > *dev_priv)
> >  {
> > u32 *payload = dev_priv->csr.dmc_payload;
> > uint32_t i, fw_size;
> > +   unsigned long flags;
> >  
> > if (!HAS_CSR(dev_priv)) {
> > DRM_ERROR("No CSR support available for this platform\n");
> > @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private 
> > *dev_priv)
> > }
> >  
> > fw_size = dev_priv->csr.dmc_fw_size;
> > +   assert_rpm_wakelock_held(dev_priv);
> > +   spin_lock_irqsave(_priv->uncore.lock, flags);
> > +
> > for (i = 0; i < fw_size; i++)
> > -   I915_WRITE(CSR_PROGRAM(i), payload[i]);
> > +   I915_WRITE_FW(CSR_PROGRAM(i), payload[i]);
> > +
> > +   spin_unlock_irqrestore(_priv->uncore.lock, flags);
> 
> Still would like to see the version without the uncore.lock. Afaict,
> there isn't a requirement here -- unless you are serialising between
> multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM.

You may also want to consider a preempt_disable around this block as
well, the argument being that we want the writes tightly grouped.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Speed up DMC firmware loading

[Chris Wilson]

Quoting David Weinehall (2017-09-04 20:08:06)
> Currently we're doing:
> 
> 1. acquire lock
> 2. write word to hardware
> 3. release lock
> 4. repeat from 1
> 
> to load the DMC firmware. Due to the cost of acquiring/releasing a lock,
> and the size of the DMC firmware, this slows down DMC loading a lot.
> 
> This patch simply acquires the lock, writes the entire firmware,
> then releases the lock.  Testing shows resume speedups
> in the order of 10ms on platforms with DMC firmware (GEN9+).
> 
> v2: Per feedback from Chris & Ville there's no need to do the whole
> forcewake dance, so lose that bit (Chris, Ville)
> 
> v3: Actually send the new version of the patch...
> 
> Signed-off-by: David Weinehall 
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c 
> b/drivers/gpu/drm/i915/intel_csr.c
> index 965988f79a55..28ea24932ef1 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private 
> *dev_priv)
>  {
> u32 *payload = dev_priv->csr.dmc_payload;
> uint32_t i, fw_size;
> +   unsigned long flags;
>  
> if (!HAS_CSR(dev_priv)) {
> DRM_ERROR("No CSR support available for this platform\n");
> @@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private 
> *dev_priv)
> }
>  
> fw_size = dev_priv->csr.dmc_fw_size;
> +   assert_rpm_wakelock_held(dev_priv);
> +   spin_lock_irqsave(_priv->uncore.lock, flags);
> +
> for (i = 0; i < fw_size; i++)
> -   I915_WRITE(CSR_PROGRAM(i), payload[i]);
> +   I915_WRITE_FW(CSR_PROGRAM(i), payload[i]);
> +
> +   spin_unlock_irqrestore(_priv->uncore.lock, flags);

Still would like to see the version without the uncore.lock. Afaict,
there isn't a requirement here -- unless you are serialising between
multiple users (concurrent intel_csr_load_program?) of CSR_PROGRAM.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Speed up DMC firmware loading

[David Weinehall]

On Mon, Sep 04, 2017 at 07:55:56PM +0100, Chris Wilson wrote:
> Quoting David Weinehall (2017-09-04 19:38:04)
> > v2: Per feedback from Chris & Ville there's no need to do the whole
> > forcewake dance, so lose that bit (Chris, Ville)
> 
> > @@ -251,9 +253,20 @@ void intel_csr_load_program(struct drm_i915_private 
> > *dev_priv)
> > return;
> > }
> >  
> > +   fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> > +   CSR_PROGRAM(0),
> > +   FW_REG_WRITE);
> > +
> > fw_size = dev_priv->csr.dmc_fw_size;
> > +   assert_rpm_wakelock_held(dev_priv);
> > +   spin_lock_irqsave(_priv->uncore.lock, flags);
> > +   intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
> 
> Still there!

Yeah, I missed the git add... Whooopsie; check v3.


Kind regards, David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Speed up DMC firmware loading

[David Weinehall]

Currently we're doing:

1. acquire lock
2. write word to hardware
3. release lock
4. repeat from 1

to load the DMC firmware. Due to the cost of acquiring/releasing a lock,
and the size of the DMC firmware, this slows down DMC loading a lot.

This patch simply acquires the lock, writes the entire firmware,
then releases the lock.  Testing shows resume speedups
in the order of 10ms on platforms with DMC firmware (GEN9+).

v2: Per feedback from Chris & Ville there's no need to do the whole
forcewake dance, so lose that bit (Chris, Ville)

v3: Actually send the new version of the patch...

Signed-off-by: David Weinehall 
---
 drivers/gpu/drm/i915/intel_csr.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 965988f79a55..28ea24932ef1 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -240,6 +240,7 @@ void intel_csr_load_program(struct drm_i915_private 
*dev_priv)
 {
u32 *payload = dev_priv->csr.dmc_payload;
uint32_t i, fw_size;
+   unsigned long flags;
 
if (!HAS_CSR(dev_priv)) {
DRM_ERROR("No CSR support available for this platform\n");
@@ -252,8 +253,13 @@ void intel_csr_load_program(struct drm_i915_private 
*dev_priv)
}
 
fw_size = dev_priv->csr.dmc_fw_size;
+   assert_rpm_wakelock_held(dev_priv);
+   spin_lock_irqsave(_priv->uncore.lock, flags);
+
for (i = 0; i < fw_size; i++)
-   I915_WRITE(CSR_PROGRAM(i), payload[i]);
+   I915_WRITE_FW(CSR_PROGRAM(i), payload[i]);
+
+   spin_unlock_irqrestore(_priv->uncore.lock, flags);
 
for (i = 0; i < dev_priv->csr.mmio_count; i++) {
I915_WRITE(dev_priv->csr.mmioaddr[i],
-- 
2.14.1

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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Speed up DMC firmware loading (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Speed up DMC firmware loading (rev2)
URL   : https://patchwork.freedesktop.org/series/29688/
State : success

== Summary ==

Series 29688v2 drm/i915: Speed up DMC firmware loading
https://patchwork.freedesktop.org/api/1.0/series/29688/revisions/2/mbox/

Test chamelium:
Subgroup dp-crc-fast:
dmesg-fail -> PASS   (fi-kbl-7500u) fdo#102514
Test gem_exec_reloc:
Subgroup basic-cpu-active:
incomplete -> PASS   (fi-byt-j1900)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
fail   -> PASS   (fi-snb-2600) fdo#100215 +1
dmesg-warn -> PASS   (fi-glk-2a)

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

fi-bdw-5557u total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:457s
fi-bdw-gvtdvmtotal:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:432s
fi-blb-e6850 total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  
time:362s
fi-bsw-n3050 total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  
time:559s
fi-bwr-2160  total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 
time:253s
fi-bxt-j4205 total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:518s
fi-byt-j1900 total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  
time:519s
fi-byt-n2820 total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  
time:514s
fi-cfl-s total:288  pass:250  dwarn:4   dfail:0   fail:0   skip:34  
time:461s
fi-elk-e7500 total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  
time:429s
fi-glk-2atotal:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:610s
fi-hsw-4770  total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:447s
fi-hsw-4770r total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:425s
fi-ilk-650   total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  
time:421s
fi-ivb-3520m total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:495s
fi-ivb-3770  total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:480s
fi-kbl-7500u total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  
time:514s
fi-kbl-7560u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:596s
fi-kbl-r total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:604s
fi-pnv-d510  total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  
time:528s
fi-skl-6260u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:467s
fi-skl-6700k total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:542s
fi-skl-6770hqtotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:516s
fi-skl-gvtdvmtotal:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  
time:444s
fi-skl-x1585ltotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:501s
fi-snb-2520m total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  
time:545s
fi-snb-2600  total:288  pass:249  dwarn:0   dfail:0   fail:1   skip:38  
time:414s

233bea3d23383193c4dfab14828a1b156eb96575 drm-tip: 2017y-09m-04d-16h-58m-50s UTC 
integration manifest
e82615b56b42 drm/i915: Speed up DMC firmware loading

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5576/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Speed up DMC firmware loading

[Chris Wilson]

Quoting David Weinehall (2017-09-04 19:38:04)
> v2: Per feedback from Chris & Ville there's no need to do the whole
> forcewake dance, so lose that bit (Chris, Ville)

> @@ -251,9 +253,20 @@ void intel_csr_load_program(struct drm_i915_private 
> *dev_priv)
> return;
> }
>  
> +   fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> +   CSR_PROGRAM(0),
> +   FW_REG_WRITE);
> +
> fw_size = dev_priv->csr.dmc_fw_size;
> +   assert_rpm_wakelock_held(dev_priv);
> +   spin_lock_irqsave(_priv->uncore.lock, flags);
> +   intel_uncore_forcewake_get__locked(dev_priv, fw_domains);

Still there!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"

[Maarten Lankhorst]

Op 04-09-17 om 17:58 schreef Ville Syrjälä:
> On Mon, Sep 04, 2017 at 12:39:25PM +0200, Maarten Lankhorst wrote:
>> Op 31-08-17 om 20:48 schreef Ville Syrjälä:
>>> On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrj...@linux.intel.com 
>>> wrote:
 From: Ville Syrjälä 

 Make the min_pixclk thing less confusing by changing it to track
 the minimum acceptable cdclk frequency instead. This means moving
 the application of the guardbands to a slightly higher level from
 the low level platform specific calc_cdclk() functions.

 The immediate benefit is elimination of the confusing 2x factors
 on GLK/CNL+ in the audio workarounds (which stems from the fact
 that the pipes produce two pixels per clock).

 v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage 
 handling
 v3: Squash with the CNL cdclk limits patch (DK)
 v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)

 Cc: Paulo Zanoni 
 Cc: Rodrigo Vivi 
 Cc: Dhinakaran Pandiyan 
 Cc: Maarten Lankhorst 
 Reviewed-by: Dhinakaran Pandiyan 
 Signed-off-by: Ville Syrjälä 
>>> I didn't get any objections from the CNL camp, so I went ahead and
>>> pushed the series. Thanks for the reviews.
>>>
>> I seem to have a WARN_ON during init now on my broadwell, likely related to 
>> this series?
>>
>> [   13.105310] i915 :00:02.0: vgaarb: changed VGA decodes: 
>> olddecodes=io+mem,decodes=io+mem:owns=io+mem
>> [   13.132264] systemd-journald[159]: Successfully sent stream file 
>> descriptor to service manager.
>> [   13.161016] WARN_ON(min_cdclk < 0)
> Hmm. I think I need to see the full dmesg to figure this one out.
>
[  188.354997] [drm:intel_modeset_init [i915]] 3 display pipes available.
[  188.356575] [drm:intel_update_cdclk [i915]] Current CD clock rate: 54 
kHz, VCO: 0 kHz, ref: 0 kHz
[  188.357067] [drm:intel_update_max_cdclk [i915]] Max CD clock rate: 54 kHz
[  188.357220] [drm:intel_update_max_cdclk [i915]] Max dotclock rate: 54 kHz
... 
[  188.363648] [drm:drm_atomic_set_mode_for_crtc [drm]] Set [MODE:640x480] for 
CRTC state 8800c79e2200
[  188.363784] [drm:intel_crtc_compute_min_cdclk [i915]] required cdclk (607500 
kHz) exceeds max (54 kHz)
[  188.363848] WARN_ON(min_cdclk < 0)
...
[  188.368005] [drm:intel_dump_pipe_config [i915]] [CRTC:36:pipe 
A][setup_hw_state]
[  188.368155] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: A, pipe bpp: 
24, dithering: 0
[  188.368276] [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0
[  188.368390] [drm:intel_dump_pipe_config [i915]] requested mode:
[  188.368692] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 
1286 54 640 656 752 800 480 490 492 525 0x40 0xa
[  188.368833] [drm:intel_dump_pipe_config [i915]] adjusted mode:
[  188.368897] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 
1286 54 640 656 752 800 480 490 492 525 0x40 0xa
[  188.369041] [drm:intel_dump_pipe_config [i915]] crtc timings: 54 640 656 
752 800 480 490 492 525, type: 0x40 flags: 0xa
[  188.369235] [drm:intel_dump_pipe_config [i915]] port clock: 54, pipe src 
size: 720x400, pixel rate 607500
[  188.369366] [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x, 
size: 0x028001e0, enabled
[  188.369490] [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0
[  188.369600] [drm:hsw_dump_hw_state [i915]] dpll_hw_state: wrpll: 0x0 spll: 
0x0
[  188.369719] [drm:intel_dump_pipe_config [i915]] planes on this crtc


Is this enough info?

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

[Intel-gfx] [PATCH v2] drm/i915: Speed up DMC firmware loading

[David Weinehall]

Currently we're doing:

1. acquire lock
2. write word to hardware
3. release lock
4. repeat from 1

to load the DMC firmware. Due to the cost of acquiring/releasing a lock,
and the size of the DMC firmware, this slows down DMC loading a lot.

This patch simply acquires the lock, writes the entire firmware,
then releases the lock.  Testing shows resume speedups
in the order of 10ms on platforms with DMC firmware (GEN9+).

v2: Per feedback from Chris & Ville there's no need to do the whole
forcewake dance, so lose that bit (Chris, Ville)

Signed-off-by: David Weinehall 
---
 drivers/gpu/drm/i915/intel_csr.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 965988f79a55..b7a6ef7e0d53 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -239,7 +239,9 @@ static void gen9_set_dc_state_debugmask(struct 
drm_i915_private *dev_priv)
 void intel_csr_load_program(struct drm_i915_private *dev_priv)
 {
u32 *payload = dev_priv->csr.dmc_payload;
+   enum forcewake_domains fw_domains;
uint32_t i, fw_size;
+   unsigned long flags;
 
if (!HAS_CSR(dev_priv)) {
DRM_ERROR("No CSR support available for this platform\n");
@@ -251,9 +253,20 @@ void intel_csr_load_program(struct drm_i915_private 
*dev_priv)
return;
}
 
+   fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
+   CSR_PROGRAM(0),
+   FW_REG_WRITE);
+
fw_size = dev_priv->csr.dmc_fw_size;
+   assert_rpm_wakelock_held(dev_priv);
+   spin_lock_irqsave(_priv->uncore.lock, flags);
+   intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
+
for (i = 0; i < fw_size; i++)
-   I915_WRITE(CSR_PROGRAM(i), payload[i]);
+   I915_WRITE_FW(CSR_PROGRAM(i), payload[i]);
+
+   intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
+   spin_unlock_irqrestore(_priv->uncore.lock, flags);
 
for (i = 0; i < dev_priv->csr.mmio_count; i++) {
I915_WRITE(dev_priv->csr.mmioaddr[i],
-- 
2.14.1

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

Re: [Intel-gfx] [PATCH i-g-t v2] tests/kms_frontbuffer_tracking: increase FBC wait timeout to 5s

[Paulo Zanoni]

Em Seg, 2017-09-04 às 11:45 +0100, Chris Wilson escreveu:
> Quoting Paulo Zanoni (2017-09-01 20:12:01)
> > Em Sex, 2017-08-25 às 14:11 +0100, Chris Wilson escreveu:
> > > Quoting Lofstedt, Marta (2017-08-25 13:50:16)
> > > > 
> > > > 
> > > > > -Original Message-
> > > > > From: Lofstedt, Marta
> > > > > Sent: Friday, August 25, 2017 2:54 PM
> > > > > To: 'Chris Wilson' ; intel-gfx@list
> > > > > s.fr
> > > > > eedesktop.org
> > > > > Subject: RE: [Intel-gfx] [PATCH i-g-t v2]
> > > > > tests/kms_frontbuffer_tracking:
> > > > > increase FBC wait timeout to 5s
> > > > > 
> > > > > 
> > > > > 
> > > > > > -Original Message-
> > > > > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > > > > > Sent: Friday, August 25, 2017 1:47 PM
> > > > > > To: Lofstedt, Marta ; intel-
> > > > > > g...@lists.freedesktop.org
> > > > > > Subject: Re: [Intel-gfx] [PATCH i-g-t v2]
> > > > > > tests/kms_frontbuffer_tracking:
> > > > > > increase FBC wait timeout to 5s
> > > > > > 
> > > > > > Quoting Marta Lofstedt (2017-08-25 11:40:29)
> > > > > > > From: "Lofstedt, Marta" 
> > > > > > > 
> > > > > > > The subtests: igt@kms_frontbuffer_tracking@fbc-*draw*
> > > > > > > has non-consistent results, pending between fail and
> > > > > > > pass.
> > > > > > > The fails are always due to "FBC disabled".
> > > > > > > With this increase in timeout the flip-flop behavior is
> > > > > > > no
> > > > > > > longer
> > > > > > > reproducible.
> > > > > > > 
> > > > > > > This is a partial revert of:
> > > > > > > 64590c7b768dc8d8dd962f812d5ff5a39e7e8b54,
> > > > > > > where the timeout was decreased from 5s to 2s.
> > > > > > > After investigating the timeout needed, the conclusion is
> > > > > > > that the
> > > > > > > longer timeout is only needed when the test swaps between
> > > > > > > some
> > > > > > > specific draw domains, typically blt vs. mmap_cpu.
> > > > > > > The objective of the FBC part of the tests is not to
> > > > > > > benchmark draw
> > > > > > > domain changes, it is to check that FBC was (re-)enabled.
> > > > > > > 
> > > > > > > V2: Added documentation
> > > > > > > 
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=10
> > > > > > > 1623
> > > > > > > Signed-off-by: Marta Lofstedt 
> > > > > > > Acked-by: Paulo Zanoni 
> > > > > > > ---
> > > > > > >  tests/kms_frontbuffer_tracking.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > > > > > b/tests/kms_frontbuffer_tracking.c
> > > > > > > index e03524f1..2538450c 100644
> > > > > > > --- a/tests/kms_frontbuffer_tracking.c
> > > > > > > +++ b/tests/kms_frontbuffer_tracking.c
> > > > > > > @@ -924,7 +924,7 @@ static bool
> > > > > > > fbc_stride_not_supported(void)
> > > > > > > 
> > > > > > >  static bool fbc_wait_until_enabled(void)  {
> > > > > > 
> > > > > > Try igt_drop_caches_set(device, DROP_RETIRE); instead of
> > > > > > relaxing the
> > > > > > timeout.
> > > > > > -Chris
> > > > > 
> > > > > OK, I will test that and do a V3 if it works!
> > > > > /Marta
> > > > 
> > > > I did some initial testing with igt_drop_caches_set inside
> > > > fbc_wait_until_enabled and it looks good, I will add this to my
> > > > weekend tests to get more results. This also appear to improve
> > > > the
> > > > runtime of the tests quite a bit. So, maybe the
> > > > igt_drop_caches_set
> > > > should be placed somewhere else so it will give runtime
> > > > improvements not only for the FBC related sub-tests.
> > > 
> > > Sure, all the waits can do with the retire first, give it a
> > > common
> > > function and a comment for the rationale (which should pretty
> > > much
> > > the
> > > same as given in the changelog). 
> > 
> > We can do that, sure, especially if it makes the tests faster...
> > 
> > > Anytime we use the GPU to invalidate
> > > the frontbuffer tracking, we have to wait for a retire to do the
> > > flush.
> > > Retirement is lazy, and is normally driven by GPU activity but we
> > > have a
> > > background kworker to make sure we notice when the system becomes
> > > idle
> > > independent of userspace - except it's low frequency.
> > 
> > ... but our current 2s timeout should have been enough for that,
> > shouldn't it? If I'm looking at the right part of the code,
> > retirement
> > should be once per second, so 2s should have been enough. But it
> > looks
> > like it's not enough
> > 
> > Unless I'm misinterpreting the round_up part, which could convert
> > the
> > 1s to 2s, which would still probably be fine...
> 
> It can bump the wait by upto a second (it tries to align wakeups on
> second boundaries). And we may skip the work if the device is busy
> elsewhere.

Okay, so you're saying that there's no amount of seconds we can wait
that will guarantee the retire handler will run, even in IGT's 

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[Paulo Zanoni]

Em Seg, 2017-09-04 às 10:00 +0200, Daniel Vetter escreveu:
> On Fri, Sep 01, 2017 at 04:44:38PM -0300, Paulo Zanoni wrote:
> > Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> > > Macros that should be C functions but aren't are really hard to
> > > read and confusing. Convert them over.
> > > 
> > > v2: Clean up commit message and keep printing the line numbers
> > > (Paulo).
> > > 
> > > v3: Actually git add (silly me).
> > > 
> > > Cc: Paulo Zanoni 
> > > Signed-off-by: Daniel Vetter 
> > 
> > Thanks for doing that. Works for me this way:
> > Reviewed-by: Paulo Zanoni 
> 
> Thanks, pushed.
> 
> > But I'm still waiting for the patch that removes those bogus line
> > numbers in every error message we print :).
> 
> Hm, which bogus line numbers where?

Every igt_log message is starts with:
(kms_frontbuffer_tracking:) where  is a number that means
nothing but looks like a line number, except that that line usually has
nothing to do with the message.

> -Daniel
> > 
> > > ---
> > >  tests/kms_frontbuffer_tracking.c | 171 -
> > > 
> > > --
> > >  1 file changed, 89 insertions(+), 82 deletions(-)
> > > 
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c
> > > index e03524f1c45b..a068c8afb6d8 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -1677,88 +1677,95 @@ static int adjust_assertion_flags(const
> > > struct test_mode *t, int flags)
> > >   return flags;
> > >  }
> > >  
> > > -#define do_crc_assertions(flags, mandatory_sink_crc) do {
> > >   
> > > \
> > > - int flags__ = (flags);  
> > >   \
> > > - struct both_crcs crc_;  
> > >   \
> > > - 
> > > \
> > > - if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))  
> > >   
> > > \
> > > - break;  
> > >   \
> > > - 
> > > \
> > > - collect_crcs(_, mandatory_sink_crc);
> > > \
> > > - print_crc("Calculated CRC:", _);
> > >   
> > > \
> > > - 
> > > \
> > > - igt_assert(wanted_crc); 
> > >   \
> > > - igt_assert_crc_equal(_.pipe, _crc->pipe);
> > >   
> > > \
> > > - if (mandatory_sink_crc) 
> > >   \
> > > - assert_sink_crc_equal(_.sink, _crc-
> > > > sink);  \
> > > 
> > > - else if (sink_crc.reliable &&   
> > >   \
> > > -  !sink_crc_equal(_.sink, _crc->sink))
> > >   
> > > \
> > > - igt_info("Sink CRC differ, but not
> > > required\n"); 
> > > \
> > > -} while (0)
> > > -
> > > -#define do_status_assertions(flags_) do {
> > >   
> > > \
> > > - if (!opt.check_status) {
> > > \
> > > - /* Make sure we settle before continuing. */
> > >   
> > > \
> > > - sleep(1);   
> > >   
> > > \
> > > - break;  
> > >   \
> > > - }   
> > >   
> > > \
> > > - 
> > > \
> > > - if (flags_ & ASSERT_FBC_ENABLED) {  
> > >   
> > > \
> > > - igt_require(!fbc_not_enough_stolen());  
> > >   \
> > > - igt_require(!fbc_stride_not_supported());   
> > >   
> > > \
> > > - if (!fbc_wait_until_enabled()) {
> > > \
> > > - fbc_print_status(); 
> > >   
> > > \
> > > - igt_assert_f(false, "FBC disabled\n");  
> > >   \
> > > - }   
> > >   
> > > \
> > > - 
> > > \
> > > - if (opt.fbc_check_compression)  
> > >   \
> > > - igt_assert(fbc_wait_for_compression()); 
> > >   \
> > > - } else if (flags_ & ASSERT_FBC_DISABLED) {  
> > >   
> > > \
> > > - igt_assert(!fbc_wait_until_enabled());  
> > >   \
> > > - }   
> > >   
> > > \
> > > - 
> > > \
> > > - if (flags_ & ASSERT_PSR_ENABLED) {  
> > >   
> > > \
> > > - if (!psr_wait_until_enabled()) {
> > > \
> > > - psr_print_status(); 
> > >   
> > > \
> > > - igt_assert_f(false, "PSR disabled\n");  
> > >   \
> > > - }   

Re: [Intel-gfx] [PATCH] drm/i915/cnl: WaFbcSkipSegments

[Paulo Zanoni]

Em Sex, 2017-09-01 às 14:31 -0700, Rodrigo Vivi escreveu:
> On Fri, Sep 1, 2017 at 2:04 PM, Paulo Zanoni  m> wrote:
> > Em Ter, 2017-08-29 às 16:08 -0700, Rodrigo Vivi escreveu:
> > > Skip compressing 1 segment at the end of the frame,
> > > avoid a pixel count mismatch nuke event when last active
> > > pixel and dummy pixel has same color for Odd Plane
> > > Width / Height.
> > > 
> > > Cc: Paulo Zanoni 
> > > Signed-off-by: Rodrigo Vivi 
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > >  drivers/gpu/drm/i915/intel_pm.c | 4 
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index e2908ae34004..0072ef79bf34 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2939,6 +2939,8 @@ enum i915_power_well_id {
> > >  #define ILK_DPFC_CHICKEN _MMIO(0x43224)
> > >  #define   ILK_DPFC_DISABLE_DUMMY0 (1<<8)
> > >  #define   ILK_DPFC_NUKE_ON_ANY_MODIFICATION  (1<<23)
> > > +#define   CNL_SKIP_SEG_EN(1<<12)
> > > +#define   CNL_SKIP_SEG_COUNT (1<<10)
> > 
> > #define CNL_SKIP_SEG_COUNT_MASK (3 << 10)
> > #define CNL_SKIP_SEG_COUNT(x) ((x) << 10)
> 
> By the definition we should add the SHIFT then as well?
> Although we are not going to read ever?!

I don't see the point in adding it if we're not using it. Most of the
times where we have the function-like macro we don't need _SHIFT, but
_MASK is needed due to the RMW.

> 
> > 
> > Also, since you're going to submit v2 anyway, I wouldn't complain
> > if
> > you drive-by fixed the definitions of ILK_DPFC_CHICKEN bits to use
> > spaces as documented in the beginning of the file. If we fix every
> > time
> > we touch a reg, we may at some point be consistent :).
> 
> Last patch I tried to fix the style on the same patch Jani nacked it.
> So I wonder if we need a bit patch to fix all the style entirely.

I just gave a suggestion, fixing the spacing is not required in this
patch.

> 
> > 
> > 
> > >  #define ILK_FBC_RT_BASE  _MMIO(0x2128)
> > >  #define   ILK_FBC_RT_VALID   (1<<0)
> > >  #define   SNB_FBC_FRONT_BUFFER   (1<<1)
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index d5ff0b9f999f..acf793256507 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -8283,6 +8283,10 @@ static void
> > > cannonlake_init_clock_gating(struct drm_i915_private *dev_priv)
> > >   I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE,
> > >  I915_READ(SLICE_UNIT_LEVEL_CLKGATE) |
> > >  SARBUNIT_CLKGATE_DIS);
> > > +
> > > + /* WaFbcSkipSegments:cnl */
> > 
> > Since this is also documented in BSpec but it doesn't include the
> > WA
> > name, can you please also document that this is WA #1133 in this
> > comment? I'd do something like:
> > 
> > /* Display WA #1133: WaFbcSkipSegments:cnl,glk. */
> 
> good idea!
> 
> > 
> > Please notice that we also need to apply this WA for GLK.
> 
> hm... indeed...
> > 
> > 
> > > + I915_WRITE(ILK_DPFC_CHICKEN, I915_READ(ILK_DPFC_CHICKEN) |
> > > +CNL_SKIP_SEG_EN | CNL_SKIP_SEG_COUNT);
> > 
> > Need to clear bit 11 too.
> 
> ouch! yeap!
> 
> Thanks,
> Rodrigo.
> 
> > 
> > val = READ();
> > val &= ~CNL_SKIP_SEG_COUNT_MASK;
> > val |= CNL_SKIP_SEG_EN | CNL_SKIP_SEG_COUNT(1);
> > 
> > >  }
> > > 
> > >  static void kabylake_init_clock_gating(struct drm_i915_private
> > > *dev_priv)
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[kbuild test robot]

Hi Maarten,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20170904]
[cannot apply to v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Move-drm_crtc_commit-to-drm_crtc_state-v4/20170905-011023
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x016-201736 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/drm_atomic_helper.c: In function 
'drm_atomic_helper_commit_hw_done':
>> drivers/gpu//drm/drm_atomic_helper.c:1850:26: error: void value not ignored 
>> as it ought to be
  old_crtc_state->commit = drm_crtc_commit_get(commit);
 ^

vim +1850 drivers/gpu//drm/drm_atomic_helper.c

  1814  
  1815  /**
  1816   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
  1817   * @old_state: atomic state object with old state structures
  1818   *
  1819   * This function is used to signal completion of the hardware commit 
step. After
  1820   * this step the driver is not allowed to read or change any permanent 
software
  1821   * or hardware modeset state. The only exception is state protected by 
other
  1822   * means than _modeset_lock locks.
  1823   *
  1824   * Drivers should try to postpone any expensive or delayed cleanup work 
after
  1825   * this function is called.
  1826   *
  1827   * This is part of the atomic helper support for nonblocking commits, 
see
  1828   * drm_atomic_helper_setup_commit() for an overview.
  1829   */
  1830  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state 
*old_state)
  1831  {
  1832  struct drm_crtc *crtc;
  1833  struct drm_crtc_state *old_crtc_state, *new_crtc_state;
  1834  struct drm_crtc_commit *commit;
  1835  int i;
  1836  
  1837  for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, 
new_crtc_state, i) {
  1838  commit = new_crtc_state->commit;
  1839  if (!commit)
  1840  continue;
  1841  
  1842  /*
  1843   * copy new_crtc_state->commit to 
old_crtc_state->commit,
  1844   * it's unsafe to touch new_crtc_state after hw_done,
  1845   * but we still need to do so in cleanup_done().
  1846   */
  1847  if (old_crtc_state->commit)
  1848  drm_crtc_commit_put(old_crtc_state->commit);
  1849  
> 1850  old_crtc_state->commit = drm_crtc_commit_get(commit);
  1851  
  1852  /* backend must have consumed any event by now */
  1853  WARN_ON(new_crtc_state->event);
  1854  complete_all(>hw_done);
  1855  }
  1856  }
  1857  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
  1858  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Wake up the device for the fbdev setup

[Ville Syrjälä]

On Sun, Sep 03, 2017 at 01:18:40PM +0100, Chris Wilson wrote:
> Quoting ville.syrj...@linux.intel.com (2017-09-01 20:54:56)
> > From: Ville Syrjälä 
> > 
> > Our fbdev setup requires the device to be awake for access
> > through the GTT. If one boots without connected displays and
> > later plugs one in, we won't have any runtime PM references when
> > the fbdev setup runs. Explicitly grab a runtime PM reference during
> > the fbdev setup to avoid the following spew:
> > 
> > [   62.518435] RPM wakelock ref not held during HW access
> > [   62.518459] [ cut here ]
> > [   62.518546] WARNING: CPU: 3 PID: 37 at 
> > ../drivers/gpu/drm/i915/intel_drv.h:1800 i915_vma_pin_iomap+0x144/0x150 
> > [i915]
> > [   62.518585] Modules linked in: i915 i2c_algo_bit drm_kms_helper 
> > syscopyarea sysfillrect sysimgblt fb_sys_fops drm intel_gtt agpgart 
> > netconsole nls_iso8859_1 nls_cp437 vfat fat efi_pstore coretemp hwmon 
> > intel_rapl x86_pkg_temp_thermal e1000e efivars ptp pps_core video evdev 
> > ip_tables x_tables ipv6 autofs4
> > [   62.518741] CPU: 3 PID: 37 Comm: kworker/3:1 Not tainted 4.13.0-rc7-skl+ 
> > #1077
> > [   62.518770] Hardware name:  /NUC7i5BNB, BIOS 
> > BNKBL357.86A.0048.2017.0704.1415 07/04/2017
> > [   62.518827] Workqueue: events i915_hotplug_work_func [i915]
> > [   62.518853] task: 88046c00dc00 task.stack: c9184000
> > [   62.518896] RIP: 0010:i915_vma_pin_iomap+0x144/0x150 [i915]
> > [   62.518919] RSP: 0018:c9187cc8 EFLAGS: 00010292
> > [   62.518942] RAX: 002a RBX: 880460044000 RCX: 
> > 0006
> > [   62.518969] RDX: 0006 RSI: 819c3e6f RDI: 
> > 819f1c0e
> > [   62.518996] RBP: c9187cd8 R08: 88046c00e4f0 R09: 
> > 
> > [   62.519022] R10: 8804669ca800 R11:  R12: 
> > 880461d2
> > [   62.519049] R13: c9187d48 R14: 880461d2 R15: 
> > 880460044000
> > [   62.519076] FS:  () GS:88047ed8() 
> > knlGS:
> > [   62.519107] CS:  0010 DS:  ES:  CR0: 80050033
> > [   62.519130] CR2: 56478ae213f0 CR3: 02c0f000 CR4: 
> > 003406e0
> > [   62.519156] Call Trace:
> > [   62.519190]  intelfb_create+0x176/0x360 [i915]
> > [   62.519216]  __drm_fb_helper_initial_config_and_unlock+0x1c7/0x3c0 
> > [drm_kms_helper]
> > [   62.519251]  drm_fb_helper_hotplug_event.part.18+0xac/0xc0 
> > [drm_kms_helper]
> > [   62.519282]  drm_fb_helper_hotplug_event+0x1a/0x20 [drm_kms_helper]
> > [   62.519324]  intel_fbdev_output_poll_changed+0x1a/0x20 [i915]
> > [   62.519352]  drm_kms_helper_hotplug_event+0x27/0x30 [drm_kms_helper]
> > [   62.519395]  i915_hotplug_work_func+0x24e/0x2b0 [i915]
> > [   62.519420]  process_one_work+0x1d3/0x6d0
> > [   62.519440]  worker_thread+0x4b/0x400
> > [   62.519458]  ? schedule+0x4a/0x90
> > [   62.519475]  ? preempt_count_sub+0x97/0xf0
> > [   62.519495]  kthread+0x114/0x150
> > [   62.519511]  ? process_one_work+0x6d0/0x6d0
> > [   62.519530]  ? kthread_create_on_node+0x40/0x40
> > [   62.519551]  ret_from_fork+0x27/0x40
> > [   62.519569] Code: c4 78 e6 e0 0f ff e9 08 ff ff ff 80 3d d5 bc 0c 00 00 
> > 0f 85 0b ff ff ff 48 c7 c7 d8 50 32 a0 c6 05 c1 bc 0c 00 01 e8 9d 78 e6 e0 
> > <0f> ff e9 f1 fe ff ff 0f 1f 44 00 00 0f 1f 44 00 00 0f b6 87 98
> > [   62.519771] ---[ end trace 5fbe271f991a58ae ]---
> > 
> > Signed-off-by: Ville Syrjälä 
> 
> Ah right, the current code only works because it assumes the fbcon is
> active on the display (thus holding a wakeref) anytime it writes through
> the fbdev's GGTT iomapping. This is one instance where we need GGTT access
> before the fbcon controls the display, hence needs an explicit wakeref.
> 
> intel_fbdev_set_suspend() is also outside of the fbcon control, I
> presume that (memset_io) works because we hold a wakeref whilst doing
> suspend/resume.
> 
> Reviewed-by: Chris Wilson 

Thanks. Pushed to dinq.

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

Re: [Intel-gfx] [PATCH 2/2] drm/i915: io unmap functions want __iomem

[Ville Syrjälä]

On Fri, Sep 01, 2017 at 07:53:05PM +0100, Chris Wilson wrote:
> Quoting ville.syrj...@linux.intel.com (2017-09-01 18:12:52)
> > From: Ville Syrjälä 
> > 
> > Don't cast away the __iomem from the io_mapping functions so that
> > sparse won't be so unhappy when we pass the pointer to the unmap
> > functions. Instead let's move the cast to where we actually use the
> > pointer.
> > 
> > Fixes the following sparse warnings:
> > i915_gem.c:1022:33: warning: incorrect type in argument 1 (different 
> > address spaces)
> > i915_gem.c:1022:33:expected void [noderef] *vaddr
> > i915_gem.c:1022:33:got void *[assigned] vaddr
> > i915_gem.c:1027:34: warning: incorrect type in argument 1 (different 
> > address spaces)
> > i915_gem.c:1027:34:expected void [noderef] *vaddr
> > i915_gem.c:1027:34:got void *[assigned] vaddr
> > i915_gem.c:1199:33: warning: incorrect type in argument 1 (different 
> > address spaces)
> > i915_gem.c:1199:33:expected void [noderef] *vaddr
> > i915_gem.c:1199:33:got void *[assigned] vaddr
> > i915_gem.c:1204:34: warning: incorrect type in argument 1 (different 
> > address spaces)
> > i915_gem.c:1204:34:expected void [noderef] *vaddr
> > i915_gem.c:1204:34:got void *[assigned] vaddr
> > 
> > Cc: Chris Wilson 
> > Signed-off-by: Ville Syrjälä 
> 
> 6 of one, half a dozen of the other. We need to do the __force cast
> somewhere, so whichever is more convenient.
> Reviewed-by: Chris Wilson 

Thanks. Series pushed to dinq.

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

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Ville Syrjälä]

On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> succeeds.

I don't think we actually want to reject overlap. I had a patch for that
years ago, but I decided to drop it because people might want to
interleave the planes in some interesting ways. Making the overlap
check accurate enough to allow that would be to total overkill. So IMO
it's perfectly fine to let the user shoot himself in the foot if they
mess up the offsets.

> 
> Signed-off-by: Gabriel Krisman Bertazi 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b28f076f98bc..ff1ed67a9eff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
> intel_framebuffer *intel_fb,
>   DRM_DEBUG_KMS("RC supported only with RGB 
> formats\n");
>   goto err;
>   }
> +
> + if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
> + DRM_DEBUG_KMS("CCS and color buffers overlap\n");
> + return -EINVAL;
> + }
>   /* fall through */
>   case I915_FORMAT_MOD_Y_TILED:
>   case I915_FORMAT_MOD_Yf_TILED:
> -- 
> 2.11.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL   : https://patchwork.freedesktop.org/series/29538/
State : failure

== Summary ==

Test kms_atomic_transition:
Subgroup plane-use-after-nonblocking-unbind-fencing:
fail   -> PASS   (shard-hsw) fdo#101847 +1
Test gem_sync:
Subgroup basic-many-each:
pass   -> FAIL   (shard-hsw)
Test perf:
Subgroup blocking:
pass   -> FAIL   (shard-hsw) fdo#102252
Test kms_setmode:
Subgroup basic:
fail   -> PASS   (shard-hsw) fdo#99912
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass   -> DMESG-WARN (shard-hsw)

fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hswtotal:2265 pass:1232 dwarn:2   dfail:0   fail:15  skip:1016 
time:9556s

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5575/shards.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/4] RFC: drm: Allow driver-specific ioctls to be registered

[marius vlad]

Indeed, we argued at first to let the driver handle the ioctls directly,
but we would like to use the DRM interface if possible.

On Mon, Sep 4, 2017 at 6:26 PM, Chris Wilson 
wrote:

> Quoting Marius Vlad (2017-09-04 16:16:41)
> > From: Marius Vlad 
> >
> > Currently driver-specific ioctls have to be declared static and are
> confined to
> > DRM core driver. This patch series provides the means to remove those
> constrains
> > and allow to register driver-specific ioctls dynamically by keeping a
> list of
> > registered ioctls in struct drm_driver, then each component of the
> driver can
> > then register its own specific ioctls using this interface.
> >
> > The driver must assign ioctl_register/ioctl_deregister in
> > its drm_driver structure in order to make use of it.
> >
> > While SoC drivers benefit the most from this approach (by not polluting
> DRM core
> > driver and allowing sub drivers to implement and register driver-specific
> > ioctls dynamically), further patches shows how easy is to convert
> drm/i915 to
> > this approach by registering GEM and perf ioctls separately.
>
> Why?
>
> You do not have to use drm_ioctl directly... Avoiding it would reduce
> our ioctl overhead considerably, for example reducing busy_ioctl from
> around 110ns to around 45ns.
> -Chris
>



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

Re: [Intel-gfx] [PATCH 0/4] RFC: drm: Allow driver-specific ioctls to be registered

[marius vlad]

There isn't any dark plot behind it.

For instance, in our use case, a DPU (Display Process Unit) which has a
blit feature (using DRM_RENDER_ALLOW) can be implemented cleanly
in a separate driver and not being dependent on the DRM core driver. If the
blit
feature is present/enabled, we can dynamically register the ioctls at
run-time.

There are other means to mitigate this, but we thought this would
beneficial
to other drivers as well.

Other SoC drivers like Exynos (G2D) provide this feature by inventing it's
own sub-driver
system/layer and have all the sub-drivers built-in.


On Mon, Sep 4, 2017 at 6:25 PM, Daniel Vetter  wrote:

> On Mon, Sep 04, 2017 at 06:16:41PM +0300, Marius Vlad wrote:
> > From: Marius Vlad 
> >
> > Currently driver-specific ioctls have to be declared static and are
> confined to
> > DRM core driver. This patch series provides the means to remove those
> constrains
> > and allow to register driver-specific ioctls dynamically by keeping a
> list of
> > registered ioctls in struct drm_driver, then each component of the
> driver can
> > then register its own specific ioctls using this interface.
> >
> > The driver must assign ioctl_register/ioctl_deregister in
> > its drm_driver structure in order to make use of it.
> >
> > While SoC drivers benefit the most from this approach (by not polluting
> DRM core
> > driver and allowing sub drivers to implement and register driver-specific
> > ioctls dynamically), further patches shows how easy is to convert
> drm/i915 to
> > this approach by registering GEM and perf ioctls separately.
>
> What exactly is the problem you're trying to solve?
>
> This awefully smells like some neat way to make loading driver modules for
> blob userspace easy ... And I can't think of any other thing you could use
> this for.
>
> And even for the blob userspace use case: Create a separate drm driver
> instance, share buffers and fences with dma_buf and dma_fence, and you're
> all good. I really have no idea what this is good for, but maybe I'm
> missing something?
> -Daniel
>
> >
> > Marius Vlad (4):
> >   drm/gpu: Support registering driver-specific ioctls dynamically
> >   drm/i915: Convert i915 to use ioctl_register/ioctl_deregister.
> >   drm/i915: Register perf_ ioctls directly in i915_perf file.
> >   drm/i915: Register GEM ioctls directly in i915_gem file.
> >
> >  drivers/gpu/drm/drm_drv.c|   1 +
> >  drivers/gpu/drm/drm_ioctl.c  |  99 ++
> --
> >  drivers/gpu/drm/i915/i915_drv.c  | 107 +++---
> -
> >  drivers/gpu/drm/i915/i915_gem.c  |  52 +++
> >  drivers/gpu/drm/i915/i915_perf.c |  21 
> >  include/drm/drm_drv.h|  34 +
> >  include/drm/drm_ioctl.h  |   6 +++
> >  7 files changed, 249 insertions(+), 71 deletions(-)
> >
> > --
> > 2.9.3
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>



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

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"

[Ville Syrjälä]

On Mon, Sep 04, 2017 at 12:39:25PM +0200, Maarten Lankhorst wrote:
> Op 31-08-17 om 20:48 schreef Ville Syrjälä:
> > On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrj...@linux.intel.com 
> > wrote:
> >> From: Ville Syrjälä 
> >>
> >> Make the min_pixclk thing less confusing by changing it to track
> >> the minimum acceptable cdclk frequency instead. This means moving
> >> the application of the guardbands to a slightly higher level from
> >> the low level platform specific calc_cdclk() functions.
> >>
> >> The immediate benefit is elimination of the confusing 2x factors
> >> on GLK/CNL+ in the audio workarounds (which stems from the fact
> >> that the pipes produce two pixels per clock).
> >>
> >> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage 
> >> handling
> >> v3: Squash with the CNL cdclk limits patch (DK)
> >> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
> >>
> >> Cc: Paulo Zanoni 
> >> Cc: Rodrigo Vivi 
> >> Cc: Dhinakaran Pandiyan 
> >> Cc: Maarten Lankhorst 
> >> Reviewed-by: Dhinakaran Pandiyan 
> >> Signed-off-by: Ville Syrjälä 
> > I didn't get any objections from the CNL camp, so I went ahead and
> > pushed the series. Thanks for the reviews.
> >
> I seem to have a WARN_ON during init now on my broadwell, likely related to 
> this series?
> 
> [   13.105310] i915 :00:02.0: vgaarb: changed VGA decodes: 
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [   13.132264] systemd-journald[159]: Successfully sent stream file 
> descriptor to service manager.
> [   13.161016] WARN_ON(min_cdclk < 0)

Hmm. I think I need to see the full dmesg to figure this one out.

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

Re: [Intel-gfx] [PATCH v3] drm/i915: Add interface to reserve fence registers for vGPU

[Chris Wilson]

Quoting Zhenyu Wang (2017-09-04 16:04:03)
> On 2017.09.04 11:01:41 +0100, Chris Wilson wrote:
> > Quoting changbin...@intel.com (2017-09-04 09:01:01)
> > > From: Changbin Du 
> > > 
> > > In the past, vGPU alloc fence registers by walking through mm.fence_list
> > > to find fence which pin_count = 0 and vma is empty. vGPU may not find
> > > enough fence registers this way. Because a fence can be bind to vma even
> > > though it is not in using. We have found such failure many times these
> > > days.
> > > 
> > > An option to resolve this issue is that we can force-remove fence from
> > > vma in this case.
> > > 
> > > This patch added two new api to the fence management code:
> > >  - i915_reserve_fence() will try to find a free fence from fence_list
> > >and force-remove vma if need.
> > >  - i915_unreserve_fence() reclaim a reserved fence after vGPU has
> > >finished.
> > > 
> > > With this change, the fence management is more clear to work with vGPU.
> > > GVTg do not need remove fence from fence_list in private.
> > > 
> > > v3: (Chris)
> > >   - Add struct_mutex lock assertion.
> > >   - Only count for unpinned fence.
> > > 
> > > v2: (Chris)
> > >   - Rename the new api for symmetry.
> > >   - Add safeguard to ensure at least 1 fence remained for host display.
> > > 
> > > Signed-off-by: Changbin Du 
> > > Reviewed-by: Chris Wilson 
> > 
> > Anyone want to give an ack/review for the GVT side, and then I'll push.
> 
> Acked-by: Zhenyu Wang 

And pushed.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/4] RFC: drm: Allow driver-specific ioctls to be registered

[Chris Wilson]

Quoting Marius Vlad (2017-09-04 16:16:41)
> From: Marius Vlad 
> 
> Currently driver-specific ioctls have to be declared static and are confined 
> to
> DRM core driver. This patch series provides the means to remove those 
> constrains
> and allow to register driver-specific ioctls dynamically by keeping a list of
> registered ioctls in struct drm_driver, then each component of the driver can
> then register its own specific ioctls using this interface.
> 
> The driver must assign ioctl_register/ioctl_deregister in
> its drm_driver structure in order to make use of it. 
> 
> While SoC drivers benefit the most from this approach (by not polluting DRM 
> core
> driver and allowing sub drivers to implement and register driver-specific 
> ioctls dynamically), further patches shows how easy is to convert drm/i915 to 
> this approach by registering GEM and perf ioctls separately.

Why?

You do not have to use drm_ioctl directly... Avoiding it would reduce
our ioctl overhead considerably, for example reducing busy_ioctl from
around 110ns to around 45ns.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/4] RFC: drm: Allow driver-specific ioctls to be registered

[Daniel Vetter]

On Mon, Sep 04, 2017 at 06:16:41PM +0300, Marius Vlad wrote:
> From: Marius Vlad 
> 
> Currently driver-specific ioctls have to be declared static and are confined 
> to
> DRM core driver. This patch series provides the means to remove those 
> constrains
> and allow to register driver-specific ioctls dynamically by keeping a list of
> registered ioctls in struct drm_driver, then each component of the driver can
> then register its own specific ioctls using this interface.
> 
> The driver must assign ioctl_register/ioctl_deregister in
> its drm_driver structure in order to make use of it. 
> 
> While SoC drivers benefit the most from this approach (by not polluting DRM 
> core
> driver and allowing sub drivers to implement and register driver-specific 
> ioctls dynamically), further patches shows how easy is to convert drm/i915 to 
> this approach by registering GEM and perf ioctls separately.

What exactly is the problem you're trying to solve?

This awefully smells like some neat way to make loading driver modules for
blob userspace easy ... And I can't think of any other thing you could use
this for.

And even for the blob userspace use case: Create a separate drm driver
instance, share buffers and fences with dma_buf and dma_fence, and you're
all good. I really have no idea what this is good for, but maybe I'm
missing something?
-Daniel

> 
> Marius Vlad (4):
>   drm/gpu: Support registering driver-specific ioctls dynamically
>   drm/i915: Convert i915 to use ioctl_register/ioctl_deregister.
>   drm/i915: Register perf_ ioctls directly in i915_perf file.
>   drm/i915: Register GEM ioctls directly in i915_gem file.
> 
>  drivers/gpu/drm/drm_drv.c|   1 +
>  drivers/gpu/drm/drm_ioctl.c  |  99 ++--
>  drivers/gpu/drm/i915/i915_drv.c  | 107 
> +++
>  drivers/gpu/drm/i915/i915_gem.c  |  52 +++
>  drivers/gpu/drm/i915/i915_perf.c |  21 
>  include/drm/drm_drv.h|  34 +
>  include/drm/drm_ioctl.h  |   6 +++
>  7 files changed, 249 insertions(+), 71 deletions(-)
> 
> -- 
> 2.9.3
> 

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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL   : https://patchwork.freedesktop.org/series/29538/
State : success

== Summary ==

Series 29538v3 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/3/mbox/

Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail   -> PASS   (fi-snb-2600) fdo#17
Test kms_flip:
Subgroup basic-flip-vs-modeset:
skip   -> PASS   (fi-skl-x1585l) fdo#101781
Test drv_module_reload:
Subgroup basic-reload:
dmesg-warn -> INCOMPLETE (fi-cfl-s) k.org#196765

fdo#17 https://bugs.freedesktop.org/show_bug.cgi?id=17
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
k.org#196765 https://bugzilla.kernel.org/show_bug.cgi?id=196765

fi-bdw-5557u total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:458s
fi-bdw-gvtdvmtotal:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:444s
fi-blb-e6850 total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  
time:358s
fi-bsw-n3050 total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  
time:563s
fi-bwr-2160  total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 
time:253s
fi-bxt-j4205 total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:517s
fi-byt-j1900 total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  
time:526s
fi-byt-n2820 total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  
time:514s
fi-cfl-s total:285  pass:250  dwarn:1   dfail:0   fail:0   skip:33 
fi-elk-e7500 total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  
time:440s
fi-glk-2atotal:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:612s
fi-hsw-4770  total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:444s
fi-hsw-4770r total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:426s
fi-ilk-650   total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  
time:424s
fi-ivb-3520m total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:506s
fi-ivb-3770  total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:482s
fi-kbl-7500u total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  
time:510s
fi-kbl-7560u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:595s
fi-kbl-r total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:596s
fi-pnv-d510  total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  
time:537s
fi-skl-6260u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:473s
fi-skl-6700k total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:538s
fi-skl-6770hqtotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:513s
fi-skl-gvtdvmtotal:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  
time:446s
fi-skl-x1585ltotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:507s
fi-snb-2520m total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  
time:551s
fi-snb-2600  total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  
time:406s

59fbfaa48e1837b7a692df34ce6696eb4035e7a8 drm-tip: 2017y-09m-04d-11h-39m-37s UTC 
integration manifest
b307bc7d0cf6 drm/atomic: Make async plane update checks work as intended, v2.
e23ef2074a02 drm/atomic: Fix freeing connector/plane state too early by 
tracking commits, v3.
08156193021b drm/atomic: Return commit in drm_crtc_commit_get for better 
annotation
5d5cf3601e90 drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, 
v2.
e283a7f56851 drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
cdfa52e3441f drm/i915: Always wait for flip_done, v2.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5575/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Add interface to reserve fence registers for vGPU

[Zhenyu Wang]

On 2017.09.04 11:01:41 +0100, Chris Wilson wrote:
> Quoting changbin...@intel.com (2017-09-04 09:01:01)
> > From: Changbin Du 
> > 
> > In the past, vGPU alloc fence registers by walking through mm.fence_list
> > to find fence which pin_count = 0 and vma is empty. vGPU may not find
> > enough fence registers this way. Because a fence can be bind to vma even
> > though it is not in using. We have found such failure many times these
> > days.
> > 
> > An option to resolve this issue is that we can force-remove fence from
> > vma in this case.
> > 
> > This patch added two new api to the fence management code:
> >  - i915_reserve_fence() will try to find a free fence from fence_list
> >and force-remove vma if need.
> >  - i915_unreserve_fence() reclaim a reserved fence after vGPU has
> >finished.
> > 
> > With this change, the fence management is more clear to work with vGPU.
> > GVTg do not need remove fence from fence_list in private.
> > 
> > v3: (Chris)
> >   - Add struct_mutex lock assertion.
> >   - Only count for unpinned fence.
> > 
> > v2: (Chris)
> >   - Rename the new api for symmetry.
> >   - Add safeguard to ensure at least 1 fence remained for host display.
> > 
> > Signed-off-by: Changbin Du 
> > Reviewed-by: Chris Wilson 
> 
> Anyone want to give an ack/review for the GVT side, and then I'll push.

Acked-by: Zhenyu Wang 

thanks
-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[Maarten Lankhorst]

Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)

The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.

Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
- Remove drm_atomic_helper_async_check hunk. (pinchartl)
Changes since v3:
- Fix use-after-free in drm_atomic_helper_commit_cleanup_done().

Signed-off-by: Maarten Lankhorst 
Reviewed-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_atomic.c|  7 
 drivers/gpu/drm/drm_atomic_helper.c | 82 -
 include/drm/drm_atomic.h|  1 -
 include/drm/drm_crtc.h  | 23 +--
 4 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
crtc->funcs->atomic_destroy_state(crtc,
  state->crtcs[i].state);
 
-   if (state->crtcs[i].commit) {
-   kfree(state->crtcs[i].commit->event);
-   state->crtcs[i].commit->event = NULL;
-   drm_crtc_commit_put(state->crtcs[i].commit);
-   }
-
-   state->crtcs[i].commit = NULL;
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..80c138cbde9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
  struct drm_atomic_state *old_state)
 {
-   struct drm_crtc_state *unused;
+   struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
int i;
 
-   for_each_new_crtc_in_state(old_state, crtc, unused, i) {
-   struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+   for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+   struct drm_crtc_commit *commit = new_crtc_state->commit;
int ret;
 
if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
kref_init(>ref);
commit->crtc = crtc;
 
-   state->crtcs[i].commit = commit;
+   new_crtc_state->commit = commit;
 
ret = stall_checks(crtc, nonblock);
if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
-   struct drm_crtc_commit *commit;
-   int i = 0;
-
-   list_for_each_entry(commit, >commit_list, commit_entry) {
-   /* skip the first entry, that's the current commit */
-   if (i == 1)
-   return commit;
-   i++;
-   }
-
-   return NULL;
-}
-
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding 
commits
  * @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct 
drm_crtc *crtc)
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state 
*old_state)
 {
struct drm_crtc *crtc;
-   struct drm_crtc_state *new_crtc_state;
+   struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
 
-   for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-   spin_lock(>commit_lock);
-   commit = preceeding_commit(crtc);
-   if (commit)
-   drm_crtc_commit_get(commit);
-   spin_unlock(>commit_lock);
+   for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+   commit = old_crtc_state->commit;
 
if (!commit)
continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct 
drm_atomic_state *old_state)
if (ret == 0)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
  crtc->base.id, crtc->name);
-
-   drm_crtc_commit_put(commit);
}
 }
 

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dsi: Send SHUTDOWN only for v3+ VBT's

[Ville Syrjälä]

On Mon, Sep 04, 2017 at 10:59:32AM +0300, Mika Kahola wrote:
> On Fri, 2017-09-01 at 16:43 +0300, Ville Syrjälä wrote:
> > On Fri, Sep 01, 2017 at 10:51:00AM +0300, Mika Kahola wrote:
> > > 
> > > According to spec we should send SHUTDOWN before
> > > MIPI_SEQ_DISPLAY_OFF for
> > > v3+ VBT's. Testing with VBT v3 the current implementation yields
> > > the
> > > following error message
> > > 
> > > *ERROR* Video mode command 0x0041 send failed.
> > > 
> > > To get rid of this error message, let's limit SHUTDOWN only for VBT
> > > versions 3 or higher.
> > In the patch you limit it to version 4+, which doesn't make sense
> > since
> > AFAIK there is no version 4 of the sequence block.
> It seems that sending SHUTDOWN signal doesn't make any sense either.
> Whenever we send that signal it just causes this error message. Do we
> really need to signal this? From functionality point of view there's no
> difference.

Well, the spec doesn't even explain what this "shut down" command does.
Is it actually the DCS "display off" command, or something else?

Did you try reverting bbdf0b2ff32a ("drm/i915/bxt: Disable device ready
before shutdown command")? That looks suspicious to me. But so does
about half of the DSI code since it never seems to follow the spec, and
we end up doing totally different things on different platforms without
any explanation why that is).

> 
> > 
> > > 
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102404
> > > Signed-off-by: Mika Kahola 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dsi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> > > b/drivers/gpu/drm/i915/intel_dsi.c
> > > index 2a0f5d3..b48b9b7 100644
> > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > @@ -916,7 +916,7 @@ static void intel_dsi_disable(struct
> > > intel_encoder *encoder,
> > >    * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field
> > > testing
> > >    * has shown that the v3 sequence works for v2 VBTs too
> > >    */
> > > - if (is_vid_mode(intel_dsi)) {
> > > + if (is_vid_mode(intel_dsi) && dev_priv-
> > > >vbt.dsi.seq_version > 3) {
> > >   /* Send Shutdown command to the panel in LP mode
> > > */
> > >   for_each_dsi_port(port, intel_dsi->ports)
> > >   dpi_send_cmd(intel_dsi, SHUTDOWN, false,
> > > port);
> > > -- 
> > > 2.7.4
> > > 
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -- 
> Mika Kahola - Intel OTC

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

Re: [Intel-gfx] [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin]

On 04/09/2017 15:43, Daniel Vetter wrote:

On Mon, Sep 04, 2017 at 03:36:56PM +0100, Tvrtko Ursulin wrote:


On 04/09/2017 15:27, Tvrtko Ursulin wrote:

On 07/08/2017 16:53, Daniel Vetter wrote:

On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

To the best of my recollection the page flipping test was added
simply to start exercising page flips with 90/270 rotation.

There is no need to do 60 flips which can take quite some time,
because we do 60 flips against each pipe and each fb geometry.

Also, calling this a stress test is also not matching the
original idea of the test.

Several changes:

1. Remove the stress from the name and reduce the number of
flips to one only.

2. Move the page flip before CRC collection for a more useful
test.

3. Add more flipping tests, for different rotation and sprite
planes.


I assume you didn't make the test overall slower with this?


4. Convert to table driven subtest generation.

Signed-off-by: Tvrtko Ursulin 
Cc: Daniel Vetter 
Cc: Chris Wilson 


Didn't do a full review, but sounds all reasonable. And I assume you've
tested this at least locally (the igt patchwork CI instance doesn't do
full runs ... yet).


Yes I've ran it on SKL. It adds more subtests so the runtime is longer,
but it also finds new bugs.


Runtime is longer compared to v1 of this patch BTW, I wasn't clear. It is
roughly on par with the situation without this patch.

But v2 adds a lot more subtests, three of which fail (flip on a rotated
sprite plane). So there is value in it I think even like that.


Failing new subtests is ok imo, but pls do give a heads-up to CI folks
before merging (but I think as long as it's failing stable, it shouldn't
cause noise). Anything that takes out the machine or driver isn't ok, and
needs to be fixed first.

So sounds all good to me to go ahead.


I thought we said full review rules on IGT, no? So I won't be merging 
this until someone can look at it in more detail.


Tvrtko

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

Re: [Intel-gfx] [PATCH i-g-t 12/12] RFC: meson build system support

[Daniel Vetter]

On Mon, Sep 04, 2017 at 04:11:17PM +0300, Jani Nikula wrote:
> On Sat, 02 Sep 2017, Daniel Vetter  wrote:
> > +if cc.has_member('struct sysinfo', 'totalram',
> > +   prefix : '#include ')
> > +   config_h.set('HAVE_STRUCT_SYSINFO_TOTALRAM', 1)
> > +endif
> > +
> > +add_project_arguments('-D_GNU_SOURCE', language : 'c')
> 
> Just something that caught my eye that bit me in the past. The project
> arguments are *not* passed on to feature tests such as
> cc.has_member(). I don't think it matters in this case, but you'll want
> to #define _GNU_SOURCE in the prefix if you're testing for GNU
> stuff. Otherwise you may end up using compat versions. Just a heads up,
> that's all.

Ow, the fun. Well since the only has_member/function check we do have
right now tests for linux vs. solaris, we should probably be safe. Or at
least know really quickly when we're not safe.

For the android stuff, we'll probably just have one switch for is_android,
for all the things missing in bionic. Very obviously I've done none of the
"how should this work on android" thinking yet :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Skip waking the device to service pwrite

[Daniel Vetter]

On Mon, Sep 04, 2017 at 11:18:07AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-09-04 09:12:12)
> > On Wed, Aug 30, 2017 at 06:48:19PM +0100, Chris Wilson wrote:
> > > If the device is in runtime suspend, resuming takes time and reduces our
> > > powersaving. If this was for a small write into an object, that resume
> > > will take longer than any savings in using the indirect GGTT access to
> > > avoid the cpu cache.
> > > 
> > > Signed-off-by: Chris Wilson 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 21 ++---
> > >  1 file changed, 18 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index 93dfa793975a..8940a6873ca5 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1229,7 +1229,21 @@ i915_gem_gtt_pwrite_fast(struct 
> > > drm_i915_gem_object *obj,
> > >   if (ret)
> > >   return ret;
> > >  
> > > - intel_runtime_pm_get(i915);
> > > + if (i915_gem_object_has_struct_page(obj)) {
> > 
> > I don't really see why we need to check for has_struct_page here (we do
> > already outside the lock grabbing), and why if that's not the case we hit
> > the slow-path?
> 
> We can't use the alternate paths if we don't have struct_page, hence we
> have to force use of GTT if !i915_gem_object_has_struct_page. The
> previous test is to also make sure we come down this path and not fail.

Ow, I've entirely misread all the code, I thought this checks for
obj->pages. With clue gained on my side:

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

Re: [Intel-gfx] [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Daniel Vetter]

On Mon, Sep 04, 2017 at 03:36:56PM +0100, Tvrtko Ursulin wrote:
> 
> On 04/09/2017 15:27, Tvrtko Ursulin wrote:
> > On 07/08/2017 16:53, Daniel Vetter wrote:
> > > On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:
> > > > From: Tvrtko Ursulin 
> > > > 
> > > > To the best of my recollection the page flipping test was added
> > > > simply to start exercising page flips with 90/270 rotation.
> > > > 
> > > > There is no need to do 60 flips which can take quite some time,
> > > > because we do 60 flips against each pipe and each fb geometry.
> > > > 
> > > > Also, calling this a stress test is also not matching the
> > > > original idea of the test.
> > > > 
> > > > Several changes:
> > > > 
> > > > 1. Remove the stress from the name and reduce the number of
> > > > flips to one only.
> > > > 
> > > > 2. Move the page flip before CRC collection for a more useful
> > > > test.
> > > > 
> > > > 3. Add more flipping tests, for different rotation and sprite
> > > > planes.
> > > 
> > > I assume you didn't make the test overall slower with this?
> > > 
> > > > 4. Convert to table driven subtest generation.
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin 
> > > > Cc: Daniel Vetter 
> > > > Cc: Chris Wilson 
> > > 
> > > Didn't do a full review, but sounds all reasonable. And I assume you've
> > > tested this at least locally (the igt patchwork CI instance doesn't do
> > > full runs ... yet).
> > 
> > Yes I've ran it on SKL. It adds more subtests so the runtime is longer,
> > but it also finds new bugs.
> 
> Runtime is longer compared to v1 of this patch BTW, I wasn't clear. It is
> roughly on par with the situation without this patch.
> 
> But v2 adds a lot more subtests, three of which fail (flip on a rotated
> sprite plane). So there is value in it I think even like that.

Failing new subtests is ok imo, but pls do give a heads-up to CI folks
before merging (but I think as long as it's failing stable, it shouldn't
cause noise). Anything that takes out the machine or driver isn't ok, and
needs to be fixed first.

So sounds all good to me to go ahead.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin]

On 04/09/2017 15:27, Tvrtko Ursulin wrote:

On 07/08/2017 16:53, Daniel Vetter wrote:

On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

To the best of my recollection the page flipping test was added
simply to start exercising page flips with 90/270 rotation.

There is no need to do 60 flips which can take quite some time,
because we do 60 flips against each pipe and each fb geometry.

Also, calling this a stress test is also not matching the
original idea of the test.

Several changes:

1. Remove the stress from the name and reduce the number of
flips to one only.

2. Move the page flip before CRC collection for a more useful
test.

3. Add more flipping tests, for different rotation and sprite
planes.


I assume you didn't make the test overall slower with this?


4. Convert to table driven subtest generation.

Signed-off-by: Tvrtko Ursulin 
Cc: Daniel Vetter 
Cc: Chris Wilson 


Didn't do a full review, but sounds all reasonable. And I assume you've
tested this at least locally (the igt patchwork CI instance doesn't do
full runs ... yet).


Yes I've ran it on SKL. It adds more subtests so the runtime is longer, 
but it also finds new bugs.


Runtime is longer compared to v1 of this patch BTW, I wasn't clear. It 
is roughly on par with the situation without this patch.


But v2 adds a lot more subtests, three of which fail (flip on a rotated 
sprite plane). So there is value in it I think even like that.


Tvrtko

Ideally someone with display-fu picks this up, fixes the bug(s) this 
exposes (or tells me the test is wrong), so we are not merging known 
failing tests? Or even that is OK?


Tvrtko


Acked-by: Daniel Vetter 

---
  tests/intel-ci/extended.testlist |   2 +-
  tests/kms_rotation_crc.c | 137 
---

  2 files changed, 85 insertions(+), 54 deletions(-)

diff --git a/tests/intel-ci/extended.testlist 
b/tests/intel-ci/extended.testlist

index 17eed013f810..fb71091758c6 100644
--- a/tests/intel-ci/extended.testlist
+++ b/tests/intel-ci/extended.testlist
@@ -82,7 +82,7 @@ igt@gem_ringfill@vebox-bomb
  igt@gem_userptr_blits@stress-mm
  igt@gem_userptr_blits@stress-mm-invalidate-close
  igt@gem_userptr_blits@stress-mm-invalidate-close-overlap
-igt@kms_rotation_crc@primary-rotation-90-flip-stress
+igt@kms_rotation_crc@primary-rotation-90-flip
  igt@pm_rpm@gem-execbuf-stress
  igt@pm_rpm@gem-execbuf-stress-extra-wait
  igt@pm_rpm@gem-execbuf-stress-pc8
diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index 83e37f126f40..20f1adb67769 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -41,7 +41,7 @@ typedef struct {
  int pos_y;
  uint32_t override_fmt;
  uint64_t override_tiling;
-unsigned int flip_stress;
+unsigned int flips;
  } data_t;
  static void
@@ -219,7 +219,7 @@ static void prepare_fbs(data_t *data, 
igt_output_t *output,
  igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, 
>fb);

-if (data->flip_stress) {
+if (data->flips) {
  igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, 
>fb_flip);

  paint_squares(data, IGT_ROTATION_0, >fb_flip, 0.92);
  }
@@ -351,15 +351,17 @@ static void test_plane_rotation(data_t *data, 
int plane_type)

  ret = igt_display_try_commit2(display, commit);
  if (data->override_fmt || data->override_tiling) {
  igt_assert_eq(ret, -EINVAL);
-} else {
-igt_assert_eq(ret, 0);
-igt_pipe_crc_collect_crc(data->pipe_crc,
-  _output);
-igt_assert_crc_equal(>ref_crc,
-  _output);
+continue;
  }
-flip_count = data->flip_stress;
+/* Verify commit was ok. */
+igt_assert_eq(ret, 0);
+
+/*
+ * If flips are requested flip away and back before
+ * checking CRC.
+ */
+flip_count = data->flips;
  while (flip_count--) {
  ret = drmModePageFlip(data->gfx_fd,
output->config.crtc->crtc_id,
@@ -376,6 +378,9 @@ static void test_plane_rotation(data_t *data, int 
plane_type)

  igt_assert_eq(ret, 0);
  wait_for_pageflip(data->gfx_fd);
  }
+
+igt_pipe_crc_collect_crc(data->pipe_crc, _output);
+igt_assert_crc_equal(>ref_crc, _output);
  }
  valid_tests++;
@@ -569,8 +574,66 @@ err_commit:
  igt_assert_eq(ret, 0);
  }
+static const char *plane_test_str(unsigned plane)
+{
+switch (plane) {
+case DRM_PLANE_TYPE_PRIMARY:
+return "primary";
+case DRM_PLANE_TYPE_OVERLAY:
+return "sprite";
+case DRM_PLANE_TYPE_CURSOR:
+return "cursor";
+

Re: [Intel-gfx] [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin]

On 07/08/2017 16:53, Daniel Vetter wrote:

On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

To the best of my recollection the page flipping test was added
simply to start exercising page flips with 90/270 rotation.

There is no need to do 60 flips which can take quite some time,
because we do 60 flips against each pipe and each fb geometry.

Also, calling this a stress test is also not matching the
original idea of the test.

Several changes:

1. Remove the stress from the name and reduce the number of
flips to one only.

2. Move the page flip before CRC collection for a more useful
test.

3. Add more flipping tests, for different rotation and sprite
planes.


I assume you didn't make the test overall slower with this?


4. Convert to table driven subtest generation.

Signed-off-by: Tvrtko Ursulin 
Cc: Daniel Vetter 
Cc: Chris Wilson 


Didn't do a full review, but sounds all reasonable. And I assume you've
tested this at least locally (the igt patchwork CI instance doesn't do
full runs ... yet).


Yes I've ran it on SKL. It adds more subtests so the runtime is longer, 
but it also finds new bugs.


Ideally someone with display-fu picks this up, fixes the bug(s) this 
exposes (or tells me the test is wrong), so we are not merging known 
failing tests? Or even that is OK?


Tvrtko


Acked-by: Daniel Vetter 

---
  tests/intel-ci/extended.testlist |   2 +-
  tests/kms_rotation_crc.c | 137 ---
  2 files changed, 85 insertions(+), 54 deletions(-)

diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
index 17eed013f810..fb71091758c6 100644
--- a/tests/intel-ci/extended.testlist
+++ b/tests/intel-ci/extended.testlist
@@ -82,7 +82,7 @@ igt@gem_ringfill@vebox-bomb
  igt@gem_userptr_blits@stress-mm
  igt@gem_userptr_blits@stress-mm-invalidate-close
  igt@gem_userptr_blits@stress-mm-invalidate-close-overlap
-igt@kms_rotation_crc@primary-rotation-90-flip-stress
+igt@kms_rotation_crc@primary-rotation-90-flip
  igt@pm_rpm@gem-execbuf-stress
  igt@pm_rpm@gem-execbuf-stress-extra-wait
  igt@pm_rpm@gem-execbuf-stress-pc8
diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index 83e37f126f40..20f1adb67769 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -41,7 +41,7 @@ typedef struct {
int pos_y;
uint32_t override_fmt;
uint64_t override_tiling;
-   unsigned int flip_stress;
+   unsigned int flips;
  } data_t;
  
  static void

@@ -219,7 +219,7 @@ static void prepare_fbs(data_t *data, igt_output_t *output,
  
  	igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, >fb);
  
-	if (data->flip_stress) {

+   if (data->flips) {
igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, 
>fb_flip);
paint_squares(data, IGT_ROTATION_0, >fb_flip, 0.92);
}
@@ -351,15 +351,17 @@ static void test_plane_rotation(data_t *data, int 
plane_type)
ret = igt_display_try_commit2(display, commit);
if (data->override_fmt || data->override_tiling) {
igt_assert_eq(ret, -EINVAL);
-   } else {
-   igt_assert_eq(ret, 0);
-   igt_pipe_crc_collect_crc(data->pipe_crc,
- _output);
-   igt_assert_crc_equal(>ref_crc,
- _output);
+   continue;
}
  
-			flip_count = data->flip_stress;

+   /* Verify commit was ok. */
+   igt_assert_eq(ret, 0);
+
+   /*
+* If flips are requested flip away and back before
+* checking CRC.
+*/
+   flip_count = data->flips;
while (flip_count--) {
ret = drmModePageFlip(data->gfx_fd,
  
output->config.crtc->crtc_id,
@@ -376,6 +378,9 @@ static void test_plane_rotation(data_t *data, int 
plane_type)
igt_assert_eq(ret, 0);
wait_for_pageflip(data->gfx_fd);
}
+
+   igt_pipe_crc_collect_crc(data->pipe_crc, _output);
+   igt_assert_crc_equal(>ref_crc, _output);
}
  
  		valid_tests++;

@@ -569,8 +574,66 @@ err_commit:
igt_assert_eq(ret, 0);
  }
  
+static const char *plane_test_str(unsigned plane)

+{
+   switch (plane) {
+   case DRM_PLANE_TYPE_PRIMARY:
+   return "primary";
+   case DRM_PLANE_TYPE_OVERLAY:
+   

Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Jani Nikula]

Thomas, Ingo, Peter -

Can I have your ack on merging the below patch via drm/i915?
Unfortunately it wasn't originally sent to the proper mailing list. You
can see the full series it's part of at [1].

BR,
Jani.


[1] https://patchwork.freedesktop.org/series/29058/


On Sun, 20 Aug 2017, Hans de Goede  wrote:
> intel_uncore_forcewake_reset() does forcewake puts and gets as such
> we need to make sure that no-one tries to access the PUNIT->PMIC bus
> (on systems where this bus is shared) while it runs, otherwise bad
> things happen.
>
> Normally this is taken care of by the i915_pmic_bus_access_notifier()
> which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other
> driver tries to access the PMIC bus, so that later forcewake gets are
> no-ops (for the duration of the bus access).
>
> But intel_uncore_forcewake_reset gets called in 3 cases:
> 1) Before registering the pmic_bus_access_notifier
> 2) After unregistering the pmic_bus_access_notifier
> 3) To reset forcewake state on a GPU reset
>
> In all 3 cases the i915_pmic_bus_access_notifier() protection is
> insufficient.
>
> This commit fixes this race by calling iosf_mbi_punit_acquire() before
> calling intel_uncore_forcewake_reset(). In the case where it is called
> directly after unregistering the pmic_bus_access_notifier, we need to
> hold the punit-lock over both calls to avoid a race where
> intel_uncore_fw_release_timer() may execute between the 2 calls.
>
> To allow holding the lock over both calls we need an unlocked
> variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since
> intel_uncore.c is the only user of this function, we simply
> modify it in this commit. Doing this in a separate commit would
> require first adding an unlocked variant, then this commit and
> then removing the unused normal variant.
>
> Signed-off-by: Hans de Goede 
> Reviewed-by: Imre Deak 
> ---
> Changes in v2:
> -Rebase on current (July 6th 2017) drm-next
>
> Changes in v3:
> -Keep punit acquired / locked over the unregister + forcewake_reset
>  call combo to avoid a race hitting between the 2 calls
> -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier
>  to not take the lock itself, since we are the only users this is done
>  in this same commit
>
> Changes in v4:
> -Fix missing rename in doc-comment
> -Add and use iosf_mbi_assert_punit_acquired() helper
> -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside
>  intel_uncore_check_forcewake_domains()
> -Add Imre's Reviewed-by
> ---
>  arch/x86/include/asm/iosf_mbi.h   | 20 +---
>  arch/x86/platform/intel/iosf_mbi.c| 20 +++-
>  drivers/gpu/drm/i915/intel_uncore.c   | 17 +
>  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
>  4 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> index c313cac36f56..0f0de4303180 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void);
>  int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
>  
>  /**
> - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier
> + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus
> + *   notifier
> + *
> + * Note the caller must call iosf_mbi_punit_acquire() before calling this
> + * to ensure the bus is inactive before unregistering (and call
> + * iosf_mbi_punit_release() afterwards).
>   *
>   * @nb: notifier_block to unregister
>   */
> -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
> +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> + struct notifier_block *nb);
>  
>  /**
>   * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier 
> chain
> @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct 
> notifier_block *nb);
>   */
>  int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
>  
> +/**
> + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
> + */
> +void iosf_mbi_assert_punit_acquired(void);
> +
>  #else /* CONFIG_IOSF_MBI is not enabled */
>  static inline
>  bool iosf_mbi_available(void)
> @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct 
> notifier_block *nb)
>  }
>  
>  static inline
> -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
> +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> + struct notifier_block *nb)
>  {
>   return 0;
>  }
> @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned 
> long val, void *v)
>   return 0;
>  }
>  
> +static inline void iosf_mbi_assert_punit_acquired(void) {}
> +
>  #endif /* 

Re: [Intel-gfx] [PATCH i-g-t 12/12] RFC: meson build system support

[Jani Nikula]

On Sat, 02 Sep 2017, Daniel Vetter  wrote:
> +if cc.has_member('struct sysinfo', 'totalram',
> + prefix : '#include ')
> + config_h.set('HAVE_STRUCT_SYSINFO_TOTALRAM', 1)
> +endif
> +
> +add_project_arguments('-D_GNU_SOURCE', language : 'c')

Just something that caught my eye that bit me in the past. The project
arguments are *not* passed on to feature tests such as
cc.has_member(). I don't think it matters in this case, but you'll want
to #define _GNU_SOURCE in the prefix if you're testing for GNU
stuff. Otherwise you may end up using compat versions. Just a heads up,
that's all.

BR,
Jani.


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

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL   : https://patchwork.freedesktop.org/series/29538/
State : failure

== Summary ==

Test kms_setmode:
Subgroup basic:
pass   -> FAIL   (shard-hsw) fdo#99912
Test kms_cursor_legacy:
Subgroup long-nonblocking-modeset-vs-cursor-atomic:
pass   -> INCOMPLETE (shard-hsw)
Test perf:
Subgroup blocking:
pass   -> FAIL   (shard-hsw) fdo#102252
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
skip   -> PASS   (shard-hsw)
Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-wc:
skip   -> PASS   (shard-hsw)
Subgroup fbc-rgb565-draw-mmap-gtt:
pass   -> FAIL   (shard-hsw)
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-C-frame-sequence:
pass   -> FAIL   (shard-hsw)
Test kms_busy:
Subgroup extended-modeset-hang-oldfb-with-reset-render-C:
pass   -> INCOMPLETE (shard-hsw)
Test kms_atomic_transition:
Subgroup plane-use-after-nonblocking-unbind-fencing:
fail   -> PASS   (shard-hsw) fdo#101847 +1

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847

shard-hswtotal:2118 pass:1153 dwarn:0   dfail:0   fail:17  skip:946 
time:8895s

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5574/shards.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/23] drm/i915/gemfs: enable THP

[Matthew Auld]

On 29 August 2017 at 15:49, Joonas Lahtinen
 wrote:
> On Mon, 2017-08-21 at 19:34 +0100, Matthew Auld wrote:
>> Enable transparent-huge-pages through gemfs by mounting with
>> huge=within_size.
>>
>> Signed-off-by: Matthew Auld 
>> Cc: Joonas Lahtinen 
>> Cc: Chris Wilson 
>
> 
>
>> +++ b/drivers/gpu/drm/i915/i915_gemfs.c
>> @@ -24,6 +24,7 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "i915_drv.h"
>>  #include "i915_gemfs.h"
>> @@ -41,6 +42,20 @@ int i915_gemfs_init(struct drm_i915_private *i915)
>>   if (IS_ERR(gemfs))
>>   return PTR_ERR(gemfs);
>>
>> + if (has_transparent_hugepage()) {
>> + struct super_block *sb = gemfs->mnt_sb;
>> + char options[] = "huge=within_size";
>
> char const options[] ?

remount_fs expects char *data, and shmem_parse_options() may try to modify data.

>
>> + int flags = 0;
>> +
>> + /* We don't consider failure to remount fatal, since this 
>> should
>> +  * only ever attempt to modify the mount options of the sb, and
>> +  * so should always leave us with a working mount upon failure.
>> +  * Hence decoupling this from the actual kern_mount is probably
>> +  * advisable.
>> +  */
>> + WARN_ON(sb->s_op->remount_fs(sb, , options));
>
> Not to have too many fallback paths, would it make sense for any error
> in setting up gemfs to result in NULL gemfs and fallback to tmpfs?

Okay, will do.

>
> With the string constified, this is:
>
> Reviewed-by: Joonas Lahtinen 
>
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/8] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Patchwork (2017-09-03 15:17:57)
> == Series Details ==
> 
> Series: series starting with [1/8] drm/i915: Try harder to finish the 
> idle-worker
> URL   : https://patchwork.freedesktop.org/series/29764/
> State : success
> 
> == Summary ==
> 
> Test kms_sysfs_edid_timing:
> warn   -> PASS   (shard-hsw) fdo#100047
> Test perf:
> Subgroup polling:
> fail   -> PASS   (shard-hsw) fdo#102252
> 
> fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
> fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
> 
> shard-hswtotal:2263 pass:1233 dwarn:0   dfail:0   fail:14  skip:1015 
> time:9262s
> 
> == Logs ==
> 
> For more details see: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5572/shards.html

Fwiw, finally we bring the incompletes on apl under control.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH 1/5] drm/i915: Always wait for flip_done

[Chris Wilson]

Quoting Maarten Lankhorst (2017-08-30 13:17:48)
> The next commit removes the wait for flip_done in in
> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
> to pass. Instead of using complicated vblank tracking which ends
> up being ignored anyway, call the correct atomic helper. :)
> 
> RFC because I'm not completely sure what we want to do with the vblank 
> waiting,
> I think for now this patch is the right way to go until we decide because it
> preserves the status quo when drm_crtc_commit was introduced.

Fwiw, this is fixing a bug in rotation:

> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
> -{
> -   /* fb updated, need to unpin old fb */
> -   if (crtc_state->fb_changed)
> -   return true;

is insufficient as the fb may be unchanged but a new vma allocated for
the rotated pages.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL   : https://patchwork.freedesktop.org/series/29538/
State : success

== Summary ==

Series 29538v2 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/2/mbox/

Test kms_flip:
Subgroup basic-flip-vs-modeset:
pass   -> SKIP   (fi-skl-x1585l) fdo#101781

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

fi-bdw-5557u total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:453s
fi-bdw-gvtdvmtotal:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:439s
fi-blb-e6850 total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  
time:361s
fi-bsw-n3050 total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  
time:554s
fi-bwr-2160  total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 
time:255s
fi-bxt-j4205 total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:519s
fi-byt-j1900 total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  
time:523s
fi-byt-n2820 total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  
time:518s
fi-cfl-s total:285  pass:250  dwarn:1   dfail:0   fail:0   skip:33 
fi-elk-e7500 total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  
time:438s
fi-glk-2atotal:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:612s
fi-hsw-4770  total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:444s
fi-hsw-4770r total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:423s
fi-ilk-650   total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  
time:427s
fi-ivb-3520m total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:507s
fi-ivb-3770  total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:472s
fi-kbl-7500u total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  
time:520s
fi-kbl-7560u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:596s
fi-kbl-r total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:598s
fi-pnv-d510  total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  
time:528s
fi-skl-6260u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:472s
fi-skl-6700k total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:534s
fi-skl-6770hqtotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:518s
fi-skl-gvtdvmtotal:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  
time:444s
fi-skl-x1585ltotal:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:483s
fi-snb-2520m total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  
time:548s
fi-snb-2600  total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  
time:403s

a53eac614143e349be9ca1ab6c4ac799f8e1f0f9 drm-tip: 2017y-09m-04d-09h-11m-21s UTC 
integration manifest
a5bb7aa90443 drm/atomic: Make async plane update checks work as intended, v2.
01321b66e3ad drm/atomic: Fix freeing connector/plane state too early by 
tracking commits, v3.
0e520aabc9b9 drm/atomic: Return commit in drm_crtc_commit_get for better 
annotation
eea1321f34ec drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, 
v2.
0c1f91d8a2ba drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
f65cba7fa70a drm/i915: Always wait for flip_done, v2.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5574/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Silence sparse by using gfp_t

[Chris Wilson]

Quoting Joonas Lahtinen (2017-09-04 07:50:38)
> On Fri, 2017-09-01 at 15:57 +0100, Chris Wilson wrote:
> > Sparse enforces that GFP flags are only manipulated inside gfp_t locals.
> > 
> > Fixes: 4d470f7359c4 ("drm/i915: Avoid undefined behaviour of "u32 >> 32"")
> > Signed-off-by: Chris Wilson 
> > Cc: Joonas Lahtinen 
> > Cc: Tvrtko Ursulin 
> 
> Isn't Fixes: bit much for sparse warning?
Impact: None ?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Daniel Vetter (2017-09-04 09:35:49)
> On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> > If a worker requeues itself, it may switch to a different kworker pool,
> > which flush_work() considers as complete. To be strict, we then need to
> > keep flushing the work until it is no longer pending.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> > Signed-off-by: Chris Wilson 
> > Cc: Mika Kuoppala 
> 
> Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
> al.

The semantics are so horrible that I wouldn't suggest that the core
should expose such a nasty trap. You have to be absolutely certain that
your work stops requeueing itself.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: warning for drm/i915: Add interface to reserve fence registers for vGPU (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Add interface to reserve fence registers for vGPU (rev3)
URL   : https://patchwork.freedesktop.org/series/29523/
State : warning

== Summary ==

Test kms_flip:
Subgroup wf_vblank-vs-dpms-interruptible:
pass   -> DMESG-WARN (shard-hsw)
Test kms_setmode:
Subgroup basic:
fail   -> PASS   (shard-hsw) fdo#99912
Test perf:
Subgroup blocking:
pass   -> FAIL   (shard-hsw) fdo#102252

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

shard-hswtotal:2265 pass:1232 dwarn:1   dfail:0   fail:16  skip:1016 
time:9570s

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5573/shards.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Mika Kuoppala (2017-09-04 11:19:09)
> Chris Wilson  writes:
> > +/*
> > + * Wait until the work is finally complete, even if it tries to postpone
> > + * by requeueing itself. Note, that if the worker never cancels itself,
> > + * we will spin forever.
> > + */
> > +static inline void drain_delayed_work(struct delayed_work *dw)
> > +{
> > + do {
> > + while (flush_delayed_work(dw))
> > + ;
> > + } while (delayed_work_pending(dw));
> 
> So we end spinning if someone doesn't let go of mutex.
> 
> Add a timeout for doing this and let it slide into
> suspend with a error message anyways instead of spinning
> forever?

Such error messages already exist (NMI). But for us if we skip flushing
the work, we leave the driver in a very inconsistent state and expect to
blow up on resume. It's a lose-lose.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation

[Maarten Lankhorst]

This will allow code to do x->commit = drm_crtc_commit_get(commit),
making it clearer where references are used.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/drm_atomic_helper.c | 3 +--
 include/drm/drm_atomic.h| 6 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 1f5cdafb050e..04629d883114 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool 
nonblock)
return -EBUSY;
}
} else if (i == 1) {
-   stall_commit = commit;
-   drm_crtc_commit_get(stall_commit);
+   stall_commit = drm_crtc_commit_get(commit);
break;
}
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 285fbc4ec360..a80a8dadef00 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -251,10 +251,14 @@ void __drm_crtc_commit_free(struct kref *kref);
  * @commit: CRTC commit
  *
  * Increases the reference of @commit.
+ *
+ * Returns:
+ * The pointer to @commit, with reference increased.
  */
-static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
+static inline struct drm_crtc_commit *drm_crtc_commit_get(struct 
drm_crtc_commit *commit)
 {
kref_get(>ref);
+   return commit;
 }
 
 /**
-- 
2.11.0

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

[Intel-gfx] [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Maarten Lankhorst]

Currently we neatly track the crtc state, but forget to look at
plane/connector state.

When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.

This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
fake crtc commit for that gets signaled on hw_done for all the
planes/connectors.

We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.

Changes since v1:
- Only create a single disable commit. (danvet)
- Fix leak in intel_legacy_cursor_update.
Changes since v2:
- Make reference counting in drm_atomic_helper_setup_commit
  more obvious. (pinchartl)
- Call cleanup_done for fake commit. (danvet)
- Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
- Add comment to drm_atomic_helper_swap_state. (pinchartl)

Signed-off-by: Maarten Lankhorst 
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart 
---
 drivers/gpu/drm/drm_atomic.c |   4 +
 drivers/gpu/drm/drm_atomic_helper.c  | 172 +--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 include/drm/drm_atomic.h |  12 +++
 include/drm/drm_connector.h  |   7 ++
 include/drm/drm_plane.h  |   7 ++
 6 files changed, 198 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2cce48f203e0..75f5f74de9bf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
}
state->num_private_objs = 0;
 
+   if (state->fake_commit) {
+   drm_crtc_commit_put(state->fake_commit);
+   state->fake_commit = NULL;
+   }
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 04629d883114..c81d46927a74 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion 
*completion)
drm_crtc_commit_put(commit);
 }
 
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+   init_completion(>flip_done);
+   init_completion(>hw_done);
+   init_completion(>cleanup_done);
+   INIT_LIST_HEAD(>commit_entry);
+   kref_init(>ref);
+   commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+   if (crtc) {
+   struct drm_crtc_state *new_crtc_state;
+
+   new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+   return new_crtc_state->commit;
+   }
+
+   if (!state->fake_commit) {
+   state->fake_commit = kzalloc(sizeof(*state->fake_commit), 
GFP_KERNEL);
+   if (!state->fake_commit)
+   return NULL;
+
+   init_commit(state->fake_commit, NULL);
+   }
+
+   return state->fake_commit;
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
 {
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+   struct drm_connector *conn;
+   struct drm_connector_state *old_conn_state, *new_conn_state;
+   struct drm_plane *plane;
+   struct drm_plane_state *old_plane_state, *new_plane_state;
struct drm_crtc_commit *commit;
int i, ret;
 
@@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
if (!commit)
return -ENOMEM;
 
-   init_completion(>flip_done);
-   init_completion(>hw_done);
-   init_completion(>cleanup_done);
-   INIT_LIST_HEAD(>commit_entry);
-   kref_init(>ref);
-   commit->crtc = crtc;
+   init_commit(commit, crtc);
 
new_crtc_state->commit = commit;
 
@@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
drm_crtc_commit_get(commit);
}
 
+   for_each_oldnew_connector_in_state(state, conn, old_conn_state, 
new_conn_state, i) {
+   /* commit tracked through new_crtc_state->commit, no need to do 
it explicitly */
+   if (new_conn_state->crtc)
+   continue;
+
+   /* Userspace is not allowed to get ahead of the previous
+

[Intel-gfx] [PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.

[Maarten Lankhorst]

When commit synchronization through drm_crtc_commit was first
introduced, we tried to solve the problem of the flip_done
needing a reference count by blocking in cleanup_done.

This has been changed by commit 24835e442f28 ("drm: reference count
event->completion") which made the waits here no longer needed.

However, even after this commit we still needed the wait because
otherwise we cannot wait for the flip_done because this item might
have been removed from the list.

Changed since v1:
- Make mention of cleanup_done completing before flip_done.

Signed-off-by: Maarten Lankhorst 
Suggested-by: Daniel Vetter 
Reviewed-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 -
 1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 9001fc1023b1..1f5cdafb050e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1863,7 +1863,6 @@ void drm_atomic_helper_commit_cleanup_done(struct 
drm_atomic_state *old_state)
struct drm_crtc_state *new_crtc_state;
struct drm_crtc_commit *commit;
int i;
-   long ret;
 
for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
commit = new_crtc_state->commit;
@@ -1873,22 +1872,6 @@ void drm_atomic_helper_commit_cleanup_done(struct 
drm_atomic_state *old_state)
complete_all(>cleanup_done);
WARN_ON(!try_wait_for_completion(>hw_done));
 
-   /* commit_list borrows our reference, need to remove before we
-* clean up our drm_atomic_state. But only after it actually
-* completed, otherwise subsequent commits won't stall 
properly. */
-   if (try_wait_for_completion(>flip_done))
-   goto del_commit;
-
-   /* We must wait for the vblank event to signal our completion
-* before releasing our reference, since the vblank work does
-* not hold a reference of its own. */
-   ret = wait_for_completion_timeout(>flip_done,
- 10*HZ);
-   if (ret == 0)
-   DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
- crtc->base.id, crtc->name);
-
-del_commit:
spin_lock(>commit_lock);
list_del(>commit_entry);
spin_unlock(>commit_lock);
-- 
2.11.0

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

[Intel-gfx] [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.

[Maarten Lankhorst]

By always keeping track of the last commit in plane_state, we know
whether there is an active update on the plane or not. With that
information we can reject the fast update, and force the slowpath
to be used as was originally intended.

We cannot use plane_state->crtc->state here, because this only mentions
the most recent commit for the crtc, but not the planes that were part
of it. We specifically care about what the last commit involving this
plane is, which can only be tracked with a pointer in the plane state.

Changes since v1:
- Clean up the whole function here, instead of partially earlier.
- Add mention in the commit message why we need commit in plane_state.
- Swap plane->state in intel_legacy_cursor_update, instead of
  reassigning all variables. With this commit We know that the cursor
  is not part of any active commits so this hack can be removed.

Cc: Gustavo Padovan 
Signed-off-by: Maarten Lankhorst 
Reviewed-by: Gustavo Padovan 
Reviewed-by: Daniel Vetter  #v1
---
 drivers/gpu/drm/drm_atomic_helper.c  | 53 ++--
 drivers/gpu/drm/i915/intel_display.c | 27 +++---
 include/drm/drm_plane.h  |  5 ++--
 3 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c81d46927a74..a2cd432d8b2d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device 
*dev,
 {
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
-   struct drm_crtc_commit *commit;
-   struct drm_plane *__plane, *plane = NULL;
-   struct drm_plane_state *__plane_state, *plane_state = NULL;
+   struct drm_plane *plane;
+   struct drm_plane_state *old_plane_state, *new_plane_state;
const struct drm_plane_helper_funcs *funcs;
-   int i, j, n_planes = 0;
+   int i, n_planes = 0;
 
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
if (drm_atomic_crtc_needs_modeset(crtc_state))
return -EINVAL;
}
 
-   for_each_new_plane_in_state(state, __plane, __plane_state, i) {
+   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, i)
n_planes++;
-   plane = __plane;
-   plane_state = __plane_state;
-   }
 
/* FIXME: we support only single plane updates for now */
-   if (!plane || n_planes != 1)
+   if (n_planes != 1)
return -EINVAL;
 
-   if (!plane_state->crtc)
+   if (!new_plane_state->crtc)
return -EINVAL;
 
funcs = plane->helper_private;
if (!funcs->atomic_async_update)
return -EINVAL;
 
-   if (plane_state->fence)
+   if (new_plane_state->fence)
return -EINVAL;
 
/*
@@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device 
*dev,
 * the plane.  This prevents our async update's changes from getting
 * overridden by a previous synchronous update's state.
 */
-   for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-   if (plane->crtc != crtc)
-   continue;
-
-   spin_lock(>commit_lock);
-   commit = list_first_entry_or_null(>commit_list,
- struct drm_crtc_commit,
- commit_entry);
-   if (!commit) {
-   spin_unlock(>commit_lock);
-   continue;
-   }
-   spin_unlock(>commit_lock);
-
-   if (!crtc->state->state)
-   continue;
+   if (old_plane_state->commit &&
+   !try_wait_for_completion(_plane_state->commit->hw_done))
+   return -EBUSY;
 
-   for_each_plane_in_state(crtc->state->state, __plane,
-   __plane_state, j) {
-   if (__plane == plane)
-   return -EINVAL;
-   }
-   }
-
-   return funcs->atomic_async_check(plane, plane_state);
+   return funcs->atomic_async_check(plane, new_plane_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_async_check);
 
@@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
}
 
for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, i) {
-   /* commit tracked through new_crtc_state->commit, no need to do 
it explicitly */
-   if (new_plane_state->crtc)
-   continue;
+   /*
+* Unlike connectors, always track planes explicitly for
+* async 

[Intel-gfx] [PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.

[Maarten Lankhorst]

Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)

The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.

Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
-Remove drm_atomic_helper_async_check hunk. (pinchartl)

Signed-off-by: Maarten Lankhorst 
Reviewed-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_atomic.c|  7 
 drivers/gpu/drm/drm_atomic_helper.c | 64 -
 include/drm/drm_atomic.h|  1 -
 include/drm/drm_crtc.h  | 23 ++---
 4 files changed, 40 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)
crtc->funcs->atomic_destroy_state(crtc,
  state->crtcs[i].state);
 
-   if (state->crtcs[i].commit) {
-   kfree(state->crtcs[i].commit->event);
-   state->crtcs[i].commit->event = NULL;
-   drm_crtc_commit_put(state->crtcs[i].commit);
-   }
-
-   state->crtcs[i].commit = NULL;
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..9001fc1023b1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
  struct drm_atomic_state *old_state)
 {
-   struct drm_crtc_state *unused;
+   struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
int i;
 
-   for_each_new_crtc_in_state(old_state, crtc, unused, i) {
-   struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+   for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+   struct drm_crtc_commit *commit = new_crtc_state->commit;
int ret;
 
if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
kref_init(>ref);
commit->crtc = crtc;
 
-   state->crtcs[i].commit = commit;
+   new_crtc_state->commit = commit;
 
ret = stall_checks(crtc, nonblock);
if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct 
drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
-   struct drm_crtc_commit *commit;
-   int i = 0;
-
-   list_for_each_entry(commit, >commit_list, commit_entry) {
-   /* skip the first entry, that's the current commit */
-   if (i == 1)
-   return commit;
-   i++;
-   }
-
-   return NULL;
-}
-
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding 
commits
  * @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct 
drm_crtc *crtc)
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state 
*old_state)
 {
struct drm_crtc *crtc;
-   struct drm_crtc_state *new_crtc_state;
+   struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
 
-   for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-   spin_lock(>commit_lock);
-   commit = preceeding_commit(crtc);
-   if (commit)
-   drm_crtc_commit_get(commit);
-   spin_unlock(>commit_lock);
+   for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+   commit = old_crtc_state->commit;
 
if (!commit)
continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct 
drm_atomic_state *old_state)
if (ret == 0)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
  crtc->base.id, crtc->name);
-
-   drm_crtc_commit_put(commit);
}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1857,7 +1835,7 @@ void 

[Intel-gfx] [PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes.

[Maarten Lankhorst]

New iteration with all feedback hopefully addressed.

Maarten Lankhorst (6):
  drm/i915: Always wait for flip_done, v2.
  drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
  drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
  drm/atomic: Return commit in drm_crtc_commit_get for better annotation
  drm/atomic: Fix freeing connector/plane state too early by tracking
commits, v3.
  drm/atomic: Make async plane update checks work as intended, v2.

 drivers/gpu/drm/drm_atomic.c |  11 +-
 drivers/gpu/drm/drm_atomic_helper.c  | 293 +++
 drivers/gpu/drm/i915/i915_drv.h  |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 109 +++--
 include/drm/drm_atomic.h |  19 ++-
 include/drm/drm_connector.h  |   7 +
 include/drm/drm_crtc.h   |  23 ++-
 include/drm/drm_plane.h  |   8 +
 8 files changed, 275 insertions(+), 198 deletions(-)

-- 
2.11.0

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

[Intel-gfx] [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.

[Maarten Lankhorst]

The next commit removes the wait for flip_done in in
drm_atomic_helper_commit_cleanup_done, but we need it for the tests
to pass. Instead of using complicated vblank tracking which ends
up being ignored anyway, call the correct atomic helper. :)

Changes since v1:
- Always call drm_atomic_helper_wait_for_flip_done,
  even for legacy cursor updates. (danvet)

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 +-
 drivers/gpu/drm/i915/intel_display.c | 84 +++-
 2 files changed, 8 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38bd08f2539b..79533ba6f387 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
struct drm_atomic_state *old_state);
void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
 struct drm_atomic_state *old_state);
-   void (*update_crtcs)(struct drm_atomic_state *state,
-unsigned int *crtc_vblank_mask);
+   void (*update_crtcs)(struct drm_atomic_state *state);
void (*audio_codec_enable)(struct drm_connector *connector,
   struct intel_encoder *encoder,
   const struct drm_display_mode 
*adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 216cd9e0e08f..a6cf1c20c712 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc 
*crtc)
return dev->driver->get_vblank_counter(dev, crtc->pipe);
 }
 
-static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
- struct drm_i915_private *dev_priv,
- unsigned crtc_mask)
-{
-   unsigned last_vblank_count[I915_MAX_PIPES];
-   enum pipe pipe;
-   int ret;
-
-   if (!crtc_mask)
-   return;
-
-   for_each_pipe(dev_priv, pipe) {
-   struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
- pipe);
-
-   if (!((1 << pipe) & crtc_mask))
-   continue;
-
-   ret = drm_crtc_vblank_get(>base);
-   if (WARN_ON(ret != 0)) {
-   crtc_mask &= ~(1 << pipe);
-   continue;
-   }
-
-   last_vblank_count[pipe] = drm_crtc_vblank_count(>base);
-   }
-
-   for_each_pipe(dev_priv, pipe) {
-   struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
- pipe);
-   long lret;
-
-   if (!((1 << pipe) & crtc_mask))
-   continue;
-
-   lret = wait_event_timeout(dev->vblank[pipe].queue,
-   last_vblank_count[pipe] !=
-   drm_crtc_vblank_count(>base),
-   msecs_to_jiffies(50));
-
-   WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
-
-   drm_crtc_vblank_put(>base);
-   }
-}
-
-static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
-{
-   /* fb updated, need to unpin old fb */
-   if (crtc_state->fb_changed)
-   return true;
-
-   /* wm changes, need vblank before final wm's */
-   if (crtc_state->update_wm_post)
-   return true;
-
-   if (crtc_state->wm.need_postvbl_update)
-   return true;
-
-   return false;
-}
-
 static void intel_update_crtc(struct drm_crtc *crtc,
  struct drm_atomic_state *state,
  struct drm_crtc_state *old_crtc_state,
- struct drm_crtc_state *new_crtc_state,
- unsigned int *crtc_vblank_mask)
+ struct drm_crtc_state *new_crtc_state)
 {
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
}
 
drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
-
-   if (needs_vblank_wait(pipe_config))
-   *crtc_vblank_mask |= drm_crtc_mask(crtc);
 }
 
-static void intel_update_crtcs(struct drm_atomic_state *state,
-  unsigned int *crtc_vblank_mask)
+static void intel_update_crtcs(struct drm_atomic_state *state)
 {
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct 
drm_atomic_state *state,

Re: [Intel-gfx] [PATCH i-g-t v2] tests/kms_frontbuffer_tracking: increase FBC wait timeout to 5s

[Chris Wilson]

Quoting Paulo Zanoni (2017-09-01 20:12:01)
> Em Sex, 2017-08-25 às 14:11 +0100, Chris Wilson escreveu:
> > Quoting Lofstedt, Marta (2017-08-25 13:50:16)
> > > 
> > > 
> > > > -Original Message-
> > > > From: Lofstedt, Marta
> > > > Sent: Friday, August 25, 2017 2:54 PM
> > > > To: 'Chris Wilson' ; intel-...@lists.fr
> > > > eedesktop.org
> > > > Subject: RE: [Intel-gfx] [PATCH i-g-t v2]
> > > > tests/kms_frontbuffer_tracking:
> > > > increase FBC wait timeout to 5s
> > > > 
> > > > 
> > > > 
> > > > > -Original Message-
> > > > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > > > > Sent: Friday, August 25, 2017 1:47 PM
> > > > > To: Lofstedt, Marta ; intel-
> > > > > g...@lists.freedesktop.org
> > > > > Subject: Re: [Intel-gfx] [PATCH i-g-t v2]
> > > > > tests/kms_frontbuffer_tracking:
> > > > > increase FBC wait timeout to 5s
> > > > > 
> > > > > Quoting Marta Lofstedt (2017-08-25 11:40:29)
> > > > > > From: "Lofstedt, Marta" 
> > > > > > 
> > > > > > The subtests: igt@kms_frontbuffer_tracking@fbc-*draw*
> > > > > > has non-consistent results, pending between fail and pass.
> > > > > > The fails are always due to "FBC disabled".
> > > > > > With this increase in timeout the flip-flop behavior is no
> > > > > > longer
> > > > > > reproducible.
> > > > > > 
> > > > > > This is a partial revert of:
> > > > > > 64590c7b768dc8d8dd962f812d5ff5a39e7e8b54,
> > > > > > where the timeout was decreased from 5s to 2s.
> > > > > > After investigating the timeout needed, the conclusion is
> > > > > > that the
> > > > > > longer timeout is only needed when the test swaps between
> > > > > > some
> > > > > > specific draw domains, typically blt vs. mmap_cpu.
> > > > > > The objective of the FBC part of the tests is not to
> > > > > > benchmark draw
> > > > > > domain changes, it is to check that FBC was (re-)enabled.
> > > > > > 
> > > > > > V2: Added documentation
> > > > > > 
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101623
> > > > > > Signed-off-by: Marta Lofstedt 
> > > > > > Acked-by: Paulo Zanoni 
> > > > > > ---
> > > > > >  tests/kms_frontbuffer_tracking.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > > > > b/tests/kms_frontbuffer_tracking.c
> > > > > > index e03524f1..2538450c 100644
> > > > > > --- a/tests/kms_frontbuffer_tracking.c
> > > > > > +++ b/tests/kms_frontbuffer_tracking.c
> > > > > > @@ -924,7 +924,7 @@ static bool
> > > > > > fbc_stride_not_supported(void)
> > > > > > 
> > > > > >  static bool fbc_wait_until_enabled(void)  {
> > > > > 
> > > > > Try igt_drop_caches_set(device, DROP_RETIRE); instead of
> > > > > relaxing the
> > > > > timeout.
> > > > > -Chris
> > > > 
> > > > OK, I will test that and do a V3 if it works!
> > > > /Marta
> > > 
> > > I did some initial testing with igt_drop_caches_set inside
> > > fbc_wait_until_enabled and it looks good, I will add this to my
> > > weekend tests to get more results. This also appear to improve the
> > > runtime of the tests quite a bit. So, maybe the igt_drop_caches_set
> > > should be placed somewhere else so it will give runtime
> > > improvements not only for the FBC related sub-tests.
> > 
> > Sure, all the waits can do with the retire first, give it a common
> > function and a comment for the rationale (which should pretty much
> > the
> > same as given in the changelog). 
> 
> We can do that, sure, especially if it makes the tests faster...
> 
> > Anytime we use the GPU to invalidate
> > the frontbuffer tracking, we have to wait for a retire to do the
> > flush.
> > Retirement is lazy, and is normally driven by GPU activity but we
> > have a
> > background kworker to make sure we notice when the system becomes
> > idle
> > independent of userspace - except it's low frequency.
> 
> ... but our current 2s timeout should have been enough for that,
> shouldn't it? If I'm looking at the right part of the code, retirement
> should be once per second, so 2s should have been enough. But it looks
> like it's not enough
> 
> Unless I'm misinterpreting the round_up part, which could convert the
> 1s to 2s, which would still probably be fine...

It can bump the wait by upto a second (it tries to align wakeups on
second boundaries). And we may skip the work if the device is busy
elsewhere.

> Anyway, 3s looks like as definitely safe even in this case. Maybe we
> could go with 3s?
> 
> We can both increase the timeout *and* do cache dropping. Although I
> think not doing the cache dropping is definitely something that needs
> to be tested, so doing the cache dropping every time may not be a good
> idea.

You are not dropping the caches, it is just doing a retire.

The real question is what is the expectation? If we want the test to
simply state that when ready FBC et al will be 

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"

[Maarten Lankhorst]

Op 31-08-17 om 20:48 schreef Ville Syrjälä:
> On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrj...@linux.intel.com wrote:
>> From: Ville Syrjälä 
>>
>> Make the min_pixclk thing less confusing by changing it to track
>> the minimum acceptable cdclk frequency instead. This means moving
>> the application of the guardbands to a slightly higher level from
>> the low level platform specific calc_cdclk() functions.
>>
>> The immediate benefit is elimination of the confusing 2x factors
>> on GLK/CNL+ in the audio workarounds (which stems from the fact
>> that the pipes produce two pixels per clock).
>>
>> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
>> v3: Squash with the CNL cdclk limits patch (DK)
>> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
>>
>> Cc: Paulo Zanoni 
>> Cc: Rodrigo Vivi 
>> Cc: Dhinakaran Pandiyan 
>> Cc: Maarten Lankhorst 
>> Reviewed-by: Dhinakaran Pandiyan 
>> Signed-off-by: Ville Syrjälä 
> I didn't get any objections from the CNL camp, so I went ahead and
> pushed the series. Thanks for the reviews.
>
I seem to have a WARN_ON during init now on my broadwell, likely related to 
this series?

[   13.105310] i915 :00:02.0: vgaarb: changed VGA decodes: 
olddecodes=io+mem,decodes=io+mem:owns=io+mem
[   13.132264] systemd-journald[159]: Successfully sent stream file descriptor 
to service manager.
[   13.161016] WARN_ON(min_cdclk < 0)
[   13.161078] [ cut here ]
[   13.161336] WARNING: CPU: 1 PID: 209 at 
drivers/gpu/drm/i915/intel_display.c:15070 
intel_modeset_setup_hw_state+0x15a4/0x3000 [i915]
[   13.161517] Modules linked in: snd_seq_device snd_timer drbg i915(+) 
cfg80211 ecdh_generic(+) prime_numbers drm_kms_helper snd syscopyarea 
sysfillrect sysimgblt fb_sys_fops drm soundcore fan thermal 
i2c_designware_platform i2c_designware_core acpi_pad parport_pc ppdev parport 
autofs4
[   13.161822] CPU: 1 PID: 209 Comm: systemd-udevd Tainted: G U  
4.13.0-rc7-patser+ #5236
[   13.161884] Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 
03/09/2015
[   13.161963] task: 8800cd054500 task.stack: 8800c684
[   13.162083] RIP: 0010:intel_modeset_setup_hw_state+0x15a4/0x3000 [i915]
[   13.162393] RSP: 0018:8800c68472a0 EFLAGS: 00010282
[   13.162434] RAX: 0016 RBX: 8800c65d4c80 RCX: 8800cd054cd8
[   13.162478] RDX:  RSI: 8800cd054d78 RDI: 8800cd054cd4
[   13.162521] RBP: 8800c68473e0 R08:  R09: 
[   13.162565] R10: 8800c65d4e57 R11:  R12: dc00
[   13.162611] R13: 8800c819 R14: 8800c65d6f88 R15: 8800c65d6e80
[   13.162654] FS:  7fc3599c08c0() GS:8800d4e8() 
knlGS:
[   13.162710] CS:  0010 DS:  ES:  CR0: 80050033
[   13.162750] CR2: 7ffd657bffb8 CR3: cb477000 CR4: 003406e0
[   13.162794] Call Trace:
[   13.162911]  ? intel_mode_from_pipe_config+0x560/0x560 [i915]
[   13.162975]  ? drm_modeset_lock+0x162/0x270 [drm]
[   13.163036]  ? drm_modeset_lock_all_ctx+0xf3/0x140 [drm]
[   13.163192]  intel_modeset_init+0x327b/0x4720 [i915]
[   13.163316]  ? intel_modeset_init_hw+0x160/0x160 [i915]
[   13.163431]  i915_driver_load+0x2c50/0x3300 [i915]
[   13.163473]  ? find_held_lock+0x36/0x1c0
[   13.163579]  ? __i915_printk+0x280/0x280 [i915]
[   13.163722]  ? wait_for_completion_killable_timeout+0x430/0x430
[   13.163775]  ? mutex_unlock+0xd/0x10
[   13.163809]  ? acpi_dev_found+0xa7/0xb0
[   13.163910]  i915_pci_probe+0x108/0x180 [i915]
[   13.164011]  ? i915_pci_remove+0x50/0x50 [i915]
[   13.164080]  local_pci_probe+0xe8/0x160
[   13.164120]  pci_device_probe+0x3fe/0x580
[   13.164190]  ? pci_device_remove+0x1b0/0x1b0
[   13.164230]  ? _raw_spin_unlock+0x2c/0x40
[   13.164271]  driver_probe_device+0x2fb/0x670
[   13.164313]  ? driver_probe_device+0x670/0x670
[   13.164352]  __driver_attach+0xff/0x140
[   13.164388]  bus_for_each_dev+0x11b/0x1b0
[   13.164675]  ? store_drivers_autoprobe+0x120/0x120
[   13.164719]  ? _raw_spin_unlock+0x2c/0x40
[   13.164759]  driver_attach+0x45/0x50
[   13.164791]  bus_add_driver+0x2a2/0x520
[   13.164832]  driver_register+0x256/0x310
[   13.164865]  ? __raw_spin_lock_init+0x2d/0xf0
[   13.164905]  __pci_register_driver+0x192/0x1a0
[   13.165013]  i915_init+0xc8/0xd5 [i915]
[   13.165081]  ? 0xc09e
[   13.165114]  do_one_initcall+0x121/0x204
[   13.165206]  ? initcall_blacklisted+0x160/0x160
[   13.165245]  ? kasan_unpoison_shadow+0x35/0x50
[   13.165282]  ? kasan_kmalloc+0xb6/0xd0
[   13.165317]  ? kasan_unpoison_shadow+0x35/0x50
[   13.165355]  ? __asan_register_globals+0x7c/0xa0
[   13.165399]  do_init_module+0x1b6/0x500
[   13.165440]  

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Arkadiusz Hiler]

On Mon, Sep 04, 2017 at 12:26:25PM +0200, Szwichtenberg, Radoslaw wrote:
> On Mon, 2017-09-04 at 12:27 +0300, Arkadiusz Hiler wrote:
> > On Mon, Sep 04, 2017 at 10:45:19AM +0200, Daniel Vetter wrote:
> > > On Mon, Sep 04, 2017 at 11:28:09AM +0300, Arkadiusz Hiler wrote:
> > > > On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> > > > > We have it. Daniel Stone said it comes from the X11 transition to the
> > > > > modular build.
> > > > > 
> > > > > Signed-off-by: Daniel Vetter 
> > > > 
> > > > It's also used for Android builds, which lack the generated config.h.
> > > > 
> > > > But is this even used nowadays by anyone?
> We do use IGTs to test kernels used in Android. We do not use the latest 
> version
> of IGT though as we are using one of older kernel versions.
> > > > 
> > > > How we are going to deal with the Android.mk with the transition to
> > > > meson? Can we just drop them?
> > > > 
> > > > Android, in recent releases, is shifting from Makefiles to Soong.
> > > > anyway. 
> > > 
> > > Looking at recent igt patches, we've stuffed all the fancy stuff (e.g.
> > > chamelium) into Makefile.am, not Makefile.sources. And no one updated the
> > > android build.
> We dont use/run chamelium tests - I guess we don't use any fancy stuff ;)
> > > 
> > > Judging from that, I think the android build is already firmly in bitrot
> > > territory :-/
> I do have automated build for latest IGTs on android N and so far all builds 
> are
> still passing.

**If you want to help maintaining the Android build,**  I would be happy
to help you with making pre-merge testing out of what you have and
plugging the results into patchwork (and mailing list).

> > > 
> > > So yeah the plan would be to just leave it there, until someone cares
> > > more. From a few googles it looks like meson might be able to
> > > cross-compile into an android environment, but it won't be able to get
> > > integrated into the android image build.
> > 
> > I am actually leaning towards getting rid of it completely from the
> > tree, instead of making the illusion that Android is supported in any
> > way.
> > 
> > If someone will care and pick up the maintenance, this should be easy
> > enough to dig up from the history.
> > 
> Is there no plan at all to support Android long term?

On IGT side of things? Not to my knowledge. We are not building it or
running on anything.

We just need someone who cares, and have resources to keep this
maintained.

-- 
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Szwichtenberg, Radoslaw]

On Mon, 2017-09-04 at 12:27 +0300, Arkadiusz Hiler wrote:
> On Mon, Sep 04, 2017 at 10:45:19AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 04, 2017 at 11:28:09AM +0300, Arkadiusz Hiler wrote:
> > > On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> > > > We have it. Daniel Stone said it comes from the X11 transition to the
> > > > modular build.
> > > > 
> > > > Signed-off-by: Daniel Vetter 
> > > 
> > > It's also used for Android builds, which lack the generated config.h.
> > > 
> > > But is this even used nowadays by anyone?
We do use IGTs to test kernels used in Android. We do not use the latest version
of IGT though as we are using one of older kernel versions.
> > > 
> > > How we are going to deal with the Android.mk with the transition to
> > > meson? Can we just drop them?
> > > 
> > > Android, in recent releases, is shifting from Makefiles to Soong.
> > > anyway. 
> > 
> > Looking at recent igt patches, we've stuffed all the fancy stuff (e.g.
> > chamelium) into Makefile.am, not Makefile.sources. And no one updated the
> > android build.
We dont use/run chamelium tests - I guess we don't use any fancy stuff ;)
> > 
> > Judging from that, I think the android build is already firmly in bitrot
> > territory :-/
I do have automated build for latest IGTs on android N and so far all builds are
still passing.
> > 
> > So yeah the plan would be to just leave it there, until someone cares
> > more. From a few googles it looks like meson might be able to
> > cross-compile into an android environment, but it won't be able to get
> > integrated into the android image build.
> 
> I am actually leaning towards getting rid of it completely from the
> tree, instead of making the illusion that Android is supported in any
> way.
> 
> If someone will care and pick up the maintenance, this should be easy
> enough to dig up from the history.
> 
Is there no plan at all to support Android long term?

Thanks,
Radek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Try harder to finish the idle-worker

[Mika Kuoppala]

Chris Wilson  writes:

> If a worker requeues itself, it may switch to a different kworker pool,
> which flush_work() considers as complete. To be strict, we then need to
> keep flushing the work until it is no longer pending.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c |  3 +--
>  drivers/gpu/drm/i915/i915_utils.h   | 13 +
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 48572b157222..1dd687a74879 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4202,8 +4202,7 @@ fault_irq_set(struct drm_i915_private *i915,
>   mutex_unlock(>drm.struct_mutex);
>  
>   /* Flush idle worker to disarm irq */
> - while (flush_delayed_work(>gt.idle_work))
> - ;
> + drain_delayed_work(>gt.idle_work);
>  
>   return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08bc518c..1829d3fa15d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4582,8 +4582,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   /* As the idle_work is rearming if it detects a race, play safe and
>* repeat the flush until it is definitely idle.
>*/
> - while (flush_delayed_work(_priv->gt.idle_work))
> - ;
> + drain_delayed_work(_priv->gt.idle_work);
>  
>   /* Assert that we sucessfully flushed all the work and
>* reset the GPU back to its idle, low power state.
> diff --git a/drivers/gpu/drm/i915/i915_utils.h 
> b/drivers/gpu/drm/i915/i915_utils.h
> index 12fc250b47b9..4f7ffa0976b1 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -119,4 +119,17 @@ static inline void __list_del_many(struct list_head 
> *head,
>   WRITE_ONCE(head->next, first);
>  }
>  
> +/*
> + * Wait until the work is finally complete, even if it tries to postpone
> + * by requeueing itself. Note, that if the worker never cancels itself,
> + * we will spin forever.
> + */
> +static inline void drain_delayed_work(struct delayed_work *dw)
> +{
> + do {
> + while (flush_delayed_work(dw))
> + ;
> + } while (delayed_work_pending(dw));

So we end spinning if someone doesn't let go of mutex.

Add a timeout for doing this and let it slide into
suspend with a error message anyways instead of spinning
forever?

-Mika

> +}
> +
>  #endif /* !__I915_UTILS_H */
> -- 
> 2.14.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Skip waking the device to service pwrite

[Chris Wilson]

Quoting Daniel Vetter (2017-09-04 09:12:12)
> On Wed, Aug 30, 2017 at 06:48:19PM +0100, Chris Wilson wrote:
> > If the device is in runtime suspend, resuming takes time and reduces our
> > powersaving. If this was for a small write into an object, that resume
> > will take longer than any savings in using the indirect GGTT access to
> > avoid the cpu cache.
> > 
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 21 ++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 93dfa793975a..8940a6873ca5 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1229,7 +1229,21 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object 
> > *obj,
> >   if (ret)
> >   return ret;
> >  
> > - intel_runtime_pm_get(i915);
> > + if (i915_gem_object_has_struct_page(obj)) {
> 
> I don't really see why we need to check for has_struct_page here (we do
> already outside the lock grabbing), and why if that's not the case we hit
> the slow-path?

We can't use the alternate paths if we don't have struct_page, hence we
have to force use of GTT if !i915_gem_object_has_struct_page. The
previous test is to also make sure we come down this path and not fail.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Add interface to reserve fence registers for vGPU

[Chris Wilson]

Quoting changbin...@intel.com (2017-09-04 09:01:01)
> From: Changbin Du 
> 
> In the past, vGPU alloc fence registers by walking through mm.fence_list
> to find fence which pin_count = 0 and vma is empty. vGPU may not find
> enough fence registers this way. Because a fence can be bind to vma even
> though it is not in using. We have found such failure many times these
> days.
> 
> An option to resolve this issue is that we can force-remove fence from
> vma in this case.
> 
> This patch added two new api to the fence management code:
>  - i915_reserve_fence() will try to find a free fence from fence_list
>and force-remove vma if need.
>  - i915_unreserve_fence() reclaim a reserved fence after vGPU has
>finished.
> 
> With this change, the fence management is more clear to work with vGPU.
> GVTg do not need remove fence from fence_list in private.
> 
> v3: (Chris)
>   - Add struct_mutex lock assertion.
>   - Only count for unpinned fence.
> 
> v2: (Chris)
>   - Rename the new api for symmetry.
>   - Add safeguard to ensure at least 1 fence remained for host display.
> 
> Signed-off-by: Changbin Du 
> Reviewed-by: Chris Wilson 

Anyone want to give an ack/review for the GVT side, and then I'll push.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Arkadiusz Hiler]

On Mon, Sep 04, 2017 at 10:45:19AM +0200, Daniel Vetter wrote:
> On Mon, Sep 04, 2017 at 11:28:09AM +0300, Arkadiusz Hiler wrote:
> > On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> > > We have it. Daniel Stone said it comes from the X11 transition to the
> > > modular build.
> > > 
> > > Signed-off-by: Daniel Vetter 
> > 
> > It's also used for Android builds, which lack the generated config.h.
> > 
> > But is this even used nowadays by anyone?
> > 
> > How we are going to deal with the Android.mk with the transition to
> > meson? Can we just drop them?
> > 
> > Android, in recent releases, is shifting from Makefiles to Soong.
> > anyway. 
> 
> Looking at recent igt patches, we've stuffed all the fancy stuff (e.g.
> chamelium) into Makefile.am, not Makefile.sources. And no one updated the
> android build.
> 
> Judging from that, I think the android build is already firmly in bitrot
> territory :-/
> 
> So yeah the plan would be to just leave it there, until someone cares
> more. From a few googles it looks like meson might be able to
> cross-compile into an android environment, but it won't be able to get
> integrated into the android image build.

I am actually leaning towards getting rid of it completely from the
tree, instead of making the illusion that Android is supported in any
way.

If someone will care and pick up the maintenance, this should be easy
enough to dig up from the history.

-- 
Arek

> We could also try to generate the Makefile.sources files from special
> meson targets (probably using the configuration generation stuff and a few
> templates).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/4] drm/i915/perf: add support for Coffeelake GT2

[Matthew Auld]

On 30 August 2017 at 17:12, Lionel Landwerlin
 wrote:
> Add the test configuration & timestamp frequency for Coffeelake GT2.
>
> Signed-off-by: Lionel Landwerlin 

Do we not want to also disable the clock-ratio-change reports? Also
can we not get away with just making the condition INTEL_GEN(i915) >=
9?

Reviewed-by: Matthew Auld 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] scripts/run-tests.sh: Use piglit's --ignore-missing

[Arkadiusz Hiler]

On Mon, Sep 04, 2017 at 10:33:32AM +0200, Daniel Vetter wrote:
> On Fri, Sep 01, 2017 at 04:09:03PM +0300, Arkadiusz Hiler wrote:
> > Recently we added a number of chamelium tests to the fast-feedback testlist.
> > 
> > Chemelium is build-optional - requires  `./configure --enable-chamelium`.
> > 
> > To mitigate issue with piglit exiting abruptly due to the (possibly)
> > missing test binaries, this makes it behave more gracefuly, considering
> > those as simply "notrun".
> > 
> > Cc: Petri Latvala 
> > Signed-off-by: Arkadiusz Hiler 
> 
> Reviewed-by: Daniel Vetter 

thanks for the review

pushed

-- 
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 12/12] RFC: meson build system support

[Daniel Vetter]

On Sat, Sep 02, 2017 at 07:04:06PM +0200, Daniel Vetter wrote:
> Why?
> 
> Because it's fast.
> 
> Like really, really fast.
> 
> Some data (from a snb laptop, so rather lower-powered):
> 
> - Incremental build after $ touch lib/igt_core.c with meson: 0.6s
>   It notices that the symbol list of the libigt.so hasn't changed and
>   doesn't bother re-linking the almost 300 binaries we have. make -j 6
>   for the same scenario takes 44s.
> 
> - Incremental build with nothing changed: make: 0.7s, meson: 0.2s This
>   means stuff like --disable-git-hash is entirely pointless with
>   meson, it's faster than a make ever can be (with 0.6s).
> 
> - Reconfigure stage: ninja reconfigure 0.8s vs. ./configure 8.6s)
> 
> - Running tests, after a full build: ninja test 6s vs. make check 24s
> 
> - Full build (i.e. including ./autogen.sh respectively meson build),
>   including tests, from a pristine git checkout. automake 2m49s vs.
>   meson 44s.
> 
> TODO:
> - cmdline options

Apparently this isn't how meson is done, but I guess we could do options
to disable chamelium and similar stuff.

> - gcc warnings, debug build, how does that work?
> - man pages

man pages and gcc warnings are now done, with the latest set of patches
from Eric and me.

> - gtkdoc

Working on this, probably hitting some bug in the meson gtkdoc support
right now.

Another thing I totally didn't tackle is install support. Half the stuff
is probably missing or in the wrong place right now.

Also, for convenience, the entire pile on fd.o:

https://cgit.freedesktop.org/~danvet/intel-gpu-tools/log/?h=stuff

I've mentioned that this stuff is fast, right?

:-)

Cheers, Daniel

> 
> Cc: Ville Syrjälä 
> Cc: Eric Anholt 
> Cc: Daniel Stone 
> Signed-off-by: Daniel Vetter 
> ---
>  .gitignore   |   1 +
>  assembler/meson.build|  73 ++
>  benchmarks/meson.build   |  36 +
>  lib/meson.build  | 166 ++
>  lib/prepend_log_domain.sh|   8 ++
>  lib/tests/meson.build|  34 +
>  lib/version.h.in |   1 +
>  meson.build  | 105 ++
>  overlay/meson.build  |  59 
>  tests/generate_testlist.sh   |  10 ++
>  tests/meson.build| 290 
> +++
>  tools/meson.build|  59 
>  tools/null_state_gen/meson.build |  15 ++
>  13 files changed, 857 insertions(+)
>  create mode 100644 assembler/meson.build
>  create mode 100644 benchmarks/meson.build
>  create mode 100644 lib/meson.build
>  create mode 100755 lib/prepend_log_domain.sh
>  create mode 100644 lib/tests/meson.build
>  create mode 100644 lib/version.h.in
>  create mode 100644 meson.build
>  create mode 100644 overlay/meson.build
>  create mode 100755 tests/generate_testlist.sh
>  create mode 100644 tests/meson.build
>  create mode 100644 tools/meson.build
>  create mode 100644 tools/null_state_gen/meson.build
> 
> diff --git a/.gitignore b/.gitignore
> index 6204965a0e32..e6919272d8b6 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -93,3 +93,4 @@ intel-gpu-tools-*/
>  
>  piglit
>  results
> +build
> diff --git a/assembler/meson.build b/assembler/meson.build
> new file mode 100644
> index ..b0e2db25
> --- /dev/null
> +++ b/assembler/meson.build
> @@ -0,0 +1,73 @@
> +lib_brw_src = [
> + 'brw_context.c',
> + 'brw_disasm.c',
> + 'brw_eu.c',
> + 'brw_eu_compact.c',
> + 'brw_eu_debug.c',
> + 'brw_eu_emit.c',
> + 'brw_eu_util.c',
> + 'gen8_disasm.c',
> + 'gen8_instruction.c',
> + 'ralloc.c',
> +]
> +
> +lib_brw = shared_library('brw', lib_brw_src,
> + dependencies : igt_deps)
> +
> +flex = find_program('flex')
> +bison = find_program('bison')
> +
> +lgen = generator(flex,
> + output : '@BASENAME@.c',
> + arguments : ['-o', '@OUTPUT@', '@INPUT@'])
> +
> +lfiles = lgen.process('lex.l')
> +
> +pgen = generator(bison,
> + output : ['@BASENAME@.c', '@BASENAME@.h'],
> + arguments : ['@INPUT@', '--defines=@OUTPUT1@', 
> '--output=@OUTPUT0@'])
> +
> +pfiles = pgen.process('gram.y')
> +
> +executable('intel-gen4asm', 'main.c', lfiles, pfiles, link_with : lib_brw)
> +
> +executable('intel-gen4disasm', 'disasm-main.c', link_with : lib_brw)
> +
> +gen4asm_testcases = [
> + 'test/mov',
> + 'test/frc',
> + 'test/rndd',
> + 'test/rndu',
> + 'test/rnde',
> + 'test/rnde-intsrc',
> + 'test/rndz',
> + 'test/lzd',
> + 'test/not',
> + 'test/immediate',
> +]
> +
> +# Those tests were already failing when the assembler was imported from
> +# the intel-gen4asm git repository:
> +#   http://cgit.freedesktop.org/xorg/app/intel-gen4asm/
> +# We disable them "for now" as a workaround to be able to release i-g-t
> +gen4asm_testcases_broken = [
> + 

[Intel-gfx] [PATCH i-g-t 3/7] meson: Add some compiler flags to reduce warnings.

[Daniel Vetter]

From: Eric Anholt 

These warnings are apparently new compared to the autotools build.  We
can fix the things they complain about later, if we want.

Signed-off-by: Eric Anholt 
Signed-off-by: Daniel Vetter 
---
 meson.build | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/meson.build b/meson.build
index fee64fcdf36e..4d6985d191f9 100644
--- a/meson.build
+++ b/meson.build
@@ -8,6 +8,17 @@ project('IGT gpu tests', 'c',
 
 cc = meson.get_compiler('c')
 
+add_global_arguments('-Wno-unused-parameter', language: 'c')
+add_global_arguments('-Wno-sign-compare', language: 'c')
+add_global_arguments('-Wno-missing-field-initializers', language: 'c')
+add_global_arguments('-Wno-clobbered', language: 'c')
+
+# Macros asserting on the range of their arguments triggers this.
+add_global_arguments('-Wno-type-limits', language: 'c')
+
+# igt_assert(0) in switch statements triggers a bunch of this.
+add_global_arguments('-Wimplicit-fallthrough=0', language: 'c')
+
 inc = include_directories('lib', '.')
 
 config_h = configuration_data()
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 7/7] meson: add manpage support

[Daniel Vetter]

It seems like meson doesn't want you to string together targets
like make does, but wants it all in one step. So another little
shell script it is.

Signed-off-by: Daniel Vetter 
---
 man/defs.rst.in |  5 +
 man/meson.build | 45 +
 man/rst2man.sh  | 16 
 meson.build |  1 +
 4 files changed, 67 insertions(+)
 create mode 100644 man/defs.rst.in
 create mode 100644 man/meson.build
 create mode 100755 man/rst2man.sh

diff --git a/man/defs.rst.in b/man/defs.rst.in
new file mode 100644
index ..54b7eec08903
--- /dev/null
+++ b/man/defs.rst.in
@@ -0,0 +1,5 @@
+.. |PACKAGE_NAME| replace:: @PACKAGE_NAME@
+.. |PACKAGE_VERSION| replace:: @PACKAGE_VERSION@
+.. |PACKAGE_STRING| replace:: @PACKAGE_STRING@
+.. |MANUAL_SECTION| replace:: 1
+.. |MANUAL_GROUP| replace:: General Commands Manual
diff --git a/man/meson.build b/man/meson.build
new file mode 100644
index ..4f9f88e87540
--- /dev/null
+++ b/man/meson.build
@@ -0,0 +1,45 @@
+manpages = [
+   'intel_aubdump',
+   'intel_audio_dump',
+   'intel_bios_dumper',
+   'intel_error_decode',
+   'intel_gpu_frequency',
+   'intel_gpu_top',
+   'intel_gtt',
+   'intel_infoframes',
+   'intel_lid',
+   'intel_panel_fitter',
+   'intel_reg',
+   'intel_stepping',
+   'intel_upload_blit_large',
+   'intel_upload_blit_large_gtt',
+   'intel_upload_blit_large_map',
+   'intel_upload_blit_small',
+   'intel_vbt_decode',
+]
+
+man_config = configuration_data()
+
+man_config.set('PACKAGE_NAME', meson.project_name())
+man_config.set('PACKAGE_VERSION', meson.project_version())
+man_config.set('PACKAGE_STRING', meson.project_name() + ' ' + 
meson.project_version())
+
+defs_rst = configure_file(input : 'defs.rst.in',
+   output : 'defs.rst',
+   configuration : man_config)
+
+rst2man = find_program('rst2man', required : false)
+rst2man_script = find_program('rst2man.sh')
+
+if rst2man.found()
+   foreach manpage : manpages
+   custom_target(manpage + '.1',
+   build_by_default : true,
+   command : [ rst2man_script, '@INPUT@', 
'@OUTPUT@' ],
+   depend_files : [ defs_rst ],
+   input: manpage + '.rst',
+   output : manpage + '.1.gz',
+   install : true,
+   install_dir : join_paths(get_option('mandir'), 
'man1'))
+   endforeach
+endif
diff --git a/man/rst2man.sh b/man/rst2man.sh
new file mode 100755
index ..fc2b5ed863b1
--- /dev/null
+++ b/man/rst2man.sh
@@ -0,0 +1,16 @@
+#!/bin/bash
+
+input=$1
+output=$2
+
+out_dir=$(dirname ${output})
+in_file=$(basename ${input})
+
+# rst2man doesn't handle multiple source directories well, and since defs.rst 
is
+# generated we first need to move it all into the build dir
+cp $input $out_dir
+
+rst2man $out_dir/$in_file ${output%.gz}
+
+rm -f ${output}
+gzip ${output%.gz}
diff --git a/meson.build b/meson.build
index 39749a0e1103..2b49a0db6500 100644
--- a/meson.build
+++ b/meson.build
@@ -120,3 +120,4 @@ if libdrm_intel.found()
subdir('assembler')
subdir('overlay')
 endif
+subdir('man')
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 5/7] meson: Use static libs to handle IGT_LOG_DOMAIN.

[Daniel Vetter]

From: Eric Anholt 

It means that compiler errors in the .c files take you to the source
place in your editor, not a preprocessed temporary.

v2: Add the library deps, fails linking otherwise.

Signed-off-by: Eric Anholt  (v1)
Signed-off-by: Daniel Vetter 
---
 lib/dummy.c   |  0
 lib/meson.build   | 36 +---
 lib/prepend_log_domain.sh |  8 
 3 files changed, 25 insertions(+), 19 deletions(-)
 create mode 100644 lib/dummy.c
 delete mode 100755 lib/prepend_log_domain.sh

diff --git a/lib/dummy.c b/lib/dummy.c
new file mode 100644
index ..e69de29bb2d1
diff --git a/lib/meson.build b/lib/meson.build
index 51d3f9e278da..f0672edf1830 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -144,19 +144,33 @@ vcs_tag(input : 'version.h.in', output : 'version.h',
fallback : 'NO-GIT',
command : [ 'git', 'log', '-n1', '--pretty=format:g%h' ] )
 
-# FIXME we don't regenerate when the script changes
-prepend_log_domain = generator(find_program('prepend_log_domain.sh'),
-   arguments : [ '@INPUT@', '@OUTPUT@' ],
-   output : '@PLAINNAME@' + '.pre.c')
-
-processed_src_dep = prepend_log_domain.process(lib_sources)
+lib_intermediates = []
+foreach f: lib_sources
+# No / in the target name
+if f.contains('uwildmat')
+name = 'uwildmat'
+else
+name = f
+endif
+
+lib = static_library('igt-' + name,
+f,
+   include_directories: inc,
+   dependencies : lib_deps,
+   c_args : [
+   '-DIGT_DATADIR="@0@"'.format(pkgdatadir),
+   '-DIGT_SRCDIR="@0@"'.format(srcdir),
+   '-DIGT_LOG_DOMAIN="@0@"'.format(f.split('.')[0]),
+   ])
+
+lib_intermediates += lib
+endforeach
 
 lib_igt_build = shared_library('igt',
-processed_src_dep,
-include_directories : inc,
-dependencies : lib_deps,
-c_args : [ '-DIGT_DATADIR="@0@"'.format(pkgdatadir),
-   '-DIGT_SRCDIR="@0@"'.format(srcdir), ])
+['dummy.c'],
+link_whole: lib_intermediates,
+dependencies: lib_deps,
+)
 
 lib_igt = declare_dependency(link_with : lib_igt_build,
include_directories : inc)
diff --git a/lib/prepend_log_domain.sh b/lib/prepend_log_domain.sh
deleted file mode 100755
index 93a911508b7f..
--- a/lib/prepend_log_domain.sh
+++ /dev/null
@@ -1,8 +0,0 @@
-#!/bin/bash
-
-input=$1
-output=$2
-basename=$(basename $1 .c)
-
-echo "#define IGT_LOG_DOMAIN \"$basename\"" > $output
-cat $input >> $output
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 1/7] lib/ioctl_wrappers: make the valgrind wrapper always emit a statement:w

[Daniel Vetter]

gcc complains otherwise about empty ; statements ...

Signed-off-by: Daniel Vetter 
---
 lib/ioctl_wrappers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index b4d6210d5942..48750427a0c1 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -60,7 +60,7 @@
 
 #define VG(x) x
 #else
-#define VG(x)
+#define VG(x) do {} while (0)
 #endif
 
 #include "ioctl_wrappers.h"
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 6/7] meson: detect cc flags

[Daniel Vetter]

Somehow my gcc has a different idea of what no-implicit-fallthrough
should look like than the one Eric used.

fixup compiler flags

Signed-off-by: Daniel Vetter 
---
 meson.build | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/meson.build b/meson.build
index 4d6985d191f9..39749a0e1103 100644
--- a/meson.build
+++ b/meson.build
@@ -8,16 +8,22 @@ project('IGT gpu tests', 'c',
 
 cc = meson.get_compiler('c')
 
-add_global_arguments('-Wno-unused-parameter', language: 'c')
-add_global_arguments('-Wno-sign-compare', language: 'c')
-add_global_arguments('-Wno-missing-field-initializers', language: 'c')
-add_global_arguments('-Wno-clobbered', language: 'c')
-
+cc_args = [
+   '-Wno-unused-parameter',
+   '-Wno-sign-compare',
+   '-Wno-missing-field-initializers',
+   '-Wno-clobbered',
 # Macros asserting on the range of their arguments triggers this.
-add_global_arguments('-Wno-type-limits', language: 'c')
-
+   '-Wno-type-limits',
 # igt_assert(0) in switch statements triggers a bunch of this.
-add_global_arguments('-Wimplicit-fallthrough=0', language: 'c')
+   '-Wimplicit-fallthrough=0',
+]
+
+foreach cc_arg : cc_args
+  if cc.has_argument(cc_arg)
+add_global_arguments(cc_arg, language : 'c')
+  endif
+endforeach
 
 inc = include_directories('lib', '.')
 
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 4/7] meson: Don't build the igt audio test without gsl available.

[Daniel Vetter]

From: Eric Anholt 

Signed-off-by: Eric Anholt 
Signed-off-by: Daniel Vetter 
---
 tests/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/meson.build b/tests/meson.build
index 73833758be0e..4dd5a9c9d4c7 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -248,7 +248,7 @@ if libdrm_amdgpu.found()
test_deps += libdrm_amdgpu
 endif
 
-if alsa.found()
+if alsa.found() and gsl.found()
test_progs += [
'audio',
]
-- 
2.9.5

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

[Intel-gfx] [PATCH i-g-t 2/7] tests/kms_plane: Appease gcc -Wempty-body

[Daniel Vetter]

Not exactly sure what's the point, but oh well.

Signed-off-by: Daniel Vetter 
---
 tests/kms_plane.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/kms_plane.c b/tests/kms_plane.c
index 927d5d37fece..812497500d2d 100644
--- a/tests/kms_plane.c
+++ b/tests/kms_plane.c
@@ -207,8 +207,9 @@ test_plane_position_with_output(data_t *data,
 
if (flags & TEST_POSITION_FULLY_COVERED)
igt_assert_crc_equal(_crc, );
-   else
+   else {
;/* FIXME: missing reference CRCs */
+   }
 
igt_assert_crc_equal(, );
 
-- 
2.9.5

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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Add interface to reserve fence registers for vGPU (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Add interface to reserve fence registers for vGPU (rev3)
URL   : https://patchwork.freedesktop.org/series/29523/
State : success

== Summary ==

Series 29523v3 drm/i915: Add interface to reserve fence registers for vGPU
https://patchwork.freedesktop.org/api/1.0/series/29523/revisions/3/mbox/

Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
fail   -> PASS   (fi-snb-2600) fdo#100215
Test kms_frontbuffer_tracking:
Subgroup basic:
pass   -> DMESG-WARN (fi-bdw-5557u) fdo#102473

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

fi-bdw-5557u total:288  pass:267  dwarn:1   dfail:0   fail:0   skip:20  
time:458s
fi-bdw-gvtdvmtotal:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:440s
fi-blb-e6850 total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  
time:358s
fi-bsw-n3050 total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  
time:556s
fi-bwr-2160  total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 
time:253s
fi-bxt-j4205 total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:520s
fi-byt-j1900 total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  
time:522s
fi-byt-n2820 total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  
time:515s
fi-cfl-s total:285  pass:250  dwarn:1   dfail:0   fail:0   skip:33 
fi-elk-e7500 total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  
time:430s
fi-glk-2atotal:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  
time:615s
fi-hsw-4770  total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:448s
fi-hsw-4770r total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  
time:423s
fi-ilk-650   total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  
time:426s
fi-ivb-3520m total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:514s
fi-ivb-3770  total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:472s
fi-kbl-7500u total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  
time:518s
fi-kbl-7560u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:592s
fi-kbl-r total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  
time:598s
fi-pnv-d510  total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  
time:536s
fi-skl-6260u total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:468s
fi-skl-6700k total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  
time:538s
fi-skl-6770hqtotal:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  
time:508s
fi-skl-gvtdvmtotal:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  
time:447s
fi-skl-x1585ltotal:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  
time:486s
fi-snb-2520m total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  
time:550s
fi-snb-2600  total:288  pass:249  dwarn:0   dfail:0   fail:1   skip:38  
time:405s

b80ab89c2d7542c89e087c35c0f9e33475007f8f drm-tip: 2017y-09m-04d-08h-27m-46s UTC 
integration manifest
8820101bd2df drm/i915: Add interface to reserve fence registers for vGPU

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5573/
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/5] Add support for subtest-specific documentation

[Daniel Vetter]

On Mon, Sep 04, 2017 at 08:42:58AM +, Szwichtenberg, Radoslaw wrote:
> On Fri, 2017-08-11 at 10:20 +0200, Daniel Vetter wrote:
> > On Thu, Aug 10, 2017 at 01:26:47PM +0300, Petri Latvala wrote:
> > > The current documentation for tests is limited to a single string per
> > > test binary. This patch adds support for documenting individual
> > > subtests.
> > > 
> > > The syntax for subtest documentation is:
> > > 
> > >    igt_document_subtest("Frob knobs to see if one of the "
> > > "crossbeams will go out of skew on the "
> > > "treadle.\n");
> > >    igt_subtest("knob-frobbing-askew")
> > >  test_frob();
> > > 
> > > or with a format string:
> > > 
> > >   for_example_loop(e) {
> > > igt_document_subtest_f("Frob %s to see if one of the "
> > >    "crossbeams will go out of skew on the "
> > >    "treadle.\n", e->readable_name);
> > > igt_subtest_f("%s-frob-askew", e->name)
> > >   test_frob(e);
> > >   }
> > 
> > I'm not sold on this layout at all. Everywhere else where we do in-line
> > code documentation it is through specially formatted comments. That gives
> > you a lot of freedom for plain-text layout, in combination with some mild
> > markup (gtk-doc for igt and rst for kernel) that result in docs which both
> > look good in the code and look good rendered.
> > 
> > This here essentially restricts you to one-liners, and a one-liner can't
> > really explain what a more complext test does.
> I second that. For many cases one-liner will do - but these more complicated
> cases are really worth the effort when documenting.

I'm definitely not against documenting more involved testcases, e.g.
kms_frontbuffer_tracking. But this RFC here very much suggests a lot more
beaurocracy documenting everything, and not really some in-depth comments
where needed.

> > If we imagine what e.g. Paulo's test documentation in
> > kms_frontbuffer_tracking.c looks like, it'll be bad.
> > 
> > I thought the test documentation that Thomas Wood worked on (no idea about
> > status) would give us the full power and expressiveness of gtkdoc, but for
> > binaries. I think that's what we want.
> > 
> > Then for testcases I think we again want to follow the inline
> > documentation style, perhaps with something like the below:
> > 
> > 
> > /**
> >  * IGT-Subtest: tests some stuff
> >  *
> >  * Longer explanation of test approach and result evalution.
> >  *
> >  * Maybe over multiple paragraphs with headlines like CAVEATS, or
> >  * explaining hw bugs and stuff
> >  */
> > igt_subtest("bla-bla-subtest")
> > 
> > 
> > There's also the question of how to associate a given doc entry with more
> > the multiple subtest names that igt_subtest_f can generate, but I guess
> > that should be solveable.
> I don't think its even worth having same text with just single words changed
> generated for every subtest if test name describes the difference (and I guess
> in many cases that is what we have now). It would be good to document such 
> tests
> in groups.

Yes, I don't think it makes much sense to document every subtest. Some are
better documented as groups, many are just plain trivial (e.g.
kms_addfb_basic), and for others it might be better to comment the exact
test approach in-line in the code.
-Daniel

> 
> Thanks,
> Radek
> > 
> > Any, in my opinion documentation has to look pleasing, both in code and
> > rendered, otherwise people will not look at it, not write it and not
> > update it. Or worse, they stick to writing full comments, because that's
> > the only thing that allows them to express what they want to document.
> > 
> > We need something much better imo than this patch series from that pov.
> > 
> > Thanks, Daniel
> > 
> > > The documentation cannot be extracted from just comments, because
> > > associating them with the correct subtest name will then require doing
> > > pattern matching in the documentation generator, for subtests where
> > > the name is generated at runtime using igt_subtest_f.
> > > 
> > > v2: Rebase, change function name in commit message to match code
> > > 
> > > v3: Fix current documentation string tracking, add missing va_end (Vinay)
> > > 
> > > v4: Fix brainfart in __igt_run_subtest
> > > 
> > > Signed-off-by: Petri Latvala 
> > > Acked-by: Leo Li 
> > > Acked-by: Arkadiusz Hiler 
> > > ---
> > >  lib/igt_aux.c  |   8 ++--
> > >  lib/igt_core.c | 149 
> > > +-
> > > ---
> > >  lib/igt_core.h |   6 ++-
> > >  3 files changed, 128 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > > index f428f15..d56f41f 100644
> > > --- a/lib/igt_aux.c
> > > +++ b/lib/igt_aux.c
> > > @@ -311,7 +311,7 @@ static void sig_handler(int i)
> > >   */
> > >  void igt_fork_signal_helper(void)
> > >  {
> > > - 

Re: [Intel-gfx] [PATCH i-g-t 1/3] Use PATH_MAX to fix some sprintf-into-short-buffers warnings.

[Daniel Vetter]

On Sat, Sep 02, 2017 at 09:38:51PM -0700, Eric Anholt wrote:
> Signed-off-by: Eric Anholt 
> ---
> 
> This little series cleans up many compiler warnings I saw when testing
> danvet's meson branch.

On the series: Reviewed-by: Daniel Vetter 

I've pulled in your other meson patches, they need minimal polish (e.g. my
gcc doesn't have all the -W flags yours has, resulting in warnings about
unknown flags without first checking for them).
-Daniel
> 
>  lib/igt_debugfs.c   | 4 ++--
>  lib/igt_sysfs.c | 4 ++--
>  tests/kms_hdmi_inject.c | 2 +-
>  tests/pm_rpm.c  | 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 090b56e03555..63183e57229b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -491,7 +491,7 @@ static void igt_pipe_crc_reset(int drm_fd)
>   }
>  
>   while ((dirent = readdir(dir))) {
> - char buf[128];
> + char buf[PATH_MAX];
>  
>   if (strcmp(dirent->d_name, "crtc-") != 0)
>   continue;
> @@ -525,7 +525,7 @@ static void igt_pipe_crc_reset(int drm_fd)
>  static void pipe_crc_exit_handler(int sig)
>  {
>   struct dirent *dirent;
> - char buf[128];
> + char buf[PATH_MAX];
>   DIR *dir;
>   int fd;
>  
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 15ed34be0088..9227e374bf44 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -192,7 +192,7 @@ int igt_sysfs_open_parameters(int device)
>   if (params < 0) { /* builtin? */
>   drm_version_t version;
>   char name[32] = "";
> - char path[128];
> + char path[PATH_MAX];
>  
>   memset(, 0, sizeof(version));
>   version.name_len = sizeof(name);
> @@ -499,7 +499,7 @@ void kick_fbcon(bool enable)
>   return;
>  
>   while ((de = readdir(dir))) {
> - char buf[128];
> + char buf[PATH_MAX];
>   int fd, len;
>  
>   if (strncmp(de->d_name, "vtcon", 5))
> diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
> index cb916acec3c1..22570a4b637a 100644
> --- a/tests/kms_hdmi_inject.c
> +++ b/tests/kms_hdmi_inject.c
> @@ -170,7 +170,7 @@ eld_is_valid(void)
>   continue;
>  
>   while ((snd_hda = readdir(dir))) {
> - char fpath[128];
> + char fpath[PATH_MAX];
>  
>   if (*snd_hda->d_name == '.' ||
>   strstr(snd_hda->d_name, "eld") == 0)
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index 47c9f1143484..9e8cf79b5128 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -594,14 +594,14 @@ static int count_i2c_valid_edids(void)
>   DIR *dir;
>  
>   struct dirent *dirent;
> - char full_name[32];
> + char full_name[PATH_MAX];
>  
>   dir = opendir("/dev/");
>   igt_assert(dir);
>  
>   while ((dirent = readdir(dir))) {
>   if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
> - snprintf(full_name, 32, "/dev/%s", dirent->d_name);
> + sprintf(full_name, "/dev/%s", dirent->d_name);
>   fd = open(full_name, O_RDWR);
>   igt_assert_neq(fd, -1);
>   if (i2c_edid_is_valid(fd))
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH i-g-t 01/12] build: Define _GNU_SOURCE in Makefile.am

[Daniel Vetter]

On Sun, Sep 03, 2017 at 10:27:03AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-09-02 18:03:55)
> > In meson I want to just set this everywhere (no reason not to), and
> > doing so will allow us to clean up a few things.
> 
> GNU_SOURCE is defined in config.h, which should be included from all
> files (at least those that depend on system library foibles). I presume
> you intend to move all of config.h onto the command line?

Wrt including config.h, we seem to be rather inconsistent with that. So
what I opted for is to put stuff like that onto the cmdline, and generate
a minimal config.h with just the few #defines we actually use in the meson
build. That seems a robust approach, since if you use such a #define
without including config.h, you'll get a complain. GNU_SOURCE otoh doesn't
result in any complaints if you don't set it, but it is rather surprising
to have it in some files, but not in others. Hence why I figured it's best
to just add it to the gcc cmdline.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Daniel Vetter]

On Mon, Sep 04, 2017 at 11:28:09AM +0300, Arkadiusz Hiler wrote:
> On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> > We have it. Daniel Stone said it comes from the X11 transition to the
> > modular build.
> > 
> > Signed-off-by: Daniel Vetter 
> 
> It's also used for Android builds, which lack the generated config.h.
> 
> But is this even used nowadays by anyone?
> 
> How we are going to deal with the Android.mk with the transition to
> meson? Can we just drop them?
> 
> Android, in recent releases, is shifting from Makefiles to Soong.
> anyway. 

Looking at recent igt patches, we've stuffed all the fancy stuff (e.g.
chamelium) into Makefile.am, not Makefile.sources. And no one updated the
android build.

Judging from that, I think the android build is already firmly in bitrot
territory :-/

So yeah the plan would be to just leave it there, until someone cares
more. From a few googles it looks like meson might be able to
cross-compile into an android environment, but it won't be able to get
integrated into the android image build.

We could also try to generate the Makefile.sources files from special
meson targets (probably using the configuration generation stuff and a few
templates).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/5] Add support for subtest-specific documentation

[Szwichtenberg, Radoslaw]

On Fri, 2017-08-11 at 10:20 +0200, Daniel Vetter wrote:
> On Thu, Aug 10, 2017 at 01:26:47PM +0300, Petri Latvala wrote:
> > The current documentation for tests is limited to a single string per
> > test binary. This patch adds support for documenting individual
> > subtests.
> > 
> > The syntax for subtest documentation is:
> > 
> >    igt_document_subtest("Frob knobs to see if one of the "
> > "crossbeams will go out of skew on the "
> > "treadle.\n");
> >    igt_subtest("knob-frobbing-askew")
> >  test_frob();
> > 
> > or with a format string:
> > 
> >   for_example_loop(e) {
> > igt_document_subtest_f("Frob %s to see if one of the "
> >    "crossbeams will go out of skew on the "
> >    "treadle.\n", e->readable_name);
> > igt_subtest_f("%s-frob-askew", e->name)
> >   test_frob(e);
> >   }
> 
> I'm not sold on this layout at all. Everywhere else where we do in-line
> code documentation it is through specially formatted comments. That gives
> you a lot of freedom for plain-text layout, in combination with some mild
> markup (gtk-doc for igt and rst for kernel) that result in docs which both
> look good in the code and look good rendered.
> 
> This here essentially restricts you to one-liners, and a one-liner can't
> really explain what a more complext test does.
I second that. For many cases one-liner will do - but these more complicated
cases are really worth the effort when documenting.
> 
> If we imagine what e.g. Paulo's test documentation in
> kms_frontbuffer_tracking.c looks like, it'll be bad.
> 
> I thought the test documentation that Thomas Wood worked on (no idea about
> status) would give us the full power and expressiveness of gtkdoc, but for
> binaries. I think that's what we want.
> 
> Then for testcases I think we again want to follow the inline
> documentation style, perhaps with something like the below:
> 
> 
>   /**
>* IGT-Subtest: tests some stuff
>*
>* Longer explanation of test approach and result evalution.
>*
>* Maybe over multiple paragraphs with headlines like CAVEATS, or
>* explaining hw bugs and stuff
>*/
>   igt_subtest("bla-bla-subtest")
> 
> 
> There's also the question of how to associate a given doc entry with more
> the multiple subtest names that igt_subtest_f can generate, but I guess
> that should be solveable.
I don't think its even worth having same text with just single words changed
generated for every subtest if test name describes the difference (and I guess
in many cases that is what we have now). It would be good to document such tests
in groups.

Thanks,
Radek
> 
> Any, in my opinion documentation has to look pleasing, both in code and
> rendered, otherwise people will not look at it, not write it and not
> update it. Or worse, they stick to writing full comments, because that's
> the only thing that allows them to express what they want to document.
> 
> We need something much better imo than this patch series from that pov.
> 
> Thanks, Daniel
> 
> > The documentation cannot be extracted from just comments, because
> > associating them with the correct subtest name will then require doing
> > pattern matching in the documentation generator, for subtests where
> > the name is generated at runtime using igt_subtest_f.
> > 
> > v2: Rebase, change function name in commit message to match code
> > 
> > v3: Fix current documentation string tracking, add missing va_end (Vinay)
> > 
> > v4: Fix brainfart in __igt_run_subtest
> > 
> > Signed-off-by: Petri Latvala 
> > Acked-by: Leo Li 
> > Acked-by: Arkadiusz Hiler 
> > ---
> >  lib/igt_aux.c  |   8 ++--
> >  lib/igt_core.c | 149 +-
> > ---
> >  lib/igt_core.h |   6 ++-
> >  3 files changed, 128 insertions(+), 35 deletions(-)
> > 
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index f428f15..d56f41f 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -311,7 +311,7 @@ static void sig_handler(int i)
> >   */
> >  void igt_fork_signal_helper(void)
> >  {
> > -   if (igt_only_list_subtests())
> > +   if (igt_only_collect_data())
> >     return;
> >  
> >     /* We pick SIGCONT as it is a "safe" signal - if we send SIGCONT to
> > @@ -344,7 +344,7 @@ void igt_fork_signal_helper(void)
> >   */
> >  void igt_stop_signal_helper(void)
> >  {
> > -   if (igt_only_list_subtests())
> > +   if (igt_only_collect_data())
> >     return;
> >  
> >     igt_stop_helper(_helper);
> > @@ -375,7 +375,7 @@ static void __attribute__((noreturn))
> > shrink_helper_process(int fd, pid_t pid)
> >   */
> >  void igt_fork_shrink_helper(int drm_fd)
> >  {
> > -   assert(!igt_only_list_subtests());
> > +   assert(!igt_only_collect_data());
> >     igt_require(igt_drop_caches_has(drm_fd, DROP_SHRINK_ALL));
> >     

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Daniel Vetter]

On Sun, Sep 03, 2017 at 10:29:06AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-09-02 18:03:56)
> > We have it. Daniel Stone said it comes from the X11 transition to the
> > modular build.
> 
> What? It's from AC_CONFIG_HEADERS([config.h])
> 
> We tell autoconf to build it so it doesn't have to put all the build
> flags on the command line.

Yes I know where it's from. What I mean is why do we include-guard this,
and Daniel Stone said very old versions of X didn't have config.h (because
they used imake or whatever the horror was again).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Try harder to finish the idle-worker

[Daniel Vetter]

On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> If a worker requeues itself, it may switch to a different kworker pool,
> which flush_work() considers as complete. To be strict, we then need to
> keep flushing the work until it is no longer pending.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 

Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
al.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c |  3 +--
>  drivers/gpu/drm/i915/i915_utils.h   | 13 +
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 48572b157222..1dd687a74879 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4202,8 +4202,7 @@ fault_irq_set(struct drm_i915_private *i915,
>   mutex_unlock(>drm.struct_mutex);
>  
>   /* Flush idle worker to disarm irq */
> - while (flush_delayed_work(>gt.idle_work))
> - ;
> + drain_delayed_work(>gt.idle_work);
>  
>   return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08bc518c..1829d3fa15d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4582,8 +4582,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   /* As the idle_work is rearming if it detects a race, play safe and
>* repeat the flush until it is definitely idle.
>*/
> - while (flush_delayed_work(_priv->gt.idle_work))
> - ;
> + drain_delayed_work(_priv->gt.idle_work);
>  
>   /* Assert that we sucessfully flushed all the work and
>* reset the GPU back to its idle, low power state.
> diff --git a/drivers/gpu/drm/i915/i915_utils.h 
> b/drivers/gpu/drm/i915/i915_utils.h
> index 12fc250b47b9..4f7ffa0976b1 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -119,4 +119,17 @@ static inline void __list_del_many(struct list_head 
> *head,
>   WRITE_ONCE(head->next, first);
>  }
>  
> +/*
> + * Wait until the work is finally complete, even if it tries to postpone
> + * by requeueing itself. Note, that if the worker never cancels itself,
> + * we will spin forever.
> + */
> +static inline void drain_delayed_work(struct delayed_work *dw)
> +{
> + do {
> + while (flush_delayed_work(dw))
> + ;
> + } while (delayed_work_pending(dw));
> +}
> +
>  #endif /* !__I915_UTILS_H */
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH i-g-t] scripts/run-tests.sh: Use piglit's --ignore-missing

[Daniel Vetter]

On Fri, Sep 01, 2017 at 04:09:03PM +0300, Arkadiusz Hiler wrote:
> Recently we added a number of chamelium tests to the fast-feedback testlist.
> 
> Chemelium is build-optional - requires  `./configure --enable-chamelium`.
> 
> To mitigate issue with piglit exiting abruptly due to the (possibly)
> missing test binaries, this makes it behave more gracefuly, considering
> those as simply "notrun".
> 
> Cc: Petri Latvala 
> Signed-off-by: Arkadiusz Hiler 

Reviewed-by: Daniel Vetter 

> ---
>  scripts/run-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh
> index 7b8de74a..a28dd876 100755
> --- a/scripts/run-tests.sh
> +++ b/scripts/run-tests.sh
> @@ -129,7 +129,7 @@ if [ "x$RESUME" != "x" ]; then
>   sudo IGT_TEST_ROOT="$IGT_TEST_ROOT" IGT_CONFIG_PATH="$IGT_CONFIG_PATH" 
> "$PIGLIT" resume "$RESULTS" $NORETRY
>  else
>   mkdir -p "$RESULTS"
> - sudo IGT_TEST_ROOT="$IGT_TEST_ROOT" IGT_CONFIG_PATH="$IGT_CONFIG_PATH" 
> "$PIGLIT" run igt -o "$RESULTS" -s $VERBOSE $EXCLUDE $FILTER
> + sudo IGT_TEST_ROOT="$IGT_TEST_ROOT" IGT_CONFIG_PATH="$IGT_CONFIG_PATH" 
> "$PIGLIT" run igt --ignore-missing -o "$RESULTS" -s $VERBOSE $EXCLUDE $FILTER
>  fi
>  
>  if [ "$SUMMARY" == "html" ]; then
> -- 
> 2.13.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH i-g-t v2] tests/gem_flink_basic: Add documentation for subtests

[Daniel Vetter]

On Thu, Aug 31, 2017 at 02:33:23PM -0700, Vinay Belgaumkar wrote:
> Added the missing IGT_TEST_DESCRIPTION and some subtest
> descriptions. Trying to establish a method to document
> subtests, it should describe the feature being tested
> rather than how. The HOW part can, if needed, be
> described in the test code.
> 
> Documenting subtests will give us a good way to trace
> feature test coverage, and also help a faster ramp
> for understanding the test code.
> 
> v2: Removed duplication, addressed comments, cc'd test author
> 
> Cc: Michał Winiarski 
> Cc: Eric Anholt 
> 
> Signed-off-by: Vinay Belgaumkar 
> ---
>  tests/gem_flink_basic.c | 36 ++--
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
> index 26ae7d6..9c8c4c3 100644
> --- a/tests/gem_flink_basic.c
> +++ b/tests/gem_flink_basic.c
> @@ -36,6 +36,8 @@
>  #include 
>  #include "drm.h"
>  
> +IGT_TEST_DESCRIPTION("Tests for flink - a way to export a gem object by 
> name");
> +
>  static void
>  test_flink(int fd)
>  {
> @@ -44,8 +46,6 @@ test_flink(int fd)
>   struct drm_gem_open open_struct;
>   int ret;
>  
> - igt_info("Testing flink and open.\n");

Do we really want to remove runtime-dumped information (maybe we could
tune it down to debug level) with comments that in my experience, no one
ever reads?

I just looked again at the igt library docs, and like last year when I've
done that, we've accumulated sizeable chunks of missing docs and warnings.
So who's going to maintain this? We can barely keep docs in shape for the
core libs, how exactly are we going to keep docs in shape for the hundreds
of testcases we have? Do you have a team of 10+ people working on this?

Same about IGT_TEST_DESCRIPTION, not enough people seem to care even for
that simple one-liner to make it work.

My proposal would be that we first try to get the docs we have into decent
shape, which means establishing as something everyone takes care of.
Adding more lofty documentation goals that won't pan out imo just doesn't
make much sense.

And yes this is from the person who did push for docs almost everywhere.
It's hard work, and it's a lot of hard work.
-Daniel

> -
>   memset(, 0, sizeof(create));
>   create.size = 16 * 1024;
>   ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, );
> @@ -69,8 +69,6 @@ test_double_flink(int fd)
>   struct drm_gem_flink flink2;
>   int ret;
>  
> - igt_info("Testing repeated flink.\n");
> -
>   memset(, 0, sizeof(create));
>   create.size = 16 * 1024;
>   ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, );
> @@ -92,8 +90,6 @@ test_bad_flink(int fd)
>   struct drm_gem_flink flink;
>   int ret;
>  
> - igt_info("Testing error return on bad flink ioctl.\n");
> -
>   flink.handle = 0x10101010;
>   ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, );
>   igt_assert(ret == -1 && errno == ENOENT);
> @@ -105,8 +101,6 @@ test_bad_open(int fd)
>   struct drm_gem_open open_struct;
>   int ret;
>  
> - igt_info("Testing error return on bad open ioctl.\n");
> -
>   open_struct.name = 0x10101010;
>   ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, _struct);
>  
> @@ -121,8 +115,6 @@ test_flink_lifetime(int fd)
>   struct drm_gem_open open_struct;
>   int ret, fd2;
>  
> - igt_info("Testing flink lifetime.\n");
> -
>   fd2 = drm_open_driver(DRIVER_INTEL);
>  
>   memset(, 0, sizeof(create));
> @@ -134,11 +126,13 @@ test_flink_lifetime(int fd)
>   ret = ioctl(fd2, DRM_IOCTL_GEM_FLINK, );
>   igt_assert_eq(ret, 0);
>  
> + /* Open another reference to the gem object */
>   open_struct.name = flink.name;
>   ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, _struct);
>   igt_assert_eq(ret, 0);
>   igt_assert(open_struct.handle != 0);
>  
> + /* Before closing the previous one */
>   close(fd2);
>   fd2 = drm_open_driver(DRIVER_INTEL);
>  
> @@ -155,14 +149,36 @@ igt_main
>   igt_fixture
>   fd = drm_open_driver(DRIVER_INTEL);
>  
> + /**
> +  * basic:
> +  * Test creation and use of flink.
> +  */
>   igt_subtest("basic")
>   test_flink(fd);
> +
> + /**
> +  * double-flink:
> +  * This test validates the ability to create multiple flinks
> +  * for the same gem object. They should obtain the same name.
> +  */
>   igt_subtest("double-flink")
>   test_double_flink(fd);
> +
> + /**
> +  * bad-flink:
> +  * Negative test for invalid flink usage.
> +  */
>   igt_subtest("bad-flink")
>   test_bad_flink(fd);
> +
>   igt_subtest("bad-open")
>   test_bad_open(fd);
> +
> + /**
> +  * flink-lifetime:
> +  * Flink lifetime is limited to that of the gem object it
> +  * points to.
> +  */
>   igt_subtest("flink-lifetime")
>   

Re: [Intel-gfx] [PATCH i-g-t 02/12] build: Nuke #ifdef HAVE_CONFIG_H cargo-cult

[Arkadiusz Hiler]

On Sat, Sep 02, 2017 at 07:03:56PM +0200, Daniel Vetter wrote:
> We have it. Daniel Stone said it comes from the X11 transition to the
> modular build.
> 
> Signed-off-by: Daniel Vetter 

It's also used for Android builds, which lack the generated config.h.

But is this even used nowadays by anyone?

How we are going to deal with the Android.mk with the transition to
meson? Can we just drop them?

Android, in recent releases, is shifting from Makefiles to Soong.
anyway. 

-- 
Cheers,
Arek
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm: Fix example comment of format modifier blob

[Daniel Vetter]

On Thu, Aug 31, 2017 at 01:00:35PM -0700, Ben Widawsky wrote:
> On 17-08-31 16:52:14, Gabriel Krisman Bertazi wrote:
> > To represent formats 98-102, the supported formats mask must be
> > 0x7c and not 0x3c.
> > 
> > Signed-off-by: Gabriel Krisman Bertazi 
> > ---
> > include/uapi/drm/drm_mode.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 54fc38c3c3f1..34b6bb34b002 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -749,9 +749,9 @@ struct drm_format_modifier {
> >  * If the number formats grew to 128, and formats 98-102 are
> >  * supported with the modifier:
> >  *
> > -* 0x003c 
> > +* 0x007c 
> >  *^
> > -*|__offset = 64, formats = 0x3c
> > +*|__offset = 64, formats = 0x7c
> >  *
> >  */
> > __u64 formats;
> > -- 
> 
> > > > hex(0x1f << 98)
> '0x7c'
> Reviewed-by: Ben Widawsky 

Applied to drm-misc-next, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fail addfb ioctl if color and CCS buffers overlap

[Daniel Vetter]

On Thu, Aug 31, 2017 at 04:52:15PM -0300, Gabriel Krisman Bertazi wrote:
> With this patch the new testcase igt@kms_ccs@pipe-X-invalid-ccs-offset
> succeeds.
> 
> Signed-off-by: Gabriel Krisman Bertazi 

Do we have igts for this? If so, please add a Testcase: line.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b28f076f98bc..ff1ed67a9eff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13989,6 +13989,11 @@ static int intel_framebuffer_init(struct 
> intel_framebuffer *intel_fb,
>   DRM_DEBUG_KMS("RC supported only with RGB 
> formats\n");
>   goto err;
>   }
> +
> + if (mode_cmd->offsets[1] < mode_cmd->pitches[0]) {
> + DRM_DEBUG_KMS("CCS and color buffers overlap\n");
> + return -EINVAL;
> + }
>   /* fall through */
>   case I915_FORMAT_MOD_Y_TILED:
>   case I915_FORMAT_MOD_Yf_TILED:
> -- 
> 2.11.0
> 

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

Re: [Intel-gfx] [PATCH v3] drm/i915: Keep the device awake whilst in the GTT domain

[Daniel Vetter]

On Fri, Sep 01, 2017 at 01:16:06PM +0100, Chris Wilson wrote:
> Since runtime suspend is very harsh on GTT mmappings (they all get
> zapped on suspend) keep the device awake while the buffer remains in
> the GTT domain. However, userspace can control the domain and
> although there is a soft contract that writes must be flushed (for e.g.
> flushing scanouts and fbc), we are intentionally lax with respect to read
> domains, allowing them to persist for as long as is feasible.
> 
> We acquire a wakeref when using the buffer in the GEM domain so that all
> subsequent operations on that object are fast, trying to avoid
> suspending while the GTT is still in active use by userspace.  To ensure
> that the device can eventually suspend, we install a timer and expire the
> GTT wakeref. So in effect we have just a fancy pm autosuspend that tries
> to estimate the cost of restoring actively used GTT mmapping.

Please tag with for-CI or something like that when throwing patches at
the shards :-) At least that's what I assuming given lack of sob and
revision of changes ...

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |   2 +
>  drivers/gpu/drm/i915/i915_drv.h  |  11 
>  drivers/gpu/drm/i915/i915_gem.c  | 103 
> ---
>  drivers/gpu/drm/i915/i915_gem_object.h   |   5 ++
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |   4 +-
>  drivers/gpu/drm/i915/intel_lrc.c |   1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c  |   3 +
>  7 files changed, 118 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 48572b157222..dbb07612aa5a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2809,6 +2809,8 @@ static int i915_runtime_pm_status(struct seq_file *m, 
> void *unused)
>   if (!HAS_RUNTIME_PM(dev_priv))
>   seq_puts(m, "Runtime power management not supported\n");
>  
> + seq_printf(m, "GTT wakeref count: %d\n",
> +atomic_read(_priv->mm.gtt_wakeref.count));
>   seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->gt.awake));
>   seq_printf(m, "IRQs disabled: %s\n",
>  yesno(!intel_irqs_enabled(dev_priv)));
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0383e879a315..14dcf6614f3c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1460,6 +1460,17 @@ struct i915_gem_mm {
>*/
>   struct list_head userfault_list;
>  
> + /* List of all objects in gtt domain, holding a wakeref.
> +  * The list is reaped periodically, and protected by its own mutex.
> +  */
> + struct {
> + struct mutex lock;
> + struct list_head list;
> + atomic_t count;
> +
> + struct delayed_work work;
> + } gtt_wakeref;
> +
>   /**
>* List of objects which are pending destruction.
>*/
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08bc518c..09baf80889e8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -235,6 +235,7 @@ i915_gem_object_get_pages_phys(struct drm_i915_gem_object 
> *obj)
>  
>  static void __start_cpu_write(struct drm_i915_gem_object *obj)
>  {
> + GEM_BUG_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
>   obj->base.read_domains = I915_GEM_DOMAIN_CPU;
>   obj->base.write_domain = I915_GEM_DOMAIN_CPU;
>   if (cpu_write_needs_clflush(obj))
> @@ -667,11 +668,13 @@ fb_write_origin(struct drm_i915_gem_object *obj, 
> unsigned int domain)
>   obj->frontbuffer_ggtt_origin : ORIGIN_CPU);
>  }
>  
> -static void
> +void
>  flush_write_domain(struct drm_i915_gem_object *obj, unsigned int 
> flush_domains)
>  {
>   struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>  
> + lockdep_assert_held(_priv->drm.struct_mutex);
> +
>   if (!(obj->base.write_domain & flush_domains))
>   return;
>  
> @@ -694,16 +697,19 @@ flush_write_domain(struct drm_i915_gem_object *obj, 
> unsigned int flush_domains)
>  
>   switch (obj->base.write_domain) {
>   case I915_GEM_DOMAIN_GTT:
> - if (INTEL_GEN(dev_priv) >= 6 && !HAS_LLC(dev_priv)) {
> - intel_runtime_pm_get(dev_priv);
> - spin_lock_irq(_priv->uncore.lock);
> - 
> POSTING_READ_FW(RING_ACTHD(dev_priv->engine[RCS]->mmio_base));
> - spin_unlock_irq(_priv->uncore.lock);
> - intel_runtime_pm_put(dev_priv);
> - }
> + mutex_lock(_priv->mm.gtt_wakeref.lock);
> + if (!list_empty(>mm.gtt_wakeref_link)) {
> + if (INTEL_GEN(dev_priv) >= 6 && !HAS_LLC(dev_priv)) {
> + spin_lock_irq(_priv->uncore.lock);
> + 
> 

Re: [Intel-gfx] [PATCH] drm/i915: Disable DRRS when PSR is enabled

[Daniel Vetter]

On Thu, Aug 31, 2017 at 05:37:31PM +, Sripada, Radhakrishna wrote:
> 
> 
> > -Original Message-
> > From: Vivi, Rodrigo
> > Sent: Wednesday, August 30, 2017 5:59 PM
> > To: Sripada, Radhakrishna 
> > Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> > ; Nikula, Jani ;
> > Taylor, Clinton A ; nicholas.stom...@gmail.com
> > Subject: Re: [PATCH] drm/i915: Disable DRRS when PSR is enabled
> > 
> > On Wed, 2017-08-30 at 17:32 -0700, Radhakrishna Sripada wrote:
> > > Some platforms donot support PSR and DRRS simultaneously. Deferring to
> > > PSR when both PSR and DRRS are supported by the panel.
> > >
> > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=10
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=10
> > 
> > "Fixes: " is only used to -fixes cherry-picks. Not a case for this
> > patch.
> Got it. Will update in the next revision of the patch.

Did you check igt coverage for this and make sure those platforms do blow
up somewhere?

We currently don't yet run the full panel tests (psr, drrs) in CI, but
we're slowly working on that problem too. Would be good to have the
testsuite ready already.
-Daniel

> > 
> > > Cc: Nicholas Stommel 
> > > Cc: Dhinakaran Pandiyan 
> > > Cc: Jani Nikula 
> > > Cc: Clinton Taylor 
> > > Cc: Rodrigo Vivi 
> > > Signed-off-by: Radhakrishna Sripada 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 10 +-
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c index d3e5fdf0d2fa..dc7a6721e0dd
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5469,11 +5469,6 @@ static void intel_dp_set_drrs_state(struct
> > drm_i915_private *dev_priv,
> > >   return;
> > >   }
> > >
> > > - /*
> > > -  * FIXME: This needs proper synchronization with psr state for some
> > > -  * platforms that cannot have PSR and DRRS enabled at the same
> > time.
> > > -  */
> > > -
> > >   dig_port = dp_to_dig_port(intel_dp);
> > >   encoder = _port->base;
> > >   intel_crtc = to_intel_crtc(encoder->base.crtc);
> > > @@ -5557,6 +5552,11 @@ void intel_edp_drrs_enable(struct intel_dp
> > *intel_dp,
> > >   return;
> > >   }
> > >
> > > + if (dev_priv->psr.enabled != NULL) {
> > 
> > if (dev_priv->psr.enabled) {
> > ?
> This looks cleaner will use this in the follow up patch.
> > 
> > > + DRM_DEBUG_KMS("PSR active. Disabling DRRS.\n");
> > > + return;
> > > + }
> > > +
> > >   mutex_lock(_priv->drrs.mutex);
> > >   if (WARN_ON(dev_priv->drrs.dp)) {
> > >   DRM_ERROR("DRRS already enabled\n");
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] drm/i915: Skip waking the device to service pwrite

[Daniel Vetter]

On Wed, Aug 30, 2017 at 06:48:19PM +0100, Chris Wilson wrote:
> If the device is in runtime suspend, resuming takes time and reduces our
> powersaving. If this was for a small write into an object, that resume
> will take longer than any savings in using the indirect GGTT access to
> avoid the cpu cache.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 21 ++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 93dfa793975a..8940a6873ca5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1229,7 +1229,21 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object 
> *obj,
>   if (ret)
>   return ret;
>  
> - intel_runtime_pm_get(i915);
> + if (i915_gem_object_has_struct_page(obj)) {

I don't really see why we need to check for has_struct_page here (we do
already outside the lock grabbing), and why if that's not the case we hit
the slow-path?

I'd have expected a simple s/pm_get/pm_get_if_in_use/ ...
-Daniel

> + /* Avoid waking the device up if we can fallback, as
> +  * waking/resuming is very slow (10-100 ms depending
> +  * on PCI sleeps and our own resume time). This easily
> +  * dwarfs any performance advantage from using the
> +  * cache bypass of indirect GGTT access.
> +  */
> + if (!intel_runtime_pm_get_if_in_use(i915)) {
> + ret = -EFAULT;
> + goto out_unlock;
> + }
> + } else {
> + intel_runtime_pm_get(i915);
> + }
> +
>   vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
>  PIN_MAPPABLE | PIN_NONBLOCK);
>   if (!IS_ERR(vma)) {
> @@ -1244,7 +1258,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object 
> *obj,
>   if (IS_ERR(vma)) {
>   ret = insert_mappable_node(ggtt, , PAGE_SIZE);
>   if (ret)
> - goto out_unlock;
> + goto out_rpm;
>   GEM_BUG_ON(!node.allocated);
>   }
>  
> @@ -1307,8 +1321,9 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object 
> *obj,
>   } else {
>   i915_vma_unpin(vma);
>   }
> -out_unlock:
> +out_rpm:
>   intel_runtime_pm_put(i915);
> +out_unlock:
>   mutex_unlock(>drm.struct_mutex);
>   return ret;
>  }
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_aux: Allow sysfs open to fail when setting suspend/resume delay

[Daniel Vetter]

On Wed, Aug 30, 2017 at 09:15:03PM +0300, Arkadiusz Hiler wrote:
> On Wed, Aug 30, 2017 at 04:56:09PM +0300, Paul Kocialkowski wrote:
> > This removes the igt_require condition on the sysfs open call used to
> > write the suspend/resume delay so that it is allowed to fail. Intsead,
> > the code that depends on it is put in a conditional block.
> > 
> > This allows running test binaries as a non-privileged user for e.g.
> > listing the available tests with the SuspendResumeDelay parameter set
> > in igtrc configuration. Sysfs access would otherwise cause it to fail.
> > 
> > Signed-off-by: Paul Kocialkowski 
> Reviewed-by: Arkadiusz Hiler 

This is the wrong fix. When enumerating tests you're not supposed to touch
anything hw or system related at all. Allocating a bit of memory and stuff
like that is ok, but not anything with effects.

The correct way to handle this is by wrapping this into an igt_fixture
block. Hitting an igt_require/assert outside of an igt_fixture or
igt_subtest should result in a assert.

You still have igt_skip and igt_require in there, so not fixed properly
yet.

Aside: We need to come up with a way to have functions shared between
parts of the library, without documenting and exporting them to tests.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Add interface to reserve fence registers for vGPU

[changbin . du]

From: Changbin Du 

In the past, vGPU alloc fence registers by walking through mm.fence_list
to find fence which pin_count = 0 and vma is empty. vGPU may not find
enough fence registers this way. Because a fence can be bind to vma even
though it is not in using. We have found such failure many times these
days.

An option to resolve this issue is that we can force-remove fence from
vma in this case.

This patch added two new api to the fence management code:
 - i915_reserve_fence() will try to find a free fence from fence_list
   and force-remove vma if need.
 - i915_unreserve_fence() reclaim a reserved fence after vGPU has
   finished.

With this change, the fence management is more clear to work with vGPU.
GVTg do not need remove fence from fence_list in private.

v3: (Chris)
  - Add struct_mutex lock assertion.
  - Only count for unpinned fence.

v2: (Chris)
  - Rename the new api for symmetry.
  - Add safeguard to ensure at least 1 fence remained for host display.

Signed-off-by: Changbin Du 
Reviewed-by: Chris Wilson 
---
 drivers/gpu/drm/i915/gvt/aperture_gm.c| 26 +++-
 drivers/gpu/drm/i915/i915_drv.h   |  3 ++
 drivers/gpu/drm/i915/i915_gem_fence_reg.c | 49 +++
 3 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/aperture_gm.c 
b/drivers/gpu/drm/i915/gvt/aperture_gm.c
index ca3d192..7c9ec4f 100644
--- a/drivers/gpu/drm/i915/gvt/aperture_gm.c
+++ b/drivers/gpu/drm/i915/gvt/aperture_gm.c
@@ -173,8 +173,8 @@ static void free_vgpu_fence(struct intel_vgpu *vgpu)
_clear_vgpu_fence(vgpu);
for (i = 0; i < vgpu_fence_sz(vgpu); i++) {
reg = vgpu->fence.regs[i];
-   list_add_tail(>link,
- _priv->mm.fence_list);
+   i915_unreserve_fence(reg);
+   vgpu->fence.regs[i] = NULL;
}
mutex_unlock(_priv->drm.struct_mutex);
 
@@ -187,24 +187,19 @@ static int alloc_vgpu_fence(struct intel_vgpu *vgpu)
struct drm_i915_private *dev_priv = gvt->dev_priv;
struct drm_i915_fence_reg *reg;
int i;
-   struct list_head *pos, *q;
 
intel_runtime_pm_get(dev_priv);
 
/* Request fences from host */
mutex_lock(_priv->drm.struct_mutex);
-   i = 0;
-   list_for_each_safe(pos, q, _priv->mm.fence_list) {
-   reg = list_entry(pos, struct drm_i915_fence_reg, link);
-   if (reg->pin_count || reg->vma)
-   continue;
-   list_del(pos);
+
+   for (i = 0; i < vgpu_fence_sz(vgpu); i++) {
+   reg = i915_reserve_fence(dev_priv);
+   if (IS_ERR(reg))
+   goto out_free_fence;
+
vgpu->fence.regs[i] = reg;
-   if (++i == vgpu_fence_sz(vgpu))
-   break;
}
-   if (i != vgpu_fence_sz(vgpu))
-   goto out_free_fence;
 
_clear_vgpu_fence(vgpu);
 
@@ -212,13 +207,14 @@ static int alloc_vgpu_fence(struct intel_vgpu *vgpu)
intel_runtime_pm_put(dev_priv);
return 0;
 out_free_fence:
+   gvt_vgpu_err("Failed to alloc fences\n");
/* Return fences to host, if fail */
for (i = 0; i < vgpu_fence_sz(vgpu); i++) {
reg = vgpu->fence.regs[i];
if (!reg)
continue;
-   list_add_tail(>link,
- _priv->mm.fence_list);
+   i915_unreserve_fence(reg);
+   vgpu->fence.regs[i] = NULL;
}
mutex_unlock(_priv->drm.struct_mutex);
intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 064bf0f..7c35d57 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3673,6 +3673,9 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
 /* i915_gem_fence_reg.c */
 int __must_check i915_vma_get_fence(struct i915_vma *vma);
 int __must_check i915_vma_put_fence(struct i915_vma *vma);
+struct drm_i915_fence_reg *
+i915_reserve_fence(struct drm_i915_private *dev_priv);
+void i915_unreserve_fence(struct drm_i915_fence_reg *fence);
 
 void i915_gem_revoke_fences(struct drm_i915_private *dev_priv);
 void i915_gem_restore_fences(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c 
b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index 5fe2cd8..dc39758 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -360,6 +360,55 @@ i915_vma_get_fence(struct i915_vma *vma)
 }
 
 /**
+ * i915_reserve_fence - Reserve a fence for vGPU
+ * @dev_priv: i915 device private
+ *
+ * This function walks the fence regs looking for a free one and remove
+ * it from the fence_list. It is used to reserve fence for vGPU to use.
+ */
+struct drm_i915_fence_reg *

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[Daniel Vetter]

On Fri, Sep 01, 2017 at 04:44:38PM -0300, Paulo Zanoni wrote:
> Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> > Macros that should be C functions but aren't are really hard to
> > read and confusing. Convert them over.
> > 
> > v2: Clean up commit message and keep printing the line numbers
> > (Paulo).
> > 
> > v3: Actually git add (silly me).
> > 
> > Cc: Paulo Zanoni 
> > Signed-off-by: Daniel Vetter 
> 
> Thanks for doing that. Works for me this way:
> Reviewed-by: Paulo Zanoni 

Thanks, pushed.

> But I'm still waiting for the patch that removes those bogus line
> numbers in every error message we print :).

Hm, which bogus line numbers where?
-Daniel
> 
> > ---
> >  tests/kms_frontbuffer_tracking.c | 171 -
> > --
> >  1 file changed, 89 insertions(+), 82 deletions(-)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e03524f1c45b..a068c8afb6d8 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1677,88 +1677,95 @@ static int adjust_assertion_flags(const
> > struct test_mode *t, int flags)
> >     return flags;
> >  }
> >  
> > -#define do_crc_assertions(flags, mandatory_sink_crc) do {  
> > \
> > -   int flags__ = (flags);  
> > \
> > -   struct both_crcs crc_;  
> > \
> > -   
> > \
> > -   if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))  
> > \
> > -   break;  
> > \
> > -   
> > \
> > -   collect_crcs(_, mandatory_sink_crc);
> > \
> > -   print_crc("Calculated CRC:", _);
> > \
> > -   
> > \
> > -   igt_assert(wanted_crc); 
> > \
> > -   igt_assert_crc_equal(_.pipe, _crc->pipe);
> > \
> > -   if (mandatory_sink_crc) 
> > \
> > -   assert_sink_crc_equal(_.sink, _crc-
> > >sink); \
> > -   else if (sink_crc.reliable &&   
> > \
> > -    !sink_crc_equal(_.sink, _crc->sink))
> > \
> > -   igt_info("Sink CRC differ, but not required\n");    
> > \
> > -} while (0)
> > -
> > -#define do_status_assertions(flags_) do {  
> > \
> > -   if (!opt.check_status) {
> > \
> > -   /* Make sure we settle before continuing. */
> > \
> > -   sleep(1);   
> > \
> > -   break;  
> > \
> > -   }   
> > \
> > -   
> > \
> > -   if (flags_ & ASSERT_FBC_ENABLED) {  
> > \
> > -   igt_require(!fbc_not_enough_stolen());  
> > \
> > -   igt_require(!fbc_stride_not_supported());   
> > \
> > -   if (!fbc_wait_until_enabled()) {
> > \
> > -   fbc_print_status(); 
> > \
> > -   igt_assert_f(false, "FBC disabled\n");  
> > \
> > -   }   
> > \
> > -   
> > \
> > -   if (opt.fbc_check_compression)  
> > \
> > -   igt_assert(fbc_wait_for_compression()); 
> > \
> > -   } else if (flags_ & ASSERT_FBC_DISABLED) {  
> > \
> > -   igt_assert(!fbc_wait_until_enabled());  
> > \
> > -   }   
> > \
> > -   
> > \
> > -   if (flags_ & ASSERT_PSR_ENABLED) {  
> > \
> > -   if (!psr_wait_until_enabled()) {
> > \
> > -   psr_print_status(); 
> > \
> > -   igt_assert_f(false, "PSR disabled\n");  
> > \
> > -   }   
> > \
> > -   } else if (flags_ & ASSERT_PSR_DISABLED) {  
> > \
> > -   igt_assert(!psr_wait_until_enabled());  
> > \
> > -   }   
> > \
> > -} while (0)
> > -
> > -#define do_assertions(flags) do {  
> > \
> > -   int flags_ = adjust_assertion_flags(t, (flags)); 

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dsi: Send SHUTDOWN only for v3+ VBT's

[Mika Kahola]

On Fri, 2017-09-01 at 16:43 +0300, Ville Syrjälä wrote:
> On Fri, Sep 01, 2017 at 10:51:00AM +0300, Mika Kahola wrote:
> > 
> > According to spec we should send SHUTDOWN before
> > MIPI_SEQ_DISPLAY_OFF for
> > v3+ VBT's. Testing with VBT v3 the current implementation yields
> > the
> > following error message
> > 
> > *ERROR* Video mode command 0x0041 send failed.
> > 
> > To get rid of this error message, let's limit SHUTDOWN only for VBT
> > versions 3 or higher.
> In the patch you limit it to version 4+, which doesn't make sense
> since
> AFAIK there is no version 4 of the sequence block.
It seems that sending SHUTDOWN signal doesn't make any sense either.
Whenever we send that signal it just causes this error message. Do we
really need to signal this? From functionality point of view there's no
difference.

> 
> > 
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102404
> > Signed-off-by: Mika Kahola 
> > ---
> >  drivers/gpu/drm/i915/intel_dsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> > b/drivers/gpu/drm/i915/intel_dsi.c
> > index 2a0f5d3..b48b9b7 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -916,7 +916,7 @@ static void intel_dsi_disable(struct
> > intel_encoder *encoder,
> >      * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field
> > testing
> >      * has shown that the v3 sequence works for v2 VBTs too
> >      */
> > -   if (is_vid_mode(intel_dsi)) {
> > +   if (is_vid_mode(intel_dsi) && dev_priv-
> > >vbt.dsi.seq_version > 3) {
> >     /* Send Shutdown command to the panel in LP mode
> > */
> >     for_each_dsi_port(port, intel_dsi->ports)
> >     dpi_send_cmd(intel_dsi, SHUTDOWN, false,
> > port);
> > -- 
> > 2.7.4
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Mika Kahola - Intel OTC

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

Re: [Intel-gfx] [RFC PATCH 1/5] drm/i915: Always wait for flip_done

[Maarten Lankhorst]

Op 04-09-17 om 09:30 schreef Daniel Vetter:
> On Thu, Aug 31, 2017 at 05:18:06PM +0200, Maarten Lankhorst wrote:
>> Op 30-08-17 om 15:40 schreef Daniel Vetter:
>>> On Wed, Aug 30, 2017 at 02:54:28PM +0200, Maarten Lankhorst wrote:
 Op 30-08-17 om 14:43 schreef Daniel Vetter:
> On Wed, Aug 30, 2017 at 02:17:48PM +0200, Maarten Lankhorst wrote:
>> The next commit removes the wait for flip_done in in
>> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
>> to pass. Instead of using complicated vblank tracking which ends
>> up being ignored anyway, call the correct atomic helper. :)
>>
>> RFC because I'm not completely sure what we want to do with the vblank 
>> waiting,
>> I think for now this patch is the right way to go until we decide 
>> because it
>> preserves the status quo when drm_crtc_commit was introduced.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  3 +-
>>  drivers/gpu/drm/i915/intel_display.c | 83 
>> +++-
>>  2 files changed, 8 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index cbbafbfb0a55..de19621864a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -707,8 +707,7 @@ struct drm_i915_display_funcs {
>>  struct drm_atomic_state *old_state);
>>  void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>>   struct drm_atomic_state *old_state);
>> -void (*update_crtcs)(struct drm_atomic_state *state,
>> - unsigned int *crtc_vblank_mask);
>> +void (*update_crtcs)(struct drm_atomic_state *state);
>>  void (*audio_codec_enable)(struct drm_connector *connector,
>> struct intel_encoder *encoder,
>> const struct drm_display_mode 
>> *adjusted_mode);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 52c73b4dabaa..3f3cb96aa11e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12114,73 +12114,10 @@ u32 intel_crtc_get_vblank_counter(struct 
>> intel_crtc *crtc)
>>  return dev->driver->get_vblank_counter(dev, crtc->pipe);
>>  }
>>  
>> -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
>> -  struct drm_i915_private 
>> *dev_priv,
>> -  unsigned crtc_mask)
>> -{
>> -unsigned last_vblank_count[I915_MAX_PIPES];
>> -enum pipe pipe;
>> -int ret;
>> -
>> -if (!crtc_mask)
>> -return;
>> -
>> -for_each_pipe(dev_priv, pipe) {
>> -struct intel_crtc *crtc = 
>> intel_get_crtc_for_pipe(dev_priv,
>> -  pipe);
>> -
>> -if (!((1 << pipe) & crtc_mask))
>> -continue;
>> -
>> -ret = drm_crtc_vblank_get(>base);
>> -if (WARN_ON(ret != 0)) {
>> -crtc_mask &= ~(1 << pipe);
>> -continue;
>> -}
>> -
>> -last_vblank_count[pipe] = 
>> drm_crtc_vblank_count(>base);
>> -}
>> -
>> -for_each_pipe(dev_priv, pipe) {
>> -struct intel_crtc *crtc = 
>> intel_get_crtc_for_pipe(dev_priv,
>> -  pipe);
>> -long lret;
>> -
>> -if (!((1 << pipe) & crtc_mask))
>> -continue;
>> -
>> -lret = wait_event_timeout(dev->vblank[pipe].queue,
>> -last_vblank_count[pipe] !=
>> -
>> drm_crtc_vblank_count(>base),
>> -msecs_to_jiffies(50));
>> -
>> -WARN(!lret, "pipe %c vblank wait timed out\n", 
>> pipe_name(pipe));
>> -
>> -drm_crtc_vblank_put(>base);
>> -}
>> -}
>> -
>> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>> -{
>> -/* fb updated, need to unpin old fb */
>> -if (crtc_state->fb_changed)
>> -return true;
>> -
>> -/* wm changes, need vblank before final wm's */
>> -if (crtc_state->update_wm_post)
>> -return true;

Re: [Intel-gfx] [RFC PATCH 5/5] drm/atomic: Make async plane update checks work as intended.

[Daniel Vetter]

On Sat, Sep 02, 2017 at 03:59:22PM -0300, Gustavo Padovan wrote:
> 2017-08-30 Daniel Vetter :
> 
> > On Wed, Aug 30, 2017 at 02:17:52PM +0200, Maarten Lankhorst wrote:
> > > By always keeping track of the last commit in plane_state, we know
> > > whether there is an active update on the plane or not. With that
> > > information we can reject the fast update, and force the slowpath
> > > to be used as was originally intended.
> > > 
> > > Cc: Gustavo Padovan 
> > > Signed-off-by: Maarten Lankhorst 
> > 
> > Makes sense, but I think like patch 1 it would be better to do this in a
> > separate series. Which would then include a patch to move i915 over to the
> > async plane support.
> 
> This patch makes sense to me and it is better than the fix I wrote but never
> got around to send it out. I can pick in here locally, put the patches I
> have here for the drivers on top of it and send to intel-gfx for CI.
> 
> Anyway, without the i915 change, this is
> 
> Reviewed-by: Gustavo Padovan 

I can supply the r-b for the i915 hunk :-)

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

Re: [Intel-gfx] [RFC PATCH 1/5] drm/i915: Always wait for flip_done

[Daniel Vetter]

On Thu, Aug 31, 2017 at 05:18:06PM +0200, Maarten Lankhorst wrote:
> Op 30-08-17 om 15:40 schreef Daniel Vetter:
> > On Wed, Aug 30, 2017 at 02:54:28PM +0200, Maarten Lankhorst wrote:
> >> Op 30-08-17 om 14:43 schreef Daniel Vetter:
> >>> On Wed, Aug 30, 2017 at 02:17:48PM +0200, Maarten Lankhorst wrote:
>  The next commit removes the wait for flip_done in in
>  drm_atomic_helper_commit_cleanup_done, but we need it for the tests
>  to pass. Instead of using complicated vblank tracking which ends
>  up being ignored anyway, call the correct atomic helper. :)
> 
>  RFC because I'm not completely sure what we want to do with the vblank 
>  waiting,
>  I think for now this patch is the right way to go until we decide 
>  because it
>  preserves the status quo when drm_crtc_commit was introduced.
> 
>  Signed-off-by: Maarten Lankhorst 
>  ---
>   drivers/gpu/drm/i915/i915_drv.h  |  3 +-
>   drivers/gpu/drm/i915/intel_display.c | 83 
>  +++-
>   2 files changed, 8 insertions(+), 78 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>  b/drivers/gpu/drm/i915/i915_drv.h
>  index cbbafbfb0a55..de19621864a9 100644
>  --- a/drivers/gpu/drm/i915/i915_drv.h
>  +++ b/drivers/gpu/drm/i915/i915_drv.h
>  @@ -707,8 +707,7 @@ struct drm_i915_display_funcs {
>   struct drm_atomic_state *old_state);
>   void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>    struct drm_atomic_state *old_state);
>  -void (*update_crtcs)(struct drm_atomic_state *state,
>  - unsigned int *crtc_vblank_mask);
>  +void (*update_crtcs)(struct drm_atomic_state *state);
>   void (*audio_codec_enable)(struct drm_connector *connector,
>  struct intel_encoder *encoder,
>  const struct drm_display_mode 
>  *adjusted_mode);
>  diff --git a/drivers/gpu/drm/i915/intel_display.c 
>  b/drivers/gpu/drm/i915/intel_display.c
>  index 52c73b4dabaa..3f3cb96aa11e 100644
>  --- a/drivers/gpu/drm/i915/intel_display.c
>  +++ b/drivers/gpu/drm/i915/intel_display.c
>  @@ -12114,73 +12114,10 @@ u32 intel_crtc_get_vblank_counter(struct 
>  intel_crtc *crtc)
>   return dev->driver->get_vblank_counter(dev, crtc->pipe);
>   }
>   
>  -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
>  -  struct drm_i915_private 
>  *dev_priv,
>  -  unsigned crtc_mask)
>  -{
>  -unsigned last_vblank_count[I915_MAX_PIPES];
>  -enum pipe pipe;
>  -int ret;
>  -
>  -if (!crtc_mask)
>  -return;
>  -
>  -for_each_pipe(dev_priv, pipe) {
>  -struct intel_crtc *crtc = 
>  intel_get_crtc_for_pipe(dev_priv,
>  -  pipe);
>  -
>  -if (!((1 << pipe) & crtc_mask))
>  -continue;
>  -
>  -ret = drm_crtc_vblank_get(>base);
>  -if (WARN_ON(ret != 0)) {
>  -crtc_mask &= ~(1 << pipe);
>  -continue;
>  -}
>  -
>  -last_vblank_count[pipe] = 
>  drm_crtc_vblank_count(>base);
>  -}
>  -
>  -for_each_pipe(dev_priv, pipe) {
>  -struct intel_crtc *crtc = 
>  intel_get_crtc_for_pipe(dev_priv,
>  -  pipe);
>  -long lret;
>  -
>  -if (!((1 << pipe) & crtc_mask))
>  -continue;
>  -
>  -lret = wait_event_timeout(dev->vblank[pipe].queue,
>  -last_vblank_count[pipe] !=
>  -
>  drm_crtc_vblank_count(>base),
>  -msecs_to_jiffies(50));
>  -
>  -WARN(!lret, "pipe %c vblank wait timed out\n", 
>  pipe_name(pipe));
>  -
>  -drm_crtc_vblank_put(>base);
>  -}
>  -}
>  -
>  -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>  -{
>  -/* fb updated, need to unpin old fb */
>  -if (crtc_state->fb_changed)
>  -return true;
>  -
>  -/* wm changes, need vblank before final wm's */
>  -if (crtc_state->update_wm_post)
>  -return true;
>  -
>  -if 

Re: [Intel-gfx] [PATCH i-g-t v2] tests/gem_flink_basic: Add documentation for subtests

[Katarzyna Dec]

On Fri, Sep 01, 2017 at 12:55:37PM +0100, Arkadiusz Hiler wrote:
> On Thu, Aug 31, 2017 at 02:33:23PM -0700, Vinay Belgaumkar wrote:
> > Added the missing IGT_TEST_DESCRIPTION and some subtest
> > descriptions. Trying to establish a method to document
> 
> Hey Vinay,
> 
> Please add appropriate tag to the subject, as this is clearly an RFC.
> 
> > subtests, it should describe the feature being tested
> > rather than how. The HOW part can, if needed, be
> > described in the test code.
> > 
> > Documenting subtests will give us a good way to trace
> > feature test coverage, and also help a faster ramp
> > for understanding the test code.
> > 
> > v2: Removed duplication, addressed comments, cc'd test author
> > 
> > Cc: Michał Winiarski 
> > Cc: Eric Anholt 
> > 
> > Signed-off-by: Vinay Belgaumkar 
> > ---
> >  tests/gem_flink_basic.c | 36 ++--
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
> > index 26ae7d6..9c8c4c3 100644
> > --- a/tests/gem_flink_basic.c
> > +++ b/tests/gem_flink_basic.c
> > @@ -36,6 +36,8 @@
> >  #include 
> >  #include "drm.h"
> >  
> > +IGT_TEST_DESCRIPTION("Tests for flink - a way to export a gem object by 
> > name");
> > +
> >  static void
> >  test_flink(int fd)
> >  {
> > @@ -44,8 +46,6 @@ test_flink(int fd)
> > struct drm_gem_open open_struct;
> > int ret;
> >  
> > -   igt_info("Testing flink and open.\n");
> > -
> > memset(, 0, sizeof(create));
> > create.size = 16 * 1024;
> > ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, );
> > @@ -69,8 +69,6 @@ test_double_flink(int fd)
> > struct drm_gem_flink flink2;
> > int ret;
> >  
> > -   igt_info("Testing repeated flink.\n");
> > -
> > memset(, 0, sizeof(create));
> > create.size = 16 * 1024;
> > ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, );
> > @@ -92,8 +90,6 @@ test_bad_flink(int fd)
> > struct drm_gem_flink flink;
> > int ret;
> >  
> > -   igt_info("Testing error return on bad flink ioctl.\n");
> > -
> > flink.handle = 0x10101010;
> > ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, );
> > igt_assert(ret == -1 && errno == ENOENT);
> > @@ -105,8 +101,6 @@ test_bad_open(int fd)
> > struct drm_gem_open open_struct;
> > int ret;
> >  
> > -   igt_info("Testing error return on bad open ioctl.\n");
> > -
> > open_struct.name = 0x10101010;
> > ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, _struct);
> >  
> > @@ -121,8 +115,6 @@ test_flink_lifetime(int fd)
> > struct drm_gem_open open_struct;
> > int ret, fd2;
> >  
> > -   igt_info("Testing flink lifetime.\n");
> > -
> > fd2 = drm_open_driver(DRIVER_INTEL);
> >  
> > memset(, 0, sizeof(create));
> > @@ -134,11 +126,13 @@ test_flink_lifetime(int fd)
> > ret = ioctl(fd2, DRM_IOCTL_GEM_FLINK, );
> > igt_assert_eq(ret, 0);
> >  
> > +   /* Open another reference to the gem object */
> > open_struct.name = flink.name;
> > ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, _struct);
> > igt_assert_eq(ret, 0);
> > igt_assert(open_struct.handle != 0);
> >  
> > +   /* Before closing the previous one */
> > close(fd2);
> > fd2 = drm_open_driver(DRIVER_INTEL);
> >  
> > @@ -155,14 +149,36 @@ igt_main
> > igt_fixture
> > fd = drm_open_driver(DRIVER_INTEL);
> >  
> > +   /**
> > +* basic:
I aggree with Arek that there is no need to tell explicitly testcase
name. It is seen from the code.
> 
> Much better than the previous proposal.
> 
> But I do not like having the subtest name twice - in the comment and in
> the igt_subtest().
> 
> IMO it's better to have a parser that can extract the name from the
> following igt_subtest() than having a place where when we have
> duplication and we can go out of sync.
> 
> 
> 
> I've been following your efforts and I have some question and thoughts
> to share.
> 
> ### Questions
> 
>  1. What's the actual problem statement? What are you trying to solve
> here?
> 
>  2. Who, in your mind, is the supposed reader of the documentation?
> How's that different form someone who is supposed to look at the
> code directly?
> 
>  3. Why are you trying to drive this? What's your motivation?
> 
> 
> ### My Two (Euro)cents
> 
> As Michal stressed, in a reply to the previous revision, we should not
> be doing C to English translation. My reasoning:
> 
> 1. Maintenance hell - if you describe inner workings of tests in too
> much detail, you will have two places that you have to update when you
> are making even the slightest adjustments.
> 
> And people will forget to update the comments, and will receive negative
> reviews, and will have to respin the series making the changes again,
> this time in the "English" implementation.
> 
> To me it's unnecessary rising of the bar for the contributors.
> 
> 2. Code is enough - I think it's safe to assume that anyone who is
> enough 

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Silence sparse by using gfp_t

[Joonas Lahtinen]

On Fri, 2017-09-01 at 15:57 +0100, Chris Wilson wrote:
> Sparse enforces that GFP flags are only manipulated inside gfp_t locals.
> 
> Fixes: 4d470f7359c4 ("drm/i915: Avoid undefined behaviour of "u32 >> 32"")
> Signed-off-by: Chris Wilson 
> Cc: Joonas Lahtinen 
> Cc: Tvrtko Ursulin 

Isn't Fixes: bit much for sparse warning?

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH 4/4] drm/i915: Expose RPCS (SSEU) configuration to userspace

[Joonas Lahtinen]

On Fri, 2017-09-01 at 18:12 +0100, Lionel Landwerlin wrote:
> From: Chris Wilson 
> 
> We want to allow userspace to reconfigure the subslice configuration for
> its own use case. To do so, we expose a context parameter to allow
> adjustment of the RPCS register stored within the context image (and
> currently not accessible via LRI). If the context is adjusted before
> first use, the adjustment is for "free"; otherwise if the context is
> active we flush the context off the GPU (stalling all users) and forcing
> the GPU to save the context to memory where we can modify it and so
> ensure that the register is reloaded on next execution.
> 
> The overhead of managing additional EU subslices can be significant,
> especially in multi-context workloads. Non-GPGPU contexts should
> preferably disable the subslices it is not using, and others should
> fine-tune the number to match their workload.
> 
> We expose complete control over the RPCS register, allowing
> configuration of slice/subslice, via masks packed into a u64 for
> simplicity. For example,
> 
>   struct drm_i915_gem_context_param arg;
> 
>   memset(, 0, sizeof(arg));
>   arg.ctx_id = ctx;
>   arg.param = I915_CONTEXT_PARAM_SSEU;
>   if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, ) == 0) {
>   union drm_i915_gem_context_param_sseu *sseu = 
> 
>   sseu->packed.subslice_mask = 0;
> 
>   drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, );
>   }
> 
> could be used to disable all subslices where supported.
> 
> v2: Fix offset of CTX_R_PWR_CLK_STATE in intel_lr_context_set_sseu() (Lionel)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100899
> Signed-off-by: Chris Wilson 
> Signed-off-by: Lionel Landwerlin 
> Cc: Dmitry Rogozhkin 
> CC: Tvrtko Ursulin 
> CC: Zhipeng Gong 
> CC: Joonas Lahtinen 

Please do link to the userspace patches, will be easier to track them
that way.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v14 0/7] drm/i915/gvt: Dma-buf support for GVT-g

[Gerd Hoffmann]

  Hi,

> > However, I see VFIO_DEVICE_QUERY_GFX_PLANE failures which I think
> > should
> > not be there.  When the guest didn't define a plane yet I get "No
> > such device"
> > errors instead of a plane_info struct with fields (drm_format,
> > width, height, size)
> > set to zero.  I also see "Bad address" errors now and then with no
> > obvious cause.
> 
> The idea is to return "No such device" error with fields set to zero.
> There are two cases, in which the "No such device" error is returned:
> one is the guest IGD driver
> has not finished the initialization and the other is the plane is
> disabled by guest IGD driver.
> If we prefer to return success in these two situations with fields
> set to zero, I can add it in the
> next version.

Yes, success should be returned in those cases.  Querying the plane
information worked and the struct is filled by the driver.  Userspace
can figure that there is no plane defined by the guest by looking at
the struct fields.

> > Cursor support isn't working too.
> 
> It seems there is some issue in i915 of version 4.13-rc6, which
> blocks the cursor on native platform.
> I just tried the rc7 (the latest staging), there is on such issue.
> Thanks.

OK, I'll go re-test with the just-released 4.13 kernel then.

thanks,
  Gerd

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