Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region

[Lazar, Lijo]

On 8/19/2021 5:29 AM, Kees Cook wrote:

On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote:


On 8/18/2021 11:34 AM, Kees Cook wrote:

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.

Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.

"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.

Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Hawking Zhang 
Cc: Feifei Xu 
Cc: Lijo Lazar 
Cc: Likun Gao 
Cc: Jiawei Gu 
Cc: Evan Quan 
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook 
Acked-by: Alex Deucher 
Link: 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3Dreserved=0
---
   drivers/gpu/drm/amd/include/atomfirmware.h   |  9 -
   .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h|  3 ++-
   drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h  |  3 ++-
   .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h   |  3 ++-


Hi Kees,


Hi! Thanks for looking into this.


The headers which define these structs are firmware/VBIOS interfaces and are
picked directly from those components. There are difficulties in grouping
them to structs at the original source as that involves other component
changes.


So, can you help me understand this a bit more? It sounds like these are
generated headers, yes? I'd like to understand your constraints and
weight them against various benefits that could be achieved here.

The groupings I made do appear to be roughly documented already,
for example:

struct   atom_common_table_header  table_header;
  // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,

Something emitted the "BOARD PARAMETERS" section heading as a comment,
so it likely also would know where it ends, yes? The good news here is
that for the dpm_info groups, they all end at the end of the existing
structs, see:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10

The matching regions in the PPTable_t structs are similarly marked with a
"BOARD PARAMETERS" section heading comment:

--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS
  
// SVI2 Board Parameters

+  struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
  
@@ -728,10 +729,10 @@ typedef struct {

uint32_t BoardVoltageCoeffB;// decode by /1000
  
uint32_t BoardReserved[7];

+  );
  
// Padding for MMHUB - do not modify this

uint32_t MmHubPadding[8]; // SMU internal use
-
  } PPTable_t;

Where they end seems known as well (the padding switches from a "Board"
to "MmHub" prefix at exactly the matching size).

So, given that these regions are already known by the export tool, how
about just updating the export tool to emit a struct there? I imagine
the problem with this would be the identifier churn needed, but that's
entirely mechanical.

However, I'm curious about another aspect of these regions: they are,
by definition, the same. Why isn't there a single struct describing
them already, given the existing redundancy? For example, look at the
member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these
the same? And then why aren't they described separately?

Fixing that would cut down on the redundancy here, and in the renaming,
you can fix the identifiers as well. It should be straight forward to
write a Coccinelle script to do this renaming for you after extracting
the structure.


The driver_if_* files updates are frequent and it is error prone to manually
group them each time we pick them for any update.


Why are these structs updated? It looks like 

RE: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend

[Chen, Guchun]

[Public]

+Leo and James to review as well.

This patch is:
Acked-by: Guchun Chen 

Regards,
Guchun

-Original Message-
From: Quan, Evan  
Sent: Thursday, August 19, 2021 11:09 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Chen, Guchun 
; Lazar, Lijo ; Quan, Evan 
; Pan, Xinhui 
Subject: [PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on 
suspend

Perform proper cleanups on UVD/VCE suspend: powergate enablement, clockgating 
enablement and dpm disablement. This can fix some hangs observed on suspending 
when UVD/VCE still using(e.g. issue "pm-suspend" when video is still playing).

Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
Signed-off-by: Evan Quan 
Signed-off-by: xinhui pan 
---
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 24   
drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 23 +++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c 
b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index 4eebf973a065..d0fc6ec18c29 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -554,6 +554,30 @@ static int uvd_v6_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+   /*
+* Proper cleanups before halting the HW engine:
+*   - cancel the delayed idle work
+*   - enable powergating
+*   - enable clockgating
+*   - disable dpm
+*
+* TODO: to align with the VCN implementation, move the
+* jobs for clockgating/powergating/dpm setting to
+* ->set_powergating_state().
+*/
+   cancel_delayed_work_sync(>uvd.idle_work);
+
+   if (adev->pm.dpm_enabled) {
+   amdgpu_dpm_enable_uvd(adev, false);
+   } else {
+   amdgpu_asic_set_uvd_clocks(adev, 0, 0);
+   /* shutdown the UVD block */
+   amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_UVD,
+  AMD_PG_STATE_GATE);
+   amdgpu_device_ip_set_clockgating_state(adev, 
AMD_IP_BLOCK_TYPE_UVD,
+  AMD_CG_STATE_GATE);
+   }
+
r = uvd_v6_0_hw_fini(adev);
if (r)
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
index 6d9108fa22e0..a594ade5d30a 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
@@ -503,6 +503,29 @@ static int vce_v3_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+   /*
+* Proper cleanups before halting the HW engine:
+*   - cancel the delayed idle work
+*   - enable powergating
+*   - enable clockgating
+*   - disable dpm
+*
+* TODO: to align with the VCN implementation, move the
+* jobs for clockgating/powergating/dpm setting to
+* ->set_powergating_state().
+*/
+   cancel_delayed_work_sync(>vce.idle_work);
+
+   if (adev->pm.dpm_enabled) {
+   amdgpu_dpm_enable_vce(adev, false);
+   } else {
+   amdgpu_asic_set_vce_clocks(adev, 0, 0);
+   amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCE,
+  AMD_PG_STATE_GATE);
+   amdgpu_device_ip_set_clockgating_state(adev, 
AMD_IP_BLOCK_TYPE_VCE,
+  AMD_CG_STATE_GATE);
+   }
+
r = vce_v3_0_hw_fini(adev);
if (r)
return r;
--
2.29.0

[PATCH] drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend

[Evan Quan]

Perform proper cleanups on UVD/VCE suspend: powergate enablement,
clockgating enablement and dpm disablement. This can fix some hangs
observed on suspending when UVD/VCE still using(e.g. issue
"pm-suspend" when video is still playing).

Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
Signed-off-by: Evan Quan 
Signed-off-by: xinhui pan 
---
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 24 
 drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 23 +++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c 
b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index 4eebf973a065..d0fc6ec18c29 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -554,6 +554,30 @@ static int uvd_v6_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+   /*
+* Proper cleanups before halting the HW engine:
+*   - cancel the delayed idle work
+*   - enable powergating
+*   - enable clockgating
+*   - disable dpm
+*
+* TODO: to align with the VCN implementation, move the
+* jobs for clockgating/powergating/dpm setting to
+* ->set_powergating_state().
+*/
+   cancel_delayed_work_sync(>uvd.idle_work);
+
+   if (adev->pm.dpm_enabled) {
+   amdgpu_dpm_enable_uvd(adev, false);
+   } else {
+   amdgpu_asic_set_uvd_clocks(adev, 0, 0);
+   /* shutdown the UVD block */
+   amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_UVD,
+  AMD_PG_STATE_GATE);
+   amdgpu_device_ip_set_clockgating_state(adev, 
AMD_IP_BLOCK_TYPE_UVD,
+  AMD_CG_STATE_GATE);
+   }
+
r = uvd_v6_0_hw_fini(adev);
if (r)
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
index 6d9108fa22e0..a594ade5d30a 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
@@ -503,6 +503,29 @@ static int vce_v3_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
+   /*
+* Proper cleanups before halting the HW engine:
+*   - cancel the delayed idle work
+*   - enable powergating
+*   - enable clockgating
+*   - disable dpm
+*
+* TODO: to align with the VCN implementation, move the
+* jobs for clockgating/powergating/dpm setting to
+* ->set_powergating_state().
+*/
+   cancel_delayed_work_sync(>vce.idle_work);
+
+   if (adev->pm.dpm_enabled) {
+   amdgpu_dpm_enable_vce(adev, false);
+   } else {
+   amdgpu_asic_set_vce_clocks(adev, 0, 0);
+   amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCE,
+  AMD_PG_STATE_GATE);
+   amdgpu_device_ip_set_clockgating_state(adev, 
AMD_IP_BLOCK_TYPE_VCE,
+  AMD_CG_STATE_GATE);
+   }
+
r = vce_v3_0_hw_fini(adev);
if (r)
return r;
-- 
2.29.0

RE: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE on suspend

[Quan, Evan]

[AMD Official Use Only]



> -Original Message-
> From: Lazar, Lijo 
> Sent: Wednesday, August 18, 2021 5:35 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org; Liu,
> Leo 
> Cc: Deucher, Alexander ; Chen, Guchun
> ; Pan, Xinhui 
> Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE
> on suspend
> 
> 
> 
> On 8/18/2021 2:15 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -Original Message-
> >> From: Lazar, Lijo 
> >> Sent: Tuesday, August 17, 2021 6:09 PM
> >> To: Quan, Evan ; amd-gfx@lists.freedesktop.org;
> >> Liu, Leo 
> >> Cc: Deucher, Alexander ; Chen, Guchun
> >> ; Pan, Xinhui 
> >> Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12
> UVD/VCE
> >> on suspend
> >>
> >>
> >>
> >> On 8/17/2021 3:13 PM, Quan, Evan wrote:
> >>> [AMD Official Use Only]
> >>>
> >>> +Leo to share his insights
> >>>
>  -Original Message-
>  From: Lazar, Lijo 
>  Sent: Tuesday, August 17, 2021 3:28 PM
>  To: Quan, Evan ; amd-
> g...@lists.freedesktop.org
>  Cc: Deucher, Alexander ; Chen,
> Guchun
>  ; Pan, Xinhui 
>  Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12
> >> UVD/VCE
>  on suspend
> 
> 
> 
>  On 8/17/2021 12:10 PM, Evan Quan wrote:
> > If the powergating of UVD/VCE is in process, wait for its
> > completion before proceeding(suspending). This can fix some hangs
> > observed on suspending when UVD/VCE still using(e.g. issue
> > "pm-suspend" when video is still playing).
> >
> > Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
> > Signed-off-by: Evan Quan 
> > Signed-off-by: xinhui pan 
> > ---
> > drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 +
> > drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 5 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > index 4eebf973a065..2fdce572baeb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> > @@ -554,6 +554,11 @@ static int uvd_v6_0_suspend(void *handle)
> > int r;
> > struct amdgpu_device *adev = (struct amdgpu_device
> *)handle;
> >
> > +   /*
> > +* If the powergating is in process, wait for its completion.
> > +*/
> > +   flush_delayed_work(>uvd.idle_work);
> > +
>  If running idle is a prerequisite before going to suspend, then
>  something else is missing here.
> 
>  Otherwise, the hang looks more like a pending work launched after
>  hardware is suspended and trying to access hardware. As the
>  hardware is going to be suspended anyway, doesn't it work with
>  cancel_delayed_work_sync - making sure that nothing is going to be
>  launched later to access hardware?
> >>> [Quan, Evan] The reason we chose flush_delayed_work instead of
> >> cancel_delayed_work_sync is we think those operations performed in
> >> idle_work(dpm disablement, powergating) seems needed considering
> the
> >> action is 'suspend'. So, instead of "cancel", maybe waiting for them
> >> completion is more proper.
> >>
> >> But it will do so only if the work is scheduled - so it doesn't seem
> >> to be a prerequisite for suspend. If it was a prerequisite, then the
> >> existing code is missing that (so that it gets done for all cases).
> > [Quan, Evan] Just confirmed that cancel_delayed_work_sync() alone
> cannot work.
> > In fact, our current driver already get cancel_delayed_work_sync()
> called(e.g. in amdgpu_uvd_suspend() on the path of uvd_v6_0_suspend).
> 
> Thanks Evan for checking this further.
> 
> If cancel_delayed_work_sync() is called in amdgpu_uvd_suspend(), then it
> means that originally executing idle_work was not considered as a
> prerequisite for suspending.
> 
> _uvd_suspend() is called "after" uvd_v6_0_hw_fini(adev), that maybe a little
> too late.
> 
> > To get the issue fixed, it has to be either:
> > 1. flush_delayed_work()
> > Or
> > 2. cancel_delayed_work_sync + amdgpu_dpm_enable_uvd/vce(adev,
> false)
> > (the job performed in idle work)
> 
> At minimum, it proves that disabling dpm is required for suspend.
> 
> > Btw, I do not think flush_delayed_work() is an incomplete fix. Since the
> UVD/VCE idle work is appended on ring->funcs->end_use.
> > So, as long as the UVD/VCE ring used and ended(it will since at least there 
> > is
> ring/ib test), there will be a chance to get the work waited and completed.
> 
> I agree that it fixes the issue, only thing is someone should look further to
> provide the right sequence of uvd suspend.
> 
> It doesn't give a root cause/right sequence - it only tells that forcing to
> execute idle_work fixes the problem probably due to an extra delay added
> by increased execution time or it disables DPM or something else.
> Someone should confirm that all of idle_work or a part of it 

