diff mbox series

drm/i915/mtl: Wake GT before sending H2G message

Message ID 20240118231728.3817168-1-vinay.belgaumkar@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/mtl: Wake GT before sending H2G message | expand

Commit Message

Vinay Belgaumkar Jan. 18, 2024, 11:17 p.m. UTC
Instead of waiting until the interrupt reaches GuC, we can grab a
forcewake while triggering the H2G interrupt. GEN11_GUC_HOST_INTERRUPT
is inside an "always on" domain with respect to RC6. However, there
could be some delays when platform is entering/exiting some higher
level platform sleep states and a H2G is triggered. A forcewake
ensures those sleep states have been fully exited and further
processing occurs as expected.

This will have an official WA soon so adding a FIXME in the comments.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Matt Roper Jan. 18, 2024, 11:50 p.m. UTC | #1
On Thu, Jan 18, 2024 at 03:17:28PM -0800, Vinay Belgaumkar wrote:
> Instead of waiting until the interrupt reaches GuC, we can grab a
> forcewake while triggering the H2G interrupt. GEN11_GUC_HOST_INTERRUPT
> is inside an "always on" domain with respect to RC6. However, there

A bit of a nitpick, but technically "always on" is a description of GT
register ranges that never get powered down.  GEN11_GUC_HOST_INTERRUPT
isn't inside the GT at all, but rather is an sgunit register and thus
isn't affected by forcewake.  This is just a special case where the
sgunit register forwards a message back to the GT's GuC, and the
workaround wants us to make sure the GT is awake before that message
gets there.

> could be some delays when platform is entering/exiting some higher
> level platform sleep states and a H2G is triggered. A forcewake
> ensures those sleep states have been fully exited and further
> processing occurs as expected.

Based on this description, is adding implicit forcewake to this register
really enough?  Implicit forcewake powers up before a read/write, but
also allows it to power back down as soon as the MMIO operation is
complete.  If the GuC is a bit slow to notice the interrupt, then we
could wind up with a sequence like

 - Driver grabs forcewake and GT powers up
 - Driver writes 0x1901f0 to trigger GuC interrupt
 - Driver releases forcewake and GT powers down
 - GuC notices interrupt (or maybe fails to notice it because the GT
   powered down before it had a chance to process it?)

which I'm guessing isn't actually going to satisfy this workaround.  Do
we actually need to keep the GT awake not just through the register
operation, but also through the GuC's processing of the interrupt?  If
so, then we probably want to do an explicit forcewake get/put to ensure
the hardware stays powered up long enough.


Matt

