drm/i915: Use true PPGTT in Gen8 if execlists are enabled

Submitted by Michel Thierry on Dec. 11, 2014, 12:07 p.m.

Details

Message ID 1418299638-16927-1-git-send-email-michel.thierry@intel.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry Dec. 11, 2014, 12:07 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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