Re: [PATCH 1/2] drm/amdgpu/debugfs: fix memory leak when pm_runtime_get_sync failed

[Christian König]

Am 16.06.20 um 08:30 schrieb Chen Tao:

Fix memory leak in amdgpu_debugfs_gpr_read not freeing data when
pm_runtime_get_sync failed.

Fixes: a9ffe2a983383 ("drm/amdgpu/debugfs: properly handle runtime pm")
Signed-off-by: Chen Tao 


Probably better to remove the duplication of result and r here and then 
use "goto err".


Regards,
Christian


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 1a4894fa3693..bea8c36a53a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -862,8 +862,10 @@ static ssize_t amdgpu_debugfs_gpr_read(struct file *f, 
char __user *buf,
return -ENOMEM;
  
  	r = pm_runtime_get_sync(adev->ddev->dev);

-   if (r < 0)
+   if (r < 0) {
+   kfree(data);
return r;
+   }
  
  	r = amdgpu_virt_enable_access_debugfs(adev);

if (r < 0)


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/radeon: Fix reference count leaks caused by pm_runtime_get_sync

[Quan, Evan]

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Evan Quan 

-Original Message-
From: amd-gfx  On Behalf Of Aditya Pakki
Sent: Sunday, June 14, 2020 10:21 AM
To: pakki...@umn.edu
Cc: wu000...@umn.edu; David Airlie ; k...@umn.edu; 
linux-ker...@vger.kernel.org; amd-gfx@lists.freedesktop.org; 
dri-de...@lists.freedesktop.org; Daniel Vetter ; Deucher, 
Alexander ; Koenig, Christian 

Subject: [PATCH] drm/radeon: Fix reference count leaks caused by 
pm_runtime_get_sync

On calling pm_runtime_get_sync() the reference count of the device is 
incremented. In case of failure, decrement the reference count before returning 
the error.

Signed-off-by: Aditya Pakki 
---
 drivers/gpu/drm/radeon/radeon_display.c | 4 +++-
 drivers/gpu/drm/radeon/radeon_drv.c | 4 +++-
 drivers/gpu/drm/radeon/radeon_kms.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index 35db79a168bf..df1a7eb73651 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -635,8 +635,10 @@ radeon_crtc_set_config(struct drm_mode_set *set,
 dev = set->crtc->dev;

 ret = pm_runtime_get_sync(dev->dev);
-if (ret < 0)
+if (ret < 0) {
+pm_runtime_put_autosuspend(dev->dev);
 return ret;
+}

 ret = drm_crtc_helper_set_config(set, ctx);

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index bbb0883e8ce6..62b5069122cc 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -549,8 +549,10 @@ long radeon_drm_ioctl(struct file *filp,
 long ret;
 dev = file_priv->minor->dev;
 ret = pm_runtime_get_sync(dev->dev);
-if (ret < 0)
+if (ret < 0) {
+pm_runtime_put_autosuspend(dev->dev);
 return ret;
+}

 ret = drm_ioctl(filp, cmd, arg);

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index c5d1dc9618a4..99ee60f8b604 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -638,8 +638,10 @@ int radeon_driver_open_kms(struct drm_device *dev, struct 
drm_file *file_priv)
 file_priv->driver_priv = NULL;

 r = pm_runtime_get_sync(dev->dev);
-if (r < 0)
+if (r < 0) {
+pm_runtime_put_autosuspend(dev->dev);
 return r;
+}

 /* new gpu have virtual address space support */
 if (rdev->family >= CHIP_CAYMAN) {
--
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cevan.quan%40amd.com%7Cc86101e02ef24c52b36408d810fdcc14%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637278029582429567&sdata=qtKTCV33q8l2GTxMUX0nlJ4fV32dXaLH7y6hymksQEo%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: Fix incorrect firmware size calculation

[Quan, Evan]

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Evan Quan 

From: amd-gfx  On Behalf Of Guo Lei
Sent: Monday, June 15, 2020 3:14 PM
To: amd-gfx@lists.freedesktop.org
Subject: [PATCH] drm/amdgpu: Fix incorrect firmware size calculation


>From 014162f69b909a59c241e7f73c3630d1da34696c Mon Sep 17 00:00:00 2001

From: Lei Guo mailto:raykwok1...@163.com>>

Date: Mon, 15 Jun 2020 13:54:26 +0800

Subject: [PATCH] drm/amdgpu: Fix incorrect firmware size calculation



[WHY]

The memcpy() function copies n bytes from memory area src to memory area

dest. So specify the firmware size in bytes.



[How]

Correct the calculation.



Signed-off-by: Lei Guo mailto:raykwok1...@163.com>>

Reviewed-by: Junwei Zhang mailto:zjunwei...@163.com>>

---

 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)



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

index 6b94587df407..c3e59b765268 100644

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

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

@@ -1960,7 +1960,7 @@ static int gfx_v9_0_mec_init(struct amdgpu_device *adev)

  fw_data = (const __le32 *)

  (adev->gfx.mec_fw->data +

  le32_to_cpu(mec_hdr->header.ucode_array_offset_bytes));

- fw_size = le32_to_cpu(mec_hdr->header.ucode_size_bytes) / 4;

+fw_size = le32_to_cpu(mec_hdr->header.ucode_size_bytes);



  r = amdgpu_bo_create_reserved(adev, mec_hdr->header.ucode_size_bytes,

PAGE_SIZE, 
AMDGPU_GEM_DOMAIN_GTT,

--

2.17.1




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu/pm: update comment to clarify Overdrive interfaces

[Quan, Evan]

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Evan Quan 

-Original Message-
From: amd-gfx  On Behalf Of Alex Deucher
Sent: Tuesday, June 16, 2020 4:26 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander 
Subject: [PATCH] drm/amdgpu/pm: update comment to clarify Overdrive interfaces

Vega10 and previous asics use one interface, vega20 and newer use another.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 395ddbe2461c..5a8e177e4f56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -696,7 +696,7 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
  * default power levels, write "r" (reset) to the file to reset them.
  *
  *
- * < For Vega20 >
+ * < For Vega20 and newer ASICs >
  *
  * Reading the file will display:
  *
--
2.25.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cevan.quan%40amd.com%7C37de4916ddd346b7d21008d8116a4b0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637278495558075603&sdata=AahmUZ6thnm%2FawZML1Y6pNGl%2BDzWovnYImLtfUaLOrc%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: fix documentation around busy_percentage

[Quan, Evan]

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Evan Quan 

-Original Message-
From: amd-gfx  On Behalf Of Alex Deucher
Sent: Tuesday, June 16, 2020 4:40 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander 
Subject: [PATCH] drm/amdgpu: fix documentation around busy_percentage

Add rename the gpu busy percentage for consistency and add the mem busy 
percentage documentation.

Signed-off-by: Alex Deucher 
---
 Documentation/gpu/amdgpu.rst   | 9 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst index 
4cc74325bf91..17112352f605 100644
--- a/Documentation/gpu/amdgpu.rst
+++ b/Documentation/gpu/amdgpu.rst
@@ -197,11 +197,14 @@ pp_power_profile_mode  .. kernel-doc:: 
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
:doc: pp_power_profile_mode

-busy_percent
-
+*_busy_percent
+~~

 .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
-   :doc: busy_percent
+   :doc: gpu_busy_percent
+
+.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+   :doc: mem_busy_percent

 GPU Product Information
 ===
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 5a8e177e4f56..42bbdf49458e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1668,7 +1668,7 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct 
device *dev,  }

 /**
- * DOC: busy_percent
+ * DOC: gpu_busy_percent
  *
  * The amdgpu driver provides a sysfs API for reading how busy the GPU
  * is as a percentage.  The file gpu_busy_percent is used for this.
--
2.25.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cevan.quan%40amd.com%7C691ad901d90841f2421008d8116c520a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637278504263353044&sdata=HMgi9aI7smavcUz4XyNbKUvCPSAjoxns7HRzDrqNf%2Bc%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: The inherent defect of the AMDGPU driver about hotplug

[Aaron Chou]

Yes, I agree.

On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher  wrote:
>
> On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou  wrote:
> >
> > About one month ago, I have asked a question about HDMI hotplug, the link 
> > is:
> >
> > https://gitlab.freedesktop.org/drm/amd/-/issues/1135#note_492460
> >
> > And I have put one patch to fix this, as follows:
> >
> >  39 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  40 index f355d9a752d2..ee657db9a228 100644
> >  41 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  42 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  43 @@ -973,7 +973,7 @@ amdgpu_connector_dvi_detect(struct
> > drm_connector *connector, bool force)
> >  44 const struct drm_encoder_helper_funcs *encoder_funcs;
> >  45 int r;
> >  46 enum drm_connector_status ret =
> > connector_status_disconnected;
> >  47 -   bool dret = false, broken_edid = false;
> >  48 +   bool dret = false, broken_edid = false, undefined_flag =
> > false;
> >  49
> >  50 if (!drm_kms_helper_is_poll_worker()) {
> >  51 r = pm_runtime_get_sync(connector->dev->dev);
> >  52 @@ -988,7 +988,12 @@ amdgpu_connector_dvi_detect(struct
> > drm_connector *connector, bool force)
> >  53
> >  54 if (amdgpu_connector->ddc_bus)
> >  55 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> > false);
> >  56 -   if (dret) {
> >  57 +
> >  58 +   /* Check the HDMI HPD pin status again */
> >  59 +   if (!amdgpu_display_hpd_sense(adev,
> > amdgpu_connector->hpd.hpd))
> >  60 +   undefined_flag = true;
> >  61 +
> >  62 +   if (dret && !undefined_flag) {
> >  63 amdgpu_connector->detected_by_load = false;
> >  64 amdgpu_connector_free_edid(connector);
> >  65 amdgpu_connector_get_edid(connector);
> >
> > Maybe the fix is sloppy, so I write the another patch:
> >
> >  16 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  17 index c770d73352a7..bb59ebc9a6c8 100644
> >  18 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  19 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> >  20 @@ -991,6 +991,7 @@ amdgpu_connector_dvi_detect(struct
> > drm_connector *connector, bool force)
> >  21 if (amdgpu_connector->ddc_bus)
> >  22 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> > false);
> >  23 if (dret) {
> >  24 +   schedule_work(&adev->hotplug_work);
> >  25 amdgpu_connector->detected_by_load = false;
> >  26 amdgpu_connector_free_edid(connector);
> >  27 amdgpu_connector_get_edid(connector);
> >
> > Which is better, or neither？
>
> As per the last discussion:
>
> "This is the part I don't understand.  The logic already checks the HPD
> status in amdgpu_connector_check_hpd_status_unchanged().  Does it
> still report connected at that point?  After that it tries to read the
> EDID in amdgpu_display_ddc_probe().  If the monitor is disconnected,
> there should be no EDID so dret should be false.  We should try and
> figure out why the first HPD check reports connected and the EDID
> probe returns true."
>
> There is already an HPD probe in the current logic.  We should try and
> understand why we need a second one rather than just adding one.  Does
> a delay at the top of that function help?
>
> Alex
>
> >
> > --
> > Regards,
> > Aaron
> > ___
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: Fix reference count leaks.

[Felix Kuehling]

On 2020-06-13 15:32, wu000...@umn.edu wrote:

From: Qiushi Wu 

kobject_init_and_add() takes reference even when it fails.
If this function returns an error, kobject_put() must be called to
properly clean up the memory associated with the object.

Signed-off-by: Qiushi Wu 


Thank you. The patch is

Reviewed-by: Felix Kuehling 

I'm applying the patch to our amd-staging-drm-next branch.

Regards,
  Felix



---
  drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
  1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index bb77f7af2b6d..dc3c4149f860 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
  
  	ret = kobject_init_and_add(dev->kobj_node, &node_type,

sys_props.kobj_nodes, "%d", id);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(dev->kobj_node);
return ret;
+   }
  
  	dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);

if (!dev->kobj_mem)
@@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(mem->kobj, &mem_type,
dev->kobj_mem, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(mem->kobj);
return ret;
+   }
  
  		mem->attr.name = "properties";

mem->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(cache->kobj, &cache_type,
dev->kobj_cache, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(cache->kobj);
return ret;
+   }
  
  		cache->attr.name = "properties";

cache->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(iolink->kobj, &iolink_type,
dev->kobj_iolink, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(iolink->kobj);
return ret;
+   }
  
  		iolink->attr.name = "properties";

iolink->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
ret = kobject_init_and_add(sys_props.kobj_topology,
&sysprops_type,  &kfd_device->kobj,
"topology");
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(sys_props.kobj_topology);
return ret;
+   }
  
  		sys_props.kobj_nodes = kobject_create_and_add("nodes",

sys_props.kobj_topology);

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: fix multiple reference count leak

[Alex Deucher]

On Mon, Jun 15, 2020 at 3:27 AM Aditya Pakki  wrote:
>
> On calling pm_runtime_get_sync() the reference count of the device
> is incremented. In case of failure, decrement the
> reference count before returning the error.

Is this required if pm_runtime_get_sync() fails?

Alex

>
> Signed-off-by: Aditya Pakki 
> ---
>  drivers/gpu/drm/radeon/radeon_connectors.c | 20 +++-
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c 
> b/drivers/gpu/drm/radeon/radeon_connectors.c
> index fe12d9d91d7a..e30834434442 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -879,8 +879,10 @@ radeon_lvds_detect(struct drm_connector *connector, bool 
> force)
>
> if (!drm_kms_helper_is_poll_worker()) {
> r = pm_runtime_get_sync(connector->dev->dev);
> -   if (r < 0)
> +   if (r < 0) {
> +   pm_runtime_put_autosuspend(connector->dev->dev);
> return connector_status_disconnected;
> +   }
> }
>
> if (encoder) {
> @@ -1025,8 +1027,10 @@ radeon_vga_detect(struct drm_connector *connector, 
> bool force)
>
> if (!drm_kms_helper_is_poll_worker()) {
> r = pm_runtime_get_sync(connector->dev->dev);
> -   if (r < 0)
> +   if (r < 0) {
> +   pm_runtime_put_autosuspend(connector->dev->dev);
> return connector_status_disconnected;
> +   }
> }
>
> encoder = radeon_best_single_encoder(connector);
> @@ -1163,8 +1167,10 @@ radeon_tv_detect(struct drm_connector *connector, bool 
> force)
>
> if (!drm_kms_helper_is_poll_worker()) {
> r = pm_runtime_get_sync(connector->dev->dev);
> -   if (r < 0)
> +   if (r < 0) {
> +   pm_runtime_put_autosuspend(connector->dev->dev);
> return connector_status_disconnected;
> +   }
> }
>
> encoder = radeon_best_single_encoder(connector);
> @@ -1247,8 +1253,10 @@ radeon_dvi_detect(struct drm_connector *connector, 
> bool force)
>
> if (!drm_kms_helper_is_poll_worker()) {
> r = pm_runtime_get_sync(connector->dev->dev);
> -   if (r < 0)
> +   if (r < 0) {
> +   pm_runtime_put_autosuspend(connector->dev->dev);
> return connector_status_disconnected;
> +   }
> }
>
> if (radeon_connector->detected_hpd_without_ddc) {
> @@ -1657,8 +1665,10 @@ radeon_dp_detect(struct drm_connector *connector, bool 
> force)
>
> if (!drm_kms_helper_is_poll_worker()) {
> r = pm_runtime_get_sync(connector->dev->dev);
> -   if (r < 0)
> +   if (r < 0) {
> +   pm_runtime_put_autosuspend(connector->dev->dev);
> return connector_status_disconnected;
> +   }
> }
>
> if (!force && radeon_check_hpd_status_unchanged(connector)) {
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Luck, Tony]

> Are we planning to keep PASID live once a task has used it once or are we 
> going to swap it lazily?  If the latter, a percpu variable might be better.

Current plan is "touch it once and the task owns it until exit(2)"

Maybe someday in the future when we have data on how applications
actually use accelerators we could look at something more complex
if usage patterns look like it would be beneficial.

-Tony
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Luck, Tony]

> So what’s the RDMSR for?  Surely you
> have some state somewhere that says “this task has a PASID.”
> Can’t you just make sure that stays in sync with the MSR?  Then, on #GP, if 
> the task already has a PASID, you know the MSR is set.

We have state that says the process ("mm") has been allocated a PASID. But not 
for each task.

E.g. a process may clone a bunch of tasks, then one of them opens a device that 
needs
a PASID.   We did internally have patches to go hunt down all those other tasks 
and
force a PASID onto each. But the code was big and ugly. Also maybe the wrong 
thing
to do if those threads didn't ever need to access the device.

PeterZ suggested that we can add a bit to the task structure for this purpose.

Fenghua is hesitant about adding an x86 only bit there.

-Tony
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Andy Lutomirski]

> On Jun 15, 2020, at 1:17 PM, Fenghua Yu  wrote:
> 
> Hi, Peter,
> 
>> On Mon, Jun 15, 2020 at 09:09:28PM +0200, Peter Zijlstra wrote:
>>> On Mon, Jun 15, 2020 at 11:55:29AM -0700, Fenghua Yu wrote:
>>> 
>>> Or do you suggest to add a random new flag in struct thread_info instead
>>> of a TIF flag?
>> 
>> Why thread_info? What's wrong with something simple like the below. It
>> takes a bit from the 'strictly current' flags word.
>> 
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index b62e6aaf28f0..fca830b97055 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -801,6 +801,9 @@ struct task_struct {
>>/* Stalled due to lack of memory */
>>unsignedin_memstall:1;
>> #endif
>> +#ifdef CONFIG_PCI_PASID
>> +unsignedhas_valid_pasid:1;
>> +#endif
>> 
>>unsigned longatomic_flags; /* Flags requiring atomic access. 
>> */
>> 
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 142b23645d82..10b3891be99e 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -955,6 +955,10 @@ static struct task_struct *dup_task_struct(struct 
>> task_struct *orig, int node)
>>tsk->use_memdelay = 0;
>> #endif
>> 
>> +#ifdef CONFIG_PCI_PASID
>> +tsk->has_valid_pasid = 0;
>> +#endif
>> +
>> #ifdef CONFIG_MEMCG
>>tsk->active_memcg = NULL;
>> #endif
> 
> The PASID MSR is x86 specific although PASID is PCIe concept and per-mm.
> Checking if the MSR has valid PASID (bit31=1) is an x86 specifc work.
> The flag should be cleared in cloned()/forked() and is only set and
> read in fixup() in x86 #GP for heuristic. It's not used anywhere outside
> of x86.
> 
> That's why we think the flag should be in x86 struct thread_info instead
> of in generice struct task_struct.
> 

Are we planning to keep PASID live once a task has used it once or are we going 
to swap it lazily?  If the latter, a percpu variable might be better.

> Please advice.
> 
> Thanks.
> 
> -Fenghua
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Andy Lutomirski]

> On Jun 15, 2020, at 1:56 PM, Luck, Tony  wrote:
> 
> 
>> 
>> Are we planning to keep PASID live once a task has used it once or are we 
>> going to swap it lazily?  If the latter, a percpu variable might be better.
> 
> Current plan is "touch it once and the task owns it until exit(2)"
> 
> Maybe someday in the future when we have data on how applications
> actually use accelerators we could look at something more complex
> if usage patterns look like it would be beneficial.
> 
> 

So what’s the RDMSR for?  Surely you
have some state somewhere that says “this task has a PASID.”  Can’t you just 
make sure that stays in sync with the MSR?  Then, on #GP, if the task already 
has a PASID, you know the MSR is set.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Fenghua Yu]

