Re: [Intel-gfx] [PATCH v4] drm/i915: edp resume/On time optimization.
On 01/13/2016 07:27 AM, abhay.ku...@intel.com wrote: From: Abhay Kumar Make resume/on codepath not to wait for panel_power_cycle_delay(t11_t12) if this time is already spent in suspend/poweron time. v2: Use CLOCK_BOOTTIME and remove jiffies for panel power cycle delay calculation(Ville). v3: Addressed below comments 1. Tracking time from where last powercycle is initiated. 2. Used ktime_get_bootime() wrapper for boottime clock. 3. Used ktime_ms_delta() to get time difference. v4: Updated v3 change log in detail. Quick tip, you can use --in-reply-to while sending to maintain context, unless there is major rewrite of patches. Regards Shobhit Cc: Ville Syrjälä Signed-off-by: Abhay Kumar --- drivers/gpu/drm/i915/intel_dp.c | 19 ++- drivers/gpu/drm/i915/intel_drv.h | 2 +- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 796e3d3..0042693 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1812,12 +1812,21 @@ static void wait_panel_off(struct intel_dp *intel_dp) static void wait_panel_power_cycle(struct intel_dp *intel_dp) { + static ktime_t panel_power_on_time; + s64 panel_power_off_duration; + DRM_DEBUG_KMS("Wait for panel power cycle\n"); + /* take the difference of currrent time and panel power off time +* and then make panel wait for t11_t12 if needed. */ + panel_power_on_time = ktime_get_boottime(); + panel_power_off_duration = ktime_ms_delta(panel_power_on_time, intel_dp->panel_power_off_time); + /* When we disable the VDD override bit last we have to do the manual * wait. */ - wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, - intel_dp->panel_power_cycle_delay); + if (panel_power_off_duration < (s64)intel_dp->panel_power_cycle_delay) + wait_remaining_ms_from_jiffies(jiffies, + intel_dp->panel_power_cycle_delay - panel_power_off_duration); wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); } @@ -1969,7 +1978,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp) I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); if ((pp & POWER_TARGET_ON) == 0) - intel_dp->last_power_cycle = jiffies; + intel_dp->panel_power_off_time = ktime_get_boottime(); power_domain = intel_display_port_aux_power_domain(intel_encoder); intel_display_power_put(dev_priv, power_domain); @@ -2118,7 +2127,7 @@ static void edp_panel_off(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); - intel_dp->last_power_cycle = jiffies; + intel_dp->panel_power_off_time = ktime_get_boottime(); wait_panel_off(intel_dp); /* We got a reference when we enabled the VDD. */ @@ -5122,7 +5131,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp) { - intel_dp->last_power_cycle = jiffies; + intel_dp->panel_power_off_time = ktime_get_boottime(); intel_dp->last_power_on = jiffies; intel_dp->last_backlight_off = jiffies; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bdfe403..06b37b8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -793,9 +793,9 @@ struct intel_dp { int backlight_off_delay; struct delayed_work panel_vdd_work; bool want_panel_vdd; - unsigned long last_power_cycle; unsigned long last_power_on; unsigned long last_backlight_off; + ktime_t panel_power_off_time; struct notifier_block edp_notifier; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ success: Fi.CI.BAT
== Summary == Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: 2016y-01m-12d-21h-16m-40s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: dmesg-warn -> PASS (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (skl-i7k-2) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-a-frame-sequence: fail -> PASS (snb-x220t) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 byt-nuc total:141 pass:123 dwarn:3 dfail:0 fail:0 skip:15 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1156/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ warning: Fi.CI.BAT
== Summary == Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: 2016y-01m-12d-21h-16m-40s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (bdw-nuci7) dmesg-warn -> PASS (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (skl-i7k-2) dmesg-warn -> PASS (ilk-hp8440p) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-a: pass -> DMESG-WARN (byt-nuc) Subgroup read-crc-pipe-a-frame-sequence: fail -> PASS (snb-x220t) bdw-nuci7total:138 pass:128 dwarn:1 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1157/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ failure: Fi.CI.BAT
== Summary == Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: 2016y-01m-12d-21h-16m-40s UTC integration manifest Test gem_basic: Subgroup create-close: pass -> DMESG-WARN (skl-i5k-2) Test gem_cpu_reloc: Subgroup basic: pass -> DMESG-FAIL (skl-i5k-2) Test gem_ctx_param_basic: Subgroup basic: pass -> DMESG-WARN (skl-i5k-2) Subgroup invalid-param-set: pass -> DMESG-WARN (skl-i5k-2) Subgroup non-root-set-no-zeromap: pass -> DMESG-WARN (skl-i5k-2) Subgroup root-set-no-zeromap-disabled: pass -> DMESG-WARN (skl-i5k-2) Test gem_mmap: Subgroup basic: pass -> DMESG-WARN (skl-i5k-2) Test gem_mmap_gtt: Subgroup basic-read: pass -> DMESG-WARN (skl-i5k-2) Subgroup basic-write: pass -> DMESG-WARN (skl-i5k-2) Test gem_storedw_loop: Subgroup basic-render: dmesg-warn -> PASS (bdw-ultra) Test kms_addfb_basic: Subgroup addfb25-modifier-no-flag: pass -> DMESG-WARN (skl-i5k-2) Subgroup addfb25-x-tiled-mismatch: pass -> DMESG-WARN (skl-i5k-2) Subgroup addfb25-yf-tiled: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-1024: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-63: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-999: pass -> DMESG-WARN (skl-i5k-2) Subgroup clobberred-modifier: pass -> DMESG-WARN (skl-i5k-2) Subgroup too-high: pass -> DMESG-WARN (skl-i5k-2) Subgroup too-wide: pass -> DMESG-WARN (skl-i5k-2) Subgroup unused-offsets: pass -> DMESG-WARN (skl-i5k-2) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (skl-i7k-2) dmesg-warn -> PASS (ilk-hp8440p) Subgroup basic-flip-vs-modeset: pass -> DMESG-WARN (ilk-hp8440p) Subgroup basic-plain-flip: pass -> DMESG-FAIL (skl-i5k-2) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-a-frame-sequence: pass -> DMESG-FAIL (skl-i5k-2) Subgroup read-crc-pipe-a-frame-sequence: fail -> PASS (snb-x220t) Subgroup read-crc-pipe-b-frame-sequence: pass -> DMESG-FAIL (skl-i5k-2) Test prime_self_import: Subgroup basic-with_two_bos: pass -> DMESG-WARN (skl-i5k-2) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 byt-nuc total:141 pass:123 dwarn:3 dfail:0 fail:0 skip:15 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 skl-i5k-2total:141 pass:108 dwarn:20 dfail:4 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1158/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ failure: Fi.CI.BAT
Daniel Vetter writes: > On Mon, Jan 11, 2016 at 02:50:28PM +0200, Mika Kuoppala wrote: >> Patchwork writes: >> >> > == Summary == >> > >> > Built on ff88655b3a5467bbc3be8c67d3e05ebf182557d3 drm-intel-nightly: >> > 2016y-01m-11d-07h-30m-16s UTC integration manifest >> > >> > Test gem_storedw_loop: >> > Subgroup basic-render: >> > pass -> DMESG-WARN (skl-i5k-2) UNSTABLE >> > dmesg-warn -> PASS (bdw-ultra) >> > Test kms_flip: >> > Subgroup basic-flip-vs-dpms: >> > dmesg-warn -> PASS (ilk-hp8440p) >> > Subgroup basic-flip-vs-modeset: >> > pass -> DMESG-WARN (skl-i5k-2) >> > dmesg-warn -> PASS (bsw-nuc-2) >> > pass -> DMESG-WARN (ilk-hp8440p) >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup basic-flip-vs-wf_vblank: >> > pass -> DMESG-WARN (byt-nuc) >> > Subgroup basic-plain-flip: >> > dmesg-warn -> PASS (bsw-nuc-2) >> > dmesg-warn -> PASS (byt-nuc) >> > Test kms_pipe_crc_basic: >> > Subgroup nonblocking-crc-pipe-b: >> > pass -> DMESG-WARN (byt-nuc) >> > Subgroup nonblocking-crc-pipe-c: >> > pass -> DMESG-WARN (bsw-nuc-2) >> > Subgroup read-crc-pipe-a: >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-b: >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-b-frame-sequence: >> > pass -> DMESG-WARN (bdw-ultra) >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-c: >> > dmesg-warn -> PASS (bsw-nuc-2) >> > Subgroup read-crc-pipe-c-frame-sequence: >> > pass -> DMESG-WARN (bsw-nuc-2) > > But what with all the pass->dmesg-warn changes above? That's considered > BAT failure too, we can't afford to sprinkle warnings all over ... And > it's a bunch of different machines. > >> > Test pm_rpm: >> > Subgroup basic-pci-d3-state: >> > dmesg-warn -> PASS (byt-nuc) >> > >> > bdw-ultratotal:138 pass:130 dwarn:1 dfail:0 fail:1 >> > skip:6 >> >> Tomi suspected this as a fluke fail. There was no igt stdout nor stderr >> for this case. We did a rerun and bdw-ultra passed. >> >> -Mika >> >> > bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 >> > byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 >> > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 >> > hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 >> > ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 >> > skl-i5k-2total:141 pass:130 dwarn:3 dfail:0 fail:0 skip:8 >> > skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 >> > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 >> > snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 >> > >> > HANGED ivb-t430s in igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b > > So is killing a machine ... pretty sure this is new-ish. > -Daniel > I did cover my back with this one. This happened on other sets on the same day and I went and asked Tomi. He said that don't worry about it. So I didn't. -Mika > >> > >> > Results at /archive/results/CI_IGT_test/Patchwork_1116/ >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ success: Fi.CI.BAT
== Summary == Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: 2016y-01m-12d-21h-16m-40s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: dmesg-warn -> PASS (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (skl-i7k-2) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-a-frame-sequence: fail -> PASS (snb-x220t) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 byt-nuc total:141 pass:123 dwarn:3 dfail:0 fail:0 skip:15 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 skl-i5k-2total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1159/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ failure: Fi.CI.BAT
Daniel Vetter writes: > On Mon, Jan 11, 2016 at 02:50:28PM +0200, Mika Kuoppala wrote: >> Patchwork writes: >> >> > == Summary == >> > >> > Built on ff88655b3a5467bbc3be8c67d3e05ebf182557d3 drm-intel-nightly: >> > 2016y-01m-11d-07h-30m-16s UTC integration manifest >> > >> > Test gem_storedw_loop: >> > Subgroup basic-render: >> > pass -> DMESG-WARN (skl-i5k-2) UNSTABLE >> > dmesg-warn -> PASS (bdw-ultra) >> > Test kms_flip: >> > Subgroup basic-flip-vs-dpms: >> > dmesg-warn -> PASS (ilk-hp8440p) >> > Subgroup basic-flip-vs-modeset: >> > pass -> DMESG-WARN (skl-i5k-2) >> > dmesg-warn -> PASS (bsw-nuc-2) >> > pass -> DMESG-WARN (ilk-hp8440p) >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup basic-flip-vs-wf_vblank: >> > pass -> DMESG-WARN (byt-nuc) >> > Subgroup basic-plain-flip: >> > dmesg-warn -> PASS (bsw-nuc-2) >> > dmesg-warn -> PASS (byt-nuc) >> > Test kms_pipe_crc_basic: >> > Subgroup nonblocking-crc-pipe-b: >> > pass -> DMESG-WARN (byt-nuc) >> > Subgroup nonblocking-crc-pipe-c: >> > pass -> DMESG-WARN (bsw-nuc-2) >> > Subgroup read-crc-pipe-a: >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-b: >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-b-frame-sequence: >> > pass -> DMESG-WARN (bdw-ultra) >> > dmesg-warn -> PASS (byt-nuc) >> > Subgroup read-crc-pipe-c: >> > dmesg-warn -> PASS (bsw-nuc-2) >> > Subgroup read-crc-pipe-c-frame-sequence: >> > pass -> DMESG-WARN (bsw-nuc-2) > > But what with all the pass->dmesg-warn changes above? That's considered > BAT failure too, we can't afford to sprinkle warnings all over ... And > it's a bunch of different machines. > Forgot to address this one in previous mail. This patchset added more debug infra and enabled it for bsw/byt. So assumpion is that it did uncover a real problem thus the warns. Is the policy that if your debug infra reveals problems, you need to fix those problems? If so, there is a chicken and egg problem as you don't always have access to hardware that your debug infra will cover. Thanks, -Mika >> > Test pm_rpm: >> > Subgroup basic-pci-d3-state: >> > dmesg-warn -> PASS (byt-nuc) >> > >> > bdw-ultratotal:138 pass:130 dwarn:1 dfail:0 fail:1 >> > skip:6 >> >> Tomi suspected this as a fluke fail. There was no igt stdout nor stderr >> for this case. We did a rerun and bdw-ultra passed. >> >> -Mika >> >> > bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 >> > byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 >> > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 >> > hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 >> > ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 >> > skl-i5k-2total:141 pass:130 dwarn:3 dfail:0 fail:0 skip:8 >> > skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 >> > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 >> > snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 >> > >> > HANGED ivb-t430s in igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b > > So is killing a machine ... pretty sure this is new-ish. > -Daniel > > >> > >> > Results at /archive/results/CI_IGT_test/Patchwork_1116/ >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ failure: Fi.CI.BAT
On Wed, Jan 13, 2016 at 10:20 AM, Mika Kuoppala wrote: >> But what with all the pass->dmesg-warn changes above? That's considered >> BAT failure too, we can't afford to sprinkle warnings all over ... And >> it's a bunch of different machines. >> > > Forgot to address this one in previous mail. This patchset > added more debug infra and enabled it for bsw/byt. So assumpion > is that it did uncover a real problem thus the warns. > > Is the policy that if your debug infra reveals problems, > you need to fix those problems? We should discuss this in the next meeting (adding Annie/Jesse for that), but personally I think adding new WARN/ERROR noise should never result in BAT regressions. If your patch uncovers existing failures even in BAT then imo the right approach is to first fix up things, then apply the WARN patch to make sure we don't break things. The problem is that once you have dmesg noise in a test, then no one will notice additional noise and regressions. And that's how we got into our mess last summer. Also dmesg noise is not really acceptable anyway and needs to be fixed (Linus/Dave will get grumpy). If that takes too long because there's lots of warn, then maybe we need to first add the new sanity check at debug level, just to help with tracking down issues. We might need to improve CI reporting so that debug level dmesg is still capture completely for BAT runs. > If so, there is a chicken and egg problem as you don't > always have access to hardware that your debug infra > will cover. Yeah, we need to enable manual submission to CI-machines. Abusing CI as a test facility simply means that you're ok with blocking everyone else with your CI result spam. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ failure: Fi.CI.BAT
Argh, actually add Annie/Jesse! -Daniel On Wed, Jan 13, 2016 at 10:39 AM, Daniel Vetter wrote: > On Wed, Jan 13, 2016 at 10:20 AM, Mika Kuoppala > wrote: >>> But what with all the pass->dmesg-warn changes above? That's considered >>> BAT failure too, we can't afford to sprinkle warnings all over ... And >>> it's a bunch of different machines. >>> >> >> Forgot to address this one in previous mail. This patchset >> added more debug infra and enabled it for bsw/byt. So assumpion >> is that it did uncover a real problem thus the warns. >> >> Is the policy that if your debug infra reveals problems, >> you need to fix those problems? > > We should discuss this in the next meeting (adding Annie/Jesse for > that), but personally I think adding new WARN/ERROR noise should never > result in BAT regressions. If your patch uncovers existing failures > even in BAT then imo the right approach is to first fix up things, > then apply the WARN patch to make sure we don't break things. The > problem is that once you have dmesg noise in a test, then no one will > notice additional noise and regressions. And that's how we got into > our mess last summer. > > Also dmesg noise is not really acceptable anyway and needs to be fixed > (Linus/Dave will get grumpy). > > If that takes too long because there's lots of warn, then maybe we > need to first add the new sanity check at debug level, just to help > with tracking down issues. We might need to improve CI reporting so > that debug level dmesg is still capture completely for BAT runs. > >> If so, there is a chicken and egg problem as you don't >> always have access to hardware that your debug infra >> will cover. > > Yeah, we need to enable manual submission to CI-machines. Abusing CI > as a test facility simply means that you're ok with blocking everyone > else with your CI result spam. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/8] drm/i915/gen9: Add GEN8_CS_CHICKEN1 to HW whitelist
Required for WaEnablePreemptionGranularityControlByUMD:skl,bxt Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6668bb0..1067ff0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5998,6 +5998,8 @@ enum skl_disp_power_wells { #define FF_SLICE_CS_CHICKEN2 _MMIO(0x20e4) #define GEN9_TSG_BARRIER_ACK_DISABLE (1<<8) +#define GEN8_CS_CHICKEN1 _MMIO(0x2580) + /* GEN7 chicken */ #define GEN7_COMMON_SLICE_CHICKEN1 _MMIO(0x7010) # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC ((1<<10) | (1<<26)) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 354da81..35e78ed 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -909,6 +909,7 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring) struct drm_device *dev = ring->dev; struct drm_i915_private *dev_priv = dev->dev_private; uint32_t tmp; + int ret; /* WaEnableLbsSlaRetryTimerDecrement:skl */ I915_WRITE(BDW_SCRATCH1, I915_READ(BDW_SCRATCH1) | @@ -979,6 +980,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring) /* WaDisableSTUnitPowerOptimization:skl,bxt */ WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN2, GEN8_ST_PO_DISABLE); + /* WaEnablePreemptionGranularityControlByUMD:skl,bxt */ + ret= wa_ring_whitelist_reg(ring, GEN8_CS_CHICKEN1); + if (ret) + return ret; + return 0; } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/8] Gen9 HW whitelist and Preemption WA patches
Set of patches that add HW whitelist framework and Preemption WA patches. HW whitelist patch is already sent to list and reviewed[1], it is just rebased in this version. This was not merged before as there was no user; this is still the case but since Preemption patches[2] are already on the list this is going to be required sooner or later. Please review the remaining patches. [1] https://patchwork.freedesktop.org/patch/60937/ [2] http://lists.freedesktop.org/archives/intel-gfx/2015-November/080965.html Arun Siluvery (8): drm/i915/gen9: Add framework to whitelist specific GPU registers drm/i915/gen9: Add GEN8_CS_CHICKEN1 to HW whitelist drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist drm/i915/bxt: Add GEN9_CS_DEBUG_MODE1 to HW whitelist drm/i915/bxt: Add GEN8_L3SQCREG4 to HW whitelist drm/i915/skl: Add GEN8_L3SQCREG4 to HW whitelist drm/i915/skl: Enable Per context Preemption granularity control drm/i915/gen9: Add WaOCLCoherentLineFlush drivers/gpu/drm/i915/i915_drv.h | 9 - drivers/gpu/drm/i915/i915_reg.h | 11 ++ drivers/gpu/drm/i915/intel_ringbuffer.c | 62 + 3 files changed, 81 insertions(+), 1 deletion(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/8] drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist
Required for WaAllowUMDToModifyHDCChicken1:skl,bxt Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_ringbuffer.c | 5 + 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 1067ff0..16ef377 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6045,6 +6045,8 @@ enum skl_disp_power_wells { #define HDC_FORCE_NON_COHERENT(1<<4) #define HDC_BARRIER_PERFORMANCE_DISABLE (1<<10) +#define GEN8_HDC_CHICKEN1 _MMIO(0x7304) + /* GEN9 chicken */ #define SLICE_ECO_CHICKEN0 _MMIO(0x7308) #define PIXEL_MASK_CAMMING_DISABLE (1 << 14) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 35e78ed..2241a92 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -985,6 +985,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring) if (ret) return ret; + /* WaAllowUMDToModifyHDCChicken1:skl,bxt */ + ret = wa_ring_whitelist_reg(ring, GEN8_HDC_CHICKEN1); + if (ret) + return ret; + return 0; } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
Some of the HW registers are privileged and cannot be written to from a non-privileged batch buffers coming from userspace unless they are on whitelist. Userspace need write access to them to implement preemption related WA. The reason for using this approach is, the register bits that control preemption granularity at the HW level are not context save/restored; so even if we set these bits always in kernel they are going to change once the context is switched out. We can consider making them non-privileged by default but these registers also contain other chicken bits which should not be allowed to be modified. In the later revisions controlling bits are save/restored at context level but in the existing revisions these are exported via other debug registers and should be on the whitelist. This patch adds changes to provide HW with a list of registers to be whitelisted. HW checks this list during execution and provides access accordingly. HW imposes a limit on the number of registers on whitelist and it is per-engine. At this point we are only enabling whitelist for RCS and we don't foresee any requirement for other engines. The registers to be whitelisted are added using generic workaround list mechanism, even these are only enablers for userspace workarounds. But by sharing this mechanism we get some test assets without additional cost (Mika). v2: rebase Reviewed-by: Mika Kuoppala Cc: Mika Kuoppala Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_drv.h | 9 - drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..660afe1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1653,11 +1653,18 @@ struct i915_wa_reg { u32 mask; }; -#define I915_MAX_WA_REGS 16 +/* + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only + * allowing it for RCS as we don't foresee any requirement of having + * a whitelist for other engines. When it is really required for + * other engines then the limit need to be increased. + */ +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) struct i915_workarounds { struct i915_wa_reg reg[I915_MAX_WA_REGS]; u32 count; + u32 hw_whitelist_index[I915_NUM_RINGS]; }; struct i915_virtual_gpu { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..6668bb0 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells { #define RING_WAIT(1<<11) /* gen3+, PRBx_CTL */ #define RING_WAIT_SEMAPHORE (1<<10) /* gen6+ */ +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0) +#define RING_MAX_NONPRIV_SLOTS 12 + #define GEN7_TLB_RD_ADDR _MMIO(0x4700) #if 0 diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 4060acf..354da81 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv, #define WA_WRITE(addr, val) WA_REG(addr, 0x, val) +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring, + i915_reg_t reg_addr) +{ + struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct i915_workarounds *wa = &dev_priv->workarounds; + const uint32_t index = wa->hw_whitelist_index[ring->id]; + + if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) + return -EINVAL; + + WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4), +reg_addr.reg); + wa->hw_whitelist_index[ring->id]++; + + return 0; +} + static int gen8_init_workarounds(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; @@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) WARN_ON(ring->id != RCS); dev_priv->workarounds.count = 0; + dev_priv->workarounds.hw_whitelist_index[ring->id] = 0; if (IS_BROADWELL(dev)) return bdw_init_workarounds(ring); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/8] drm/i915/skl: Add GEN8_L3SQCREG4 to HW whitelist
Required for WaDisableLSQCROPERFforOCL:skl Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5eb4eea..b8dbd2c 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1097,6 +1097,11 @@ static int skl_init_workarounds(struct intel_engine_cs *ring) GEN7_HALF_SLICE_CHICKEN1, GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE); + /* WaDisableLSQCROPERFforOCL:skl */ + ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4); + if (ret) + return ret; + return skl_tune_iz_hashing(ring); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 8/8] drm/i915/gen9: Add WaOCLCoherentLineFlush
This is mainly required for preemption. Cc: Dave Gordon Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5a2ad10..af95091 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -980,6 +980,10 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring) /* WaDisableSTUnitPowerOptimization:skl,bxt */ WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN2, GEN8_ST_PO_DISABLE); + /* WaOCLCoherentLineFlush:skl,bxt */ + I915_WRITE(GEN8_L3SQCREG4, (I915_READ(GEN8_L3SQCREG4) | + GEN8_LQSC_FLUSH_COHERENT_LINES)); + /* WaEnablePreemptionGranularityControlByUMD:skl,bxt */ ret= wa_ring_whitelist_reg(ring, GEN8_CS_CHICKEN1); if (ret) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/8] drm/i915/bxt: Add GEN9_CS_DEBUG_MODE1 to HW whitelist
Required for, WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt WaDisableObjectLevelPreemptionForInstancedDraw:bxt WaDisableObjectLevelPreemtionForInstanceId:bxt According to WA database these are only applicable for BXT:A0 but since A0 and A1 shares the same GT these are extended for A1 as well. These are also required for SKL until B0 but not adding them because they are pre-production steppings. Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 9 + 2 files changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 16ef377..eabd2af 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5998,6 +5998,7 @@ enum skl_disp_power_wells { #define FF_SLICE_CS_CHICKEN2 _MMIO(0x20e4) #define GEN9_TSG_BARRIER_ACK_DISABLE (1<<8) +#define GEN9_CS_DEBUG_MODE1_MMIO(0x20EC) #define GEN8_CS_CHICKEN1 _MMIO(0x2580) /* GEN7 chicken */ diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 2241a92..7a46cf1 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1132,6 +1132,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring) GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE); } + /* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */ + /* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */ + /* WaDisableObjectLevelPreemtionForInstanceId:bxt */ + if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) { + ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1); + if (ret) + return ret; + } + return 0; } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/8] drm/i915/bxt: Add GEN8_L3SQCREG4 to HW whitelist
Required for WaDisableLSQCROPERFforOCL:bxt According to WA database these are only applicable for BXT:A0 but since A0 and A1 shares the same GT these are extended for A1 as well. Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 7a46cf1..5eb4eea 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1135,10 +1135,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring) /* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */ /* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */ /* WaDisableObjectLevelPreemtionForInstanceId:bxt */ + /* WaDisableLSQCROPERFforOCL:bxt */ if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) { ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1); if (ret) return ret; + + ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4); + if (ret) + return ret; } return 0; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/8] drm/i915/skl: Enable Per context Preemption granularity control
Per context preemption granularity control is only available from SKL:E0+ Cc: Dave Gordon Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++ 2 files changed, 13 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index eabd2af..97774a3 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5995,6 +5995,9 @@ enum skl_disp_power_wells { #define SKL_DFSM_CDCLK_LIMIT_450 (2 << 23) #define SKL_DFSM_CDCLK_LIMIT_337_5 (3 << 23) +#define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20E0) +#define GEN9_FFSC_PERCTX_PREEMPT_CTRL(1<<14) + #define FF_SLICE_CS_CHICKEN2 _MMIO(0x20e4) #define GEN9_TSG_BARRIER_ACK_DISABLE (1<<8) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index b8dbd2c..5a2ad10 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1045,6 +1045,16 @@ static int skl_init_workarounds(struct intel_engine_cs *ring) if (ret) return ret; + /* +* Actual WA is to disable percontext preemption granularity control +* until D0 which is the default case so this is equivalent to +* !WaDisablePerCtxtPreemptionGranularityControl:skl +*/ + if (IS_SKL_REVID(dev, SKL_REVID_E0, REVID_FOREVER)) { + I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1, + _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL)); + } + if (IS_SKL_REVID(dev, 0, SKL_REVID_D0)) { /* WaDisableChickenBitTSGBarrierAckForFFSliceCS:skl */ I915_WRITE(FF_SLICE_CS_CHICKEN2, -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ warning: Fi.CI.BAT
== Summary == Built on dd4a7926b4118f72b7ae0f7b97e9644172df472c drm-intel-nightly: 2016y-01m-13d-09h-05m-34s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE pass -> DMESG-WARN (bdw-ultra) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-a: pass -> DMESG-WARN (byt-nuc) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1160/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
On Wed, Jan 13, 2016 at 10:06:26AM +, Arun Siluvery wrote: > Some of the HW registers are privileged and cannot be written to from a > non-privileged batch buffers coming from userspace unless they are on > whitelist. > Userspace need write access to them to implement preemption related WA. > > The reason for using this approach is, the register bits that control > preemption > granularity at the HW level are not context save/restored; so even if we set > these > bits always in kernel they are going to change once the context is switched > out. > We can consider making them non-privileged by default but these registers also > contain other chicken bits which should not be allowed to be modified. > > In the later revisions controlling bits are save/restored at context level but > in the existing revisions these are exported via other debug registers and > should > be on the whitelist. This patch adds changes to provide HW with a list of > registers > to be whitelisted. HW checks this list during execution and provides access > accordingly. > > HW imposes a limit on the number of registers on whitelist and it is > per-engine. > At this point we are only enabling whitelist for RCS and we don't foresee any > requirement for other engines. > > The registers to be whitelisted are added using generic workaround list > mechanism, > even these are only enablers for userspace workarounds. But by sharing this > mechanism we get some test assets without additional cost (Mika). > > v2: rebase > > Reviewed-by: Mika Kuoppala > Cc: Mika Kuoppala > Signed-off-by: Arun Siluvery > --- > drivers/gpu/drm/i915/i915_drv.h | 9 - > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++ > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 104bd18..660afe1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1653,11 +1653,18 @@ struct i915_wa_reg { > u32 mask; > }; > > -#define I915_MAX_WA_REGS 16 > +/* > + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only > + * allowing it for RCS as we don't foresee any requirement of having > + * a whitelist for other engines. When it is really required for > + * other engines then the limit need to be increased. > + */ > +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) > > struct i915_workarounds { > struct i915_wa_reg reg[I915_MAX_WA_REGS]; > u32 count; > + u32 hw_whitelist_index[I915_NUM_RINGS]; > }; > > struct i915_virtual_gpu { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 0a98889..6668bb0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells { > #define RING_WAIT (1<<11) /* gen3+, PRBx_CTL */ > #define RING_WAIT_SEMAPHORE(1<<10) /* gen6+ */ > > +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0) > +#define RING_MAX_NONPRIV_SLOTS 12 > + > #define GEN7_TLB_RD_ADDR _MMIO(0x4700) > > #if 0 > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 4060acf..354da81 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv, > > #define WA_WRITE(addr, val) WA_REG(addr, 0x, val) > > +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring, > + i915_reg_t reg_addr) > +{ > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > + struct i915_workarounds *wa = &dev_priv->workarounds; > + const uint32_t index = wa->hw_whitelist_index[ring->id]; > + > + if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > + return -EINVAL; > + > + WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4), > + reg_addr.reg); No _MMIO junk outside of i915_reg.h please. Just parametrize the reg define properly. > + wa->hw_whitelist_index[ring->id]++; > + > + return 0; > +} > + > static int gen8_init_workarounds(struct intel_engine_cs *ring) > { > struct drm_device *dev = ring->dev; > @@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) > WARN_ON(ring->id != RCS); > > dev_priv->workarounds.count = 0; > + dev_priv->workarounds.hw_whitelist_index[ring->id] = 0; > > if (IS_BROADWELL(dev)) > return bdw_init_workarounds(ring); > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists
[Intel-gfx] [PATCH] drm/i915: Expose exec parameter to force non IA-Coherent for Gen9+
Starting from Gen9 we can use IA-Coherent caches. Coherence may be not required in certain cases and can be disabled in an easy way. To do this we can set HDC_FORCE_NON_COHERENT bit in HDC_CHICKEN0 register. This register is part of HW context, however it is private and cannot be programmed from non-privileged batch buffer. New parameter is to override default programming and allow UMD to decide whether IA-Coherency is not needed for submitted batch buffer. When flag is set KMD emits commands to disable coherency before batch buffer execution starts. After execution finished state is restored. When WaForceEnableNonCoherent is programmed, it does not make sense to allow for coherency because this can lead to GPU hangs. In such situation flag is ignored. Signed-off-by: Artur Harasimiuk --- drivers/gpu/drm/i915/i915_dma.c| 3 +++ drivers/gpu/drm/i915/i915_drv.h| 4 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 drivers/gpu/drm/i915/intel_lrc.c | 28 drivers/gpu/drm/i915/intel_ringbuffer.c| 7 +++ include/uapi/drm/i915_drm.h| 8 +++- 6 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896c..79ecf20 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -172,6 +172,9 @@ static int i915_getparam(struct drm_device *dev, void *data, case I915_PARAM_HAS_EXEC_SOFTPIN: value = 1; break; + case I915_PARAM_HAS_EXEC_FORCE_NON_COHERENT: + value = INTEL_INFO(dev)->gen >= 9; + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..71d739c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -886,6 +886,10 @@ struct intel_context { } engine[I915_NUM_RINGS]; struct list_head link; + + struct { + unsigned int WaForceEnableNonCoherent:1; + } wa; }; enum fb_op_origin { diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index d469c47..2997a58 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1400,6 +1400,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (!i915_gem_check_execbuffer(args)) return -EINVAL; + if ((args->flags & I915_EXEC_FORCE_NON_COHERENT) && + INTEL_INFO(dev)->gen < 9) + return -EINVAL; + ret = validate_exec_list(dev, exec, args->buffer_count); if (ret) return ret; @@ -1494,6 +1498,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, i915_gem_context_reference(ctx); + /* Clear this flag when WA is programmed */ + if (ctx->wa.WaForceEnableNonCoherent) + args->flags &= ~I915_EXEC_FORCE_NON_COHERENT; + if (ctx->ppgtt) vm = &ctx->ppgtt->base; else diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index ab344e0..4482a6a 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -879,6 +879,24 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request) return intel_logical_ring_begin(request, 0); } +static inline void +intel_lr_emit_force_non_coherent(struct i915_execbuffer_params *params, + struct drm_i915_gem_execbuffer2 *args, bool force) +{ + if (args->flags & I915_EXEC_FORCE_NON_COHERENT) { + struct intel_ringbuffer *ringbuf = + params->ctx->engine[params->ring->id].ringbuf; + + intel_logical_ring_emit(ringbuf, MI_NOOP); + intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(1)); + intel_logical_ring_emit(ringbuf, HDC_CHICKEN0.reg); + intel_logical_ring_emit(ringbuf, force ? + _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT) : + _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT)); + intel_logical_ring_advance(ringbuf); + } +} + /** * execlists_submission() - submit a batchbuffer for execution, Execlists style * @dev: DRM device. @@ -959,6 +977,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, dev_priv->relative_constants_mode = instp_mode; } + intel_lr_emit_force_non_coherent(params, args, true); + exec_start = params->batch_obj_vm_offset + args->batch_start_offset; @@ -966,6 +986,8 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, if (ret) return ret; + int
[Intel-gfx] ✓ success: Fi.CI.BAT
== Summary == Built on dd4a7926b4118f72b7ae0f7b97e9644172df472c drm-intel-nightly: 2016y-01m-13d-09h-05m-34s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1161/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Init power domains early in driver load
Since commit ac9b8236551d1177fd07b56aef9b565d1864420d Author: Ville Syrjälä Date: Fri Nov 27 18:55:26 2015 +0200 drm/i915: Introduce a gmbus power domain gmbus also needs the power domain infrastructure right from the start, since as soon as we register the i2c controllers someone can use them. v2: Adjust cleanup paths too (Chris). v3: Rebase onto -nightly (totally bogus tree I had lying around) and also move dpio init head (Ville). v4: Ville instead suggested to move gmbus setup later in the sequence, since it's only needed by the modeset code. v5: Move even close to the actual user, right next to the comment that states where we really need gmbus (and interrupts!). Cc: Ville Syrjälä Cc: Patrik Jakobsson Cc: Imre Deak Cc: Jani Nikula Cc: Meelis Roos Cc: Chris Wilson Fixes: ac9b8236551d ("drm/i915: Introduce a gmbus power domain") Cc: sta...@vger.kernel.org References: http://www.spinics.net/lists/intel-gfx/msg83075.html Signed-off-by: Daniel Vetter --- Resending since our CI seems to have missed it. -Daniel --- drivers/gpu/drm/i915/i915_dma.c | 6 +++--- drivers/gpu/drm/i915/intel_display.c | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896ce32e6..a0f5659032fc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -406,6 +406,8 @@ static int i915_load_modeset_init(struct drm_device *dev) if (ret) goto cleanup_gem_stolen; + intel_setup_gmbus(dev); + /* Important: The output setup functions called by modeset_init need * working irqs for e.g. gmbus and dp aux transfers. */ intel_modeset_init(dev); @@ -455,6 +457,7 @@ cleanup_gem: cleanup_irq: intel_guc_ucode_fini(dev); drm_irq_uninstall(dev); + intel_teardown_gmbus(dev); cleanup_gem_stolen: i915_gem_cleanup_stolen(dev); cleanup_vga_switcheroo: @@ -1029,7 +1032,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) /* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar(dev); - intel_setup_gmbus(dev); intel_opregion_setup(dev); i915_gem_load(dev); @@ -1102,7 +1104,6 @@ out_gem_unload: if (dev->pdev->msi_enabled) pci_disable_msi(dev->pdev); - intel_teardown_gmbus(dev); intel_teardown_mchbar(dev); pm_qos_remove_request(&dev_priv->pm_qos); destroy_workqueue(dev_priv->gpu_error.hangcheck_wq); @@ -1204,7 +1205,6 @@ int i915_driver_unload(struct drm_device *dev) intel_csr_ucode_fini(dev_priv); - intel_teardown_gmbus(dev); intel_teardown_mchbar(dev); destroy_workqueue(dev_priv->hotplug.dp_wq); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 20a73d9e19d7..86fce41281fb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -16125,6 +16125,8 @@ void intel_modeset_cleanup(struct drm_device *dev) mutex_lock(&dev->struct_mutex); intel_cleanup_gt_powersave(dev); mutex_unlock(&dev->struct_mutex); + + intel_teardown_gmbus(dev); } /* -- 2.7.0.rc3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/22] drm/omap: Nuke close hooks
Hi Daniel, On 11/01/16 23:41, Daniel Vetter wrote: > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c > index dfafdb602ad2..603a65498b40 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -175,17 +175,6 @@ static int omap_atomic_commit(struct drm_device *dev, > priv->commit.pending |= commit->crtcs; > spin_unlock(&priv->commit.lock); > > - /* Keep track of all CRTC events to unlink them in preclose(). */ > - spin_lock_irqsave(&dev->event_lock, flags); > - for (i = 0; i < dev->mode_config.num_crtc; ++i) { > - struct drm_crtc_state *cstate = state->crtc_states[i]; > - > - if (cstate && cstate->event) > - list_add_tail(&cstate->event->base.link, > - &priv->commit.events); > - } > - spin_unlock_irqrestore(&dev->event_lock, flags); > - > /* Swap the state, this is the point of no return. */ > drm_atomic_helper_swap_state(dev, state); 'flags' local needs to be removed from omap_atomic_commit, otherwise: drivers/gpu/drm/omapdrm/omap_drv.c:145:16: error: unused variable 'flags' Tomi signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/omap: Nuke close hooks
Again since the core takes care of this we can remove them. While at it also remove the postclose hook, it's empty. v2: Laurent pointed me at even more code to delete. v3: Remove unused flags (Tomi). Cc: Laurent Pinchart Cc: Tomi Valkeinen Acked-by: Daniel Stone Reviewed-by: Alex Deucher Signed-off-by: Daniel Vetter --- drivers/gpu/drm/omapdrm/omap_crtc.c | 13 +--- drivers/gpu/drm/omapdrm/omap_drv.c | 42 - drivers/gpu/drm/omapdrm/omap_drv.h | 1 - 3 files changed, 1 insertion(+), 55 deletions(-) diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2ed0754ed19e..d38fcbcc43a8 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -269,18 +269,7 @@ static void omap_crtc_complete_page_flip(struct drm_crtc *crtc) return; spin_lock_irqsave(&dev->event_lock, flags); - - list_del(&event->base.link); - - /* -* Queue the event for delivery if it's still linked to a file -* handle, otherwise just destroy it. -*/ - if (event->base.file_priv) - drm_crtc_send_vblank_event(crtc, event); - else - event->base.destroy(&event->base); - + drm_crtc_send_vblank_event(crtc, event); spin_unlock_irqrestore(&dev->event_lock, flags); } diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index dfafdb602ad2..33370f42e4d7 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -142,7 +142,6 @@ static int omap_atomic_commit(struct drm_device *dev, { struct omap_drm_private *priv = dev->dev_private; struct omap_atomic_state_commit *commit; - unsigned long flags; unsigned int i; int ret; @@ -175,17 +174,6 @@ static int omap_atomic_commit(struct drm_device *dev, priv->commit.pending |= commit->crtcs; spin_unlock(&priv->commit.lock); - /* Keep track of all CRTC events to unlink them in preclose(). */ - spin_lock_irqsave(&dev->event_lock, flags); - for (i = 0; i < dev->mode_config.num_crtc; ++i) { - struct drm_crtc_state *cstate = state->crtc_states[i]; - - if (cstate && cstate->event) - list_add_tail(&cstate->event->base.link, - &priv->commit.events); - } - spin_unlock_irqrestore(&dev->event_lock, flags); - /* Swap the state, this is the point of no return. */ drm_atomic_helper_swap_state(dev, state); @@ -673,7 +661,6 @@ static int dev_load(struct drm_device *dev, unsigned long flags) priv->wq = alloc_ordered_workqueue("omapdrm", 0); init_waitqueue_head(&priv->commit.wait); spin_lock_init(&priv->commit.lock); - INIT_LIST_HEAD(&priv->commit.events); spin_lock_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list); @@ -787,33 +774,6 @@ static void dev_lastclose(struct drm_device *dev) } } -static void dev_preclose(struct drm_device *dev, struct drm_file *file) -{ - struct omap_drm_private *priv = dev->dev_private; - struct drm_pending_event *event; - unsigned long flags; - - DBG("preclose: dev=%p", dev); - - /* -* Unlink all pending CRTC events to make sure they won't be queued up -* by a pending asynchronous commit. -*/ - spin_lock_irqsave(&dev->event_lock, flags); - list_for_each_entry(event, &priv->commit.events, link) { - if (event->file_priv == file) { - file->event_space += event->event->length; - event->file_priv = NULL; - } - } - spin_unlock_irqrestore(&dev->event_lock, flags); -} - -static void dev_postclose(struct drm_device *dev, struct drm_file *file) -{ - DBG("postclose: dev=%p, file=%p", dev, file); -} - static const struct vm_operations_struct omap_gem_vm_ops = { .fault = omap_gem_fault, .open = drm_gem_vm_open, @@ -838,8 +798,6 @@ static struct drm_driver omap_drm_driver = { .unload = dev_unload, .open = dev_open, .lastclose = dev_lastclose, - .preclose = dev_preclose, - .postclose = dev_postclose, .set_busid = drm_platform_set_busid, .get_vblank_counter = drm_vblank_no_hw_counter, .enable_vblank = omap_irq_enable_vblank, diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 9e0030731c37..c23cbe6fe9e4 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -106,7 +106,6 @@ struct omap_drm_private { /* atomic commit */ struct { - struct list_head events; wait_queue_head_t wait; u32 pending; spinlock_t lock;/* Protects commit.pending */ -- 2.7.0.rc3 __
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Splitting intel_dp_detect
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > intel_dp_detect() is called for not just detection but > during modes enumeration as well. Repeating the whole > sequence during each of these calls is wasteful and > time consuming. > This patch moves probing for panel, DPCD read etc done in > intel_dp_detect() to a new function intel_dp_long_pulse(). > Note that the behavior of intel_dp_detect() is changed to > report connected or disconnected depending on whether the > EDID is available or not. > This change will be required by further patches in the series > to avoid performing duplicated DPCD operations on hotplug. > > v2: Moved a hunk to next patch of the series. > Moved intel_dp_unset_edid to out. (Ander) > v3: Rephrased commit message and intel_dp_unset_dp() is called > within intel_dp_set_dp() to free the previous EDID. (Ander) > > Tested-by: Nathan D Ciobanu > Signed-off-by: Sivakumar Thulasimani > Signed-off-by: Shubhangi Shrivastava > --- > drivers/gpu/drm/i915/intel_dp.c | 56 +--- > - > 1 file changed, 35 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 796e3d3..e3b4208 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, > bool sync); > static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > static void vlv_steal_power_sequencer(struct drm_device *dev, > enum pipe pipe); > +static void intel_dp_unset_edid(struct intel_dp *intel_dp); > > static unsigned int intel_dp_unused_lane_mask(int lane_count) > { > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) > struct intel_connector *intel_connector = intel_dp > ->attached_connector; > struct edid *edid; > > + intel_dp_unset_edid(intel_dp); > edid = intel_dp_get_edid(intel_dp); > intel_connector->detect_edid = edid; > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > intel_dp->has_audio = false; > } > > -static enum drm_connector_status > -intel_dp_detect(struct drm_connector *connector, bool force) > +static void > +intel_dp_long_pulse(struct intel_connector *intel_connector) > { > + struct drm_connector *connector = &intel_connector->base; > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > bool ret; > u8 sink_irq_vector; > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > - connector->base.id, connector->name); > - intel_dp_unset_edid(intel_dp); > - > - if (intel_dp->is_mst) { > - /* MST devices are disconnected from a monitor POV */ > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > - return connector_status_disconnected; > - } > - > power_domain = intel_display_port_aux_power_domain(intel_encoder); > intel_display_power_get(to_i915(dev), power_domain); > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > intel_dp_probe_oui(intel_dp); > > ret = intel_dp_probe_mst(intel_dp); > - if (ret) { > - /* if we are in MST mode then this connector > -won't appear connected or have anything with EDID on it */ > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; This deletion is new in this version of the patch. I think we still need the hunk above, otherwise we might not properly update the encoder type when we switch from an HDMI sink connected through a level shifter to an MST sink. Ander > - status = connector_status_disconnected; > + if (ret) > goto out; > - } > > /* >* Clearing NACK and defer counts to get their exact values > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > } > > out: > + if (status != connector_status_connected) > + intel_dp_unset_edid(intel_dp); > intel_display_power_put(to_i915(dev), power_domain); > - return status; > + return; > +} > + > +static enum drm_connector_status > +intel_dp_detect(struct drm_connector *connector, bool force) > +{ > + struct intel_dp *intel_dp = intel_attached_dp(connector); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct intel_connector *intel_connector = > to_intel_connector(connector); > + > +
Re: [Intel-gfx] [PATCH 20/22] drm/tilcdc: Nuke preclose hook
On 12/01/16 17:12, Daniel Vetter wrote: > Different approaches to the same problem: > > - omap just unlinks the event from fpriv and still process it normally. > But then before sending it out it checks whether the fpriv is still > there or not and either sends it, or deletes the event directly. This > way the vblank_put is always called from the worker/irq handler as part > of the event processing. > > This is the same approach I implemented in core with this series. > > - tilcdc (and most other drivers) entirely destroy the event in the > preclose hook, which means they must also release any other resources > acquired as part of that event. Therefore they have the vblank_put here. > But the vblank_put is obviously also in the normal event processing > paths, so with the new approach of only unlinking it we can handle this > without any special cases in the driver. > > I hope this explains what's going on. Since you're about driver maintainer > no. 3 with the same question: Can you pls review the kerneldoc and make > sure it explains this well? I tried to improve it already a bit after > Laurent's/Thomas' questions. Ok, makes sense. I think the kernel doc is fine. I wasn't able to test tilcdc, as it seems to crash on drm-next at the moment... Need to debug that first. Tomi signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
Arun Siluvery writes: > Some of the HW registers are privileged and cannot be written to from a > non-privileged batch buffers coming from userspace unless they are on > whitelist. > Userspace need write access to them to implement preemption related WA. > > The reason for using this approach is, the register bits that control > preemption > granularity at the HW level are not context save/restored; so even if we set > these > bits always in kernel they are going to change once the context is switched > out. > We can consider making them non-privileged by default but these registers also > contain other chicken bits which should not be allowed to be modified. > > In the later revisions controlling bits are save/restored at context level but > in the existing revisions these are exported via other debug registers and > should > be on the whitelist. This patch adds changes to provide HW with a list of > registers > to be whitelisted. HW checks this list during execution and provides access > accordingly. > > HW imposes a limit on the number of registers on whitelist and it is > per-engine. > At this point we are only enabling whitelist for RCS and we don't foresee any > requirement for other engines. > > The registers to be whitelisted are added using generic workaround list > mechanism, > even these are only enablers for userspace workarounds. But by sharing this > mechanism we get some test assets without additional cost (Mika). > > v2: rebase > > Reviewed-by: Mika Kuoppala > Cc: Mika Kuoppala > Signed-off-by: Arun Siluvery > --- > drivers/gpu/drm/i915/i915_drv.h | 9 - > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++ > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 104bd18..660afe1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1653,11 +1653,18 @@ struct i915_wa_reg { > u32 mask; > }; > > -#define I915_MAX_WA_REGS 16 > +/* > + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only > + * allowing it for RCS as we don't foresee any requirement of having > + * a whitelist for other engines. When it is really required for > + * other engines then the limit need to be increased. > + */ > +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) > > struct i915_workarounds { > struct i915_wa_reg reg[I915_MAX_WA_REGS]; > u32 count; > + u32 hw_whitelist_index[I915_NUM_RINGS]; > }; > > struct i915_virtual_gpu { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 0a98889..6668bb0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells { > #define RING_WAIT (1<<11) /* gen3+, PRBx_CTL */ > #define RING_WAIT_SEMAPHORE(1<<10) /* gen6+ */ > > +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0) > +#define RING_MAX_NONPRIV_SLOTS 12 > + > #define GEN7_TLB_RD_ADDR _MMIO(0x4700) > > #if 0 > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 4060acf..354da81 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv, > > #define WA_WRITE(addr, val) WA_REG(addr, 0x, val) > > +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring, > + i915_reg_t reg_addr) When you remove the _MMIO Ville noted, please also remove the inline here too. -Mika > +{ > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > + struct i915_workarounds *wa = &dev_priv->workarounds; > + const uint32_t index = wa->hw_whitelist_index[ring->id]; > + > + if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > + return -EINVAL; > + > + WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4), > + reg_addr.reg); > + wa->hw_whitelist_index[ring->id]++; > + > + return 0; > +} > + > static int gen8_init_workarounds(struct intel_engine_cs *ring) > { > struct drm_device *dev = ring->dev; > @@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) > WARN_ON(ring->id != RCS); > > dev_priv->workarounds.count = 0; > + dev_priv->workarounds.hw_whitelist_index[ring->id] = 0; > > if (IS_BROADWELL(dev)) > return bdw_init_workarounds(ring); > -- > 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ success: Fi.CI.BAT
== Summary == Built on 8da57dfe6c675c35109dac986e3f8b627cffab49 drm-intel-nightly: 2016y-01m-13d-10h-33m-04s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE dmesg-warn -> PASS (bdw-nuci7) dmesg-warn -> PASS (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (ilk-hp8440p) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-b: dmesg-warn -> PASS (bdw-ultra) Subgroup suspend-read-crc-pipe-a: dmesg-warn -> PASS (snb-x220t) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1162/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ warning: Fi.CI.BAT
== Summary == Built on 8da57dfe6c675c35109dac986e3f8b627cffab49 drm-intel-nightly: 2016y-01m-13d-10h-33m-04s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE dmesg-warn -> PASS (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (ilk-hp8440p) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-b: dmesg-warn -> PASS (bdw-ultra) Subgroup read-crc-pipe-c: pass -> DMESG-WARN (bdw-ultra) Subgroup suspend-read-crc-pipe-a: dmesg-warn -> PASS (snb-x220t) bdw-nuci7total:138 pass:128 dwarn:1 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1163/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: reboot notifier delay for eDP panels
On Tue, Jan 12, 2016 at 09:06:24AM -0800, Clint Taylor wrote: > On 01/12/2016 05:21 AM, Ville Syrjälä wrote: > > On Mon, Jan 11, 2016 at 01:52:17PM -0800, clinton.a.tay...@intel.com wrote: > >> From: Clint Taylor > >> > >> Add reboot notifier for all platforms. This guarantees T12 delay > >> compliance during reboot cycles when pre-os enables the panel within > >> 500ms. > >> > >> Signed-off-by: Clint Taylor > >> --- > >> drivers/gpu/drm/i915/intel_dp.c | 11 --- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c > >> b/drivers/gpu/drm/i915/intel_dp.c > >> index 796e3d3..dbbd27a 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -126,6 +126,7 @@ static struct intel_dp *intel_attached_dp(struct > >> drm_connector *connector) > >> static void intel_dp_link_down(struct intel_dp *intel_dp); > >> static bool edp_panel_vdd_on(struct intel_dp *intel_dp); > >> static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync); > >> +static void edp_panel_off(struct intel_dp *intel_dp); > >> static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > >> static void vlv_steal_power_sequencer(struct drm_device *dev, > >> enum pipe pipe); > >> @@ -596,6 +597,10 @@ static int edp_notify_handler(struct notifier_block > >> *this, unsigned long code, > >>I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF); > >>msleep(intel_dp->panel_power_cycle_delay); > >>} > >> + else > >> + { > >> + edp_panel_off(intel_dp); > >> + } > > > > I don't think that actually waits for the power cycle delay. Also > > you can't call it without having vdd already enabled unless you > > specifically want to see a WARN. > > > edp_panel_off() does wait for panel off time. This has also been > confirmed with a scope watching vdd. Panel off delay yes, but not power cycle delay. Unless you wait for that as well, then the firmware/whoever could re-enable vdd too soon AFAICS. > > > I suppose you could just do something along these lines: > > > > if (have_panel_power || have_panel_vdd) { > Good catch. I will add the check to make sure panel power is on before > calling edp_panel_off(). If the panel power was off I will still need to > check to make sure it has been off for the required time. Which is why my sample code had the wait_panel_power_cycle() call uncnditionally. > > > edp_panel_vdd_on > > edp_panel_off > > } > > wait_panel_power_cycle > > > > Also should change VLV/CHV to do it the same way. > > VLV/CHV PPS handles things a little different and the existing code was > required to prevent VDD from asserting during the reboot cycle. We > increased the delay to MAX the VLV handler to prevent the VDD from > asserting once the PPS cycle completed. That doesn't sound sane at all. Are you saying the hardware re-enabled vdd on its own somehow unless you frobbed with the delay? > I will test on my CHV to confirm > the old handler is still required. > > > > >> > >>pps_unlock(intel_dp); > >> > >> @@ -5796,10 +5801,10 @@ static bool intel_edp_init_connector(struct > >> intel_dp *intel_dp, > >>} > >>mutex_unlock(&dev->mode_config.mutex); > >> > >> - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > >> - intel_dp->edp_notifier.notifier_call = edp_notify_handler; > >> - register_reboot_notifier(&intel_dp->edp_notifier); > >> + intel_dp->edp_notifier.notifier_call = edp_notify_handler; > >> + register_reboot_notifier(&intel_dp->edp_notifier); > >> > >> + if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > >>/* > >> * Figure out the current pipe for the initial backlight setup. > >> * If the current pipe isn't valid, try the PPS pipe, and if > >> that > >> -- > >> 1.7.9.5 > >> > >> ___ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
Some of the HW registers are privileged and cannot be written to from a non-privileged batch buffers coming from userspace unless they are on whitelist. Userspace need write access to them to implement preemption related WA. The reason for using this approach is, the register bits that control preemption granularity at the HW level are not context save/restored; so even if we set these bits always in kernel they are going to change once the context is switched out. We can consider making them non-privileged by default but these registers also contain other chicken bits which should not be allowed to be modified. In the later revisions controlling bits are save/restored at context level but in the existing revisions these are exported via other debug registers and should be on the whitelist. This patch adds changes to provide HW with a list of registers to be whitelisted. HW checks this list during execution and provides access accordingly. HW imposes a limit on the number of registers on whitelist and it is per-engine. At this point we are only enabling whitelist for RCS and we don't foresee any requirement for other engines. The registers to be whitelisted are added using generic workaround list mechanism, even these are only enablers for userspace workarounds. But by sharing this mechanism we get some test assets without additional cost (Mika). v2: rebase v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika). Reviewed-by: Mika Kuoppala Cc: Mika Kuoppala Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_drv.h | 9 - drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 17 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..660afe1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1653,11 +1653,18 @@ struct i915_wa_reg { u32 mask; }; -#define I915_MAX_WA_REGS 16 +/* + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only + * allowing it for RCS as we don't foresee any requirement of having + * a whitelist for other engines. When it is really required for + * other engines then the limit need to be increased. + */ +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) struct i915_workarounds { struct i915_wa_reg reg[I915_MAX_WA_REGS]; u32 count; + u32 hw_whitelist_index[I915_NUM_RINGS]; }; struct i915_virtual_gpu { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..7938814 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells { #define RING_WAIT(1<<11) /* gen3+, PRBx_CTL */ #define RING_WAIT_SEMAPHORE (1<<10) /* gen6+ */ +#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4) +#define RING_MAX_NONPRIV_SLOTS 12 + #define GEN7_TLB_RD_ADDR _MMIO(0x4700) #if 0 diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 4060acf..279dd51 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv, #define WA_WRITE(addr, val) WA_REG(addr, 0x, val) +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, +i915_reg_t reg_addr) +{ + struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct i915_workarounds *wa = &dev_priv->workarounds; + const uint32_t index = wa->hw_whitelist_index[ring->id]; + + if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) + return -EINVAL; + + WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg); + wa->hw_whitelist_index[ring->id]++; + + return 0; +} + static int gen8_init_workarounds(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; @@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) WARN_ON(ring->id != RCS); dev_priv->workarounds.count = 0; + dev_priv->workarounds.hw_whitelist_index[ring->id] = 0; if (IS_BROADWELL(dev)) return bdw_init_workarounds(ring); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 10/38] drm/i915: Force MMIO flips when scheduler enabled
On 12/01/2016 21:53, Chris Wilson wrote: On Tue, Jan 12, 2016 at 03:07:03PM +0100, Daniel Vetter wrote: On Tue, Jan 12, 2016 at 11:19:26AM +, John Harrison wrote: On 11/01/2016 22:16, Chris Wilson wrote: On Mon, Jan 11, 2016 at 06:42:39PM +, john.c.harri...@intel.com wrote: From: John Harrison MMIO flips are the preferred mechanism now but more importantly, Says who? I asked this exact question at the linux architecture forum quite some time ago - does the scheduler need to worry about managing non-batch buffer work such as page flips. The answer from everyone present was no, MMIO flips are the way to go so don't over complicate the scheduler trying to support ring flips. Indeed, execlist mode already forces MMIO flips anyway. Two wrongs do not make a right, as they say. CS flips work very nicely with execlists. They might have done at one point but if you don't test it then it don't work and right now it ain't being tested because: static bool use_mmio_flip(struct intel_engine_cs *ring, struct drm_i915_gem_object *obj) ... else if (i915.enable_execlists) return true; Atomic will kill CS flips. We can mourn them and scream about the loss, but imo best is to just skip that all and move on to acceptance. So mmio flips (or well, atomic flips) is still the way to go for everything. The real issue I think here is that not trying to feed a request into the scheduler for the flip has lead to a poor interface into the scheduler. For a CS flip request, we know the ordering, it's contents, we have to choose the context though, but we have a good idea of the deadline which gives a good challenge to a scheduler. That was my take. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: tidy up a few leftovers
On 12/01/16 13:53, Daniel Vetter wrote: On Thu, Jan 07, 2016 at 10:20:52AM +, Dave Gordon wrote: There are a few bits of code which the transformations implemented by the previous patch reveal to be suboptimal, once the notion of a per- ring default context has gone away. So this tidies up the leftovers. It could have been squashed into the previous patch, but that would have made that patch less clearly a simple transformation. In particular, any change which alters the code block structure or indentation has been deferred into this separate patch, because such things tend to make diffs more difficult to read. Signed-off-by: Dave Gordon Yeah I think this was good to be split up, since I'm not convinced it's a net improvement (from a quick look at least). Still plenty of default context checks that seem superfluous (e.g. not pinning the default context when going through execlist submission is imo a needless special case - besides that we really should use active tracking). So I'll refrain from ack or nack on this one. -Daniel --- drivers/gpu/drm/i915/i915_debugfs.c | 15 +-- drivers/gpu/drm/i915/i915_gem.c | 6 ++ drivers/gpu/drm/i915/intel_lrc.c| 38 + 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2613708..bbb23da 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1942,11 +1942,8 @@ static int i915_context_status(struct seq_file *m, void *unused) seq_puts(m, "HW context "); describe_ctx(m, ctx); - for_each_ring(ring, dev_priv, i) { - if (dev_priv->kernel_context == ctx) - seq_printf(m, "(default context %s) ", - ring->name); - } + if (ctx == dev_priv->kernel_context) + seq_printf(m, "(kernel context) "); This is replacing the multiple messages in the debugfs output: "(default context render ring) (default context blitter ring) (default context ..." (which are redundant because the same context is the default for all rings) with the single more concise message "(kernel context)". One part of one of Chris' patches does the same or an equivalent thing, so I don't think it's contentious. if (i915.enable_execlists) { seq_putc(m, '\n'); @@ -2037,13 +2034,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused) if (ret) return ret; - list_for_each_entry(ctx, &dev_priv->context_list, link) { - for_each_ring(ring, dev_priv, i) { - if (dev_priv->kernel_context != ctx) + list_for_each_entry(ctx, &dev_priv->context_list, link) + if (ctx != dev_priv->kernel_context) + for_each_ring(ring, dev_priv, i) i915_dump_lrc_obj(m, ring, ctx->engine[i].state); - } - } Since we know that there is only one kernel context, the if() should be outside rather than outside the for_each_ring(). The {} are redundant anyway. mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8f101121..4f45eb2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2677,10 +2677,8 @@ void i915_gem_request_free(struct kref *req_ref) i915_gem_request_remove_from_client(req); if (ctx) { - if (i915.enable_execlists) { - if (ctx != req->i915->kernel_context) - intel_lr_context_unpin(req); - } + if (i915.enable_execlists && ctx != req->i915->kernel_context) + intel_lr_context_unpin(req); Trivial rewrite of the nested if() to reduce indentation. Actually removing this test is left for a subsequent rationalisation of the execlist code (one of Chris' patches will do it). i915_gem_context_unreference(ctx); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 5a3..8c4c9b9 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -660,16 +660,10 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req, int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request) { - int ret; + int ret = 0; request->ringbuf = request->ctx->engine[request->ring->id].ringbuf; - if (request->ctx != request->i915->kernel_context) { - ret = intel_lr_context_pin(request); - if (ret) - return ret; - } - if (i915.enable_guc_submission) {
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On 11/01/16 16:56, Chris Wilson wrote: On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote: On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin wrote: Don't know, I leave this one to whoever grabbed the lock around intel_init_gt_powersave in the first place. Maybe there was a special reason.. after git blame od intel_display.c eventually completed, adding Imre and Ville to cc. Hmm. I don't recall the details anymore, but looking at the code pushing the locking down to valleyview_setup_pctx() looks entirely reasonable to me. iirc, this locking only exists to keep the WARN() at bay. But it is pedagogical, I guess. Don't really know this area, but what about the intel_gen6_powersave_work->valleyview_enable_rps->valleyview_check_pctx which dereferences the dev_priv->vlv_pctx, which is set/cleared in valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be outside both struct_mutex and the rps lock? Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix for reserved space WARN_ON when ring begin fails
From: John Harrison The reserved space code was not cleaning up properly in the case where the intel_ring_begin() call failed. This led to WARN_ONs firing about a double reserve call when running the gem_reset_stats IGT test. Signed-off-by: John Harrison --- drivers/gpu/drm/i915/intel_lrc.c| 8 +++- drivers/gpu/drm/i915/intel_ringbuffer.c | 8 +++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 06180dc..7dcc299 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -835,6 +835,8 @@ int intel_logical_ring_begin(struct drm_i915_gem_request *req, int num_dwords) int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request) { + int ret; + /* * The first call merely notes the reserve request and is common for * all back ends. The subsequent localised _begin() call actually @@ -845,7 +847,11 @@ int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request) */ intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST); - return intel_logical_ring_begin(request, 0); + ret = intel_logical_ring_begin(request, 0); + if (ret) + intel_ring_reserved_space_cancel(request->ringbuf); + + return ret; } /** diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index c9b081f..f1d3a4a 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2296,6 +2296,8 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request) int intel_ring_reserve_space(struct drm_i915_gem_request *request) { + int ret; + /* * The first call merely notes the reserve request and is common for * all back ends. The subsequent localised _begin() call actually @@ -2306,7 +2308,11 @@ int intel_ring_reserve_space(struct drm_i915_gem_request *request) */ intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST); - return intel_ring_begin(request, 0); + ret = intel_ring_begin(request, 0); + if (ret) + intel_ring_reserved_space_cancel(request->ringbuf); + + return ret; } void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Expose exec parameter to force non IA-Coherent for Gen9+
On Wed, Jan 13, 2016 at 11:44:39AM +0100, Artur Harasimiuk wrote: > Starting from Gen9 we can use IA-Coherent caches. Coherence may be not > required in certain cases and can be disabled in an easy way. To do this > we can set HDC_FORCE_NON_COHERENT bit in HDC_CHICKEN0 register. This > register is part of HW context, however it is private and cannot be > programmed from non-privileged batch buffer. > > New parameter is to override default programming and allow UMD to > decide whether IA-Coherency is not needed for submitted batch buffer. > When flag is set KMD emits commands to disable coherency before batch > buffer execution starts. After execution finished state is restored. What's the usecase for batch vs context flag? > When WaForceEnableNonCoherent is programmed, it does not make sense to > allow for coherency because this can lead to GPU hangs. In such > situation flag is ignored. > > Signed-off-by: Artur Harasimiuk > --- > drivers/gpu/drm/i915/i915_dma.c| 3 +++ > drivers/gpu/drm/i915/i915_drv.h| 4 > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 > drivers/gpu/drm/i915/intel_lrc.c | 28 > drivers/gpu/drm/i915/intel_ringbuffer.c| 7 +++ > include/uapi/drm/i915_drm.h| 8 +++- > 6 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 44a896c..79ecf20 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -172,6 +172,9 @@ static int i915_getparam(struct drm_device *dev, void > *data, > case I915_PARAM_HAS_EXEC_SOFTPIN: > value = 1; > break; > + case I915_PARAM_HAS_EXEC_FORCE_NON_COHERENT: > + value = INTEL_INFO(dev)->gen >= 9; This should actually report the w/a status, or else you need to provide some other means for userspace to detect if it can successfully force non-coherency. > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 104bd18..71d739c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -886,6 +886,10 @@ struct intel_context { > } engine[I915_NUM_RINGS]; > > struct list_head link; > + > + struct { > + unsigned int WaForceEnableNonCoherent:1; > + } wa; > }; > > enum fb_op_origin { > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index d469c47..2997a58 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1400,6 +1400,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void > *data, > if (!i915_gem_check_execbuffer(args)) > return -EINVAL; > > + if ((args->flags & I915_EXEC_FORCE_NON_COHERENT) && > + INTEL_INFO(dev)->gen < 9) > + return -EINVAL; It would seem easier to simply ignore this request in all situations where it doesn't apply, rather than the current select few. > ret = validate_exec_list(dev, exec, args->buffer_count); > if (ret) > return ret; > @@ -1494,6 +1498,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void > *data, > > i915_gem_context_reference(ctx); > > + /* Clear this flag when WA is programmed */ > + if (ctx->wa.WaForceEnableNonCoherent) > + args->flags &= ~I915_EXEC_FORCE_NON_COHERENT; > + > if (ctx->ppgtt) > vm = &ctx->ppgtt->base; > else > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index ab344e0..4482a6a 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -879,6 +879,24 @@ int intel_logical_ring_reserve_space(struct > drm_i915_gem_request *request) > return intel_logical_ring_begin(request, 0); > } > > +static inline void > +intel_lr_emit_force_non_coherent(struct i915_execbuffer_params *params, > + struct drm_i915_gem_execbuffer2 *args, bool force) > +{ > + if (args->flags & I915_EXEC_FORCE_NON_COHERENT) { > + struct intel_ringbuffer *ringbuf = > + params->ctx->engine[params->ring->id].ringbuf; This should be using the request->ringbuf > + Missing ring_begin. > + intel_logical_ring_emit(ringbuf, MI_NOOP); > + intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(1)); > + intel_logical_ring_emit(ringbuf, HDC_CHICKEN0.reg); intel_logical_ring_emit_reg(ringbuf, HDC_CHICKEN0); > + intel_logical_ring_emit(ringbuf, force ? > + _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT) : > + _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT));
Re: [Intel-gfx] [PATCH v2 2/9] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v3.
On Mon, Jan 11, 2016 at 01:27:42PM +0100, Maarten Lankhorst wrote: > Currently we perform our own wait in post_plane_update, > but the atomic core performs another one in wait_for_vblanks. > This means that 2 vblanks are done when a fb is changed, > which is a bit overkill. > > Merge them by creating a helper function that takes a crtc mask > for the planes to wait on. > > The broadwell vblank workaround may look gone entirely but this is > not the case. pipe_config->wm_changed is set to true > when any plane is turned on, which forces a vblank wait. Still NAK, because you just removed the comment entirely. > > Changes since v1: > - Removing the double vblank wait on broadwell moved to its own commit. > Changes since v2: > - Move out POWER_DOMAIN_MODESET handling to its own commit. > > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/intel_atomic.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 80 > ++-- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 61 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 9682d94af710..ba9a57f33c43 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->disable_cxsr = false; > crtc_state->wm_changed = false; > crtc_state->wm.need_postvbl_update = false; > + crtc_state->fb_changed = false; > > return &crtc_state->base; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 2aa1c5367625..eac73f005a70 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4796,9 +4796,6 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > to_intel_crtc_state(crtc->base.state); > struct drm_device *dev = crtc->base.dev; > > - if (atomic->wait_vblank) > - intel_wait_for_vblank(dev, crtc->pipe); > - > intel_frontbuffer_flip(dev, atomic->fb_bits); > > crtc->wm.cxsr_allowed = true; > @@ -11872,6 +11869,9 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (!was_visible && !visible) > return 0; > > + if (fb != old_plane_state->base.fb) > + pipe_config->fb_changed = true; Isn't that going to slow down cursor updates once again? > + > turn_off = was_visible && (!visible || mode_changed); > turn_on = visible && (!was_visible || mode_changed); > > @@ -11887,8 +11887,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - if (is_crtc_enabled) > - intel_crtc->atomic.wait_vblank = true; > pipe_config->disable_cxsr = true; > } > } else if (intel_wm_need_update(plane, plane_state)) { > @@ -11940,14 +11938,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > plane_state->rotation != BIT(DRM_ROTATE_0)) > intel_crtc->atomic.disable_fbc = true; > > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (turn_on && IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - > intel_crtc->atomic.update_fbc |= visible || mode_changed; > break; > case DRM_PLANE_TYPE_CURSOR: > @@ -11962,12 +11952,10 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (IS_IVYBRIDGE(dev) && > needs_scaling(to_intel_plane_state(plane_state)) && > !needs_scaling(old_plane_state)) { > - to_intel_crtc_state(crtc_state)->disable_lp_wm = true; > - } else if (turn_off && !mode_changed) { > - intel_crtc->atomic.wait_vblank = true; > + pipe_config->disable_lp_wm = true; > + } else if (turn_off && !mode_changed) > intel_crtc->atomic.update_sprite_watermarks |= > 1 << i; > - } > > break; > } > @@ -13509,6 +13497,48 @@ static int intel_atomic_prepare_commit(struct > drm_device *dev, > return ret; > } > > +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
Re: [Intel-gfx] [PATCH v2 3/9] drm/i915: Remove intel_crtc->atomic.disable_ips.
On Mon, Jan 11, 2016 at 01:27:43PM +0100, Maarten Lankhorst wrote: > This is a revert of commit 066cf55b9ce3 "drm/i915: Fix IPS related flicker". > intel_pre_disable_primary already handles this, and now everything > goes through the atomic path there's no need to try to disable ips twice. If it's a revert why wasn't it done with 'git revert'? Also I'm not sure it isn't a step backwards. Based on the spec we should be able to keep IPS enabled as long as one plane (possibly referring to either primary or sprite) is enabled on the pipe. So in theory we should keep the IPS handling out of the primary plane code, and instead we should compute the IPS state based on the set of active planes on the pipe. > > Signed-off-by: Maarten Lankhorst > Reviewed-by: Ander Conselvan de Oliveira > --- > drivers/gpu/drm/i915/intel_display.c | 16 +--- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 1 insertion(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index eac73f005a70..62044ad5c6ec 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4823,9 +4823,6 @@ static void intel_pre_plane_update(struct intel_crtc > *crtc) > if (atomic->disable_fbc) > intel_fbc_deactivate(crtc); > > - if (crtc->atomic.disable_ips) > - hsw_disable_ips(crtc); > - > if (atomic->pre_disable_primary) > intel_pre_disable_primary(&crtc->base); > > @@ -11907,19 +11904,8 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > intel_crtc->atomic.pre_disable_primary = turn_off; > intel_crtc->atomic.post_enable_primary = turn_on; > > - if (turn_off) { > - /* > - * FIXME: Actually if we will still have any other > - * plane enabled on the pipe we could let IPS enabled > - * still, but for now lets consider that when we make > - * primary invisible by setting DSPCNTR to 0 on > - * update_primary_plane function IPS needs to be > - * disable. > - */ > - intel_crtc->atomic.disable_ips = true; > - > + if (turn_off) > intel_crtc->atomic.disable_fbc = true; > - } > > /* >* FBC does not work on some platforms for rotated > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 8940aa4cf50c..39adf7cd0b36 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -566,7 +566,6 @@ struct intel_mmio_flip { > struct intel_crtc_atomic_commit { > /* Sleepable operations to perform before commit */ > bool disable_fbc; > - bool disable_ips; > bool pre_disable_primary; > > /* Sleepable operations to perform after commit */ > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
On Wed, Jan 13, 2016 at 12:28:17PM +, Arun Siluvery wrote: > Some of the HW registers are privileged and cannot be written to from a > non-privileged batch buffers coming from userspace unless they are on > whitelist. > Userspace need write access to them to implement preemption related WA. You need to be clear that this is the hw whitelist and not our sw whitelist. > The reason for using this approach is, the register bits that control > preemption > granularity at the HW level are not context save/restored; so even if we set > these > bits always in kernel they are going to change once the context is switched > out. > We can consider making them non-privileged by default but these registers also > contain other chicken bits which should not be allowed to be modified. > > In the later revisions controlling bits are save/restored at context level but > in the existing revisions these are exported via other debug registers and > should > be on the whitelist. This patch adds changes to provide HW with a list of > registers > to be whitelisted. HW checks this list during execution and provides access > accordingly. > > HW imposes a limit on the number of registers on whitelist and it is > per-engine. > At this point we are only enabling whitelist for RCS and we don't foresee any > requirement for other engines. > > The registers to be whitelisted are added using generic workaround list > mechanism, > even these are only enablers for userspace workarounds. But by sharing this > mechanism we get some test assets without additional cost (Mika). > > v2: rebase > > v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to > i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika). > > Reviewed-by: Mika Kuoppala > Cc: Mika Kuoppala > Signed-off-by: Arun Siluvery > --- > drivers/gpu/drm/i915/i915_drv.h | 9 - > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 17 + > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 104bd18..660afe1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1653,11 +1653,18 @@ struct i915_wa_reg { > u32 mask; > }; > > -#define I915_MAX_WA_REGS 16 > +/* > + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only > + * allowing it for RCS as we don't foresee any requirement of having > + * a whitelist for other engines. When it is really required for > + * other engines then the limit need to be increased. > + */ > +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) > > struct i915_workarounds { > struct i915_wa_reg reg[I915_MAX_WA_REGS]; > u32 count; > + u32 hw_whitelist_index[I915_NUM_RINGS]; This is a counter only to be used whilst constructing the reg list. It also implies that this list of wa_reg is now engine specific. Hmm, looks like it always has been, just been relying on RCS running first. I would fix that up first. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix for reserved space WARN_ON when ring begin fails
On Wed, Jan 13, 2016 at 12:52:40PM +, john.c.harri...@intel.com wrote: > From: John Harrison > > The reserved space code was not cleaning up properly in the case where > the intel_ring_begin() call failed. This led to WARN_ONs firing about > a double reserve call when running the gem_reset_stats IGT test. *reserve_space() failure leads to a i915_gem_request_cancel() which calls intel_ring_reserved_space_cancel(). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] vsync + vaapi question
Hi, On Thu, Dec 24, 2015 at 08:42:01PM +0100, Lukas Hejtmanek wrote: > > Hard? The fault isn't in libva this time at least. > > applied patch results in hard lockup when playing a movie (short sound loop > and even sysrq keys are not working). I thought that driver would reset gpu > eventualy but it seems to be completely dead. > > So I reverted the patch and it seems lockups are gone. For ubuntu, that vaapi sync patch just landed. Had to switch to xv from vaapi to avoid frequent lockup. Anything out there I can try to fix lockups? -- Lukáš Hejtmánek ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 10/38] drm/i915: Force MMIO flips when scheduler enabled
On Wed, Jan 13, 2016 at 12:37:44PM +, John Harrison wrote: > On 12/01/2016 21:53, Chris Wilson wrote: > >On Tue, Jan 12, 2016 at 03:07:03PM +0100, Daniel Vetter wrote: > >>On Tue, Jan 12, 2016 at 11:19:26AM +, John Harrison wrote: > >>>On 11/01/2016 22:16, Chris Wilson wrote: > On Mon, Jan 11, 2016 at 06:42:39PM +, john.c.harri...@intel.com wrote: > >From: John Harrison > > > >MMIO flips are the preferred mechanism now but more importantly, > Says who? > >>>I asked this exact question at the linux architecture forum quite some time > >>>ago - does the scheduler need to worry about managing non-batch buffer work > >>>such as page flips. The answer from everyone present was no, MMIO flips are > >>>the way to go so don't over complicate the scheduler trying to support ring > >>>flips. Indeed, execlist mode already forces MMIO flips anyway. > >Two wrongs do not make a right, as they say. CS flips work very nicely > >with execlists. > They might have done at one point but if you don't test it then it > don't work and right now it ain't being tested because: > static bool use_mmio_flip(struct intel_engine_cs *ring, > struct drm_i915_gem_object *obj) > ... > else if (i915.enable_execlists) > return true; Indeed, but that is not what I have in my kernels. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests
On 12/01/16 14:27, Chris Wilson wrote: On Tue, Jan 12, 2016 at 01:56:48PM +, Chris Wilson wrote: But we were removing the engine->default_context as it complicated the rest of the code. I strongly prefer keeping the contexts explicit as context separation should be first and foremost in the driver. $ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc drivers/gpu/drm/i915/i915_gem_evict.c: req = i915_gem_request_alloc(ring, dev_priv->kernel_context); drivers/gpu/drm/i915/intel_overlay.c: return i915_gem_request_alloc(ring, dev_priv->kernel_context); Changing those *two* callsites to pass NULL seems on the odd side, and at least for the eviction case discards important information. -Chris Those specific lines won't be touched by my patch, as *they don't actually exist in today's drm-intel-nightly* branch. If you want to add *new* calls to i915_gem_request_alloc() such as the above then you're quite free to pass any context you want, whether it's a real user context, the default kernel context explicitly, if you think it's important that the reader know that that specific context will be used; or NULL if you don't care what context is used. dev_priv->kernel_context carries exactly the same amount of information as NULL; they both mean "I don't have a specific context to use here, so (I'm going to) use the one the driver provides for such activities". Having the option of using NULL rather than"dev_priv->kernel_context" explicitly doesn't prevent you from doing so where the caller cares. But I think most callers *don't* care. .Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Splitting intel_dp_detect
On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote: > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > > intel_dp_detect() is called for not just detection but > > during modes enumeration as well. Repeating the whole > > sequence during each of these calls is wasteful and > > time consuming. > > This patch moves probing for panel, DPCD read etc done in > > intel_dp_detect() to a new function intel_dp_long_pulse(). > > Note that the behavior of intel_dp_detect() is changed to > > report connected or disconnected depending on whether the > > EDID is available or not. > > This change will be required by further patches in the series > > to avoid performing duplicated DPCD operations on hotplug. > > > > v2: Moved a hunk to next patch of the series. > > Moved intel_dp_unset_edid to out. (Ander) > > v3: Rephrased commit message and intel_dp_unset_dp() is called > > within intel_dp_set_dp() to free the previous EDID. (Ander) > > > > Tested-by: Nathan D Ciobanu > > Signed-off-by: Sivakumar Thulasimani > > Signed-off-by: Shubhangi Shrivastava > > --- > > drivers/gpu/drm/i915/intel_dp.c | 56 +- > > -- > > - > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 796e3d3..e3b4208 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, > > bool sync); > > static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > > static void vlv_steal_power_sequencer(struct drm_device *dev, > > enum pipe pipe); > > +static void intel_dp_unset_edid(struct intel_dp *intel_dp); > > > > static unsigned int intel_dp_unused_lane_mask(int lane_count) > > { > > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) > > struct intel_connector *intel_connector = intel_dp > > ->attached_connector; > > struct edid *edid; > > > > + intel_dp_unset_edid(intel_dp); > > edid = intel_dp_get_edid(intel_dp); > > intel_connector->detect_edid = edid; > > > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > intel_dp->has_audio = false; > > } > > > > -static enum drm_connector_status > > -intel_dp_detect(struct drm_connector *connector, bool force) > > +static void > > +intel_dp_long_pulse(struct intel_connector *intel_connector) > > { > > + struct drm_connector *connector = &intel_connector->base; > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > struct intel_encoder *intel_encoder = &intel_dig_port->base; > > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > bool ret; > > u8 sink_irq_vector; > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > - connector->base.id, connector->name); > > - intel_dp_unset_edid(intel_dp); > > - > > - if (intel_dp->is_mst) { > > - /* MST devices are disconnected from a monitor POV */ > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > - return connector_status_disconnected; > > - } > > - > > power_domain = intel_display_port_aux_power_domain(intel_encoder); > > intel_display_power_get(to_i915(dev), power_domain); > > > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > intel_dp_probe_oui(intel_dp); > > > > ret = intel_dp_probe_mst(intel_dp); > > - if (ret) { > > - /* if we are in MST mode then this connector > > - won't appear connected or have anything with EDID on it > > */ > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > This deletion is new in this version of the patch. I think we still need the > hunk above, otherwise we might not properly update the encoder type when we > switch from an HDMI sink connected through a level shifter to an MST sink. > > Ander > > > > - status = connector_status_disconnected; > > + if (ret) > > goto out; Also, there is no call to intel_dp_unset_edid() for this case, since the code will reach the label 'out' with status being connected. So in this case the return value of intel_dp_detect() will depend on the stale value of intel_dp->detect_edid. Ander > > - } > > > > /* > > * Clearing NACK and defer counts to get their exact values > > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > } > > > > out: > > + if (status != connector_status_connected) > > + intel_dp_unset_edid(intel_dp); > > intel_display_power_put(to_i
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: > On 11/01/16 16:56, Chris Wilson wrote: > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote: > > > On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin wrote: > > > > Don't know, I leave this one to whoever grabbed the lock around > > > > intel_init_gt_powersave in the first place. Maybe there was a > > > > special > > > > reason.. after git blame od intel_display.c eventually > > > > completed, adding > > > > Imre and Ville to cc. > > > > > > Hmm. I don't recall the details anymore, but looking at the code > > > pushing > > > the locking down to valleyview_setup_pctx() looks entirely > > > reasonable to > > > me. > > > > iirc, this locking only exists to keep the WARN() at bay. But it is > > pedagogical, I guess. > > Don't really know this area, but what about the > intel_gen6_powersave_work->valleyview_enable_rps- > >valleyview_check_pctx > which dereferences the dev_priv->vlv_pctx, which is set/cleared in > valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be > outside both struct_mutex and the rps lock? dev_priv->vlv_pctx is not protected on the premise that the driver init/cleanup functions can't race and gen6_powersave_work() is scheduled only after intel_init_gt_powersave() and flushed before intel_cleanup_gt_powersave(). rps_lock protects the RPS HW accesses themselves and struct_mutex was taken for the GEM allocation. Taking it at high level around intel_init_gt_powersave() was kind of a copy&paste in the commit you found, there is more on that in 79f5b2c75992. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests
On Wed, Jan 13, 2016 at 01:27:51PM +, Dave Gordon wrote: > On 12/01/16 14:27, Chris Wilson wrote: > >On Tue, Jan 12, 2016 at 01:56:48PM +, Chris Wilson wrote: > >>But we were removing the engine->default_context as it complicated the > >>rest of the code. I strongly prefer keeping the contexts explicit as > >>context separation should be first and foremost in the driver. > > > >$ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc > >drivers/gpu/drm/i915/i915_gem_evict.c: req = > >i915_gem_request_alloc(ring, dev_priv->kernel_context); > >drivers/gpu/drm/i915/intel_overlay.c:return > >i915_gem_request_alloc(ring, dev_priv->kernel_context); > > > >Changing those *two* callsites to pass NULL seems on the odd side, and > >at least for the eviction case discards important information. > >-Chris > > Those specific lines won't be touched by my patch, as *they don't > actually exist in today's drm-intel-nightly* branch. If you want to > add *new* calls to i915_gem_request_alloc() such as the above then > you're quite free to pass any context you want, whether it's a real > user context, the default kernel context explicitly, if you think > it's important that the reader know that that specific context will > be used; or NULL if you don't care what context is used. They are the same calls as the ones you are patching. They are not new calls, they are the only users of the kernel_context for emission. Which is why I am suggesting a different series of steps to take in tidying this up. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
On 13/01/2016 13:03, Chris Wilson wrote: On Wed, Jan 13, 2016 at 12:28:17PM +, Arun Siluvery wrote: Some of the HW registers are privileged and cannot be written to from a non-privileged batch buffers coming from userspace unless they are on whitelist. Userspace need write access to them to implement preemption related WA. You need to be clear that this is the hw whitelist and not our sw whitelist. ok, I will update the commit msg to clarify this. The reason for using this approach is, the register bits that control preemption granularity at the HW level are not context save/restored; so even if we set these bits always in kernel they are going to change once the context is switched out. We can consider making them non-privileged by default but these registers also contain other chicken bits which should not be allowed to be modified. In the later revisions controlling bits are save/restored at context level but in the existing revisions these are exported via other debug registers and should be on the whitelist. This patch adds changes to provide HW with a list of registers to be whitelisted. HW checks this list during execution and provides access accordingly. HW imposes a limit on the number of registers on whitelist and it is per-engine. At this point we are only enabling whitelist for RCS and we don't foresee any requirement for other engines. The registers to be whitelisted are added using generic workaround list mechanism, even these are only enablers for userspace workarounds. But by sharing this mechanism we get some test assets without additional cost (Mika). v2: rebase v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika). Reviewed-by: Mika Kuoppala Cc: Mika Kuoppala Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_drv.h | 9 - drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 17 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..660afe1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1653,11 +1653,18 @@ struct i915_wa_reg { u32 mask; }; -#define I915_MAX_WA_REGS 16 +/* + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only + * allowing it for RCS as we don't foresee any requirement of having + * a whitelist for other engines. When it is really required for + * other engines then the limit need to be increased. + */ +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) struct i915_workarounds { struct i915_wa_reg reg[I915_MAX_WA_REGS]; u32 count; + u32 hw_whitelist_index[I915_NUM_RINGS]; This is a counter only to be used whilst constructing the reg list. It also implies that this list of wa_reg is now engine specific. Hmm, looks like it always has been, just been relying on RCS running first. I would fix that up first. We are initializing all WA only for RCS, similarly HW whitelist also which is engine specific. HW whitelist is initialized along with WA so essentially we are doing it only for RCS. Afaik there is no requirement to have it for other engines, we can actually limit hw_whitelist_index to RCS. I only allowed for all rings because spec defined it for other engines and also we don't have to change this when required. It is not clear what needs fixing up. regards Arun -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix for reserved space WARN_ON when ring begin fails
On 13/01/2016 13:09, Chris Wilson wrote: On Wed, Jan 13, 2016 at 12:52:40PM +, john.c.harri...@intel.com wrote: From: John Harrison The reserved space code was not cleaning up properly in the case where the intel_ring_begin() call failed. This led to WARN_ONs firing about a double reserve call when running the gem_reset_stats IGT test. *reserve_space() failure leads to a i915_gem_request_cancel() which calls intel_ring_reserved_space_cancel(). -Chris Hmm, this is very true. Evidently something weird is going on in my tree. Ignore this patch! ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/7] drm/i915: Cache LRC state page in the context
On 12/01/16 16:45, Dave Gordon wrote: On 12/01/16 13:11, Chris Wilson wrote: On Tue, Jan 12, 2016 at 12:54:25PM +, Tvrtko Ursulin wrote: On 12/01/16 12:12, Chris Wilson wrote: On Tue, Jan 12, 2016 at 11:56:11AM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin LRC lifetime is well defined so we can cache the page pointing to the object backing store in the context in order to avoid walking over the object SG page list from the interrupt context without the big lock held. v2: Also cache the mapping. (Chris Wilson) v3: Unmap on the error path. Then we only use the lrc_state_page in the unmapping, hardly worth the cost of saving it. Ok. Do you also know if this would now require any flushing or something if previously kunmap_atomic might have done something under the covers? kmap_atomic only changes the PTE and the unmap would flush the TLB. In terms of our pointer access, using kmap/kmap_atomic is equivalent. (Just kmap_atomic is meant to be cheaper to set up and a limited resource which can only be used without preemption.) The reg_state is better associated with the ring (since it basically contains the analog of the RING_HEAD and friends). Hm, not sure. The page belongs to the object from that anonymous struct in intel_context so I think it is best to keep them together. The page does sure, but the state does not. The result is that we get: ring->registers[CTX_RING_BUFFER_STATE+1] = ring->vma->node.state; ASSIGN_CTX_PDP(ppgtt, ring->registers, 3); ASSIGN_CTX_PDP(ppgtt, ring->registers, 2); ASSIGN_CTX_PDP(ppgtt, ring->registers, 1); ASSIGN_CTX_PDP(ppgtt, ring->registers, 0); ring->registers[CTX_RING_TAIL+1] = req->tail; which makes a lot more sense, to me, when viewed against the underlying architecture of the hardware and comparing against the legacy ringbuffer. -Chris When you say "ring", do you mean "engine" or "ringbuffer"? The register state can't be associated with the engine /per se/, because it has to be per-context as well as per-engine. It doesn't really belong with the ringbuffer; in particular I've seen code for allocating the ringbuffer in stolen memory and dropping it during hibernation, whereas this register state shouldn't be lost across hibernation. So the lifetime of a register state image and a ringbuffer are different, therefore they don't belong together. The register state is pretty much the /definition/ of a context, in hardware terms. OK, it's got an HWSP and other bits, but the register save area is what the h/w really cares about. So it makes sense that the kmapping for that area is also part of (the CPU's idea of) the context. Yes, ctx.engine[engine_id].registers[regno] = ... is a bit clumsy, but I'd expect to cache the register-image pointer in a local here, along the lines of: uint32_t *registers = ctx.engine[engine_id].registers; registers[CTX_RING_TAIL+1] = req->tail; etc. [aside] Some of this would be much less clumsy if the anonymous engine[] struct had a name, say, "engine_info", so we could just do struct engine_info *eip = &ctx.engines[engine->id]; once at the beginning of any per-engine function and then use eip->ringbuf, eip->state, eip->registers, etc without continually repeating the 'ctx.' and 'engine->id' bits over and over ... [/aside] Apart from that, I think Tvrtko's patch has lost the dirtying from: > -page = i915_gem_object_get_dirty_page(ctx_obj, LRC_STATE_PN); in execlists_update_context(), so should add set_page_dirty(lrc_state_page) instead (and that's the use for it, so you /do/ want to cache it). It is still there (i915_gem_object_get_dirty_page) but at the point of pinning, not each ctx update. Do you think that is a problem? I thought no one can flush and clear the dirty page while the ctx is pinned. On the more general level, can we agree to let this fix with the pointer cached where it currently is (in the context) and leave the anonymous struct - which we all dislike - and I'd add it should probably be defined in intel_lrc.h - for a later cleanup? If so I only need to respin with the struct page * caching removed. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ failure: Fi.CI.BAT
== Summary == Built on 4d09810b01441f9124c072a866f608b748f92f6c drm-intel-nightly: 2016y-01m-13d-12h-32m-08s UTC integration manifest Test gem_basic: Subgroup create-close: pass -> DMESG-WARN (skl-i5k-2) Test gem_cpu_reloc: Subgroup basic: pass -> DMESG-FAIL (skl-i5k-2) Test gem_ctx_param_basic: Subgroup basic: pass -> DMESG-WARN (skl-i5k-2) Subgroup invalid-param-set: pass -> DMESG-WARN (skl-i5k-2) Subgroup non-root-set-no-zeromap: pass -> DMESG-WARN (skl-i5k-2) Subgroup root-set-no-zeromap-disabled: pass -> DMESG-WARN (skl-i5k-2) Test gem_mmap: Subgroup basic: pass -> DMESG-WARN (skl-i5k-2) Test gem_mmap_gtt: Subgroup basic-read: pass -> DMESG-WARN (skl-i5k-2) Subgroup basic-write: pass -> DMESG-WARN (skl-i5k-2) Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE pass -> DMESG-WARN (bdw-nuci7) pass -> DMESG-WARN (bdw-ultra) Test kms_addfb_basic: Subgroup addfb25-modifier-no-flag: pass -> DMESG-WARN (skl-i5k-2) Subgroup addfb25-x-tiled-mismatch: pass -> DMESG-WARN (skl-i5k-2) Subgroup addfb25-yf-tiled: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-1024: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-63: pass -> DMESG-WARN (skl-i5k-2) Subgroup bad-pitch-999: pass -> DMESG-WARN (skl-i5k-2) Subgroup clobberred-modifier: pass -> DMESG-WARN (skl-i5k-2) Subgroup too-high: pass -> DMESG-WARN (skl-i5k-2) Subgroup too-wide: pass -> DMESG-WARN (skl-i5k-2) Subgroup unused-offsets: pass -> DMESG-WARN (skl-i5k-2) Test kms_flip: Subgroup basic-plain-flip: pass -> DMESG-FAIL (skl-i5k-2) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-a-frame-sequence: pass -> DMESG-FAIL (skl-i5k-2) Subgroup read-crc-pipe-b: pass -> DMESG-WARN (ilk-hp8440p) Subgroup read-crc-pipe-b-frame-sequence: pass -> DMESG-FAIL (skl-i5k-2) Test prime_self_import: Subgroup basic-with_two_bos: pass -> DMESG-WARN (skl-i5k-2) bdw-nuci7total:138 pass:128 dwarn:1 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:99 dwarn:5 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:107 dwarn:21 dfail:4 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1165/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
On Wed, Jan 13, 2016 at 01:41:47PM +, Arun Siluvery wrote: > On 13/01/2016 13:03, Chris Wilson wrote: > >On Wed, Jan 13, 2016 at 12:28:17PM +, Arun Siluvery wrote: > >>Some of the HW registers are privileged and cannot be written to from a > >>non-privileged batch buffers coming from userspace unless they are on > >>whitelist. > >>Userspace need write access to them to implement preemption related WA. > > > >You need to be clear that this is the hw whitelist and not our sw > >whitelist. > > > ok, I will update the commit msg to clarify this. > > >>The reason for using this approach is, the register bits that control > >>preemption > >>granularity at the HW level are not context save/restored; so even if we > >>set these > >>bits always in kernel they are going to change once the context is switched > >>out. > >>We can consider making them non-privileged by default but these registers > >>also > >>contain other chicken bits which should not be allowed to be modified. > >> > >>In the later revisions controlling bits are save/restored at context level > >>but > >>in the existing revisions these are exported via other debug registers and > >>should > >>be on the whitelist. This patch adds changes to provide HW with a list of > >>registers > >>to be whitelisted. HW checks this list during execution and provides access > >>accordingly. > >> > >>HW imposes a limit on the number of registers on whitelist and it is > >>per-engine. > >>At this point we are only enabling whitelist for RCS and we don't foresee > >>any > >>requirement for other engines. > >> > >>The registers to be whitelisted are added using generic workaround list > >>mechanism, > >>even these are only enablers for userspace workarounds. But by sharing this > >>mechanism we get some test assets without additional cost (Mika). > >> > >>v2: rebase > >> > >>v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to > >>i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika). > >> > >>Reviewed-by: Mika Kuoppala > >>Cc: Mika Kuoppala > >>Signed-off-by: Arun Siluvery > >>--- > >> drivers/gpu/drm/i915/i915_drv.h | 9 - > >> drivers/gpu/drm/i915/i915_reg.h | 3 +++ > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 17 + > >> 3 files changed, 28 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h > >>b/drivers/gpu/drm/i915/i915_drv.h > >>index 104bd18..660afe1 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.h > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>@@ -1653,11 +1653,18 @@ struct i915_wa_reg { > >>u32 mask; > >> }; > >> > >>-#define I915_MAX_WA_REGS 16 > >>+/* > >>+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only > >>+ * allowing it for RCS as we don't foresee any requirement of having > >>+ * a whitelist for other engines. When it is really required for > >>+ * other engines then the limit need to be increased. > >>+ */ > >>+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) > >> > >> struct i915_workarounds { > >>struct i915_wa_reg reg[I915_MAX_WA_REGS]; > >>u32 count; > >>+ u32 hw_whitelist_index[I915_NUM_RINGS]; > > > >This is a counter only to be used whilst constructing the reg list. It > >also implies that this list of wa_reg is now engine specific. Hmm, looks > >like it always has been, just been relying on RCS running first. > > > >I would fix that up first. > We are initializing all WA only for RCS, similarly HW whitelist also > which is engine specific. > > HW whitelist is initialized along with WA so essentially we are > doing it only for RCS. Afaik there is no requirement to have it for > other engines, we can actually limit hw_whitelist_index to RCS. I > only allowed for all rings because spec defined it for other engines > and also we don't have to change this when required. It is not clear > what needs fixing up. You can at least remove the appearance of it doing performing cross-engine setup. That mistake has already been observed by users where we have used RCS to initialise the other rings, and this patch looks like it could be doing exactly the same - or enabling others to make such a mistake. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Only grab timestamps when needed
From: Tvrtko Ursulin No need to call ktime_get_raw_ns twice per unlimited wait and can also elimate a local variable. v2: Added comment about silencing the compiler warning. (Daniel Vetter) Signed-off-by: Tvrtko Ursulin Reviewed-by: Dave Gordon Acked-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ddc21d4b388d..6b0102da859c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1251,7 +1251,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; DEFINE_WAIT(wait); unsigned long timeout_expire; - s64 before, now; + s64 before = 0; /* Only to silence a compiler warning. */ int ret; WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled"); @@ -1271,14 +1271,17 @@ int __i915_wait_request(struct drm_i915_gem_request *req, return -ETIME; timeout_expire = jiffies + nsecs_to_jiffies_timeout(*timeout); + + /* +* Record current time in case interrupted by signal, or wedged. +*/ + before = ktime_get_raw_ns(); } if (INTEL_INFO(dev_priv)->gen >= 6) gen6_rps_boost(dev_priv, rps, req->emitted_jiffies); - /* Record current time in case interrupted by signal, or wedged */ trace_i915_gem_request_wait_begin(req); - before = ktime_get_raw_ns(); /* Optimistic spin for the next jiffie before touching IRQs */ ret = __i915_spin_request(req, state); @@ -1343,11 +1346,10 @@ int __i915_wait_request(struct drm_i915_gem_request *req, finish_wait(&ring->irq_queue, &wait); out: - now = ktime_get_raw_ns(); trace_i915_gem_request_wait_end(req); if (timeout) { - s64 tres = *timeout - (now - before); + s64 tres = *timeout - (ktime_get_raw_ns() - before); *timeout = tres < 0 ? 0 : tres; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/22] drm/tegra: Stop cancelling page flip events
On Mon, Jan 11, 2016 at 10:41:13PM +0100, Daniel Vetter wrote: > The core takes care of that now. > > v2: Fixup misplaced hunk. > > Cc: Thierry Reding > Cc: Terje Bergström > Acked-by: Daniel Stone > Reviewed-by: Alex Deucher > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/dc.c | 17 - > drivers/gpu/drm/tegra/drm.c | 3 --- > drivers/gpu/drm/tegra/drm.h | 1 - > 3 files changed, 21 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > Current DP detection has DPCD operations split across > intel_dp_hpd_pulse and intel_dp_detect which contains > duplicates as well. Also intel_dp_detect is called > during modes enumeration as well which will result > in multiple dpcd operations. So this patch tries > to solve both these by bringing all DPCD operations > in one single function and make intel_dp_detect > use existing values instead of repeating same steps. > > v2: Pulled in a hunk from last patch of the series to > this patch. (Ander) > v3: Added MST hotplug handling. (Ander) Reviewed-by: Ander Conselvan de Oliveira > > Tested-by: Nathan D Ciobanu > Signed-off-by: Sivakumar Thulasimani > Signed-off-by: Shubhangi Shrivastava > --- > drivers/gpu/drm/i915/intel_dp.c | 71 +--- > - > 1 file changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e3b4208..137757b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4645,8 +4645,19 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > intel_dp_probe_oui(intel_dp); > > ret = intel_dp_probe_mst(intel_dp); > - if (ret) > + if (ret) { > + goto out; > + } else if (connector->status == connector_status_connected) { > + /* > + * If display was connected already and is still connected > + * check links status, there has been known issues of > + * link loss triggerring long pulse > + */ > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > goto out; > + } > > /* >* Clearing NACK and defer counts to get their exact values > @@ -4677,8 +4688,21 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > } > > out: > - if (status != connector_status_connected) > + if (status != connector_status_connected) { > intel_dp_unset_edid(intel_dp); > + /* > + * If we were in MST mode, and device is not there, > + * get out of MST mode > + */ > + if (intel_dp->is_mst) { > + DRM_DEBUG_KMS("MST device may have disappeared %d vs > %d\n", > + intel_dp->is_mst, intel_dp > ->mst_mgr.mst_state); > + intel_dp->is_mst = false; > + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > + intel_dp->is_mst); > + } > + } > + > intel_display_power_put(to_i915(dev), power_domain); > return; > } > @@ -4701,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > return connector_status_disconnected; > } > > - intel_dp_long_pulse(intel_dp->attached_connector); > + if (force) > + intel_dp_long_pulse(intel_dp->attached_connector); > > if (intel_connector->detect_edid) > return connector_status_connected; > @@ -5034,25 +5059,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > /* indicate that we need to restart link training */ > intel_dp->train_set_valid = false; > > - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) > - goto mst_fail; > - > - if (!intel_dp_get_dpcd(intel_dp)) { > - goto mst_fail; > - } > - > - intel_dp_probe_oui(intel_dp); > + intel_dp_long_pulse(intel_dp->attached_connector); > + if (intel_dp->is_mst) > + ret = IRQ_HANDLED; > + goto put_power; > > - if (!intel_dp_probe_mst(intel_dp)) { > - drm_modeset_lock(&dev->mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev > ->mode_config.connection_mutex); > - goto mst_fail; > - } > } else { > if (intel_dp->is_mst) { > - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > - goto mst_fail; > + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { > + /* > + * If we were in MST mode, and device is not > + * there, get out of MST mode > + */ > + DRM_DEBUG_KMS("MST device may have > disappeared %d vs %d\n", > + intel_dp->is_mst, intel_dp > ->mst_mgr.mst_state); > +
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On 13/01/16 13:36, Imre Deak wrote: On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: On 11/01/16 16:56, Chris Wilson wrote: On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote: On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin wrote: Don't know, I leave this one to whoever grabbed the lock around intel_init_gt_powersave in the first place. Maybe there was a special reason.. after git blame od intel_display.c eventually completed, adding Imre and Ville to cc. Hmm. I don't recall the details anymore, but looking at the code pushing the locking down to valleyview_setup_pctx() looks entirely reasonable to me. iirc, this locking only exists to keep the WARN() at bay. But it is pedagogical, I guess. Don't really know this area, but what about the intel_gen6_powersave_work->valleyview_enable_rps- valleyview_check_pctx which dereferences the dev_priv->vlv_pctx, which is set/cleared in valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be outside both struct_mutex and the rps lock? dev_priv->vlv_pctx is not protected on the premise that the driver init/cleanup functions can't race and gen6_powersave_work() is scheduled only after intel_init_gt_powersave() and flushed before intel_cleanup_gt_powersave(). rps_lock protects the RPS HW accesses themselves and struct_mutex was taken for the GEM allocation. Taking it at high level around intel_init_gt_powersave() was kind of a copy&paste in the commit you found, there is more on that in 79f5b2c75992. Thanks for digging this out. It is more involved than what Chris pasted then, and while I hoped to be able to quickly shove that into a patch, I cannot allow the time for more the extensive analysis. So my patch fixes the issue of struct_mutex not being held in intel_alloc_initial_plane_obj while calling i915_gem_object_create_stolen_for_preallocated and I'll leave struct_mutex/rps.lock locking inversion in RPM to someone else. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Expose exec parameter to force non IA-Coherent for Gen9+
On 13/01/16 12:53, Chris Wilson wrote: On Wed, Jan 13, 2016 at 11:44:39AM +0100, Artur Harasimiuk wrote: Starting from Gen9 we can use IA-Coherent caches. Coherence may be not required in certain cases and can be disabled in an easy way. To do this we can set HDC_FORCE_NON_COHERENT bit in HDC_CHICKEN0 register. This register is part of HW context, however it is private and cannot be programmed from non-privileged batch buffer. New parameter is to override default programming and allow UMD to decide whether IA-Coherency is not needed for submitted batch buffer. When flag is set KMD emits commands to disable coherency before batch buffer execution starts. After execution finished state is restored. What's the usecase for batch vs context flag? When WaForceEnableNonCoherent is programmed, it does not make sense to allow for coherency because this can lead to GPU hangs. In such situation flag is ignored. Signed-off-by: Artur Harasimiuk --- drivers/gpu/drm/i915/i915_dma.c| 3 +++ drivers/gpu/drm/i915/i915_drv.h| 4 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 drivers/gpu/drm/i915/intel_lrc.c | 28 drivers/gpu/drm/i915/intel_ringbuffer.c| 7 +++ include/uapi/drm/i915_drm.h| 8 +++- 6 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896c..79ecf20 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -172,6 +172,9 @@ static int i915_getparam(struct drm_device *dev, void *data, case I915_PARAM_HAS_EXEC_SOFTPIN: value = 1; break; + case I915_PARAM_HAS_EXEC_FORCE_NON_COHERENT: + value = INTEL_INFO(dev)->gen >= 9; This should actually report the w/a status, or else you need to provide some other means for userspace to detect if it can successfully force non-coherency. Agreed; especially with the naming used, something called *Force*Something shouldn't be reported as available but then silently ignored on use because some workaround is in effect. If user code knows that it wants to *force* the use of a particular mode then it has to know if it doesn't take effect. OTOH is it really "Force", or is it "Allow"? Or "Request", or "Prefer"? + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..71d739c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -886,6 +886,10 @@ struct intel_context { } engine[I915_NUM_RINGS]; struct list_head link; + + struct { + unsigned int WaForceEnableNonCoherent:1; + } wa; }; enum fb_op_origin { diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index d469c47..2997a58 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1400,6 +1400,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (!i915_gem_check_execbuffer(args)) return -EINVAL; + if ((args->flags & I915_EXEC_FORCE_NON_COHERENT) && + INTEL_INFO(dev)->gen < 9) + return -EINVAL; It would seem easier to simply ignore this request in all situations where it doesn't apply, rather than the current select few. ret = validate_exec_list(dev, exec, args->buffer_count); if (ret) return ret; @@ -1494,6 +1498,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, i915_gem_context_reference(ctx); + /* Clear this flag when WA is programmed */ + if (ctx->wa.WaForceEnableNonCoherent) + args->flags &= ~I915_EXEC_FORCE_NON_COHERENT; + This seems back-to-front (I'm only looking at the names, without checking what this bit really does), but surely if the "ForceEnableNonCoherent" workaround is in effect it should force the "FORCE_NON_COHERENT" flag ON, not OFF? Also, the use of a per-batch bool doesn't seem to leave any don't-care option. Is there any use case where the user code absolutely *doesn't* want this non-coherency? Or which of these are useful: 1. Require non-coherent, fail if not possible 2. Request non-coherent, warn me but continue if not possible 3. Prefer non-coherent, ignore failure (don't even tell me) 4. Don't care, use system default 5. Prefer coherent, ignore failure (don't tell me) 6. Request coherent, warn me (but continue) if not possible 7. Require coherent, fail if not possible Then we just need to make sure the names mean what they would appear to. .Dave. if (ctx->ppgtt) vm = &ctx->ppgtt->base; else diff --git a/drivers/gp
[Intel-gfx] [PATCH] drm/crtc-helper: Add caveat to disable_unused_functions doc
This shouldn't be used by atomic drivers any more, it confuses the state tracking. Cc: Maxime Ripard Cc: Laurent Pinchart Acked-by: Laurent Pinchart Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc_helper.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index a02a7f9a6a9d..a278fbbe23e0 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -220,6 +220,15 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev) * disconnected connectors. Then it will disable all unused encoders and CRTCs * either by calling their disable callback if available or by calling their * dpms callback with DRM_MODE_DPMS_OFF. + * + * NOTE: + * + * This function is part of the legacy modeset helper library and will cause + * major confusion with atomic drivers. This is because atomic helpers guarantee + * to never call ->disable() hooks on a disabled function, or ->enable() hooks + * on an enabled functions. drm_helper_disable_unused_functions() on the other + * hand throws such guarantees into the wind and calls disable hooks + * unconditionally on unused functions. */ void drm_helper_disable_unused_functions(struct drm_device *dev) { -- 2.7.0.rc3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On Wed, Jan 13, 2016 at 02:11:42PM +, Tvrtko Ursulin wrote: > > On 13/01/16 13:36, Imre Deak wrote: > >On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: > >>On 11/01/16 16:56, Chris Wilson wrote: > >>>On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote: > On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin wrote: > >Don't know, I leave this one to whoever grabbed the lock around > >intel_init_gt_powersave in the first place. Maybe there was a > >special > >reason.. after git blame od intel_display.c eventually > >completed, adding > >Imre and Ville to cc. > > Hmm. I don't recall the details anymore, but looking at the code > pushing > the locking down to valleyview_setup_pctx() looks entirely > reasonable to > me. > >>> > >>>iirc, this locking only exists to keep the WARN() at bay. But it is > >>>pedagogical, I guess. > >> > >>Don't really know this area, but what about the > >>intel_gen6_powersave_work->valleyview_enable_rps- > >>>valleyview_check_pctx > >>which dereferences the dev_priv->vlv_pctx, which is set/cleared in > >>valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be > >>outside both struct_mutex and the rps lock? > > > >dev_priv->vlv_pctx is not protected on the premise that the driver > >init/cleanup functions can't race and gen6_powersave_work() is > >scheduled only after intel_init_gt_powersave() and flushed > >before intel_cleanup_gt_powersave(). > > > >rps_lock protects the RPS HW accesses themselves and struct_mutex was > >taken for the GEM allocation. Taking it at high level around > >intel_init_gt_powersave() was kind of a copy&paste in the commit you > >found, there is more on that in 79f5b2c75992. > > Thanks for digging this out. > > It is more involved than what Chris pasted then, and while I hoped > to be able to quickly shove that into a patch, I cannot allow the > time for more the extensive analysis. Imre just confirmed that struct_mutex is only for the GEM allocations in the vlv_setup_ctx, right Imre? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On 12/01/16 16:19, Daniel Vetter wrote: On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote: On 11/01/16 08:43, Daniel Vetter wrote: On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: On 08/01/16 11:29, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Purpose is to catch places which iterate the object VMA list without holding the big lock. Implemented by open coding list_for_each_entry to make the macro compatible with existing call sites. Signed-off-by: Tvrtko Ursulin Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_debugfs.c | 8 drivers/gpu/drm/i915/i915_drv.h | 6 ++ drivers/gpu/drm/i915/i915_gem.c | 24 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c| 4 ++-- 6 files changed, 26 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 714a45cf8a51..d7c2a3201161 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) u64 size = 0; struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(&vma->node)) size += vma->node.size; @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); if (obj->base.name) seq_printf(m, " (name: %d)", obj->base.name); - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (vma->pin_count > 0) pin_count++; } @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_printf(m, " (display)"); if (obj->fence_reg != I915_FENCE_REG_NONE) seq_printf(m, " (fence: %d)", obj->fence_reg); - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", i915_is_ggtt(vma->vm) ? "g" : "pp", vma->node.start, vma->node.size); @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data) stats->shared += obj->base.size; if (USES_FULL_PPGTT(obj->base.dev)) { - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(&vma->node)) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b77a5d84eac2..0406a020dfcc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data( void i915_gem_free_object(struct drm_gem_object *obj); void i915_gem_vma_destroy(struct i915_vma *vma); +#define i915_gem_obj_for_each_vma(vma, obj) \ + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \ +vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\ +&vma->vma_link != (&(obj)->vma_list); \ +vma = list_next_entry(vma, vma_link)) + Unfortunately error capture is not happy with this approach. Can't even see that error capture attempts to grab the mutex anywhere. So what? Drop the idea or add a "doing error capture" flag somewhere? Fix the bugs. Not surprise at all that we've screwed this up all over the place ;-) Afaics modeset code isn't much better either ... Ok I'll drop this patch then since the series contains fixes to all but one related issues. The remaining one is then: [ 17.370366] [ cut here ] [ 17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]() [ 17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex)) [ 17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core [ 17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G U 4.4.0-rc8-160107+ #105 [ 17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014 [ 17.443161] a0227790 8800a98439b8 81280d82 8800a9843a00 [ 17.451677] 8800a98439f0 81049c8c 8801495d
[Intel-gfx] [PATCH] drm/i915/dp: fall back to 18 bpp when sink capability is unknown
Per DP spec, the source device should fall back to 18 bpp, VESA range RGB when the sink capability is unknown. Fix the color depth clamping. 18 bpp color depth should ensure full color range in automatic mode. The clamping has been HDMI specific since its introduction in commit 996a2239f93b03c5972923f04b097f65565c5bed Author: Daniel Vetter Date: Fri Apr 19 11:24:34 2013 +0200 drm/i915: Disable high-bpc on pre-1.4 EDID screens Cc: sta...@vger.kernel.org Reported-by: Dihan Wickremasuriya Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=105331 Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 07ca19b0ec17..6eaecd9385ab 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12171,11 +12171,21 @@ connected_sink_compute_bpp(struct intel_connector *connector, pipe_config->pipe_bpp = connector->base.display_info.bpc*3; } - /* Clamp bpp to 8 on screens without EDID 1.4 */ - if (connector->base.display_info.bpc == 0 && bpp > 24) { - DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n", - bpp); - pipe_config->pipe_bpp = 24; + /* Clamp bpp to default limit on screens without EDID 1.4 */ + if (connector->base.display_info.bpc == 0) { + int type = connector->base.connector_type; + int clamp_bpp = 24; + + /* Fall back to 18 bpp when DP sink capability is unknown. */ + if (type == DRM_MODE_CONNECTOR_DisplayPort || + type == DRM_MODE_CONNECTOR_eDP) + clamp_bpp = 18; + + if (bpp > clamp_bpp) { + DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n", + bpp, clamp_bpp); + pipe_config->pipe_bpp = clamp_bpp; + } } } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On ke, 2016-01-13 at 14:32 +, Chris Wilson wrote: > On Wed, Jan 13, 2016 at 02:11:42PM +, Tvrtko Ursulin wrote: > > > > On 13/01/16 13:36, Imre Deak wrote: > > > On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: > > > > On 11/01/16 16:56, Chris Wilson wrote: > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä > > > > > wrote: > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin > > > > > > wrote: > > > > > > > Don't know, I leave this one to whoever grabbed the lock > > > > > > > around > > > > > > > intel_init_gt_powersave in the first place. Maybe there > > > > > > > was a > > > > > > > special > > > > > > > reason.. after git blame od intel_display.c eventually > > > > > > > completed, adding > > > > > > > Imre and Ville to cc. > > > > > > > > > > > > Hmm. I don't recall the details anymore, but looking at the > > > > > > code > > > > > > pushing > > > > > > the locking down to valleyview_setup_pctx() looks entirely > > > > > > reasonable to > > > > > > me. > > > > > > > > > > iirc, this locking only exists to keep the WARN() at bay. But > > > > > it is > > > > > pedagogical, I guess. > > > > > > > > Don't really know this area, but what about the > > > > intel_gen6_powersave_work->valleyview_enable_rps- > > > > > valleyview_check_pctx > > > > which dereferences the dev_priv->vlv_pctx, which is set/cleared > > > > in > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would now > > > > be > > > > outside both struct_mutex and the rps lock? > > > > > > dev_priv->vlv_pctx is not protected on the premise that the > > > driver > > > init/cleanup functions can't race and gen6_powersave_work() is > > > scheduled only after intel_init_gt_powersave() and flushed > > > before intel_cleanup_gt_powersave(). > > > > > > rps_lock protects the RPS HW accesses themselves and struct_mutex > > > was > > > taken for the GEM allocation. Taking it at high level around > > > intel_init_gt_powersave() was kind of a copy&paste in the commit > > > you > > > found, there is more on that in 79f5b2c75992. > > > > Thanks for digging this out. > > > > It is more involved than what Chris pasted then, and while I hoped > > to be able to quickly shove that into a patch, I cannot allow the > > time for more the extensive analysis. > > Imre just confirmed that struct_mutex is only for the GEM > allocations in the vlv_setup_ctx, right Imre? Yes, that's my understanding. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Handle PipeC fused off on GEN7+
Starting with Gen7 (IVB) Display PipeC can be fused off on some production parts. When disabled, display hardware will prevent the pipe C register bit from being set to 1. Fixed by adjusting pipe_count to reflect this. Signed-off-by: Gabriel Feceoru --- drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896c..c3b93e7 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -813,6 +813,9 @@ static void intel_device_info_runtime_init(struct drm_device *dev) !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) { DRM_INFO("Display fused off, disabling\n"); info->num_pipes = 0; + } else if (I915_READ(FUSE_STRAP) & IVB_PIPE_C_DISABLE) { + DRM_INFO("PipeC fused off\n"); + info->num_pipes -= 1; } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..a182739 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5945,6 +5945,7 @@ enum skl_disp_power_wells { #define ILK_INTERNAL_GRAPHICS_DISABLE (1 << 31) #define ILK_INTERNAL_DISPLAY_DISABLE (1 << 30) #define ILK_DISPLAY_DEBUG_DISABLE (1 << 29) +#define IVB_PIPE_C_DISABLE(1 << 28) #define ILK_HDCP_DISABLE (1 << 25) #define ILK_eDP_A_DISABLE (1 << 24) #define HSW_CDCLK_LIMIT (1 << 24) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On 13/01/16 14:41, Imre Deak wrote: On ke, 2016-01-13 at 14:32 +, Chris Wilson wrote: On Wed, Jan 13, 2016 at 02:11:42PM +, Tvrtko Ursulin wrote: On 13/01/16 13:36, Imre Deak wrote: On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: On 11/01/16 16:56, Chris Wilson wrote: On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä wrote: On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko Ursulin wrote: Don't know, I leave this one to whoever grabbed the lock around intel_init_gt_powersave in the first place. Maybe there was a special reason.. after git blame od intel_display.c eventually completed, adding Imre and Ville to cc. Hmm. I don't recall the details anymore, but looking at the code pushing the locking down to valleyview_setup_pctx() looks entirely reasonable to me. iirc, this locking only exists to keep the WARN() at bay. But it is pedagogical, I guess. Don't really know this area, but what about the intel_gen6_powersave_work->valleyview_enable_rps- valleyview_check_pctx which dereferences the dev_priv->vlv_pctx, which is set/cleared in valleyview_setup_pctx/valleyview_cleanup_pctx, which would now be outside both struct_mutex and the rps lock? dev_priv->vlv_pctx is not protected on the premise that the driver init/cleanup functions can't race and gen6_powersave_work() is scheduled only after intel_init_gt_powersave() and flushed before intel_cleanup_gt_powersave(). rps_lock protects the RPS HW accesses themselves and struct_mutex was taken for the GEM allocation. Taking it at high level around intel_init_gt_powersave() was kind of a copy&paste in the commit you found, there is more on that in 79f5b2c75992. Thanks for digging this out. It is more involved than what Chris pasted then, and while I hoped to be able to quickly shove that into a patch, I cannot allow the time for more the extensive analysis. Imre just confirmed that struct_mutex is only for the GEM allocations in the vlv_setup_ctx, right Imre? Yes, that's my understanding. I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking from lower down to the top level so it sounded like doing the reverse would not be straightforward. Perhaps I overestimated it. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Handle PipeC fused off on GEN7+
On Wed, Jan 13, 2016 at 04:46:43PM +0200, Gabriel Feceoru wrote: > Starting with Gen7 (IVB) Display PipeC can be fused off on some production > parts. When disabled, display hardware will prevent the pipe C register bit > from being set to 1. > > Fixed by adjusting pipe_count to reflect this. The title is misleading, it's really just IVB/HSW/BDW not gen7+ You want to add the changelog of the patch in the commit message as well, eg. v2: Rename HSW_PIPE_C_DISABLE to IVB_PIPE_C_DISABLE as it already exists on ivybridge (Ville) also, we can get rid of the MMIO access (see below) -- Damien > Signed-off-by: Gabriel Feceoru > --- > drivers/gpu/drm/i915/i915_dma.c | 3 +++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 44a896c..c3b93e7 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -813,6 +813,9 @@ static void intel_device_info_runtime_init(struct > drm_device *dev) >!(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) { > DRM_INFO("Display fused off, disabling\n"); > info->num_pipes = 0; > + } else if (I915_READ(FUSE_STRAP) & IVB_PIPE_C_DISABLE) { > + DRM_INFO("PipeC fused off\n"); > + info->num_pipes -= 1; FUSE_STRAP is alreay read above, it's in the fuse_trap variable. HTH, -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ warning: Fi.CI.BAT
== Summary == Built on 4d09810b01441f9124c072a866f608b748f92f6c drm-intel-nightly: 2016y-01m-13d-12h-32m-08s UTC integration manifest Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (skl-i5k-2) UNSTABLE pass -> DMESG-WARN (bdw-ultra) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (ilk-hp8440p) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1169/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > When created originally intel_dp_check_link_status() > was supposed to handle only link training for short > pulse but has grown into handler for short pulse itself. > This patch cleans up this function by splitting it into > two halves. First intel_dp_short_pulse() is called, > which will be entry point and handle all logic for > short pulse handling while intel_dp_check_link_status() > will retain its original purpose of only doing link > status related work. > The link retraining part when EQ is not correct is > retained to intel_dp_check_link_status whereas other > operations are handled as part of intel_dp_short_pulse. > This change is required to avoid performing all DPCD > related operations on performing link retraining. > > Tested-by: Nathan D Ciobanu > Signed-off-by: Sivakumar Thulasimani > Signed-off-by: Shubhangi Shrivastava > --- > drivers/gpu/drm/i915/intel_dp.c | 56 > - > 1 file changed, 33 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 137757b..842790e 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4289,6 +4289,33 @@ go_again: > return -EINVAL; > } > > +static void > +intel_dp_check_link_status(struct intel_dp *intel_dp) > +{ > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) > ->base; > + u8 link_status[DP_LINK_STATUS_SIZE]; > + > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + DRM_ERROR("Failed to get link status\n"); > + return; > + } > + > + if (!intel_encoder->base.crtc) > + return; > + > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > + return; > + > + /* if link training is requested we should perform it always */ > + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || > + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > + intel_encoder->base.name); > + intel_dp_start_link_train(intel_dp); > + intel_dp_stop_link_train(intel_dp); > + } > +} > + > /* > * According to DP spec > * 5.1.2: > @@ -4298,15 +4325,12 @@ go_again: > * 4. Check link status on receipt of hot-plug interrupt > */ > static void > -intel_dp_check_link_status(struct intel_dp *intel_dp) > +intel_dp_short_pulse(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > - struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp) > ->base; > u8 sink_irq_vector; > u8 link_status[DP_LINK_STATUS_SIZE]; > > - WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > - I think it's better to move this WARN to the new intel_dp_check_link_status(). > /* >* Clearing compliance test variables to allow capturing >* of values for next automated test request. > @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > intel_dp->compliance_test_type = 0; > intel_dp->compliance_test_data = 0; > > - if (!intel_encoder->base.crtc) > - return; > - > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > - return; > - > /* Try to read receiver status if the link appears to be up */ > if (!intel_dp_get_link_status(intel_dp, link_status)) { > return; There is now two calls to intel_dp_get_link_status()and the value of link_status is not used in this function, so maybe just remove it from here. Looks good otherwise. Ander > @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > DRM_DEBUG_DRIVER("CP or sink specific irq > unhandled\n"); > } > > - /* if link training is requested we should perform it always */ > - if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || > - (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { > - DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > - intel_encoder->base.name); > - intel_dp_start_link_train(intel_dp); > - intel_dp_stop_link_train(intel_dp); > - } > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > } > > /* XXX this is probably wrong for multiple downstream ports */ > @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > } > } > > - if (!intel_dp->is_mst) { > - drm_modeset_lock(&dev->mode_config.connection_mutex, > NULL); > - intel_dp_check_link_sta
[Intel-gfx] [PATCH 1/5] drm/i915: use hlist_for_each_entry
Use hlist_for_each_entry() instead of hlist_for_each() to simplify the code. Signed-off-by: Geliang Tang --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 5d01ea6..8f194be 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -192,14 +192,10 @@ static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle) return NULL; return eb->lut[handle]; } else { - struct hlist_head *head; - struct hlist_node *node; - - head = &eb->buckets[handle & eb->and]; - hlist_for_each(node, head) { - struct i915_vma *vma; + struct i915_vma *vma; - vma = hlist_entry(node, struct i915_vma, exec_node); + hlist_for_each_entry(vma, &eb->buckets[handle & eb->and], +exec_node) { if (vma->exec_handle == handle) return vma; } -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/5] drm/i915: use kobj_to_dev()
Use kobj_to_dev() instead of open-coding it. Signed-off-by: Geliang Tang --- drivers/gpu/drm/i915/i915_sysfs.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 37e3f0d..c6188dd 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -164,7 +164,7 @@ i915_l3_read(struct file *filp, struct kobject *kobj, struct bin_attribute *attr, char *buf, loff_t offset, size_t count) { - struct device *dev = container_of(kobj, struct device, kobj); + struct device *dev = kobj_to_dev(kobj); struct drm_minor *dminor = dev_to_drm_minor(dev); struct drm_device *drm_dev = dminor->dev; struct drm_i915_private *dev_priv = drm_dev->dev_private; @@ -200,7 +200,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj, struct bin_attribute *attr, char *buf, loff_t offset, size_t count) { - struct device *dev = container_of(kobj, struct device, kobj); + struct device *dev = kobj_to_dev(kobj); struct drm_minor *dminor = dev_to_drm_minor(dev); struct drm_device *drm_dev = dminor->dev; struct drm_i915_private *dev_priv = drm_dev->dev_private; @@ -521,7 +521,7 @@ static ssize_t error_state_read(struct file *filp, struct kobject *kobj, loff_t off, size_t count) { - struct device *kdev = container_of(kobj, struct device, kobj); + struct device *kdev = kobj_to_dev(kobj); struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor->dev; struct i915_error_state_file_priv error_priv; @@ -556,7 +556,7 @@ static ssize_t error_state_write(struct file *file, struct kobject *kobj, struct bin_attribute *attr, char *buf, loff_t off, size_t count) { - struct device *kdev = container_of(kobj, struct device, kobj); + struct device *kdev = kobj_to_dev(kobj); struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor->dev; int ret; -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dp: fall back to 18 bpp when sink capability is unknown
On Wed, Jan 13, 2016 at 04:35:20PM +0200, Jani Nikula wrote: > Per DP spec, the source device should fall back to 18 bpp, VESA range > RGB when the sink capability is unknown. Fix the color depth > clamping. 18 bpp color depth should ensure full color range in automatic > mode. > > The clamping has been HDMI specific since its introduction in > > commit 996a2239f93b03c5972923f04b097f65565c5bed > Author: Daniel Vetter > Date: Fri Apr 19 11:24:34 2013 +0200 > > drm/i915: Disable high-bpc on pre-1.4 EDID screens > > Cc: sta...@vger.kernel.org > Reported-by: Dihan Wickremasuriya > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=105331 > Signed-off-by: Jani Nikula Makes sense to me as far as the spec is concerned. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_display.c | 20 +++- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 07ca19b0ec17..6eaecd9385ab 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12171,11 +12171,21 @@ connected_sink_compute_bpp(struct intel_connector > *connector, > pipe_config->pipe_bpp = connector->base.display_info.bpc*3; > } > > - /* Clamp bpp to 8 on screens without EDID 1.4 */ > - if (connector->base.display_info.bpc == 0 && bpp > 24) { > - DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit > of 24\n", > - bpp); > - pipe_config->pipe_bpp = 24; > + /* Clamp bpp to default limit on screens without EDID 1.4 */ > + if (connector->base.display_info.bpc == 0) { > + int type = connector->base.connector_type; > + int clamp_bpp = 24; > + > + /* Fall back to 18 bpp when DP sink capability is unknown. */ > + if (type == DRM_MODE_CONNECTOR_DisplayPort || > + type == DRM_MODE_CONNECTOR_eDP) > + clamp_bpp = 18; > + > + if (bpp > clamp_bpp) { > + DRM_DEBUG_KMS("clamping display bpp (was %d) to default > limit of %d\n", > + bpp, clamp_bpp); > + pipe_config->pipe_bpp = clamp_bpp; > + } > } > } > > -- > 2.1.4 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: use hlist_for_each_entry
On Wed, Jan 13, 2016 at 10:48:39PM +0800, Geliang Tang wrote: > Use hlist_for_each_entry() instead of hlist_for_each() to simplify > the code. > > Signed-off-by: Geliang Tang > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 5d01ea6..8f194be 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -192,14 +192,10 @@ static struct i915_vma *eb_get_vma(struct eb_vmas *eb, > unsigned long handle) > return NULL; > return eb->lut[handle]; > } else { > - struct hlist_head *head; > - struct hlist_node *node; > - > - head = &eb->buckets[handle & eb->and]; > - hlist_for_each(node, head) { > - struct i915_vma *vma; > + struct i915_vma *vma; > > - vma = hlist_entry(node, struct i915_vma, exec_node); > + hlist_for_each_entry(vma, &eb->buckets[handle & eb->and], > + exec_node) { Keep the head = &eb->buckets[handle & eb->and]; local assignment as it makes the line splitting neater (and iirc something like gcc -Os doesn't use the temporary assignment - but then who would use CONFIG_OPTIMIZE_FOR_SIZE!). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ failure: Fi.CI.BAT
== Summary == Built on 4d09810b01441f9124c072a866f608b748f92f6c drm-intel-nightly: 2016y-01m-13d-12h-32m-08s UTC integration manifest Test gem_ctx_basic: pass -> FAIL (hsw-gt2) Test gem_storedw_loop: Subgroup basic-render: dmesg-warn -> PASS (skl-i7k-2) UNSTABLE Test kms_pipe_crc_basic: Subgroup read-crc-pipe-c: pass -> DMESG-WARN (bdw-ultra) bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:136 dwarn:0 dfail:0 fail:1 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1170/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ warning: Fi.CI.BAT
On Wed, Jan 13, 2016 at 12:13:17PM -, Patchwork wrote: > == Summary == > > Built on 8da57dfe6c675c35109dac986e3f8b627cffab49 drm-intel-nightly: > 2016y-01m-13d-10h-33m-04s UTC integration manifest > > Test gem_storedw_loop: > Subgroup basic-render: > pass -> DMESG-WARN (skl-i5k-2) UNSTABLE > dmesg-warn -> PASS (bdw-ultra) This is a pre-existing bug already tracked in https://bugs.freedesktop.org/show_bug.cgi?id=93693 > Test kms_flip: > Subgroup basic-flip-vs-dpms: > dmesg-warn -> PASS (ilk-hp8440p) > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-b: > dmesg-warn -> PASS (bdw-ultra) > Subgroup read-crc-pipe-c: > pass -> DMESG-WARN (bdw-ultra) This seems to be a very sporadic issue on bdw-ultra with pipe B/C and the rpm wakelock check. Preexisting, but not yet tracked in bugzilla it seems. I created a new one: https://bugs.freedesktop.org/show_bug.cgi?id=93699 > Subgroup suspend-read-crc-pipe-a: > dmesg-warn -> PASS (snb-x220t) > > bdw-nuci7total:138 pass:128 dwarn:1 dfail:0 fail:0 skip:9 > bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 > bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 > hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 > ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 > ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 > skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 > skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 > snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 > > Results at /archive/results/CI_IGT_test/Patchwork_1163/ Patch merged with Ville's r-b from the previous round. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On ke, 2016-01-13 at 14:53 +, Tvrtko Ursulin wrote: > On 13/01/16 14:41, Imre Deak wrote: > > On ke, 2016-01-13 at 14:32 +, Chris Wilson wrote: > > > On Wed, Jan 13, 2016 at 02:11:42PM +, Tvrtko Ursulin wrote: > > > > > > > > On 13/01/16 13:36, Imre Deak wrote: > > > > > On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: > > > > > > On 11/01/16 16:56, Chris Wilson wrote: > > > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä > > > > > > > wrote: > > > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko > > > > > > > > Ursulin > > > > > > > > wrote: > > > > > > > > > Don't know, I leave this one to whoever grabbed the > > > > > > > > > lock > > > > > > > > > around > > > > > > > > > intel_init_gt_powersave in the first place. Maybe > > > > > > > > > there > > > > > > > > > was a > > > > > > > > > special > > > > > > > > > reason.. after git blame od intel_display.c > > > > > > > > > eventually > > > > > > > > > completed, adding > > > > > > > > > Imre and Ville to cc. > > > > > > > > > > > > > > > > Hmm. I don't recall the details anymore, but looking at > > > > > > > > the > > > > > > > > code > > > > > > > > pushing > > > > > > > > the locking down to valleyview_setup_pctx() looks > > > > > > > > entirely > > > > > > > > reasonable to > > > > > > > > me. > > > > > > > > > > > > > > iirc, this locking only exists to keep the WARN() at bay. > > > > > > > But > > > > > > > it is > > > > > > > pedagogical, I guess. > > > > > > > > > > > > Don't really know this area, but what about the > > > > > > intel_gen6_powersave_work->valleyview_enable_rps- > > > > > > > valleyview_check_pctx > > > > > > which dereferences the dev_priv->vlv_pctx, which is > > > > > > set/cleared > > > > > > in > > > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would > > > > > > now > > > > > > be > > > > > > outside both struct_mutex and the rps lock? > > > > > > > > > > dev_priv->vlv_pctx is not protected on the premise that the > > > > > driver > > > > > init/cleanup functions can't race and gen6_powersave_work() > > > > > is > > > > > scheduled only after intel_init_gt_powersave() and flushed > > > > > before intel_cleanup_gt_powersave(). > > > > > > > > > > rps_lock protects the RPS HW accesses themselves and > > > > > struct_mutex > > > > > was > > > > > taken for the GEM allocation. Taking it at high level around > > > > > intel_init_gt_powersave() was kind of a copy&paste in the > > > > > commit > > > > > you > > > > > found, there is more on that in 79f5b2c75992. > > > > > > > > Thanks for digging this out. > > > > > > > > It is more involved than what Chris pasted then, and while I > > > > hoped > > > > to be able to quickly shove that into a patch, I cannot allow > > > > the > > > > time for more the extensive analysis. > > > > > > Imre just confirmed that struct_mutex is only for the GEM > > > allocations in the vlv_setup_ctx, right Imre? > > > > Yes, that's my understanding. > > I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking > from > lower down to the top level so it sounded like doing the reverse > would > not be straightforward. Perhaps I overestimated it. Ok, some more background info :) Initially the HW access was also protected by struct_mutex but 4fc688ce7 added rps_lock for just the the HW access. So as I understand we don't need struct_mutex for anything else besides the GEM allocation. So feel free to add my ACK on the change moving the lock to vlv_setup_ctx(). Btw, we could also remove struct_mutex from intel_enable_gt_powersave() where it's taken for old platforms and rely on mchdev_lock or take rps_lock instead, but someone needs to double-check this. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
Some of the HW registers are privileged and cannot be written to from non-privileged batch buffers coming from userspace unless they are added to the HW whitelist. This whitelist is maintained by HW and it is different from SW whitelist. Userspace need write access to them to implement preemption related WA. The reason for using this approach is, the register bits that control preemption granularity at the HW level are not context save/restored; so even if we set these bits always in kernel they are going to change once the context is switched out. We can consider making them non-privileged by default but these registers also contain other chicken bits which should not be allowed to be modified. In the later revisions controlling bits are save/restored at context level but in the existing revisions these are exported via other debug registers and should be on the whitelist. This patch adds changes to provide HW with a list of registers to be whitelisted. HW checks this list during execution and provides access accordingly. HW imposes a limit on the number of registers on whitelist and it is per-engine. At this point we are only enabling whitelist for RCS and we don't foresee any requirement for other engines. The registers to be whitelisted are added using generic workaround list mechanism, even these are only enablers for userspace workarounds. But by sharing this mechanism we get some test assets without additional cost (Mika). v2: rebase v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika). v4: Clarify that this is HW whitelist and different from the one maintained in driver. This list is engine specific but it gets initialized along with other WA which is RCS specific thing, so make it clear that we are not doing any cross engine setup during initialization (Chris). Reviewed-by: Mika Kuoppala Cc: Mika Kuoppala Cc: Chris Wilson Signed-off-by: Arun Siluvery --- drivers/gpu/drm/i915/i915_drv.h | 9 - drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 17 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..83fccc0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1653,11 +1653,18 @@ struct i915_wa_reg { u32 mask; }; -#define I915_MAX_WA_REGS 16 +/* + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only + * allowing it for RCS as we don't foresee any requirement of having + * a whitelist for other engines. When it is really required for + * other engines then the limit need to be increased. + */ +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS) struct i915_workarounds { struct i915_wa_reg reg[I915_MAX_WA_REGS]; u32 count; + u32 hw_whitelist_count[I915_NUM_RINGS]; }; struct i915_virtual_gpu { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..7938814 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells { #define RING_WAIT(1<<11) /* gen3+, PRBx_CTL */ #define RING_WAIT_SEMAPHORE (1<<10) /* gen6+ */ +#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4) +#define RING_MAX_NONPRIV_SLOTS 12 + #define GEN7_TLB_RD_ADDR _MMIO(0x4700) #if 0 diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 4060acf..4fe3240 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv, #define WA_WRITE(addr, val) WA_REG(addr, 0x, val) +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, +i915_reg_t reg_addr) +{ + struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct i915_workarounds *wa = &dev_priv->workarounds; + const uint32_t index = wa->hw_whitelist_count[ring->id]; + + if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) + return -EINVAL; + + WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg); + wa->hw_whitelist_count[ring->id]++; + + return 0; +} + static int gen8_init_workarounds(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; @@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) WARN_ON(ring->id != RCS); dev_priv->workarounds.count = 0; + dev_priv->workarounds.hw_whitelist_count[RCS] = 0; if (IS_BROADWELL(dev)) return bdw_init_workarounds(ring); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/
[Intel-gfx] ✓ success: Fi.CI.BAT
== Summary == Built on aa7ddea990dfc10c7e90ad10820e0121a9667453 drm-intel-nightly: 2016y-01m-13d-15h-05m-13s UTC integration manifest Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-b: skip -> PASS (bdw-nuci7) Subgroup nonblocking-crc-pipe-c: pass -> SKIP (bdw-nuci7) bdw-nuci7total:138 pass:128 dwarn:0 dfail:0 fail:0 skip:10 bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 Results at /archive/results/CI_IGT_test/Patchwork_1171/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915: use kobj_to_dev()
On Wed, Jan 13, 2016 at 10:48:40PM +0800, Geliang Tang wrote: > Use kobj_to_dev() instead of open-coding it. > > Signed-off-by: Geliang Tang Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/i915_sysfs.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c > b/drivers/gpu/drm/i915/i915_sysfs.c > index 37e3f0d..c6188dd 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -164,7 +164,7 @@ i915_l3_read(struct file *filp, struct kobject *kobj, >struct bin_attribute *attr, char *buf, >loff_t offset, size_t count) > { > - struct device *dev = container_of(kobj, struct device, kobj); > + struct device *dev = kobj_to_dev(kobj); > struct drm_minor *dminor = dev_to_drm_minor(dev); > struct drm_device *drm_dev = dminor->dev; > struct drm_i915_private *dev_priv = drm_dev->dev_private; > @@ -200,7 +200,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj, > struct bin_attribute *attr, char *buf, > loff_t offset, size_t count) > { > - struct device *dev = container_of(kobj, struct device, kobj); > + struct device *dev = kobj_to_dev(kobj); > struct drm_minor *dminor = dev_to_drm_minor(dev); > struct drm_device *drm_dev = dminor->dev; > struct drm_i915_private *dev_priv = drm_dev->dev_private; > @@ -521,7 +521,7 @@ static ssize_t error_state_read(struct file *filp, struct > kobject *kobj, > loff_t off, size_t count) > { > > - struct device *kdev = container_of(kobj, struct device, kobj); > + struct device *kdev = kobj_to_dev(kobj); > struct drm_minor *minor = dev_to_drm_minor(kdev); > struct drm_device *dev = minor->dev; > struct i915_error_state_file_priv error_priv; > @@ -556,7 +556,7 @@ static ssize_t error_state_write(struct file *file, > struct kobject *kobj, >struct bin_attribute *attr, char *buf, >loff_t off, size_t count) > { > - struct device *kdev = container_of(kobj, struct device, kobj); > + struct device *kdev = kobj_to_dev(kobj); > struct drm_minor *minor = dev_to_drm_minor(kdev); > struct drm_device *dev = minor->dev; > int ret; > -- > 2.5.0 > > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ warning: Fi.CI.BAT
On Wed, Jan 13, 2016 at 08:24:52AM -, Patchwork wrote: > == Summary == > > Built on 06d0112e293dfdea7f796d4085f755898850947b drm-intel-nightly: > 2016y-01m-12d-21h-16m-40s UTC integration manifest > > Test gem_storedw_loop: > Subgroup basic-render: > pass -> DMESG-WARN (bdw-nuci7) That I take it is this: https://bugs.freedesktop.org/show_bug.cgi?id=93693 > dmesg-warn -> PASS (bdw-ultra) > Test kms_flip: > Subgroup basic-flip-vs-dpms: > dmesg-warn -> PASS (skl-i7k-2) > dmesg-warn -> PASS (ilk-hp8440p) > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-a: > pass -> DMESG-WARN (byt-nuc) This is the age old [drm:vlv_check_no_gt_access [i915]] *ERROR* GT register access while GT waking disabled https://bugs.freedesktop.org/show_bug.cgi?id=93121 So these have nothing to do with these patches it seems. > Subgroup read-crc-pipe-a-frame-sequence: > fail -> PASS (snb-x220t) > > bdw-nuci7total:138 pass:128 dwarn:1 dfail:0 fail:0 skip:9 > bdw-ultratotal:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 > bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 > byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 > hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 > hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 > ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 > skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 > snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 > > Results at /archive/results/CI_IGT_test/Patchwork_1157/ -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock
On Wed, Jan 13, 2016 at 05:25:09PM +0200, Imre Deak wrote: > On ke, 2016-01-13 at 14:53 +, Tvrtko Ursulin wrote: > > On 13/01/16 14:41, Imre Deak wrote: > > > On ke, 2016-01-13 at 14:32 +, Chris Wilson wrote: > > > > On Wed, Jan 13, 2016 at 02:11:42PM +, Tvrtko Ursulin wrote: > > > > > > > > > > On 13/01/16 13:36, Imre Deak wrote: > > > > > > On ke, 2016-01-13 at 12:46 +, Tvrtko Ursulin wrote: > > > > > > > On 11/01/16 16:56, Chris Wilson wrote: > > > > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä > > > > > > > > wrote: > > > > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +, Tvrtko > > > > > > > > > Ursulin > > > > > > > > > wrote: > > > > > > > > > > Don't know, I leave this one to whoever grabbed the > > > > > > > > > > lock > > > > > > > > > > around > > > > > > > > > > intel_init_gt_powersave in the first place. Maybe > > > > > > > > > > there > > > > > > > > > > was a > > > > > > > > > > special > > > > > > > > > > reason.. after git blame od intel_display.c > > > > > > > > > > eventually > > > > > > > > > > completed, adding > > > > > > > > > > Imre and Ville to cc. > > > > > > > > > > > > > > > > > > Hmm. I don't recall the details anymore, but looking at > > > > > > > > > the > > > > > > > > > code > > > > > > > > > pushing > > > > > > > > > the locking down to valleyview_setup_pctx() looks > > > > > > > > > entirely > > > > > > > > > reasonable to > > > > > > > > > me. > > > > > > > > > > > > > > > > iirc, this locking only exists to keep the WARN() at bay. > > > > > > > > But > > > > > > > > it is > > > > > > > > pedagogical, I guess. > > > > > > > > > > > > > > Don't really know this area, but what about the > > > > > > > intel_gen6_powersave_work->valleyview_enable_rps- > > > > > > > > valleyview_check_pctx > > > > > > > which dereferences the dev_priv->vlv_pctx, which is > > > > > > > set/cleared > > > > > > > in > > > > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would > > > > > > > now > > > > > > > be > > > > > > > outside both struct_mutex and the rps lock? > > > > > > > > > > > > dev_priv->vlv_pctx is not protected on the premise that the > > > > > > driver > > > > > > init/cleanup functions can't race and gen6_powersave_work() > > > > > > is > > > > > > scheduled only after intel_init_gt_powersave() and flushed > > > > > > before intel_cleanup_gt_powersave(). > > > > > > > > > > > > rps_lock protects the RPS HW accesses themselves and > > > > > > struct_mutex > > > > > > was > > > > > > taken for the GEM allocation. Taking it at high level around > > > > > > intel_init_gt_powersave() was kind of a copy&paste in the > > > > > > commit > > > > > > you > > > > > > found, there is more on that in 79f5b2c75992. > > > > > > > > > > Thanks for digging this out. > > > > > > > > > > It is more involved than what Chris pasted then, and while I > > > > > hoped > > > > > to be able to quickly shove that into a patch, I cannot allow > > > > > the > > > > > time for more the extensive analysis. > > > > > > > > Imre just confirmed that struct_mutex is only for the GEM > > > > allocations in the vlv_setup_ctx, right Imre? > > > > > > Yes, that's my understanding. > > > > I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking > > from > > lower down to the top level so it sounded like doing the reverse > > would > > not be straightforward. Perhaps I overestimated it. > > Ok, some more background info :) > > Initially the HW access was also protected by struct_mutex but > 4fc688ce7 added rps_lock for just the the HW access. So as I understand > we don't need struct_mutex for anything else besides the GEM > allocation. So feel free to add my ACK on the change moving the lock to > vlv_setup_ctx(). > > Btw, we could also remove struct_mutex from intel_enable_gt_powersave() > where it's taken for old platforms and rely on mchdev_lock or take > rps_lock instead, but someone needs to double-check this. For more context we needed dev->struct_mutex for ilk rc6 support, which needed a powerctx allocated from gem (not just stolen). That support died in fire in commit a561165493e5fec2f74bd3ae0577ed659e44ab7f Author: John Harrison Date: Thu Mar 5 14:03:03 2015 + drm/i915: Remove ironlake rc6 support Maybe reference that too, on top of the commit Imre dug out? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3] drm/i915: Handle PipeC fused off on IVB/HSW/BDW
Some Gen7/8 production parts may have the Display Pipe C fused off. In this case, the display hardware will prevent the Pipe C register bit from being set to 1. Fixed by adjusting pipe_count to reflect this. v2: Rename HSW_PIPE_C_DISABLE to IVB_PIPE_C_DISABLE as it already exists on ivybridge (Ville) v3: Remove unnecessary MMIO read, correct the description (Damien) Signed-off-by: Gabriel Feceoru --- drivers/gpu/drm/i915/i915_dma.c | 3 +++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896c..dd0d100 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -813,6 +813,9 @@ static void intel_device_info_runtime_init(struct drm_device *dev) !(sfuse_strap & SFUSE_STRAP_FUSE_LOCK))) { DRM_INFO("Display fused off, disabling\n"); info->num_pipes = 0; + } else if (fuse_strap & IVB_PIPE_C_DISABLE) { + DRM_INFO("PipeC fused off\n"); + info->num_pipes -= 1; } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0a98889..a182739 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5945,6 +5945,7 @@ enum skl_disp_power_wells { #define ILK_INTERNAL_GRAPHICS_DISABLE (1 << 31) #define ILK_INTERNAL_DISPLAY_DISABLE (1 << 30) #define ILK_DISPLAY_DEBUG_DISABLE (1 << 29) +#define IVB_PIPE_C_DISABLE(1 << 28) #define ILK_HDCP_DISABLE (1 << 25) #define ILK_eDP_A_DISABLE (1 << 24) #define HSW_CDCLK_LIMIT (1 << 24) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: disable non-sequential pfits on ivb/hsw
The existing code assumes a sequential mapping of panel fitters to pipes (pfit0-pipeA, pfit1-pipeB, pfit2-pipeC), but boot firmware can arbitrarily assign any pipe to a pfit on IVB hardware e.g. Macbook UEFI uses pfit 0 and pipe C for eDP1 when the firmware boots in a non-16:10 resolution (the last-used resolution is stored in NVRAM by OS X so the firmware can immediately restore it at boot). When this happens, the display will appear letterboxed due to incorrect aspect ratio and attempting to switch to alternative resolutions will fail. Fix this by disabling any panel fitters which have been non-sequentially assigned at boot time. Link: https://bugs.freedesktop.org/show_bug.cgi?id=93523 Signed-off-by: Chris Bainbridge --- drivers/gpu/drm/i915/intel_display.c | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 32cf97346978..9e588139a2dd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9170,6 +9170,24 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc, struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; uint32_t tmp; + int pipe; + + /* +* PF_CTL assumes panel fitter 0 is on pipe A, panel fitter 1 is on +* pipe B, and panel fitter 2 is on pipe C, but firmware can init IVB +* panel fitters to any arbitrary pipe (Macbook UEFI uses pfit 0 for +* pipe C), so find and disable any other mappings. +*/ + for (pipe = 0; pipe < INTEL_INFO(dev)->num_pipes; pipe++) { + tmp = I915_READ(PF_CTL(pipe)); + if (IS_GEN7(dev) && (tmp & PF_ENABLE) && + PF_PIPE_SEL_IVB(pipe) != (tmp & PF_PIPE_SEL_MASK_IVB)) { + DRM_DEBUG_KMS("disabling initial panel fitter\n"); + I915_WRITE(PF_CTL(pipe), 0); + I915_WRITE(PF_WIN_POS(pipe), 0); + I915_WRITE(PF_WIN_SZ(pipe), 0); + } + } tmp = I915_READ(PF_CTL(crtc->pipe)); @@ -9177,14 +9195,6 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc, pipe_config->pch_pfit.enabled = true; pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe)); pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe)); - - /* We currently do not free assignements of panel fitters on -* ivb/hsw (since we don't use the higher upscaling modes which -* differentiates them) so just WARN about this case for now. */ - if (IS_GEN7(dev)) { - WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) != - PF_PIPE_SEL_IVB(crtc->pipe)); - } } } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH libdrm] Synchronize drm/drm_fourcc.h with Linux’ version
This adds R8, RG88 and GR88, as well as the non-subsampled NV24/NV42 formats. Signed-off-by: Emmanuel Gil Peyrot --- include/drm/drm_fourcc.h | 9 + 1 file changed, 9 insertions(+) diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index e741b09..59fc54d 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -34,6 +34,13 @@ /* color index */ #define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */ +/* 8 bpp Red */ +#define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ + +/* 16 bpp RG */ +#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 little endian */ +#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 little endian */ + /* 8 bpp RGB */ #define DRM_FORMAT_RGB332 fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B 3:3:2 */ #define DRM_FORMAT_BGR233 fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R 2:3:3 */ @@ -106,6 +113,8 @@ #define DRM_FORMAT_NV21fourcc_code('N', 'V', '2', '1') /* 2x2 subsampled Cb:Cr plane */ #define DRM_FORMAT_NV16fourcc_code('N', 'V', '1', '6') /* 2x1 subsampled Cr:Cb plane */ #define DRM_FORMAT_NV61fourcc_code('N', 'V', '6', '1') /* 2x1 subsampled Cb:Cr plane */ +#define DRM_FORMAT_NV24fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */ +#define DRM_FORMAT_NV42fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */ /* * 3 plane YCbCr -- 2.7.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/22] drm/imx: Unconfuse preclose logic
Am Montag, den 11.01.2016, 22:41 +0100 schrieb Daniel Vetter: > So this one is special, since it tries to prevent races when userspace > crashes simply by disabling the vblank machinery. Well except that imx > always has vblanks enabled, and the disable_vblank hook actually just > tries to cancel a pending pageflip. Without any locking whatsoever. Of > course this is wrong, since it'll result in the hw not actually > displaying what drm thinks is the current frontbuffer. > > Well since the core takes care of the disappearing DRM fd now. So we > can nuke all this confused code without ill side-effects. > > Someone else needs to audit the locking for ->newfb and > ->page_flip_event and fix it up. Common approach is to reuse > dev->event_lock for this. > > Cc: Sascha Hauer > Cc: Philipp Zabel > Acked-by: Daniel Stone > Reviewed-by: Alex Deucher > Signed-off-by: Daniel Vetter Acked-by: Philipp Zabel regards Philipp ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/22] drm/atmel: Nuke preclose
On Mon, 11 Jan 2016 22:41:06 +0100 Daniel Vetter wrote: > The only thing this did was cancle pending flip events, and the core > takes care of that now. > > Cc: Boris Brezillon > Acked-by: Daniel Stone > Reviewed-by: Alex Deucher > Signed-off-by: Daniel Vetter Acked-by: Boris Brezillon > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 18 -- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 10 -- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 3 --- > 3 files changed, 31 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 468a14f266a7..9863291a9a54 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -280,24 +280,6 @@ static void atmel_hlcdc_crtc_destroy(struct drm_crtc *c) > kfree(crtc); > } > > -void atmel_hlcdc_crtc_cancel_page_flip(struct drm_crtc *c, > -struct drm_file *file) > -{ > - struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c); > - struct drm_pending_vblank_event *event; > - struct drm_device *dev = c->dev; > - unsigned long flags; > - > - spin_lock_irqsave(&dev->event_lock, flags); > - event = crtc->event; > - if (event && event->base.file_priv == file) { > - event->base.destroy(&event->base); > - drm_vblank_put(dev, crtc->id); > - crtc->event = NULL; > - } > - spin_unlock_irqrestore(&dev->event_lock, flags); > -} > - > static void atmel_hlcdc_crtc_finish_page_flip(struct atmel_hlcdc_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index a45b32ba029e..3d8d16402d07 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -619,15 +619,6 @@ static void atmel_hlcdc_dc_connector_unplug_all(struct > drm_device *dev) > mutex_unlock(&dev->mode_config.mutex); > } > > -static void atmel_hlcdc_dc_preclose(struct drm_device *dev, > - struct drm_file *file) > -{ > - struct drm_crtc *crtc; > - > - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > - atmel_hlcdc_crtc_cancel_page_flip(crtc, file); > -} > - > static void atmel_hlcdc_dc_lastclose(struct drm_device *dev) > { > struct atmel_hlcdc_dc *dc = dev->dev_private; > @@ -698,7 +689,6 @@ static struct drm_driver atmel_hlcdc_dc_driver = { > .driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | > DRIVER_MODESET | DRIVER_PRIME | > DRIVER_ATOMIC, > - .preclose = atmel_hlcdc_dc_preclose, > .lastclose = atmel_hlcdc_dc_lastclose, > .irq_handler = atmel_hlcdc_dc_irq_handler, > .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > index cf6b375bc38d..fed517f297da 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h > @@ -152,9 +152,6 @@ int atmel_hlcdc_plane_prepare_disc_area(struct > drm_crtc_state *c_state); > > void atmel_hlcdc_crtc_irq(struct drm_crtc *c); > > -void atmel_hlcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, > -struct drm_file *file); > - > void atmel_hlcdc_crtc_suspend(struct drm_crtc *crtc); > void atmel_hlcdc_crtc_resume(struct drm_crtc *crtc); > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Expose exec parameter to force non IA-Coherent for Gen9+
On 13/01/2016 14:11, Dave Gordon wrote: On 13/01/16 12:53, Chris Wilson wrote: On Wed, Jan 13, 2016 at 11:44:39AM +0100, Artur Harasimiuk wrote: Starting from Gen9 we can use IA-Coherent caches. Coherence may be not required in certain cases and can be disabled in an easy way. To do this we can set HDC_FORCE_NON_COHERENT bit in HDC_CHICKEN0 register. This register is part of HW context, however it is private and cannot be programmed from non-privileged batch buffer. Looks like what you need is to add this register to HW whitelist so that UMD can program required behaviour between batches but you need strong justification and approval to whitelist a register. Is there a real usecase or this is only for debug? regards Arun New parameter is to override default programming and allow UMD to decide whether IA-Coherency is not needed for submitted batch buffer. When flag is set KMD emits commands to disable coherency before batch buffer execution starts. After execution finished state is restored. What's the usecase for batch vs context flag? When WaForceEnableNonCoherent is programmed, it does not make sense to allow for coherency because this can lead to GPU hangs. In such situation flag is ignored. Signed-off-by: Artur Harasimiuk --- drivers/gpu/drm/i915/i915_dma.c| 3 +++ drivers/gpu/drm/i915/i915_drv.h| 4 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 drivers/gpu/drm/i915/intel_lrc.c | 28 drivers/gpu/drm/i915/intel_ringbuffer.c| 7 +++ include/uapi/drm/i915_drm.h| 8 +++- 6 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 44a896c..79ecf20 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -172,6 +172,9 @@ static int i915_getparam(struct drm_device *dev, void *data, case I915_PARAM_HAS_EXEC_SOFTPIN: value = 1; break; +case I915_PARAM_HAS_EXEC_FORCE_NON_COHERENT: +value = INTEL_INFO(dev)->gen >= 9; This should actually report the w/a status, or else you need to provide some other means for userspace to detect if it can successfully force non-coherency. Agreed; especially with the naming used, something called *Force*Something shouldn't be reported as available but then silently ignored on use because some workaround is in effect. If user code knows that it wants to *force* the use of a particular mode then it has to know if it doesn't take effect. OTOH is it really "Force", or is it "Allow"? Or "Request", or "Prefer"? +break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..71d739c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -886,6 +886,10 @@ struct intel_context { } engine[I915_NUM_RINGS]; struct list_head link; + +struct { +unsigned int WaForceEnableNonCoherent:1; +} wa; }; enum fb_op_origin { diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index d469c47..2997a58 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1400,6 +1400,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (!i915_gem_check_execbuffer(args)) return -EINVAL; +if ((args->flags & I915_EXEC_FORCE_NON_COHERENT) && +INTEL_INFO(dev)->gen < 9) +return -EINVAL; It would seem easier to simply ignore this request in all situations where it doesn't apply, rather than the current select few. ret = validate_exec_list(dev, exec, args->buffer_count); if (ret) return ret; @@ -1494,6 +1498,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, i915_gem_context_reference(ctx); +/* Clear this flag when WA is programmed */ +if (ctx->wa.WaForceEnableNonCoherent) +args->flags &= ~I915_EXEC_FORCE_NON_COHERENT; + This seems back-to-front (I'm only looking at the names, without checking what this bit really does), but surely if the "ForceEnableNonCoherent" workaround is in effect it should force the "FORCE_NON_COHERENT" flag ON, not OFF? Also, the use of a per-batch bool doesn't seem to leave any don't-care option. Is there any use case where the user code absolutely *doesn't* want this non-coherency? Or which of these are useful: 1.Require non-coherent, fail if not possible 2.Request non-coherent, warn me but continue if not possible 3.Prefer non-coherent, ignore failure (don't even tell me) 4.Don't care, use system default 5.Prefer coherent, ignore failure (don't tell me) 6.Request coherent, warn me (but continue) if not possible 7.Require coherent, fail if not possible Then we just nee
Re: [Intel-gfx] [PATCH] drm/i915: disable non-sequential pfits on ivb/hsw
On Wed, Jan 13, 2016 at 02:33:47PM +, Chris Bainbridge wrote: > The existing code assumes a sequential mapping of panel fitters to pipes > (pfit0-pipeA, pfit1-pipeB, pfit2-pipeC), but boot firmware can > arbitrarily assign any pipe to a pfit on IVB hardware e.g. Macbook UEFI > uses pfit 0 and pipe C for eDP1 when the firmware boots in a non-16:10 > resolution (the last-used resolution is stored in NVRAM by OS X so the > firmware can immediately restore it at boot). When this happens, the > display will appear letterboxed due to incorrect aspect ratio and > attempting to switch to alternative resolutions will fail. Fix this by > disabling any panel fitters which have been non-sequentially assigned at > boot time. > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=93523 > Signed-off-by: Chris Bainbridge > --- > drivers/gpu/drm/i915/intel_display.c | 26 ++ > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 32cf97346978..9e588139a2dd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9170,6 +9170,24 @@ static void ironlake_get_pfit_config(struct intel_crtc > *crtc, get_config should never touch hw state, only read it out. The right place to put fixup code is in sanitize_crtc. What we need in get_config would be a check to make sure pfit is assigned to our pipe (and not take over the state if so). -Daniel > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > uint32_t tmp; > + int pipe; > + > + /* > + * PF_CTL assumes panel fitter 0 is on pipe A, panel fitter 1 is on > + * pipe B, and panel fitter 2 is on pipe C, but firmware can init IVB > + * panel fitters to any arbitrary pipe (Macbook UEFI uses pfit 0 for > + * pipe C), so find and disable any other mappings. > + */ > + for (pipe = 0; pipe < INTEL_INFO(dev)->num_pipes; pipe++) { > + tmp = I915_READ(PF_CTL(pipe)); > + if (IS_GEN7(dev) && (tmp & PF_ENABLE) && > + PF_PIPE_SEL_IVB(pipe) != (tmp & PF_PIPE_SEL_MASK_IVB)) { > + DRM_DEBUG_KMS("disabling initial panel fitter\n"); > + I915_WRITE(PF_CTL(pipe), 0); > + I915_WRITE(PF_WIN_POS(pipe), 0); > + I915_WRITE(PF_WIN_SZ(pipe), 0); > + } > + } > > tmp = I915_READ(PF_CTL(crtc->pipe)); > > @@ -9177,14 +9195,6 @@ static void ironlake_get_pfit_config(struct intel_crtc > *crtc, > pipe_config->pch_pfit.enabled = true; > pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe)); > pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe)); > - > - /* We currently do not free assignements of panel fitters on > - * ivb/hsw (since we don't use the higher upscaling modes which > - * differentiates them) so just WARN about this case for now. */ > - if (IS_GEN7(dev)) { > - WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) != > - PF_PIPE_SEL_IVB(crtc->pipe)); > - } > } > } > > -- > 2.1.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: disable non-sequential pfits on ivb/hsw
On Wed, Jan 13, 2016 at 05:13:31PM +0100, Daniel Vetter wrote: > On Wed, Jan 13, 2016 at 02:33:47PM +, Chris Bainbridge wrote: > > The existing code assumes a sequential mapping of panel fitters to pipes > > (pfit0-pipeA, pfit1-pipeB, pfit2-pipeC), but boot firmware can > > arbitrarily assign any pipe to a pfit on IVB hardware e.g. Macbook UEFI > > uses pfit 0 and pipe C for eDP1 when the firmware boots in a non-16:10 > > resolution (the last-used resolution is stored in NVRAM by OS X so the > > firmware can immediately restore it at boot). When this happens, the > > display will appear letterboxed due to incorrect aspect ratio and > > attempting to switch to alternative resolutions will fail. Fix this by > > disabling any panel fitters which have been non-sequentially assigned at > > boot time. > > > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=93523 > > Signed-off-by: Chris Bainbridge > > --- > > drivers/gpu/drm/i915/intel_display.c | 26 ++ > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 32cf97346978..9e588139a2dd 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9170,6 +9170,24 @@ static void ironlake_get_pfit_config(struct > > intel_crtc *crtc, > > get_config should never touch hw state, only read it out. The right place > to put fixup code is in sanitize_crtc. What we need in get_config would be > a check to make sure pfit is assigned to our pipe (and not take over the > state if so). maybe even throw a new sanitize_pfit function in for clarity, since the problem is that pfit is _not_ associated with the crtc at a hw level. -Daniel > > > struct drm_device *dev = crtc->base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t tmp; > > + int pipe; > > + > > + /* > > +* PF_CTL assumes panel fitter 0 is on pipe A, panel fitter 1 is on > > +* pipe B, and panel fitter 2 is on pipe C, but firmware can init IVB > > +* panel fitters to any arbitrary pipe (Macbook UEFI uses pfit 0 for > > +* pipe C), so find and disable any other mappings. > > +*/ > > + for (pipe = 0; pipe < INTEL_INFO(dev)->num_pipes; pipe++) { > > + tmp = I915_READ(PF_CTL(pipe)); > > + if (IS_GEN7(dev) && (tmp & PF_ENABLE) && > > + PF_PIPE_SEL_IVB(pipe) != (tmp & PF_PIPE_SEL_MASK_IVB)) { > > + DRM_DEBUG_KMS("disabling initial panel fitter\n"); > > + I915_WRITE(PF_CTL(pipe), 0); > > + I915_WRITE(PF_WIN_POS(pipe), 0); > > + I915_WRITE(PF_WIN_SZ(pipe), 0); > > + } > > + } > > > > tmp = I915_READ(PF_CTL(crtc->pipe)); > > > > @@ -9177,14 +9195,6 @@ static void ironlake_get_pfit_config(struct > > intel_crtc *crtc, > > pipe_config->pch_pfit.enabled = true; > > pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe)); > > pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe)); > > - > > - /* We currently do not free assignements of panel fitters on > > -* ivb/hsw (since we don't use the higher upscaling modes which > > -* differentiates them) so just WARN about this case for now. */ > > - if (IS_GEN7(dev)) { > > - WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) != > > - PF_PIPE_SEL_IVB(crtc->pipe)); > > - } > > } > > } > > > > -- > > 2.1.4 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3] drm/i915: Do not call API requiring struct_mutex where it is not available
From: Tvrtko Ursulin LRC code was calling GEM API like i915_gem_obj_ggtt_offset from places where the struct_mutex cannot be grabbed (irq handlers). To avoid that this patch caches some interesting bits and values in the engine and context structures. Some usages are also removed where they are not needed like a few asserts which are either impossible or have been checked already during engine initialization. Side benefit is also that interrupt handlers and command submission stop evaluating invariant conditionals, like what Gen we are running on, on every interrupt and every command submitted. This patch deals with logical ring context id and descriptors while subsequent patches will deal with the remaining issues. v2: * Cache the VMA instead of the address. (Chris Wilson) * Incorporate Dave Gordon's good comments and function name. v3: * Extract ctx descriptor template to a function and group functions dealing with ctx descriptor & co together near top of the file. (Dave Gordon) Signed-off-by: Tvrtko Ursulin Cc: Daniel Vetter Cc: Dave Gordon --- drivers/gpu/drm/i915/i915_debugfs.c | 15 ++-- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_gem_gtt.h | 1 - drivers/gpu/drm/i915/intel_lrc.c| 151 +++- drivers/gpu/drm/i915/intel_lrc.h| 4 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 2 + 6 files changed, 103 insertions(+), 72 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index e3377abc0d4d..0b3550f05026 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1994,12 +1994,13 @@ static int i915_context_status(struct seq_file *m, void *unused) } static void i915_dump_lrc_obj(struct seq_file *m, - struct intel_engine_cs *ring, - struct drm_i915_gem_object *ctx_obj) + struct intel_context *ctx, + struct intel_engine_cs *ring) { struct page *page; uint32_t *reg_state; int j; + struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; unsigned long ggtt_offset = 0; if (ctx_obj == NULL) { @@ -2009,7 +2010,7 @@ static void i915_dump_lrc_obj(struct seq_file *m, } seq_printf(m, "CONTEXT: %s %u\n", ring->name, - intel_execlists_ctx_id(ctx_obj)); + intel_execlists_ctx_id(ctx, ring)); if (!i915_gem_obj_ggtt_bound(ctx_obj)) seq_puts(m, "\tNot bound in GGTT\n"); @@ -2058,8 +2059,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused) list_for_each_entry(ctx, &dev_priv->context_list, link) { for_each_ring(ring, dev_priv, i) { if (ring->default_context != ctx) - i915_dump_lrc_obj(m, ring, - ctx->engine[i].state); + i915_dump_lrc_obj(m, ctx, ring); } } @@ -2133,11 +2133,8 @@ static int i915_execlists(struct seq_file *m, void *data) seq_printf(m, "\t%d requests in queue\n", count); if (head_req) { - struct drm_i915_gem_object *ctx_obj; - - ctx_obj = head_req->ctx->engine[ring_id].state; seq_printf(m, "\tHead request id: %u\n", - intel_execlists_ctx_id(ctx_obj)); + intel_execlists_ctx_id(head_req->ctx, ring)); seq_printf(m, "\tHead request tail: %u\n", head_req->tail); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd1809936..c900db272b96 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -883,6 +883,8 @@ struct intel_context { struct drm_i915_gem_object *state; struct intel_ringbuffer *ringbuf; int pin_count; + struct i915_vma *lrc_vma; + u64 lrc_desc; } engine[I915_NUM_RINGS]; struct list_head link; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index b448ad832dcf..e5737963ab79 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -44,7 +44,6 @@ typedef uint64_t gen8_ppgtt_pml4e_t; #define gtt_total_entries(gtt) ((gtt).base.total >> PAGE_SHIFT) - /* gen6-hsw has bit 11-4 for physical addr bit 39-32 */ #define GEN6_GTT_ADDR_ENCODE(addr) ((addr) | (((addr) >> 28) & 0xff0)) #define GEN6_PTE_ADDR_ENCODE(addr) GEN6_GTT_ADDR_ENCODE(addr) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 5027699c5291..1a0d85325072 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +
Re: [Intel-gfx] ✗ failure: Fi.CI.BAT
On Wed, Jan 13, 2016 at 03:13:40PM -, Patchwork wrote: > == Summary == > > Built on 4d09810b01441f9124c072a866f608b748f92f6c drm-intel-nightly: > 2016y-01m-13d-12h-32m-08s UTC integration manifest > > Test gem_ctx_basic: > pass -> FAIL (hsw-gt2) This seems to be a complete fluke. I looked at detailed results and there's simply no output. Long-term history doesn't show a failure either. I think we can shrug this one off (for now at least). > Test gem_storedw_loop: > Subgroup basic-render: > dmesg-warn -> PASS (skl-i7k-2) UNSTABLE > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-c: > pass -> DMESG-WARN (bdw-ultra) https://bugs.freedesktop.org/show_bug.cgi?id=93699 So looks good, I'll apply the patch. -Daniel > > bdw-nuci7total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 > bdw-ultratotal:138 pass:131 dwarn:1 dfail:0 fail:0 skip:6 > bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 > hsw-gt2 total:141 pass:136 dwarn:0 dfail:0 fail:1 skip:4 > hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 > ilk-hp8440p total:141 pass:100 dwarn:4 dfail:0 fail:0 skip:37 > ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 > skl-i5k-2total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 > skl-i7k-2total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 > snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 > > Results at /archive/results/CI_IGT_test/Patchwork_1170/ > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v10] drm/i915: Extend LRC pinning to cover GPU context writeback
Use the first retired request on a new context to unpin the old context. This ensures that the hw context remains bound until it has been written back to by the GPU. Now that the context is pinned until later in the request/context lifecycle, it no longer needs to be pinned from context_queue to retire_requests. This fixes an issue with GuC submission where the GPU might not have finished writing back the context before it is unpinned. This results in a GPU hang. v2: Moved the new pin to cover GuC submission (Alex Dai) Moved the new unpin to request_retire to fix coverage leak v3: Added switch to default context if freeing a still pinned context just in case the hw was actually still using it v4: Unwrapped context unpin to allow calling without a request v5: Only create a switch to idle context if the ring doesn't already have a request pending on it (Alex Dai) Rename unsaved to dirty to avoid double negatives (Dave Gordon) Changed _no_req postfix to __ prefix for consistency (Dave Gordon) Split out per engine cleanup from context_free as it was getting unwieldy Corrected locking (Dave Gordon) v6: Removed some bikeshedding (Mika Kuoppala) Added explanation of the GuC hang that this fixes (Daniel Vetter) v7: Removed extra per request pinning from ring reset code (Alex Dai) Added forced ring unpin/clean in error case in context free (Alex Dai) v8: Renamed lrc specific last_context to lrc_last_context as there were some reset cases where the codepaths leaked (Mika Kuoppala) NULL'd last_context in reset case - there was a pointer leak if someone did reset->close context. v9: Rebase over "Fix context/engine cleanup order" v10: Rebase over nightly, remove WARN_ON which caused the dependency on dev. Signed-off-by: Nick Hoath Issue: VIZ-4277 Cc: Daniel Vetter Cc: David Gordon Cc: Chris Wilson Cc: Alex Dai Cc: Mika Kuoppala --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 3 + drivers/gpu/drm/i915/intel_lrc.c| 138 ++-- drivers/gpu/drm/i915/intel_lrc.h| 1 + drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 5 files changed, 121 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..d28e10a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -882,6 +882,7 @@ struct intel_context { struct { struct drm_i915_gem_object *state; struct intel_ringbuffer *ringbuf; + bool dirty; int pin_count; } engine[I915_NUM_RINGS]; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ddc21d4..7b79405 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1413,6 +1413,9 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) { trace_i915_gem_request_retire(request); + if (i915.enable_execlists) + intel_lr_context_complete_check(request); + /* We know the GPU must have read the request to have * sent us the seqno + interrupt, so use the position * of tail of the request to update the last known position diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 5027699..b661058 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -585,9 +585,6 @@ static int execlists_context_queue(struct drm_i915_gem_request *request) struct drm_i915_gem_request *cursor; int num_elements = 0; - if (request->ctx != ring->default_context) - intel_lr_context_pin(request); - i915_gem_request_reference(request); spin_lock_irq(&ring->execlist_lock); @@ -763,6 +760,13 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) if (intel_ring_stopped(ring)) return; + if (request->ctx != ring->default_context) { + if (!request->ctx->engine[ring->id].dirty) { + intel_lr_context_pin(request); + request->ctx->engine[ring->id].dirty = true; + } + } + if (dev_priv->guc.execbuf_client) i915_guc_submit(dev_priv->guc.execbuf_client, request); else @@ -989,12 +993,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) spin_unlock_irq(&ring->execlist_lock); list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) { - struct intel_context *ctx = req->ctx; - struct drm_i915_gem_object *ctx_obj = - ctx->engine[ring->id].state; - - if (ctx_obj && (ctx != ring->default_context)) - intel_lr_context_unpin(req); list_del(&req->execlist_link); i915_gem_request_un
[Intel-gfx] [PATCH] drm/i915: Dump power well states on unclaimed trace
It is beneficial to know the exact sw states of power wells at the moment when unclaimed register access is detect. When the backtrace has been printed to dmesg, it is followed by a power well states, for example: --[power wells, wakeref_count 2] -- Name sw statecount display off 0 dpio-tx-b-01 off 0 dpio-tx-b-23 off 0 dpio-tx-c-01 off 0 dpio-tx-c-23 off 0 dpio-common off 0 - [power wells end] This helps bug triaging as it is immediately obvious that the unclaimed access trace is not a fluke and not about out of bounds access. Rather the call chain shown by above warn on trace have failed to enable required power well. Cc: Ville Syrjälä Cc: Imre Deak Signed-off-by: Mika Kuoppala --- drivers/gpu/drm/i915/intel_drv.h| 1 + drivers/gpu/drm/i915/intel_runtime_pm.c | 26 ++ drivers/gpu/drm/i915/intel_uncore.c | 4 +++- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e27954d2edad..b83faec2d526 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1445,6 +1445,7 @@ int intel_power_domains_init(struct drm_i915_private *); void intel_power_domains_fini(struct drm_i915_private *); void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume); void intel_power_domains_suspend(struct drm_i915_private *dev_priv); +void intel_power_domains_dump_wells(struct drm_i915_private *dev_priv); void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv); void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv); void intel_runtime_pm_enable(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index bbca527184d0..43af603aebe6 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2217,6 +2217,32 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); } +void intel_power_domains_dump_wells(struct drm_i915_private *dev_priv) +{ + struct i915_power_domains *power_domains; + struct i915_power_well *power_well; + int i; + + power_domains = &dev_priv->power_domains; + + /* Intentionally omitting power domain lock */ + + pr_info("--[power wells, wakeref_count %d] --\n", + atomic_read(&dev_priv->pm.wakeref_count)); + pr_info("%-20s %-11s %-6s\n", "Name", "sw state", "count"); + + for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) { + if (power_well->always_on) + continue; + + pr_info("%-20s %-11s %-6d\n", + power_well->name, + power_well->hw_enabled ? "on" : "off", + power_well->count); + } + pr_info("- [power wells end] \n"); +} + /** * intel_runtime_pm_get - grab a runtime pm reference * @dev_priv: i915 device instance diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index c3c13dc929cb..90875009f789 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -635,8 +635,10 @@ __unclaimed_reg_debug(struct drm_i915_private *dev_priv, "Unclaimed register detected %s %s register 0x%x\n", before ? "before" : "after", read ? "reading" : "writing to", -i915_mmio_reg_offset(reg))) +i915_mmio_reg_offset(reg))) { i915.mmio_debug--; /* Only report the first N failures */ + intel_power_domains_dump_wells(dev_priv); + } } static inline void -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: disable non-sequential pfits on ivb/hsw
On Wed, Jan 13, 2016 at 05:14:15PM +0100, Daniel Vetter wrote: > On Wed, Jan 13, 2016 at 05:13:31PM +0100, Daniel Vetter wrote: > > On Wed, Jan 13, 2016 at 02:33:47PM +, Chris Bainbridge wrote: > > > The existing code assumes a sequential mapping of panel fitters to pipes > > > (pfit0-pipeA, pfit1-pipeB, pfit2-pipeC), but boot firmware can > > > arbitrarily assign any pipe to a pfit on IVB hardware e.g. Macbook UEFI > > > uses pfit 0 and pipe C for eDP1 when the firmware boots in a non-16:10 > > > resolution (the last-used resolution is stored in NVRAM by OS X so the > > > firmware can immediately restore it at boot). When this happens, the > > > display will appear letterboxed due to incorrect aspect ratio and > > > attempting to switch to alternative resolutions will fail. Fix this by > > > disabling any panel fitters which have been non-sequentially assigned at > > > boot time. > > > > > > Link: https://bugs.freedesktop.org/show_bug.cgi?id=93523 > > > Signed-off-by: Chris Bainbridge > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 26 ++ > > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 32cf97346978..9e588139a2dd 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -9170,6 +9170,24 @@ static void ironlake_get_pfit_config(struct > > > intel_crtc *crtc, > > > > get_config should never touch hw state, only read it out. The right place > > to put fixup code is in sanitize_crtc. What we need in get_config would be > > a check to make sure pfit is assigned to our pipe (and not take over the > > state if so). > > maybe even throw a new sanitize_pfit function in for clarity, since the > problem is that pfit is _not_ associated with the crtc at a hw level. Ideally we'd make the crtc<->pfit mapping flexible in the driver since IIRC the first pfit could have special powers. But I guess it could be a bit too much work for little gain. Although it shouldn't be too different from the SKL scaler assignment stuff, so maybe not that much work... -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ warning: Fi.CI.BAT
== Summary == Built on 0f3950521e4fd6b37f9d48d8484e2e0ce926ecca drm-intel-nightly: 2016y-01m-13d-15h-43m-52s UTC integration manifest Test gem_basic: Subgroup create-close: dmesg-warn -> PASS (skl-i7k-2) Test gem_cpu_reloc: Subgroup basic: dmesg-fail -> PASS (skl-i7k-2) Test gem_ctx_param_basic: Subgroup basic: dmesg-warn -> PASS (skl-i7k-2) Subgroup invalid-param-set: dmesg-warn -> PASS (skl-i7k-2) Subgroup non-root-set-no-zeromap: dmesg-warn -> PASS (skl-i7k-2) Subgroup root-set-no-zeromap-disabled: dmesg-warn -> PASS (skl-i7k-2) Test gem_mmap: Subgroup basic: dmesg-warn -> PASS (skl-i7k-2) Test gem_mmap_gtt: Subgroup basic-read: dmesg-warn -> PASS (skl-i7k-2) Test gem_storedw_loop: Subgroup basic-render: pass -> DMESG-WARN (bdw-nuci7) Test kms_addfb_basic: Subgroup addfb25-modifier-no-flag: dmesg-warn -> PASS (skl-i7k-2) Subgroup addfb25-x-tiled-mismatch: dmesg-warn -> PASS (skl-i7k-2) Subgroup addfb25-yf-tiled: dmesg-warn -> PASS (skl-i7k-2) Subgroup bad-pitch-1024: dmesg-warn -> PASS (skl-i7k-2) Subgroup bad-pitch-63: dmesg-warn -> PASS (skl-i7k-2) Subgroup bad-pitch-999: dmesg-warn -> PASS (skl-i7k-2) Subgroup clobberred-modifier: dmesg-warn -> PASS (skl-i7k-2) Subgroup too-high: dmesg-warn -> PASS (skl-i7k-2) Subgroup too-wide: dmesg-warn -> PASS (skl-i7k-2) Subgroup unused-offsets: dmesg-warn -> PASS (skl-i7k-2) Test kms_flip: Subgroup basic-plain-flip: dmesg-fail -> PASS (skl-i7k-2) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-a-frame-sequence: dmesg-fail -> PASS (skl-i7k-2) Subgroup nonblocking-crc-pipe-b-frame-sequence: dmesg-warn -> PASS (bdw-ultra) Subgroup read-crc-pipe-a-frame-sequence: pass -> SKIP (bdw-nuci7) Subgroup read-crc-pipe-b: pass -> DMESG-WARN (bdw-ultra) Subgroup read-crc-pipe-b-frame-sequence: dmesg-fail -> PASS (skl-i7k-2) Test prime_self_import: Subgroup basic-with_two_bos: dmesg-warn -> PASS (skl-i7k-2) bdw-nuci7total:138 pass:127 dwarn:1 dfail:0 fail:0 skip:10 bdw-ultratotal:138 pass:130 dwarn:2 dfail:0 fail:0 skip:6 bsw-nuc-2total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 hsw-xps12total:138 pass:133 dwarn:1 dfail:0 fail:0 skip:4 ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 ivb-t430stotal:135 pass:122 dwarn:3 dfail:4 fail:0 skip:6 skl-i5k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 skl-i7k-2total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 snb-x220ttotal:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 Results at /archive/results/CI_IGT_test/Patchwork_1172/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Force ordering on request submission and hangcheck
Hangcheck is run on irq context and might be active on a completely different CPU that is submitting requests. And as we have been very careful not to add locking to hangcheck to guard against driver failures, we need to be careful with the coherency. Update ring last seqno and add to request lists early, with write memory barrier, before pushing the request to ring. On hangcheck side, use read memory barrier before inspecting the ring last seqno. This ensures that when hangcheck is inspecting the state, it will not see a active ring without requests in it, and then falsely report it as a missed irq situation. References: https://bugs.freedesktop.org/show_bug.cgi?id=93693 Cc: Chris Wilson Cc: Daniel Vetter Signed-off-by: Mika Kuoppala --- drivers/gpu/drm/i915/i915_gem.c | 14 +++--- drivers/gpu/drm/i915/i915_irq.c | 7 --- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ddc21d4b388d..0ca3856ffbf1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2584,6 +2584,17 @@ void __i915_add_request(struct drm_i915_gem_request *request, */ request->postfix = intel_ring_get_tail(ringbuf); + /* Hangcheck runs in irq context and might be even +* on different CPU. So we need to the do last seqno and +* list addition early with memory barrier. Otherwise hangcheck +* might see active ring without any requests and +* consider it falsely as a missed irq situation. +*/ + request->previous_seqno = ring->last_submitted_seqno; + ring->last_submitted_seqno = request->seqno; + list_add_tail(&request->list, &ring->request_list); + wmb(); /* Flush last_submitted_seqno and request->list for hangcheck */ + if (i915.enable_execlists) ret = ring->emit_request(request); else { @@ -2605,9 +2616,6 @@ void __i915_add_request(struct drm_i915_gem_request *request, request->batch_obj = obj; request->emitted_jiffies = jiffies; - request->previous_seqno = ring->last_submitted_seqno; - ring->last_submitted_seqno = request->seqno; - list_add_tail(&request->list, &ring->request_list); trace_i915_gem_request_add(request); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 25a89373df63..4dd8e6f0c1de 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2801,8 +2801,9 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) static bool ring_idle(struct intel_engine_cs *ring, u32 seqno) { - return (list_empty(&ring->request_list) || - i915_seqno_passed(seqno, ring->last_submitted_seqno)); + rmb(); /* Ordering against __i915_add_request() */ + return (i915_seqno_passed(seqno, ring->last_submitted_seqno) + || list_empty(&ring->request_list)); } static bool @@ -3101,8 +3102,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work) semaphore_clear_deadlocks(dev_priv); - seqno = ring->get_seqno(ring, false); acthd = intel_ring_get_active_head(ring); + seqno = ring->get_seqno(ring, false); if (ring->hangcheck.seqno == seqno) { if (ring_idle(ring, seqno)) { -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] drm/i915: Reviewed fb offsets[] prep patches
On Tue, Jan 12, 2016 at 09:08:30PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > Here's a repost of some already reviewed patches from my larger fb > offsets[] series [1] from last year, for the sake of the CI system. > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/078050.html > > Ville Syrjälä (7): > drm/i915: Pass modifier instead of tiling_mode to > gen4_compute_page_offset() > drm/i915: Factor out intel_tile_width() > drm/i915: Redo intel_tile_height() as intel_tile_size() / > intel_tile_width() > drm/i915: change intel_fill_fb_ggtt_view() to use the real tile size > drm/i915: Use intel_tile_{size,width,height}() in > intel_gen4_compute_page_offset() > drm/i915: s/intel_gen4_compute_page_offset/intel_compute_tile_offset/ > drm/i915: Refactor intel_surf_alignment() Series pushed to dinq. Thanks for the reviews. > > drivers/gpu/drm/i915/intel_display.c | 248 > +-- > drivers/gpu/drm/i915/intel_drv.h | 19 ++- > drivers/gpu/drm/i915/intel_sprite.c | 32 ++--- > 3 files changed, 145 insertions(+), 154 deletions(-) > > -- > 2.4.10 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/20] drm/i915: Generalise common GPU engine reset request/unrequest code
From: Tomas Elf GPU engine reset handshaking is something that is applicable to both full GPU reset and engine reset, which is something that is part of the upcoming TDR per-engine hang recovery patches. Break out the common engine reset request/unrequest code (originally written by Mika Kuoppala) for reuse later in the TDR enablement patch series. Signed-off-by: Tomas Elf Cc: Mika Kuoppala --- drivers/gpu/drm/i915/intel_uncore.c | 46 ++--- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index c3c13dc..2df4246 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1529,32 +1529,50 @@ static int wait_for_register(struct drm_i915_private *dev_priv, return wait_for((I915_READ(reg) & mask) == value, timeout_ms); } +static inline int gen8_request_engine_reset(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->dev->dev_private; + int ret = 0; + + I915_WRITE(RING_RESET_CTL(engine->mmio_base), + _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET)); + + ret = wait_for_register(dev_priv, + RING_RESET_CTL(engine->mmio_base), + RESET_CTL_READY_TO_RESET, + RESET_CTL_READY_TO_RESET, + 700); + if (ret) + DRM_ERROR("%s: reset request timeout\n", engine->name); + + return ret; +} + +static inline int gen8_unrequest_engine_reset(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->dev->dev_private; + + I915_WRITE(RING_RESET_CTL(engine->mmio_base), + _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET)); + + return 0; +} + static int gen8_do_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_engine_cs *engine; int i; - for_each_ring(engine, dev_priv, i) { - I915_WRITE(RING_RESET_CTL(engine->mmio_base), - _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET)); - - if (wait_for_register(dev_priv, - RING_RESET_CTL(engine->mmio_base), - RESET_CTL_READY_TO_RESET, - RESET_CTL_READY_TO_RESET, - 700)) { - DRM_ERROR("%s: reset request timeout\n", engine->name); + for_each_ring(engine, dev_priv, i) + if (gen8_request_engine_reset(engine)) goto not_ready; - } - } return gen6_do_reset(dev); not_ready: for_each_ring(engine, dev_priv, i) - I915_WRITE(RING_RESET_CTL(engine->mmio_base), - _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET)); + gen8_unrequest_engine_reset(engine); return -EIO; } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/20] TDR/watchdog support for gen8
These patches were sent previously a while ago[1] so rebased on latest nightly and resending again for feedback. This patch series adds support for Per engine resets, watchdog timeout reset. Please see [1] for detailed description. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-October/078696.html Tim Gore (1): drm/i915: drm/i915 changes to simulated hangs Tomas Elf (19): drm/i915: Make i915_gem_reset_ring_status() public drm/i915: Generalise common GPU engine reset request/unrequest code drm/i915: TDR / per-engine hang recovery support for gen8. drm/i915: TDR / per-engine hang detection drm/i915: Extending i915_gem_check_wedge to check engine reset in progress drm/i915: Reinstate hang recovery work queue. drm/i915: Watchdog timeout: Hang detection integration into error handler drm/i915: Watchdog timeout: IRQ handler for gen8 drm/i915: Watchdog timeout: Ringbuffer command emission for gen8 drm/i915: Watchdog timeout: DRM kernel interface enablement drm/i915: Fake lost context event interrupts through forced CSB checking. drm/i915: Debugfs interface for per-engine hang recovery. drm/i915: Test infrastructure for context state inconsistency simulation drm/i915: TDR/watchdog trace points. drm/i915: Port of Added scheduler support to __wait_request() calls drm/i915: Fix __i915_wait_request() behaviour during hang detection. drm/i915: Extended error state with TDR count, watchdog count and engine reset count drm/i915: TDR / per-engine hang recovery kernel docs drm/i915: Enable TDR / per-engine hang recovery Documentation/DocBook/gpu.tmpl | 476 ++ drivers/gpu/drm/i915/i915_debugfs.c | 163 +- drivers/gpu/drm/i915/i915_dma.c | 80 +++ drivers/gpu/drm/i915/i915_drv.c | 328 drivers/gpu/drm/i915/i915_drv.h | 90 +++- drivers/gpu/drm/i915/i915_gem.c | 152 +- drivers/gpu/drm/i915/i915_gpu_error.c | 8 +- drivers/gpu/drm/i915/i915_irq.c | 263 -- drivers/gpu/drm/i915/i915_params.c | 19 + drivers/gpu/drm/i915/i915_params.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 9 + drivers/gpu/drm/i915/i915_trace.h | 354 - drivers/gpu/drm/i915/intel_display.c| 5 +- drivers/gpu/drm/i915/intel_lrc.c| 865 +++- drivers/gpu/drm/i915/intel_lrc.h| 16 +- drivers/gpu/drm/i915/intel_lrc_tdr.h| 39 ++ drivers/gpu/drm/i915/intel_ringbuffer.c | 90 +++- drivers/gpu/drm/i915/intel_ringbuffer.h | 95 drivers/gpu/drm/i915/intel_uncore.c | 197 +++- include/uapi/drm/i915_drm.h | 5 +- 20 files changed, 3134 insertions(+), 122 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_lrc_tdr.h -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/20] drm/i915: TDR / per-engine hang detection
From: Tomas Elf With the per-engine hang recovery path already in place this patch adds per-engine hang detection by letting the periodic hang checker detect hangs on individual engines and communicate this to the error handler. During hang checking every engine is checked and the hang detection status for each engine is aggregated into a single 32-bit engine flag mask that contains all the engine flags (1 << ring->id) of all the hung engines or'ed together. The per-engine path in the error handler then sets up the hangcheck state for each invidual, hung engine based on the engine flag mask before potentially calling the per-engine hang recovery path. This allows the hang detection to happen in lock-step for all engines in parallel and lets the driver process all hung engines in turn in the error handler. Signed-off-by: Tomas Elf --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- drivers/gpu/drm/i915/i915_irq.c | 41 +++-- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index e3377ab..6d1b6c3 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4720,7 +4720,7 @@ i915_wedged_set(void *data, u64 val) intel_runtime_pm_get(dev_priv); - i915_handle_error(dev, val, + i915_handle_error(dev, 0x0, val, "Manually setting wedged to %llu", val); 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 e866f14..85cf692 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2765,8 +2765,8 @@ static inline void i915_hangcheck_reinit(struct intel_engine_cs *engine) /* i915_irq.c */ void i915_queue_hangcheck(struct drm_device *dev); -__printf(3, 4) -void i915_handle_error(struct drm_device *dev, bool wedged, +__printf(4, 5) +void i915_handle_error(struct drm_device *dev, u32 engine_mask, bool wedged, const char *fmt, ...); extern void intel_irq_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6a0ec37..fef74cf 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2712,15 +2712,29 @@ static void i915_report_and_clear_eir(struct drm_device *dev) /** * i915_handle_error - handle a gpu error - * @dev: drm device + * @dev: drm device + * @engine_mask: Bit mask containing the engine flags of all engines + * associated with one or more detected errors. + * May be 0x0. + * If wedged is set to true this implies that one or more + * engine hangs were detected. In this case we will + * attempt to reset all engines that have been detected + * as hung. + * If a previous engine reset was attempted too recently + * or if one of the current engine resets fails we fall + * back to legacy full GPU reset. + * @wedged:true = Hang detected, invoke hang recovery. + * @fmt, ...: Error message describing reason for error. * * Do some basic checking of register state at error time and * dump it to the syslog. Also call i915_capture_error_state() to make * sure we get a record and make it available in debugfs. Fire a uevent * so userspace knows something bad happened (should trigger collection - * of a ring dump etc.). + * of a ring dump etc.). If a hang was detected (wedged = true) try to + * reset the associated engine. Failing that, try to fall back to legacy + * full GPU reset recovery mode. */ -void i915_handle_error(struct drm_device *dev, bool wedged, +void i915_handle_error(struct drm_device *dev, u32 engine_mask, bool wedged, const char *fmt, ...) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2729,12 +2743,6 @@ void i915_handle_error(struct drm_device *dev, bool wedged, struct intel_engine_cs *engine; - /* -* NB: Placeholder until the hang checker supports -* per-engine hang detection. -*/ - u32 engine_mask = 0; - va_start(args, fmt); vscnprintf(error_msg, sizeof(error_msg), fmt, args); va_end(args); @@ -3162,7 +3170,7 @@ ring_stuck(struct intel_engine_cs *ring, u64 acthd) */ tmp = I915_READ_CTL(ring); if (tmp & RING_WAIT) { - i915_handle_error(dev, false, + i915_handle_error(dev, intel_ring_flag(ring), false, "Kicking stuck wait on %s", ring->name); I915_WRITE_CTL(ring, tmp); @@ -3174,7 +3182,7 @@ ring_stuck(struct intel_engine_cs *ring, u64 acthd)
[Intel-gfx] [PATCH 01/20] drm/i915: Make i915_gem_reset_ring_status() public
From: Tomas Elf Makes i915_gem_reset_ring_status() public for use from engine reset path in order to replicate the same behavior as in full GPU reset but for a single engine. Signed-off-by: Tomas Elf Cc: Chris Wilson Cc: Mika Kuoppala --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 104bd18..703a320 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3007,6 +3007,8 @@ static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv) } void i915_gem_reset(struct drm_device *dev); +void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv, + struct intel_engine_cs *ring); bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force); int __must_check i915_gem_init(struct drm_device *dev); int i915_gem_init_rings(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6c60e04..e3cfed2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2775,8 +2775,8 @@ i915_gem_find_active_request(struct intel_engine_cs *ring) return NULL; } -static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv, - struct intel_engine_cs *ring) +void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv, + struct intel_engine_cs *ring) { struct drm_i915_gem_request *request; bool ring_hung; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/20] drm/i915: TDR / per-engine hang recovery support for gen8.
From: Tomas Elf TDR = Timeout Detection and Recovery. This change introduces support for TDR-style per-engine reset as an initial, less intrusive hang recovery option to be attempted before falling back to the legacy full GPU reset recovery mode if necessary. Initially we're only supporting gen8 but adding support for gen7 is straight-forward since we've already established an extensible framework where gen7 support can be plugged in (add corresponding versions of intel_ring_enable, intel_ring_disable, intel_ring_save, intel_ring_restore, etc.). 1. Per-engine recovery vs. Full GPU recovery To capture the state of a single engine being detected as hung there is now a new flag for every engine that can be set once the decision has been made to schedule hang recovery for that particular engine. This patch only provides the hang recovery path but not the hang detection integration so for now there is no way of detecting individual engines as hung and targetting that individual engine for per-engine hang recovery. The following algorithm is used to determine when to use which recovery mode given that hang detection has somehow detected a hang on an individual engine and given that per-engine hang recovery has been enabled (which it by default is not): 1. The error handler checks all engines that have been marked as hung by the hang checker and checks how long ago it was since it last attempted to do per-engine hang recovery for each respective, currently hung engine. If the measured time period is within a certain time window, i.e. the last per-engine hang recovery was done too recently, it is determined that the previously attempted per-engine hang recovery was ineffective and the step is taken to promote the current hang to a full GPU reset. The default value for this time window is 10 seconds, meaning any hang happening within 10 seconds of a previous hang on the same engine will be promoted to full GPU reset. (of course, as long as the per-engine hang recovery option is disabled this won't matter and the error handler will always go for legacy full GPU reset) 2. If the error handler determines that no currently hung engine has recently had hang recovery a per-engine hang recovery is scheduled. 3. If the decision to go with per-engine hang recovery is not taken, or if per-engine hang recovery is attempted but failed for whatever reason, TDR falls back to legacy full GPU recovery. NOTE: Gen7 and earlier will always promote to full GPU reset since there is currently no per-engine reset support for these gens. 2. Context Submission Status Consistency. Per-engine hang recovery on gen8 (or execlist submission mode in general) relies on the basic concept of context submission status consistency. What this means is that we make sure that the status of the hardware and the driver when it comes to the submission of the currently running context on any engine is consistent. For example, when submitting a context to the corresponding ELSP port of an engine we expect the owning request of that context to be at the head of the corresponding execution list queue. Likewise, as long as the context is executing on the GPU we expect the EXECLIST_STATUS register and the context status buffer (CSB) to reflect this. Thus, if the context submission status is consistent the ID of the currently executing context should be in EXECLIST_STATUS and it should be consistent with the context of the head request element in the execution list queue corresponding to that engine. The reason why this is important for per-engine hang recovery in execlist mode is because this recovery mode relies on context resubmission in order to resume execution following the recovery. If a context has been determined to be hung and the per-engine hang recovery mode is engaged leading to the resubmission of that context it's important that the hardware is in fact not busy doing something else or is being idle since a resubmission during this state could cause unforseen side-effects such as unexpected preemptions. There are rare, although consistently reproducable, situations that have shown up in practice where the driver and hardware are no longer consistent with each other, e.g. due to lost context completion interrupts after which the hardware would be idle but the driver would still think that a context would still be active. 3. There is a new reset path for engine reset alongside the legacy full GPU reset path. This path does the following: 1) Check for context submission consistency to make sure that the context that the hardware is currently stuck on is actually what the driver is working on. If not then clearly we're not in a consistently hung state and we bail out early. 2) Disable/idle the engine. This is done through reset handshaking on gen8+ unlike