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

[Tvrtko Ursulin]

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 at 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 at 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