RE: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Liu, Monk]

[AMD Official Use Only]

Hi Andrey and Daniel

We worked for a really long time on this new feature to AMD that finally can 
pick up the bad job from all timedout ones, and the change in scheduler 
(get/put fence in drm_sched_job_timedout, and remove the bad job delete and put 
back) is the last piece for us.

While we understand and realized that after the "bad job list node delete 
logic" being removed from job_timedout,  there will be race issues introduced 
if vendor's job_timeout calback is accessing the bad job  in parallel of 
scheduler doing "sched->ops->free_job(leanup_job)".

And to not introduce impact at all on those vendors I'd like to proposal a very 
simple change (which introduced a new bool member for scheduler to indicate if 
the del/put-back logic is needed or not) , check  patch here below:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 47ea468..5e0bdc4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -495,6 +495,8 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring,
return r;
}
 
+   ring->sched.keep_bad_job = true;
+
return 0;
 }
 
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 92d8de2..e7ac384 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
 {
struct drm_gpu_scheduler *sched;
struct drm_sched_job *job;
+   struct dma_fence *f = NULL;
 
sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
 
@@ -328,7 +329,11 @@ static void drm_sched_job_timedout(struct work_struct 
*work)
 * drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
 * is parked at which point it's safe.
 */
-   list_del_init(>list);
+   if (sched->keep_bad_job == false)
+   list_del_init(>list);
+   else
+   f = dma_fence_get(job->s_fence->parent);//get parent 
fence here to prevent hw_fence dropping to zero due to sched-main's 
cleanup_jobs, for amdgpu once parent fence drop to zero the sched_job will be 
kfree-ed 
+
spin_unlock(>job_list_lock);
 
job->sched->ops->timedout_job(job);
@@ -341,6 +346,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
job->sched->ops->free_job(job);
sched->free_guilty = false;
}
+
+   dma_fence_put(f);
} else {
spin_unlock(>job_list_lock);
}
@@ -396,7 +403,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct 
drm_sched_job *bad)
 * (earlier) cleanups and drm_sched_get_cleanup_job will not be called
 * now until the scheduler thread is unparked.
 */
