Re: [PATCH] drm/xe: Restore pci state upon resume

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Den 2024-09-12 kl. 23:40, skrev Lucas De Marchi:
> On Thu, Sep 12, 2024 at 05:20:40PM GMT, Rodrigo Vivi wrote:
>> On Thu, Sep 12, 2024 at 03:11:12PM -0500, Lucas De Marchi wrote:
>>> On Thu, Sep 12, 2024 at 03:05:30PM GMT, Rodrigo Vivi wrote:
>>> > The pci state was saved, but not restored. Restore
>>> > right after the power state transition request like
>>> > every other driver.
>>> >
>>> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
>>>
>>> oh, no
>>>
>>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> > ---
>>> > drivers/gpu/drm/xe/xe_pci.c | 2 ++
>>> > 1 file changed, 2 insertions(+)
>>> >
>>> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> > index 5ba4ec229494..6d29ef4b396f 100644
>>> > --- a/drivers/gpu/drm/xe/xe_pci.c
>>> > +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> > @@ -949,6 +949,8 @@ static int xe_pci_resume(struct device *dev)
>>> >     if (err)
>>> >         return err;
>>> >
>>> > +    pci_restore_state(pdev);
>>> > +
>>>
>>> something bad happened... I was sure this line was here before. Indeed.
>>> git log -L:xe_pci_resume:drivers/gpu/drm/xe/xe_pci.c to the rescue
>>>
>>> See commit f6761c68c0ac ("drm/xe/display: Improve s2idle handling.")
>>
>> hmmm...
>> Perhaps
>>
>> Fixes: f6761c68c0ac ("drm/xe/display: Improve s2idle handling.")
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>
>> then?!
> 
> yes, reading that patch I don't understand why it got removed.
> 
> Lucas De Marchi
Likely a conflict from rebasing, sorry for the noise!
> 
>>
>> The restoration shouldn't have anything to do with that case.
>> and it pairs with pci_save_state(pdev) is still there...
>>
>> It is not in i915, but I have an i915 patch to add the restore
>> there as well.
>>
>> Also, I might as well in a follow up bring back the
>> pci_set_power_state(pdev, PCI_D3hot) to Xe.
>> This is probably the right way, aligns with all other drivers
>> and i915. So that shouldn't be the cause of the power drainage
>> on suspend anyway, or i915 would be also draining it.
>>
>>>
>>> Lucas De Marchi
>>>
>>> >     err = pci_enable_device(pdev);
>>> >     if (err)
>>> >         return err;
>>> > --
>>> > 2.46.0
>>> >