| Message ID | 1465287291-2187-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/06/16 09:14, Dave Gordon wrote: > The last stage of the GuC loader also sanitises the GuC submission > settings, so should be called unconditionally (even on platforms > without a GuC) to ensure consistent settings; in particular, this > prevents any attempt to use GuC submission on GuCless platforms! > > Also fix error path handling and clarify DRM_INFO fallback message. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 8 +++----- > drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++++---- > 2 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 1bfc260..eae8d7a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4930,11 +4930,9 @@ int i915_gem_init_engines(struct drm_device *dev) > intel_mocs_init_l3cc_table(dev); > > /* We can't enable contexts until all firmware is loaded */ > - if (HAS_GUC(dev)) { > - ret = intel_guc_setup(dev); > - if (ret) > - goto out; > - } > + ret = intel_guc_setup(dev); > + if (ret) > + goto out; > > /* > * Increment the next seqno by 0x100 so we have a visible break > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c > index f2b88c7..4e34c2e 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -425,9 +425,13 @@ int intel_guc_setup(struct drm_device *dev) > if (!i915.enable_guc_loading) { > err = 0; > goto fail; > - } else if (fw_path == NULL || *fw_path == '\0') { > - if (*fw_path == '\0') Ops. I can only assume I meant !fw_path. > - DRM_INFO("No GuC firmware known for this platform\n"); > + } else if (fw_path == NULL) { > + /* Device is known to have no uCode (e.g. no GuC) */ > + err = -ENXIO; > + goto fail; > + } else if (*fw_path == '\0') { > + /* Device has a GuC but we don't know what f/w to load? */ > + DRM_INFO("No GuC firmware known for this platform\n"); > err = -ENODEV; > goto fail; > } > @@ -535,7 +539,7 @@ int intel_guc_setup(struct drm_device *dev) > if (fw_path == NULL) > DRM_INFO("GuC submission without firmware not supported\n"); > if (ret == 0) > - DRM_INFO("Falling back to execlist mode\n"); > + DRM_INFO("Falling back from GuC submission to execlist mode\n"); > else > DRM_ERROR("GuC init failed: %d\n", ret); > } > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On 07/06/16 09:43, Patchwork wrote: > == Series Details == > > Series: series starting with [1/3] drm/i915/guc: fix GuC loading/submission check > URL : https://patchwork.freedesktop.org/series/8380/ > State : failure > > == Summary == > > Series 8380v1 Series without cover letter > http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/1/mbox > > Test core_auth: > Subgroup basic-auth: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Test drv_module_reload_basic: > pass -> DMESG-WARN (ro-byt-n2820) Bug 93381 - [BAT BYT] dmesg WARNING in snd-hda > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-cmd: > pass -> FAIL (ro-byt-n2820) Bug 95372 - [BAT BYT] Sporadic failure from igt/gem_exec_flush@basic-batch-kernel-default-cmd > Subgroup basic-uc-ro-default: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup basic-wb-ro-before-default: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Test gem_exec_store: > Subgroup basic-bsd: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Test gem_mmap_gtt: > Subgroup basic-short: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup basic-write-gtt: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Test gem_pwrite: > Subgroup basic: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Test kms_addfb_basic: > Subgroup addfb25-bad-modifier: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup addfb25-yf-tiled: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup bad-pitch-0: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup bad-pitch-1024: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Subgroup basic-x-tiled: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Subgroup basic-y-tiled: > pass -> DMESG-WARN (ro-skl-i7-6700hq) > Subgroup clobberred-modifier: > dmesg-warn -> PASS (ro-skl-i7-6700hq) > Subgroup tile-pitch-mismatch: > pass -> DMESG-WARN (ro-skl-i7-6700hq) All the warnings on 'ro-skl-i7-6700hq' appear to be https://bugs.freedesktop.org/show_bug.cgi?id=95632 "[BAT SKL] *ERROR* Potential atomic update failure on pipe A" So all issues accounted for, patchset ready for merge :) .Dave. > fi-bdw-i7-5557u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 > fi-hsw-i7-4770k total:209 pass:190 dwarn:0 dfail:0 fail:0 skip:19 > fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 skip:25 > fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 skip:39 > ro-bdw-i5-5250u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 > ro-bdw-i7-5600u total:102 pass:75 dwarn:0 dfail:0 fail:0 skip:26 > ro-bsw-n3050 total:209 pass:168 dwarn:0 dfail:0 fail:2 skip:39 > ro-byt-n2820 total:209 pass:168 dwarn:1 dfail:0 fail:3 skip:37 > ro-hsw-i3-4010u total:209 pass:186 dwarn:0 dfail:0 fail:0 skip:23 > ro-hsw-i7-4770r total:102 pass:82 dwarn:0 dfail:0 fail:0 skip:19 > ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 skip:57 > ro-skl-i7-6700hq total:204 pass:172 dwarn:11 dfail:0 fail:0 skip:21 > ro-snb-i7-2620M total:102 pass:72 dwarn:0 dfail:0 fail:0 skip:29 > fi-skl-i5-6260u failed to connect after reboot > ro-bdw-i7-5557U failed to connect after reboot > ro-ivb2-i7-3770 failed to connect after reboot > ro-ivb-i7-3770 failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1125/ > > 55d1291 drm-intel-nightly: 2016y-06m-06d-16h-28m-05s UTC integration manifest > 57871c9 drm/i915/guc: enable GuC loading & submission by default > 3fdd902 drm/i915/guc: disable GuC submission earlier during GuC (re)load > 5492d57 drm/i915/guc: fix GuC loading/submission check
On 07/06/16 11:54, Dave Gordon wrote: > On 07/06/16 09:43, Patchwork wrote: >> == Series Details == >> >> Series: series starting with [1/3] drm/i915/guc: fix GuC >> loading/submission check >> URL : https://patchwork.freedesktop.org/series/8380/ >> State : failure >> >> == Summary == >> >> Series 8380v1 Series without cover letter >> http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/1/mbox >> >> Test core_auth: >> Subgroup basic-auth: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Test drv_module_reload_basic: >> pass -> DMESG-WARN (ro-byt-n2820) > > Bug 93381 - [BAT BYT] dmesg WARNING in snd-hda > >> Test gem_exec_flush: >> Subgroup basic-batch-kernel-default-cmd: >> pass -> FAIL (ro-byt-n2820) > > Bug 95372 - [BAT BYT] Sporadic failure from > igt/gem_exec_flush@basic-batch-kernel-default-cmd > >> Subgroup basic-uc-ro-default: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup basic-wb-ro-before-default: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Test gem_exec_store: >> Subgroup basic-bsd: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Test gem_mmap_gtt: >> Subgroup basic-short: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup basic-write-gtt: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Test gem_pwrite: >> Subgroup basic: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Test kms_addfb_basic: >> Subgroup addfb25-bad-modifier: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup addfb25-yf-tiled: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup bad-pitch-0: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup bad-pitch-1024: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Subgroup basic-x-tiled: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Subgroup basic-y-tiled: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) >> Subgroup clobberred-modifier: >> dmesg-warn -> PASS (ro-skl-i7-6700hq) >> Subgroup tile-pitch-mismatch: >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > > All the warnings on 'ro-skl-i7-6700hq' appear to be > https://bugs.freedesktop.org/show_bug.cgi?id=95632 > "[BAT SKL] *ERROR* Potential atomic update failure on pipe A" > > So all issues accounted for, patchset ready for merge :) > > .Dave. > >> fi-bdw-i7-5557u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 >> fi-hsw-i7-4770k total:209 pass:190 dwarn:0 dfail:0 fail:0 >> skip:19 >> fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 >> skip:25 >> fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 >> skip:39 >> ro-bdw-i5-5250u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 >> ro-bdw-i7-5600u total:102 pass:75 dwarn:0 dfail:0 fail:0 >> skip:26 >> ro-bsw-n3050 total:209 pass:168 dwarn:0 dfail:0 fail:2 >> skip:39 >> ro-byt-n2820 total:209 pass:168 dwarn:1 dfail:0 fail:3 >> skip:37 >> ro-hsw-i3-4010u total:209 pass:186 dwarn:0 dfail:0 fail:0 >> skip:23 >> ro-hsw-i7-4770r total:102 pass:82 dwarn:0 dfail:0 fail:0 >> skip:19 >> ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 >> skip:57 >> ro-skl-i7-6700hq total:204 pass:172 dwarn:11 dfail:0 fail:0 >> skip:21 >> ro-snb-i7-2620M total:102 pass:72 dwarn:0 dfail:0 fail:0 >> skip:29 >> fi-skl-i5-6260u failed to connect after reboot >> ro-bdw-i7-5557U failed to connect after reboot >> ro-ivb2-i7-3770 failed to connect after reboot >> ro-ivb-i7-3770 failed to connect after reboot >> >> Results at /archive/results/CI_IGT_test/RO_Patchwork_1125/ >> >> 55d1291 drm-intel-nightly: 2016y-06m-06d-16h-28m-05s UTC integration >> manifest >> 57871c9 drm/i915/guc: enable GuC loading & submission by default >> 3fdd902 drm/i915/guc: disable GuC submission earlier during GuC (re)load >> 5492d57 drm/i915/guc: fix GuC loading/submission check Merged, thanks for the patches and review. Regards, Tvrtko
On Tue, Jun 07, 2016 at 02:23:34PM +0100, Tvrtko Ursulin wrote: > > On 07/06/16 11:54, Dave Gordon wrote: > >On 07/06/16 09:43, Patchwork wrote: > >>== Series Details == > >> > >>Series: series starting with [1/3] drm/i915/guc: fix GuC > >>loading/submission check > >>URL : https://patchwork.freedesktop.org/series/8380/ > >>State : failure > >> > >>== Summary == > >> > >>Series 8380v1 Series without cover letter > >>http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/1/mbox > >> > >>Test core_auth: > >> Subgroup basic-auth: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >>Test drv_module_reload_basic: > >> pass -> DMESG-WARN (ro-byt-n2820) > > > >Bug 93381 - [BAT BYT] dmesg WARNING in snd-hda > > > >>Test gem_exec_flush: > >> Subgroup basic-batch-kernel-default-cmd: > >> pass -> FAIL (ro-byt-n2820) > > > >Bug 95372 - [BAT BYT] Sporadic failure from > >igt/gem_exec_flush@basic-batch-kernel-default-cmd > > > >> Subgroup basic-uc-ro-default: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup basic-wb-ro-before-default: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >>Test gem_exec_store: > >> Subgroup basic-bsd: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >>Test gem_mmap_gtt: > >> Subgroup basic-short: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup basic-write-gtt: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >>Test gem_pwrite: > >> Subgroup basic: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >>Test kms_addfb_basic: > >> Subgroup addfb25-bad-modifier: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup addfb25-yf-tiled: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup bad-pitch-0: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup bad-pitch-1024: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >> Subgroup basic-x-tiled: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >> Subgroup basic-y-tiled: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > >> Subgroup clobberred-modifier: > >> dmesg-warn -> PASS (ro-skl-i7-6700hq) > >> Subgroup tile-pitch-mismatch: > >> pass -> DMESG-WARN (ro-skl-i7-6700hq) > > > >All the warnings on 'ro-skl-i7-6700hq' appear to be > >https://bugs.freedesktop.org/show_bug.cgi?id=95632 > >"[BAT SKL] *ERROR* Potential atomic update failure on pipe A" > > > >So all issues accounted for, patchset ready for merge :) > > > >.Dave. > > > >>fi-bdw-i7-5557u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 > >>fi-hsw-i7-4770k total:209 pass:190 dwarn:0 dfail:0 fail:0 > >>skip:19 > >>fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 > >>skip:25 > >>fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 > >>skip:39 > >>ro-bdw-i5-5250u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 > >>ro-bdw-i7-5600u total:102 pass:75 dwarn:0 dfail:0 fail:0 > >>skip:26 > >>ro-bsw-n3050 total:209 pass:168 dwarn:0 dfail:0 fail:2 > >>skip:39 > >>ro-byt-n2820 total:209 pass:168 dwarn:1 dfail:0 fail:3 > >>skip:37 > >>ro-hsw-i3-4010u total:209 pass:186 dwarn:0 dfail:0 fail:0 > >>skip:23 > >>ro-hsw-i7-4770r total:102 pass:82 dwarn:0 dfail:0 fail:0 > >>skip:19 > >>ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 > >>skip:57 > >>ro-skl-i7-6700hq total:204 pass:172 dwarn:11 dfail:0 fail:0 > >>skip:21 > >>ro-snb-i7-2620M total:102 pass:72 dwarn:0 dfail:0 fail:0 > >>skip:29 > >>fi-skl-i5-6260u failed to connect after reboot > >>ro-bdw-i7-5557U failed to connect after reboot > >>ro-ivb2-i7-3770 failed to connect after reboot > >>ro-ivb-i7-3770 failed to connect after reboot > >> > >>Results at /archive/results/CI_IGT_test/RO_Patchwork_1125/ > >> > >>55d1291 drm-intel-nightly: 2016y-06m-06d-16h-28m-05s UTC integration > >>manifest > >>57871c9 drm/i915/guc: enable GuC loading & submission by default > >>3fdd902 drm/i915/guc: disable GuC submission earlier during GuC (re)load > >>5492d57 drm/i915/guc: fix GuC loading/submission check > > Merged, thanks for the patches and review. What review? This patch has not addressed my concerns at all. It is changes the default submission and makes Skylake slower for what benefit? The changelog still doesn't explain why we would want to take the risk and current regressions. -Chris
On 07/06/16 21:00, Chris Wilson wrote: > On Tue, Jun 07, 2016 at 02:23:34PM +0100, Tvrtko Ursulin wrote: >> >> On 07/06/16 11:54, Dave Gordon wrote: >>> On 07/06/16 09:43, Patchwork wrote: >>>> == Series Details == >>>> >>>> Series: series starting with [1/3] drm/i915/guc: fix GuC >>>> loading/submission check >>>> URL : https://patchwork.freedesktop.org/series/8380/ >>>> State : failure >>>> >>>> == Summary == >>>> >>>> Series 8380v1 Series without cover letter >>>> http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/1/mbox >>>> >>>> Test core_auth: >>>> Subgroup basic-auth: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Test drv_module_reload_basic: >>>> pass -> DMESG-WARN (ro-byt-n2820) >>> >>> Bug 93381 - [BAT BYT] dmesg WARNING in snd-hda >>> >>>> Test gem_exec_flush: >>>> Subgroup basic-batch-kernel-default-cmd: >>>> pass -> FAIL (ro-byt-n2820) >>> >>> Bug 95372 - [BAT BYT] Sporadic failure from >>> igt/gem_exec_flush@basic-batch-kernel-default-cmd >>> >>>> Subgroup basic-uc-ro-default: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup basic-wb-ro-before-default: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Test gem_exec_store: >>>> Subgroup basic-bsd: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Test gem_mmap_gtt: >>>> Subgroup basic-short: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup basic-write-gtt: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Test gem_pwrite: >>>> Subgroup basic: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Test kms_addfb_basic: >>>> Subgroup addfb25-bad-modifier: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup addfb25-yf-tiled: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup bad-pitch-0: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup bad-pitch-1024: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Subgroup basic-x-tiled: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Subgroup basic-y-tiled: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>>> Subgroup clobberred-modifier: >>>> dmesg-warn -> PASS (ro-skl-i7-6700hq) >>>> Subgroup tile-pitch-mismatch: >>>> pass -> DMESG-WARN (ro-skl-i7-6700hq) >>> >>> All the warnings on 'ro-skl-i7-6700hq' appear to be >>> https://bugs.freedesktop.org/show_bug.cgi?id=95632 >>> "[BAT SKL] *ERROR* Potential atomic update failure on pipe A" >>> >>> So all issues accounted for, patchset ready for merge :) >>> >>> .Dave. >>> >>>> fi-bdw-i7-5557u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 >>>> fi-hsw-i7-4770k total:209 pass:190 dwarn:0 dfail:0 fail:0 >>>> skip:19 >>>> fi-skl-i7-6700k total:209 pass:184 dwarn:0 dfail:0 fail:0 >>>> skip:25 >>>> fi-snb-i7-2600 total:209 pass:170 dwarn:0 dfail:0 fail:0 >>>> skip:39 >>>> ro-bdw-i5-5250u total:102 pass:93 dwarn:0 dfail:0 fail:0 skip:8 >>>> ro-bdw-i7-5600u total:102 pass:75 dwarn:0 dfail:0 fail:0 >>>> skip:26 >>>> ro-bsw-n3050 total:209 pass:168 dwarn:0 dfail:0 fail:2 >>>> skip:39 >>>> ro-byt-n2820 total:209 pass:168 dwarn:1 dfail:0 fail:3 >>>> skip:37 >>>> ro-hsw-i3-4010u total:209 pass:186 dwarn:0 dfail:0 fail:0 >>>> skip:23 >>>> ro-hsw-i7-4770r total:102 pass:82 dwarn:0 dfail:0 fail:0 >>>> skip:19 >>>> ro-ilk1-i5-650 total:204 pass:146 dwarn:0 dfail:0 fail:1 >>>> skip:57 >>>> ro-skl-i7-6700hq total:204 pass:172 dwarn:11 dfail:0 fail:0 >>>> skip:21 >>>> ro-snb-i7-2620M total:102 pass:72 dwarn:0 dfail:0 fail:0 >>>> skip:29 >>>> fi-skl-i5-6260u failed to connect after reboot >>>> ro-bdw-i7-5557U failed to connect after reboot >>>> ro-ivb2-i7-3770 failed to connect after reboot >>>> ro-ivb-i7-3770 failed to connect after reboot >>>> >>>> Results at /archive/results/CI_IGT_test/RO_Patchwork_1125/ >>>> >>>> 55d1291 drm-intel-nightly: 2016y-06m-06d-16h-28m-05s UTC integration >>>> manifest >>>> 57871c9 drm/i915/guc: enable GuC loading & submission by default >>>> 3fdd902 drm/i915/guc: disable GuC submission earlier during GuC (re)load >>>> 5492d57 drm/i915/guc: fix GuC loading/submission check >> >> Merged, thanks for the patches and review. > > What review? This patch has not addressed my concerns at all. > > It is changes the default submission and makes Skylake slower for what > benefit? The changelog still doesn't explain why we would want to take > the risk and current regressions. > -Chris *"It's Intel POR"* That means people higher up the foodchain have decided that the GuC is the future and the minions have just got to get on with it. Of course, if it introduces real regressions we'll turn it off again until they're resolved, but we're more likely to discover any remaining issues with it on by default -- otherwise it's just not getting much exposure beyond the in-house validation. Let's just hope they're all exposed and resolved before September! .Dave.
On 07/06/16 09:41, Tvrtko Ursulin wrote: > > On 07/06/16 09:14, Dave Gordon wrote: >> The last stage of the GuC loader also sanitises the GuC submission >> settings, so should be called unconditionally (even on platforms >> without a GuC) to ensure consistent settings; in particular, this >> prevents any attempt to use GuC submission on GuCless platforms! >> >> Also fix error path handling and clarify DRM_INFO fallback message. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 8 +++----- >> drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++++---- >> 2 files changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index 1bfc260..eae8d7a 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -4930,11 +4930,9 @@ int i915_gem_init_engines(struct drm_device *dev) >> intel_mocs_init_l3cc_table(dev); >> >> /* We can't enable contexts until all firmware is loaded */ >> - if (HAS_GUC(dev)) { >> - ret = intel_guc_setup(dev); >> - if (ret) >> - goto out; >> - } >> + ret = intel_guc_setup(dev); >> + if (ret) >> + goto out; >> >> /* >> * Increment the next seqno by 0x100 so we have a visible break >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >> b/drivers/gpu/drm/i915/intel_guc_loader.c >> index f2b88c7..4e34c2e 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >> @@ -425,9 +425,13 @@ int intel_guc_setup(struct drm_device *dev) >> if (!i915.enable_guc_loading) { >> err = 0; >> goto fail; >> - } else if (fw_path == NULL || *fw_path == '\0') { >> - if (*fw_path == '\0') > > Ops. I can only assume I meant !fw_path. > >> - DRM_INFO("No GuC firmware known for this platform\n"); >> + } else if (fw_path == NULL) { >> + /* Device is known to have no uCode (e.g. no GuC) */ >> + err = -ENXIO; >> + goto fail; >> + } else if (*fw_path == '\0') { >> + /* Device has a GuC but we don't know what f/w to load? */ >> + DRM_INFO("No GuC firmware known for this platform\n"); >> err = -ENODEV; >> goto fail; >> } >> @@ -535,7 +539,7 @@ int intel_guc_setup(struct drm_device *dev) >> if (fw_path == NULL) >> DRM_INFO("GuC submission without firmware not >> supported\n"); >> if (ret == 0) >> - DRM_INFO("Falling back to execlist mode\n"); >> + DRM_INFO("Falling back from GuC submission to execlist >> mode\n"); >> else >> DRM_ERROR("GuC init failed: %d\n", ret); >> } >> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Bah now on BDW we get on boot: [drm:gen8_init_common_ring] Execlists enabled for render ring [drm:gen8_init_common_ring] Execlists enabled for blitter ring [drm:gen8_init_common_ring] Execlists enabled for bsd ring [drm:gen8_init_common_ring] Execlists enabled for video enhancement ring [drm:intel_guc_setup] GuC fw status: path (null), fetch NONE, load NONE [drm] GuC firmware load skipped [drm] GuC submission without firmware not supported [drm] Falling back from GuC submission to execlist mode Which is a bit untidy. :( Regards, Tvrtko
On 09/06/16 12:04, Tvrtko Ursulin wrote: > > On 07/06/16 09:41, Tvrtko Ursulin wrote: >> >> On 07/06/16 09:14, Dave Gordon wrote: >>> The last stage of the GuC loader also sanitises the GuC submission >>> settings, so should be called unconditionally (even on platforms >>> without a GuC) to ensure consistent settings; in particular, this >>> prevents any attempt to use GuC submission on GuCless platforms! >>> >>> Also fix error path handling and clarify DRM_INFO fallback message. >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 8 +++----- >>> drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++++---- >>> 2 files changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c >>> b/drivers/gpu/drm/i915/i915_gem.c >>> index 1bfc260..eae8d7a 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -4930,11 +4930,9 @@ int i915_gem_init_engines(struct drm_device *dev) >>> intel_mocs_init_l3cc_table(dev); >>> >>> /* We can't enable contexts until all firmware is loaded */ >>> - if (HAS_GUC(dev)) { >>> - ret = intel_guc_setup(dev); >>> - if (ret) >>> - goto out; >>> - } >>> + ret = intel_guc_setup(dev); >>> + if (ret) >>> + goto out; >>> >>> /* >>> * Increment the next seqno by 0x100 so we have a visible break >>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >>> b/drivers/gpu/drm/i915/intel_guc_loader.c >>> index f2b88c7..4e34c2e 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c >>> @@ -425,9 +425,13 @@ int intel_guc_setup(struct drm_device *dev) >>> if (!i915.enable_guc_loading) { >>> err = 0; >>> goto fail; >>> - } else if (fw_path == NULL || *fw_path == '\0') { >>> - if (*fw_path == '\0') >> >> Ops. I can only assume I meant !fw_path. >> >>> - DRM_INFO("No GuC firmware known for this platform\n"); >>> + } else if (fw_path == NULL) { >>> + /* Device is known to have no uCode (e.g. no GuC) */ >>> + err = -ENXIO; >>> + goto fail; >>> + } else if (*fw_path == '\0') { >>> + /* Device has a GuC but we don't know what f/w to load? */ >>> + DRM_INFO("No GuC firmware known for this platform\n"); >>> err = -ENODEV; >>> goto fail; >>> } >>> @@ -535,7 +539,7 @@ int intel_guc_setup(struct drm_device *dev) >>> if (fw_path == NULL) >>> DRM_INFO("GuC submission without firmware not >>> supported\n"); >>> if (ret == 0) >>> - DRM_INFO("Falling back to execlist mode\n"); >>> + DRM_INFO("Falling back from GuC submission to execlist >>> mode\n"); >>> else >>> DRM_ERROR("GuC init failed: %d\n", ret); >>> } >>> >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Bah now on BDW we get on boot: > > [drm:gen8_init_common_ring] Execlists enabled for render ring > [drm:gen8_init_common_ring] Execlists enabled for blitter ring > [drm:gen8_init_common_ring] Execlists enabled for bsd ring > [drm:gen8_init_common_ring] Execlists enabled for video enhancement ring > [drm:intel_guc_setup] GuC fw status: path (null), fetch NONE, load NONE > [drm] GuC firmware load skipped > [drm] GuC submission without firmware not supported > [drm] Falling back from GuC submission to execlist mode > > Which is a bit untidy. :( > > Regards, > Tvrtko You shouldn't get the second or third [drm] message, if you haven't overridden the default values. The default for enable_guc_submission is (-1) which gets mapped to HAS_GUC_SCHED() which is 0 on BDW; you only get the extra messages if you've set it to a non-default value. .Dave.
On 10/06/16 17:59, Patchwork wrote: > == Series Details == > > Series: series starting with drm/i915/guc: suppress GuC-related message on non-GuC platforms (rev2) > URL : https://patchwork.freedesktop.org/series/8380/ > State : failure > > == Summary == > > Series 8380v2 Series without cover letter > http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/2/mbox > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-cmd: > pass -> FAIL (ro-byt-n2820) Bug 95372 - [BAT BYT] Sporadic failure from igt/gem_exec_flush@basic-batch-kernel-default-cmd > Test kms_pipe_crc_basic: > Subgroup nonblocking-crc-pipe-c-frame-sequence: > fail -> PASS (ro-ivb-i7-3770) > Subgroup suspend-read-crc-pipe-a: > dmesg-warn -> SKIP (ro-bdw-i7-5557U) > Subgroup suspend-read-crc-pipe-b: > dmesg-warn -> SKIP (ro-bdw-i7-5557U) > Subgroup suspend-read-crc-pipe-c: > skip -> DMESG-WARN (ro-bdw-i5-5250u) Looks like https://bugs.freedesktop.org/show_bug.cgi?id=94605#c17 as do the two that have gone from WARN to SKIP. Connector problem? > ro-bdw-i5-5250u total:213 pass:197 dwarn:4 dfail:0 fail:0 skip:12 > ro-bdw-i7-5557U total:213 pass:197 dwarn:1 dfail:0 fail:0 skip:15 > ro-bdw-i7-5600u total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28 > ro-bsw-n3050 total:213 pass:171 dwarn:1 dfail:0 fail:2 skip:39 > ro-byt-n2820 total:213 pass:173 dwarn:0 dfail:0 fail:3 skip:37 > ro-hsw-i3-4010u total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23 > ro-hsw-i7-4770r total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23 > ro-ilk-i7-620lm total:213 pass:150 dwarn:0 dfail:0 fail:1 skip:62 > ro-ilk1-i5-650 total:208 pass:150 dwarn:0 dfail:0 fail:1 skip:57 > ro-ivb-i7-3770 total:213 pass:181 dwarn:0 dfail:0 fail:0 skip:32 > ro-ivb2-i7-3770 total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28 > ro-skl3-i5-6260u total:213 pass:201 dwarn:1 dfail:0 fail:0 skip:11 > ro-snb-i7-2620M total:213 pass:174 dwarn:0 dfail:0 fail:1 skip:38 > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1160/ > > 5de1a00 drm-intel-nightly: 2016y-06m-10d-15h-26m-55s UTC integration manifest > 8f7efa4 drm/i915/guc: suppress GuC-related message on non-GuC platforms >
On 10/06/16 19:14, Dave Gordon wrote: > On 10/06/16 17:59, Patchwork wrote: >> == Series Details == >> >> Series: series starting with drm/i915/guc: suppress GuC-related >> message on non-GuC platforms (rev2) >> URL : https://patchwork.freedesktop.org/series/8380/ >> State : failure >> >> == Summary == >> >> Series 8380v2 Series without cover letter >> http://patchwork.freedesktop.org/api/1.0/series/8380/revisions/2/mbox >> >> Test gem_exec_flush: >> Subgroup basic-batch-kernel-default-cmd: >> pass -> FAIL (ro-byt-n2820) > > Bug 95372 - [BAT BYT] Sporadic failure from > igt/gem_exec_flush@basic-batch-kernel-default-cmd > >> Test kms_pipe_crc_basic: >> Subgroup nonblocking-crc-pipe-c-frame-sequence: >> fail -> PASS (ro-ivb-i7-3770) >> Subgroup suspend-read-crc-pipe-a: >> dmesg-warn -> SKIP (ro-bdw-i7-5557U) >> Subgroup suspend-read-crc-pipe-b: >> dmesg-warn -> SKIP (ro-bdw-i7-5557U) >> Subgroup suspend-read-crc-pipe-c: >> skip -> DMESG-WARN (ro-bdw-i5-5250u) > > Looks like https://bugs.freedesktop.org/show_bug.cgi?id=94605#c17 > as do the two that have gone from WARN to SKIP. Connector problem? > >> ro-bdw-i5-5250u total:213 pass:197 dwarn:4 dfail:0 fail:0 >> skip:12 >> ro-bdw-i7-5557U total:213 pass:197 dwarn:1 dfail:0 fail:0 >> skip:15 >> ro-bdw-i7-5600u total:213 pass:185 dwarn:0 dfail:0 fail:0 >> skip:28 >> ro-bsw-n3050 total:213 pass:171 dwarn:1 dfail:0 fail:2 >> skip:39 >> ro-byt-n2820 total:213 pass:173 dwarn:0 dfail:0 fail:3 >> skip:37 >> ro-hsw-i3-4010u total:213 pass:190 dwarn:0 dfail:0 fail:0 >> skip:23 >> ro-hsw-i7-4770r total:213 pass:190 dwarn:0 dfail:0 fail:0 >> skip:23 >> ro-ilk-i7-620lm total:213 pass:150 dwarn:0 dfail:0 fail:1 >> skip:62 >> ro-ilk1-i5-650 total:208 pass:150 dwarn:0 dfail:0 fail:1 >> skip:57 >> ro-ivb-i7-3770 total:213 pass:181 dwarn:0 dfail:0 fail:0 >> skip:32 >> ro-ivb2-i7-3770 total:213 pass:185 dwarn:0 dfail:0 fail:0 >> skip:28 >> ro-skl3-i5-6260u total:213 pass:201 dwarn:1 dfail:0 fail:0 >> skip:11 >> ro-snb-i7-2620M total:213 pass:174 dwarn:0 dfail:0 fail:1 >> skip:38 >> >> Results at /archive/results/CI_IGT_test/RO_Patchwork_1160/ >> >> 5de1a00 drm-intel-nightly: 2016y-06m-10d-15h-26m-55s UTC integration >> manifest >> 8f7efa4 drm/i915/guc: suppress GuC-related message on non-GuC platforms Merged, thanks for the patch and review! :) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1bfc260..eae8d7a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4930,11 +4930,9 @@ int i915_gem_init_engines(struct drm_device *dev) intel_mocs_init_l3cc_table(dev); /* We can't enable contexts until all firmware is loaded */ - if (HAS_GUC(dev)) { - ret = intel_guc_setup(dev); - if (ret) - goto out; - } + ret = intel_guc_setup(dev); + if (ret) + goto out; /* * Increment the next seqno by 0x100 so we have a visible break diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index f2b88c7..4e34c2e 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -425,9 +425,13 @@ int intel_guc_setup(struct drm_device *dev) if (!i915.enable_guc_loading) { err = 0; goto fail; - } else if (fw_path == NULL || *fw_path == '\0') { - if (*fw_path == '\0') - DRM_INFO("No GuC firmware known for this platform\n"); + } else if (fw_path == NULL) { + /* Device is known to have no uCode (e.g. no GuC) */ + err = -ENXIO; + goto fail; + } else if (*fw_path == '\0') { + /* Device has a GuC but we don't know what f/w to load? */ + DRM_INFO("No GuC firmware known for this platform\n"); err = -ENODEV; goto fail; } @@ -535,7 +539,7 @@ int intel_guc_setup(struct drm_device *dev) if (fw_path == NULL) DRM_INFO("GuC submission without firmware not supported\n"); if (ret == 0) - DRM_INFO("Falling back to execlist mode\n"); + DRM_INFO("Falling back from GuC submission to execlist mode\n"); else DRM_ERROR("GuC init failed: %d\n", ret); }
The last stage of the GuC loader also sanitises the GuC submission settings, so should be called unconditionally (even on platforms without a GuC) to ensure consistent settings; in particular, this prevents any attempt to use GuC submission on GuCless platforms! Also fix error path handling and clarify DRM_INFO fallback message. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 8 +++----- drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++++---- 2 files changed, 11 insertions(+), 9 deletions(-)