| 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 |
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 >
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 >>
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 --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 */ }; /*
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(-)