-   if (bad && bad->sched == sched)
+   if (bad && bad->sched == sched && sched->keep_bad_job == false)
/*
 * Add at the head of the queue to reflect it was the earliest
 * job extracted.
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 4ea8606..5f9a640 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -301,6 +301,7 @@ struct drm_gpu_scheduler {
atomic_t_score;
boolready;
boolfree_guilty;
+   bool keep_bad_job;
 };
 
 int drm_sched_init(struct drm_gpu_scheduler *sched,


Thanks 

--
Monk Liu | Cloud-GPU Core team
--

-Original Message-
From: Daniel Vetter  
Sent: Wednesday, August 18, 2021 10:43 PM
To: Grodzovsky, Andrey 
Cc: Daniel Vetter ; Alex Deucher ; 
Chen, JingWen ; Maling list - DRI developers 
; amd-gfx list 
; Liu, Monk ; Koenig, 
Christian 
Subject: Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-08-18 10:32 a.m., Daniel Vetter wrote:
> > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:
> > > On 2021-08-18 10:02 a.m., Alex Deucher wrote:
> > > 
> > > > + dri-devel
> > > > 
> > > > Since scheduler is a shared component, please add dri-devel on 
> > > > all scheduler patches.
> > > > 
> > > > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  
> > > > wrote:
> > > > > [Why]
> > > > > for bailing job, this commit will delete it from pending list 
> > > > > thus the bailing job will never have a chance to be 
> > > > > resubmitted even in advance tdr mode.
> > > > > 
> > > > > [How]
> > > > > after embeded hw_fence into amdgpu_job is done, the race 
> > > > > condition that this commit tries to work around is 

Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region

[Kees Cook]

On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote:
> 
> On 8/18/2021 11:34 AM, Kees Cook wrote:
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > intentionally writing across neighboring fields.
> > 
> > Use struct_group() in structs:
> > struct atom_smc_dpm_info_v4_5
> > struct atom_smc_dpm_info_v4_6
> > struct atom_smc_dpm_info_v4_7
> > struct atom_smc_dpm_info_v4_10
> > PPTable_t
> > so the grouped members can be referenced together. This will allow
> > memcpy() and sizeof() to more easily reason about sizes, improve
> > readability, and avoid future warnings about writing beyond the end of
> > the first member.
> > 
> > "pahole" shows no size nor member offset changes to any structs.
> > "objdump -d" shows no object code changes.
> > 
> > Cc: "Christian König" 
> > Cc: "Pan, Xinhui" 
> > Cc: David Airlie 
> > Cc: Daniel Vetter 
> > Cc: Hawking Zhang 
> > Cc: Feifei Xu 
> > Cc: Lijo Lazar 
> > Cc: Likun Gao 
> > Cc: Jiawei Gu 
> > Cc: Evan Quan 
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: dri-de...@lists.freedesktop.org
> > Signed-off-by: Kees Cook 
> > Acked-by: Alex Deucher 
> > Link: 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C92b8d2f072f0444b9f8508d9620f6971%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648640625729624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rKh5LUXCRUsorYM3kSpG2tkB%2Fczwl9I9EBnWBCtbg6Q%3Dreserved=0
> > ---
> >   drivers/gpu/drm/amd/include/atomfirmware.h   |  9 -
> >   .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h|  3 ++-
> >   drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h  |  3 ++-
> >   .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h   |  3 ++-
> 
> Hi Kees,

Hi! Thanks for looking into this.

> The headers which define these structs are firmware/VBIOS interfaces and are
> picked directly from those components. There are difficulties in grouping
> them to structs at the original source as that involves other component
> changes.

So, can you help me understand this a bit more? It sounds like these are
generated headers, yes? I'd like to understand your constraints and
weight them against various benefits that could be achieved here.

The groupings I made do appear to be roughly documented already,
for example:

   struct   atom_common_table_header  table_header;
 // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,

Something emitted the "BOARD PARAMETERS" section heading as a comment,
so it likely also would know where it ends, yes? The good news here is
that for the dpm_info groups, they all end at the end of the existing
structs, see:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10

The matching regions in the PPTable_t structs are similarly marked with a
"BOARD PARAMETERS" section heading comment:

--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
   // SECTION: BOARD PARAMETERS
 
   // SVI2 Board Parameters
+  struct_group(v4_6,
   uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
   uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
 
@@ -728,10 +729,10 @@ typedef struct {
   uint32_t BoardVoltageCoeffB;// decode by /1000
 
   uint32_t BoardReserved[7];
+  );
 
   // Padding for MMHUB - do not modify this
   uint32_t MmHubPadding[8]; // SMU internal use
-
 } PPTable_t;

Where they end seems known as well (the padding switches from a "Board"
to "MmHub" prefix at exactly the matching size).

So, given that these regions are already known by the export tool, how
about just updating the export tool to emit a struct there? I imagine
the problem with this would be the identifier churn needed, but that's
entirely mechanical.

However, I'm curious about another aspect of these regions: they are,
by definition, the same. Why isn't there a single struct describing
them already, given the existing redundancy? For example, look at the
member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these
the same? And then why aren't they described separately?

Fixing that would cut down on the redundancy here, and in the renaming,
you can fix the identifiers as well. It should be straight forward to
write a Coccinelle script to do this renaming for you after extracting
the structure.

> The driver_if_* files updates are frequent and it is error prone to manually
> group them 

[pull] amdgpu, amdkfd drm-fixes-5.14

[Alex Deucher]

Hi Dave, Daniel,

Fixes for 5.14.

The following changes since commit 7c60610d476766e128cc4284bb6349732cbd6606:

  Linux 5.14-rc6 (2021-08-15 13:40:53 -1000)

are available in the Git repository at:

  https://gitlab.freedesktop.org/agd5f/linux.git 
tags/amd-drm-fixes-5.14-2021-08-18

for you to fetch changes up to 37717b8c9f0e8c4dd73fc522769cc14649b4f657:

  drm/amd/display: Use DCN30 watermark calc for DCN301 (2021-08-18 18:30:00 
-0400)


amd-drm-fixes-5.14-2021-08-18:

amdgpu:
- vega10 SMU workload fix
- DCN VM fix
- DCN 3.01 watermark fix

amdkfd:
- SVM fix


Jake Wang (1):
  drm/amd/display: Ensure DCN save after VM setup

Kenneth Feng (2):
  Revert "drm/amd/pm: fix workload mismatch on vega10"
  drm/amd/pm: change the workload type for some cards

Yifan Zhang (1):
  drm/amdkfd: fix random KFDSVMRangeTest.SetGetAttributesTest test failure

Zhan Liu (1):
  drm/amd/display: Use DCN30 watermark calc for DCN301

 drivers/gpu/drm/amd/amdkfd/kfd_svm.c   |  8 ++
 drivers/gpu/drm/amd/display/dc/core/dc.c   |  6 ++
 drivers/gpu/drm/amd/display/dc/core/dc_vm_helper.c |  3 +
 drivers/gpu/drm/amd/display/dc/dc.h|  1 +
 .../drm/amd/display/dc/dcn301/dcn301_resource.c| 96 +-
 drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c | 12 +++
 drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.h |  1 +
 drivers/gpu/drm/amd/display/dc/dcn31/dcn31_init.c  |  1 +
 drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h  |  1 +
 drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h|  5 ++
 .../gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c  | 15 +++-
 11 files changed, 53 insertions(+), 96 deletions(-)

Re: [PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled

[Alex Deucher]

Applied.  Let's see how long this one lasts :)

Alex

On Tue, Aug 17, 2021 at 4:23 AM Michel Dänzer  wrote:
>
> From: Michel Dänzer 
>
> schedule_delayed_work does not push back the work if it was already
> scheduled before, so amdgpu_device_delay_enable_gfx_off ran ~100 ms
> after the first time GFXOFF was disabled and re-enabled, even if GFXOFF
> was disabled and re-enabled again during those 100 ms.
>
> This resulted in frame drops / stutter with the upcoming mutter 41
> release on Navi 14, due to constantly enabling GFXOFF in the HW and
> disabling it again (for getting the GPU clock counter).
>
> To fix this, call cancel_delayed_work_sync when the disable count
> transitions from 0 to 1, and only schedule the delayed work on the
> reverse transition, not if the disable count was already 0. This makes
> sure the delayed work doesn't run at unexpected times, and allows it to
> be lock-free.
>
> v2:
> * Use cancel_delayed_work_sync & mutex_trylock instead of
>   mod_delayed_work.
> v3:
> * Make amdgpu_device_delay_enable_gfx_off lock-free (Christian König)
> v4:
> * Fix race condition between amdgpu_gfx_off_ctrl incrementing
>   adev->gfx.gfx_off_req_count and amdgpu_device_delay_enable_gfx_off
>   checking for it to be 0 (Evan Quan)
>
> Cc: sta...@vger.kernel.org
> Reviewed-by: Lijo Lazar  # v3
> Acked-by: Christian König  # v3
> Signed-off-by: Michel Dänzer 
> ---
>
> Alex, probably best to wait a bit longer before picking this up. :)
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c| 36 +++---
>  2 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index f3fd5ec710b6..f944ed858f3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2777,12 +2777,11 @@ static void amdgpu_device_delay_enable_gfx_off(struct 
> work_struct *work)
> struct amdgpu_device *adev =
> container_of(work, struct amdgpu_device, 
> gfx.gfx_off_delay_work.work);
>
> -   mutex_lock(>gfx.gfx_off_mutex);
> -   if (!adev->gfx.gfx_off_state && !adev->gfx.gfx_off_req_count) {
> -   if (!amdgpu_dpm_set_powergating_by_smu(adev, 
> AMD_IP_BLOCK_TYPE_GFX, true))
> -   adev->gfx.gfx_off_state = true;
> -   }
> -   mutex_unlock(>gfx.gfx_off_mutex);
> +   WARN_ON_ONCE(adev->gfx.gfx_off_state);
> +   WARN_ON_ONCE(adev->gfx.gfx_off_req_count);
> +
> +   if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, 
> true))
> +   adev->gfx.gfx_off_state = true;
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index a0be0772c8b3..b4ced45301be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -563,24 +563,38 @@ void amdgpu_gfx_off_ctrl(struct amdgpu_device *adev, 
> bool enable)
>
> mutex_lock(>gfx.gfx_off_mutex);
>
> -   if (!enable)
> -   adev->gfx.gfx_off_req_count++;
> -   else if (adev->gfx.gfx_off_req_count > 0)
> +   if (enable) {
> +   /* If the count is already 0, it means there's an imbalance 
> bug somewhere.
> +* Note that the bug may be in a different caller than the 
> one which triggers the
> +* WARN_ON_ONCE.
> +*/
> +   if (WARN_ON_ONCE(adev->gfx.gfx_off_req_count == 0))
> +   goto unlock;
> +
> adev->gfx.gfx_off_req_count--;
>
> -   if (enable && !adev->gfx.gfx_off_state && 
> !adev->gfx.gfx_off_req_count) {
> -   schedule_delayed_work(>gfx.gfx_off_delay_work, 
> GFX_OFF_DELAY_ENABLE);
> -   } else if (!enable && adev->gfx.gfx_off_state) {
> -   if (!amdgpu_dpm_set_powergating_by_smu(adev, 
> AMD_IP_BLOCK_TYPE_GFX, false)) {
> -   adev->gfx.gfx_off_state = false;
> +   if (adev->gfx.gfx_off_req_count == 0 && 
> !adev->gfx.gfx_off_state)
> +   schedule_delayed_work(>gfx.gfx_off_delay_work, 
> GFX_OFF_DELAY_ENABLE);
> +   } else {
> +   if (adev->gfx.gfx_off_req_count == 0) {
> +   
> cancel_delayed_work_sync(>gfx.gfx_off_delay_work);
> +
> +   if (adev->gfx.gfx_off_state &&
> +   !amdgpu_dpm_set_powergating_by_smu(adev, 
> AMD_IP_BLOCK_TYPE_GFX, false)) {
> +   adev->gfx.gfx_off_state = false;
>
> -   if (adev->gfx.funcs->init_spm_golden) {
> -   dev_dbg(adev->dev, "GFXOFF is disabled, 
> re-init SPM golden settings\n");
> -   amdgpu_gfx_init_spm_golden(adev);
> +   if (adev->gfx.funcs->init_spm_golden) {
> +   

Re: [PATCH v6 02/13] mm: remove extra ZONE_DEVICE struct page refcount

[Ralph Campbell]

On 8/17/21 5:35 PM, Felix Kuehling wrote:

Am 2021-08-17 um 8:01 p.m. schrieb Ralph Campbell:

On 8/12/21 11:31 PM, Alex Sierra wrote:

From: Ralph Campbell 

ZONE_DEVICE struct pages have an extra reference count that
complicates the
code for put_page() and several places in the kernel that need to
check the
reference count to see that a page is not being used (gup, compaction,
migration, etc.). Clean up the code so the reference count doesn't
need to
be treated specially for ZONE_DEVICE.

v2:
AS: merged this patch in linux 5.11 version

v5:
AS: add condition at try_grab_page to check for the zone device type,
while
page ref counter is checked less/equal to zero. In case of device
zone, pages
ref counter are initialized to zero.

Signed-off-by: Ralph Campbell 
Signed-off-by: Alex Sierra 
---
   arch/powerpc/kvm/book3s_hv_uvmem.c |  2 +-
   drivers/gpu/drm/nouveau/nouveau_dmem.c |  2 +-
   fs/dax.c   |  4 +-
   include/linux/dax.h    |  2 +-
   include/linux/memremap.h   |  7 +--
   include/linux/mm.h | 13 +
   lib/test_hmm.c |  2 +-
   mm/internal.h  |  8 +++
   mm/memremap.c  | 68 +++---
   mm/migrate.c   |  5 --
   mm/page_alloc.c    |  3 ++
   mm/swap.c  | 45 ++---
   12 files changed, 46 insertions(+), 115 deletions(-)


I haven't seen a response to the issues I raised back at v3 of this
series.
https://lore.kernel.org/linux-mm/4f6dd918-d79b-1aa7-3a4c-caa67ddc2...@nvidia.com/


Did I miss something?

I think part of the response was that we did more testing. Alex added
support for DEVICE_GENERIC pages to test_hmm and he ran DAX tests
recommended by Theodore Tso. In that testing he ran into a WARN_ON_ONCE
about a zero page refcount in try_get_page. The fix is in the latest
version of patch 2. But it's already obsolete because John Hubbard is
about to remove that function altogether.

I think the issues you raised were more uncertainty than known bugs. It
seems the fact that you can have DAX pages with 0 refcount is a feature
more than a bug.

Regards,
   Felix


Did you test on a system without CONFIG_ARCH_HAS_PTE_SPECIAL defined?
In that case, mmap() of a DAX device will call insert_page() which calls
get_page() which would trigger VM_BUG_ON_PAGE().

I can believe it is OK for PTE_SPECIAL page table entries to have no
struct page or that MEMORY_DEVICE_GENERIC struct pages be mapped with
a zero reference count using insert_pfn().

I find it hard to believe that other MM developers don't see an issue
with a struct page with refcount == 0 and mapcount == 1.

I don't see where init_page_count() is being called for the
MEMORY_DEVICE_GENERIC or MEMORY_DEVICE_PRIVATE struct pages the AMD
driver allocates and passes to migrate_vma_setup().
Looks like svm_migrate_get_vram_page() needs to call init_page_count()
instead of get_page(). (I'm looking at branch origin/alexsierrag/device_generic
https://github.com/RadeonOpenCompute/ROCK-Kernel-Driver.git)

Also, what about the other places where is_device_private_page() is called?
Don't they need to be updated to call is_device_page() instead?
One of my goals for this patch was to remove special casing reference counts
for ZONE_DEVICE pages in rmap.c, etc.

I still think this patch needs an ACK from a FS/DAX maintainer.

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Andrey Grodzovsky]

On 2021-08-18 10:42 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:32 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:02 a.m., Alex Deucher wrote:


+ dri-devel

Since scheduler is a shared component, please add dri-devel on all
scheduler patches.

On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:

[Why]
for bailing job, this commit will delete it from pending list thus the
bailing job will never have a chance to be resubmitted even in advance
tdr mode.

[How]
after embeded hw_fence into amdgpu_job is done, the race condition that
this commit tries to work around is completely solved.So revert this
commit.
This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
v2:
add dma_fence_get/put() around timedout_job to avoid concurrent delete
during processing timedout_job

Signed-off-by: Jingwen Chen 
---
drivers/gpu/drm/scheduler/sched_main.c | 23 +--
1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index a2a953693b45..f9b9b3aefc4a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
{
   struct drm_gpu_scheduler *sched;
   struct drm_sched_job *job;
+   struct dma_fence *fence;
   enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;

   sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
@@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
*work)

   if (job) {
   /*
-* Remove the bad job so it cannot be freed by concurrent
-* drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
-* is parked at which point it's safe.
+* Get job->s_fence->parent here to avoid concurrent delete 
during
+* processing timedout_job
*/
-   list_del_init(>list);
+   fence = dma_fence_get(job->s_fence->parent);

While this is true for amdgpu, it has no meaning for other drivers for whom
we haven't
done the refactoring of embedding HW fence (parent) into the job structure.
In fact thinking
about it, unless you do the HW fence embedding for all the drivers using the
scheduler you cannot
revert this patch or you will just break them.

btw, why did you do that embedding? I do still have my patches with
dma_fence annotations floating around, but my idea at least was to fix
that issue with a mempool, not with embeddeding. What was the motivation
for embedding the wh fence?
-Daniel


The motivation was 2 fold, avoid memory allocation during jobs submissions
(HW fence allocation) because as Christian explained this leads to deadlock
with
mm code during evictions due to memory pressure (Christian can clarify if I
messed

Yeah that's the exact same thing I've chased with my dma_fence
annotations, but thus far zero to none interested in getting it sorted. I
think it'd be good to have some cross-driver agreement on how this should
be solved before someone just charges ahead ...


this explanation). Second is to exactly revert this patch because while it
solved the issue
described in the patch it created another with drivers who baildc out early
during TDR handling
for various reason and the job would just leak because it was already
removed form pending list.

Can't we reinsert it before we restart the scheduler thread? It might need
a separate list for that due to the lockless queue tricks. Or am I
thinking about the wrong kind of "we lost the job"?
-Danile



If you look at the original patch it would reinsert it even earlier - 
right after stopping the  SW scheduler thread, and even then it was to 
late for
some drivers as they would decide to return back from their TDR handler 
even before that. It is solvable but in an ugly way as far as I see, you 
need to
require each driver in his code to put the job back in the list if they 
do it before reaching the place where scheduler framework does it. Kind of

spaghetti code seems to me.

Andrey





Andrey





Andrey



   spin_unlock(>job_list_lock);

   status = job->sched->ops->timedout_job(job);
@@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
   job->sched->ops->free_job(job);
   sched->free_guilty = false;
   }
+   dma_fence_put(fence);
   } else {
   spin_unlock(>job_list_lock);
   }
@@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
struct drm_sched_job *bad)

   kthread_park(sched->thread);

-   /*
-* Reinsert back the bad job here - now 

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Daniel Vetter]

On Wed, Aug 18, 2021 at 10:36:32AM -0400, Andrey Grodzovsky wrote:
> 
> On 2021-08-18 10:32 a.m., Daniel Vetter wrote:
> > On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:
> > > On 2021-08-18 10:02 a.m., Alex Deucher wrote:
> > > 
> > > > + dri-devel
> > > > 
> > > > Since scheduler is a shared component, please add dri-devel on all
> > > > scheduler patches.
> > > > 
> > > > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  
> > > > wrote:
> > > > > [Why]
> > > > > for bailing job, this commit will delete it from pending list thus the
> > > > > bailing job will never have a chance to be resubmitted even in advance
> > > > > tdr mode.
> > > > > 
> > > > > [How]
> > > > > after embeded hw_fence into amdgpu_job is done, the race condition 
> > > > > that
> > > > > this commit tries to work around is completely solved.So revert this
> > > > > commit.
> > > > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > > > > v2:
> > > > > add dma_fence_get/put() around timedout_job to avoid concurrent delete
> > > > > during processing timedout_job
> > > > > 
> > > > > Signed-off-by: Jingwen Chen 
> > > > > ---
> > > > >drivers/gpu/drm/scheduler/sched_main.c | 23 +--
> > > > >1 file changed, 5 insertions(+), 18 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > index a2a953693b45..f9b9b3aefc4a 100644
> > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > > @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct 
> > > > > work_struct *work)
> > > > >{
> > > > >   struct drm_gpu_scheduler *sched;
> > > > >   struct drm_sched_job *job;
> > > > > +   struct dma_fence *fence;
> > > > >   enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
> > > > > 
> > > > >   sched = container_of(work, struct drm_gpu_scheduler, 
> > > > > work_tdr.work);
> > > > > @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct 
> > > > > work_struct *work)
> > > > > 
> > > > >   if (job) {
> > > > >   /*
> > > > > -* Remove the bad job so it cannot be freed by 
> > > > > concurrent
> > > > > -* drm_sched_cleanup_jobs. It will be reinserted back 
> > > > > after sched->thread
> > > > > -* is parked at which point it's safe.
> > > > > +* Get job->s_fence->parent here to avoid concurrent 
> > > > > delete during
> > > > > +* processing timedout_job
> > > > >*/
> > > > > -   list_del_init(>list);
> > > > > +   fence = dma_fence_get(job->s_fence->parent);
> > > 
> > > While this is true for amdgpu, it has no meaning for other drivers for 
> > > whom
> > > we haven't
> > > done the refactoring of embedding HW fence (parent) into the job 
> > > structure.
> > > In fact thinking
> > > about it, unless you do the HW fence embedding for all the drivers using 
> > > the
> > > scheduler you cannot
> > > revert this patch or you will just break them.
> > btw, why did you do that embedding? I do still have my patches with
> > dma_fence annotations floating around, but my idea at least was to fix
> > that issue with a mempool, not with embeddeding. What was the motivation
> > for embedding the wh fence?
> > -Daniel
> 
> 
> The motivation was 2 fold, avoid memory allocation during jobs submissions
> (HW fence allocation) because as Christian explained this leads to deadlock
> with
> mm code during evictions due to memory pressure (Christian can clarify if I
> messed

Yeah that's the exact same thing I've chased with my dma_fence
annotations, but thus far zero to none interested in getting it sorted. I
think it'd be good to have some cross-driver agreement on how this should
be solved before someone just charges ahead ...

> this explanation). Second is to exactly revert this patch because while it
> solved the issue
> described in the patch it created another with drivers who baildc out early
> during TDR handling
> for various reason and the job would just leak because it was already
> removed form pending list.

Can't we reinsert it before we restart the scheduler thread? It might need
a separate list for that due to the lockless queue tricks. Or am I
thinking about the wrong kind of "we lost the job"?
-Danile

> 
> Andrey
> 
> 
> > 
> > 
> > > Andrey
> > > 
> > > 
> > > > >   spin_unlock(>job_list_lock);
> > > > > 
> > > > >   status = job->sched->ops->timedout_job(job);
> > > > > @@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct 
> > > > > work_struct *work)
> > > > >   job->sched->ops->free_job(job);
> > > > >   sched->free_guilty = false;
> > > > >   }
> > > > > +   dma_fence_put(fence);
> > > > >   } else {

Re: [PATCH] Revert "drm/scheduler: Avoid accessing freed bad job."

[Jingwen Chen]

Sorry, just get what you mean, will submit a v2 patch.

On Wed Aug 18, 2021 at 04:08:37PM +0800, Jingwen Chen wrote:
> On Tue Aug 17, 2021 at 03:43:58PM +0200, Christian König wrote:
> > 
> > 
> > Am 17.08.21 um 15:37 schrieb Andrey Grodzovsky:
> > > On 2021-08-17 12:28 a.m., Jingwen Chen wrote:
> > > > [Why]
> > > > for bailing job, this commit will delete it from pending list thus the
> > > > bailing job will never have a chance to be resubmitted even in advance
> > > > tdr mode.
> > > > 
> > > > [How]
> > > > after embeded hw_fence into amdgpu_job is done, the race condition that
> > > > this commit tries to work around is completely solved.So revert this
> > > > commit.
> > > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > > 
> > > 
> > > Can you elaborate please how this solves the race ?
> > > As far as I see and  with this patch reverted, in drm_sched_job_timedout
> > > you get a pointer to next job to process in timed out handler,
> > > immediately
> > > next this job is actually finished and it's fence signaled, this in turn
> > > triggers
> > > drm_sched_get_cleanup_job which fetches this job and returns to
> Hi Andrey,
> 
> if drm_sched_job_timedout is triggered first, drm_sched_get_cleanup_job will 
> return
> NULL when the timeout worker is running according to this code:
>   if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>   !cancel_delayed_work(>work_tdr)) ||
>   kthread_should_park())
>   return NULL;
> 
> But yes a dma_fence_get(job->s_fence->parent) is needed before
> processing timedout_job. When the bad job is signaled just before
> processing, the amdgpu_fence_free can be triggered by the dma_fence_put() of 
> HW fence.
> And if I'm understanding this race condition correctly, the spin_lock is
> still needed here to avoid the drm_sched_get_cleanup_job get the
> spin_lock first and then enter the tdr work.
> > > drm_sched_main
> > > which in turn call free_job callabck->...->amdgpu_fence_free which frees
> > > the job
> > > from the HW dma_fence release callback. After that you proceed with a
> > > freed job
> > > in timed out handler.
> > > 
> > > If you could take the HW fence reference from drm_sched_job_timedout
> > > before
> > > starting processing then yes, I think it would work.
> > 
> > Yes, precisely that's what I had in mind as well and seems to be missing
> > from this patch.
> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > Andrey
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Jingwen Chen 
> > > > ---
> > > >   drivers/gpu/drm/scheduler/sched_main.c | 27 --
> > > >   1 file changed, 27 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index a2a953693b45..31d1176d939f 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -317,21 +317,10 @@ static void drm_sched_job_timedout(struct
> > > > work_struct *work)
> > > >   enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
> > > >     sched = container_of(work, struct drm_gpu_scheduler,
> > > > work_tdr.work);
> > > > -
> > > > -    /* Protects against concurrent deletion in
> > > > drm_sched_get_cleanup_job */
> > > > -    spin_lock(>job_list_lock);
> > > >   job = list_first_entry_or_null(>pending_list,
> > > >  struct drm_sched_job, list);
> > > >     if (job) {
> > > > -    /*
> > > > - * Remove the bad job so it cannot be freed by concurrent
> > > > - * drm_sched_cleanup_jobs. It will be reinserted back after
> > > > sched->thread
> > > > - * is parked at which point it's safe.
> > > > - */
> > > > -    list_del_init(>list);
> > > > -    spin_unlock(>job_list_lock);
> > > > -
> > > >   status = job->sched->ops->timedout_job(job);
> > > >     /*
> > > > @@ -342,8 +331,6 @@ static void drm_sched_job_timedout(struct
> > > > work_struct *work)
> > > >   job->sched->ops->free_job(job);
> > > >   sched->free_guilty = false;
> > > >   }
> > > > -    } else {
> > > > -    spin_unlock(>job_list_lock);
> > > >   }
> > > >     if (status != DRM_GPU_SCHED_STAT_ENODEV) {
> > > > @@ -392,20 +379,6 @@ void drm_sched_stop(struct drm_gpu_scheduler
> > > > *sched, struct drm_sched_job *bad)
> > > >     kthread_park(sched->thread);
> > > >   -    /*
> > > > - * Reinsert back the bad job here - now it's safe as
> > > > - * drm_sched_get_cleanup_job cannot race against us and release the
> > > > - * bad job at this point - we parked (waited for) any in progress
> > > > - * (earlier) cleanups and drm_sched_get_cleanup_job will not be
> > > > called
> > > > - * now until the scheduler thread is unparked.
> > > > - */
> > > > -    if (bad && bad->sched == sched)
> > > > -    /*
> > > > - * Add at the head of the queue to 

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Andrey Grodzovsky]

On 2021-08-18 10:32 a.m., Daniel Vetter wrote:

On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:

On 2021-08-18 10:02 a.m., Alex Deucher wrote:


+ dri-devel

Since scheduler is a shared component, please add dri-devel on all
scheduler patches.

On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:

[Why]
for bailing job, this commit will delete it from pending list thus the
bailing job will never have a chance to be resubmitted even in advance
tdr mode.

[How]
after embeded hw_fence into amdgpu_job is done, the race condition that
this commit tries to work around is completely solved.So revert this
commit.
This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
v2:
add dma_fence_get/put() around timedout_job to avoid concurrent delete
during processing timedout_job

Signed-off-by: Jingwen Chen 
---
   drivers/gpu/drm/scheduler/sched_main.c | 23 +--
   1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index a2a953693b45..f9b9b3aefc4a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
   {
  struct drm_gpu_scheduler *sched;
  struct drm_sched_job *job;
+   struct dma_fence *fence;
  enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;

  sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
@@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
*work)

  if (job) {
  /*
-* Remove the bad job so it cannot be freed by concurrent
-* drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
-* is parked at which point it's safe.
+* Get job->s_fence->parent here to avoid concurrent delete 
during
+* processing timedout_job
   */
-   list_del_init(>list);
+   fence = dma_fence_get(job->s_fence->parent);


While this is true for amdgpu, it has no meaning for other drivers for whom
we haven't
done the refactoring of embedding HW fence (parent) into the job structure.
In fact thinking
about it, unless you do the HW fence embedding for all the drivers using the
scheduler you cannot
revert this patch or you will just break them.

btw, why did you do that embedding? I do still have my patches with
dma_fence annotations floating around, but my idea at least was to fix
that issue with a mempool, not with embeddeding. What was the motivation
for embedding the wh fence?
-Daniel



The motivation was 2 fold, avoid memory allocation during jobs submissions
(HW fence allocation) because as Christian explained this leads to 
deadlock with
mm code during evictions due to memory pressure (Christian can clarify 
if I messed
this explanation). Second is to exactly revert this patch because while 
it solved the issue
described in the patch it created another with drivers who baildc out 
early during TDR handling
for various reason and the job would just leak because it was already 
removed form pending list.


Andrey






Andrey



  spin_unlock(>job_list_lock);

  status = job->sched->ops->timedout_job(job);
@@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
  job->sched->ops->free_job(job);
  sched->free_guilty = false;
  }
+   dma_fence_put(fence);
  } else {
  spin_unlock(>job_list_lock);
  }