> 
> This will have an official WA soon so adding a FIXME in the comments.
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index dfefad5a5fec..121458a31886 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1800,7 +1800,10 @@ static const struct intel_forcewake_range __mtl_fw_ranges[] = {
>  	GEN_FW_RANGE(0x24000, 0x2ffff, 0), /*
>  		0x24000 - 0x2407f: always on
>  		0x24080 - 0x2ffff: reserved */
> -	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT)
> +	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT),
> +	GEN_FW_RANGE(0x40000, 0x1901ec, 0),
> +	GEN_FW_RANGE(0x1901f0, 0x1901f0, FORCEWAKE_GT)
> +		/* FIXME: WA to wake GT while triggering H2G */
>  };
>  
>  /*
> -- 
> 2.38.1
>
Vinay Belgaumkar Jan. 19, 2024, 1:21 a.m. UTC | #2
On 1/18/2024 3:50 PM, Matt Roper wrote:
> On Thu, Jan 18, 2024 at 03:17:28PM -0800, Vinay Belgaumkar wrote:
>> Instead of waiting until the interrupt reaches GuC, we can grab a
>> forcewake while triggering the H2G interrupt. GEN11_GUC_HOST_INTERRUPT
>> is inside an "always on" domain with respect to RC6. However, there
> A bit of a nitpick, but technically "always on" is a description of GT
> register ranges that never get powered down.  GEN11_GUC_HOST_INTERRUPT
> isn't inside the GT at all, but rather is an sgunit register and thus
> isn't affected by forcewake.  This is just a special case where the
> sgunit register forwards a message back to the GT's GuC, and the
> workaround wants us to make sure the GT is awake before that message
> gets there.
True, can modify the description to reflect this.
>
>> could be some delays when platform is entering/exiting some higher
>> level platform sleep states and a H2G is triggered. A forcewake
>> ensures those sleep states have been fully exited and further
>> processing occurs as expected.
> Based on this description, is adding implicit forcewake to this register
> really enough?  Implicit forcewake powers up before a read/write, but
> also allows it to power back down as soon as the MMIO operation is
> complete.  If the GuC is a bit slow to notice the interrupt, then we
> could wind up with a sequence like
>
>   - Driver grabs forcewake and GT powers up
>   - Driver writes 0x1901f0 to trigger GuC interrupt
>   - Driver releases forcewake and GT powers down
>   - GuC notices interrupt (or maybe fails to notice it because the GT
>     powered down before it had a chance to process it?)
>
> which I'm guessing isn't actually going to satisfy this workaround.  Do
> we actually need to keep the GT awake not just through the register
> operation, but also through the GuC's processing of the interrupt?  If
> so, then we probably want to do an explicit forcewake get/put to ensure
> the hardware stays powered up long enough.

The issue being addressed here is not GT entering C6, but the higher 
platform sleep states. Once we force wake GT while writing to the H2G 
register, that should bring us out of sleep. After clearing the 
forcewake (which would happen after the write for 0x1901f0 goes 
through), we still have C6 hysteresis and the hysteresis counters for 
the higher platform sleep states which should give GuC enough time to 
process the interrupt before we enter C6 and then subsequently these 
higher sleep states.

Thanks,

Vinay.

>
>
> Matt
>
>> This will have an official WA soon so adding a FIXME in the comments.
>>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index dfefad5a5fec..121458a31886 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1800,7 +1800,10 @@ static const struct intel_forcewake_range __mtl_fw_ranges[] = {
>>   	GEN_FW_RANGE(0x24000, 0x2ffff, 0), /*
>>   		0x24000 - 0x2407f: always on
>>   		0x24080 - 0x2ffff: reserved */
>> -	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT)
>> +	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT),
>> +	GEN_FW_RANGE(0x40000, 0x1901ec, 0),
>> +	GEN_FW_RANGE(0x1901f0, 0x1901f0, FORCEWAKE_GT)
>> +		/* FIXME: WA to wake GT while triggering H2G */
>>   };
>>   
>>   /*
>> -- 
>> 2.38.1
>>
Matt Roper Jan. 19, 2024, 1:39 a.m. UTC | #3
On Thu, Jan 18, 2024 at 05:21:23PM -0800, Belgaumkar, Vinay wrote:
> 
> On 1/18/2024 3:50 PM, Matt Roper wrote:
> > On Thu, Jan 18, 2024 at 03:17:28PM -0800, Vinay Belgaumkar wrote:
> > > Instead of waiting until the interrupt reaches GuC, we can grab a
> > > forcewake while triggering the H2G interrupt. GEN11_GUC_HOST_INTERRUPT
> > > is inside an "always on" domain with respect to RC6. However, there
> > A bit of a nitpick, but technically "always on" is a description of GT
> > register ranges that never get powered down.  GEN11_GUC_HOST_INTERRUPT
> > isn't inside the GT at all, but rather is an sgunit register and thus
> > isn't affected by forcewake.  This is just a special case where the
> > sgunit register forwards a message back to the GT's GuC, and the
> > workaround wants us to make sure the GT is awake before that message
> > gets there.
> True, can modify the description to reflect this.
> > 
> > > could be some delays when platform is entering/exiting some higher
> > > level platform sleep states and a H2G is triggered. A forcewake
> > > ensures those sleep states have been fully exited and further
> > > processing occurs as expected.
> > Based on this description, is adding implicit forcewake to this register
> > really enough?  Implicit forcewake powers up before a read/write, but
> > also allows it to power back down as soon as the MMIO operation is
> > complete.  If the GuC is a bit slow to notice the interrupt, then we
> > could wind up with a sequence like
> > 
> >   - Driver grabs forcewake and GT powers up
> >   - Driver writes 0x1901f0 to trigger GuC interrupt
> >   - Driver releases forcewake and GT powers down
> >   - GuC notices interrupt (or maybe fails to notice it because the GT
> >     powered down before it had a chance to process it?)
> > 
> > which I'm guessing isn't actually going to satisfy this workaround.  Do
> > we actually need to keep the GT awake not just through the register
> > operation, but also through the GuC's processing of the interrupt?  If
> > so, then we probably want to do an explicit forcewake get/put to ensure
> > the hardware stays powered up long enough.
> 
> The issue being addressed here is not GT entering C6, but the higher
> platform sleep states. Once we force wake GT while writing to the H2G
> register, that should bring us out of sleep. After clearing the forcewake
> (which would happen after the write for 0x1901f0 goes through), we still
> have C6 hysteresis and the hysteresis counters for the higher platform sleep
> states which should give GuC enough time to process the interrupt before we
> enter C6 and then subsequently these higher sleep states.

Okay, makes sense.  Hopefully the finalize the workaround details and
documentation soon, but this looks reasonable with the information we
have at the moment.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> 
> Thanks,
> 
> Vinay.
> 
> > 
> > 
> > Matt
> > 
> > > This will have an official WA soon so adding a FIXME in the comments.
> > > 
> > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index dfefad5a5fec..121458a31886 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1800,7 +1800,10 @@ static const struct intel_forcewake_range __mtl_fw_ranges[] = {
> > >   	GEN_FW_RANGE(0x24000, 0x2ffff, 0), /*
> > >   		0x24000 - 0x2407f: always on
> > >   		0x24080 - 0x2ffff: reserved */
> > > -	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT)
> > > +	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT),
> > > +	GEN_FW_RANGE(0x40000, 0x1901ec, 0),
> > > +	GEN_FW_RANGE(0x1901f0, 0x1901f0, FORCEWAKE_GT)
> > > +		/* FIXME: WA to wake GT while triggering H2G */
> > >   };
> > >   /*
> > > -- 
> > > 2.38.1
> > >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index dfefad5a5fec..121458a31886 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1800,7 +1800,10 @@  static const struct intel_forcewake_range __mtl_fw_ranges[] = {
 	GEN_FW_RANGE(0x24000, 0x2ffff, 0), /*
 		0x24000 - 0x2407f: always on
 		0x24080 - 0x2ffff: reserved */
-	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT)
+	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_GT),
+	GEN_FW_RANGE(0x40000, 0x1901ec, 0),
+	GEN_FW_RANGE(0x1901f0, 0x1901f0, FORCEWAKE_GT)
+		/* FIXME: WA to wake GT while triggering H2G */
 };
 
 /*