diff mbox

[v2,2/2] drm/i915: resize the GuC WOPCM for rc6

Message ID 1453399861-5557-3-git-send-email-peter.antoine@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Antoine Jan. 21, 2016, 6:11 p.m. UTC 
This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
spaces do not overlap.

Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
 drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

jeff.mcgee@intel.com Jan. 21, 2016, 9:41 p.m. UTC | #1 
On Thu, Jan 21, 2016 at 06:11:01PM +0000, Peter Antoine wrote:
> This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
> spaces do not overlap.
> 
> Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 685c799..cb938b0 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -58,7 +58,8 @@
>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>  
>  #define GUC_WOPCM_SIZE			_MMIO(0xc050)
> -#define   GUC_WOPCM_SIZE_VALUE  	  (0x80 << 12)	/* 512KB */
> +#define   GUC_WOPCM_SIZE_VALUE		(0x80 << 12)	/* 512KB */
> +#define   BXT_GUC_WOPCM_SIZE_VALUE	(0x70 << 12)	/* 448KB */
>  
>  /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>  #define	GUC_WOPCM_TOP			(GUC_WOPCM_SIZE_VALUE)
Should GUC_WOPCM_TOP be dynamically assigned the proper value, or is it
sufficient to leave at the max possible WOPCM size? If the later, might be
worth a comment.
-Jeff

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 8182d11..69c85d1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -304,7 +304,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	/* init WOPCM */
> -	I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
> +	if (IS_BROXTON(dev))
> +		I915_WRITE(GUC_WOPCM_SIZE, BXT_GUC_WOPCM_SIZE_VALUE);
> +	else
> +		I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
> +
>  	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
>  
>  	/* Enable MIA caching. GuC clock gating is disabled. */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Peter Antoine Jan. 22, 2016, 9:45 a.m. UTC | #2 
Reply inline.

On Thu, 21 Jan 2016, Jeff McGee wrote:

> On Thu, Jan 21, 2016 at 06:11:01PM +0000, Peter Antoine wrote:
>> This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
>> spaces do not overlap.
>>
>> Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
>>  drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index 685c799..cb938b0 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -58,7 +58,8 @@
>>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>>
>>  #define GUC_WOPCM_SIZE			_MMIO(0xc050)
>> -#define   GUC_WOPCM_SIZE_VALUE  	  (0x80 << 12)	/* 512KB */
>> +#define   GUC_WOPCM_SIZE_VALUE		(0x80 << 12)	/* 512KB */
>> +#define   BXT_GUC_WOPCM_SIZE_VALUE	(0x70 << 12)	/* 448KB */
>>
>>  /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>>  #define	GUC_WOPCM_TOP			(GUC_WOPCM_SIZE_VALUE)
> Should GUC_WOPCM_TOP be dynamically assigned the proper value, or is it
> sufficient to leave at the max possible WOPCM size? If the later, might be
> worth a comment.
> -Jeff
I'll send a follow up patch to add a comment this.

The whole area needs to be not mapped, as it is still accessed from the 
GuC.

-Peter.

>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 8182d11..69c85d1 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -304,7 +304,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>
>>  	/* init WOPCM */
>> -	I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
>> +	if (IS_BROXTON(dev))
>> +		I915_WRITE(GUC_WOPCM_SIZE, BXT_GUC_WOPCM_SIZE_VALUE);
>> +	else
>> +		I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
>> +
>>  	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
>>
>>  	/* Enable MIA caching. GuC clock gating is disabled. */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47
Dave Gordon Feb. 3, 2016, 3:39 p.m. UTC | #3 
On 21/01/16 21:41, Jeff McGee wrote:
> On Thu, Jan 21, 2016 at 06:11:01PM +0000, Peter Antoine wrote:
>> This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
>> spaces do not overlap.
>>
>> Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
>>   drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index 685c799..cb938b0 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -58,7 +58,8 @@
>>   #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>>
>>   #define GUC_WOPCM_SIZE			_MMIO(0xc050)
>> -#define   GUC_WOPCM_SIZE_VALUE  	  (0x80 << 12)	/* 512KB */
>> +#define   GUC_WOPCM_SIZE_VALUE		(0x80 << 12)	/* 512KB */
>> +#define   BXT_GUC_WOPCM_SIZE_VALUE	(0x70 << 12)	/* 448KB */
>>
>>   /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>>   #define	GUC_WOPCM_TOP			(GUC_WOPCM_SIZE_VALUE)
> Should GUC_WOPCM_TOP be dynamically assigned the proper value, or is it
> sufficient to leave at the max possible WOPCM size? If the later, might be
> worth a comment.
> -Jeff