@@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
struct drm_sched_job *bad)

  kthread_park(sched->thread);

-   /*
-* Reinsert back the bad job here - now it's safe as
-* drm_sched_get_cleanup_job cannot race against us and release the
-* bad job at this point - we parked (waited for) any in progress
-* (earlier) cleanups and drm_sched_get_cleanup_job will not be called
-* now until the scheduler thread is unparked.
-*/
-   if (bad && bad->sched == sched)
-   /*
-* Add at the head of the queue to reflect it was the earliest
-* job extracted.
-*/
-   list_add(>list, >pending_list);
-
  /*
   * Iterate the job list from later to  earlier one and either deactive
   * their HW callbacks or remove them from pending list if they already
--
2.25.1

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Daniel Vetter]

On Wed, Aug 18, 2021 at 10:26:25AM -0400, Andrey Grodzovsky wrote:
> On 2021-08-18 10:02 a.m., Alex Deucher wrote:
> 
> > + dri-devel
> > 
> > Since scheduler is a shared component, please add dri-devel on all
> > scheduler patches.
> > 
> > On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:
> > > [Why]
> > > for bailing job, this commit will delete it from pending list thus the
> > > bailing job will never have a chance to be resubmitted even in advance
> > > tdr mode.
> > > 
> > > [How]
> > > after embeded hw_fence into amdgpu_job is done, the race condition that
> > > this commit tries to work around is completely solved.So revert this
> > > commit.
> > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > > v2:
> > > add dma_fence_get/put() around timedout_job to avoid concurrent delete
> > > during processing timedout_job
> > > 
> > > Signed-off-by: Jingwen Chen 
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 23 +--
> > >   1 file changed, 5 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index a2a953693b45..f9b9b3aefc4a 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct 
> > > *work)
> > >   {
> > >  struct drm_gpu_scheduler *sched;
> > >  struct drm_sched_job *job;
> > > +   struct dma_fence *fence;
> > >  enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
> > > 
> > >  sched = container_of(work, struct drm_gpu_scheduler, 
> > > work_tdr.work);
> > > @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct 
> > > work_struct *work)
> > > 
> > >  if (job) {
> > >  /*
> > > -* Remove the bad job so it cannot be freed by concurrent
> > > -* drm_sched_cleanup_jobs. It will be reinserted back 
> > > after sched->thread
> > > -* is parked at which point it's safe.
> > > +* Get job->s_fence->parent here to avoid concurrent 
> > > delete during
> > > +* processing timedout_job
> > >   */
> > > -   list_del_init(>list);
> > > +   fence = dma_fence_get(job->s_fence->parent);
> 
> 
> While this is true for amdgpu, it has no meaning for other drivers for whom
> we haven't
> done the refactoring of embedding HW fence (parent) into the job structure.
> In fact thinking
> about it, unless you do the HW fence embedding for all the drivers using the
> scheduler you cannot
> revert this patch or you will just break them.