Hi, Peter,

On Mon, Jun 15, 2020 at 09:09:28PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 11:55:29AM -0700, Fenghua Yu wrote:
> 
> > Or do you suggest to add a random new flag in struct thread_info instead
> > of a TIF flag?
> 
> Why thread_info? What's wrong with something simple like the below. It
> takes a bit from the 'strictly current' flags word.
> 
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b62e6aaf28f0..fca830b97055 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -801,6 +801,9 @@ struct task_struct {
>   /* Stalled due to lack of memory */
>   unsignedin_memstall:1;
>  #endif
> +#ifdef CONFIG_PCI_PASID
> + unsignedhas_valid_pasid:1;
> +#endif
>  
>   unsigned long   atomic_flags; /* Flags requiring atomic 
> access. */
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 142b23645d82..10b3891be99e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -955,6 +955,10 @@ static struct task_struct *dup_task_struct(struct 
> task_struct *orig, int node)
>   tsk->use_memdelay = 0;
>  #endif
>  
> +#ifdef CONFIG_PCI_PASID
> + tsk->has_valid_pasid = 0;
> +#endif
> +
>  #ifdef CONFIG_MEMCG
>   tsk->active_memcg = NULL;
>  #endif

The PASID MSR is x86 specific although PASID is PCIe concept and per-mm.
Checking if the MSR has valid PASID (bit31=1) is an x86 specifc work.
The flag should be cleared in cloned()/forked() and is only set and
read in fixup() in x86 #GP for heuristic. It's not used anywhere outside
of x86.

That's why we think the flag should be in x86 struct thread_info instead
of in generice struct task_struct.

Please advice.

Thanks.

-Fenghua
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix documentation around busy_percentage

[Nirmoy]

Reviewed-by: Nirmoy Das 

On 6/15/20 10:40 PM, Alex Deucher wrote:

Add rename the gpu busy percentage for consistency and
add the mem busy percentage documentation.

Signed-off-by: Alex Deucher 
---
  Documentation/gpu/amdgpu.rst   | 9 ++---
  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
  2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
index 4cc74325bf91..17112352f605 100644
--- a/Documentation/gpu/amdgpu.rst
+++ b/Documentation/gpu/amdgpu.rst
@@ -197,11 +197,14 @@ pp_power_profile_mode
  .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
 :doc: pp_power_profile_mode
  
-busy_percent

-
+*_busy_percent
+~~
  
  .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c

-   :doc: busy_percent
+   :doc: gpu_busy_percent
+
+.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+   :doc: mem_busy_percent
  
  GPU Product Information

  ===
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 5a8e177e4f56..42bbdf49458e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1668,7 +1668,7 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct 
device *dev,
  }
  
  /**

- * DOC: busy_percent
+ * DOC: gpu_busy_percent
   *
   * The amdgpu driver provides a sysfs API for reading how busy the GPU
   * is as a percentage.  The file gpu_busy_percent is used for this.

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix documentation around busy_percentage

[Alex Deucher]

Add rename the gpu busy percentage for consistency and
add the mem busy percentage documentation.

Signed-off-by: Alex Deucher 
---
 Documentation/gpu/amdgpu.rst   | 9 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
index 4cc74325bf91..17112352f605 100644
--- a/Documentation/gpu/amdgpu.rst
+++ b/Documentation/gpu/amdgpu.rst
@@ -197,11 +197,14 @@ pp_power_profile_mode
 .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
:doc: pp_power_profile_mode
 
-busy_percent
-
+*_busy_percent
+~~
 
 .. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
-   :doc: busy_percent
+   :doc: gpu_busy_percent
+
+.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+   :doc: mem_busy_percent
 
 GPU Product Information
 ===
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 5a8e177e4f56..42bbdf49458e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1668,7 +1668,7 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct 
device *dev,
 }
 
 /**
- * DOC: busy_percent
+ * DOC: gpu_busy_percent
  *
  * The amdgpu driver provides a sysfs API for reading how busy the GPU
  * is as a percentage.  The file gpu_busy_percent is used for this.
-- 
2.25.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/pm: update comment to clarify Overdrive interfaces

[Nirmoy]

Acked-by: Nirmoy Das 

On 6/15/20 10:25 PM, Alex Deucher wrote:

Vega10 and previous asics use one interface, vega20 and newer
use another.

Signed-off-by: Alex Deucher 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 395ddbe2461c..5a8e177e4f56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -696,7 +696,7 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
   * default power levels, write "r" (reset) to the file to reset them.
   *
   *
- * < For Vega20 >
+ * < For Vega20 and newer ASICs >
   *
   * Reading the file will display:
   *

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/pm: update comment to clarify Overdrive interfaces

[Alex Deucher]

Vega10 and previous asics use one interface, vega20 and newer
use another.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 395ddbe2461c..5a8e177e4f56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -696,7 +696,7 @@ static ssize_t amdgpu_set_pp_table(struct device *dev,
  * default power levels, write "r" (reset) to the file to reset them.
  *
  *
- * < For Vega20 >
+ * < For Vega20 and newer ASICs >
  *
  * Reading the file will display:
  *
-- 
2.25.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amd/display: Update DCN3 bounding box

[Lakha, Bhawanpreet]

[AMD Official Use Only - Internal Distribution Only]

Series is

Reviewed-by: Bhawanpreet.Lakha

From: amd-gfx  on behalf of Jerry 
(Fangzhi) Zuo 
Sent: June 15, 2020 3:01 PM
To: amd-gfx@lists.freedesktop.org 
Cc: Deucher, Alexander ; Zuo, Jerry 
; Wentland, Harry ; Kazlauskas, 
Nicholas ; Lee, Alvin 
Subject: [PATCH 1/2] drm/amd/display: Update DCN3 bounding box

From: Alvin Lee 

[Why]
We want to update the bounding box to have more granular control of the
DCFCLK.

[How]
Setup DCFCLK to use STA values and also optimal values based on
UCLK.

Signed-off-by: Alvin Lee 
Signed-off-by: Jerry (Fangzhi) Zuo 
Reviewed-by: Hersen Wu 
---
 .../drm/amd/display/dc/dcn30/dcn30_resource.c | 114 --
 1 file changed, 102 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c 
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
index 654fdbbff86b..821bde9a376e 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
@@ -2237,9 +2237,41 @@ bool dcn30_validate_bandwidth(struct dc *dc, struct 
dc_state *context,
 return out;
 }

+static void get_optimal_dcfclk_fclk_for_uclk(unsigned int uclk_mts,
+   unsigned int 
*optimal_dcfclk,
+   unsigned int 
*optimal_fclk)
+{
+   double bw_from_dram, bw_from_dram1, bw_from_dram2;
+
+   bw_from_dram1 = uclk_mts * dcn3_0_soc.num_chans *
+   dcn3_0_soc.dram_channel_width_bytes * 
(dcn3_0_soc.max_avg_dram_bw_use_normal_percent / 100);
+   bw_from_dram2 = uclk_mts * dcn3_0_soc.num_chans *
+   dcn3_0_soc.dram_channel_width_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100);
+
+   bw_from_dram = (bw_from_dram1 < bw_from_dram2) ? bw_from_dram1 : 
bw_from_dram2;
+
+   if (optimal_fclk)
+   *optimal_fclk = bw_from_dram /
+   (dcn3_0_soc.fabric_datapath_to_dcn_data_return_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100));
+
+   if (optimal_dcfclk)
+   *optimal_dcfclk =  bw_from_dram /
+   (dcn3_0_soc.return_bus_width_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100));
+}
+
 static void dcn30_update_bw_bounding_box(struct dc *dc, struct clk_bw_params 
*bw_params)
 {
-   unsigned int i;
+   unsigned int i, j;
+   unsigned int num_states = 0;
+
+   unsigned int dcfclk_mhz[DC__VOLTAGE_STATES] = {0};
+   unsigned int dram_speed_mts[DC__VOLTAGE_STATES] = {0};
+   unsigned int optimal_uclk_for_dcfclk_sta_targets[DC__VOLTAGE_STATES] = 
{0};
+   unsigned int optimal_dcfclk_for_uclk[DC__VOLTAGE_STATES] = {0};
+
+   unsigned int dcfclk_sta_targets[DC__VOLTAGE_STATES] = {694, 875, 1000, 
1200};
+   unsigned int num_dcfclk_sta_targets = 4;
+   unsigned int num_uclk_states;

 if (dc->ctx->dc_bios->vram_info.num_chans)
 dcn3_0_soc.num_chans = dc->ctx->dc_bios->vram_info.num_chans;
@@ -2250,13 +2282,78 @@ static void dcn30_update_bw_bounding_box(struct dc *dc, 
struct clk_bw_params *bw
 dcn3_0_soc.dispclk_dppclk_vco_speed_mhz = 
dc->clk_mgr->dentist_vco_freq_khz / 1000.0;
 dc->dml.soc.dispclk_dppclk_vco_speed_mhz = 
dc->clk_mgr->dentist_vco_freq_khz / 1000.0;

-   /* UCLK first, it determines number of states */
 if (bw_params->clk_table.entries[0].memclk_mhz) {
-   dcn3_0_soc.num_states = bw_params->clk_table.num_entries;
+
+   if (bw_params->clk_table.entries[1].dcfclk_mhz > 
dcfclk_sta_targets[num_dcfclk_sta_targets-1]) {
+   // If max DCFCLK is greater than the max DCFCLK STA 
target, insert into the DCFCLK STA target array
+   dcfclk_sta_targets[num_dcfclk_sta_targets] = 
bw_params->clk_table.entries[1].dcfclk_mhz;
+   num_dcfclk_sta_targets++;
+   } else if (bw_params->clk_table.entries[1].dcfclk_mhz < 
dcfclk_sta_targets[num_dcfclk_sta_targets-1]) {
+   // If max DCFCLK is less than the max DCFCLK STA 
target, cap values and remove duplicates
+   for (i = 0; i < num_dcfclk_sta_targets; i++) {
+   if (dcfclk_sta_targets[i] > 
bw_params->clk_table.entries[1].dcfclk_mhz) {
+   dcfclk_sta_targets[i] = 
bw_params->clk_table.entries[1].dcfclk_mhz;
+   break;
+   }
+   }
+   // Update size of array since we "removed" duplicates
+   num_dcfclk_sta_targets = i + 1;
+   }
+
+   num_uclk_states = bw_params->clk_table.num_entries;
+
+   // Calculate optimal dcfclk for each uclk
+   for (i = 0; i < num_uclk_states; i++) {
+ 

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Mon, Jun 15, 2020 at 11:55:29AM -0700, Fenghua Yu wrote:

> Or do you suggest to add a random new flag in struct thread_info instead
> of a TIF flag?

Why thread_info? What's wrong with something simple like the below. It
takes a bit from the 'strictly current' flags word.


diff --git a/include/linux/sched.h b/include/linux/sched.h
index b62e6aaf28f0..fca830b97055 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -801,6 +801,9 @@ struct task_struct {
/* Stalled due to lack of memory */
unsignedin_memstall:1;
 #endif
+#ifdef CONFIG_PCI_PASID
+   unsignedhas_valid_pasid:1;
+#endif
 
unsigned long   atomic_flags; /* Flags requiring atomic 
access. */
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 142b23645d82..10b3891be99e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -955,6 +955,10 @@ static struct task_struct *dup_task_struct(struct 
task_struct *orig, int node)
tsk->use_memdelay = 0;
 #endif
 
+#ifdef CONFIG_PCI_PASID
+   tsk->has_valid_pasid = 0;
+#endif
+
 #ifdef CONFIG_MEMCG
tsk->active_memcg = NULL;
 #endif
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: The inherent defect of the AMDGPU driver about hotplug

[Alex Deucher]

On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou  wrote:
>
> About one month ago, I have asked a question about HDMI hotplug, the link is:
>
> https://gitlab.freedesktop.org/drm/amd/-/issues/1135#note_492460
>
> And I have put one patch to fix this, as follows:
>
>  39 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  40 index f355d9a752d2..ee657db9a228 100644
>  41 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  42 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  43 @@ -973,7 +973,7 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  44 const struct drm_encoder_helper_funcs *encoder_funcs;
>  45 int r;
>  46 enum drm_connector_status ret =
> connector_status_disconnected;
>  47 -   bool dret = false, broken_edid = false;
>  48 +   bool dret = false, broken_edid = false, undefined_flag =
> false;
>  49
>  50 if (!drm_kms_helper_is_poll_worker()) {
>  51 r = pm_runtime_get_sync(connector->dev->dev);
>  52 @@ -988,7 +988,12 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  53
>  54 if (amdgpu_connector->ddc_bus)
>  55 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> false);
>  56 -   if (dret) {
>  57 +
>  58 +   /* Check the HDMI HPD pin status again */
>  59 +   if (!amdgpu_display_hpd_sense(adev,
> amdgpu_connector->hpd.hpd))
>  60 +   undefined_flag = true;
>  61 +
>  62 +   if (dret && !undefined_flag) {
>  63 amdgpu_connector->detected_by_load = false;
>  64 amdgpu_connector_free_edid(connector);
>  65 amdgpu_connector_get_edid(connector);
>
> Maybe the fix is sloppy, so I write the another patch:
>
>  16 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  17 index c770d73352a7..bb59ebc9a6c8 100644
>  18 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  19 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  20 @@ -991,6 +991,7 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  21 if (amdgpu_connector->ddc_bus)
>  22 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> false);
>  23 if (dret) {
>  24 +   schedule_work(&adev->hotplug_work);
>  25 amdgpu_connector->detected_by_load = false;
>  26 amdgpu_connector_free_edid(connector);
>  27 amdgpu_connector_get_edid(connector);
>
> Which is better, or neither？

As per the last discussion:

"This is the part I don't understand.  The logic already checks the HPD
status in amdgpu_connector_check_hpd_status_unchanged().  Does it
still report connected at that point?  After that it tries to read the
EDID in amdgpu_display_ddc_probe().  If the monitor is disconnected,
there should be no EDID so dret should be false.  We should try and
figure out why the first HPD check reports connected and the EDID
probe returns true."

There is already an HPD probe in the current logic.  We should try and
understand why we need a second one rather than just adding one.  Does
a delay at the top of that function help?

Alex

>
> --
> Regards,
> Aaron
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/display: Update bounding box states

[Jerry (Fangzhi) Zuo]

From: Alvin Lee 

[Why]
We need to update each p-state in the bounding box

[How]
Update states when assigning values to clocks

Signed-off-by: Alvin Lee 
Signed-off-by: Jerry (Fangzhi) Zuo 
Reviewed-by: Hersen Wu 
---
 .../drm/amd/display/dc/dcn30/dcn30_resource.c | 65 +++
 1 file changed, 23 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c 
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
index 821bde9a376e..27e84d90306b 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
@@ -168,17 +168,18 @@ struct _vcs_dpi_ip_params_st dcn3_0_ip = {
 
 struct _vcs_dpi_soc_bounding_box_st dcn3_0_soc = {
.clock_limits = {
-   /* State 0 should have clocks set below WM set B 
minimums */
{
.state = 0,
-   },
-   /* State 1 is max */
-   {
-   .state = 1,
+   .dispclk_mhz = 1217.0,
+   .dppclk_mhz = 1217.0,
+   .phyclk_mhz = 810.0,
+   .phyclk_d18_mhz = 667.0,
+   .socclk_mhz = 1200.0,
+   .dscclk_mhz = 405.6,
},
},
.min_dcfclk = 500.0, /* TODO: set this to actual min DCFCLK */
-   .num_states = 2,
+   .num_states = 1,
.sr_exit_time_us = 12,
.sr_enter_plus_exit_time_us = 20,
.urgent_latency_us = 4.0,
@@ -204,6 +205,7 @@ struct _vcs_dpi_soc_bounding_box_st dcn3_0_soc = {
.round_trip_ping_latency_dcfclk_cycles = 191,
.urgent_out_of_order_return_per_channel_bytes = 4096,
.channel_interleave_bytes = 256,
+   .num_banks = 8,
.gpuvm_min_page_size_bytes = 4096,
.hostvm_min_page_size_bytes = 4096,
.dram_clock_change_latency_us = 404,
@@ -2354,43 +2356,22 @@ static void dcn30_update_bw_bounding_box(struct dc *dc, 
struct clk_bw_params *bw
dcn3_0_soc.clock_limits[i].dcfclk_mhz = dcfclk_mhz[i];
dcn3_0_soc.clock_limits[i].fabricclk_mhz = 
dcfclk_mhz[i];
dcn3_0_soc.clock_limits[i].dram_speed_mts = 
dram_speed_mts[i];
-   }
-   }
 
-   /* Fill all states with max values of all other clocks */
-   for (i = 0; i < dcn3_0_soc.num_states; i++) {
-   /* Some clocks can come from bw_params, if so fill from 
bw_params[1], otherwise fill from dcn3_0_soc[1] */
-   /* Temporarily ignore bw_params values */
-
-   /* DTBCLK */
-   /*if (bw_params->clk_table.entries[0].dtbclk_mhz)
-   dcn3_0_soc.clock_limits[i].dtbclk_mhz = 
bw_params->clk_table.entries[1].dtbclk_mhz;
-   else*/
-   dcn3_0_soc.clock_limits[i].dtbclk_mhz = 
dcn3_0_soc.clock_limits[1].dtbclk_mhz;
-
-   /* DISPCLK */
-   /*if (bw_params->clk_table.entries[0].dispclk_mhz)
-   dcn3_0_soc.clock_limits[i].dispclk_mhz = 
bw_params->clk_table.entries[1].dispclk_mhz;
-   else*/
-   dcn3_0_soc.clock_limits[i].dispclk_mhz = 
dcn3_0_soc.clock_limits[1].dispclk_mhz;
-
-   /* DPPCLK */
-   /*if (bw_params->clk_table.entries[0].dppclk_mhz)
-   dcn3_0_soc.clock_limits[i].dppclk_mhz = 
bw_params->clk_table.entries[1].dppclk_mhz;
-   else*/
-   dcn3_0_soc.clock_limits[i].dppclk_mhz = 
dcn3_0_soc.clock_limits[1].dppclk_mhz;
-
-   /* PHYCLK */
-   /*if (bw_params->clk_table.entries[0].phyclk_mhz)
-   dcn3_0_soc.clock_limits[i].phyclk_mhz = 
bw_params->clk_table.entries[1].phyclk_mhz;
-   else*/
-   dcn3_0_soc.clock_limits[i].phyclk_mhz = 
dcn3_0_soc.clock_limits[1].phyclk_mhz;
-
-   /* These clocks cannot come from bw_params, always fill from 
dcn3_0_soc[1] */
-   /* FCLK, PHYCLK_D18, SOCCLK, DSCCLK */
-   dcn3_0_soc.clock_limits[i].phyclk_d18_mhz = 
dcn3_0_soc.clock_limits[1].phyclk_d18_mhz;
-   dcn3_0_soc.clock_limits[i].socclk_mhz = 
dcn3_0_soc.clock_limits[1].socclk_mhz;
-   dcn3_0_soc.clock_limits[i].dscclk_mhz = 
dcn3_0_soc.clock_limits[1].dscclk_mhz;
+   /* Fill all states with max values of all other clocks 
*/
+   dcn3_0_soc.clock_limits[i].dtbclk_mhz = 
dcn3_0_soc.clock_limits[0].dtbclk_mhz;
+   dcn3_0_soc.clock_limits[i].dispclk_mhz = 
dcn3_0_soc.clock_limits[0].dispclk_mhz;
+   dcn3_0_soc.clock_limits[i].dppclk_mhz = 
dcn3_0_soc.clock_limits[0].dppclk_mhz;
+   dcn3_0_soc.clock_limits[i].phyclk_mhz = 
dcn3_0_so

[PATCH 1/2] drm/amd/display: Update DCN3 bounding box

[Jerry (Fangzhi) Zuo]

From: Alvin Lee 

[Why]
We want to update the bounding box to have more granular control of the
DCFCLK.

[How]
Setup DCFCLK to use STA values and also optimal values based on
UCLK.

Signed-off-by: Alvin Lee 
Signed-off-by: Jerry (Fangzhi) Zuo 
Reviewed-by: Hersen Wu 
---
 .../drm/amd/display/dc/dcn30/dcn30_resource.c | 114 --
 1 file changed, 102 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c 
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
index 654fdbbff86b..821bde9a376e 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
@@ -2237,9 +2237,41 @@ bool dcn30_validate_bandwidth(struct dc *dc, struct 
dc_state *context,
return out;
 }
 
+static void get_optimal_dcfclk_fclk_for_uclk(unsigned int uclk_mts,
+   unsigned int 
*optimal_dcfclk,
+   unsigned int 
*optimal_fclk)
+{
+   double bw_from_dram, bw_from_dram1, bw_from_dram2;
+
+   bw_from_dram1 = uclk_mts * dcn3_0_soc.num_chans *
+   dcn3_0_soc.dram_channel_width_bytes * 
(dcn3_0_soc.max_avg_dram_bw_use_normal_percent / 100);
+   bw_from_dram2 = uclk_mts * dcn3_0_soc.num_chans *
+   dcn3_0_soc.dram_channel_width_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100);
+
+   bw_from_dram = (bw_from_dram1 < bw_from_dram2) ? bw_from_dram1 : 
bw_from_dram2;
+
+   if (optimal_fclk)
+   *optimal_fclk = bw_from_dram /
+   (dcn3_0_soc.fabric_datapath_to_dcn_data_return_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100));
+
+   if (optimal_dcfclk)
+   *optimal_dcfclk =  bw_from_dram /
+   (dcn3_0_soc.return_bus_width_bytes * 
(dcn3_0_soc.max_avg_sdp_bw_use_normal_percent / 100));
+}
+
 static void dcn30_update_bw_bounding_box(struct dc *dc, struct clk_bw_params 
*bw_params)
 {
-   unsigned int i;
+   unsigned int i, j;
+   unsigned int num_states = 0;
+
+   unsigned int dcfclk_mhz[DC__VOLTAGE_STATES] = {0};
+   unsigned int dram_speed_mts[DC__VOLTAGE_STATES] = {0};
+   unsigned int optimal_uclk_for_dcfclk_sta_targets[DC__VOLTAGE_STATES] = 
{0};
+   unsigned int optimal_dcfclk_for_uclk[DC__VOLTAGE_STATES] = {0};
+
+   unsigned int dcfclk_sta_targets[DC__VOLTAGE_STATES] = {694, 875, 1000, 
1200};
+   unsigned int num_dcfclk_sta_targets = 4;
+   unsigned int num_uclk_states;
 
if (dc->ctx->dc_bios->vram_info.num_chans)
dcn3_0_soc.num_chans = dc->ctx->dc_bios->vram_info.num_chans;
@@ -2250,13 +2282,78 @@ static void dcn30_update_bw_bounding_box(struct dc *dc, 
struct clk_bw_params *bw
dcn3_0_soc.dispclk_dppclk_vco_speed_mhz = 
dc->clk_mgr->dentist_vco_freq_khz / 1000.0;
dc->dml.soc.dispclk_dppclk_vco_speed_mhz = 
dc->clk_mgr->dentist_vco_freq_khz / 1000.0;
 
-   /* UCLK first, it determines number of states */
if (bw_params->clk_table.entries[0].memclk_mhz) {
-   dcn3_0_soc.num_states = bw_params->clk_table.num_entries;
+
+   if (bw_params->clk_table.entries[1].dcfclk_mhz > 
dcfclk_sta_targets[num_dcfclk_sta_targets-1]) {
+   // If max DCFCLK is greater than the max DCFCLK STA 
target, insert into the DCFCLK STA target array
+   dcfclk_sta_targets[num_dcfclk_sta_targets] = 
bw_params->clk_table.entries[1].dcfclk_mhz;
+   num_dcfclk_sta_targets++;
+   } else if (bw_params->clk_table.entries[1].dcfclk_mhz < 
dcfclk_sta_targets[num_dcfclk_sta_targets-1]) {
+   // If max DCFCLK is less than the max DCFCLK STA 
target, cap values and remove duplicates
+   for (i = 0; i < num_dcfclk_sta_targets; i++) {
+   if (dcfclk_sta_targets[i] > 
bw_params->clk_table.entries[1].dcfclk_mhz) {
+   dcfclk_sta_targets[i] = 
bw_params->clk_table.entries[1].dcfclk_mhz;
+   break;
+   }
+   }
+   // Update size of array since we "removed" duplicates
+   num_dcfclk_sta_targets = i + 1;
+   }
+
+   num_uclk_states = bw_params->clk_table.num_entries;
+
+   // Calculate optimal dcfclk for each uclk
+   for (i = 0; i < num_uclk_states; i++) {
+   
get_optimal_dcfclk_fclk_for_uclk(bw_params->clk_table.entries[i].memclk_mhz * 
16,
+   &optimal_dcfclk_for_uclk[i], NULL);
+   if (optimal_dcfclk_for_uclk[i] < 
bw_params->clk_table.entries[0].dcfclk_mhz) {
+   optimal_dcfclk_for_uclk[i] = 
bw_params->clk_table.entries[0].dcfclk_mhz;
+   }
+   

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Fenghua Yu]

Hi, Peter,

On Mon, Jun 15, 2020 at 08:31:16PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 11:12:59AM -0700, Fenghua Yu wrote:
> > > I don't get why you need a rdmsr here, or why not having one would
> > > require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?
> > 
> > My concern is TIF flags are precious (only 3 slots available). Defining
> > a new TIF flag may be not worth it while rdmsr() can check if PASID
> > is valid in the MSR. And performance here might not be a big issue
> > in #GP.
> > 
> > But if you think using TIF flag is better, I can define a new TIF flag
> > and maintain it per thread (init 0 when clone()/fork(), set 1 in fixup()).
> > Then we can avoid using rdmsr() to check valid PASID in the MSR.
> 
> WHY ?!?! What do you need a TIF flag for?

We need "a way" to check if the per thread MSR has a valid PASID. If yes,
no need to fix up the MSR (wrmsr()), and let other handler to handle the #GP.
Otherwise, apply the heuristics and fix up the MSR and exit the #GP.

The way to check the valid PASID in the MSR is rdmsr() in this series.
A TIF flag will be much faster than rdmsr() and seems a sutiable way
to check valid PASID status per thread. That's why it could replace
rdmsr() to check PASID in the MSR.

Or do you suggest to add a random new flag in struct thread_info instead
of a TIF flag?

Thanks.

-Fenghua

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Mon, Jun 15, 2020 at 11:19:21AM -0700, Raj, Ashok wrote:
> On Mon, Jun 15, 2020 at 06:03:57PM +0200, Peter Zijlstra wrote:
> > 
> > I don't get why you need a rdmsr here, or why not having one would
> > require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?
> > 
> > > > > +  */
> > > > > + rdmsrl(MSR_IA32_PASID, pasid_msr);
> > > > > + if (pasid_msr & MSR_IA32_PASID_VALID)
> > > > > + return false;
> > > > > +
> > > > > + /* Fix up the MSR if the MSR doesn't have a valid PASID. */
> > > > > + wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);
> > 
> > How much more expensive is the wrmsr over the rdmsr? Can't we just
> > unconditionally write the current PASID and call it a day?
> 
> The reason to check the rdmsr() is because we are using a hueristic taking
> GP faults. If we already setup the MSR, but we get it a second time it
> means the reason is something other than PASID_MSR not being set.
> 
> Ideally we should use the TIF_ to track this would be cheaper, but we were
> told those bits aren't easy to give out. 

Why do you need a TIF flag? Why not any other random flag in current?
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Mon, Jun 15, 2020 at 11:12:59AM -0700, Fenghua Yu wrote:
> > I don't get why you need a rdmsr here, or why not having one would
> > require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?
> 
> My concern is TIF flags are precious (only 3 slots available). Defining
> a new TIF flag may be not worth it while rdmsr() can check if PASID
> is valid in the MSR. And performance here might not be a big issue
> in #GP.
> 
> But if you think using TIF flag is better, I can define a new TIF flag
> and maintain it per thread (init 0 when clone()/fork(), set 1 in fixup()).
> Then we can avoid using rdmsr() to check valid PASID in the MSR.

WHY ?!?! What do you need a TIF flag for?
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Fenghua Yu]

On Mon, Jun 15, 2020 at 06:03:57PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 08:48:54AM -0700, Fenghua Yu wrote:
> > Hi, Peter,
> > On Mon, Jun 15, 2020 at 09:56:49AM +0200, Peter Zijlstra wrote:
> > > On Fri, Jun 12, 2020 at 05:41:33PM -0700, Fenghua Yu wrote:
> > > > +/*
> > > > + * Apply some heuristics to see if the #GP fault was caused by a thread
> > > > + * that hasn't had the IA32_PASID MSR initialized.  If it looks like 
> > > > that
> > > > + * is the problem, try initializing the IA32_PASID MSR. If the 
> > > > heuristic
> > > > + * guesses incorrectly, take one more #GP fault.
> > > 
> > > How is that going to help? Aren't we then going to run this same
> > > heuristic again and again and again?
> > 
> > The heuristic always initializes the MSR with the per mm PASID IIF the
> > mm has a valid PASID but the MSR doesn't have one. This heuristic usually
> > happens only once on the first #GP in a thread.
> 
> But it doesn't guarantee the PASID is the right one. Suppose both the mm
> has a PASID and the MSR has a VALID one, but the MSR isn't the mm one.
> Then we NO-OP. So if the exception was due to us having the wrong PASID,
> we stuck.

The MSR for each thread was cleared during fork() and clone(). The PASID
was cleared during mm_init(). The per-mm PASID was assigned when fist
bind_mm() is called and remains the same value until process exit().

The MSR is only fixed up when the first ENQCMD is executed in a thread:
bit 31 in the MSR is 0 and the PASID in the mm is non-zero.

The MSR remains the same PASID value once it's fixed up until the thread
exits.

So the work flow ensures the PASID goes from mm's PASID to the MSR.

The PASID could be unbund from the mm. In this case, iommu will generate
#PF and kernel oops instead of #GP.

> 
> > If the next #GP still comes in, the heuristic finds out the MSR already
> > has a valid PASID and thus will not fixup the MSR any more. The fixup()
> > returns "false" and lets others to handle the #GP.
> > 
> > So the heuristic will be executed once (at most) and won't be executed
> > again and again.
> 
> So I get that you want to set the PASID on-demand, but I find this all
> really weird code to make that happen.

We could keep PASID same in all threads sychronously by propogating the MSRs
when the PASID is bound to the mm via IPIs or taskworks to all
threads in the process. But the code is complex and error-prone and
overhead could be high:
1. The user can call driver to do binding and unbinding multiple times.
   The IPIs or taskworks will be sent multiple times to make sure only
   the last IPIs or taskworks take action.
2. Even if a thread never executes ENQCMD and thus never uses the MSR,
   the MSR still needs to be updated whenever bind_mm() and needs to be
   context switched each time. This could cause high overhead.

Setting the PASID on-demand is simpler and cleaner and was recommended
by Thomas.

> 
> > > > +bool __fixup_pasid_exception(void)
> > > > +{
> > > > +   u64 pasid_msr;
> > > > +   unsigned int pasid;
> > > > +
> > > > +   /*
> > > > +* This function is called only when this #GP was triggered 
> > > > from user
> > > > +* space. So the mm cannot be NULL.
> > > > +*/
> > > > +   pasid = current->mm->pasid;
> > > > +   /* If the mm doesn't have a valid PASID, then can't help. */
> > > > +   if (invalid_pasid(pasid))
> > > > +   return false;
> > > > +
> > > > +   /*
> > > > +* Since IRQ is disabled now, the current task still owns the 
> > > > FPU on
> > > 
> > > That's just weird and confusing. What you want to say is that you rely
> > > on the exception disabling the interrupt.
> > 
> > I checked SDM again. You are right. #GP can be interrupted by machine check
> > or other interrupts. So I cannot assume the current task still owns the FPU.
> > Instead of directly rdmsr() and wrmsr(), I will add helpers that can access
> > either the MSR on the processor or the PASID state in the memory.
> 
> That's not in fact what I meant, but yes, you can take exceptions while
> !IF just fine.
> 
> > > > +* this CPU and the PASID MSR can be directly accessed.
> > > > +*
> > > > +* If the MSR has a valid PASID, the #GP must be for some other 
> > > > reason.
> > > > +*
> > > > +* If rdmsr() is really a performance issue, a TIF_ flag may be
> > > > +* added to check if the thread has a valid PASID instead of 
> > > > rdmsr().
> > > 
> > > I don't understand any of this. Nobody except us writes to this MSR, we
> > > should bloody well know what's in it. What gives?
> > 
> > Patch 4 describes how to manage the MSR and patch 7 describes the format
> > of the MSR (20-bit PASID value and bit 31 is valid bit).
> > 
> > Are they sufficient to help? Or do you mean something else?
> 
> I don't get why you need a rdmsr here, or why not having one would
> require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?

My conc

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Raj, Ashok]

On Mon, Jun 15, 2020 at 06:03:57PM +0200, Peter Zijlstra wrote:
> 
> I don't get why you need a rdmsr here, or why not having one would
> require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?
> 
> > > > +*/
> > > > +   rdmsrl(MSR_IA32_PASID, pasid_msr);
> > > > +   if (pasid_msr & MSR_IA32_PASID_VALID)
> > > > +   return false;
> > > > +
> > > > +   /* Fix up the MSR if the MSR doesn't have a valid PASID. */
> > > > +   wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);
> 
> How much more expensive is the wrmsr over the rdmsr? Can't we just
> unconditionally write the current PASID and call it a day?

The reason to check the rdmsr() is because we are using a hueristic taking
GP faults. If we already setup the MSR, but we get it a second time it
means the reason is something other than PASID_MSR not being set.

Ideally we should use the TIF_ to track this would be cheaper, but we were
told those bits aren't easy to give out. 

Cheers,
Ashok
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Luck, Tony]

>> The heuristic always initializes the MSR with the per mm PASID IIF the
>> mm has a valid PASID but the MSR doesn't have one. This heuristic usually
>> happens only once on the first #GP in a thread.
>
> But it doesn't guarantee the PASID is the right one. Suppose both the mm
> has a PASID and the MSR has a VALID one, but the MSR isn't the mm one.
> Then we NO-OP. So if the exception was due to us having the wrong PASID,
> we stuck.

ENQCMD only checks the 'valid' bit of the IA32_PASID MSR to decide whether
to #GP or not.  H/W has no concept of the "right" pasid value.

If IA32_PASID is valid with the wrong value ... then the system is about to
see some major corruption because the operations in the accelerator are
not going to translate to the physical addresses for pages owned by the process
that issued the ENQCMD.

-Tony
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/amdgpu: Fix SQ_DEBUG_STS_GLOBAL* registers

[Alex Deucher]

On Mon, Jun 15, 2020 at 12:18 PM Tom St Denis  wrote:
>
> Forgot to subtract the SOC15 IP offsetand add the BASE_IDX values.
>
> Signed-off-by: Tom St Denis 

Reviewed-by: Alex Deucher 

> ---
>  .../gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h   | 6 --
>  .../gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h   | 6 --
>  drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h  | 9 ++---
>  drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h  | 9 ++---
>  .../gpu/drm/amd/include/asic_reg/gc/gc_9_2_1_offset.h| 9 ++---
>  5 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h 
> b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
> index aab3d22c3b0f..baac40fa70e7 100644
> --- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
> +++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
> @@ -21,8 +21,10 @@
>  #ifndef _gc_10_1_0_OFFSET_HEADER
>  #define _gc_10_1_0_OFFSET_HEADER
>
> -#define mmSQ_DEBUG_STS_GLOBAL
>   0x2309
> -#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x2310
> +#define mmSQ_DEBUG_STS_GLOBAL
>   0x0309
> +#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX   
>   0
> +#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x0310
> +#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX  
>   0
>
>  // addressBlock: gc_sdma0_sdma0dec
>  // base address: 0x4980
> diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h 
> b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
> index 16c7f6f2467e..0bde3b4e9567 100644
> --- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
> +++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
> @@ -22,8 +22,10 @@
>  #ifndef _gc_10_3_0_OFFSET_HEADER
>  #define _gc_10_3_0_OFFSET_HEADER
>
> -#define mmSQ_DEBUG_STS_GLOBAL
>   0x2309
> -#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x2310
> +#define mmSQ_DEBUG_STS_GLOBAL
>   0x0309
> +#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX   
>   0
> +#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x0310
> +#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX  
>   0
>
>  // addressBlock: gc_sdma0_sdma0dec
>  // base address: 0x4980
> diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h 
> b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
> index e3e1a9c1153b..12d451e5475b 100644
> --- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
> +++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
> @@ -21,9 +21,12 @@
>  #ifndef _gc_9_0_OFFSET_HEADER
>  #define _gc_9_0_OFFSET_HEADER
>
> -#define mmSQ_DEBUG_STS_GLOBAL
>   0x2309
> -#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x2310
> -#define mmSQ_DEBUG_STS_GLOBAL3   
>   0x2311
> +#define mmSQ_DEBUG_STS_GLOBAL
>   0x0309
> +#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX   
>   0
> +#define mmSQ_DEBUG_STS_GLOBAL2   
>   0x0310
> +#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX  
>   0
> +#define mmSQ_DEBUG_STS_GLOBAL3   
>   0x0311
> +#define mmSQ_DEBUG_STS_GLOBAL3_BASE_IDX  
>   0
>
>  // addressBlock: gc_grbmdec
>  // base address: 0x8000
> diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h 
> b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
> index 6b1ad9082a2c..d17d1e622e4f 100644
> --- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
> +++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
> @@ -21,9 +21,12 @@
>  #ifndef _gc_9_1_OFFSET_HEADER
>  #define _gc_9_1_OFFSET_HEADER
>
> -#define mmSQ_DEBUG_STS_GLOBAL
>   0x2309
> -#define mmSQ_DEBUG_STS_GLOBAL2  

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Mon, Jun 15, 2020 at 08:48:54AM -0700, Fenghua Yu wrote:
> Hi, Peter,
> On Mon, Jun 15, 2020 at 09:56:49AM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 12, 2020 at 05:41:33PM -0700, Fenghua Yu wrote:
> > > +/*
> > > + * Apply some heuristics to see if the #GP fault was caused by a thread
> > > + * that hasn't had the IA32_PASID MSR initialized.  If it looks like that
> > > + * is the problem, try initializing the IA32_PASID MSR. If the heuristic
> > > + * guesses incorrectly, take one more #GP fault.
> > 
> > How is that going to help? Aren't we then going to run this same
> > heuristic again and again and again?
> 
> The heuristic always initializes the MSR with the per mm PASID IIF the
> mm has a valid PASID but the MSR doesn't have one. This heuristic usually
> happens only once on the first #GP in a thread.

But it doesn't guarantee the PASID is the right one. Suppose both the mm
has a PASID and the MSR has a VALID one, but the MSR isn't the mm one.
Then we NO-OP. So if the exception was due to us having the wrong PASID,
we stuck.

> If the next #GP still comes in, the heuristic finds out the MSR already
> has a valid PASID and thus will not fixup the MSR any more. The fixup()
> returns "false" and lets others to handle the #GP.
> 
> So the heuristic will be executed once (at most) and won't be executed
> again and again.

So I get that you want to set the PASID on-demand, but I find this all
really weird code to make that happen.

> > > +bool __fixup_pasid_exception(void)
> > > +{
> > > + u64 pasid_msr;
> > > + unsigned int pasid;
> > > +
> > > + /*
> > > +  * This function is called only when this #GP was triggered from user
> > > +  * space. So the mm cannot be NULL.
> > > +  */
> > > + pasid = current->mm->pasid;
> > > + /* If the mm doesn't have a valid PASID, then can't help. */
> > > + if (invalid_pasid(pasid))
> > > + return false;
> > > +
> > > + /*
> > > +  * Since IRQ is disabled now, the current task still owns the FPU on
> > 
> > That's just weird and confusing. What you want to say is that you rely
> > on the exception disabling the interrupt.
> 
> I checked SDM again. You are right. #GP can be interrupted by machine check
> or other interrupts. So I cannot assume the current task still owns the FPU.
> Instead of directly rdmsr() and wrmsr(), I will add helpers that can access
> either the MSR on the processor or the PASID state in the memory.

That's not in fact what I meant, but yes, you can take exceptions while
!IF just fine.

> > > +  * this CPU and the PASID MSR can be directly accessed.
> > > +  *
> > > +  * If the MSR has a valid PASID, the #GP must be for some other reason.
> > > +  *
> > > +  * If rdmsr() is really a performance issue, a TIF_ flag may be
> > > +  * added to check if the thread has a valid PASID instead of rdmsr().
> > 
> > I don't understand any of this. Nobody except us writes to this MSR, we
> > should bloody well know what's in it. What gives?
> 
> Patch 4 describes how to manage the MSR and patch 7 describes the format
> of the MSR (20-bit PASID value and bit 31 is valid bit).
> 
> Are they sufficient to help? Or do you mean something else?

I don't get why you need a rdmsr here, or why not having one would
require a TIF flag. Is that because this MSR is XSAVE/XRSTOR managed?

> > > +  */
> > > + rdmsrl(MSR_IA32_PASID, pasid_msr);
> > > + if (pasid_msr & MSR_IA32_PASID_VALID)
> > > + return false;
> > > +
> > > + /* Fix up the MSR if the MSR doesn't have a valid PASID. */
> > > + wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);

How much more expensive is the wrmsr over the rdmsr? Can't we just
unconditionally write the current PASID and call it a day?
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/amdgpu: Fix SQ_DEBUG_STS_GLOBAL* registers

[Tom St Denis]

Forgot to subtract the SOC15 IP offsetand add the BASE_IDX values.

Signed-off-by: Tom St Denis 
---
 .../gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h   | 6 --
 .../gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h   | 6 --
 drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h  | 9 ++---
 drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h  | 9 ++---
 .../gpu/drm/amd/include/asic_reg/gc/gc_9_2_1_offset.h| 9 ++---
 5 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h 
b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
index aab3d22c3b0f..baac40fa70e7 100644
--- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
+++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_1_0_offset.h
@@ -21,8 +21,10 @@
 #ifndef _gc_10_1_0_OFFSET_HEADER
 #define _gc_10_1_0_OFFSET_HEADER
 
-#define mmSQ_DEBUG_STS_GLOBAL  
0x2309
-#define mmSQ_DEBUG_STS_GLOBAL2 
0x2310
+#define mmSQ_DEBUG_STS_GLOBAL  
0x0309
+#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX 
0
+#define mmSQ_DEBUG_STS_GLOBAL2 
0x0310
+#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX
0
 
 // addressBlock: gc_sdma0_sdma0dec
 // base address: 0x4980
diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h 
b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
index 16c7f6f2467e..0bde3b4e9567 100644
--- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
+++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_10_3_0_offset.h
@@ -22,8 +22,10 @@
 #ifndef _gc_10_3_0_OFFSET_HEADER
 #define _gc_10_3_0_OFFSET_HEADER
 
-#define mmSQ_DEBUG_STS_GLOBAL  
0x2309
-#define mmSQ_DEBUG_STS_GLOBAL2 
0x2310
+#define mmSQ_DEBUG_STS_GLOBAL  
0x0309
+#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX 
0
+#define mmSQ_DEBUG_STS_GLOBAL2 
0x0310
+#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX
0
 
 // addressBlock: gc_sdma0_sdma0dec
 // base address: 0x4980
diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h 
b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
index e3e1a9c1153b..12d451e5475b 100644
--- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
+++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_0_offset.h
@@ -21,9 +21,12 @@
 #ifndef _gc_9_0_OFFSET_HEADER
 #define _gc_9_0_OFFSET_HEADER
 
-#define mmSQ_DEBUG_STS_GLOBAL  
0x2309
-#define mmSQ_DEBUG_STS_GLOBAL2 
0x2310
-#define mmSQ_DEBUG_STS_GLOBAL3 
0x2311
+#define mmSQ_DEBUG_STS_GLOBAL  
0x0309
+#define mmSQ_DEBUG_STS_GLOBAL_BASE_IDX 
0
+#define mmSQ_DEBUG_STS_GLOBAL2 
0x0310
+#define mmSQ_DEBUG_STS_GLOBAL2_BASE_IDX
0
+#define mmSQ_DEBUG_STS_GLOBAL3 
0x0311
+#define mmSQ_DEBUG_STS_GLOBAL3_BASE_IDX
0
 
 // addressBlock: gc_grbmdec
 // base address: 0x8000
diff --git a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h 
b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
index 6b1ad9082a2c..d17d1e622e4f 100644
--- a/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
+++ b/drivers/gpu/drm/amd/include/asic_reg/gc/gc_9_1_offset.h
@@ -21,9 +21,12 @@
 #ifndef _gc_9_1_OFFSET_HEADER
 #define _gc_9_1_OFFSET_HEADER
 
-#define mmSQ_DEBUG_STS_GLOBAL  
0x2309
-#define mmSQ_DEBUG_STS_GLOBAL2 
0x2310
-#define mmSQ_DEBUG_STS_GLOBAL3 
0x2311
+#define mmSQ_DEBUG_STS_GLOBAL  
0x0309
+#define

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Fenghua Yu]

Hi, Peter,
On Mon, Jun 15, 2020 at 09:56:49AM +0200, Peter Zijlstra wrote:
> On Fri, Jun 12, 2020 at 05:41:33PM -0700, Fenghua Yu wrote:
> > +/*
> > + * Apply some heuristics to see if the #GP fault was caused by a thread
> > + * that hasn't had the IA32_PASID MSR initialized.  If it looks like that
> > + * is the problem, try initializing the IA32_PASID MSR. If the heuristic
> > + * guesses incorrectly, take one more #GP fault.
> 
> How is that going to help? Aren't we then going to run this same
> heuristic again and again and again?

The heuristic always initializes the MSR with the per mm PASID IIF the
mm has a valid PASID but the MSR doesn't have one. This heuristic usually
happens only once on the first #GP in a thread.

If the next #GP still comes in, the heuristic finds out the MSR already
has a valid PASID and thus will not fixup the MSR any more. The fixup()
returns "false" and lets others to handle the #GP.

So the heuristic will be executed once (at most) and won't be executed
again and again.

> 
> > + */
> > +bool __fixup_pasid_exception(void)
> > +{
> > +   u64 pasid_msr;
> > +   unsigned int pasid;
> > +
> > +   /*
> > +* This function is called only when this #GP was triggered from user
> > +* space. So the mm cannot be NULL.
> > +*/
> > +   pasid = current->mm->pasid;
> > +   /* If the mm doesn't have a valid PASID, then can't help. */
> > +   if (invalid_pasid(pasid))
> > +   return false;
> > +
> > +   /*
> > +* Since IRQ is disabled now, the current task still owns the FPU on
> 
> That's just weird and confusing. What you want to say is that you rely
> on the exception disabling the interrupt.

I checked SDM again. You are right. #GP can be interrupted by machine check
or other interrupts. So I cannot assume the current task still owns the FPU.
Instead of directly rdmsr() and wrmsr(), I will add helpers that can access
either the MSR on the processor or the PASID state in the memory.

> 
> > +* this CPU and the PASID MSR can be directly accessed.
> > +*
> > +* If the MSR has a valid PASID, the #GP must be for some other reason.
> > +*
> > +* If rdmsr() is really a performance issue, a TIF_ flag may be
> > +* added to check if the thread has a valid PASID instead of rdmsr().
> 
> I don't understand any of this. Nobody except us writes to this MSR, we
> should bloody well know what's in it. What gives?

Patch 4 describes how to manage the MSR and patch 7 describes the format
of the MSR (20-bit PASID value and bit 31 is valid bit).

Are they sufficient to help? Or do you mean something else?

> > +*/
> > +   rdmsrl(MSR_IA32_PASID, pasid_msr);
> > +   if (pasid_msr & MSR_IA32_PASID_VALID)
> > +   return false;
> > +
> > +   /* Fix up the MSR if the MSR doesn't have a valid PASID. */
> > +   wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);
> > +
> > +   return true;
> > +}
> > -- 
> > 2.19.1
> > 

Thanks.

-Fenghua
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 00/12] x86: tag application address space for devices

[Fenghua Yu]

Hi, Peter,
On Mon, Jun 15, 2020 at 09:52:02AM +0200, Peter Zijlstra wrote:
> On Fri, Jun 12, 2020 at 05:41:21PM -0700, Fenghua Yu wrote:
> 
> > This series only provides simple and basic support for ENQCMD and the MSR:
> > 1. Clean up type definitions (patch 1-3). These patches can be in a
> >separate series.
> >- Define "pasid" as "unsigned int" consistently (patch 1 and 2).
> >- Define "flags" as "unsigned int"
> > 2. Explain different various technical terms used in the series (patch 4).
> > 3. Enumerate support for ENQCMD in the processor (patch 5).
> > 4. Handle FPU PASID state and the MSR during context switch (patches 6-7).
> > 5. Define "pasid" in mm_struct (patch 8).
> > 5. Clear PASID state for new mm and forked and cloned thread (patch 9-10).
> > 6. Allocate and free PASID for a process (patch 11).
> > 7. Fix up the PASID MSR in #GP handler when one thread in a process
> >executes ENQCMD for the first time (patches 12).
> 
> If this is per mm, should not switch_mm() update the MSR ? I'm not
> seeing that, nor do I see it explained why not.

PASID value is per mm and all threads in a process have the same PASID
value in the MSR. However, the MSR is per thread and is context switched
by XSAVES/XRSTROS in patches 6-7.

Thanks.

-Fenghua
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 00/12] x86: tag application address space for devices

