Re: [PATCH 2/2] drm/ttm: Double check mem_type of BO while eviction

[Christian König]

Am 10.11.21 um 05:31 schrieb xinhui pan:

BO might sit in a wrong lru list as there is a small period of memory
moving and lru list updating.

Lets skip eviction if we hit such mismatch.

Suggested-by: Christian König 
Signed-off-by: xinhui pan 


Reviewed-by: Christian König  for the series.

Going to add a CC: stable to the second patch and push it to drm-misc-fixes.

Thanks,
Christian.


---
  drivers/gpu/drm/ttm/ttm_bo.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e307004f0b28..0137a302f261 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -594,7 +594,8 @@ static bool ttm_bo_evict_swapout_allowable(struct 
ttm_buffer_object *bo,
*busy = !ret;
}
  
-	if (ret && place && !bo->bdev->funcs->eviction_valuable(bo, place)) {

+   if (ret && place && (bo->resource->mem_type != place->mem_type ||
+   !bo->bdev->funcs->eviction_valuable(bo, place))) {
ret = false;
if (*locked) {
dma_resv_unlock(bo->base.resv);

[RFC 1/2] ACPI: platform_profile: Add support for notification chains

[Mario Limonciello]

Allow other drivers to initialize relative to current active
profile and react to platform profile changes.

Drivers wishing to utilize this should register for notification
at module load and unregister when unloading.

Notifications will come in the from a notifier call.

Signed-off-by: Mario Limonciello 
---
 drivers/acpi/platform_profile.c  | 52 +++-
 include/linux/platform_profile.h | 10 ++
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index d418462ab791..ca5d962020a2 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -21,6 +21,24 @@ static const char * const profile_names[] = {
[PLATFORM_PROFILE_PERFORMANCE] = "performance",
 };
 static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
+static BLOCKING_NOTIFIER_HEAD(platform_profile_chain_head);
+
+int platform_profile_register_notifier(struct notifier_block *nb)
+{
+   return blocking_notifier_chain_register(_profile_chain_head, 
nb);
+}
+EXPORT_SYMBOL_GPL(platform_profile_register_notifier);
+
+int platform_profile_unregister_notifier(struct notifier_block *nb)
+{
+   return blocking_notifier_chain_unregister(_profile_chain_head, 
nb);
+}
+EXPORT_SYMBOL_GPL(platform_profile_unregister_notifier);
+
+static void platform_profile_call_notifier(unsigned long action, void *data)
+{
+   blocking_notifier_call_chain(_profile_chain_head, action, 
data);
+}
 
 static ssize_t platform_profile_choices_show(struct device *dev,
struct device_attribute *attr,
@@ -49,11 +67,8 @@ static ssize_t platform_profile_choices_show(struct device 
*dev,
return len;
 }
 
-static ssize_t platform_profile_show(struct device *dev,
-   struct device_attribute *attr,
-   char *buf)
+int platform_profile_get(enum platform_profile_option *profile)
 {
-   enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
int err;
 
err = mutex_lock_interruptible(_lock);
@@ -65,15 +80,28 @@ static ssize_t platform_profile_show(struct device *dev,
return -ENODEV;
}
 
-   err = cur_profile->profile_get(cur_profile, );
+   err = cur_profile->profile_get(cur_profile, profile);
mutex_unlock(_lock);
if (err)
return err;
 
/* Check that profile is valid index */
-   if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names
+   if (WARN_ON((*profile < 0) || (*profile >= ARRAY_SIZE(profile_names
return -EIO;
 
+   return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_get);
+
+static ssize_t platform_profile_show(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
+   int ret = platform_profile_get();
+
+   if (ret)
+   return ret;
return sysfs_emit(buf, "%s\n", profile_names[profile]);
 }
 
@@ -106,8 +134,10 @@ static ssize_t platform_profile_store(struct device *dev,
}
 
err = cur_profile->profile_set(cur_profile, i);
-   if (!err)
+   if (!err) {
sysfs_notify(acpi_kobj, NULL, "platform_profile");
+   platform_profile_call_notifier(PLATFORM_PROFILE_CHANGED, );
+   }
 
mutex_unlock(_lock);
if (err)
@@ -130,9 +160,17 @@ static const struct attribute_group platform_profile_group 
= {
 
 void platform_profile_notify(void)
 {
+   enum platform_profile_option profile;
+   int ret;
+
if (!cur_profile)
return;
sysfs_notify(acpi_kobj, NULL, "platform_profile");
+   ret = platform_profile_get();
+   if (ret)
+   return;
+   platform_profile_call_notifier(PLATFORM_PROFILE_CHANGED, );
+
 }
 EXPORT_SYMBOL_GPL(platform_profile_notify);
 
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index a6329003aee7..dca9d47e18eb 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -11,6 +11,8 @@
 
 #include 
 
+struct notifier_block;
+
 /*
  * If more options are added please update profile_names array in
  * platform_profile.c and sysfs-platform_profile documentation.
@@ -37,5 +39,13 @@ struct platform_profile_handler {
 int platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_remove(void);
 void platform_profile_notify(void);
+int platform_profile_get(enum platform_profile_option *profile);
+
+int platform_profile_register_notifier(struct notifier_block *nb);
+int platform_profile_unregister_notifier(struct notifier_block *nb);
+
+enum platform_profile_notifier_actions {
+   PLATFORM_PROFILE_CHANGED,
+};
 
 #endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.25.1

[RFC 2/2] drm/amd/pm: Add support for reacting to platform profile notification

[Mario Limonciello]

Various drivers provide platform profile support to let users set a hint
in their GUI whether they want to run in a high performance, low battery
life or balanced configuration.

Drivers that provide this typically work with the firmware on their system
to configure hardware.  In the case of AMDGPU however, the notification
path doesn't come through firmware and can instead be provided directly
to the driver from a notification chain.

Use the information of the newly selected profile to tweak
`dpm_force_performance_level` to that profile IFF the user hasn't manually
selected `manual` or any other `profile_*` options.

Signed-off-by: Mario Limonciello 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h |   3 +
 drivers/gpu/drm/amd/pm/amdgpu_pm.c  | 105 +++-
 2 files changed, 90 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index b85b67a88a3d..27b0be23b6ac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1097,6 +1097,9 @@ struct amdgpu_device {
 
struct amdgpu_reset_control *reset_cntl;
uint32_t
ip_versions[HW_ID_MAX][HWIP_MAX_INSTANCE];
+
+   /* platform profile notifications */
+   struct notifier_block   platform_profile_notifier;
 };
 
 static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 41472ed99253..33fc52b90d4c 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "hwmgr.h"
@@ -200,6 +201,33 @@ static ssize_t amdgpu_set_power_dpm_state(struct device 
*dev,
return count;
 }
 
+static int amdgpu_get_forced_level(struct device *dev, enum 
amd_dpm_forced_level *level)
+{
+   struct drm_device *ddev = dev_get_drvdata(dev);
+   struct amdgpu_device *adev = drm_to_adev(ddev);
+   int ret;
+
+   if (amdgpu_in_reset(adev))
+   return -EPERM;
+   if (adev->in_suspend && !adev->in_runpm)
+   return -EPERM;
+
+   ret = pm_runtime_get_sync(ddev->dev);
+   if (ret < 0) {
+   pm_runtime_put_autosuspend(ddev->dev);
+   return ret;
+   }
+
+   if (adev->powerplay.pp_funcs->get_performance_level)
+   *level = amdgpu_dpm_get_performance_level(adev);
+   else
+   *level = adev->pm.dpm.forced_level;
+
+   pm_runtime_mark_last_busy(ddev->dev);
+   pm_runtime_put_autosuspend(ddev->dev);
+
+   return 0;
+}
 
 /**
  * DOC: power_dpm_force_performance_level
@@ -264,29 +292,13 @@ static ssize_t 
amdgpu_get_power_dpm_force_performance_level(struct device *dev,
struct 
device_attribute *attr,
char *buf)
 {
-   struct drm_device *ddev = dev_get_drvdata(dev);
-   struct amdgpu_device *adev = drm_to_adev(ddev);
enum amd_dpm_forced_level level = 0xff;
int ret;
 
-   if (amdgpu_in_reset(adev))
-   return -EPERM;
-   if (adev->in_suspend && !adev->in_runpm)
-   return -EPERM;
+   ret = amdgpu_get_forced_level(dev, );
 
-   ret = pm_runtime_get_sync(ddev->dev);
-   if (ret < 0) {
-   pm_runtime_put_autosuspend(ddev->dev);
+   if (ret < 0)
return ret;
-   }
-
-   if (adev->powerplay.pp_funcs->get_performance_level)
-   level = amdgpu_dpm_get_performance_level(adev);
-   else
-   level = adev->pm.dpm.forced_level;
-
-   pm_runtime_mark_last_busy(ddev->dev);
-   pm_runtime_put_autosuspend(ddev->dev);
 
return sysfs_emit(buf, "%s\n",
  (level == AMD_DPM_FORCED_LEVEL_AUTO) ? "auto" :
@@ -405,6 +417,59 @@ static ssize_t 
amdgpu_set_power_dpm_force_performance_level(struct device *dev,
return count;
 }
 
+static void amdgpu_update_profile(struct device *dev, enum 
platform_profile_option *profile)
+{
+   enum amd_dpm_forced_level level;
+   const char *str;
+   int ret;
+
+   ret = amdgpu_get_forced_level(dev, );
+   if (ret < 0)
+   return;
+
+   /* only update profile if we're in fixed modes right now that need 
updating */
+   switch (level) {
+   case AMD_DPM_FORCED_LEVEL_LOW:
+   if (*profile < PLATFORM_PROFILE_BALANCED)
+   return;
+   break;
+   case AMD_DPM_FORCED_LEVEL_HIGH:
+   if (*profile > PLATFORM_PROFILE_BALANCED)
+   return;
+   break;
+   case AMD_DPM_FORCED_LEVEL_AUTO:
+   if (*profile == PLATFORM_PROFILE_BALANCED)
+   return;
+   break;
+   default:
+   dev_dbg(dev, 

[RFC 0/2] Let amdgpu react to platform profile changes

[Mario Limonciello]

Many OEM platform provide a platform profile knob that can be used to make
firmware tunings to the system to allow operating in a higher or lower
performance mode trading off power consumption.

Software like power-profiles-daemon to expose this knob to the UI.

As we know the user's intent to go into power saving or performance mode
from this, we can also let amdgpu react to the change.

This patch series is sent as RFC right now only to amd-gfx, and if it's
a good enough idea will re-send as PATCH to linux-acpi + amd-gfx.

Mario Limonciello (2):
  ACPI: platform_profile: Add support for notification chains
  drm/amd/pm: Add support for reacting to platform profile notification

 drivers/acpi/platform_profile.c |  52 --
 drivers/gpu/drm/amd/amdgpu/amdgpu.h |   3 +
 drivers/gpu/drm/amd/pm/amdgpu_pm.c  | 105 +++-
 include/linux/platform_profile.h|  10 +++
 4 files changed, 145 insertions(+), 25 deletions(-)

-- 
2.25.1

Re: [PATCH] drm/amdgpu: add missed support for UVD IP_VERSION(3, 0, 64)

[Alex Deucher]

Reviewed-by: Alex Deucher 

On Wed, Nov 10, 2021 at 12:16 AM Guchun Chen  wrote:
>
> Fixes: 5b30f206dbd1("drm/amdgpu/amdgpu_vcn: convert to IP version checking")
> Signed-off-by: Flora Cui 
> Signed-off-by: Guchun Chen 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c   | 1 +
>  drivers/gpu/drm/amd/amdgpu/nv.c   | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index a20d21409c95..ff70bc233489 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -882,6 +882,7 @@ static int amdgpu_discovery_set_mm_ip_blocks(struct 
> amdgpu_device *adev)
> break;
> case IP_VERSION(3, 0, 0):
> case IP_VERSION(3, 0, 16):
> +   case IP_VERSION(3, 0, 64):
> case IP_VERSION(3, 1, 1):
> case IP_VERSION(3, 0, 2):
> amdgpu_device_ip_block_add(adev, _v3_0_ip_block);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> index 2658414c503d..4f7c70845785 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> @@ -134,6 +134,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
> adev->vcn.indirect_sram = true;
> break;
> case IP_VERSION(3, 0, 0):
> +   case IP_VERSION(3, 0, 64):
> if (adev->ip_versions[GC_HWIP][0] == IP_VERSION(10, 3, 0))
> fw_name = FIRMWARE_SIENNA_CICHLID;
> else
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> index febc903adf58..59eafa31c626 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -182,6 +182,7 @@ static int nv_query_video_codecs(struct amdgpu_device 
> *adev, bool encode,
>  {
> switch (adev->ip_versions[UVD_HWIP][0]) {
> case IP_VERSION(3, 0, 0):
> +   case IP_VERSION(3, 0, 64):
> if (amdgpu_sriov_vf(adev)) {
> if (encode)
> *codecs = _sc_video_codecs_encode;
> --
> 2.17.1
>

[PATCH] drm/amdgpu: add missed support for UVD IP_VERSION(3, 0, 64)

[Guchun Chen]

Fixes: 5b30f206dbd1("drm/amdgpu/amdgpu_vcn: convert to IP version checking")
Signed-off-by: Flora Cui 
Signed-off-by: Guchun Chen 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c   | 1 +
 drivers/gpu/drm/amd/amdgpu/nv.c   | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index a20d21409c95..ff70bc233489 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -882,6 +882,7 @@ static int amdgpu_discovery_set_mm_ip_blocks(struct 
amdgpu_device *adev)
break;
case IP_VERSION(3, 0, 0):
case IP_VERSION(3, 0, 16):
+   case IP_VERSION(3, 0, 64):
case IP_VERSION(3, 1, 1):
case IP_VERSION(3, 0, 2):
amdgpu_device_ip_block_add(adev, _v3_0_ip_block);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index 2658414c503d..4f7c70845785 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -134,6 +134,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
adev->vcn.indirect_sram = true;
break;
case IP_VERSION(3, 0, 0):
+   case IP_VERSION(3, 0, 64):
if (adev->ip_versions[GC_HWIP][0] == IP_VERSION(10, 3, 0))
fw_name = FIRMWARE_SIENNA_CICHLID;
else
diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index febc903adf58..59eafa31c626 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -182,6 +182,7 @@ static int nv_query_video_codecs(struct amdgpu_device 
*adev, bool encode,
 {
switch (adev->ip_versions[UVD_HWIP][0]) {
case IP_VERSION(3, 0, 0):
+   case IP_VERSION(3, 0, 64):
if (amdgpu_sriov_vf(adev)) {
if (encode)
*codecs = _sc_video_codecs_encode;
-- 
2.17.1

Re: [PATCH 5/5] drm/amdkfd: svm deferred work pin mm

[Felix Kuehling]

On 2021-11-09 6:04 p.m., Philip Yang wrote:

Make sure mm does not remove when prange deferred work insert mmu range
notifier, to avoid WARNING:

WARNING: CPU: 6 PID: 1787 at mm/mmu_notifier.c:932 
__mmu_interval_notifier_insert+0xdd/0xf0
Workqueue: events svm_range_deferred_list_work [amdgpu]
RIP: 0010:__mmu_interval_notifier_insert+0xdd/0xf0
Call Trace:
   svm_range_deferred_list_work+0x156/0x320 [amdgpu]
   process_one_work+0x29f/0x5e0
   worker_thread+0x39/0x3e0

Signed-off-by: Philip Yang 
---
  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 2083a10b35c2..fddf0a93d6f1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1992,6 +1992,13 @@ static void svm_range_deferred_list_work(struct 
work_struct *work)
 prange->start, prange->last, prange->work_item.op);
  
  		mm = prange->work_item.mm;

+   if (!mmget_not_zero(mm)) {


You can only rely on mmget_not_zero if there is an mmgrab-reference 
still active. Otherwise the mm_struct may already be freed and allocated 
for something else and the mm_user counter may be corrupted. You're safe 
until kfd_process_wq_release calls put_task_struct(p->lead_thread) 
because the task holds a reference to the mm_struct.


One way to guarantee that at least the mm_struct still exists while this 
worker runs is, to add cancel_work_sync(>svms.deferred_list_work) in 
kfd_process_notifier_release. We should probably do that anyway.



+   pr_debug("skip range %p as mm is gone\n", prange);
+   spin_lock(>deferred_list_lock);
+   list_del_init(>deferred_list);
+   continue;


If the mm is gone, you can probably just break here. All the items in 
the list should have the same mm.


That makes me wonder why we need the mm pointer in the work_item at all. 
We could just get an mm reference from get_task_mm(p->lead_thread) 
(where p is the container of svms). And you can do that outside the 
loop. You'll still need a matching mmput call.


Regards,
  Felix



+   }
+
  retry:
mmap_write_lock(mm);
mutex_lock(>lock);
@@ -2032,6 +2039,7 @@ static void svm_range_deferred_list_work(struct 
work_struct *work)
svm_range_handle_list_op(svms, prange);
mutex_unlock(>lock);
mmap_write_unlock(mm);
+   mmput(mm);
  
  		spin_lock(>deferred_list_lock);

}

[PATCH 1/2] drm/ttm: Put BO in its memory manager's lru list

[xinhui pan]

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Cc: sta...@vger.kernel.org
Reviewed-by: Christian König 
Signed-off-by: xinhui pan 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked)
ttm_bo_unreserve(bo);
+   else
+   ttm_bo_move_to_lru_tail_unlocked(bo);
 
ttm_bo_put(bo);
return ret;
-- 
2.25.1

[PATCH 2/2] drm/ttm: Double check mem_type of BO while eviction

[xinhui pan]

BO might sit in a wrong lru list as there is a small period of memory
moving and lru list updating.

Lets skip eviction if we hit such mismatch.

Suggested-by: Christian König 
Signed-off-by: xinhui pan 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e307004f0b28..0137a302f261 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -594,7 +594,8 @@ static bool ttm_bo_evict_swapout_allowable(struct 
ttm_buffer_object *bo,
*busy = !ret;
}
 
-   if (ret && place && !bo->bdev->funcs->eviction_valuable(bo, place)) {
+   if (ret && place && (bo->resource->mem_type != place->mem_type ||
+   !bo->bdev->funcs->eviction_valuable(bo, place))) {
ret = false;
if (*locked) {
dma_resv_unlock(bo->base.resv);
-- 
2.25.1

Re: [PATCH 4/5] drm/amdkfd: restore pages race with process termination

[Felix Kuehling]

On 2021-11-09 6:04 p.m., Philip Yang wrote:

restore pages work can not find kfd process or mm struct if process is
destroyed before drain retry fault work schedule to run, this is not
failure, return 0 to avoid dump GPU vm fault kernel log.
I wonder if this could also be solved by draining page fault interrupts 
in kfd_process_notifier_release before we remove the process from the 
hash table. Because that function runs while holding the mmap lock, we'd 
need to detect the draining condition for the process in the page fault 
handler before it tries to take the mmap lock. Maybe that's even a 
helpful optimization that speeds up interrupt draining by just ignoring 
all retry faults during that time.


That would also allow draining faults in svm_range_unmap_from_cpu 
instead of the delayed worker. And I believe that would also elegantly 
fix the vma removal race.


Regards,
  Felix




Signed-off-by: Philip Yang 
---
  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 8f77d5746b2c..2083a10b35c2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2596,7 +2596,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
p = kfd_lookup_process_by_pasid(pasid);
if (!p) {
pr_debug("kfd process not founded pasid 0x%x\n", pasid);
-   return -ESRCH;
+   return 0;
}
if (!p->xnack_enabled) {
pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);
@@ -2610,7 +2610,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
mm = get_task_mm(p->lead_thread);
if (!mm) {
pr_debug("svms 0x%p failed to get mm\n", svms);
-   r = -ESRCH;
+   r = 0;
goto out;
}
  

Re: [PATCH 3/5] drm/amdkfd: restore pages race with vma remove

[Felix Kuehling]

On 2021-11-09 6:04 p.m., Philip Yang wrote:

Before restore pages takes mmap read or write lock, vma maybe removed.
Check if vma exists before creating unregistered range or verifying
range access permission, and return 0 if vma is removed to avoid restore
pages return failure to report GPU vm fault to application.


Your patch basically means, we never return a VM fault to an application 
that accesses invalid addresses. The application will just spin on that 
address forever. This basically breaks the debugger. I think you need to 
refine that.


While draining faults (svms->drain_pagefaults is set), I think we can 
return success if no VMA is found. During that time we are expecting 
"straggler" faults. But once the draining is done, any fault interrupts 
we get should be from bad application accesses and should result in a VM 
fault for the application.


Also, much of your patch seems to be refactoring to avoid a duplicate 
VMA lookup. I'd split that into a separate patch for clarity.


Regards,
  Felix



Signed-off-by: Philip Yang 
---
  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 64 
  1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 64f642935600..8f77d5746b2c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2336,20 +2336,13 @@ svm_range_best_restore_location(struct svm_range 
*prange,
  }
  
  static int

-svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
-  unsigned long *start, unsigned long *last,
-  bool *is_heap_stack)
+svm_range_get_range_boundaries(struct kfd_process *p, struct vm_area_struct 
*vma,
+  int64_t addr, unsigned long *start,
+  unsigned long *last, bool *is_heap_stack)
  {
-   struct vm_area_struct *vma;
struct interval_tree_node *node;
unsigned long start_limit, end_limit;
  
-	vma = find_vma(p->mm, addr << PAGE_SHIFT);

-   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-   pr_debug("VMA does not exist in address [0x%llx]\n", addr);
-   return -EFAULT;
-   }
-
*is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
  vma->vm_end >= vma->vm_mm->start_brk) ||
 (vma->vm_start <= vma->vm_mm->start_stack &&
@@ -2444,9 +2437,10 @@ svm_range_check_vm_userptr(struct kfd_process *p, 
uint64_t start, uint64_t last,
  
  static struct

  svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
-   struct kfd_process *p,
-   struct mm_struct *mm,
-   int64_t addr)
+  struct kfd_process *p,
+  struct mm_struct *mm,
+  struct vm_area_struct *vma,
+  int64_t addr)
  {
struct svm_range *prange = NULL;
unsigned long start, last;
@@ -2456,7 +2450,7 @@ svm_range *svm_range_create_unregistered_range(struct 
amdgpu_device *adev,
uint64_t bo_l = 0;
int r;
  
-	if (svm_range_get_range_boundaries(p, addr, , ,

+   if (svm_range_get_range_boundaries(p, vma, addr, , ,
   _heap_stack))
return NULL;
  
@@ -2558,21 +2552,22 @@ svm_range_count_fault(struct amdgpu_device *adev, struct kfd_process *p,

WRITE_ONCE(pdd->faults, pdd->faults + 1);
  }
  
-static bool

-svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool write_fault)
+static int
+svm_fault_allowed(struct mm_struct *mm, struct vm_area_struct *vma,
+ uint64_t addr, bool write_fault)
  {
unsigned long requested = VM_READ;
-   struct vm_area_struct *vma;
  
  	if (write_fault)

requested |= VM_WRITE;
  
-	vma = find_vma(mm, addr << PAGE_SHIFT);

-   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-   pr_debug("address 0x%llx VMA is removed\n", addr);
-   return true;
+   if (!vma) {
+   vma = find_vma(mm, addr << PAGE_SHIFT);
+   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+   pr_debug("address 0x%llx VMA is removed\n", addr);
+   return -EFAULT;
+   }
}
-
pr_debug("requested 0x%lx, vma permission flags 0x%lx\n", requested,
vma->vm_flags);
return (vma->vm_flags & requested) == requested;
@@ -2590,6 +2585,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
int32_t best_loc;
int32_t gpuidx = MAX_GPU_INSTANCE;
bool write_locked = false;
+   struct vm_area_struct 

Re: [PATCH 2/5] drm/amdkfd: check child range to drain retry fault

[Felix Kuehling]

On 2021-11-09 6:04 p.m., Philip Yang wrote:

If unmapping partial range, the parent prange list op is update
notifier, child range list op is unmap range, need check child range to
set drain retry fault flag.

Signed-off-by: Philip Yang 


I think this could be simplified by simply setting 
svms->drain_pagefaults in svm_range_unmap_from_cpu. The mmap lock 
ensures that this is serialized with the deferred list worker reading 
and clearing svms->drain_pagefaults. You can also use READ_ONCE and 
WRITE_ONCE to be safe.


Regards,
  Felix



---
  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 77239b06b236..64f642935600 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2049,8 +2049,19 @@ svm_range_add_list_work(struct svm_range_list *svms, 
struct svm_range *prange,
 * before the range is freed to avoid straggler interrupts on
 * unmapped memory causing "phantom faults".
 */
-   if (op == SVM_OP_UNMAP_RANGE)
+   if (op == SVM_OP_UNMAP_RANGE) {
+   pr_debug("set range drain_pagefaults true\n");
svms->drain_pagefaults = true;
+   } else {
+   struct svm_range *pchild;
+
+   list_for_each_entry(pchild, >child_list, child_list)
+   if (pchild->work_item.op == SVM_OP_UNMAP_RANGE) {
+   pr_debug("set child drain_pagefaults true\n");
+   svms->drain_pagefaults = true;
+   }
+   }
+
/* if prange is on the deferred list */
if (!list_empty(>deferred_list)) {
pr_debug("update exist prange 0x%p work op %d\n", prange, op);

Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain

[Felix Kuehling]

On 2021-11-09 2:12 p.m., Ramesh Errabolu wrote:

MMIO/DOORBELL BOs encode control data and should be pinned in GTT
domain before enabling PCIe connected peer devices in accessing it

Signed-off-by: Ramesh Errabolu 


Reviewed-by: Felix Kuehling 



---
  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 70 +++
  1 file changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 08675f89bfb2..5e063fac0250 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1297,6 +1297,56 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void 
**process_info,
return ret;
  }
  
+/**

+ * amdgpu_amdkfd_gpuvm_pin_bo() - Pins a BO using following criteria
+ * @bo: Handle of buffer object being pinned
+ * @domain: Domain into which BO should be pinned
+ *
+ *   - USERPTR BOs are UNPINNABLE and will return error
+ *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
+ * PIN count incremented. It is valid to PIN a BO multiple times
+ *
+ * Return: ZERO if successful in pinning, Non-Zero in case of error.
+ * Will return -EINVAL if input BO parameter is a USERPTR type.
+ */
+static int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain)
+{
+   int ret = 0;
+
+   ret = amdgpu_bo_reserve(bo, false);
+   if (unlikely(ret))
+   return ret;
+
+   ret = amdgpu_bo_pin_restricted(bo, domain, 0, 0);
+   if (ret)
+   pr_err("Error in Pinning BO to domain: %d\n", domain);
+
+   amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD, false);
+   amdgpu_bo_unreserve(bo);
+
+   return ret;
+}
+
+/**
+ * amdgpu_amdkfd_gpuvm_unpin_bo() - Unpins BO using following criteria
+ * @bo: Handle of buffer object being unpinned
+ *
+ *   - Is a illegal request for USERPTR BOs and is ignored
+ *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
+ * PIN count decremented. Calls to UNPIN must balance calls to PIN
+ */
+void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo)
+{
+   int ret = 0;
+
+   ret = amdgpu_bo_reserve(bo, false);
+   if (unlikely(ret))
+   return;
+
+   amdgpu_bo_unpin(bo);
+   amdgpu_bo_unreserve(bo);
+}
+
  int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
   struct file *filp, u32 pasid,
   void **process_info,
@@ -1523,10 +1573,22 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
if (offset)
*offset = amdgpu_bo_mmap_offset(bo);
  
+	if (flags & (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |

+   KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
+   ret = amdgpu_amdkfd_gpuvm_pin_bo(bo, AMDGPU_GEM_DOMAIN_GTT);
+   if (ret) {
+   pr_err("Pinning MMIO/DOORBELL BO during ALLOC 
FAILED\n");
+   goto err_pin_bo;
+   }
+   bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
+   bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
+   }
+
return 0;
  
  allocate_init_user_pages_failed:

remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+err_pin_bo:
drm_vma_node_revoke(>vma_node, drm_priv);
  err_node_allow:
drm_gem_object_put(gobj);
@@ -1559,6 +1621,14 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
bool is_imported = false;
  
  	mutex_lock(>lock);

+
+   /* Unpin MMIO/DOORBELL BO's that were pinnned during allocation */
+   if (mem->alloc_flags &
+   (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
+KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
+   amdgpu_amdkfd_gpuvm_unpin_bo(mem->bo);
+   }
+
mapped_to_gpu_memory = mem->mapped_to_gpu_memory;
is_imported = mem->is_imported;
mutex_unlock(>lock);

Re: [PATCH] drm/amdgpu: drop jpeg IP initialization in SRIOV case

[Alex Deucher]

On Tue, Nov 9, 2021 at 9:14 PM Guchun Chen  wrote:
>
> Fixes: 67a765c6352d("drm/amdgpu: clean up set IP function")
>
> Signed-off-by: Guchun Chen 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index d7c8d9e3c203..a20d21409c95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -867,7 +867,8 @@ static int amdgpu_discovery_set_mm_ip_blocks(struct 
> amdgpu_device *adev)
> case IP_VERSION(2, 0, 2):
> case IP_VERSION(2, 2, 0):
> amdgpu_device_ip_block_add(adev, _v2_0_ip_block);
> -   amdgpu_device_ip_block_add(adev, _v2_0_ip_block);
> +   if (!amdgpu_sriov_vf(adev))
> +   amdgpu_device_ip_block_add(adev, 
> _v2_0_ip_block);
> break;
> case IP_VERSION(2, 0, 3):
> break;
> --
> 2.17.1
>

[PATCH v2 1/1] drm/amdgpu: Fix MMIO HDP flush on SRIOV

[Felix Kuehling]

Disable HDP register remapping on SRIOV and set rmmio_remap.reg_offset
to the fixed address of the VF register for hdp_v*_flush_hdp.

Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 4 
 drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 4 
 drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c | 4 
 drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/nv.c| 8 +---
 drivers/gpu/drm/amd/amdgpu/soc15.c | 8 +---
 7 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c 
b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
index 4ecd2b5808ce..ee7cab37dfd5 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
@@ -359,6 +359,10 @@ static void nbio_v2_3_init_registers(struct amdgpu_device 
*adev)
 
if (def != data)
WREG32_PCIE(smnPCIE_CONFIG_CNTL, data);
+
+   if (amdgpu_sriov_vf(adev))
+   adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0,
+   mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 
2;
 }
 
 #define NAVI10_PCIE__LC_L0S_INACTIVITY_DEFAULT 0x // off by 
default, no gains over L1
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c 
b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
index 0d2d629e2d6a..4bbacf1be25a 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
@@ -276,6 +276,10 @@ static void nbio_v6_1_init_registers(struct amdgpu_device 
*adev)
 
if (def != data)
WREG32_PCIE(smnPCIE_CI_CNTL, data);
+
+   if (amdgpu_sriov_vf(adev))
+   adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0,
+   mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 
2;
 }
 
 static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
index 3c00666a13e1..37a4039fdfc5 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_0.c
@@ -273,7 +273,9 @@ const struct nbio_hdp_flush_reg nbio_v7_0_hdp_flush_reg = {
 
 static void nbio_v7_0_init_registers(struct amdgpu_device *adev)
 {
-
+   if (amdgpu_sriov_vf(adev))
+   adev->rmmio_remap.reg_offset =
+   SOC15_REG_OFFSET(NBIO, 0, 
mmHDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
 }
 
 const struct amdgpu_nbio_funcs nbio_v7_0_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c 
b/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c
index 8f2a315e7c73..3444332ea110 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c
@@ -371,6 +371,10 @@ static void nbio_v7_2_init_registers(struct amdgpu_device 
*adev)
if (def != data)
WREG32_PCIE_PORT(SOC15_REG_OFFSET(NBIO, 0, 
regPCIE_CONFIG_CNTL), data);
}
+
+   if (amdgpu_sriov_vf(adev))
+   adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0,
+   regBIF_BX_PF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
 }
 
 const struct amdgpu_nbio_funcs nbio_v7_2_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c 
b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
index b8bd03d16dba..e96516d3fd45 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
@@ -362,7 +362,9 @@ const struct nbio_hdp_flush_reg nbio_v7_4_hdp_flush_reg_ald 
= {
 
 static void nbio_v7_4_init_registers(struct amdgpu_device *adev)
 {
-
+   if (amdgpu_sriov_vf(adev))
+   adev->rmmio_remap.reg_offset = SOC15_REG_OFFSET(NBIO, 0,
+   mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 
2;
 }
 
 static void nbio_v7_4_handle_ras_controller_intr_no_bifring(struct 
amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index febc903adf58..7088528079c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -730,8 +730,10 @@ static int nv_common_early_init(void *handle)
 #define MMIO_REG_HOLE_OFFSET (0x8 - PAGE_SIZE)
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-   adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET;
-   adev->rmmio_remap.bus_addr = adev->rmmio_base + MMIO_REG_HOLE_OFFSET;
+   if (!amdgpu_sriov_vf(adev)) {
+   adev->rmmio_remap.reg_offset = MMIO_REG_HOLE_OFFSET;
+   adev->rmmio_remap.bus_addr = adev->rmmio_base + 
MMIO_REG_HOLE_OFFSET;
+   }
adev->smc_rreg = NULL;
adev->smc_wreg = NULL;
adev->pcie_rreg = _pcie_rreg;
@@ -1031,7 +1033,7 @@ static int nv_common_hw_init(void *handle)
 * for the purpose of expose those registers
 * to process space
 */
-   if (adev->nbio.funcs->remap_hdp_registers)
+   if (adev->nbio.funcs->remap_hdp_registers && 

[PATCH] drm/amdgpu: drop jpeg IP initialization in SRIOV case

[Guchun Chen]

Fixes: 67a765c6352d("drm/amdgpu: clean up set IP function")

Signed-off-by: Guchun Chen 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index d7c8d9e3c203..a20d21409c95 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -867,7 +867,8 @@ static int amdgpu_discovery_set_mm_ip_blocks(struct 
amdgpu_device *adev)
case IP_VERSION(2, 0, 2):
case IP_VERSION(2, 2, 0):
amdgpu_device_ip_block_add(adev, _v2_0_ip_block);
-   amdgpu_device_ip_block_add(adev, _v2_0_ip_block);
+   if (!amdgpu_sriov_vf(adev))
+   amdgpu_device_ip_block_add(adev, 
_v2_0_ip_block);
break;
case IP_VERSION(2, 0, 3):
break;
-- 
2.17.1

[PATCH 1/5] drm/amdgpu: handle IH ring1 overflow

[Philip Yang]

IH ring1 is used to process GPU retry fault, overflow is enabled to
drain retry fault before unmapping the range, wptr may pass rptr,
amdgpu_ih_process should check rptr equals to the latest wptr to exit,
otherwise it will continue to recover outdatad retry fault after drain
retry fault is done, and generate false GPU vm fault because range is
unmapped from cpu.

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
index f3d62e196901..d1ef61811169 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
@@ -223,7 +223,7 @@ int amdgpu_ih_wait_on_checkpoint_process(struct 
amdgpu_device *adev,
  */
 int amdgpu_ih_process(struct amdgpu_device *adev, struct amdgpu_ih_ring *ih)
 {
-   unsigned int count = AMDGPU_IH_MAX_NUM_IVS;
+   unsigned int count;
u32 wptr;
 
if (!ih->enabled || adev->shutdown)
@@ -232,6 +232,8 @@ int amdgpu_ih_process(struct amdgpu_device *adev, struct 
amdgpu_ih_ring *ih)
wptr = amdgpu_ih_get_wptr(adev, ih);
 
 restart_ih:
+   count = AMDGPU_IH_MAX_NUM_IVS;
+
DRM_DEBUG("%s: rptr %d, wptr %d\n", __func__, ih->rptr, wptr);
 
/* Order reading of wptr vs. reading of IH ring data */
@@ -240,6 +242,9 @@ int amdgpu_ih_process(struct amdgpu_device *adev, struct 
amdgpu_ih_ring *ih)
while (ih->rptr != wptr && --count) {
amdgpu_irq_dispatch(adev, ih);
ih->rptr &= ih->ptr_mask;
+
+   if (ih == >irq.ih1)
+   wptr = amdgpu_ih_get_wptr(adev, ih);
}
 
amdgpu_ih_set_rptr(adev, ih);
-- 
2.17.1

[PATCH 4/5] drm/amdkfd: restore pages race with process termination

[Philip Yang]

restore pages work can not find kfd process or mm struct if process is
destroyed before drain retry fault work schedule to run, this is not
failure, return 0 to avoid dump GPU vm fault kernel log.

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 8f77d5746b2c..2083a10b35c2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2596,7 +2596,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
p = kfd_lookup_process_by_pasid(pasid);
if (!p) {
pr_debug("kfd process not founded pasid 0x%x\n", pasid);
-   return -ESRCH;
+   return 0;
}
if (!p->xnack_enabled) {
pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);
@@ -2610,7 +2610,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
mm = get_task_mm(p->lead_thread);
if (!mm) {
pr_debug("svms 0x%p failed to get mm\n", svms);
-   r = -ESRCH;
+   r = 0;
goto out;
}
 
-- 
2.17.1

[PATCH 5/5] drm/amdkfd: svm deferred work pin mm

[Philip Yang]

Make sure mm does not remove when prange deferred work insert mmu range
notifier, to avoid WARNING:

WARNING: CPU: 6 PID: 1787 at mm/mmu_notifier.c:932 
__mmu_interval_notifier_insert+0xdd/0xf0
Workqueue: events svm_range_deferred_list_work [amdgpu]
RIP: 0010:__mmu_interval_notifier_insert+0xdd/0xf0
Call Trace:
  svm_range_deferred_list_work+0x156/0x320 [amdgpu]
  process_one_work+0x29f/0x5e0
  worker_thread+0x39/0x3e0

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 2083a10b35c2..fddf0a93d6f1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1992,6 +1992,13 @@ static void svm_range_deferred_list_work(struct 
work_struct *work)
 prange->start, prange->last, prange->work_item.op);
 
mm = prange->work_item.mm;
+   if (!mmget_not_zero(mm)) {
+   pr_debug("skip range %p as mm is gone\n", prange);
+   spin_lock(>deferred_list_lock);
+   list_del_init(>deferred_list);
+   continue;
+   }
+
 retry:
mmap_write_lock(mm);
mutex_lock(>lock);
@@ -2032,6 +2039,7 @@ static void svm_range_deferred_list_work(struct 
work_struct *work)
svm_range_handle_list_op(svms, prange);
mutex_unlock(>lock);
mmap_write_unlock(mm);
+   mmput(mm);
 
spin_lock(>deferred_list_lock);
}
-- 
2.17.1

[PATCH 3/5] drm/amdkfd: restore pages race with vma remove

[Philip Yang]

Before restore pages takes mmap read or write lock, vma maybe removed.
Check if vma exists before creating unregistered range or verifying
range access permission, and return 0 if vma is removed to avoid restore
pages return failure to report GPU vm fault to application.

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 64 
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 64f642935600..8f77d5746b2c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2336,20 +2336,13 @@ svm_range_best_restore_location(struct svm_range 
*prange,
 }
 
 static int
-svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
-  unsigned long *start, unsigned long *last,
-  bool *is_heap_stack)
+svm_range_get_range_boundaries(struct kfd_process *p, struct vm_area_struct 
*vma,
+  int64_t addr, unsigned long *start,
+  unsigned long *last, bool *is_heap_stack)
 {
-   struct vm_area_struct *vma;
struct interval_tree_node *node;
unsigned long start_limit, end_limit;
 
-   vma = find_vma(p->mm, addr << PAGE_SHIFT);
-   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-   pr_debug("VMA does not exist in address [0x%llx]\n", addr);
-   return -EFAULT;
-   }
-
*is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
  vma->vm_end >= vma->vm_mm->start_brk) ||
 (vma->vm_start <= vma->vm_mm->start_stack &&
@@ -2444,9 +2437,10 @@ svm_range_check_vm_userptr(struct kfd_process *p, 
uint64_t start, uint64_t last,
 
 static struct
 svm_range *svm_range_create_unregistered_range(struct amdgpu_device *adev,
-   struct kfd_process *p,
-   struct mm_struct *mm,
-   int64_t addr)
+  struct kfd_process *p,
+  struct mm_struct *mm,
+  struct vm_area_struct *vma,
+  int64_t addr)
 {
struct svm_range *prange = NULL;
unsigned long start, last;
@@ -2456,7 +2450,7 @@ svm_range *svm_range_create_unregistered_range(struct 
amdgpu_device *adev,
uint64_t bo_l = 0;
int r;
 
-   if (svm_range_get_range_boundaries(p, addr, , ,
+   if (svm_range_get_range_boundaries(p, vma, addr, , ,
   _heap_stack))
return NULL;
 
@@ -2558,21 +2552,22 @@ svm_range_count_fault(struct amdgpu_device *adev, 
struct kfd_process *p,
WRITE_ONCE(pdd->faults, pdd->faults + 1);
 }
 
-static bool
-svm_fault_allowed(struct mm_struct *mm, uint64_t addr, bool write_fault)
+static int
+svm_fault_allowed(struct mm_struct *mm, struct vm_area_struct *vma,
+ uint64_t addr, bool write_fault)
 {
unsigned long requested = VM_READ;
-   struct vm_area_struct *vma;
 
if (write_fault)
requested |= VM_WRITE;
 
-   vma = find_vma(mm, addr << PAGE_SHIFT);
-   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
-   pr_debug("address 0x%llx VMA is removed\n", addr);
-   return true;
+   if (!vma) {
+   vma = find_vma(mm, addr << PAGE_SHIFT);
+   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+   pr_debug("address 0x%llx VMA is removed\n", addr);
+   return -EFAULT;
+   }
}
-
pr_debug("requested 0x%lx, vma permission flags 0x%lx\n", requested,
vma->vm_flags);
return (vma->vm_flags & requested) == requested;
@@ -2590,6 +2585,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
int32_t best_loc;
int32_t gpuidx = MAX_GPU_INSTANCE;
bool write_locked = false;
+   struct vm_area_struct *vma = NULL;
int r = 0;
 
if (!KFD_IS_SVM_API_SUPPORTED(adev->kfd.dev)) {
@@ -2636,7 +2632,15 @@ svm_range_restore_pages(struct amdgpu_device *adev, 
unsigned int pasid,
write_locked = true;
goto retry_write_locked;
}
-   prange = svm_range_create_unregistered_range(adev, p, mm, addr);
+
+   vma = find_vma(p->mm, addr << PAGE_SHIFT);
+   if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+   pr_debug("VMA not found address [0x%llx]\n", addr);
+   mmap_write_downgrade(mm);
+   r = 0;
+   goto out_unlock_svms;
+   }
+   prange = 

[PATCH 2/5] drm/amdkfd: check child range to drain retry fault

[Philip Yang]

If unmapping partial range, the parent prange list op is update
notifier, child range list op is unmap range, need check child range to
set drain retry fault flag.

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 77239b06b236..64f642935600 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2049,8 +2049,19 @@ svm_range_add_list_work(struct svm_range_list *svms, 
struct svm_range *prange,
 * before the range is freed to avoid straggler interrupts on
 * unmapped memory causing "phantom faults".
 */
-   if (op == SVM_OP_UNMAP_RANGE)
+   if (op == SVM_OP_UNMAP_RANGE) {
+   pr_debug("set range drain_pagefaults true\n");
svms->drain_pagefaults = true;
+   } else {
+   struct svm_range *pchild;
+
+   list_for_each_entry(pchild, >child_list, child_list)
+   if (pchild->work_item.op == SVM_OP_UNMAP_RANGE) {
+   pr_debug("set child drain_pagefaults true\n");
+   svms->drain_pagefaults = true;
+   }
+   }
+
/* if prange is on the deferred list */
if (!list_empty(>deferred_list)) {
pr_debug("update exist prange 0x%p work op %d\n", prange, op);
-- 
2.17.1

[PATCH v2 3/3] drm/amdkfd: convert misc checks to IP version checking

[Graham Sider]

Switch to IP version checking instead of asic_type on various KFD
version checks.

Signed-off-by: Graham Sider 
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c  |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_device.c   | 27 ++-
 .../drm/amd/amdkfd/kfd_device_queue_manager.c |  3 +--
 .../amd/amdkfd/kfd_device_queue_manager_v9.c  |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c   |  6 +++--
 drivers/gpu/drm/amd/amdkfd/kfd_migrate.c  |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  7 +++--
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c  |  6 ++---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c |  4 +--
 10 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 2466a73b8c7d..f70117b00b14 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1603,7 +1603,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file 
*filep,
}
mutex_unlock(>mutex);
 
-   if (dev->device_info->asic_family == CHIP_ALDEBARAN) {
+   if (KFD_GC_VERSION(dev) == IP_VERSION(9, 4, 2)) {
err = amdgpu_amdkfd_gpuvm_sync_memory(dev->adev,
(struct kgd_mem *) mem, true);
if (err) {
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 19dd472e9b06..b6d887edac85 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -1992,7 +1992,7 @@ static int kfd_fill_gpu_direct_io_link_to_cpu(int 
*avail_size,
sub_type_hdr->flags |= CRAT_IOLINK_FLAGS_BI_DIRECTIONAL;
sub_type_hdr->io_interface_type = CRAT_IOLINK_TYPE_XGMI;
sub_type_hdr->num_hops_xgmi = 1;
-   if (kdev->adev->asic_type == CHIP_ALDEBARAN) {
+   if (KFD_GC_VERSION(kdev) == IP_VERSION(9, 4, 2)) {
sub_type_hdr->minimum_bandwidth_mbs =
amdgpu_amdkfd_get_xgmi_bandwidth_mbytes(
kdev->adev, NULL, true);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index ee813bd57c92..594dd28a391f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -848,23 +848,23 @@ struct kfd_dev *kgd2kfd_probe(struct amdgpu_device *adev, 
bool vf)
 static void kfd_cwsr_init(struct kfd_dev *kfd)
 {
if (cwsr_enable && kfd->device_info->supports_cwsr) {
-   if (kfd->device_info->asic_family < CHIP_VEGA10) {
+   if (KFD_GC_VERSION(kfd) < IP_VERSION(9, 0, 1)) {
BUILD_BUG_ON(sizeof(cwsr_trap_gfx8_hex) > PAGE_SIZE);
kfd->cwsr_isa = cwsr_trap_gfx8_hex;
kfd->cwsr_isa_size = sizeof(cwsr_trap_gfx8_hex);
-   } else if (kfd->device_info->asic_family == CHIP_ARCTURUS) {
+   } else if (KFD_GC_VERSION(kfd) == IP_VERSION(9, 4, 1)) {
BUILD_BUG_ON(sizeof(cwsr_trap_arcturus_hex) > 
PAGE_SIZE);
kfd->cwsr_isa = cwsr_trap_arcturus_hex;
kfd->cwsr_isa_size = sizeof(cwsr_trap_arcturus_hex);
-   } else if (kfd->device_info->asic_family == CHIP_ALDEBARAN) {
+   } else if (KFD_GC_VERSION(kfd) == IP_VERSION(9, 4, 2)) {
BUILD_BUG_ON(sizeof(cwsr_trap_aldebaran_hex) > 
PAGE_SIZE);
kfd->cwsr_isa = cwsr_trap_aldebaran_hex;
kfd->cwsr_isa_size = sizeof(cwsr_trap_aldebaran_hex);
-   } else if (kfd->device_info->asic_family < CHIP_NAVI10) {
+   } else if (KFD_GC_VERSION(kfd) < IP_VERSION(10, 1, 1)) {
BUILD_BUG_ON(sizeof(cwsr_trap_gfx9_hex) > PAGE_SIZE);
kfd->cwsr_isa = cwsr_trap_gfx9_hex;
kfd->cwsr_isa_size = sizeof(cwsr_trap_gfx9_hex);
-   } else if (kfd->device_info->asic_family < CHIP_SIENNA_CICHLID) 
{
+   } else if (KFD_GC_VERSION(kfd) < IP_VERSION(10, 3, 0)) {
BUILD_BUG_ON(sizeof(cwsr_trap_nv1x_hex) > PAGE_SIZE);
kfd->cwsr_isa = cwsr_trap_nv1x_hex;
kfd->cwsr_isa_size = sizeof(cwsr_trap_nv1x_hex);
@@ -886,14 +886,16 @@ static int kfd_gws_init(struct kfd_dev *kfd)
return 0;
 
if (hws_gws_support
-   || (kfd->device_info->asic_family == CHIP_VEGA10
+   || (KFD_GC_VERSION(kfd) == IP_VERSION(9, 0, 1)
&& kfd->mec2_fw_version >= 0x81b3)
-   || (kfd->device_info->asic_family >= CHIP_VEGA12
-   && kfd->device_info->asic_family <= CHIP_RAVEN
+   || ((KFD_GC_VERSION(kfd) == IP_VERSION(9, 2, 

[PATCH v2 2/3] drm/amdkfd: convert switches to IP version checking

[Graham Sider]

Converts KFD switch statements to use IP version checking instead
of asic_type.

Signed-off-by: Graham Sider 
---
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 124 +-
 drivers/gpu/drm/amd/amdkfd/kfd_device.c   |   8 +-
 .../drm/amd/amdkfd/kfd_device_queue_manager.c |  33 ++---
 drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c  |  29 +---
 .../gpu/drm/amd/amdkfd/kfd_packet_manager.c   |  33 ++---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c |  30 +
 6 files changed, 102 insertions(+), 155 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 1dc6cb7446e0..19dd472e9b06 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -1377,67 +1377,71 @@ static int kfd_fill_gpu_cache_info(struct kfd_dev *kdev,
pcache_info = vegam_cache_info;
num_of_cache_types = ARRAY_SIZE(vegam_cache_info);
break;
-   case CHIP_VEGA10:
-   pcache_info = vega10_cache_info;
-   num_of_cache_types = ARRAY_SIZE(vega10_cache_info);
-   break;
-   case CHIP_VEGA12:
-   pcache_info = vega12_cache_info;
-   num_of_cache_types = ARRAY_SIZE(vega12_cache_info);
-   break;
-   case CHIP_VEGA20:
-   case CHIP_ARCTURUS:
-   pcache_info = vega20_cache_info;
-   num_of_cache_types = ARRAY_SIZE(vega20_cache_info);
-   break;
-   case CHIP_ALDEBARAN:
-   pcache_info = aldebaran_cache_info;
-   num_of_cache_types = ARRAY_SIZE(aldebaran_cache_info);
-   break;
-   case CHIP_RAVEN:
-   pcache_info = raven_cache_info;
-   num_of_cache_types = ARRAY_SIZE(raven_cache_info);
-   break;
-   case CHIP_RENOIR:
-   pcache_info = renoir_cache_info;
-   num_of_cache_types = ARRAY_SIZE(renoir_cache_info);
-   break;
-   case CHIP_NAVI10:
-   case CHIP_NAVI12:
-   case CHIP_CYAN_SKILLFISH:
-   pcache_info = navi10_cache_info;
-   num_of_cache_types = ARRAY_SIZE(navi10_cache_info);
-   break;
-   case CHIP_NAVI14:
-   pcache_info = navi14_cache_info;
-   num_of_cache_types = ARRAY_SIZE(navi14_cache_info);
-   break;
-   case CHIP_SIENNA_CICHLID:
-   pcache_info = sienna_cichlid_cache_info;
-   num_of_cache_types = ARRAY_SIZE(sienna_cichlid_cache_info);
-   break;
-   case CHIP_NAVY_FLOUNDER:
-   pcache_info = navy_flounder_cache_info;
-   num_of_cache_types = ARRAY_SIZE(navy_flounder_cache_info);
-   break;
-   case CHIP_DIMGREY_CAVEFISH:
-   pcache_info = dimgrey_cavefish_cache_info;
-   num_of_cache_types = ARRAY_SIZE(dimgrey_cavefish_cache_info);
-   break;
-   case CHIP_VANGOGH:
-   pcache_info = vangogh_cache_info;
-   num_of_cache_types = ARRAY_SIZE(vangogh_cache_info);
-   break;
-   case CHIP_BEIGE_GOBY:
-   pcache_info = beige_goby_cache_info;
-   num_of_cache_types = ARRAY_SIZE(beige_goby_cache_info);
-   break;
-   case CHIP_YELLOW_CARP:
-   pcache_info = yellow_carp_cache_info;
-   num_of_cache_types = ARRAY_SIZE(yellow_carp_cache_info);
-   break;
default:
-   return -EINVAL;
+   switch(KFD_GC_VERSION(kdev)) {
+   case IP_VERSION(9, 0, 1):
+   pcache_info = vega10_cache_info;
+   num_of_cache_types = ARRAY_SIZE(vega10_cache_info);
+   break;
+   case IP_VERSION(9, 2, 1):
+   pcache_info = vega12_cache_info;
+   num_of_cache_types = ARRAY_SIZE(vega12_cache_info);
+   break;
+   case IP_VERSION(9, 4, 0):
+   case IP_VERSION(9, 4, 1):
+   pcache_info = vega20_cache_info;
+   num_of_cache_types = ARRAY_SIZE(vega20_cache_info);
+   break;
+   case IP_VERSION(9, 4, 2):
+   pcache_info = aldebaran_cache_info;
+   num_of_cache_types = ARRAY_SIZE(aldebaran_cache_info);
+   break;
+   case IP_VERSION(9, 1, 0):
+   case IP_VERSION(9, 2, 2):
+   pcache_info = raven_cache_info;
+   num_of_cache_types = ARRAY_SIZE(raven_cache_info);
+   break;
+   case IP_VERSION(9, 3, 0):
+   pcache_info = renoir_cache_info;
+   num_of_cache_types = ARRAY_SIZE(renoir_cache_info);
+   break;
+   case IP_VERSION(10, 1, 10):
+   case IP_VERSION(10, 1, 

[PATCH v2 1/3] drm/amdkfd: convert KFD_IS_SOC to IP version checking

[Graham Sider]

Defined as GC HWIP >= IP_VERSION(9, 0, 1).

Also defines KFD_GC_VERSION to return GC HWIP version.

Signed-off-by: Graham Sider 
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c  | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 4 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 3 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c  | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 2e3d74f7fbfb..2466a73b8c7d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -321,7 +321,7 @@ static int kfd_ioctl_create_queue(struct file *filep, 
struct kfd_process *p,
/* Return gpu_id as doorbell offset for mmap usage */
args->doorbell_offset = KFD_MMAP_TYPE_DOORBELL;
args->doorbell_offset |= KFD_MMAP_GPU_ID(args->gpu_id);
-   if (KFD_IS_SOC15(dev->device_info->asic_family))
+   if (KFD_IS_SOC15(dev))
/* On SOC15 ASICs, include the doorbell offset within the
 * process doorbell frame, which is 2 pages.
 */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index 0a60317509c8..4f7aec6a481b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -157,7 +157,7 @@ static int allocate_doorbell(struct qcm_process_device 
*qpd, struct queue *q)
 {
struct kfd_dev *dev = qpd->dqm->dev;
 
-   if (!KFD_IS_SOC15(dev->device_info->asic_family)) {
+   if (!KFD_IS_SOC15(dev)) {
/* On pre-SOC15 chips we need to use the queue ID to
 * preserve the user mode ABI.
 */
@@ -202,7 +202,7 @@ static void deallocate_doorbell(struct qcm_process_device 
*qpd,
unsigned int old;
struct kfd_dev *dev = qpd->dqm->dev;
 
-   if (!KFD_IS_SOC15(dev->device_info->asic_family) ||
+   if (!KFD_IS_SOC15(dev) ||
q->properties.type == KFD_QUEUE_TYPE_SDMA ||
q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)
return;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 78ae96fc8a6a..352709034acf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -183,7 +183,8 @@ enum cache_policy {
cache_policy_noncoherent
 };
 
-#define KFD_IS_SOC15(chip) ((chip) >= CHIP_VEGA10)
+#define KFD_GC_VERSION(dev) ((dev)->adev->ip_versions[GC_HWIP][0])
+#define KFD_IS_SOC15(dev)   ((KFD_GC_VERSION(dev)) >= (IP_VERSION(9, 0, 1)))
 
 struct kfd_event_interrupt_class {
bool (*interrupt_isr)(struct kfd_dev *dev,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index f29b3932e3dc..fafc7b187fad 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1431,7 +1431,7 @@ static int init_doorbell_bitmap(struct qcm_process_device 
*qpd,
int range_start = dev->shared_resources.non_cp_doorbells_start;
int range_end = dev->shared_resources.non_cp_doorbells_end;
 
-   if (!KFD_IS_SOC15(dev->device_info->asic_family))
+   if (!KFD_IS_SOC15(dev))
return 0;
 
qpd->doorbell_bitmap =
-- 
2.25.1

RE: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain

[Errabolu, Ramesh]

[AMD Official Use Only]

Based on my experiments I am able conclude that I can avoid validating the BO 
prior to pinning it. I don't have the code history that led me to validating 
the BO in the first place. In any case I posted an updated patch to the 
DRM-NEXT branch in addition to a standalone patch for the DKMS branch as well.

Thanks for pointing this out.

Regards,
Ramesh

From: Errabolu, Ramesh 
Sent: Tuesday, November 9, 2021 1:32 AM
To: Yu, Lang 
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain

I will experiment to see if it is not needed. Will update patch based on the 
results.

Regards,
Ramesh

From: Yu, Lang mailto:lang...@amd.com>>
Sent: Tuesday, November 9, 2021 12:44 AM
To: Errabolu, Ramesh mailto:ramesh.errab...@amd.com>>
Cc: amd-gfx@lists.freedesktop.org 
mailto:amd-gfx@lists.freedesktop.org>>
Subject: Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain

On Tue, Nov 09, 2021 at 02:12:00PM +0800, Errabolu, Ramesh wrote:
> [AMD Official Use Only]
>
> Responses in line
>
> -Original Message-
> From: Yu, Lang mailto:lang...@amd.com>>
> Sent: Monday, November 8, 2021 11:27 PM
> To: Errabolu, Ramesh mailto:ramesh.errab...@amd.com>>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain
>
> On Mon, Nov 08, 2021 at 07:37:44PM -0600, Ramesh Errabolu wrote:
> > MMIO/DOORBELL BOs encode control data and should be pinned in GTT
> > domain before enabling PCIe connected peer devices in accessing it
> >
> > Signed-off-by: Ramesh Errabolu 
> > mailto:ramesh.errab...@amd.com>>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 25 +
> >  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 55
> > +++
> >  2 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > index 4c1d6536a7a5..d9a1cfd7876f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > @@ -300,6 +300,31 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
> > amdgpu_device *adev,
> >uint64_t va, void *drm_priv,
> >struct kgd_mem **mem, uint64_t *size,
> >uint64_t *mmap_offset);
> > +
> > +/**
> > + * amdgpu_amdkfd_gpuvm_pin_bo() - Pins a BO using following criteria
> > + * @bo: Handle of buffer object being pinned
> > + * @domain: Domain into which BO should be pinned
> > + *
> > + *   - USERPTR BOs are UNPINNABLE and will return error
> > + *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
> > + * PIN count incremented. It is valid to PIN a BO multiple times
> > + *
> > + * Return: ZERO if successful in pinning, Non-Zero in case of error.
> > + * Will return -EINVAL if input BO parameter is a USERPTR type.
> > + */
> > +int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain);
> > +
> > +/**
> > + * amdgpu_amdkfd_gpuvm_unpin_bo() - Unpins BO using following
> > +criteria
> > + * @bo: Handle of buffer object being unpinned
> > + *
> > + *   - Is a illegal request for USERPTR BOs and is ignored
> > + *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
> > + * PIN count decremented. Calls to UNPIN must balance calls to PIN
> > + */
> > +void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo);
> > +
> >  int amdgpu_amdkfd_get_tile_config(struct amdgpu_device *adev,
> >  struct tile_config *config);
> >  void amdgpu_amdkfd_ras_poison_consumption_handler(struct
> > amdgpu_device *adev); diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index 4fa814358552..f4ffc41873dd 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -1299,6 +1299,36 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void 
> > **process_info,
> >  return ret;
> >  }
> >
> > +int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain) {
> > +   int ret = 0;
> > +
> > +   ret = amdgpu_bo_reserve(bo, false);
> > +   if (unlikely(ret))
> > +   return ret;
> > +
> > +   ret = amdgpu_bo_pin_restricted(bo, domain, 0, 0);
> > +   if (ret)
> > +   pr_err("Error in Pinning BO to domain: %d\n", domain);
> > +
> > +   amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD, false);
> > +   amdgpu_bo_unreserve(bo);
> > +
> > +   return ret;
> > +}
> > +
> > +void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo) {
> > +   int ret = 0;
> > +
> > +   ret = amdgpu_bo_reserve(bo, false);
> > +   if (unlikely(ret))
> > +   return;
> > +
> > +   amdgpu_bo_unpin(bo);
> > +   amdgpu_bo_unreserve(bo);
> > +}
> > +
> >  int 

[PATCH] drm/amdgpu: Pin MMIO/DOORBELL BO's in GTT domain

[Ramesh Errabolu]

MMIO/DOORBELL BOs encode control data and should be pinned in GTT
domain before enabling PCIe connected peer devices in accessing it

Signed-off-by: Ramesh Errabolu 
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 70 +++
 1 file changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 08675f89bfb2..5e063fac0250 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1297,6 +1297,56 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void 
**process_info,
return ret;
 }
 
+/**
+ * amdgpu_amdkfd_gpuvm_pin_bo() - Pins a BO using following criteria
+ * @bo: Handle of buffer object being pinned
+ * @domain: Domain into which BO should be pinned
+ *
+ *   - USERPTR BOs are UNPINNABLE and will return error
+ *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
+ * PIN count incremented. It is valid to PIN a BO multiple times
+ *
+ * Return: ZERO if successful in pinning, Non-Zero in case of error.
+ * Will return -EINVAL if input BO parameter is a USERPTR type.
+ */
+static int amdgpu_amdkfd_gpuvm_pin_bo(struct amdgpu_bo *bo, u32 domain)
+{
+   int ret = 0;
+
+   ret = amdgpu_bo_reserve(bo, false);
+   if (unlikely(ret))
+   return ret;
+
+   ret = amdgpu_bo_pin_restricted(bo, domain, 0, 0);
+   if (ret)
+   pr_err("Error in Pinning BO to domain: %d\n", domain);
+
+   amdgpu_bo_sync_wait(bo, AMDGPU_FENCE_OWNER_KFD, false);
+   amdgpu_bo_unreserve(bo);
+
+   return ret;
+}
+
+/**
+ * amdgpu_amdkfd_gpuvm_unpin_bo() - Unpins BO using following criteria
+ * @bo: Handle of buffer object being unpinned
+ *
+ *   - Is a illegal request for USERPTR BOs and is ignored
+ *   - All other BO types (GTT, VRAM, MMIO and DOORBELL) will have their
+ * PIN count decremented. Calls to UNPIN must balance calls to PIN
+ */
+void amdgpu_amdkfd_gpuvm_unpin_bo(struct amdgpu_bo *bo)
+{
+   int ret = 0;
+
+   ret = amdgpu_bo_reserve(bo, false);
+   if (unlikely(ret))
+   return;
+
+   amdgpu_bo_unpin(bo);
+   amdgpu_bo_unreserve(bo);
+}
+
 int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
   struct file *filp, u32 pasid,
   void **process_info,
@@ -1523,10 +1573,22 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
if (offset)
*offset = amdgpu_bo_mmap_offset(bo);
 
+   if (flags & (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
+   KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
+   ret = amdgpu_amdkfd_gpuvm_pin_bo(bo, AMDGPU_GEM_DOMAIN_GTT);
+   if (ret) {
+   pr_err("Pinning MMIO/DOORBELL BO during ALLOC 
FAILED\n");
+   goto err_pin_bo;
+   }
+   bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
+   bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
+   }
+
return 0;
 
 allocate_init_user_pages_failed:
remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+err_pin_bo:
drm_vma_node_revoke(>vma_node, drm_priv);
 err_node_allow:
drm_gem_object_put(gobj);
@@ -1559,6 +1621,14 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
bool is_imported = false;
 
mutex_lock(>lock);
+
+   /* Unpin MMIO/DOORBELL BO's that were pinnned during allocation */
+   if (mem->alloc_flags &
+   (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
+KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
+   amdgpu_amdkfd_gpuvm_unpin_bo(mem->bo);
+   }
+
mapped_to_gpu_memory = mem->mapped_to_gpu_memory;
is_imported = mem->is_imported;
mutex_unlock(>lock);
-- 
2.31.1

Re: [PATCH 2/2] drm/sched: serialize job_timeout and scheduler

[Rob Clark]

On Tue, Nov 9, 2021 at 1:07 AM Daniel Vetter  wrote:
>
> On Mon, Nov 08, 2021 at 03:39:17PM -0800, Rob Clark wrote:
> > I stumbled across this thread when I ran into the same issue, while
> > working out how to move drm/msm to use scheduler's retire +
> > timeout/recovery (and get rid of our own mirror list of in-flight
> > jobs).  We already have hw error detection enabled, and it can signal
> > quite fast, so assuming the first job on the list is the guilty job
> > just won't work.
> >
> > But I was considering a slightly different approach to fixing this,
> > instead just handling it all in drm_sched_main() and getting rid of
> > the complicated kthread parking gymnastics.  Ie. something along the
> > lines of:
>
> So handling timeouts in the main sched thread wont work as soon as you
> have multiple engines and reset that impacts across engines:
>
> - Nothing is simplified since you still need to stop the other scheduler
>   threads.
>
> - You get deadlocks if 2 schedulers time out at the same time, and both
>   want to stop the other one.
>
> Hence workqueue. Now the rule for the wq is that you can only have one per
> reset domain, so
> - single engine you just take the one drm/sched provides
> - if reset affects all your engines in the chip, then you allocate on in
>   the drm_device and pass that to all
> - if you have a complex of gpus all interconnected (e.g. xgmi hive for
>   amd), then it's one wq for the entire hive
>
> _All_ reset related things must be run on that workqueue or things breaks,
> which means if you get hw fault that also needs to be run there. I guess
> we should either patch drm/sched to check you call that function from the
> right workqueue, or just handle it internally.

Hmm, ok.. I guess it would be useful to better document the reasoning
for the current design, that would have steered me more towards the
approach taken in this patch.

BR,
-R

> -Daniel
>
> >
> > -
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 67382621b429..4d6ce775c316 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -764,6 +764,45 @@ static bool drm_sched_blocked(struct
> > drm_gpu_scheduler *sched)
> > return false;
> >  }
> >
> > +static bool handle_timeout(struct drm_gpu_scheduler *sched)
> > +{
> > +   struct drm_sched_job *bad;
> > +
> > +   if (!sched->has_timeout)
> > +   return false;
> > +
> > +   sched->has_timeout = false;
> > +
> > +   spin_lock(>job_list_lock);
> > +   bad = list_first_entry_or_null(>pending_list,
> > +  struct drm_sched_job, list);
> > +
> > +   if (!bad) {
> > +   spin_unlock(>job_list_lock);
> > +   return false;
> > +   }
> > +
> > +   spin_unlock(>job_list_lock);
> > +
> > +   if (sched->timeout_wq == system_wq) {
> > +   /*
> > +* If driver has no specific requirements about serializing
> > +* reset wrt. other engines, just call timedout_job() 
> > directly
> > +*/
> > +   sched->ops->timedout_job(job);
> > +   } else {
> > +   /*
> > +* Otherwise queue it on timeout_wq and wait for it to 
> > complete
> > +*/
> > +   ... more typing needed here ...
> > +   }
> > +
> > +   if (sched->free_guilty) {
> > +   sched->ops->free_job(job);
> > +   sched->free_guilty = false;
> > +   }
> > +}
> > +
> >  /**
> >   * drm_sched_main - main scheduler thread
> >   *
> > @@ -787,6 +826,7 @@ static int drm_sched_main(void *param)
> >
> > wait_event_interruptible(sched->wake_up_worker,
> >  (cleanup_job =
> > drm_sched_get_cleanup_job(sched)) ||
> > +handle_timeout(sched) ||
> >  (!drm_sched_blocked(sched) &&
> >   (entity =
> > drm_sched_select_entity(sched))) ||
> >  kthread_should_stop());
> > -
> >
> > drm_sched_fault() and the sw timeout handler would just set
> > sched->has_timeout and kick sched->wake_up_worker.
> >
> > And since we handle the timeout case after
> > drm_sched_get_cleanup_job(), we know that all of the successfully
> > completed jobs have already been popped off the list, and won't be
> > unfairly maligned.
> >
> > BR,
> > -R
> >
> > On Tue, Aug 31, 2021 at 6:29 PM Liu, Monk  wrote:
> > >
> > > [AMD Official Use Only]
> > >
> > > Okay, I will reprepare this patch
> > >
> > > Thanks
> > >
> > > --
> > > Monk Liu | Cloud-GPU Core team
> > > --
> > >
> > > -Original Message-
> > > From: Daniel Vetter 
> > > Sent: Tuesday, 

Re: 回复: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Christian König]

In general the correct idea, but the wrong place to check that.

Calling amdgpu_ttm_bo_eviction_valuable() is only optional, but that 
check must be mandatory for correct operation.


This needs to be inside ttm_bo_evict_swapout_allowable().

Christian.

Am 09.11.21 um 14:41 schrieb Pan, Xinhui:

[AMD Official Use Only]

yes, a double check is needed.
how about change below.

As long as we detect such mismatch, it indicates another eviction is on going. 
return false here is reasonable.

@@ -1335,6 +1336,8 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,
 struct dma_fence *f;
 int i;

+   if (bo->resource->mem_type != place->mem_type)
+   return false;
 /* Swapout? */
 if (bo->resource->mem_type == TTM_PL_SYSTEM)
 return true;


发件人: Koenig, Christian 
发送时间: 2021年11月9日 21:18
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Exactly that's the reason why we should have the double check in TTM
I've mentioned in the other mail.

Christian.

Am 09.11.21 um 14:16 schrieb Pan, Xinhui:

[AMD Official Use Only]

Actually this patch does not totally fix the mismatch of lru list with mem_type as 
mem_type is changed in ->move() and lru list is changed after that.

During this small period, another eviction could still happed and evict this 
mismatched BO from sMam(say, its lru list is on vram domain) to sMem.

发件人: Pan, Xinhui 
发送时间: 2021年11月9日 21:05
收件人: Koenig, Christian; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.

I think that amdgpu_bo_move() does support copy from sysMem to sysMem correctly.
maybe something below is needed.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c83ef42ca702..aa63ae7ddf1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
bool evict,
  }
  if (old_mem->mem_type == TTM_PL_SYSTEM &&
  (new_mem->mem_type == TTM_PL_TT ||
-new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
+new_mem->mem_type == AMDGPU_PL_PREEMPT ||
+new_mem->mem_type == TTM_PL_SYSTEM)) {
  ttm_bo_move_null(bo, new_mem);
  goto out;
  }

otherwise, amdgpu_move_blit() is called to do the system memory copy which use 
a wrong address.
   206 /* Map only what can't be accessed directly */
   207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
   208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
   209 mm_cur->start;
   210 return 0;
   211 }

line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such addr, 
page fault happens.



发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:35
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never
cause a crash.

Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:

[AMD Official Use Only]

I hit vulkan cts test hang with navi23.

dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
And some debug log also says amdgu copy one BO from system Domain to system 
Domain which is really weird.

发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:20
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Am 09.11.21 um 12:19 schrieb xinhui pan:

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 

Interesting find, did you trigger that somehow or did you just stumbled
over it by reading the code?

Patch is Reviewed-by: Christian König , I will
pick that up for drm-misc-next.

Thanks,
Christian.


---
 drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
 ret = ttm_bo_evict(bo, ctx);
 if (locked)
 ttm_bo_unreserve(bo);
+  

Re: [PATCH 5/8] drm: Implement method to free unused pages

[Arunpravin]

On 04/11/21 12:46 am, Matthew Auld wrote:
> On 25/10/2021 14:00, Arunpravin wrote:
>> On contiguous allocation, we round up the size
>> to the *next* power of 2, implement a function
>> to free the unused pages after the newly allocate block.
>>
>> Signed-off-by: Arunpravin 
> 
> Ideally this gets added with some user, so we can see it in action? 
> Maybe squash the next patch here?
[Arun] ok
> 
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 103 
>>   include/drm/drm_buddy.h |   4 ++
>>   2 files changed, 107 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 9d3547bcc5da..0da8510736eb 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -284,6 +284,109 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, 
>> u64 e2)
>>  return s1 <= s2 && e1 >= e2;
>>   }
>>   
>> +/**
>> + * drm_buddy_free_unused_pages - free unused pages
>> + *
>> + * @mm: DRM buddy manager
>> + * @actual_size: original size requested
>> + * @blocks: output list head to add allocated blocks
>> + *
>> + * For contiguous allocation, we round up the size to the nearest
>> + * power of two value, drivers consume *actual* size, so remaining
>> + * portions are unused and it can be freed.
>> + *
>> + * Returns:
>> + * 0 on success, error code on failure.
>> + */
>> +int drm_buddy_free_unused_pages(struct drm_buddy_mm *mm,
> 
> drm_buddy_block_trim?
[Arun] ok
> 
>> +u64 actual_size,
> 
> new_size?
[Arun] ok
> 
>> +struct list_head *blocks)
>> +{
>> +struct drm_buddy_block *block;
>> +struct drm_buddy_block *buddy;
>> +u64 actual_start;
>> +u64 actual_end;
>> +LIST_HEAD(dfs);
>> +u64 count = 0;
>> +int err;
>> +
>> +if (!list_is_singular(blocks))
>> +return -EINVAL;
>> +
>> +block = list_first_entry_or_null(blocks,
>> + struct drm_buddy_block,
>> + link);
>> +
>> +if (!block)
>> +return -EINVAL;
> 
> list_is_singular() already ensures that I guess?
[Arun] yes it checks the list empty status, I will remove 'if (!block)'
check
> 
> 
>> +
>> +if (actual_size > drm_buddy_block_size(mm, block))
>> +return -EINVAL;
>> +
>> +if (actual_size == drm_buddy_block_size(mm, block))
>> +return 0;
> 
> Probably need to check the alignment of the actual_size, and also check 
> that it is non-zero?
[Arun] ok
> 
>> +
>> +list_del(>link);
>> +
>> +actual_start = drm_buddy_block_offset(block);
>> +actual_end = actual_start + actual_size - 1;
>> +
>> +if (drm_buddy_block_is_allocated(block))
> 
> That should rather be a programmer error.
[Arun] ok, I will check for the allocation status and return -EINVAL if
the block is not allocated.
> 
>> +mark_free(mm, block);
>> +
>> +list_add(>tmp_link, );
>> +
>> +while (1) {
>> +block = list_first_entry_or_null(,
>> + struct drm_buddy_block,
>> + tmp_link);
>> +
>> +if (!block)
>> +break;
>> +
>> +list_del(>tmp_link);
>> +
>> +if (count == actual_size)
>> +return 0;
> 
> 
> Check for overlaps somewhere here to avoid needless searching and splitting?
[Arun] ok
> 
>> +
>> +if (contains(actual_start, actual_end, 
>> drm_buddy_block_offset(block),
>> +(drm_buddy_block_offset(block) + 
>> drm_buddy_block_size(mm, block) - 1))) {
> 
> Could maybe record the start/end for better readability?
[Arun] ok
> 
>> +BUG_ON(!drm_buddy_block_is_free(block));
>> +
>> +/* Allocate only required blocks */
>> +mark_allocated(block);
>> +mm->avail -= drm_buddy_block_size(mm, block);
>> +list_add_tail(>link, blocks);
>> +count += drm_buddy_block_size(mm, block);
>> +continue;
>> +}
>> +
>> +if (drm_buddy_block_order(block) == 0)
>> +continue;
> 
> Should be impossible with overlaps check added.
[Arun] yes, I will remove
> 
>> +
>> +if (!drm_buddy_block_is_split(block)) {
> 
> That should always be true.
[Arun] ok
> 
>> +err = split_block(mm, block);
>> +
>> +if (unlikely(err))
>> +goto err_undo;
>> +}
>> +
>> +list_add(>right->tmp_link, );
>> +list_add(>left->tmp_link, );
>> +}
>> +
>> +return -ENOSPC;
> 
> 
> Would it make sense to factor out part of the alloc_range for this? It 
> looks roughly the same.
[Arun] This function gets called for non-range allocations (0..max_size)
as well on contiguous allocation. alloc_range() is called only for range

Re: [PATCH] drm/amd/display: log amdgpu_dm_atomic_check() failure cause

[Harry Wentland]

On 2021-11-09 00:14, Shirish S wrote:
> update developers with next level of info about unsupported
> display configuration query that led to atomic check failure.
> 
> Signed-off-by: Shirish S 

Reviewed-by: Harry Wentland 

Harry

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 69 ++-
>  1 file changed, 51 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b1d9e89e5ae9..b7044c04a7c5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10755,8 +10755,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   trace_amdgpu_dm_atomic_check_begin(state);
>  
>   ret = drm_atomic_helper_check_modeset(dev, state);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("drm_atomic_helper_check_modeset() failed\n");
>   goto fail;
> + }
>  
>   /* Check connector changes */
>   for_each_oldnew_connector_in_state(state, connector, old_con_state, 
> new_con_state, i) {
> @@ -10772,6 +10774,7 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  
>   new_crtc_state = drm_atomic_get_crtc_state(state, 
> new_con_state->crtc);
>   if (IS_ERR(new_crtc_state)) {
> + DRM_DEBUG_DRIVER("drm_atomic_get_crtc_state() 
> failed\n");
>   ret = PTR_ERR(new_crtc_state);
>   goto fail;
>   }
> @@ -10786,8 +10789,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, 
> new_crtc_state, i) {
>   if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>   ret = add_affected_mst_dsc_crtcs(state, crtc);
> - if (ret)
> + if (ret) {
> + 
> DRM_DEBUG_DRIVER("add_affected_mst_dsc_crtcs() failed\n");
>   goto fail;
> + }
>   }
>   }
>   }
> @@ -10802,19 +10807,25 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   continue;
>  
>   ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("amdgpu_dm_verify_lut_sizes() 
> failed\n");
>   goto fail;
> + }
>  
>   if (!new_crtc_state->enable)
>   continue;
>  
>   ret = drm_atomic_add_affected_connectors(state, crtc);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("drm_atomic_add_affected_connectors() 
> failed\n");
>   goto fail;
> + }
>  
>   ret = drm_atomic_add_affected_planes(state, crtc);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("drm_atomic_add_affected_planes() 
> failed\n");
>   goto fail;
> + }
>  
>   if (dm_old_crtc_state->dsc_force_changed)
>   new_crtc_state->mode_changed = true;
> @@ -10851,6 +10862,7 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  
>   if (IS_ERR(new_plane_state)) {
>   ret = PTR_ERR(new_plane_state);
> + DRM_DEBUG_DRIVER("new_plane_state is BAD\n");
>   goto fail;
>   }
>   }
> @@ -10863,8 +10875,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   new_plane_state,
>   false,
>   _and_validation_needed);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("dm_update_plane_state() failed\n");
>   goto fail;
> + }
>   }
>  
>   /* Disable all crtcs which require disable */
> @@ -10874,8 +10888,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  new_crtc_state,
>  false,
>  _and_validation_needed);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_DRIVER("DISABLE: dm_update_crtc_state() 
> failed\n");
>   goto fail;
> + }
>   }
>  
>   /* Enable all crtcs which require enable */
> @@ -10885,8 +10901,10 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>  new_crtc_state,
>  true,
>

回复: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Pan, Xinhui]

[AMD Official Use Only]

yes, a double check is needed.
how about change below.

As long as we detect such mismatch, it indicates another eviction is on going. 
return false here is reasonable.

@@ -1335,6 +1336,8 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct 
ttm_buffer_object *bo,
struct dma_fence *f;
int i;

+   if (bo->resource->mem_type != place->mem_type)
+   return false;
/* Swapout? */
if (bo->resource->mem_type == TTM_PL_SYSTEM)
return true;


发件人: Koenig, Christian 
发送时间: 2021年11月9日 21:18
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Exactly that's the reason why we should have the double check in TTM
I've mentioned in the other mail.

Christian.

Am 09.11.21 um 14:16 schrieb Pan, Xinhui:
> [AMD Official Use Only]
>
> Actually this patch does not totally fix the mismatch of lru list with 
> mem_type as mem_type is changed in ->move() and lru list is changed after 
> that.
>
> During this small period, another eviction could still happed and evict this 
> mismatched BO from sMam(say, its lru list is on vram domain) to sMem.
> 
> 发件人: Pan, Xinhui 
> 发送时间: 2021年11月9日 21:05
> 收件人: Koenig, Christian; amd-gfx@lists.freedesktop.org
> 抄送: dri-de...@lists.freedesktop.org
> 主题: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list
>
> Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.
>
> I think that amdgpu_bo_move() does support copy from sysMem to sysMem 
> correctly.
> maybe something below is needed.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index c83ef42ca702..aa63ae7ddf1e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
>  }
>  if (old_mem->mem_type == TTM_PL_SYSTEM &&
>  (new_mem->mem_type == TTM_PL_TT ||
> -new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
> +new_mem->mem_type == AMDGPU_PL_PREEMPT ||
> +new_mem->mem_type == TTM_PL_SYSTEM)) {
>  ttm_bo_move_null(bo, new_mem);
>  goto out;
>  }
>
> otherwise, amdgpu_move_blit() is called to do the system memory copy which 
> use a wrong address.
>   206 /* Map only what can't be accessed directly */
>   207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
>   208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
>   209 mm_cur->start;
>   210 return 0;
>   211 }
>
> line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such 
> addr, page fault happens.
>
>
> 
> 发件人: Koenig, Christian 
> 发送时间: 2021年11月9日 20:35
> 收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
> 抄送: dri-de...@lists.freedesktop.org
> 主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list
>
> Mhm, I'm not sure what the rational behind that is.
>
> Not moving the BO would make things less efficient, but should never
> cause a crash.
>
> Maybe we should add a CC: stable tag and push it to -fixes instead?
>
> Christian.
>
> Am 09.11.21 um 13:28 schrieb Pan, Xinhui:
>> [AMD Official Use Only]
>>
>> I hit vulkan cts test hang with navi23.
>>
>> dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
>> And some debug log also says amdgu copy one BO from system Domain to system 
>> Domain which is really weird.
>> 
>> 发件人: Koenig, Christian 
>> 发送时间: 2021年11月9日 20:20
>> 收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
>> 抄送: dri-de...@lists.freedesktop.org
>> 主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list
>>
>> Am 09.11.21 um 12:19 schrieb xinhui pan:
>>> After we move BO to a new memory region, we should put it to
>>> the new memory manager's lru list regardless we unlock the resv or not.
>>>
>>> Signed-off-by: xinhui pan 
>> Interesting find, did you trigger that somehow or did you just stumbled
>> over it by reading the code?
>>
>> Patch is Reviewed-by: Christian König , I will
>> pick that up for drm-misc-next.
>>
>> Thanks,
>> Christian.
>>
>>> ---
>>> drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>> index f1367107925b..e307004f0b28 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> @@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>>> ret = ttm_bo_evict(bo, ctx);
>>> if (locked)
>>> ttm_bo_unreserve(bo);
>>> + else
>>> + ttm_bo_move_to_lru_tail_unlocked(bo);
>>>

Re: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Christian König]

Exactly that's the reason why we should have the double check in TTM 
I've mentioned in the other mail.


Christian.

Am 09.11.21 um 14:16 schrieb Pan, Xinhui:

[AMD Official Use Only]

Actually this patch does not totally fix the mismatch of lru list with mem_type as 
mem_type is changed in ->move() and lru list is changed after that.

During this small period, another eviction could still happed and evict this 
mismatched BO from sMam(say, its lru list is on vram domain) to sMem.

发件人: Pan, Xinhui 
发送时间: 2021年11月9日 21:05
收件人: Koenig, Christian; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.

I think that amdgpu_bo_move() does support copy from sysMem to sysMem correctly.
maybe something below is needed.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c83ef42ca702..aa63ae7ddf1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
bool evict,
 }
 if (old_mem->mem_type == TTM_PL_SYSTEM &&
 (new_mem->mem_type == TTM_PL_TT ||
-new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
+new_mem->mem_type == AMDGPU_PL_PREEMPT ||
+new_mem->mem_type == TTM_PL_SYSTEM)) {
 ttm_bo_move_null(bo, new_mem);
 goto out;
 }

otherwise, amdgpu_move_blit() is called to do the system memory copy which use 
a wrong address.
  206 /* Map only what can't be accessed directly */
  207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
  208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
  209 mm_cur->start;
  210 return 0;
  211 }

line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such addr, 
page fault happens.



发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:35
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never
cause a crash.

Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:

[AMD Official Use Only]

I hit vulkan cts test hang with navi23.

dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
And some debug log also says amdgu copy one BO from system Domain to system 
Domain which is really weird.

发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:20
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Am 09.11.21 um 12:19 schrieb xinhui pan:

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 

Interesting find, did you trigger that somehow or did you just stumbled
over it by reading the code?

Patch is Reviewed-by: Christian König , I will
pick that up for drm-misc-next.

Thanks,
Christian.


---
drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked)
ttm_bo_unreserve(bo);
+ else
+ ttm_bo_move_to_lru_tail_unlocked(bo);

ttm_bo_put(bo);
return ret;

Re: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Christian König]

Yeah, but that should never happen in the first place.

Even when the BO is on the wrong LRU TTM should check that beforehand.

In other words when we pick a BO from the LRU we should still double 
check bo->resource->mem_type to make sure it is what we are searching for.


Christian.

Am 09.11.21 um 14:05 schrieb Pan, Xinhui:

[AMD Official Use Only]

Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.

I think that amdgpu_bo_move() does support copy from sysMem to sysMem correctly.
maybe something below is needed.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c83ef42ca702..aa63ae7ddf1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
bool evict,
 }
 if (old_mem->mem_type == TTM_PL_SYSTEM &&
 (new_mem->mem_type == TTM_PL_TT ||
-new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
+new_mem->mem_type == AMDGPU_PL_PREEMPT ||
+new_mem->mem_type == TTM_PL_SYSTEM)) {
 ttm_bo_move_null(bo, new_mem);
 goto out;
 }

otherwise, amdgpu_move_blit() is called to do the system memory copy which use 
a wrong address.
  206 /* Map only what can't be accessed directly */
  207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
  208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
  209 mm_cur->start;
  210 return 0;
  211 }

line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such addr, 
page fault happens.



发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:35
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never
cause a crash.

Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:

[AMD Official Use Only]

I hit vulkan cts test hang with navi23.

dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
And some debug log also says amdgu copy one BO from system Domain to system 
Domain which is really weird.

发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:20
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Am 09.11.21 um 12:19 schrieb xinhui pan:

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 

Interesting find, did you trigger that somehow or did you just stumbled
over it by reading the code?

Patch is Reviewed-by: Christian König , I will
pick that up for drm-misc-next.

Thanks,
Christian.


---
drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked)
ttm_bo_unreserve(bo);
+ else
+ ttm_bo_move_to_lru_tail_unlocked(bo);

ttm_bo_put(bo);
return ret;

回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Pan, Xinhui]

[AMD Official Use Only]

Actually this patch does not totally fix the mismatch of lru list with mem_type 
as mem_type is changed in ->move() and lru list is changed after that.

During this small period, another eviction could still happed and evict this 
mismatched BO from sMam(say, its lru list is on vram domain) to sMem.

发件人: Pan, Xinhui 
发送时间: 2021年11月9日 21:05
收件人: Koenig, Christian; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: 回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.

I think that amdgpu_bo_move() does support copy from sysMem to sysMem correctly.
maybe something below is needed.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c83ef42ca702..aa63ae7ddf1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
bool evict,
}
if (old_mem->mem_type == TTM_PL_SYSTEM &&
(new_mem->mem_type == TTM_PL_TT ||
-new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
+new_mem->mem_type == AMDGPU_PL_PREEMPT ||
+new_mem->mem_type == TTM_PL_SYSTEM)) {
ttm_bo_move_null(bo, new_mem);
goto out;
}

otherwise, amdgpu_move_blit() is called to do the system memory copy which use 
a wrong address.
 206 /* Map only what can't be accessed directly */
 207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
 208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
 209 mm_cur->start;
 210 return 0;
 211 }

line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such addr, 
page fault happens.



发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:35
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never
cause a crash.

Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:
> [AMD Official Use Only]
>
> I hit vulkan cts test hang with navi23.
>
> dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
> And some debug log also says amdgu copy one BO from system Domain to system 
> Domain which is really weird.
> 
> 发件人: Koenig, Christian 
> 发送时间: 2021年11月9日 20:20
> 收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
> 抄送: dri-de...@lists.freedesktop.org
> 主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list
>
> Am 09.11.21 um 12:19 schrieb xinhui pan:
>> After we move BO to a new memory region, we should put it to
>> the new memory manager's lru list regardless we unlock the resv or not.
>>
>> Signed-off-by: xinhui pan 
> Interesting find, did you trigger that somehow or did you just stumbled
> over it by reading the code?
>
> Patch is Reviewed-by: Christian König , I will
> pick that up for drm-misc-next.
>
> Thanks,
> Christian.
>
>> ---
>>drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
>>1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index f1367107925b..e307004f0b28 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>>ret = ttm_bo_evict(bo, ctx);
>>if (locked)
>>ttm_bo_unreserve(bo);
>> + else
>> + ttm_bo_move_to_lru_tail_unlocked(bo);
>>
>>ttm_bo_put(bo);
>>return ret;

回复: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Pan, Xinhui]

[AMD Official Use Only]

Yes, a stable tag is needed. vulkan guys say 5.14 hit this issue too.

I think that amdgpu_bo_move() does support copy from sysMem to sysMem correctly.
maybe something below is needed.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c83ef42ca702..aa63ae7ddf1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -485,7 +485,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, 
bool evict,
}
if (old_mem->mem_type == TTM_PL_SYSTEM &&
(new_mem->mem_type == TTM_PL_TT ||
-new_mem->mem_type == AMDGPU_PL_PREEMPT)) {
+new_mem->mem_type == AMDGPU_PL_PREEMPT ||
+new_mem->mem_type == TTM_PL_SYSTEM)) {
ttm_bo_move_null(bo, new_mem);
goto out;
}

otherwise, amdgpu_move_blit() is called to do the system memory copy which use 
a wrong address.
 206 /* Map only what can't be accessed directly */
 207 if (!tmz && mem->start != AMDGPU_BO_INVALID_OFFSET) {
 208 *addr = amdgpu_ttm_domain_start(adev, mem->mem_type) +
 209 mm_cur->start;
 210 return 0;
 211 }

line 208, *addr is zero. So when amdgpu_copy_buffer submit job with such addr, 
page fault happens.



发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:35
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never
cause a crash.

Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:
> [AMD Official Use Only]
>
> I hit vulkan cts test hang with navi23.
>
> dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
> And some debug log also says amdgu copy one BO from system Domain to system 
> Domain which is really weird.
> 
> 发件人: Koenig, Christian 
> 发送时间: 2021年11月9日 20:20
> 收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
> 抄送: dri-de...@lists.freedesktop.org
> 主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list
>
> Am 09.11.21 um 12:19 schrieb xinhui pan:
>> After we move BO to a new memory region, we should put it to
>> the new memory manager's lru list regardless we unlock the resv or not.
>>
>> Signed-off-by: xinhui pan 
> Interesting find, did you trigger that somehow or did you just stumbled
> over it by reading the code?
>
> Patch is Reviewed-by: Christian König , I will
> pick that up for drm-misc-next.
>
> Thanks,
> Christian.
>
>> ---
>>drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
>>1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index f1367107925b..e307004f0b28 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>>ret = ttm_bo_evict(bo, ctx);
>>if (locked)
>>ttm_bo_unreserve(bo);
>> + else
>> + ttm_bo_move_to_lru_tail_unlocked(bo);
>>
>>ttm_bo_put(bo);
>>return ret;

Re: 回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Christian König]

Mhm, I'm not sure what the rational behind that is.

Not moving the BO would make things less efficient, but should never 
cause a crash.


Maybe we should add a CC: stable tag and push it to -fixes instead?

Christian.

Am 09.11.21 um 13:28 schrieb Pan, Xinhui:

[AMD Official Use Only]

I hit vulkan cts test hang with navi23.

dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
And some debug log also says amdgu copy one BO from system Domain to system 
Domain which is really weird.

发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:20
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Am 09.11.21 um 12:19 schrieb xinhui pan:

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 

Interesting find, did you trigger that somehow or did you just stumbled
over it by reading the code?

Patch is Reviewed-by: Christian König , I will
pick that up for drm-misc-next.

Thanks,
Christian.


---
   drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
   ret = ttm_bo_evict(bo, ctx);
   if (locked)
   ttm_bo_unreserve(bo);
+ else
+ ttm_bo_move_to_lru_tail_unlocked(bo);

   ttm_bo_put(bo);
   return ret;

回复: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Pan, Xinhui]

[AMD Official Use Only]

I hit vulkan cts test hang with navi23.

dmesg says gmc page fault with address 0x0, 0x1000, 0x2000
And some debug log also says amdgu copy one BO from system Domain to system 
Domain which is really weird.

发件人: Koenig, Christian 
发送时间: 2021年11月9日 20:20
收件人: Pan, Xinhui; amd-gfx@lists.freedesktop.org
抄送: dri-de...@lists.freedesktop.org
主题: Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

Am 09.11.21 um 12:19 schrieb xinhui pan:
> After we move BO to a new memory region, we should put it to
> the new memory manager's lru list regardless we unlock the resv or not.
>
> Signed-off-by: xinhui pan 

Interesting find, did you trigger that somehow or did you just stumbled
over it by reading the code?

Patch is Reviewed-by: Christian König , I will
pick that up for drm-misc-next.

Thanks,
Christian.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index f1367107925b..e307004f0b28 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>   ret = ttm_bo_evict(bo, ctx);
>   if (locked)
>   ttm_bo_unreserve(bo);
> + else
> + ttm_bo_move_to_lru_tail_unlocked(bo);
>
>   ttm_bo_put(bo);
>   return ret;

Re: [PATCH] drm/ttm: Put BO in its memory manager's lru list

[Christian König]

Am 09.11.21 um 12:19 schrieb xinhui pan:

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 


Interesting find, did you trigger that somehow or did you just stumbled 
over it by reading the code?


Patch is Reviewed-by: Christian König , I will 
pick that up for drm-misc-next.


Thanks,
Christian.


---
  drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked)
ttm_bo_unreserve(bo);
+   else
+   ttm_bo_move_to_lru_tail_unlocked(bo);
  
  	ttm_bo_put(bo);

return ret;

[PATCH] drm/ttm: Put BO in its memory manager's lru list

[xinhui pan]

After we move BO to a new memory region, we should put it to
the new memory manager's lru list regardless we unlock the resv or not.

Signed-off-by: xinhui pan 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index f1367107925b..e307004f0b28 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -701,6 +701,8 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
ret = ttm_bo_evict(bo, ctx);
if (locked)
ttm_bo_unreserve(bo);
+   else
+   ttm_bo_move_to_lru_tail_unlocked(bo);
 
ttm_bo_put(bo);
return ret;
-- 
2.25.1

Re: [PATCH 2/2] drm/sched: serialize job_timeout and scheduler

[Daniel Vetter]

On Mon, Nov 08, 2021 at 03:39:17PM -0800, Rob Clark wrote:
> I stumbled across this thread when I ran into the same issue, while
> working out how to move drm/msm to use scheduler's retire +
> timeout/recovery (and get rid of our own mirror list of in-flight
> jobs).  We already have hw error detection enabled, and it can signal
> quite fast, so assuming the first job on the list is the guilty job
> just won't work.
> 
> But I was considering a slightly different approach to fixing this,
> instead just handling it all in drm_sched_main() and getting rid of
> the complicated kthread parking gymnastics.  Ie. something along the
> lines of:

So handling timeouts in the main sched thread wont work as soon as you
have multiple engines and reset that impacts across engines:

- Nothing is simplified since you still need to stop the other scheduler
  threads.

- You get deadlocks if 2 schedulers time out at the same time, and both
  want to stop the other one.

Hence workqueue. Now the rule for the wq is that you can only have one per
reset domain, so
- single engine you just take the one drm/sched provides
- if reset affects all your engines in the chip, then you allocate on in
  the drm_device and pass that to all
- if you have a complex of gpus all interconnected (e.g. xgmi hive for
  amd), then it's one wq for the entire hive

_All_ reset related things must be run on that workqueue or things breaks,
which means if you get hw fault that also needs to be run there. I guess
we should either patch drm/sched to check you call that function from the
right workqueue, or just handle it internally.
-Daniel

> 
> -
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 67382621b429..4d6ce775c316 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -764,6 +764,45 @@ static bool drm_sched_blocked(struct
> drm_gpu_scheduler *sched)
> return false;
>  }
> 
> +static bool handle_timeout(struct drm_gpu_scheduler *sched)
> +{
> +   struct drm_sched_job *bad;
> +
> +   if (!sched->has_timeout)
> +   return false;
> +
> +   sched->has_timeout = false;
> +
> +   spin_lock(>job_list_lock);
> +   bad = list_first_entry_or_null(>pending_list,
> +  struct drm_sched_job, list);
> +
> +   if (!bad) {
> +   spin_unlock(>job_list_lock);
> +   return false;
> +   }
> +
> +   spin_unlock(>job_list_lock);
> +
> +   if (sched->timeout_wq == system_wq) {
> +   /*
> +* If driver has no specific requirements about serializing
> +* reset wrt. other engines, just call timedout_job() directly
> +*/
> +   sched->ops->timedout_job(job);
> +   } else {
> +   /*
> +* Otherwise queue it on timeout_wq and wait for it to 
> complete
> +*/
> +   ... more typing needed here ...
> +   }
> +
> +   if (sched->free_guilty) {
> +   sched->ops->free_job(job);
> +   sched->free_guilty = false;
> +   }
> +}
> +
>  /**
>   * drm_sched_main - main scheduler thread
>   *
> @@ -787,6 +826,7 @@ static int drm_sched_main(void *param)
> 
> wait_event_interruptible(sched->wake_up_worker,
>  (cleanup_job =
> drm_sched_get_cleanup_job(sched)) ||
> +handle_timeout(sched) ||
>  (!drm_sched_blocked(sched) &&
>   (entity =
> drm_sched_select_entity(sched))) ||
>  kthread_should_stop());
> -
> 
> drm_sched_fault() and the sw timeout handler would just set
> sched->has_timeout and kick sched->wake_up_worker.
> 
> And since we handle the timeout case after
> drm_sched_get_cleanup_job(), we know that all of the successfully
> completed jobs have already been popped off the list, and won't be
> unfairly maligned.
> 
> BR,
> -R
> 
> On Tue, Aug 31, 2021 at 6:29 PM Liu, Monk  wrote:
> >
> > [AMD Official Use Only]
> >
> > Okay, I will reprepare this patch
> >
> > Thanks
> >
> > --
> > Monk Liu | Cloud-GPU Core team
> > --
> >
> > -Original Message-
> > From: Daniel Vetter 
> > Sent: Tuesday, August 31, 2021 9:02 PM
> > To: Liu, Monk 
> > Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Chen, 
> > Jingwen 
> > Subject: Re: [PATCH 2/2] drm/sched: serialize job_timeout and scheduler
> >
> > On Tue, Aug 31, 2021 at 02:59:02PM +0200, Daniel Vetter wrote:
> > > Can we please have some actual commit message here, with detailed
> > > explanation of the race/bug/whatever, how you fix it and why this is
> > > the best option?
> > >
> > > On Tue, Aug 31, 

RE: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting

[Quan, Evan]

[AMD Official Use Only]



> -Original Message-
> From: Lazar, Lijo 
> Sent: Tuesday, November 9, 2021 3:29 PM
> To: Koenig, Christian ; Borislav Petkov
> ; Paul Menzel ; Liu, Leo
> 
> Cc: Deucher, Alexander ; Quan, Evan
> ; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/9/2021 12:46 PM, Christian König wrote:
> > Am 08.11.21 um 15:41 schrieb Lazar, Lijo:
> >>
> >>
> >> On 11/8/2021 7:44 PM, Christian König wrote:
> >>> Am 08.11.21 um 12:15 schrieb Borislav Petkov:
>  On Mon, Nov 08, 2021 at 09:51:03AM +0100, Paul Menzel wrote:
> > Please elaborate the kind of issues.
>  It fails to reboot on Carrizo-based laptops.
> >>>
> >>> That doesn't necessary sounds like a good idea to me then.
> >>>
> >>> What exactly is going wrong here? And what is the rational that we
> >>> must fix this by avoiding updating the current state?
> >>>
> >>
> >> Reboot will trigger a suspend of IPs. As part of UVD/VCE suspend, now
> >> there is an added logic to power gate the IP as part of suspend
> >> sequence. In case of UVD/VCE, power gating would have already
> >> happened as part of idle work execution.
> >>
> >> In any case, power gating is done by SMU FW. The assumption here is -
> >> the logic to power gate IP could involve register programming. AFAIK,
> >> accessing some UVD/VCE regs during powergate state could cause a hang
> >> unless the anti-hang mechanism is not programmed. That means either
> >> FW or driver needs to track the state of IP before accessing those
> >> regs and in this case probably FW is assuming driver to be responsible.
> >> i.e., not to call power off again if it's already powered down.
> >>
> >> Though that seems to be a bad assumption on part of FW, it is still a
> >> possibility. Haven't seen the actual FW code, it's a very old program.
> >
> >
> > Ok guys I've double checked the git history and found that this here
> > is not as it is intended to be.
> >
> > See the code in question was just added in August by the following commit:
> >
> > commit 859e4659273f1df3a23e3990826bcb41e85f68a5
> > Author: Evan Quan 
> > Date:   Thu Aug 19 12:07:59 2021 +0800
> >
> >      drm/amdgpu: add missing cleanups for more ASICs on UVD/VCE
> > suspend
> >
> >      This is a supplement for commit below:
> >      "drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE on suspend".
> >
> >      Signed-off-by: Evan Quan 
> >      Reviewed-by: Guchun Chen 
> >      Signed-off-by: Alex Deucher 
> >
> > Before that we where just not touching the UVD power state at all
> > during suspend and so we won't had a problem in the first place.
> >
> > So what we should do instead is to just revert commit
> > 859e4659273f1df3a23e3990826bcb41e85f68a5 with a proper fixes Tag and
> > explanation why that is a bad idea.
> >
> 
> Yeah, right. If I remember correctly, this commit was originally added
> to fix hangs with S3 suspend/resume cycles triggered during video
> playback. Reverting could bring back that one. Evan will know more
> background details.
[Quan, Evan] Yes, 859e4659273f1df3a23e3990826bcb41e85f68a5 aimed the issue 
below. It cannot be reverted.
"If the UVD/VCE is still using on reboot/suspend triggered, idle work will be 
not triggered and the add-on power gate logic can help to put the Ips into 
correct gate state."

BR
Evan
> 
> Thanks,
> Lijo
> 
> > Regards,
> > Christian.
> >
> >
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> See we usually assume that updating to the already set state is
> >>> unproblematic and that here sounds like just trying to mitigated some
> >>> issues instead of fixing the root cause.
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> 
>  Whoever commits this, pls add
> 
>  Link: https://lore.kernel.org/r/yv81vidwqlwva...@zn.tnic
> 
>  so that it is clear what the whole story way.
> 
>  Thx.
> 
> >>>
> >

RE: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting

[Quan, Evan]

[AMD Official Use Only]



> -Original Message-
> From: Lazar, Lijo 
> Sent: Tuesday, November 9, 2021 12:15 PM
> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander ; Borislav Petkov
> 
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/9/2021 9:10 AM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -Original Message-
> >> From: Lazar, Lijo 
> >> Sent: Monday, November 8, 2021 7:16 PM
> >> To: Quan, Evan ; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander ; Borislav Petkov
> >> 
> >> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> >> setting
> >>
> >>
> >>
> >> On 11/8/2021 10:17 AM, Evan Quan wrote:
> >>> Just bail out if the target IP block is already in the desired
> >>> powergate/ungate state. This can avoid some duplicate settings which
> >>> sometime may cause unexpected issues.
> >>>
> >>> Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> >>> Signed-off-by: Evan Quan 
> >>> Tested-by: Borislav Petkov 
> >>> ---
> >>>drivers/gpu/drm/amd/amdgpu/amdgpu.h  |  7 +++
> >>>drivers/gpu/drm/amd/include/amd_shared.h |  3 ++-
> >>>drivers/gpu/drm/amd/pm/amdgpu_dpm.c  | 13 -
> >>>drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c |  3 +++
> >>>drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c|  3 +++
> >>>drivers/gpu/drm/amd/pm/powerplay/si_dpm.c|  3 +++
> >>>drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c|  3 +++
> >>>7 files changed, 33 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> index b85b67a88a3d..19fa21ad8a44 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>> @@ -767,9 +767,16 @@ enum amd_hw_ip_block_type {
> >>>#define HW_ID_MAX  300
> >>>#define IP_VERSION(mj, mn, rv) (((mj) << 16) | ((mn) << 8) |
> >>> (rv))
> >>>
> >>> +enum amd_ip_powergate_state {
> >>> + POWERGATE_STATE_INITIAL,
> >>> + POWERGATE_STATE_GATE,
> >>> + POWERGATE_STATE_UNGATE,
> >>> +};
> >>> +
> >>
> >> To reflect the current state, they could be named like
> >> POWERGATE_STATE_UNKNOWN/ON/OFF.
> > [Quan, Evan] This may be more confusing. POWERGATE_STATE_ON means
> "gfx on" or "gate on which means gfx off"?
> 
> What I meant is -
>   PG_STATE_ON - Power gated
>   PG_STATE_OFF - Not power gated
[Quan, Evan] Yeah, but I mean other user may be confusing about these. Since 
when we take about the PG state, we usually use "Gate" or "Ungate". How about 
POWER_STATE_ON - Power on/ungate
POWER_STATE_OFF - Power off/gate ?

BR
Evan
> Thanks,
> Lijo
> 
> >>
> >>
> >>>struct amd_powerplay {
> >>>   void *pp_handle;
> >>>   const struct amd_pm_funcs *pp_funcs;
> >>> + atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
> >>
> >> Maybe add another field in amdgpu_ip_block_status? Downside is it
> >> won't reflect the PG state achieved through paths other than PMFW
> >> control and ipblock needs to be queried through
> >> amdgpu_device_ip_get_ip_block()
> > [Quan, Evan] Yes, we will need to track pg state other than PMFW control
> then.
> > That will need extra effort and seems unnecessary considering there is no
> such use case(need to know the PG state out of PMFW control).
> >
> > BR
> > Evan
> >>
> >> Thanks,
> >> Lijo
> >>
> >>>};
> >>>
> >>>/* polaris10 kickers */
> >>> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> >> b/drivers/gpu/drm/amd/include/amd_shared.h
> >>> index f1a46d16f7ea..4b9e68a79f06 100644
> >>> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> >>> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> >>> @@ -98,7 +98,8 @@ enum amd_ip_block_type {
> >>>   AMD_IP_BLOCK_TYPE_ACP,
> >>>   AMD_IP_BLOCK_TYPE_VCN,
> >>>   AMD_IP_BLOCK_TYPE_MES,
> >>> - AMD_IP_BLOCK_TYPE_JPEG
> >>> + AMD_IP_BLOCK_TYPE_JPEG,
> >>> + AMD_IP_BLOCK_TYPE_NUM,
> >>>};
> >>>
> >>>enum amd_clockgating_state {
> >>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >> b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> index 03581d5b1836..a6b337b6f4a9 100644
> >>> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> >>> @@ -927,6 +927,14 @@ int
> >> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> >> uint32_t block
> >>>{
> >>>   int ret = 0;
> >>>   const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> >>> + enum amd_ip_powergate_state pg_state =
> >>> + gate ? POWERGATE_STATE_GATE :
> >> POWERGATE_STATE_UNGATE;
> >>> +
> >>> + if (atomic_read(>powerplay.pg_state[block_type]) ==
> >> pg_state) {
> >>> + dev_dbg(adev->dev, "IP block%d already in the target %s
> >> state!",
> >>> + block_type, gate ? "gate" : "ungate");
> >>> + return 0;
> >>> + }
> >>>
> >>>   switch (block_type) {