Re: [PATCH v1] drm/xe/pm: Wait for lmem ready in resume

["Poosa, Karthik" <karthik.poosa@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4209 bytes --]


On 09-05-2025 21:26, Rodrigo Vivi wrote:
> On Thu, May 08, 2025 at 10:47:59AM -0500, Lucas De Marchi wrote:
>> On Thu, May 08, 2025 at 11:28:29AM +0530, Poosa, Karthik wrote:
>>> On 22-04-2025 21:30, Lucas De Marchi wrote:
>>>> On Thu, Apr 10, 2025 at 04:30:57PM +0530, Karthik Poosa wrote:
>>>>> Add wait for LMEM ready during system and runtime resume.
>>>>> This wait is there in probe and is missing during resume.
>>>>>
>>>>> Signed-off-by: Karthik Poosa<karthik.poosa@intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/xe/xe_device.c | 2 +-
>>>>> drivers/gpu/drm/xe/xe_device.h | 1 +
>>>>> drivers/gpu/drm/xe/xe_pm.c     | 8 ++++++++
>>>>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>>>>> b/drivers/gpu/drm/xe/xe_device.c
>>>>> index 75e753e0a682..4c0d9eb51d1f 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>>>> @@ -630,7 +630,7 @@ static bool verify_lmem_ready(struct xe_device *xe)
>>>>>      return !!val;
>>>>> }
>>>>>
>>>>> -static int wait_for_lmem_ready(struct xe_device *xe)
>>>>> +int wait_for_lmem_ready(struct xe_device *xe)
>>>>> {
>>>>>      unsigned long timeout, start;
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_device.h
>>>>> b/drivers/gpu/drm/xe/xe_device.h
>>>>> index 0bc3bc8e6803..60bc92f9ab22 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_device.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_device.h
>>>>> @@ -47,6 +47,7 @@ int xe_device_probe_early(struct xe_device *xe);
>>>>> int xe_device_probe(struct xe_device *xe);
>>>>> void xe_device_remove(struct xe_device *xe);
>>>>> void xe_device_shutdown(struct xe_device *xe);
>>>>> +int wait_for_lmem_ready(struct xe_device *xe);
>>>> if you are exporting the function, please follow the naming:
>>>> xe_device_*.
>>>>
>>>> However, I'm not sure this is really needed, particularly on pm runtime
>>>> resume. At the very least it needs better explanation in the commit
>>>> message. Do we have any known failures this is supposedly to fix?
>>>>
>>>> Lucas De Marchi
>>>>
>>> While there are no current issues without this check, it's a prudent
>>> measure to prevent any future problems.
>> no, not without proper justification. I see no reason to wait for it in
>> pm runtime resume. We shouldn't sprinkle sleeps in the driver to be
>> prudent.
> We need this on D3Cold -> D0. If power went off, during the wake-up, memory
> links needs to be retrained and this is an indication that everything went
> well on memory bring up and that we are able to access it from the driver.
>
> That said, I believe the code should be changed here to only have this if
> d3cold was allowed to start with, and not in every runtime resume.

In runtime resume this function is already under, under d3cold.allowed flag

         if (xe->d3cold.allowed) {

                 err = xe_pcode_ready(xe, true);
                 if (err)
                         goto out;

                 err = xe_device_wait_for_vram_ready(xe);
                 if (err)
                         goto out;        }

We shall add the check in system resume also.

>> Lucas De Marchi
>>
>>>>> void xe_device_wmb(struct xe_device *xe);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>>>> index 4e112fbacada..2e59670660c1 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>>>> @@ -182,6 +182,10 @@ int xe_pm_resume(struct xe_device *xe)
>>>>>      if (err)
>>>>>          return err;
>>>>>
>>>>> +    err = wait_for_lmem_ready(xe);
>>>>> +    if (err)
>>>>> +        goto err;
>>>>> +
>>>>>      xe_display_pm_resume_early(xe);
>>>>>
>>>>>      /*
>>>>> @@ -478,6 +482,10 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>>>>          if (err)
>>>>>              goto out;
>>>>>
>>>>> +        err = wait_for_lmem_ready(xe);
>>>>> +        if (err)
>>>>> +            goto out;
>>>>> +
>>>>>          xe_display_pm_resume_early(xe);
>>>>>
>>>>>          /*
>>>>> -- 
>>>>> 2.25.1
>>>>>

[-- Attachment #2: Type: text/html, Size: 6302 bytes --]