[Peter Zijlstra]

On Fri, Jun 12, 2020 at 05:41:21PM -0700, Fenghua Yu wrote:

> This series only provides simple and basic support for ENQCMD and the MSR:
> 1. Clean up type definitions (patch 1-3). These patches can be in a
>separate series.
>- Define "pasid" as "unsigned int" consistently (patch 1 and 2).
>- Define "flags" as "unsigned int"
> 2. Explain different various technical terms used in the series (patch 4).
> 3. Enumerate support for ENQCMD in the processor (patch 5).
> 4. Handle FPU PASID state and the MSR during context switch (patches 6-7).
> 5. Define "pasid" in mm_struct (patch 8).
> 5. Clear PASID state for new mm and forked and cloned thread (patch 9-10).
> 6. Allocate and free PASID for a process (patch 11).
> 7. Fix up the PASID MSR in #GP handler when one thread in a process
>executes ENQCMD for the first time (patches 12).

If this is per mm, should not switch_mm() update the MSR ? I'm not
seeing that, nor do I see it explained why not.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Fri, Jun 12, 2020 at 05:41:33PM -0700, Fenghua Yu wrote:
> +/*
> + * Apply some heuristics to see if the #GP fault was caused by a thread
> + * that hasn't had the IA32_PASID MSR initialized.  If it looks like that
> + * is the problem, try initializing the IA32_PASID MSR. If the heuristic
> + * guesses incorrectly, take one more #GP fault.

How is that going to help? Aren't we then going to run this same
heuristic again and again and again?

> + */
> +bool __fixup_pasid_exception(void)
> +{
> + u64 pasid_msr;
> + unsigned int pasid;
> +
> + /*
> +  * This function is called only when this #GP was triggered from user
> +  * space. So the mm cannot be NULL.
> +  */
> + pasid = current->mm->pasid;
> + /* If the mm doesn't have a valid PASID, then can't help. */
> + if (invalid_pasid(pasid))
> + return false;
> +
> + /*
> +  * Since IRQ is disabled now, the current task still owns the FPU on

That's just weird and confusing. What you want to say is that you rely
on the exception disabling the interrupt.

> +  * this CPU and the PASID MSR can be directly accessed.
> +  *
> +  * If the MSR has a valid PASID, the #GP must be for some other reason.
> +  *
> +  * If rdmsr() is really a performance issue, a TIF_ flag may be
> +  * added to check if the thread has a valid PASID instead of rdmsr().

I don't understand any of this. Nobody except us writes to this MSR, we
should bloody well know what's in it. What gives?

> +  */
> + rdmsrl(MSR_IA32_PASID, pasid_msr);
> + if (pasid_msr & MSR_IA32_PASID_VALID)
> + return false;
> +
> + /* Fix up the MSR if the MSR doesn't have a valid PASID. */
> + wrmsrl(MSR_IA32_PASID, pasid | MSR_IA32_PASID_VALID);
> +
> + return true;
> +}
> -- 
> 2.19.1
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 12/12] x86/traps: Fix up invalid PASID