btw, why did you do that embedding? I do still have my patches with
dma_fence annotations floating around, but my idea at least was to fix
that issue with a mempool, not with embeddeding. What was the motivation
for embedding the wh fence?
-Daniel


> 
> Andrey
> 
> 
> > >  spin_unlock(>job_list_lock);
> > > 
> > >  status = job->sched->ops->timedout_job(job);
> > > @@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct 
> > > *work)
> > >  job->sched->ops->free_job(job);
> > >  sched->free_guilty = false;
> > >  }
> > > +   dma_fence_put(fence);
> > >  } else {
> > >  spin_unlock(>job_list_lock);
> > >  }
> > > @@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
> > > struct drm_sched_job *bad)
> > > 
> > >  kthread_park(sched->thread);
> > > 
> > > -   /*
> > > -* Reinsert back the bad job here - now it's safe as
> > > -* drm_sched_get_cleanup_job cannot race against us and release 
> > > the
> > > -* bad job at this point - we parked (waited for) any in progress
> > > -* (earlier) cleanups and drm_sched_get_cleanup_job will not be 
> > > called
> > > -* now until the scheduler thread is unparked.
> > > -*/
> > > -   if (bad && bad->sched == sched)
> > > -   /*
> > > -* Add at the head of the queue to reflect it was the 
> > > earliest
> > > -* job extracted.
> > > -*/
> > > -   list_add(>list, >pending_list);
> > > -
> > >  /*
> > >   * Iterate the job list from later to  earlier one and either 
> > > deactive
> > >   * their HW callbacks or remove them from pending list if they 
> > > already
> > > --
> > > 2.25.1
> > > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Daniel Vetter]

On Wed, Aug 18, 2021 at 10:02:06AM -0400, Alex Deucher wrote:
> + dri-devel
> 
> Since scheduler is a shared component, please add dri-devel on all
> scheduler patches.

Do we need a MAINTAINRS entry specifically for this, or just oversight?

> On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:
> >
> > [Why]
> > for bailing job, this commit will delete it from pending list thus the
> > bailing job will never have a chance to be resubmitted even in advance
> > tdr mode.
> >
> > [How]
> > after embeded hw_fence into amdgpu_job is done, the race condition that
> > this commit tries to work around is completely solved.So revert this
> > commit.

Does this also hold for all other drivers? In general the commit message
feels rather rushed and I have no idea what's really going on.

Also at least around tdr there's been some solid clarifications around
how this is supposed to work between tdr and main scheduler thread, would
be good to explain how that all fits together. Or should fit together.
-Daniel

> > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > v2:
> > add dma_fence_get/put() around timedout_job to avoid concurrent delete
> > during processing timedout_job
> >
> > Signed-off-by: Jingwen Chen 
> > ---
> >  drivers/gpu/drm/scheduler/sched_main.c | 23 +--
> >  1 file changed, 5 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index a2a953693b45..f9b9b3aefc4a 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct 
> > *work)
> >  {
> > struct drm_gpu_scheduler *sched;
> > struct drm_sched_job *job;
> > +   struct dma_fence *fence;
> > enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
> >
> > sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> > @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
> > *work)
> >
> > if (job) {
> > /*
> > -* Remove the bad job so it cannot be freed by concurrent
> > -* drm_sched_cleanup_jobs. It will be reinserted back after 
> > sched->thread
> > -* is parked at which point it's safe.
> > +* Get job->s_fence->parent here to avoid concurrent delete 
> > during
> > +* processing timedout_job
> >  */
> > -   list_del_init(>list);
> > +   fence = dma_fence_get(job->s_fence->parent);
> > spin_unlock(>job_list_lock);
> >
> > status = job->sched->ops->timedout_job(job);
> > @@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct 
> > *work)
> > job->sched->ops->free_job(job);
> > sched->free_guilty = false;
> > }
> > +   dma_fence_put(fence);
> > } else {
> > spin_unlock(>job_list_lock);
> > }
> > @@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
> > struct drm_sched_job *bad)
> >
> > kthread_park(sched->thread);
> >
> > -   /*
> > -* Reinsert back the bad job here - now it's safe as
> > -* drm_sched_get_cleanup_job cannot race against us and release the
> > -* bad job at this point - we parked (waited for) any in progress
> > -* (earlier) cleanups and drm_sched_get_cleanup_job will not be 
> > called
> > -* now until the scheduler thread is unparked.
> > -*/
> > -   if (bad && bad->sched == sched)
> > -   /*
> > -* Add at the head of the queue to reflect it was the 
> > earliest
> > -* job extracted.
> > -*/
> > -   list_add(>list, >pending_list);
> > -
> > /*
> >  * Iterate the job list from later to  earlier one and either 
> > deactive
> >  * their HW callbacks or remove them from pending list if they 
> > already
> > --
> > 2.25.1
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Andrey Grodzovsky]

On 2021-08-18 10:02 a.m., Alex Deucher wrote:


+ dri-devel

Since scheduler is a shared component, please add dri-devel on all
scheduler patches.

On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:

[Why]
for bailing job, this commit will delete it from pending list thus the
bailing job will never have a chance to be resubmitted even in advance
tdr mode.

[How]
after embeded hw_fence into amdgpu_job is done, the race condition that
this commit tries to work around is completely solved.So revert this
commit.
This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
v2:
add dma_fence_get/put() around timedout_job to avoid concurrent delete
during processing timedout_job

Signed-off-by: Jingwen Chen 
---
  drivers/gpu/drm/scheduler/sched_main.c | 23 +--
  1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index a2a953693b45..f9b9b3aefc4a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
  {
 struct drm_gpu_scheduler *sched;
 struct drm_sched_job *job;
+   struct dma_fence *fence;
 enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;

 sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
@@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
*work)

 if (job) {
 /*
-* Remove the bad job so it cannot be freed by concurrent
-* drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
-* is parked at which point it's safe.
+* Get job->s_fence->parent here to avoid concurrent delete 
during
+* processing timedout_job
  */
-   list_del_init(>list);
+   fence = dma_fence_get(job->s_fence->parent);



While this is true for amdgpu, it has no meaning for other drivers for 
whom we haven't
done the refactoring of embedding HW fence (parent) into the job 
structure. In fact thinking
about it, unless you do the HW fence embedding for all the drivers using 
the scheduler you cannot

revert this patch or you will just break them.

Andrey



 spin_unlock(>job_list_lock);

 status = job->sched->ops->timedout_job(job);
@@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
 job->sched->ops->free_job(job);
 sched->free_guilty = false;
 }
+   dma_fence_put(fence);
 } else {
 spin_unlock(>job_list_lock);
 }
