| Message ID | e59e680be0d56fdb4c116f45fac94350832752ec.1665458639.git.ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Firm up gt park/unpark | expand |
On Mon, 10 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > Do display work only on platforms with display. This avoids holding the > runtime PM wakeref for an additional 100+ ms after GT has been parked. > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 36 +++++++++++++++------------ > 1 file changed, 20 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index f553e2173bdad..26aa2e979a148 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -70,19 +70,21 @@ static int __gt_unpark(struct intel_wakeref *wf) > > GT_TRACE(gt, "\n"); > > - /* > - * It seems that the DMC likes to transition between the DC states a lot > - * when there are no connected displays (no active power domains) during > - * command submission. > - * > - * This activity has negative impact on the performance of the chip with > - * huge latencies observed in the interrupt handler and elsewhere. > - * > - * Work around it by grabbing a GT IRQ power domain whilst there is any > - * GT activity, preventing any DC state transitions. > - */ > - gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); > - GEM_BUG_ON(!gt->awake); > + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { Feels like something's wrong if you need both of those. BR, Jani. > + /* > + * It seems that the DMC likes to transition between the DC states a lot > + * when there are no connected displays (no active power domains) during > + * command submission. > + * > + * This activity has negative impact on the performance of the chip with > + * huge latencies observed in the interrupt handler and elsewhere. > + * > + * Work around it by grabbing a GT IRQ power domain whilst there is any > + * GT activity, preventing any DC state transitions. > + */ > + gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); > + GEM_BUG_ON(!gt->awake); > + } > > intel_rc6_unpark(>->rc6); > intel_rps_unpark(>->rps); > @@ -115,9 +117,11 @@ static int __gt_park(struct intel_wakeref *wf) > /* Everything switched off, flush any residual interrupt just in case */ > intel_synchronize_irq(i915); > > - /* Defer dropping the display power well for 100ms, it's slow! */ > - GEM_BUG_ON(!wakeref); > - intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); > + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { > + /* Defer dropping the display power well for 100ms, it's slow! */ > + GEM_BUG_ON(!wakeref); > + intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); > + } > > return 0; > }
On Tue, 11 Oct 2022 00:22:34 -0700, Jani Nikula wrote: > Hi Jani, > On Mon, 10 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > > Do display work only on platforms with display. This avoids holding the > > runtime PM wakeref for an additional 100+ ms after GT has been parked. > > > > Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025 > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 36 +++++++++++++++------------ > > 1 file changed, 20 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > index f553e2173bdad..26aa2e979a148 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > @@ -70,19 +70,21 @@ static int __gt_unpark(struct intel_wakeref *wf) > > > > GT_TRACE(gt, "\n"); > > > > - /* > > - * It seems that the DMC likes to transition between the DC states a lot > > - * when there are no connected displays (no active power domains) during > > - * command submission. > > - * > > - * This activity has negative impact on the performance of the chip with > > - * huge latencies observed in the interrupt handler and elsewhere. > > - * > > - * Work around it by grabbing a GT IRQ power domain whilst there is any > > - * GT activity, preventing any DC state transitions. > > - */ > > - gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); > > - GEM_BUG_ON(!gt->awake); > > + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { > > Feels like something's wrong if you need both of those. Don't think so: /* Only valid when HAS_DISPLAY() is true */ #define INTEL_DISPLAY_ENABLED(dev_priv) \ (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), \ !(dev_priv)->params.disable_display && \ !intel_opregion_headless_sku(dev_priv)) Maybe inside display code INTEL_DISPLAY_ENABLED is sufficient since code paths have previously invoked HAS_DISPLAY, but not in non-display code. Thanks. -- Ashutosh > > + /* > > + * It seems that the DMC likes to transition between the DC states a lot > > + * when there are no connected displays (no active power domains) during > > + * command submission. > > + * > > + * This activity has negative impact on the performance of the chip with > > + * huge latencies observed in the interrupt handler and elsewhere. > > + * > > + * Work around it by grabbing a GT IRQ power domain whilst there is any > > + * GT activity, preventing any DC state transitions. > > + */ > > + gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); > > + GEM_BUG_ON(!gt->awake); > > + } > > > > intel_rc6_unpark(>->rc6); > > intel_rps_unpark(>->rps); > > @@ -115,9 +117,11 @@ static int __gt_park(struct intel_wakeref *wf) > > /* Everything switched off, flush any residual interrupt just in case */ > > intel_synchronize_irq(i915); > > > > - /* Defer dropping the display power well for 100ms, it's slow! */ > > - GEM_BUG_ON(!wakeref); > > - intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); > > + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { > > + /* Defer dropping the display power well for 100ms, it's slow! */ > > + GEM_BUG_ON(!wakeref); > > + intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); > > + } > > > > return 0; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center
On 11/10/2022 08:34, Dixit, Ashutosh wrote: > On Tue, 11 Oct 2022 00:22:34 -0700, Jani Nikula wrote: >> > > Hi Jani, > >> On Mon, 10 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: >>> Do display work only on platforms with display. This avoids holding the >>> runtime PM wakeref for an additional 100+ ms after GT has been parked. >>> >>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025 >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 36 +++++++++++++++------------ >>> 1 file changed, 20 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>> index f553e2173bdad..26aa2e979a148 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>> @@ -70,19 +70,21 @@ static int __gt_unpark(struct intel_wakeref *wf) >>> >>> GT_TRACE(gt, "\n"); >>> >>> - /* >>> - * It seems that the DMC likes to transition between the DC states a lot >>> - * when there are no connected displays (no active power domains) during >>> - * command submission. >>> - * >>> - * This activity has negative impact on the performance of the chip with >>> - * huge latencies observed in the interrupt handler and elsewhere. >>> - * >>> - * Work around it by grabbing a GT IRQ power domain whilst there is any >>> - * GT activity, preventing any DC state transitions. >>> - */ >>> - gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); >>> - GEM_BUG_ON(!gt->awake); >>> + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { >> >> Feels like something's wrong if you need both of those. > > Don't think so: > > /* Only valid when HAS_DISPLAY() is true */ > #define INTEL_DISPLAY_ENABLED(dev_priv) \ > (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), \ > !(dev_priv)->params.disable_display && \ > !intel_opregion_headless_sku(dev_priv)) > > Maybe inside display code INTEL_DISPLAY_ENABLED is sufficient since code > paths have previously invoked HAS_DISPLAY, but not in non-display code. AFAIR this workaround is only needed when DMC is loaded so I wonder if we could detect that instead? Although then I am not sure why we haven't done it like that from the start. Maybe there was a good reason and I just can't remember it. Looking at the history, b68763741aa2 ("drm/i915: Restore GT performance in headless mode with DMC loaded") which added the workaround did not add the 100ms delay. That was added later in 81ff52b70577 ("drm/i915/gt: Ratelimit display power w/a"). That part sounds like it makes sense - if there is cost in these transitions usual approach is too add some hysteresis. (And AFAIR in this particular case it was actually a matter or re-adding the hysteresis which was lost once GEM idle work handler approach was removed.) So I guess the main question is can we guard this with (ideally something better than) HAS_DMC. Perhaps back then GPUs wo/ display were simply not in our minds? Or obtaining the "DC off" power well was perhaps a no-op in it's own right when there is no display? If that was the case and isn't any more would that be feasible to re-add? Regards, Tvrtko > > Thanks. > -- > Ashutosh > >>> + /* >>> + * It seems that the DMC likes to transition between the DC states a lot >>> + * when there are no connected displays (no active power domains) during >>> + * command submission. >>> + * >>> + * This activity has negative impact on the performance of the chip with >>> + * huge latencies observed in the interrupt handler and elsewhere. >>> + * >>> + * Work around it by grabbing a GT IRQ power domain whilst there is any >>> + * GT activity, preventing any DC state transitions. >>> + */ >>> + gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); >>> + GEM_BUG_ON(!gt->awake); >>> + } >>> >>> intel_rc6_unpark(>->rc6); >>> intel_rps_unpark(>->rps); >>> @@ -115,9 +117,11 @@ static int __gt_park(struct intel_wakeref *wf) >>> /* Everything switched off, flush any residual interrupt just in case */ >>> intel_synchronize_irq(i915); >>> >>> - /* Defer dropping the display power well for 100ms, it's slow! */ >>> - GEM_BUG_ON(!wakeref); >>> - intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); >>> + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { >>> + /* Defer dropping the display power well for 100ms, it's slow! */ >>> + GEM_BUG_ON(!wakeref); >>> + intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); >>> + } >>> >>> return 0; >>> } >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On 11/10/2022 09:30, Tvrtko Ursulin wrote: > > On 11/10/2022 08:34, Dixit, Ashutosh wrote: >> On Tue, 11 Oct 2022 00:22:34 -0700, Jani Nikula wrote: >>> >> >> Hi Jani, >> >>> On Mon, 10 Oct 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: >>>> Do display work only on platforms with display. This avoids holding the >>>> runtime PM wakeref for an additional 100+ ms after GT has been parked. >>>> >>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025 >>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 36 >>>> +++++++++++++++------------ >>>> 1 file changed, 20 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>>> b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>>> index f553e2173bdad..26aa2e979a148 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >>>> @@ -70,19 +70,21 @@ static int __gt_unpark(struct intel_wakeref *wf) >>>> >>>> GT_TRACE(gt, "\n"); >>>> >>>> - /* >>>> - * It seems that the DMC likes to transition between the DC >>>> states a lot >>>> - * when there are no connected displays (no active power >>>> domains) during >>>> - * command submission. >>>> - * >>>> - * This activity has negative impact on the performance of the >>>> chip with >>>> - * huge latencies observed in the interrupt handler and elsewhere. >>>> - * >>>> - * Work around it by grabbing a GT IRQ power domain whilst >>>> there is any >>>> - * GT activity, preventing any DC state transitions. >>>> - */ >>>> - gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); >>>> - GEM_BUG_ON(!gt->awake); >>>> + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { >>> >>> Feels like something's wrong if you need both of those. >> >> Don't think so: >> >> /* Only valid when HAS_DISPLAY() is true */ >> #define INTEL_DISPLAY_ENABLED(dev_priv) \ >> (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), \ >> !(dev_priv)->params.disable_display >> && \ >> !intel_opregion_headless_sku(dev_priv)) >> >> Maybe inside display code INTEL_DISPLAY_ENABLED is sufficient since code >> paths have previously invoked HAS_DISPLAY, but not in non-display code. > > AFAIR this workaround is only needed when DMC is loaded so I wonder if > we could detect that instead? > > Although then I am not sure why we haven't done it like that from the > start. Maybe there was a good reason and I just can't remember it. > > Looking at the history, b68763741aa2 ("drm/i915: Restore GT performance > in headless mode with DMC loaded") which added the workaround did not > add the 100ms delay. That was added later in 81ff52b70577 ("drm/i915/gt: > Ratelimit display power w/a"). That part sounds like it makes sense - if > there is cost in these transitions usual approach is too add some > hysteresis. (And AFAIR in this particular case it was actually a matter > or re-adding the hysteresis which was lost once GEM idle work handler > approach was removed.) > > So I guess the main question is can we guard this with (ideally > something better than) HAS_DMC. Perhaps back then GPUs wo/ display were > simply not in our minds? Or obtaining the "DC off" power well was > perhaps a no-op in it's own right when there is no display? If that was > the case and isn't any more would that be feasible to re-add? Oops or not - we still need a rpm reference in the current scheme, display or no display! Back in the day that rpm was separate and explicit from this wa... So I guess this code stays as is and only possible improvements can be in the PMU area. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index f553e2173bdad..26aa2e979a148 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -70,19 +70,21 @@ static int __gt_unpark(struct intel_wakeref *wf) GT_TRACE(gt, "\n"); - /* - * It seems that the DMC likes to transition between the DC states a lot - * when there are no connected displays (no active power domains) during - * command submission. - * - * This activity has negative impact on the performance of the chip with - * huge latencies observed in the interrupt handler and elsewhere. - * - * Work around it by grabbing a GT IRQ power domain whilst there is any - * GT activity, preventing any DC state transitions. - */ - gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); - GEM_BUG_ON(!gt->awake); + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { + /* + * It seems that the DMC likes to transition between the DC states a lot + * when there are no connected displays (no active power domains) during + * command submission. + * + * This activity has negative impact on the performance of the chip with + * huge latencies observed in the interrupt handler and elsewhere. + * + * Work around it by grabbing a GT IRQ power domain whilst there is any + * GT activity, preventing any DC state transitions. + */ + gt->awake = intel_display_power_get(i915, POWER_DOMAIN_GT_IRQ); + GEM_BUG_ON(!gt->awake); + } intel_rc6_unpark(>->rc6); intel_rps_unpark(>->rps); @@ -115,9 +117,11 @@ static int __gt_park(struct intel_wakeref *wf) /* Everything switched off, flush any residual interrupt just in case */ intel_synchronize_irq(i915); - /* Defer dropping the display power well for 100ms, it's slow! */ - GEM_BUG_ON(!wakeref); - intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); + if (HAS_DISPLAY(i915) && INTEL_DISPLAY_ENABLED(i915)) { + /* Defer dropping the display power well for 100ms, it's slow! */ + GEM_BUG_ON(!wakeref); + intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); + } return 0; }
Do display work only on platforms with display. This avoids holding the runtime PM wakeref for an additional 100+ ms after GT has been parked. Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 36 +++++++++++++++------------ 1 file changed, 20 insertions(+), 16 deletions(-)