[Peter Zijlstra]

On Fri, Jun 12, 2020 at 05:41:33PM -0700, Fenghua Yu wrote:
> @@ -447,6 +458,18 @@ dotraplinkage void do_general_protection(struct pt_regs 
> *regs, long error_code)
>   int ret;
>  
>   RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> + /*
> +  * Perform the check for a user mode PASID exception before enable
> +  * interrupts. Doing this here ensures that the PASID MSR can be simply
> +  * accessed because the contents are known to be still associated
> +  * with the current process.
> +  */
> + if (user_mode(regs) && fixup_pasid_exception()) {
> + cond_local_irq_enable(regs);
> + return;

OK, so we're done with the exception, lets enable interrupts?

> + }
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: remove perf level dpm in one-VF

[Wang, Kevin(Yang)]

[AMD Official Use Only - Internal Distribution Only]

please add these smu messages into patch commit,
after fixed,
Reviewed-by: Kevin Wang 

Best Regards,
Kevin

From: Sheng, Wenhui 
Sent: Sunday, June 14, 2020 3:38 PM
To: Wang, Kevin(Yang) ; amd-gfx@lists.freedesktop.org 

Subject: RE: [PATCH] drm/amdgpu: remove perf level dpm in one-VF


[AMD Official Use Only - Internal Distribution Only]