@@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
struct drm_sched_job *bad)

 kthread_park(sched->thread);

-   /*
-* Reinsert back the bad job here - now it's safe as
-* drm_sched_get_cleanup_job cannot race against us and release the
-* bad job at this point - we parked (waited for) any in progress
-* (earlier) cleanups and drm_sched_get_cleanup_job will not be called
-* now until the scheduler thread is unparked.
-*/
-   if (bad && bad->sched == sched)
-   /*
-* Add at the head of the queue to reflect it was the earliest
-* job extracted.
-*/
-   list_add(>list, >pending_list);
-
 /*
  * Iterate the job list from later to  earlier one and either deactive
  * their HW callbacks or remove them from pending list if they already
--
2.25.1

Re: [PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Alex Deucher]

+ dri-devel

Since scheduler is a shared component, please add dri-devel on all
scheduler patches.

On Wed, Aug 18, 2021 at 7:21 AM Jingwen Chen  wrote:
>
> [Why]
> for bailing job, this commit will delete it from pending list thus the
> bailing job will never have a chance to be resubmitted even in advance
> tdr mode.
>
> [How]
> after embeded hw_fence into amdgpu_job is done, the race condition that
> this commit tries to work around is completely solved.So revert this
> commit.
> This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> v2:
> add dma_fence_get/put() around timedout_job to avoid concurrent delete
> during processing timedout_job
>
> Signed-off-by: Jingwen Chen 
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 23 +--
>  1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> b/drivers/gpu/drm/scheduler/sched_main.c
> index a2a953693b45..f9b9b3aefc4a 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct 
> *work)
>  {
> struct drm_gpu_scheduler *sched;
> struct drm_sched_job *job;
> +   struct dma_fence *fence;
> enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
>
> sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> @@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
> *work)
>
> if (job) {
> /*
> -* Remove the bad job so it cannot be freed by concurrent
> -* drm_sched_cleanup_jobs. It will be reinserted back after 
> sched->thread
> -* is parked at which point it's safe.
> +* Get job->s_fence->parent here to avoid concurrent delete 
> during
> +* processing timedout_job
>  */
> -   list_del_init(>list);
> +   fence = dma_fence_get(job->s_fence->parent);
> spin_unlock(>job_list_lock);
>
> status = job->sched->ops->timedout_job(job);
> @@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct 
> *work)
> job->sched->ops->free_job(job);
> sched->free_guilty = false;
> }
> +   dma_fence_put(fence);
> } else {
> spin_unlock(>job_list_lock);
> }
> @@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
> struct drm_sched_job *bad)
>
> kthread_park(sched->thread);
>
> -   /*
> -* Reinsert back the bad job here - now it's safe as
> -* drm_sched_get_cleanup_job cannot race against us and release the
> -* bad job at this point - we parked (waited for) any in progress
> -* (earlier) cleanups and drm_sched_get_cleanup_job will not be called
> -* now until the scheduler thread is unparked.
> -*/
> -   if (bad && bad->sched == sched)
> -   /*
> -* Add at the head of the queue to reflect it was the earliest
> -* job extracted.
> -*/
> -   list_add(>list, >pending_list);
> -
> /*
>  * Iterate the job list from later to  earlier one and either deactive
>  * their HW callbacks or remove them from pending list if they already
> --
> 2.25.1
>

Re: [PATCH v2] drm/amd/display: Fix two cursor duplication when using overlay

[Rodrigo Siqueira]

Hi Simon,

On 08/18, Simon Ser wrote:
> Hm. This patch causes a regression for me. I was using primary + overlay
> not covering the whole primary plane + cursor before. This patch breaks it.

Which branch are you using? Recently, I reverted part of that patch,
see:

  Revert "drm/amd/display: Fix overlay validation by considering cursors"
 
> This patch makes the overlay plane very useless for me, because the primary
> plane is always under the overlay plane.

I'm curious about your use case with overlay planes. Could you help me
to understand it better? If possible, describe:

1. Context and scenario
2. Compositor
3. Kernel version
4. If you know which IGT test describe your test?

I'm investigating overlay issues in our driver, and a userspace
perspective might help me.

> > Basically, we cannot draw the cursor at the same size and position on
> > two separated pipes since it uses extra bandwidth and DML only run
> > with one cursor.
> 
> I don't understand this limitation. Why would it be necessary to draw the
> cursor on two separate pipes? Isn't it only necessary to draw it once on
> the overlay pipe, and not draw it on the primary pipe?

I will try to provide some background. Harry and Nick, feel free to
correct me or add extra information.

In the amdgpu driver and from the DRM perspective, we expose cursors as
a plane, but we don't have a real plane dedicated to cursors from the
hardware perspective. We have part of our HUPB handling cursors (see
commit "drm/amd/display: Add DCN3.1 DCHHUB" for a hardware block
overview), which requires a different way to deal with the cursor plane
since they are not planes in the hardware. As a result, we have some
limitations, such as not support multiple cursors with overlay; to
support this, we need to deal with these aspects:

1. We need to make multiple cursor match in the same position and size.
Again, keep in mind that cursors are handled in the HUBP, which is the
first part of our pipe, and it is not a plane.

2. Fwiu, our Display Mode Library (DML), has gaps with multiple cursor
support, which can lead to bandwidth problems such as underflow. Part of
these limitations came from DCN1.0; the new ASIC probably can support
multiple cursors without issues.

Additionally, we fully support a strategy named underlay, which inverts
the logic around the overlay. The idea is to put the DE in the overlay
plane covering the entire screen and the other fb in the primary plane
behind the overlay (DE); this can be useful for playback video
scenarios.

Best Regards

-- 
Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH] Revert "drm/scheduler: Avoid accessing freed bad job."

[Jingwen Chen]

On Tue Aug 17, 2021 at 03:43:58PM +0200, Christian König wrote:
> 
> 
> Am 17.08.21 um 15:37 schrieb Andrey Grodzovsky:
> > On 2021-08-17 12:28 a.m., Jingwen Chen wrote:
> > > [Why]
> > > for bailing job, this commit will delete it from pending list thus the
> > > bailing job will never have a chance to be resubmitted even in advance
> > > tdr mode.
> > > 
> > > [How]
> > > after embeded hw_fence into amdgpu_job is done, the race condition that
> > > this commit tries to work around is completely solved.So revert this
> > > commit.
> > > This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
> > 
> > 
> > Can you elaborate please how this solves the race ?
> > As far as I see and  with this patch reverted, in drm_sched_job_timedout
> > you get a pointer to next job to process in timed out handler,
> > immediately
> > next this job is actually finished and it's fence signaled, this in turn
> > triggers
> > drm_sched_get_cleanup_job which fetches this job and returns to
Hi Andrey,

if drm_sched_job_timedout is triggered first, drm_sched_get_cleanup_job will 
return
NULL when the timeout worker is running according to this code:
if ((sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!cancel_delayed_work(>work_tdr)) ||
kthread_should_park())
return NULL;

But yes a dma_fence_get(job->s_fence->parent) is needed before
processing timedout_job. When the bad job is signaled just before
processing, the amdgpu_fence_free can be triggered by the dma_fence_put() of HW 
fence.
And if I'm understanding this race condition correctly, the spin_lock is
still needed here to avoid the drm_sched_get_cleanup_job get the
spin_lock first and then enter the tdr work.
> > drm_sched_main
> > which in turn call free_job callabck->...->amdgpu_fence_free which frees
> > the job
> > from the HW dma_fence release callback. After that you proceed with a
> > freed job
> > in timed out handler.
> > 
> > If you could take the HW fence reference from drm_sched_job_timedout
> > before
> > starting processing then yes, I think it would work.
> 
> Yes, precisely that's what I had in mind as well and seems to be missing
> from this patch.
> 
> Regards,
> Christian.
> 
> > 
> > Andrey
> > 
> > 
> > > 
> > > Signed-off-by: Jingwen Chen 
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 27 --
> > >   1 file changed, 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index a2a953693b45..31d1176d939f 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -317,21 +317,10 @@ static void drm_sched_job_timedout(struct
> > > work_struct *work)
> > >   enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
> > >     sched = container_of(work, struct drm_gpu_scheduler,
> > > work_tdr.work);
> > > -
> > > -    /* Protects against concurrent deletion in
> > > drm_sched_get_cleanup_job */
> > > -    spin_lock(>job_list_lock);
> > >   job = list_first_entry_or_null(>pending_list,
> > >  struct drm_sched_job, list);
> > >     if (job) {
> > > -    /*
> > > - * Remove the bad job so it cannot be freed by concurrent
> > > - * drm_sched_cleanup_jobs. It will be reinserted back after
> > > sched->thread
> > > - * is parked at which point it's safe.
> > > - */
> > > -    list_del_init(>list);
> > > -    spin_unlock(>job_list_lock);
> > > -
> > >   status = job->sched->ops->timedout_job(job);
> > >     /*
> > > @@ -342,8 +331,6 @@ static void drm_sched_job_timedout(struct
> > > work_struct *work)
> > >   job->sched->ops->free_job(job);
> > >   sched->free_guilty = false;
> > >   }
> > > -    } else {
> > > -    spin_unlock(>job_list_lock);
> > >   }
> > >     if (status != DRM_GPU_SCHED_STAT_ENODEV) {
> > > @@ -392,20 +379,6 @@ void drm_sched_stop(struct drm_gpu_scheduler
> > > *sched, struct drm_sched_job *bad)
> > >     kthread_park(sched->thread);
> > >   -    /*
> > > - * Reinsert back the bad job here - now it's safe as
> > > - * drm_sched_get_cleanup_job cannot race against us and release the
> > > - * bad job at this point - we parked (waited for) any in progress
> > > - * (earlier) cleanups and drm_sched_get_cleanup_job will not be
> > > called
> > > - * now until the scheduler thread is unparked.
> > > - */
> > > -    if (bad && bad->sched == sched)
> > > -    /*
> > > - * Add at the head of the queue to reflect it was the earliest
> > > - * job extracted.
> > > - */
> > > -    list_add(>list, >pending_list);
> > > -
> > >   /*
> > >    * Iterate the job list from later to  earlier one and either
> > > deactive
> > >    * their HW callbacks or remove them from pending list if they
> > > already
> 

Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region

[Lazar, Lijo]

On 8/18/2021 11:34 AM, Kees Cook wrote:

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.

Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.

"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.

Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Hawking Zhang 
Cc: Feifei Xu 
Cc: Lijo Lazar 
Cc: Likun Gao 
Cc: Jiawei Gu 
Cc: Evan Quan 
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook 
Acked-by: Alex Deucher 
Link: 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C92b8d2f072f0444b9f8508d9620f6971%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648640625729624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rKh5LUXCRUsorYM3kSpG2tkB%2Fczwl9I9EBnWBCtbg6Q%3Dreserved=0
---
  drivers/gpu/drm/amd/include/atomfirmware.h   |  9 -
  .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h|  3 ++-
  drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h  |  3 ++-
  .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h   |  3 ++-


Hi Kees,

The headers which define these structs are firmware/VBIOS interfaces and 
are picked directly from those components. There are difficulties in 
grouping them to structs at the original source as that involves other 
component changes.


The driver_if_* files updates are frequent and it is error prone to 
manually group them each time we pick them for any update. Our usage of 
memcpy in this way is restricted only to a very few places.


As another option - is it possible to have a helper function/macro like 
memcpy_fortify() which takes the extra arguments and does the extra 
compile time checks? We will use the helper whenever we have such kind 
of usage.


Thanks,
Lijo


  drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c|  6 +++---
  drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c  | 12 
  drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c   |  6 +++---
  7 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h 
b/drivers/gpu/drm/amd/include/atomfirmware.h
index 44955458fe38..7bf3edf15410 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5
  {
struct   atom_common_table_header  table_header;
  // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,
  // I2C Control
struct smudpm_i2c_controller_config_v2  I2cControllers[8];
  
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5

uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16 
fractional bits (Q16.16)

uint32_t BoardReserved[9];

-
+  );
  };
  
  struct atom_smc_dpm_info_v4_6

@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6
// section: board parameters
uint32_t i2c_padding[3];   // old i2c control are moved to new area
  
+  struct_group(dpm_info,

uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will 
request. multiple steps are taken if voltage change exceeds this value.
uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will 
request. multiple steps are taken if voltage change exceeds this value.
  
@@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6
  
// reserved

uint32_t   boardreserved[10];
+  );
  };
  
  struct atom_smc_dpm_info_v4_7

  {
struct   atom_common_table_header  table_header;
  // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,
  // I2C Control
struct smudpm_i2c_controller_config_v2  I2cControllers[8];
  
@@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7

uint8_t  Padding8_Psi2;
  
uint32_t BoardReserved[5];

+  );
  };
  
  struct smudpm_i2c_controller_config_v3

@@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10
struct   atom_common_table_header  table_header;
  
// SECTION: BOARD PARAMETERS

+  struct_group(dpm_info,
// Telemetry Settings
uint16_t GfxMaxCurrent; // in Amps
uint8_t   GfxOffset; // in Amps
@@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10
uint16_t spare5;
  
uint32_t reserved[16];

+  );
  };
  
  /*

diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h 

[PATCH v2] Revert "drm/scheduler: Avoid accessing freed bad job."

[Jingwen Chen]

[Why]
for bailing job, this commit will delete it from pending list thus the
bailing job will never have a chance to be resubmitted even in advance
tdr mode.

[How]
after embeded hw_fence into amdgpu_job is done, the race condition that
this commit tries to work around is completely solved.So revert this
commit.
This reverts commit 135517d3565b48f4def3b1b82008bc17eb5d1c90.
v2:
add dma_fence_get/put() around timedout_job to avoid concurrent delete
during processing timedout_job

Signed-off-by: Jingwen Chen 
---
 drivers/gpu/drm/scheduler/sched_main.c | 23 +--
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index a2a953693b45..f9b9b3aefc4a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -314,6 +314,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
 {
struct drm_gpu_scheduler *sched;
struct drm_sched_job *job;
+   struct dma_fence *fence;
enum drm_gpu_sched_stat status = DRM_GPU_SCHED_STAT_NOMINAL;
 
sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
@@ -325,11 +326,10 @@ static void drm_sched_job_timedout(struct work_struct 
*work)
 
if (job) {
/*
-* Remove the bad job so it cannot be freed by concurrent
-* drm_sched_cleanup_jobs. It will be reinserted back after 
sched->thread
-* is parked at which point it's safe.
+* Get job->s_fence->parent here to avoid concurrent delete 
during
+* processing timedout_job
 */
-   list_del_init(>list);
+   fence = dma_fence_get(job->s_fence->parent);
spin_unlock(>job_list_lock);
 
status = job->sched->ops->timedout_job(job);
@@ -342,6 +342,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
job->sched->ops->free_job(job);
sched->free_guilty = false;
}
+   dma_fence_put(fence);
} else {
spin_unlock(>job_list_lock);
}
@@ -392,20 +393,6 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
struct drm_sched_job *bad)
 