This isn't the right interpretation of these values.

GUC_WOPCM_TOP is the value defining the top of the GTT address range NOT 
available to the GuC and hence where GuC-accessible objects must NOT be 
placed.

GUC_WOPCM_SIZE_VALUE is the value written to the GUC_WOPCM_SIZE 
register, defining how much WOPCM space CAN be used.

The former is an architectural constant (512K); the latter is a 
software-defined boundary between areas of memory within that range that 
are used for different purposes.

Therefore, GUC_WOPCM_TOP must NOT be defined in terms of 
GUC_WOPCM_SIZE_VALUE, but GUC_WOPCM_SIZE_VALUE could be defined in terms 
of GUC_WOPCM_TOP, in particular as (GUC_WOPCM_TOP-reserved).

.Dave.
Rodrigo Vivi April 5, 2016, 4:27 p.m. UTC | #4 
Hi Peter,

This patch is required for the BXT firmware loading.  (Maybe/Probably
something similar for KBL is also required)
Do you have plans to fix this interpretation as Dave pointed and send
a new version?

Thanks,
Rodrigo.

On Wed, Feb 3, 2016 at 7:39 AM, Dave Gordon <david.s.gordon@intel.com> wrote:
> On 21/01/16 21:41, Jeff McGee wrote:
>>
>> On Thu, Jan 21, 2016 at 06:11:01PM +0000, Peter Antoine wrote:
>>>
>>> This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
>>> spaces do not overlap.
>>>
>>> Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
>>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
>>>   drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
>>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>>> index 685c799..cb938b0 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>>> @@ -58,7 +58,8 @@
>>>   #define GUC_MAX_IDLE_COUNT            _MMIO(0xC3E4)
>>>
>>>   #define GUC_WOPCM_SIZE                        _MMIO(0xc050)
>>> -#define   GUC_WOPCM_SIZE_VALUE           (0x80 << 12)  /* 512KB */
>>> +#define   GUC_WOPCM_SIZE_VALUE         (0x80 << 12)    /* 512KB */
>>> +#define   BXT_GUC_WOPCM_SIZE_VALUE     (0x70 << 12)    /* 448KB */
>>>
>>>   /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>>>   #define       GUC_WOPCM_TOP                   (GUC_WOPCM_SIZE_VALUE)
>>
>> Should GUC_WOPCM_TOP be dynamically assigned the proper value, or is it
>> sufficient to leave at the max possible WOPCM size? If the later, might be
>> worth a comment.
>> -Jeff
>
>
> This isn't the right interpretation of these values.
>
> GUC_WOPCM_TOP is the value defining the top of the GTT address range NOT
> available to the GuC and hence where GuC-accessible objects must NOT be
> placed.
>
> GUC_WOPCM_SIZE_VALUE is the value written to the GUC_WOPCM_SIZE register,
> defining how much WOPCM space CAN be used.
>
> The former is an architectural constant (512K); the latter is a
> software-defined boundary between areas of memory within that range that are
> used for different purposes.
>
> Therefore, GUC_WOPCM_TOP must NOT be defined in terms of
> GUC_WOPCM_SIZE_VALUE, but GUC_WOPCM_SIZE_VALUE could be defined in terms of
> GUC_WOPCM_TOP, in particular as (GUC_WOPCM_TOP-reserved).
>
> .Dave.
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Peter Antoine April 6, 2016, 8:32 a.m. UTC | #5 
Ok,

The value written to GUC_WOPCM_SIZE is done on i915-gem.c@6474:
 	/* init WOPCM */
 	if (IS_BROXTON(dev))
 		I915_WRITE(GUC_WOPCM_SIZE, BXT_GUC_WOPCM_SIZE_VALUE);
 	else
 		I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);

