| Message ID | 1418299638-16927-1-git-send-email-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +1-1 364/366 364/366
SNB 448/450 448/450
IVB 497/498 497/498
BYT 289/289 289/289
HSW 563/564 563/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt_gem_fenced_exec_thrash_no-spare-fences-busy PASS(2, M26M37) DMESG_WARN(1, M37)
ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(7, M26)PASS(23, M26M37) PASS(1, M37)
Note: You need to pay more attention to line start with '*'
On Thu, Dec 11, 2014 at 12:07:18PM +0000, Michel Thierry wrote: > When execlists submission is enabled, try full ppgtt by default. > > Note, this patch considers that execlist support has been enabled by > default on Gen8. > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 171f6ea..4ed3904 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > > has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; > has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; > - if (IS_GEN8(dev)) > - has_full_ppgtt = false; /* XXX why? */ > + if (IS_GEN8(dev) && !i915.enable_execlists) > + has_full_ppgtt = false; /* Only enforce with execlists */ Imo this has outlived it's usefulness - enable_ppgtt is an unsafe parameter so everyone setting it themselves gets what they need. Afair this was just because of the execlist depency on gen8 for ppgtt. -Daniel > > /* > * We don't allow disabling PPGTT for gen9+ as it's a requirement for > @@ -72,7 +72,10 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > return 0; > } > > - return has_aliasing_ppgtt ? 1 : 0; > + if (INTEL_INFO(dev)->gen < 8) > + return has_aliasing_ppgtt ? 1 : 0; > + else > + return has_full_ppgtt ? 2 : has_aliasing_ppgtt ? 1 : 0; > } > > > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/15/2014 10:11 AM, Daniel Vetter wrote: > On Thu, Dec 11, 2014 at 12:07:18PM +0000, Michel Thierry wrote: >> When execlists submission is enabled, try full ppgtt by default. >> >> Note, this patch considers that execlist support has been enabled by >> default on Gen8. >> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 171f6ea..4ed3904 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) >> >> has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; >> has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; >> - if (IS_GEN8(dev)) >> - has_full_ppgtt = false; /* XXX why? */ >> + if (IS_GEN8(dev) && !i915.enable_execlists) >> + has_full_ppgtt = false; /* Only enforce with execlists */ > Imo this has outlived it's usefulness - enable_ppgtt is an unsafe > parameter so everyone setting it themselves gets what they need. > Afair this was just because of the execlist depency on gen8 for ppgtt. > -Daniel Not sure if I'm following you up on this... The aim was to change the default value to _full_ only when execlists are also enable (after Thomas' patch). In gen8, we don't want to have full ppgtt with legacy ring submission.
On Mon, Dec 15, 2014 at 12:47:14PM +0000, Michel Thierry wrote: > On 12/15/2014 10:11 AM, Daniel Vetter wrote: > >On Thu, Dec 11, 2014 at 12:07:18PM +0000, Michel Thierry wrote: > >>When execlists submission is enabled, try full ppgtt by default. > >> > >>Note, this patch considers that execlist support has been enabled by > >>default on Gen8. > >> > >>Signed-off-by: Michel Thierry <michel.thierry@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>index 171f6ea..4ed3904 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>@@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > >> has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; > >> has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; > >>- if (IS_GEN8(dev)) > >>- has_full_ppgtt = false; /* XXX why? */ > >>+ if (IS_GEN8(dev) && !i915.enable_execlists) > >>+ has_full_ppgtt = false; /* Only enforce with execlists */ > >Imo this has outlived it's usefulness - enable_ppgtt is an unsafe > >parameter so everyone setting it themselves gets what they need. > >Afair this was just because of the execlist depency on gen8 for ppgtt. > >-Daniel > Not sure if I'm following you up on this... > The aim was to change the default value to _full_ only when execlists are > also enable (after Thomas' patch). In gen8, we don't want to have full > ppgtt with legacy ring submission. Yeah I gotten confused. The comment is a bit misleading thought, maybe "Full ppgtt needs execlist since otherwise the ctx switch can hang"? -Daniel
On Mon, Dec 15, 2014 at 03:22:08PM +0100, Daniel Vetter wrote: > On Mon, Dec 15, 2014 at 12:47:14PM +0000, Michel Thierry wrote: > > On 12/15/2014 10:11 AM, Daniel Vetter wrote: > > >On Thu, Dec 11, 2014 at 12:07:18PM +0000, Michel Thierry wrote: > > >>When execlists submission is enabled, try full ppgtt by default. > > >> > > >>Note, this patch considers that execlist support has been enabled by > > >>default on Gen8. > > >> > > >>Signed-off-by: Michel Thierry <michel.thierry@intel.com> > > >>--- > > >> drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- > > >> 1 file changed, 6 insertions(+), 3 deletions(-) > > >> > > >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>index 171f6ea..4ed3904 100644 > > >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>@@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > > >> has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; > > >> has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; > > >>- if (IS_GEN8(dev)) > > >>- has_full_ppgtt = false; /* XXX why? */ > > >>+ if (IS_GEN8(dev) && !i915.enable_execlists) > > >>+ has_full_ppgtt = false; /* Only enforce with execlists */ > > >Imo this has outlived it's usefulness - enable_ppgtt is an unsafe > > >parameter so everyone setting it themselves gets what they need. > > >Afair this was just because of the execlist depency on gen8 for ppgtt. > > >-Daniel > > Not sure if I'm following you up on this... > > The aim was to change the default value to _full_ only when execlists are > > also enable (after Thomas' patch). In gen8, we don't want to have full > > ppgtt with legacy ring submission. > > Yeah I gotten confused. The comment is a bit misleading thought, maybe > "Full ppgtt needs execlist since otherwise the ctx switch can hang"? Actually I wasnt' confused: gen8+ has full ppgtt, like gen7. It's broken without execlist though, so what we should do is just change the default below. Otherwise if a user explicitly sets execlist=0 and ppgtt=2 we won't obey that, which doesn't make sense - for isolating bugs this might be a valid testcase (even when we know there's problems). So your patch should do two things: - Generally allow full ppgtt on gen8 - the current restriction doesn't make sense. - Switch the default to full ppgtt when execlist is enabled on gen8+, leave the default to aliasing/disabled everywhere else as is. -Daniel
On 12/15/2014 2:28 PM, Daniel Vetter wrote: > On Mon, Dec 15, 2014 at 03:22:08PM +0100, Daniel Vetter wrote: >> On Mon, Dec 15, 2014 at 12:47:14PM +0000, Michel Thierry wrote: >>> On 12/15/2014 10:11 AM, Daniel Vetter wrote: >>>> On Thu, Dec 11, 2014 at 12:07:18PM +0000, Michel Thierry wrote: >>>>> When execlists submission is enabled, try full ppgtt by default. >>>>> >>>>> Note, this patch considers that execlist support has been enabled by >>>>> default on Gen8. >>>>> >>>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- >>>>> 1 file changed, 6 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>> index 171f6ea..4ed3904 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >>>>> @@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) >>>>> has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; >>>>> has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; >>>>> - if (IS_GEN8(dev)) >>>>> - has_full_ppgtt = false; /* XXX why? */ >>>>> + if (IS_GEN8(dev) && !i915.enable_execlists) >>>>> + has_full_ppgtt = false; /* Only enforce with execlists */ >>>> Imo this has outlived it's usefulness - enable_ppgtt is an unsafe >>>> parameter so everyone setting it themselves gets what they need. >>>> Afair this was just because of the execlist depency on gen8 for ppgtt. >>>> -Daniel >>> Not sure if I'm following you up on this... >>> The aim was to change the default value to _full_ only when execlists are >>> also enable (after Thomas' patch). In gen8, we don't want to have full >>> ppgtt with legacy ring submission. >> Yeah I gotten confused. The comment is a bit misleading thought, maybe >> "Full ppgtt needs execlist since otherwise the ctx switch can hang"? > Actually I wasnt' confused: gen8+ has full ppgtt, like gen7. It's broken > without execlist though, so what we should do is just change the default > below. Otherwise if a user explicitly sets execlist=0 and ppgtt=2 we won't > obey that, which doesn't make sense - for isolating bugs this might be a > valid testcase (even when we know there's problems). So your patch should > do two things: > - Generally allow full ppgtt on gen8 - the current restriction doesn't > make sense. > - Switch the default to full ppgtt when execlist is enabled on gen8+, leave > the default to aliasing/disabled everywhere else as is. > -Daniel Thanks for clarifying. I'll send a new patch. -Michel
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 171f6ea..4ed3904 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -40,8 +40,8 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6; has_full_ppgtt = INTEL_INFO(dev)->gen >= 7; - if (IS_GEN8(dev)) - has_full_ppgtt = false; /* XXX why? */ + if (IS_GEN8(dev) && !i915.enable_execlists) + has_full_ppgtt = false; /* Only enforce with execlists */ /* * We don't allow disabling PPGTT for gen9+ as it's a requirement for @@ -72,7 +72,10 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) return 0; } - return has_aliasing_ppgtt ? 1 : 0; + if (INTEL_INFO(dev)->gen < 8) + return has_aliasing_ppgtt ? 1 : 0; + else + return has_full_ppgtt ? 2 : has_aliasing_ppgtt ? 1 : 0; }
When execlists submission is enabled, try full ppgtt by default. Note, this patch considers that execlist support has been enabled by default on Gen8. Signed-off-by: Michel Thierry <michel.thierry@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)