kthread_park(sched->thread);
 
-   /*
-* Reinsert back the bad job here - now it's safe as
-* drm_sched_get_cleanup_job cannot race against us and release the
-* bad job at this point - we parked (waited for) any in progress
-* (earlier) cleanups and drm_sched_get_cleanup_job will not be called
-* now until the scheduler thread is unparked.
-*/
-   if (bad && bad->sched == sched)
-   /*
-* Add at the head of the queue to reflect it was the earliest
-* job extracted.
-*/
-   list_add(>list, >pending_list);
-
/*
 * Iterate the job list from later to  earlier one and either deactive
 * their HW callbacks or remove them from pending list if they already
-- 
2.25.1

Re: [PATCH v2] drm/amd/display: Fix two cursor duplication when using overlay

[Simon Ser]

Hm. This patch causes a regression for me. I was using primary + overlay
not covering the whole primary plane + cursor before. This patch breaks it.

This patch makes the overlay plane very useless for me, because the primary
plane is always under the overlay plane.

> Basically, we cannot draw the cursor at the same size and position on
> two separated pipes since it uses extra bandwidth and DML only run
> with one cursor.

I don't understand this limitation. Why would it be necessary to draw the
cursor on two separate pipes? Isn't it only necessary to draw it once on
the overlay pipe, and not draw it on the primary pipe?

Re: [PATCH] drm/amdgpu: fix radv vulkan fps drop after s3 resume

[Mohan Marimuthu, Yogesh]

[Public]

[Why]
After s3, In radv there is huge fps drop in games. This is because
when memory is allocated using radv_amdgpu_winsys_bo_create()
with both AMDGPU_GEM_DOMAIN_VRAM and AMDGPU_GEM_DOMAIN_GTT domains
set, the kernel memory management after resume fails to move the data
back to VRAM. In kernel memory management, ttm_bo_mem_compat()
function returns true and hence data is not moved back to VRAM.

[How]
Implement the idea suggested by Christian Koenig. During suspend
move the data to system RAM instead of GTT. Due to this ttm_bo_mem_compat()
will return false and data will be moved back to VRAM.

Suggested-by: Christian König mailto:christian.koe...@amd.com
Signed-off-by: Yogesh mohan marimuthu mailto:yogesh.mohanmarimu...@amd.com
Reviewed-by: Christian König mailto:christian.koe...@amd.com
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 446943e32..44ec59998 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -136,7 +136,13 @@ static void amdgpu_evict_flags(struct ttm_buffer_object 
*bo,
return;
 
case TTM_PL_VRAM:
-   if (!adev->mman.buffer_funcs_enabled) {
+   /* Move data to system memory for S3 so that while resume
+* ttm_bo_mem_compat() will return false and data will be
+* moved back to VRAM also in case of bo with both
+* AMDGPU_GEM_DOMAIN_GTT and AMDGPU_GEM_DOMAIN_VRAM domain
+* set in bo->preferred_domains.
+*/
+   if (!adev->mman.buffer_funcs_enabled || adev->in_s3) {
/* Move to system memory */
amdgpu_bo_placement_from_domain(abo, 
AMDGPU_GEM_DOMAIN_CPU);
} else if (!amdgpu_gmc_vram_full_visible(>gmc) &&
-- 
2.25.1

Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE on suspend

[Lazar, Lijo]

On 8/18/2021 2:15 PM, Quan, Evan wrote:

[AMD Official Use Only]




-Original Message-
From: Lazar, Lijo 
Sent: Tuesday, August 17, 2021 6:09 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org; Liu,
Leo 
Cc: Deucher, Alexander ; Chen, Guchun
; Pan, Xinhui 
Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE
on suspend



On 8/17/2021 3:13 PM, Quan, Evan wrote:

[AMD Official Use Only]

+Leo to share his insights


-Original Message-
From: Lazar, Lijo 
Sent: Tuesday, August 17, 2021 3:28 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Chen, Guchun
; Pan, Xinhui 
Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12

UVD/VCE

on suspend



On 8/17/2021 12:10 PM, Evan Quan wrote:

If the powergating of UVD/VCE is in process, wait for its completion
before proceeding(suspending). This can fix some hangs observed on
suspending when UVD/VCE still using(e.g. issue "pm-suspend" when
video is still playing).

Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
Signed-off-by: Evan Quan 
Signed-off-by: xinhui pan 
---
drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 +
drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 5 +
2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index 4eebf973a065..2fdce572baeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -554,6 +554,11 @@ static int uvd_v6_0_suspend(void *handle)
int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;

+   /*
+* If the powergating is in process, wait for its completion.
+*/
+   flush_delayed_work(>uvd.idle_work);
+

If running idle is a prerequisite before going to suspend, then
something else is missing here.

Otherwise, the hang looks more like a pending work launched after
hardware is suspended and trying to access hardware. As the hardware
is going to be suspended anyway, doesn't it work with
cancel_delayed_work_sync - making sure that nothing is going to be
launched later to access hardware?

[Quan, Evan] The reason we chose flush_delayed_work instead of

cancel_delayed_work_sync is we think those operations performed in
idle_work(dpm disablement, powergating) seems needed considering the
action is 'suspend'. So, instead of "cancel", maybe waiting for them
completion is more proper.

But it will do so only if the work is scheduled - so it doesn't seem to be a
prerequisite for suspend. If it was a prerequisite, then the existing code is
missing that (so that it gets done for all cases).

[Quan, Evan] Just confirmed that cancel_delayed_work_sync() alone cannot work.
In fact, our current driver already get cancel_delayed_work_sync() called(e.g. 
in amdgpu_uvd_suspend() on the path of uvd_v6_0_suspend).


Thanks Evan for checking this further.

If cancel_delayed_work_sync() is called in amdgpu_uvd_suspend(), then it 
means that originally executing idle_work was not considered as a 
prerequisite for suspending.


_uvd_suspend() is called "after" uvd_v6_0_hw_fini(adev), that maybe a 
little too late.



To get the issue fixed, it has to be either:
1. flush_delayed_work()
Or
2. cancel_delayed_work_sync + amdgpu_dpm_enable_uvd/vce(adev, false) (the job 
performed in idle work)


At minimum, it proves that disabling dpm is required for suspend.


Btw, I do not think flush_delayed_work() is an incomplete fix. Since the UVD/VCE idle 
work is appended on ring->funcs->end_use.
So, as long as the UVD/VCE ring used and ended(it will since at least there is 
ring/ib test), there will be a chance to get the work waited and completed.


I agree that it fixes the issue, only thing is someone should look 
further to provide the right sequence of uvd suspend.


It doesn't give a root cause/right sequence - it only tells that forcing 
to execute idle_work fixes the problem probably due to an extra delay 
added by increased execution time or it disables DPM or something else. 
Someone should confirm that all of idle_work or a part of it (as dpm 
disable) is required for proper suspend sequence.


That said, I don't have any objections to this fix either. If there are 
other things, probably fix it this way and move on.


Thanks,
Lijo



BR
Evan




Then this may be a potential issue for other suspend calls also where
work is pending to be launched when hardware is suspended.

[Quan, Evan] Do you mean we need to check whether there is similar issue

for other IPs?




Yes, if there are cases where other IPs may schedule a delayed work and call
hw_fini without cancelling the work.

Thanks,
Lijo


BR
Evan


Thanks,
Lijo


r = uvd_v6_0_hw_fini(adev);
if (r)
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
index 6d9108fa22e0..f0adecd5ec0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
@@ -503,6 

Re: [PATCH] drm/amdgpu: fix radv vulkan fps drop after s3 resume

[Christian König]

Am 17.08.21 um 20:26 schrieb Mohan Marimuthu, Yogesh:


[Public]


[Why]

After s3, In radv there is huge fps drop in games. This is because

when memory is allocated using radv_amdgpu_winsys_bo_create()

with both AMDGPU_GEM_DOMAIN_VRAM and AMDGPU_GEM_DOMAIN_GTT domains

set, the kernel memory management after resume fails to move the data

back to VRAM. In kernel memory management, ttm_bo_mem_compat()

function returns true and hence data is not moved back to VRAM.

[How]

Implement the idea suggested by Christian Koenig. During suspend

move the data to system RAM instead of GTT. Due to this 
ttm_bo_mem_compat()


will return false and data will be moved back to VRAM.

Signed-off-by: Christian König christian.koe...@amd.com 





Suggested-by: would be better here since I wasn't involved in the coding.

Signed-off-by: Yogesh mohan marimuthu yogesh.mohanmarimu...@amd.com 





Reviewed-by: Christian König 


---

drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 +++-

1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c


index 446943e32..44ec59998 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

@@ -136,7 +136,13 @@ static void amdgpu_evict_flags(struct 
ttm_buffer_object *bo,


    return;

   case TTM_PL_VRAM:

-    if (!adev->mman.buffer_funcs_enabled) {

+   /* Move data to system memory for S3 so 
that while resume


+   * ttm_bo_mem_compat() will return false 
and data will be


+   * moved back to VRAM also in case of bo 
with both


+   * AMDGPU_GEM_DOMAIN_GTT and 
AMDGPU_GEM_DOMAIN_VRAM domain


+   * set in bo->preferred_domains.

+   */

+   if (!adev->mman.buffer_funcs_enabled || 
adev->in_s3) {


/* Move to system memory */

amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_CPU);

    } else if 
(!amdgpu_gmc_vram_full_visible(>gmc) &&


--

2.25.1

RE: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE on suspend

[Quan, Evan]

[AMD Official Use Only]



> -Original Message-
> From: Lazar, Lijo 
> Sent: Tuesday, August 17, 2021 6:09 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org; Liu,
> Leo 
> Cc: Deucher, Alexander ; Chen, Guchun
> ; Pan, Xinhui 
> Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE
> on suspend
> 
> 
> 
> On 8/17/2021 3:13 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> > +Leo to share his insights
> >
> >> -Original Message-
> >> From: Lazar, Lijo 
> >> Sent: Tuesday, August 17, 2021 3:28 PM
> >> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander ; Chen, Guchun
> >> ; Pan, Xinhui 
> >> Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12
> UVD/VCE
> >> on suspend
> >>
> >>
> >>
> >> On 8/17/2021 12:10 PM, Evan Quan wrote:
> >>> If the powergating of UVD/VCE is in process, wait for its completion
> >>> before proceeding(suspending). This can fix some hangs observed on
> >>> suspending when UVD/VCE still using(e.g. issue "pm-suspend" when
> >>> video is still playing).
> >>>
> >>> Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
> >>> Signed-off-by: Evan Quan 
> >>> Signed-off-by: xinhui pan 
> >>> ---
> >>>drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 +
> >>>drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 5 +
> >>>2 files changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> >>> b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> >>> index 4eebf973a065..2fdce572baeb 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> >>> @@ -554,6 +554,11 @@ static int uvd_v6_0_suspend(void *handle)
> >>>   int r;
> >>>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>>
> >>> + /*
> >>> +  * If the powergating is in process, wait for its completion.
> >>> +  */
> >>> + flush_delayed_work(>uvd.idle_work);
> >>> +
> >> If running idle is a prerequisite before going to suspend, then
> >> something else is missing here.
> >>
> >> Otherwise, the hang looks more like a pending work launched after
> >> hardware is suspended and trying to access hardware. As the hardware
> >> is going to be suspended anyway, doesn't it work with
> >> cancel_delayed_work_sync - making sure that nothing is going to be
> >> launched later to access hardware?
> > [Quan, Evan] The reason we chose flush_delayed_work instead of
> cancel_delayed_work_sync is we think those operations performed in
> idle_work(dpm disablement, powergating) seems needed considering the
> action is 'suspend'. So, instead of "cancel", maybe waiting for them
> completion is more proper.
> 
> But it will do so only if the work is scheduled - so it doesn't seem to be a
> prerequisite for suspend. If it was a prerequisite, then the existing code is
> missing that (so that it gets done for all cases).
[Quan, Evan] Just confirmed that cancel_delayed_work_sync() alone cannot work. 
In fact, our current driver already get cancel_delayed_work_sync() called(e.g. 
in amdgpu_uvd_suspend() on the path of uvd_v6_0_suspend).
To get the issue fixed, it has to be either:
1. flush_delayed_work()
Or 
2. cancel_delayed_work_sync + amdgpu_dpm_enable_uvd/vce(adev, false) (the job 
performed in idle work)

Btw, I do not think flush_delayed_work() is an incomplete fix. Since the 
UVD/VCE idle work is appended on ring->funcs->end_use.
So, as long as the UVD/VCE ring used and ended(it will since at least there is 
ring/ib test), there will be a chance to get the work waited and completed.

BR
Evan
> 
> >>
> >> Then this may be a potential issue for other suspend calls also where
> >> work is pending to be launched when hardware is suspended.
> > [Quan, Evan] Do you mean we need to check whether there is similar issue
> for other IPs?
> >
> 
> Yes, if there are cases where other IPs may schedule a delayed work and call
> hw_fini without cancelling the work.
> 
> Thanks,
> Lijo
> 
> > BR
> > Evan
> >>
> >> Thanks,
> >> Lijo
> >>
> >>>   r = uvd_v6_0_hw_fini(adev);
> >>>   if (r)
> >>>   return r;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >>> b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >>> index 6d9108fa22e0..f0adecd5ec0b 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >>> @@ -503,6 +503,11 @@ static int vce_v3_0_suspend(void *handle)
> >>>   int r;
> >>>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>>
> >>> + /*
> >>> +  * If the powergating is in process, wait for its completion.
> >>> +  */
> >>> + flush_delayed_work(>vce.idle_work);
> >>> +
> >>>   r = vce_v3_0_hw_fini(adev);
> >>>   if (r)
> >>>   return r;
> >>>

Re: [PATCH] drm/amd/amdgpu:flush ttm delayed work before cancel_sync

[Christian König]

Am 17.08.21 um 11:50 schrieb YuBiao Wang:

[Why]
In some cases when we unload driver, warning call trace
will show up in vram_mgr_fini which claims that LRU is not empty, caused
by the ttm bo inside delay deleted queue.

[How]
We should flush delayed work to make sure the delay deleting is done.

Signed-off-by: YuBiao Wang 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 4d266c40382c..0b5764aa98a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3824,8 +3824,10 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
  {
dev_info(adev->dev, "amdgpu: finishing device.\n");
flush_delayed_work(>delayed_init_work);
-   if (adev->mman.initialized)
+   if (adev->mman.initialized) {
+   flush_delayed_work(>mman.bdev.wq);
ttm_bo_lock_delayed_workqueue(>mman.bdev);
+   }


If you flush the delayed work you can drop the call to 
ttm_bo_lock_delayed_workqueue().


It would also be nice to have to wrap this into a 
ttm_bo_flush_delayed_workqueue() function.


Apart from that looks good to me,
Christian.


adev->shutdown = true;
  
  	/* make sure IB test finished before entering exclusive mode

[PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region

[Kees Cook]

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.

Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.

"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.

Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Hawking Zhang 
Cc: Feifei Xu 
Cc: Lijo Lazar 
Cc: Likun Gao 
Cc: Jiawei Gu 
Cc: Evan Quan 
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook 
Acked-by: Alex Deucher 
Link: 
https://lore.kernel.org/lkml/cadnq5_npb8uyvd+r4uhgf-w8-cqj3joodjvijr_y9w9wqj7...@mail.gmail.com
---
 drivers/gpu/drm/amd/include/atomfirmware.h   |  9 -
 .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h|  3 ++-
 drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h  |  3 ++-
 .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h   |  3 ++-
 drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c|  6 +++---
 drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c  | 12 
 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c   |  6 +++---
 7 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h 
b/drivers/gpu/drm/amd/include/atomfirmware.h
index 44955458fe38..7bf3edf15410 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5
 {
   struct   atom_common_table_header  table_header;
 // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,
 // I2C Control
   struct smudpm_i2c_controller_config_v2  I2cControllers[8];
 
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5
   uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16 
fractional bits (Q16.16)
   
   uint32_t BoardReserved[9];
-
+  );
 };
 
 struct atom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6
   // section: board parameters
   uint32_t i2c_padding[3];   // old i2c control are moved to new area
 
+  struct_group(dpm_info,
   uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will 
request. multiple steps are taken if voltage change exceeds this value.
   uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will 
request. multiple steps are taken if voltage change exceeds this value.
 
@@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6
 
   // reserved
   uint32_t   boardreserved[10];
+  );
 };
 
 struct atom_smc_dpm_info_v4_7
 {
   struct   atom_common_table_header  table_header;
 // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,
 // I2C Control
   struct smudpm_i2c_controller_config_v2  I2cControllers[8];
 
@@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7
   uint8_t  Padding8_Psi2;
 
   uint32_t BoardReserved[5];
+  );
 };
 
 struct smudpm_i2c_controller_config_v3
@@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10
   struct   atom_common_table_header  table_header;
 
   // SECTION: BOARD PARAMETERS
+  struct_group(dpm_info,
   // Telemetry Settings
   uint16_t GfxMaxCurrent; // in Amps
   uint8_t   GfxOffset; // in Amps
@@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10
   uint16_t spare5;
 
   uint32_t reserved[16];
+  );
 };
 
 /* 
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h 
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
index 43d43d6addc0..8093a98800c3 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
   // SECTION: BOARD PARAMETERS
 
   // SVI2 Board Parameters
+  struct_group(v4_6,
   uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
   uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will 
request. Multiple steps are taken if voltage change exceeds this value.
 
@@ -728,10 +729,10 @@ typedef struct {
   uint32_t BoardVoltageCoeffB;// decode by /1000
 
   uint32_t BoardReserved[7];
+  );
 
   // Padding for MMHUB - do not modify this
   uint32_t MmHubPadding[8]; // SMU internal use
-
 } PPTable_t;
 
 typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h 
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
index 04752ade1016..0b4e6e907e95 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
+++