So I think it is correct on BXT. The definition is confusing as the space 
on BXT is smaller so that the RC6 data can be written to the WOPCM.

I don't think there is anything to do.

Peter.

On Tue, 5 Apr 2016, Rodrigo Vivi wrote:

> Hi Peter,
>
> This patch is required for the BXT firmware loading.  (Maybe/Probably
> something similar for KBL is also required)
> Do you have plans to fix this interpretation as Dave pointed and send
> a new version?
>
> Thanks,
> Rodrigo.
>
> On Wed, Feb 3, 2016 at 7:39 AM, Dave Gordon <david.s.gordon@intel.com> wrote:
>> On 21/01/16 21:41, Jeff McGee wrote:
>>>
>>> On Thu, Jan 21, 2016 at 06:11:01PM +0000, Peter Antoine wrote:
>>>>
>>>> This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory
>>>> spaces do not overlap.
>>>>
>>>> Issue: https://jira01.devtools.intel.com/browse/VIZ-6638
>>>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_guc_reg.h     | 3 ++-
>>>>   drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++-
>>>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>>>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>>>> index 685c799..cb938b0 100644
>>>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>>>> @@ -58,7 +58,8 @@
>>>>   #define GUC_MAX_IDLE_COUNT            _MMIO(0xC3E4)
>>>>
>>>>   #define GUC_WOPCM_SIZE                        _MMIO(0xc050)
>>>> -#define   GUC_WOPCM_SIZE_VALUE           (0x80 << 12)  /* 512KB */
>>>> +#define   GUC_WOPCM_SIZE_VALUE         (0x80 << 12)    /* 512KB */
>>>> +#define   BXT_GUC_WOPCM_SIZE_VALUE     (0x70 << 12)    /* 448KB */
>>>>
>>>>   /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>>>>   #define       GUC_WOPCM_TOP                   (GUC_WOPCM_SIZE_VALUE)
>>>
>>> Should GUC_WOPCM_TOP be dynamically assigned the proper value, or is it
>>> sufficient to leave at the max possible WOPCM size? If the later, might be
>>> worth a comment.
>>> -Jeff
>>
>>
>> This isn't the right interpretation of these values.
>>
>> GUC_WOPCM_TOP is the value defining the top of the GTT address range NOT
>> available to the GuC and hence where GuC-accessible objects must NOT be
>> placed.
>>
>> GUC_WOPCM_SIZE_VALUE is the value written to the GUC_WOPCM_SIZE register,
>> defining how much WOPCM space CAN be used.
>>
>> The former is an architectural constant (512K); the latter is a
>> software-defined boundary between areas of memory within that range that are
>> used for different purposes.
>>
>> Therefore, GUC_WOPCM_TOP must NOT be defined in terms of
>> GUC_WOPCM_SIZE_VALUE, but GUC_WOPCM_SIZE_VALUE could be defined in terms of
>> GUC_WOPCM_TOP, in particular as (GUC_WOPCM_TOP-reserved).
>>
>> .Dave.
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
>

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index 685c799..cb938b0 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -58,7 +58,8 @@ 
 #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
 
 #define GUC_WOPCM_SIZE			_MMIO(0xc050)
-#define   GUC_WOPCM_SIZE_VALUE  	  (0x80 << 12)	/* 512KB */
+#define   GUC_WOPCM_SIZE_VALUE		(0x80 << 12)	/* 512KB */
+#define   BXT_GUC_WOPCM_SIZE_VALUE	(0x70 << 12)	/* 448KB */
 
 /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
 #define	GUC_WOPCM_TOP			(GUC_WOPCM_SIZE_VALUE)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 8182d11..69c85d1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -304,7 +304,11 @@  static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	/* init WOPCM */
-	I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
+	if (IS_BROXTON(dev))
+		I915_WRITE(GUC_WOPCM_SIZE, BXT_GUC_WOPCM_SIZE_VALUE);
+	else
+		I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE);
+
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
 
 	/* Enable MIA caching. GuC clock gating is disabled. */