[AMD Official Use Only - Internal Distribution Only]



Hi Kevin,



At least 3  SMU messages not supported in one-VF mode , which could break the 
node power_dpm_force_performance_level functionality



SMU_MSG_SetSoftMaxByFreq
SMU_MSG_SetSoftMinByFreq

SMU_MSG_TransferTableDram2Smu



Brs

Wenhui

From: Wang, Kevin(Yang) 
Sent: Friday, June 12, 2020 7:04 PM
To: Sheng, Wenhui ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: remove perf level dpm in one-VF



[AMD Official Use Only - Internal Distribution Only]



which smu message will break this node work on navi12 asic?



Best Regards,

Kevin



From: amd-gfx 
mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Wenhui Sheng mailto:wenhui.sh...@amd.com>>
Sent: Friday, June 12, 2020 5:30 PM
To: amd-gfx@lists.freedesktop.org 
mailto:amd-gfx@lists.freedesktop.org>>
Cc: Sheng, Wenhui mailto:wenhui.sh...@amd.com>>
Subject: [PATCH] drm/amdgpu: remove perf level dpm in one-VF



On Navi12 platform, node power_dpm_force_performance_level
doesn't work correctly in one-VF mode.

Signed-off-by: Wenhui Sheng mailto:wenhui.sh...@amd.com>>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 395ddbe2461c..4e7b579afe0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1877,7 +1877,7 @@ static ssize_t 
amdgpu_set_thermal_throttling_logging(struct device *dev,

 static struct amdgpu_device_attr amdgpu_device_attrs[] = {
 AMDGPU_DEVICE_ATTR_RW(power_dpm_state,  
ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
-   AMDGPU_DEVICE_ATTR_RW(power_dpm_force_performance_level,
ATTR_FLAG_BASIC|ATTR_FLAG_ONEVF),
+   AMDGPU_DEVICE_ATTR_RW(power_dpm_force_performance_level,
ATTR_FLAG_BASIC),
 AMDGPU_DEVICE_ATTR_RO(pp_num_states,
ATTR_FLAG_BASIC),
 AMDGPU_DEVICE_ATTR_RO(pp_cur_state, 
ATTR_FLAG_BASIC),
 AMDGPU_DEVICE_ATTR_RW(pp_force_state,   
ATTR_FLAG_BASIC),
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CKevin1.Wang%40amd.com%7C76d3cddef9744f5c7ee908d80eb3502e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637275511045991505&sdata=1uvfK9R%2FMAFkZrgPk5aEnVMQs9zYOJQUFdLWwLzMbs4%3D&reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

The inherent defect of the AMDGPU driver about hotplug

[Aaron Chou]

About one month ago, I have asked a question about HDMI hotplug, the link is:

https://gitlab.freedesktop.org/drm/amd/-/issues/1135#note_492460

And I have put one patch to fix this, as follows:

 39 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 40 index f355d9a752d2..ee657db9a228 100644
 41 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 42 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 43 @@ -973,7 +973,7 @@ amdgpu_connector_dvi_detect(struct
drm_connector *connector, bool force)
 44 const struct drm_encoder_helper_funcs *encoder_funcs;
 45 int r;
 46 enum drm_connector_status ret =
connector_status_disconnected;
 47 -   bool dret = false, broken_edid = false;
 48 +   bool dret = false, broken_edid = false, undefined_flag =
false;
 49
 50 if (!drm_kms_helper_is_poll_worker()) {
 51 r = pm_runtime_get_sync(connector->dev->dev);
 52 @@ -988,7 +988,12 @@ amdgpu_connector_dvi_detect(struct
drm_connector *connector, bool force)
 53
 54 if (amdgpu_connector->ddc_bus)
 55 dret = amdgpu_display_ddc_probe(amdgpu_connector,
false);
 56 -   if (dret) {
 57 +
 58 +   /* Check the HDMI HPD pin status again */
 59 +   if (!amdgpu_display_hpd_sense(adev,
amdgpu_connector->hpd.hpd))
 60 +   undefined_flag = true;
 61 +
 62 +   if (dret && !undefined_flag) {
 63 amdgpu_connector->detected_by_load = false;
 64 amdgpu_connector_free_edid(connector);
 65 amdgpu_connector_get_edid(connector);

Maybe the fix is sloppy, so I write the another patch:

 16 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 17 index c770d73352a7..bb59ebc9a6c8 100644
 18 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 19 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
 20 @@ -991,6 +991,7 @@ amdgpu_connector_dvi_detect(struct
drm_connector *connector, bool force)
 21 if (amdgpu_connector->ddc_bus)
 22 dret = amdgpu_display_ddc_probe(amdgpu_connector,
false);
 23 if (dret) {
 24 +   schedule_work(&adev->hotplug_work);
 25 amdgpu_connector->detected_by_load = false;
 26 amdgpu_connector_free_edid(connector);
 27 amdgpu_connector_get_edid(connector);

Which is better, or neither？

--
Regards,
Aaron
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdkfd: Fix reference count leaks.

[wu000273]

From: Qiushi Wu 

kobject_init_and_add() takes reference even when it fails.
If this function returns an error, kobject_put() must be called to
properly clean up the memory associated with the object.

Signed-off-by: Qiushi Wu 
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index bb77f7af2b6d..dc3c4149f860 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -632,8 +632,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
 
ret = kobject_init_and_add(dev->kobj_node, &node_type,
sys_props.kobj_nodes, "%d", id);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(dev->kobj_node);
return ret;
+   }
 
dev->kobj_mem = kobject_create_and_add("mem_banks", dev->kobj_node);
if (!dev->kobj_mem)
@@ -680,8 +682,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(mem->kobj, &mem_type,
dev->kobj_mem, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(mem->kobj);
return ret;
+   }
 
mem->attr.name = "properties";
mem->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -699,8 +703,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(cache->kobj, &cache_type,
dev->kobj_cache, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(cache->kobj);
return ret;
+   }
 
cache->attr.name = "properties";
cache->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -718,8 +724,10 @@ static int kfd_build_sysfs_node_entry(struct 
kfd_topology_device *dev,
return -ENOMEM;
ret = kobject_init_and_add(iolink->kobj, &iolink_type,
dev->kobj_iolink, "%d", i);
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(iolink->kobj);
return ret;
+   }
 
iolink->attr.name = "properties";
iolink->attr.mode = KFD_SYSFS_FILE_MODE;
@@ -798,8 +806,10 @@ static int kfd_topology_update_sysfs(void)
ret = kobject_init_and_add(sys_props.kobj_topology,
&sysprops_type,  &kfd_device->kobj,
"topology");
-   if (ret < 0)
+   if (ret < 0) {
+   kobject_put(sys_props.kobj_topology);
return ret;
+   }
 
sys_props.kobj_nodes = kobject_create_and_add("nodes",
sys_props.kobj_topology);
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/radeon: Fix reference count leaks caused by pm_runtime_get_sync

[Aditya Pakki]

On calling pm_runtime_get_sync() the reference count of the device
is incremented. In case of failure, decrement the
reference count before returning the error.

Signed-off-by: Aditya Pakki 
---
 drivers/gpu/drm/radeon/radeon_display.c | 4 +++-
 drivers/gpu/drm/radeon/radeon_drv.c | 4 +++-
 drivers/gpu/drm/radeon/radeon_kms.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index 35db79a168bf..df1a7eb73651 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -635,8 +635,10 @@ radeon_crtc_set_config(struct drm_mode_set *set,
dev = set->crtc->dev;
 
ret = pm_runtime_get_sync(dev->dev);
-   if (ret < 0)
+   if (ret < 0) {
+   pm_runtime_put_autosuspend(dev->dev);
return ret;
+   }
 
ret = drm_crtc_helper_set_config(set, ctx);
 
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index bbb0883e8ce6..62b5069122cc 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -549,8 +549,10 @@ long radeon_drm_ioctl(struct file *filp,
long ret;
dev = file_priv->minor->dev;
ret = pm_runtime_get_sync(dev->dev);
-   if (ret < 0)
+   if (ret < 0) {
+   pm_runtime_put_autosuspend(dev->dev);
return ret;
+   }
 
ret = drm_ioctl(filp, cmd, arg);

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index c5d1dc9618a4..99ee60f8b604 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -638,8 +638,10 @@ int radeon_driver_open_kms(struct drm_device *dev, struct 
drm_file *file_priv)
file_priv->driver_priv = NULL;
 
r = pm_runtime_get_sync(dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(dev->dev);
return r;
+   }
 
/* new gpu have virtual address space support */
if (rdev->family >= CHIP_CAYMAN) {
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu/display: fix ref count leak when pm_runtime_get_sync fails

[Navid Emamdoost]

The call to pm_runtime_get_sync increments the counter even in case of
failure, leading to incorrect ref count.
In case of failure, decrement the ref count before returning.

Signed-off-by: Navid Emamdoost 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index f355d9a752d2..a1aec205435d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -716,8 +716,10 @@ amdgpu_connector_lvds_detect(struct drm_connector 
*connector, bool force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (encoder) {
@@ -854,8 +856,10 @@ amdgpu_connector_vga_detect(struct drm_connector 
*connector, bool force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
encoder = amdgpu_connector_best_single_encoder(connector);
@@ -977,8 +981,10 @@ amdgpu_connector_dvi_detect(struct drm_connector 
*connector, bool force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) {
@@ -1328,8 +1334,10 @@ amdgpu_connector_dp_detect(struct drm_connector 
*connector, bool force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) {
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: Fix incorrect firmware size calculation

[Guo Lei]

From 014162f69b909a59c241e7f73c3630d1da34696c Mon Sep 17 00:00:00 2001

From: Lei Guo 

Date: Mon, 15 Jun 2020 13:54:26 +0800

Subject: [PATCH] drm/amdgpu: Fix incorrect firmware size calculation




[WHY]

The memcpy() function copies n bytes from memory area src to memory area

dest. So specify the firmware size in bytes.




[How]

Correct the calculation.




Signed-off-by: Lei Guo 

Reviewed-by: Junwei Zhang 

---

 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)




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

index 6b94587df407..c3e59b765268 100644

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

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

@@ -1960,7 +1960,7 @@ static int gfx_v9_0_mec_init(struct amdgpu_device *adev)

 fw_data = (const __le32 *)

 (adev->gfx.mec_fw->data +

  le32_to_cpu(mec_hdr->header.ucode_array_offset_bytes));

-fw_size = le32_to_cpu(mec_hdr->header.ucode_size_bytes) / 4;

+fw_size = le32_to_cpu(mec_hdr->header.ucode_size_bytes);

 

 r = amdgpu_bo_create_reserved(adev, mec_hdr->header.ucode_size_bytes,

   PAGE_SIZE, AMDGPU_GEM_DOMAIN_GTT,

-- 

2.17.1


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/radeon: fix multiple reference count leak

[Aditya Pakki]

On calling pm_runtime_get_sync() the reference count of the device
is incremented. In case of failure, decrement the
reference count before returning the error.

Signed-off-by: Aditya Pakki 
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c 
b/drivers/gpu/drm/radeon/radeon_connectors.c
index fe12d9d91d7a..e30834434442 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -879,8 +879,10 @@ radeon_lvds_detect(struct drm_connector *connector, bool 
force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (encoder) {
@@ -1025,8 +1027,10 @@ radeon_vga_detect(struct drm_connector *connector, bool 
force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
encoder = radeon_best_single_encoder(connector);
@@ -1163,8 +1167,10 @@ radeon_tv_detect(struct drm_connector *connector, bool 
force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
encoder = radeon_best_single_encoder(connector);
@@ -1247,8 +1253,10 @@ radeon_dvi_detect(struct drm_connector *connector, bool 
force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (radeon_connector->detected_hpd_without_ddc) {
@@ -1657,8 +1665,10 @@ radeon_dp_detect(struct drm_connector *connector, bool 
force)
 
if (!drm_kms_helper_is_poll_worker()) {
r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
+   if (r < 0) {
+   pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+   }
}
 
if (!force && radeon_check_hpd_status_unchanged(connector)) {
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/display: fix ref count leak in amdgpu_drm_ioctl

[Navid Emamdoost]

in amdgpu_drm_ioctl the call to pm_runtime_get_sync increments the
counter even in case of failure, leading to incorrect
ref count. In case of failure, decrement the ref count before returning.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 126e74758a34..d73924e35a57 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1373,11 +1373,12 @@ long amdgpu_drm_ioctl(struct file *filp,
dev = file_priv->minor->dev;
ret = pm_runtime_get_sync(dev->dev);
if (ret < 0)
-   return ret;
+   goto out;
 
ret = drm_ioctl(filp, cmd, arg);
 
pm_runtime_mark_last_busy(dev->dev);
+out:
pm_runtime_put_autosuspend(dev->dev);
return ret;
 }
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix ref count leak in amdgpu_display_crtc_set_config

[Markus Elfring]

> in amdgpu_display_crtc_set_config, …

* Can the term “reference count” become relevant also for this commit message
  besides other possible adjustments?

* Can it be nicer to combine proposed updates for this software module
  as a patch series (with a cover letter)?

* Would you like to add the tag “Fixes”?


…
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
…
> @@ -306,6 +306,7 @@  int amdgpu_display_crtc_set_config(struct drm_mode_set 
> *set,
>   adev->have_disp_power_ref = false;
>   }
>
> +out:
>   /* drop the power reference we got coming in here */
>   pm_runtime_put_autosuspend(dev->dev);
>   return ret;

Perhaps use the label “put_runtime” instead?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix ref count leak in amdgpu_display_crtc_set_config

[Navid Emamdoost]

in amdgpu_display_crtc_set_config, the call to pm_runtime_get_sync
increments the counter even in case of failure, leading to incorrect
ref count. In case of failure, decrement the ref count before returning.

Signed-off-by: Navid Emamdoost 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index f7143d927b6d..5e51f0acf744 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -282,7 +282,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set,
 
ret = pm_runtime_get_sync(dev->dev);
if (ret < 0)
-   return ret;
+   goto out;
 
ret = drm_crtc_helper_set_config(set, ctx);
 
@@ -297,7 +297,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set,
   take the current one */
if (active && !adev->have_disp_power_ref) {
adev->have_disp_power_ref = true;
-   return ret;
+   goto out;
}
/* if we have no active crtcs, then drop the power ref
   we got before */
@@ -306,6 +306,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set,
adev->have_disp_power_ref = false;
}
 
+out:
/* drop the power reference we got coming in here */
pm_runtime_put_autosuspend(dev->dev);
return ret;
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix ref count leak in amdgpu_driver_open_kms

[Navid Emamdoost]

in amdgpu_driver_open_kms the call to pm_runtime_get_sync increments the
counter even in case of failure, leading to incorrect
ref count. In case of failure, decrement the ref count before returning.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index d7e17e34fee1..bd40aa307462 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -991,7 +991,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct 
drm_file *file_priv)
 
r = pm_runtime_get_sync(dev->dev);
if (r < 0)
-   return r;
+   goto pm_put;
 
fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
if (unlikely(!fpriv)) {
@@ -1042,6 +1042,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct 
drm_file *file_priv)
 
 out_suspend:
pm_runtime_mark_last_busy(dev->dev);
+pm_put:
pm_runtime_put_autosuspend(dev->dev);
 
return r;
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 11/12] x86/mmu: Allocate/free PASID

[Lu Baolu]

Hi Fenghua,

On 6/13/20 8:41 AM, Fenghua Yu wrote:

A PASID is allocated for an "mm" the first time any thread attaches
to an SVM capable device. Later device attachments (whether to the same
device or another SVM device) will re-use the same PASID.

The PASID is freed when the process exits (so no need to keep
reference counts on how many SVM devices are sharing the PASID).

Signed-off-by: Fenghua Yu 
Reviewed-by: Tony Luck 
---
v2:
- Define a helper free_bind() to simplify error exit code in bind_mm()
   (Thomas)
- Fix a ret error code in bind_mm() (Thomas)
- Change pasid's type from "int" to "unsigned int" to have consistent
   pasid type in iommu (Thomas)
- Simplify alloc_pasid() a bit.

  arch/x86/include/asm/iommu.h   |   2 +
  arch/x86/include/asm/mmu_context.h |  14 
  drivers/iommu/intel/svm.c  | 101 +
  3 files changed, 105 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index bf1ed2ddc74b..ed41259fe7ac 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -26,4 +26,6 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
return -EINVAL;
  }
  
+void __free_pasid(struct mm_struct *mm);

+
  #endif /* _ASM_X86_IOMMU_H */
diff --git a/arch/x86/include/asm/mmu_context.h 
b/arch/x86/include/asm/mmu_context.h
index 47562147e70b..f8c91ce8c451 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  
  extern atomic64_t last_mm_ctx_id;
  
@@ -117,9 +118,22 @@ static inline int init_new_context(struct task_struct *tsk,

init_new_context_ldt(mm);
return 0;
  }
+
+static inline void free_pasid(struct mm_struct *mm)
+{
+   if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
+   return;
+
+   if (!cpu_feature_enabled(X86_FEATURE_ENQCMD))
+   return;
+
+   __free_pasid(mm);
+}
+
  static inline void destroy_context(struct mm_struct *mm)
  {
destroy_context_ldt(mm);
+   free_pasid(mm);
  }
  
  extern void switch_mm(struct mm_struct *prev, struct mm_struct *next,

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 4e775e12ae52..27dc866b8461 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -425,6 +425,53 @@ int intel_svm_unbind_gpasid(struct device *dev, unsigned 
int pasid)
return ret;
  }
  
+static void free_bind(struct intel_svm *svm, struct intel_svm_dev *sdev,

+ bool new_pasid)
+{
+   if (new_pasid)
+   ioasid_free(svm->pasid);
+   kfree(svm);
+   kfree(sdev);
+}
+
+/*
+ * If this mm already has a PASID, use it. Otherwise allocate a new one.
+ * Let the caller know if a new PASID is allocated via 'new_pasid'.
+ */
+static int alloc_pasid(struct intel_svm *svm, struct mm_struct *mm,
+  unsigned int pasid_max, bool *new_pasid,
+  unsigned int flags)
+{
+   unsigned int pasid;
+
+   *new_pasid = false;
+
+   /*
+* Reuse the PASID if the mm already has a PASID and not a private
+* PASID is requested.
+*/
+   if (mm && mm->pasid && !(flags & SVM_FLAG_PRIVATE_PASID)) {
+   /*
+* Once a PASID is allocated for this mm, the PASID
+* stays with the mm until the mm is dropped. Reuse
+* the PASID which has been already allocated for the
+* mm instead of allocating a new one.
+*/
+   ioasid_set_data(mm->pasid, svm);


How about adding some sanity checks here? For example,

void *p = ioasid_find(NULL, mm->pasid, NULL);

if (!p)
ioasid_set_data(mm->pasid, svm);
else if (IS_ERR(p) || p != svm)
return INVALID_IOSASID;

Best regards,
baolu
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx