Re: [Intel-gfx] [PATCH 1/2] drm/i915/gt: Don't do display work on platforms without display

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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(&gt->rc6);
>>> 	intel_rps_unpark(&gt->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