[PATCH 1/3] drm/amd/pm: Update SMUv13.0.6 PMFW headers

[Asad Kamal]

From: Lijo Lazar 

Update PMFW interface headers to for new metrics table format and
throttling information.

v2: Added dummy definition for compilation error

Signed-off-by: Lijo Lazar 
Signed-off-by: Asad Kamal 
Reviewed-by: Hawking Zhang 
---
 .../inc/pmfw_if/smu13_driver_if_v13_0_6.h | 31 ++-
 .../pm/swsmu/inc/pmfw_if/smu_v13_0_6_pmfw.h   | 13 +---
 .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  |  2 ++
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_6.h 
b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_6.h
index de84fff39799..ca4a5e99ccd1 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_6.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_6.h
@@ -26,7 +26,7 @@
 // *** IMPORTANT ***
 // PMFW TEAM: Always increment the interface version if
 // anything is changed in this file
-#define SMU13_0_6_DRIVER_IF_VERSION 0x08042023
+#define SMU13_0_6_DRIVER_IF_VERSION 0x08042024
 
 //I2C Interface
 #define NUM_I2C_CONTROLLERS8
@@ -125,11 +125,28 @@ typedef struct {
 #define IH_INTERRUPT_ID_TO_DRIVER   0xFE
 #define IH_INTERRUPT_CONTEXT_ID_THERMAL_THROTTLING  0x7
 
-//thermal over-temp mask defines
-#define THROTTLER_TEMP_CCD_BIT 5
-#define THROTTLER_TEMP_XCD_BIT 6
-#define THROTTLER_TEMP_HBM_BIT 7
-#define THROTTLER_TEMP_AID_BIT 8
-#define THROTTLER_VRHOT_BIT9
+//thermal over-temp mask defines for IH interrupt to host
+#define THROTTLER_PROCHOT_BIT   0
+#define THROTTLER_PPT_BIT   1
+#define THROTTLER_THERMAL_SOCKET_BIT2//AID, XCD, CCD throttling
+#define THROTTLER_THERMAL_VR_BIT3//VRHOT
+#define THROTTLER_THERMAL_HBM_BIT   4
+
+// These defines are used with the following messages:
+// SMC_MSG_TransferTableDram2Smu
+// SMC_MSG_TransferTableSmu2Dram
+// #define TABLE_PPTABLE 0
+// #define TABLE_AVFS_PSM_DEBUG  1
+// #define TABLE_AVFS_FUSE_OVERRIDE  2
+// #define TABLE_PMSTATUSLOG 3
+// #define TABLE_SMU_METRICS 4
+// #define TABLE_DRIVER_SMU_CONFIG   5
+// #define TABLE_I2C_COMMANDS6
+// #define TABLE_COUNT   7
+
+// // Table transfer status
+// #define TABLE_TRANSFER_OK 0x0
+// #define TABLE_TRANSFER_FAILED 0xFF
+// #define TABLE_TRANSFER_PENDING0xAB
 
 #endif
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_6_pmfw.h 
b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_6_pmfw.h
index 3fe403615d86..252aef190c5c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_6_pmfw.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_6_pmfw.h
@@ -123,9 +123,9 @@ typedef enum {
   VOLTAGE_GUARDBAND_COUNT
 } GFX_GUARDBAND_e;
 
-#define SMU_METRICS_TABLE_VERSION 0x3
+#define SMU_METRICS_TABLE_VERSION 0x5
 
-typedef struct {
+typedef struct __attribute__((packed, aligned(4))) {
   uint32_t AccumulationCounter;
 
   //TEMPERATURE
@@ -202,11 +202,16 @@ typedef struct {
 
   // New Items at end to maintain driver compatibility
   uint32_t GfxclkFrequency[8];
+
+  //PSNs
+  uint64_t PublicSerialNumber_AID[4];
+  uint64_t PublicSerialNumber_XCD[8];
+  uint64_t PublicSerialNumber_CCD[12];
 } MetricsTable_t;
 
-#define SMU_VF_METRICS_TABLE_VERSION 0x1
+#define SMU_VF_METRICS_TABLE_VERSION 0x3
 
-typedef struct {
+typedef struct __attribute__((packed, aligned(4))) {
   uint32_t AccumulationCounter;
   uint32_t InstGfxclk_TargFreq;
   uint64_t AccGfxclk_TargFreq;
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 41b49cc827cd..27fd71afc73f 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -82,6 +82,8 @@
 
 #define smnPCIE_ESM_CTRL 0x111003D0
 
+#define THROTTLER_TEMP_HBM_BIT 2
+
 static const struct cmn2asic_msg_mapping 
smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
MSG_MAP(TestMessage, PPSMC_MSG_TestMessage, 
0),
MSG_MAP(GetSmuVersion,   PPSMC_MSG_GetSmuVersion,   
1),
-- 
2.34.1

[PATCH 2/3] drm/amd/pm: Add throttle status in power context

[Asad Kamal]

From: Lijo Lazar 

Keep throttle status indicator in SMUv13 power context

Signed-off-by: Lijo Lazar 
Reviewed-by: Hawking Zhang 
---
 drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h 
b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
index 3ae8d5d252a3..5a99a091965e 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
@@ -119,6 +119,7 @@ struct smu_13_0_power_context {
uint32_tpower_source;
uint8_t in_power_limit_boost_mode;
enum smu_13_0_power_state power_state;
+   atomic_tthrottle_status;
 };
 
 #if defined(SWSMU_CODE_LAYER_L2) || defined(SWSMU_CODE_LAYER_L3)
-- 
2.34.1

[PATCH 3/3] drm/amd/pm: Fix SMUv13.0.6 throttle status report

[Asad Kamal]

From: Lijo Lazar 

Instead of accumulated counters, PMFW will pass the throttle reason
along with throttle interrupt. Use that context information to report the
exact reason for throttling.

v2: Removed Dummy definition

Signed-off-by: Asad Kamal 
Signed-off-by: Lijo Lazar 
Reviewed-by: Hawking Zhang 
---
 .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  | 95 +--
 1 file changed, 45 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 27fd71afc73f..b9f32e0364db 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -82,8 +82,6 @@
 
 #define smnPCIE_ESM_CTRL 0x111003D0
 
-#define THROTTLER_TEMP_HBM_BIT 2
-
 static const struct cmn2asic_msg_mapping 
smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
MSG_MAP(TestMessage, PPSMC_MSG_TestMessage, 
0),
MSG_MAP(GetSmuVersion,   PPSMC_MSG_GetSmuVersion,   
1),
@@ -174,17 +172,12 @@ static const struct cmn2asic_mapping 
smu_v13_0_6_table_map[SMU_TABLE_COUNT] = {
TAB_MAP(I2C_COMMANDS),
 };
 
-#define THROTTLER_PROCHOT_GFX_BIT  0
-#define THROTTLER_PPT_BIT 1
-#define THROTTLER_TEMP_SOC_BIT 2
-#define THROTTLER_TEMP_VR_GFX_BIT 3
-
 static const uint8_t smu_v13_0_6_throttler_map[] = {
[THROTTLER_PPT_BIT] = (SMU_THROTTLER_PPT0_BIT),
-   [THROTTLER_TEMP_SOC_BIT]= (SMU_THROTTLER_TEMP_GPU_BIT),
-   [THROTTLER_TEMP_HBM_BIT]= (SMU_THROTTLER_TEMP_MEM_BIT),
-   [THROTTLER_TEMP_VR_GFX_BIT] = (SMU_THROTTLER_TEMP_VR_GFX_BIT),
-   [THROTTLER_PROCHOT_GFX_BIT] = (SMU_THROTTLER_PROCHOT_GFX_BIT),
+   [THROTTLER_THERMAL_SOCKET_BIT]  = (SMU_THROTTLER_TEMP_GPU_BIT),
+   [THROTTLER_THERMAL_HBM_BIT] = (SMU_THROTTLER_TEMP_MEM_BIT),
+   [THROTTLER_THERMAL_VR_BIT]  = (SMU_THROTTLER_TEMP_VR_GFX_BIT),
+   [THROTTLER_PROCHOT_BIT] = (SMU_THROTTLER_PROCHOT_GFX_BIT),
 };
 
 struct PPTable_t {
@@ -642,16 +635,14 @@ static int smu_v13_0_6_freqs_in_same_level(int32_t 
frequency1,
return (abs(frequency1 - frequency2) <= EPSILON);
 }
 
-static uint32_t smu_v13_0_6_get_throttler_status(struct smu_context *smu,
-MetricsTable_t *metrics)
+static uint32_t smu_v13_0_6_get_throttler_status(struct smu_context *smu)
 {
+   struct smu_power_context *smu_power = &smu->smu_power;
+   struct smu_13_0_power_context *power_context = smu_power->power_context;
uint32_t  throttler_status = 0;
 
-   throttler_status |= metrics->ProchotResidencyAcc > 0 ? 1U << 
THROTTLER_PROCHOT_GFX_BIT : 0;
-   throttler_status |= metrics->PptResidencyAcc > 0 ? 1U << 
THROTTLER_PPT_BIT : 0;
-   throttler_status |= metrics->SocketThmResidencyAcc > 0 ?  1U << 
THROTTLER_TEMP_SOC_BIT : 0;
-   throttler_status |= metrics->VrThmResidencyAcc > 0 ? 1U << 
THROTTLER_TEMP_VR_GFX_BIT : 0;
-   throttler_status |= metrics->HbmThmResidencyAcc > 0 ? 1U << 
THROTTLER_TEMP_HBM_BIT : 0;
+   throttler_status = atomic_read(&power_context->throttle_status);
+   dev_dbg(smu->adev->dev, "SMU Throttler status: %u", throttler_status);
 
return throttler_status;
 }
@@ -721,9 +712,6 @@ static int smu_v13_0_6_get_smu_metrics_data(struct 
smu_context *smu,
case METRICS_TEMPERATURE_VRSOC:
*value = SMUQ10_TO_UINT(metrics->MaxVrTemperature);
break;
-   case METRICS_THROTTLER_STATUS:
-   *value = smu_v13_0_6_get_throttler_status(smu, metrics);
-   break;
default:
*value = UINT_MAX;
break;
@@ -1290,13 +1278,11 @@ static int smu_v13_0_6_irq_process(struct amdgpu_device 
*adev,
   struct amdgpu_iv_entry *entry)
 {
struct smu_context *smu = adev->powerplay.pp_handle;
+   struct smu_power_context *smu_power = &smu->smu_power;
+   struct smu_13_0_power_context *power_context = smu_power->power_context;
uint32_t client_id = entry->client_id;
-   uint32_t src_id = entry->src_id;
-   /*
-* ctxid is used to distinguish different
-* events for SMCToHost interrupt
-*/
uint32_t ctxid = entry->src_data[0];
+   uint32_t src_id = entry->src_id;
uint32_t data;
 
if (client_id == SOC15_IH_CLIENTID_MP1) {
@@ -1305,7 +1291,10 @@ static int smu_v13_0_6_irq_process(struct amdgpu_device 
*adev,
data = RREG32_SOC15(MP1, 0, regMP1_SMN_IH_SW_INT_CTRL);
data = REG_SET_FIELD(data, MP1_SMN_IH_SW_INT_CTRL, 
INT_ACK, 1);
WREG32_SOC15(MP1, 0, regMP1_SMN_IH_SW_INT_CTRL, data);
-
+   /*
+* ctxid is used to distinguish different events for 
SMCToHost
+* interrupt.

RE: [PATCH 3/3] drm/amd/pm: Fix SMUv13.0.6 throttle status report

[Ma, Le]

[AMD Official Use Only - General]

Series is Reviewed-by: Le Ma 

> -Original Message-
> From: Kamal, Asad 
> Sent: Thursday, June 1, 2023 3:28 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking ; Lazar, Lijo
> ; Ma, Le ; Zhang, Morris
> 
> Subject: [PATCH 3/3] drm/amd/pm: Fix SMUv13.0.6 throttle status report
>
> From: Lijo Lazar 
>
> Instead of accumulated counters, PMFW will pass the throttle reason along
> with throttle interrupt. Use that context information to report the exact
> reason for throttling.
>
> v2: Removed Dummy definition
>
> Signed-off-by: Asad Kamal 
> Signed-off-by: Lijo Lazar 
> Reviewed-by: Hawking Zhang 
> ---
>  .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  | 95 +
> --
>  1 file changed, 45 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> index 27fd71afc73f..b9f32e0364db 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> @@ -82,8 +82,6 @@
>
>  #define smnPCIE_ESM_CTRL 0x111003D0
>
> -#define THROTTLER_TEMP_HBM_BIT 2
> -
>  static const struct cmn2asic_msg_mapping
> smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
>   MSG_MAP(TestMessage,
> PPSMC_MSG_TestMessage,0),
>   MSG_MAP(GetSmuVersion,
> PPSMC_MSG_GetSmuVersion,  1),
> @@ -174,17 +172,12 @@ static const struct cmn2asic_mapping
> smu_v13_0_6_table_map[SMU_TABLE_COUNT] = {
>   TAB_MAP(I2C_COMMANDS),
>  };
>
> -#define THROTTLER_PROCHOT_GFX_BIT  0
> -#define THROTTLER_PPT_BIT 1
> -#define THROTTLER_TEMP_SOC_BIT 2
> -#define THROTTLER_TEMP_VR_GFX_BIT 3
> -
>  static const uint8_t smu_v13_0_6_throttler_map[] = {
>   [THROTTLER_PPT_BIT] = (SMU_THROTTLER_PPT0_BIT),
> - [THROTTLER_TEMP_SOC_BIT]= (SMU_THROTTLER_TEMP_GPU_BIT),
> - [THROTTLER_TEMP_HBM_BIT]=
> (SMU_THROTTLER_TEMP_MEM_BIT),
> - [THROTTLER_TEMP_VR_GFX_BIT] =
> (SMU_THROTTLER_TEMP_VR_GFX_BIT),
> - [THROTTLER_PROCHOT_GFX_BIT] =
> (SMU_THROTTLER_PROCHOT_GFX_BIT),
> + [THROTTLER_THERMAL_SOCKET_BIT]  =
> (SMU_THROTTLER_TEMP_GPU_BIT),
> + [THROTTLER_THERMAL_HBM_BIT] =
> (SMU_THROTTLER_TEMP_MEM_BIT),
> + [THROTTLER_THERMAL_VR_BIT]  =
> (SMU_THROTTLER_TEMP_VR_GFX_BIT),
> + [THROTTLER_PROCHOT_BIT] =
> (SMU_THROTTLER_PROCHOT_GFX_BIT),
>  };
>
>  struct PPTable_t {
> @@ -642,16 +635,14 @@ static int
> smu_v13_0_6_freqs_in_same_level(int32_t frequency1,
>   return (abs(frequency1 - frequency2) <= EPSILON);  }
>
> -static uint32_t smu_v13_0_6_get_throttler_status(struct smu_context *smu,
> -  MetricsTable_t *metrics)
> +static uint32_t smu_v13_0_6_get_throttler_status(struct smu_context
> +*smu)
>  {
> + struct smu_power_context *smu_power = &smu->smu_power;
> + struct smu_13_0_power_context *power_context =
> +smu_power->power_context;
>   uint32_t  throttler_status = 0;
>
> - throttler_status |= metrics->ProchotResidencyAcc > 0 ? 1U <<
> THROTTLER_PROCHOT_GFX_BIT : 0;
> - throttler_status |= metrics->PptResidencyAcc > 0 ? 1U <<
> THROTTLER_PPT_BIT : 0;
> - throttler_status |= metrics->SocketThmResidencyAcc > 0 ?  1U <<
> THROTTLER_TEMP_SOC_BIT : 0;
> - throttler_status |= metrics->VrThmResidencyAcc > 0 ? 1U <<
> THROTTLER_TEMP_VR_GFX_BIT : 0;
> - throttler_status |= metrics->HbmThmResidencyAcc > 0 ? 1U <<
> THROTTLER_TEMP_HBM_BIT : 0;
> + throttler_status = atomic_read(&power_context->throttle_status);
> + dev_dbg(smu->adev->dev, "SMU Throttler status: %u",
> throttler_status);
>
>   return throttler_status;
>  }
> @@ -721,9 +712,6 @@ static int smu_v13_0_6_get_smu_metrics_data(struct
> smu_context *smu,
>   case METRICS_TEMPERATURE_VRSOC:
>   *value = SMUQ10_TO_UINT(metrics->MaxVrTemperature);
>   break;
> - case METRICS_THROTTLER_STATUS:
> - *value = smu_v13_0_6_get_throttler_status(smu, metrics);
> - break;
>   default:
>   *value = UINT_MAX;
>   break;
> @@ -1290,13 +1278,11 @@ static int smu_v13_0_6_irq_process(struct
> amdgpu_device *adev,
>  struct amdgpu_iv_entry *entry)
>  {
>   struct smu_context *smu = adev->powerplay.pp_handle;
> + struct smu_power_context *smu_power = &smu->smu_power;
> + struct smu_13_0_power_context *power_context =
> +smu_power->power_context;
>   uint32_t client_id = entry->client_id;
> - uint32_t src_id = entry->src_id;
> - /*
> -  * ctxid is used to distinguish different
> -  * events for SMCToHost interrupt
> -  */
>   uint32_t ctxid = entry->src_data[0];
> + uint32_t src_id = entry->src_id;
>   uint32_t data;
>
>   if (client_id == SOC15_IH_CLIENTID_MP1) { @@ -1305,7 +1291,10
> @@ static int smu_v13_0_6_irq_process(struct amdgpu_dev

Re: [PATCH v5 09/13] drm/tegra: Use regular fbdev I/O helpers

[Thierry Reding]

On Tue, May 30, 2023 at 05:12:24PM +0200, Thomas Zimmermann wrote:
> Use the regular fbdev helpers for framebuffer I/O instead of DRM's
> helpers. Tegra does not use damage handling, so DRM's fbdev helpers
> are mere wrappers around the fbdev code.
> 
> By using fbdev helpers directly within each DRM fbdev emulation,
> we can eventually remove DRM's wrapper functions entirely.
> 
> v4:
>   * use initializer macros for struct fb_ops
> v2:
>   * use FB_SYS_HELPERS option
> 
> Signed-off-by: Thomas Zimmermann 
> Acked-by: Sam Ravnborg 
> Cc: Thierry Reding 
> Cc: Mikko Perttunen 
> Cc: Jonathan Hunter 
> ---
>  drivers/gpu/drm/tegra/Kconfig | 1 +
>  drivers/gpu/drm/tegra/fbdev.c | 8 +++-
>  2 files changed, 4 insertions(+), 5 deletions(-)

Acked-by: Thierry Reding 


signature.asc
Description: PGP signature

Re: [PATCH AUTOSEL 6.1 4/9] drm/amd/display: Do not set drr on pipe commit

[Sasha Levin]

On Mon, May 15, 2023 at 03:04:43PM +0200, Michel Dänzer wrote:

On 5/11/23 21:39, Sasha Levin wrote:

From: Wesley Chalmers 

[ Upstream commit 474f01015ffdb74e01c2eb3584a2822c64e7b2be ]

[WHY]
Writing to DRR registers such as OTG_V_TOTAL_MIN on the same frame as a
pipe commit can cause underflow.

[HOW]
Move DMUB p-state delegate into optimze_bandwidth; enabling FAMS sets
optimized_required.

This change expects that Freesync requests are blocked when
optimized_required is true.


This change caused a regression, see 
https://patchwork.freedesktop.org/patch/532240/?series=116487&rev=1#comment_972234
 / 9deeb132-a317-7419-e9da-cbc0a379c...@daenzer.net .


Dropped, thanks!

--
Thanks,
Sasha

[PATCH] [PATCH] drm/amdgpu: add option params to enforce process isolation between graphics and compute

[Chong Li]

enforce process isolation between graphics and compute via using the same 
reserved vmid.

Signed-off-by: Chong Li 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  9 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 13 -
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ce196badf42d..48c5c547d85a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -215,6 +215,7 @@ extern int amdgpu_force_asic_type;
 extern int amdgpu_smartshift_bias;
 extern int amdgpu_use_xgmi_p2p;
 extern int amdgpu_mtype_local;
+extern int enforce_isolation;
 #ifdef CONFIG_HSA_AMD
 extern int sched_policy;
 extern bool debug_evictions;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 3d91e123f9bd..2e0ebd92b4cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -973,6 +973,15 @@ MODULE_PARM_DESC(
4 = 
AMDGPU_CPX_PARTITION_MODE)");
 module_param_named(user_partt_mode, amdgpu_user_partt_mode, uint, 0444);
 
+
+/**
+ * DOC: enforce_isolation (int)
+ * enforce process isolation between graphics and compute via using the same 
reserved vmid.
+ */
+int enforce_isolation = 0;
+module_param(enforce_isolation, int, 0444);
+MODULE_PARM_DESC(enforce_isolation, "enforce process isolation between 
graphics and compute . 1 = On, 0 = Off");
+
 /* These devices are not supported by amdgpu.
  * They are supported by the mach64, r128, radeon drivers
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index c991ca0b7a1c..33efa17d08ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -409,7 +409,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct 
amdgpu_ring *ring,
if (r || !idle)
goto error;
 
-   if (vm->reserved_vmid[vmhub]) {
+   if (vm->reserved_vmid[vmhub] || (enforce_isolation && (vmhub == 
AMDGPU_GFXHUB(0 {
r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
if (r || !id)
goto error;
@@ -578,6 +578,17 @@ void amdgpu_vmid_mgr_init(struct amdgpu_device *adev)
list_add_tail(&id_mgr->ids[j].list, &id_mgr->ids_lru);
}
}
+
+   if (enforce_isolation) {
+   struct amdgpu_vmid_mgr *id_mgr = 
&adev->vm_manager.id_mgr[AMDGPU_GFXHUB(0)];
+   struct amdgpu_vmid *id = NULL;
+   ++id_mgr->reserved_use_count;
+   id = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vmid,
+   list);
+   /* Remove from normal round robin handling */
+   list_del_init(&id->list);
+   id_mgr->reserved = id;
+   }
 }
 
 /**
-- 
2.34.1

[PATCH] drm/amdgpu: add option params to enforce process isolation between graphics and compute

[Chong Li]

enforce process isolation between graphics and compute via using the same 
reserved vmid.

Signed-off-by: Chong Li 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  9 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 13 -
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ce196badf42d..48c5c547d85a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -215,6 +215,7 @@ extern int amdgpu_force_asic_type;
 extern int amdgpu_smartshift_bias;
 extern int amdgpu_use_xgmi_p2p;
 extern int amdgpu_mtype_local;
+extern int enforce_isolation;
 #ifdef CONFIG_HSA_AMD
 extern int sched_policy;
 extern bool debug_evictions;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 3d91e123f9bd..2e0ebd92b4cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -973,6 +973,15 @@ MODULE_PARM_DESC(
4 = 
AMDGPU_CPX_PARTITION_MODE)");
 module_param_named(user_partt_mode, amdgpu_user_partt_mode, uint, 0444);
 
+
+/**
+ * DOC: enforce_isolation (int)
+ * enforce process isolation between graphics and compute via using the same 
reserved vmid.
+ */
+int enforce_isolation = 0;
+module_param(enforce_isolation, int, 0444);
+MODULE_PARM_DESC(enforce_isolation, "enforce process isolation between 
graphics and compute . 1 = On, 0 = Off");
+
 /* These devices are not supported by amdgpu.
  * They are supported by the mach64, r128, radeon drivers
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index c991ca0b7a1c..33efa17d08ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -409,7 +409,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct 
amdgpu_ring *ring,
if (r || !idle)
goto error;
 
-   if (vm->reserved_vmid[vmhub]) {
+   if (vm->reserved_vmid[vmhub] || (enforce_isolation && (vmhub == 
AMDGPU_GFXHUB(0 {
r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
if (r || !id)
goto error;
@@ -578,6 +578,17 @@ void amdgpu_vmid_mgr_init(struct amdgpu_device *adev)
list_add_tail(&id_mgr->ids[j].list, &id_mgr->ids_lru);
}
}
+
+   if (enforce_isolation) {
+   struct amdgpu_vmid_mgr *id_mgr = 
&adev->vm_manager.id_mgr[AMDGPU_GFXHUB(0)];
+   struct amdgpu_vmid *id = NULL;
+   ++id_mgr->reserved_use_count;
+   id = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vmid,
+   list);
+   /* Remove from normal round robin handling */
+   list_del_init(&id->list);
+   id_mgr->reserved = id;
+   }
 }
 
 /**
-- 
2.34.1

RE: [PATCH v2] drm/amd/display: Drop unused DCN_BASE variable in dcn314_resource.c

[Li, Roman]

[Public]

Reviewed-by: Roman Li 

> -Original Message-
> From: SHANMUGAM, SRINIVASAN 
> Sent: Wednesday, May 31, 2023 1:46 PM
> To: Pillai, Aurabindo ; Mahfooz, Hamza
> ; Siqueira, Rodrigo
> ; Wentland, Harry
> ; Li, Roman 
> Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> 
> Subject: [PATCH v2] drm/amd/display: Drop unused DCN_BASE variable in
> dcn314_resource.c
>
> Fixes the following W=1 kernel build warning:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dcn314/dcn314_resource.c:12
> 8:29: warning: ‘DCN_BASE’ defined but not used [-Wunused-const-variable=]
>   128 | static const struct IP_BASE DCN_BASE = { { { { 0x0012,
> 0x00C0, 0x34C0, 0x9000, 0x02403C00, 0, 0, 0 } },
>   | ^~~~
>
> Suggested-by: Roman Li 
> Cc: Hamza Mahfooz 
> Cc: Rodrigo Siqueira 
> Cc: Harry Wentland 
> Cc: Aurabindo Pillai 
> Signed-off-by: Srinivasan Shanmugam 
> ---
>
> v2:
>  - Remove even unused IP_BASE_INSTANCE and IP_BASE struct definitions
>altogether (Roman)
>
>  .../drm/amd/display/dc/dcn314/dcn314_resource.c | 17 -
>  1 file changed, 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_resource.c
> b/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_resource.c
> index 3592efcc7fae..837884c4f03a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_resource.c
> @@ -117,23 +117,6 @@
>  #define regBIF_BX2_BIOS_SCRATCH_60x003e
>  #define regBIF_BX2_BIOS_SCRATCH_6_BASE_IDX   1
>
> -struct IP_BASE_INSTANCE {
> - unsigned int segment[MAX_SEGMENT];
> -};
> -
> -struct IP_BASE {
> - struct IP_BASE_INSTANCE instance[MAX_INSTANCE];
> -};
> -
> -static const struct IP_BASE DCN_BASE = { { { { 0x0012, 0x00C0,
> 0x34C0, 0x9000, 0x02403C00, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } },
> - { { 0, 0, 0, 0, 0, 0, 0, 0 } } } };
> -
> -
>  #define DC_LOGGER_INIT(logger)
>
>  enum dcn31_clk_src_array_id {
> --
> 2.25.1

Re: [PATCH 0/4] Enable legacy OD support for SMU13

[Alex Deucher]

Series is:
Reviewed-by: Alex Deucher 

On Wed, May 31, 2023 at 9:59 PM Evan Quan  wrote:
>
> Enable the following OD features support for SMU13:
> - Maxinum and minimum gfxclk frequency settings
> - Maxinum and minimum uclk frequency settings
> - Voltage offset settings for gfxclk v/f curve line
>   - This is quite different from previous generations/ASICs. For SMU13,
> there are six anchor points defined on the v/f curve. And what user
> configurable are the voltage offsets for those anchor points.
>
> Evan Quan (4):
>   drm/amd/pm: update SMU13 header files for coming OD support
>   drm/amd/pm: fulfill SMU13 OD settings init and restore
>   drm/amd/pm: fulfill the OD support for SMU13.0.0
>   drm/amd/pm: fulfill the OD support for SMU13.0.7
>
>  drivers/gpu/drm/amd/pm/amdgpu_pm.c|  26 +-
>  .../gpu/drm/amd/pm/inc/smu_v13_0_0_pptable.h  |  16 +-
>  .../inc/pmfw_if/smu13_driver_if_v13_0_0.h |  18 +-
>  .../inc/pmfw_if/smu13_driver_if_v13_0_7.h |  29 +-
>  .../amd/pm/swsmu/inc/smu_v13_0_7_pptable.h|  16 +-
>  .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c|  13 +-
>  .../drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c  | 488 +-
>  .../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c  | 487 -
>  8 files changed, 1038 insertions(+), 55 deletions(-)
>
> --
> 2.34.1
>

[PATCH] drm/amd/pm: Fix power context allocation in SMU13

[Hawking Zhang]

From: Lijo Lazar 

Use the right data structure for allocation.

Signed-off-by: Lijo Lazar 
Reviewed-by: Hawking Zhang 
---
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index da059b02a153..09ac66ab9c34 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -534,11 +534,11 @@ int smu_v13_0_init_power(struct smu_context *smu)
if (smu_power->power_context || smu_power->power_context_size != 0)
return -EINVAL;
 
-   smu_power->power_context = kzalloc(sizeof(struct smu_13_0_dpm_context),
+   smu_power->power_context = kzalloc(sizeof(struct 
smu_13_0_power_context),
   GFP_KERNEL);
if (!smu_power->power_context)
return -ENOMEM;
-   smu_power->power_context_size = sizeof(struct smu_13_0_dpm_context);
+   smu_power->power_context_size = sizeof(struct smu_13_0_power_context);
 
return 0;
 }
-- 
2.17.1

Re: [PATCH] drm/radeon: fix race condition UAF in radeon_gem_set_domain_ioctl

[Christian König]

Am 26.05.23 um 14:37 schrieb Min Li:

Userspace can race to free the gobj(robj converted from), robj should not
be accessed again after drm_gem_object_put, otherwith it will result in
use-after-free.

Signed-off-by: Min Li 
---
  drivers/gpu/drm/radeon/radeon_gem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index bdc5af23f005..450c7cbdd28a 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -478,7 +478,7 @@ int radeon_gem_set_domain_ioctl(struct drm_device *dev, 
void *data,
  
  	drm_gem_object_put(gobj);

up_read(&rdev->exclusive_lock);
-   r = radeon_gem_handle_lockup(robj->rdev, r);
+   r = radeon_gem_handle_lockup(rdev, r);


This also makes the robj unused which the kernel test robot also 
complained about.


Please remove that local variable and re-submit.

Apart from that the patch looks good to me,
Christian.


return r;
  }
  

Re: [PATCH] Revert "drm/amdgpu: remove TOPDOWN flags when allocating VRAM in large bar system"

[Christian König]

Am 20.05.23 um 11:25 schrieb Arunpravin Paneer Selvam:

This reverts commit c105518679b6e87232874ffc989ec403bee59664.

This patch disables the TOPDOWN flag for APU and few dGPU cards
which has the VRAM size equal to the BAR size.

When we enable the TOPDOWN flag, we get the free blocks at
the highest available memory region and we don't split the
lower order blocks. This change is required to keep off
the fragmentation related issues particularly in ASIC
which has VRAM space <= 500MiB

Hence, we are reverting this patch.

Gitlab issue link - https://gitlab.freedesktop.org/drm/amd/-/issues/2270

Signed-off-by: Arunpravin Paneer Selvam 


For now Reviewed-by: Christian König  for this 
change.


The extra overhead for the topdown handling is intentional here as far 
as I can see.


Christian.


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 2bd1a54ee866..ca5fc07faf6c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -139,7 +139,7 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, 
u32 domain)
  
  		if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)

places[c].lpfn = visible_pfn;
-   else if (adev->gmc.real_vram_size != 
adev->gmc.visible_vram_size)
+   else
places[c].flags |= TTM_PL_FLAG_TOPDOWN;
  
  		if (flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)

Re: [PATCH 10/66] drm/amd/display: Do not set drr on pipe commit

[Michel Dänzer]

On 5/31/23 22:14, Aurabindo Pillai wrote:
> On 5/11/23 03:06, Michel Dänzer wrote:
>> On 5/10/23 22:54, Aurabindo Pillai wrote:
>>> On 5/10/23 09:20, Michel Dänzer wrote:
 On 5/9/23 23:07, Pillai, Aurabindo wrote:
>
> Sorry - the firmware in the previous message is for DCN32. For Navi2x, 
> please use the firmware attached here.

 Same problem (contents of /sys/kernel/debug/dri/0/amdgpu_firmware_info 
 below).

 Even if it did work with newer FW, the kernel must keep working with older 
 FW, so in that case the new behaviour would need to be guarded by the FW 
 version.

>>>
>>> Agreed. Were you able to repro the hang on any other modes/monitors? 
>>
>> Haven't tried specifically, and this is the only system I have with VRR.
>>
>>
> Hi Michel,
> 
> I've fixed a related issue on Navi21. Could you please try the attached DMCUB 
> along with the patches to be applied on top of amd-staging-drm-next and check 
> if the hang/corruption is gone? 

Thanks, though I'm afraid that made it kind of worse: Now it already hangs when 
Steam starts up in Big Picture mode. Same with the new DMCUB firmware or older 
one.

This time, only

 amdgpu :0c:00.0: [drm] *ERROR* [CRTC:82:crtc-0] flip_done timed out

appears in dmesg.


-- 
Earthling Michel Dänzer|  https://redhat.com
Libre software enthusiast  | Mesa and Xwayland developer

RE: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

[AMD Official Use Only - General]

> -Original Message-
> From: Alex Deucher 
> Sent: Wednesday, May 31, 2023 10:22 PM
> To: Limonciello, Mario 
> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> 
> Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> again
>
> On Wed, May 31, 2023 at 9:26 AM Alex Deucher 
> wrote:
> >
> > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> >  wrote:
> > >
> > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> showed
> > > improvements to power consumption over suspend when s0ix wasn't
> enabled in
> > > BIOS and the system didn't support S3.
> > >
> > > This patch however was misguided because the reason the system didn't
> > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > This prevented the BIOS from allowing S3.
> > >
> > > Also allowing GPUs to use the s2idle path actually causes problems if
> > > they're invoked on systems that may not support s2idle in the platform
> > > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> > > for any reason, which could lead to unexpected flows.
> > >
> > > The original commit also fixed a problem during resume from suspend to
> idle
> > > without hardware support, but this is no longer necessary with commit
> > > ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than Raven")
> > >
> > > Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS
> support")
> > > to make it match the expected behavior again.
> > >
> > > Cc: Rafael Ávila de Espíndola 
> > > Link:
> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> /amdgpu_acpi.c#L1060
> > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > Signed-off-by: Mario Limonciello 
> >
> > Patch 1 is:
> > Reviewed-by: Alex Deucher 
> > Patch 2 seems a bit much, but I could be convinced if you think it
> > will actually help more than a warn would.  Users already assume warn
> > is a kernel crash.  I'm not sure the average user makes a distinction
> > between warn and err.
> >
>
> You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> system is configured properly") as well, otherwise, we'll break
> runtime pm.
>

Can you elaborate more on your thought process?  d2a197a45daacd was added in 
5.18
and cf488dcd0ab7 was added in 6.3.  I can't imagine runtime PM is broken the 
whole time
on dGPUs.

> Alex
>
> > Alex
> >
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > index aeeec211861c..e1b01554e323 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > @@ -1092,16 +1092,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> amdgpu_device *adev)
> > >  * S0ix even though the system is suspending to idle, so return 
> > > false
> > >  * in that case.
> > >  */
> > > -   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > +   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> > > dev_warn_once(adev->dev,
> > >   "Power consumption will be higher as BIOS 
> > > has not been
> configured for suspend-to-idle.\n"
> > >   "To use suspend-to-idle change the sleep 
> > > mode in BIOS
> setup.\n");
> > > +   return false;
> > > +   }
> > >
> > >  #if !IS_ENABLED(CONFIG_AMD_PMC)
> > > dev_warn_once(adev->dev,
> > >   "Power consumption will be higher as the kernel has 
> > > not been
> compiled with CONFIG_AMD_PMC.\n");
> > > -#endif /* CONFIG_AMD_PMC */
> > > +   return false;
> > > +#else
> > > return true;
> > > +#endif /* CONFIG_AMD_PMC */
> > >  }
> > >
> > >  #endif /* CONFIG_SUSPEND */
> > > --
> > > 2.34.1
> > >

Re: [PATCH 10/66] drm/amd/display: Do not set drr on pipe commit

[Pillai, Aurabindo]

[Public]

I see, thanks for the info. I'll try repro'ing it locally. But do you have the 
open userspace stack from AMD's packaged driver installed ? If not, could you 
please try downloading from https://www.amd.com/en/support/linux-drivers and 
install just the open components? You can run:

sudo amdgpu-install --use-case=graphics --no-dkms


--

Regards,
Jay

From: amd-gfx  on behalf of Michel 
Dänzer 
Sent: Thursday, June 1, 2023 10:59 AM
To: Pillai, Aurabindo ; Zhuo, Qingqing (Lillian) 
; amd-gfx@lists.freedesktop.org 
; Chalmers, Wesley 
Cc: Wang, Chao-kai (Stylon) ; Li, Sun peng (Leo) 
; Wentland, Harry ; Siqueira, 
Rodrigo ; Li, Roman ; Chiu, Solomon 
; Lin, Wayne ; Lakha, Bhawanpreet 
; Gutierrez, Agustin ; 
Kotarac, Pavle 
Subject: Re: [PATCH 10/66] drm/amd/display: Do not set drr on pipe commit

On 5/31/23 22:14, Aurabindo Pillai wrote:
> On 5/11/23 03:06, Michel Dänzer wrote:
>> On 5/10/23 22:54, Aurabindo Pillai wrote:
>>> On 5/10/23 09:20, Michel Dänzer wrote:
 On 5/9/23 23:07, Pillai, Aurabindo wrote:
>
> Sorry - the firmware in the previous message is for DCN32. For Navi2x, 
> please use the firmware attached here.

 Same problem (contents of /sys/kernel/debug/dri/0/amdgpu_firmware_info 
 below).

 Even if it did work with newer FW, the kernel must keep working with older 
 FW, so in that case the new behaviour would need to be guarded by the FW 
 version.

>>>
>>> Agreed. Were you able to repro the hang on any other modes/monitors?
>>
>> Haven't tried specifically, and this is the only system I have with VRR.
>>
>>
> Hi Michel,
>
> I've fixed a related issue on Navi21. Could you please try the attached DMCUB 
> along with the patches to be applied on top of amd-staging-drm-next and check 
> if the hang/corruption is gone?

Thanks, though I'm afraid that made it kind of worse: Now it already hangs when 
Steam starts up in Big Picture mode. Same with the new DMCUB firmware or older 
one.

This time, only

 amdgpu :0c:00.0: [drm] *ERROR* [CRTC:82:crtc-0] flip_done timed out

appears in dmesg.


--
Earthling Michel Dänzer|  https://redhat.com
Libre software enthusiast  | Mesa and Xwayland developer

Re: [PATCH 10/66] drm/amd/display: Do not set drr on pipe commit

[Michel Dänzer]

On 6/1/23 17:45, Pillai, Aurabindo wrote:
> 
> I see, thanks for the info. I'll try repro'ing it locally.

Thanks. Note that I'm using a GNOME Wayland session, which doesn't support VRR 
upstream yet (I'm building mutter with 
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1154 for that). I don't 
know if it's reproducible with Xorg.


> But do you have the open userspace stack from AMD's packaged driver installed 
> ? If not, could you please try downloading from 
> https://www.amd.com/en/support/linux-drivers 
>  and install just the open 
> components?

I don't, and I'd rather not unless it's absolutely necessary. I'm not sure how 
the user-space drivers could affect this.

I'll happily test further patches though.


-- 
Earthling Michel Dänzer|  https://redhat.com
Libre software enthusiast  | Mesa and Xwayland developer

Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support again

[Alex Deucher]

On Thu, Jun 1, 2023 at 11:33 AM Limonciello, Mario
 wrote:
>
> [AMD Official Use Only - General]
>
> > -Original Message-
> > From: Alex Deucher 
> > Sent: Wednesday, May 31, 2023 10:22 PM
> > To: Limonciello, Mario 
> > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > 
> > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > again
> >
> > On Wed, May 31, 2023 at 9:26 AM Alex Deucher 
> > wrote:
> > >
> > > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> > >  wrote:
> > > >
> > > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > showed
> > > > improvements to power consumption over suspend when s0ix wasn't
> > enabled in
> > > > BIOS and the system didn't support S3.
> > > >
> > > > This patch however was misguided because the reason the system didn't
> > > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > > This prevented the BIOS from allowing S3.
> > > >
> > > > Also allowing GPUs to use the s2idle path actually causes problems if
> > > > they're invoked on systems that may not support s2idle in the platform
> > > > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` 
> > > > fails
> > > > for any reason, which could lead to unexpected flows.
> > > >
> > > > The original commit also fixed a problem during resume from suspend to
> > idle
> > > > without hardware support, but this is no longer necessary with commit
> > > > ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than Raven")
> > > >
> > > > Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS
> > support")
> > > > to make it match the expected behavior again.
> > > >
> > > > Cc: Rafael Ávila de Espíndola 
> > > > Link:
> > https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > /amdgpu_acpi.c#L1060
> > > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > > Signed-off-by: Mario Limonciello 
> > >
> > > Patch 1 is:
> > > Reviewed-by: Alex Deucher 
> > > Patch 2 seems a bit much, but I could be convinced if you think it
> > > will actually help more than a warn would.  Users already assume warn
> > > is a kernel crash.  I'm not sure the average user makes a distinction
> > > between warn and err.
> > >
> >
> > You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> > system is configured properly") as well, otherwise, we'll break
> > runtime pm.
> >
>
> Can you elaborate more on your thought process?  d2a197a45daacd was added in 
> 5.18
> and cf488dcd0ab7 was added in 6.3.  I can't imagine runtime PM is broken the 
> whole time
> on dGPUs.

I tested this patch yesterday and it broke runtime pm because
amdgpu_pmops_prepare() returned 1.  I haven't delved into what
condition broke.  Reverting this patch restored runtime pm.  This is a
Threadripper box that only supports S3.  The dGPUs were polaris and
navi2x.

Alex


>
> > Alex
> >
> > > Alex
> > >
> > > > ---
> > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > index aeeec211861c..e1b01554e323 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > @@ -1092,16 +1092,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > amdgpu_device *adev)
> > > >  * S0ix even though the system is suspending to idle, so return 
> > > > false
> > > >  * in that case.
> > > >  */
> > > > -   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > > +   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> > > > dev_warn_once(adev->dev,
> > > >   "Power consumption will be higher as BIOS 
> > > > has not been
> > configured for suspend-to-idle.\n"
> > > >   "To use suspend-to-idle change the sleep 
> > > > mode in BIOS
> > setup.\n");
> > > > +   return false;
> > > > +   }
> > > >
> > > >  #if !IS_ENABLED(CONFIG_AMD_PMC)
> > > > dev_warn_once(adev->dev,
> > > >   "Power consumption will be higher as the kernel 
> > > > has not been
> > compiled with CONFIG_AMD_PMC.\n");
> > > > -#endif /* CONFIG_AMD_PMC */
> > > > +   return false;
> > > > +#else
> > > > return true;
> > > > +#endif /* CONFIG_AMD_PMC */
> > > >  }
> > > >
> > > >  #endif /* CONFIG_SUSPEND */
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH] drm/amd/pm: Fix power context allocation in SMU13

[Alex Deucher]

On Thu, Jun 1, 2023 at 10:31 AM Hawking Zhang  wrote:
>
> From: Lijo Lazar 
>
> Use the right data structure for allocation.
>
> Signed-off-by: Lijo Lazar 
> Reviewed-by: Hawking Zhang 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c 
> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index da059b02a153..09ac66ab9c34 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -534,11 +534,11 @@ int smu_v13_0_init_power(struct smu_context *smu)
> if (smu_power->power_context || smu_power->power_context_size != 0)
> return -EINVAL;
>
> -   smu_power->power_context = kzalloc(sizeof(struct 
> smu_13_0_dpm_context),
> +   smu_power->power_context = kzalloc(sizeof(struct 
> smu_13_0_power_context),
>GFP_KERNEL);
> if (!smu_power->power_context)
> return -ENOMEM;
> -   smu_power->power_context_size = sizeof(struct smu_13_0_dpm_context);
> +   smu_power->power_context_size = sizeof(struct smu_13_0_power_context);
>
> return 0;
>  }
> --
> 2.17.1
>

Re: [PATCH v5 11/13] drm/msm: Use regular fbdev I/O helpers

[Abhinav Kumar]

On 5/30/2023 8:02 AM, Thomas Zimmermann wrote:

Use the regular fbdev helpers for framebuffer I/O instead of DRM's
helpers. Msm does not use damage handling, so DRM's fbdev helpers
are mere wrappers around the fbdev code.

By using fbdev helpers directly within each DRM fbdev emulation,
we can eventually remove DRM's wrapper functions entirely.

Msm's fbdev emulation has been incomplete as it didn't implement
damage handling. Partilly fix this by implementing damage handling
for write and draw operation. It is still missing for mmaped pages.

v4:
* use initializer macros for struct fb_ops
* partially support damage handling
v2:
* use FB_SYS_HELPERS option

Signed-off-by: Thomas Zimmermann 
Reviewed-by: Dmitry Baryshkov 
Acked-by: Sam Ravnborg 
Cc: Rob Clark 
Cc: Abhinav Kumar 
Cc: Dmitry Baryshkov 
Cc: Sean Paul 
---


Reviewed-by: Abhinav Kumar 

[PATCH] drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+

[Aurabindo Pillai]

Due to FPO, firmware will try to change OTG timings, which would only
latch if min/max selectors for vtotal are written by the driver.

Signed-off-by: Aurabindo Pillai 
---
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 15 +++
 drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c |  6 ++
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
index e1975991e075..633989fd2514 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
@@ -930,19 +930,10 @@ void optc1_set_drr(
OTG_FORCE_LOCK_ON_EVENT, 0,
OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
OTG_SET_V_TOTAL_MIN_MASK, 0);
-
-   // Setup manual flow control for EOF via TRIG_A
-   optc->funcs->setup_manual_trigger(optc);
-
-   } else {
-   REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
-   OTG_SET_V_TOTAL_MIN_MASK, 0,
-   OTG_V_TOTAL_MIN_SEL, 0,
-   OTG_V_TOTAL_MAX_SEL, 0,
-   OTG_FORCE_LOCK_ON_EVENT, 0);
-
-   optc->funcs->set_vtotal_min_max(optc, 0, 0);
}
+
+   // Setup manual flow control for EOF via TRIG_A
+   optc->funcs->setup_manual_trigger(optc);
 }
 
 void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, 
int vtotal_max)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
index e0edc163d767..042ce082965f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
@@ -455,6 +455,12 @@ void optc2_setup_manual_trigger(struct timing_generator 
*optc)
 {
struct optc *optc1 = DCN10TG_FROM_TG(optc);
 
+   REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
+OTG_V_TOTAL_MIN_SEL, 1,
+OTG_V_TOTAL_MAX_SEL, 1,
+OTG_FORCE_LOCK_ON_EVENT, 0,
+OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA 
*/
+
REG_SET_8(OTG_TRIGA_CNTL, 0,
OTG_TRIGA_SOURCE_SELECT, 21,
OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,
-- 
2.40.1

[linux-next:master] BUILD REGRESSION 571d71e886a5edc89b4ea6d0fe6f445282938320

[kernel test robot]

tree/branch: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: 571d71e886a5edc89b4ea6d0fe6f445282938320  Add linux-next specific 
files for 20230601

Error/Warning reports:

https://lore.kernel.org/oe-kbuild-all/202305230552.wobyqyya-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202305311652.op9x8xkw-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306010248.g3zqqg4w-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306011356.mntu7q9z-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306011435.2bxshfue-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306011753.7examz0m-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306011915.bwdy8aj8-...@intel.com

Error/Warning: (recently discovered and may have been fixed)

drivers/net/dsa/qca/qca8k-leds.c:377:31: error: 'struct led_classdev' has no 
member named 'hw_control_is_supported'
drivers/net/dsa/qca/qca8k-leds.c:378:31: error: 'struct led_classdev' has no 
member named 'hw_control_set'
drivers/net/dsa/qca/qca8k-leds.c:379:31: error: 'struct led_classdev' has no 
member named 'hw_control_get'
drivers/net/dsa/qca/qca8k-leds.c:380:31: error: 'struct led_classdev' has no 
member named 'hw_control_trigger'
drivers/net/dsa/qca/qca8k-leds.c:406:18: error: no member named 
'hw_control_get_device' in 'struct led_classdev'
drivers/net/dsa/qca/qca8k-leds.c:406:31: error: 'struct led_classdev' has no 
member named 'hw_control_get_device'
include/drm/drm_print.h:456:39: error: format '%ld' expects argument of type 
'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} 
[-Werror=format=]
include/linux/usb/typec_mux.h:76:33: warning: 'fwnode_typec_mux_get' used but 
never defined
include/linux/usb/typec_mux.h:77:1: error: expected identifier or '('
include/linux/usb/typec_mux.h:77:1: error: expected identifier or '(' before 
'{' token
mm/zswap.c:1183:6: warning: variable 'ret' is used uninitialized whenever 'if' 
condition is true [-Wsometimes-uninitialized]

Unverified Error/Warning (likely false positive, please contact us if 
interested):

arch/arm64/kvm/mmu.c:147:3-9: preceding lock on line 140
fs/smb/client/cifsfs.c:982 cifs_smb3_do_mount() warn: possible memory leak of 
'cifs_sb'
fs/smb/client/cifssmb.c:4089 CIFSFindFirst() warn: missing error code? 'rc'
fs/smb/client/cifssmb.c:4216 CIFSFindNext() warn: missing error code? 'rc'
fs/smb/client/connect.c:2725 cifs_match_super() error: 'tlink' dereferencing 
possible ERR_PTR()
fs/smb/client/connect.c:2924 generic_ip_connect() error: we previously assumed 
'socket' could be null (see line 2912)
fs/xfs/scrub/fscounters.c:459 xchk_fscounters() warn: ignoring unreachable code.
kernel/events/uprobes.c:478 uprobe_write_opcode() warn: passing zero to 
'PTR_ERR'
{standard input}:1078: Error: pcrel too far

Error/Warning ids grouped by kconfigs:

gcc_recent_errors
|-- arc-randconfig-c004-20230531
|   |-- 
include-linux-usb-typec_mux.h:error:expected-identifier-or-(-before-token
|   `-- 
include-linux-usb-typec_mux.h:warning:fwnode_typec_mux_get-used-but-never-defined
|-- arm64-randconfig-c033-20230531
|   |-- arch-arm64-kvm-mmu.c:preceding-lock-on-line
|   |-- 
include-linux-usb-typec_mux.h:error:expected-identifier-or-(-before-token
|   `-- 
include-linux-usb-typec_mux.h:warning:fwnode_typec_mux_get-used-but-never-defined
|-- arm64-randconfig-r001-20230531
|   |-- 
drivers-net-dsa-qca-qca8k-leds.c:error:struct-led_classdev-has-no-member-named-hw_control_get
|   |-- 
drivers-net-dsa-qca-qca8k-leds.c:error:struct-led_classdev-has-no-member-named-hw_control_get_device
|   |-- 
drivers-net-dsa-qca-qca8k-leds.c:error:struct-led_classdev-has-no-member-named-hw_control_is_supported
|   |-- 
drivers-net-dsa-qca-qca8k-leds.c:error:struct-led_classdev-has-no-member-named-hw_control_set
|   `-- 
drivers-net-dsa-qca-qca8k-leds.c:error:struct-led_classdev-has-no-member-named-hw_control_trigger
|-- arm64-randconfig-r016-20230601
|   `-- 
include-linux-usb-typec_mux.h:error:expected-identifier-or-(-before-token
|-- i386-allyesconfig
|   `-- 
include-drm-drm_print.h:error:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int
|-- i386-randconfig-m021-20230531
|   |-- 
fs-smb-client-cifsfs.c-cifs_smb3_do_mount()-warn:possible-memory-leak-of-cifs_sb
|   |-- fs-smb-client-cifssmb.c-CIFSFindFirst()-warn:missing-error-code-rc
|   |-- fs-smb-client-cifssmb.c-CIFSFindNext()-warn:missing-error-code-rc
|   |-- 
fs-smb-client-connect.c-cifs_match_super()-error:tlink-dereferencing-possible-ERR_PTR()
|   |-- 
fs-smb-client-connect.c-generic_ip_connect()-error:we-previously-assumed-socket-could-be-null-(see-line-)
|   |-- 
fs-xfs-scrub-fscounters.c-xchk_fscount

RE: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

[AMD Official Use Only - General]

> -Original Message-
> From: Alex Deucher 
> Sent: Thursday, June 1, 2023 11:15 AM
> To: Limonciello, Mario 
> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> 
> Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> again
>
> On Thu, Jun 1, 2023 at 11:33 AM Limonciello, Mario
>  wrote:
> >
> > [AMD Official Use Only - General]
> >
> > > -Original Message-
> > > From: Alex Deucher 
> > > Sent: Wednesday, May 31, 2023 10:22 PM
> > > To: Limonciello, Mario 
> > > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > > 
> > > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > > again
> > >
> > > On Wed, May 31, 2023 at 9:26 AM Alex Deucher
> 
> > > wrote:
> > > >
> > > > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> > > >  wrote:
> > > > >
> > > > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > > showed
> > > > > improvements to power consumption over suspend when s0ix wasn't
> > > enabled in
> > > > > BIOS and the system didn't support S3.
> > > > >
> > > > > This patch however was misguided because the reason the system
> didn't
> > > > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > > > This prevented the BIOS from allowing S3.
> > > > >
> > > > > Also allowing GPUs to use the s2idle path actually causes problems if
> > > > > they're invoked on systems that may not support s2idle in the platform
> > > > > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` 
> > > > > fails
> > > > > for any reason, which could lead to unexpected flows.
> > > > >
> > > > > The original commit also fixed a problem during resume from suspend
> to
> > > idle
> > > > > without hardware support, but this is no longer necessary with commit
> > > > > ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than
> Raven")
> > > > >
> > > > > Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS
> > > support")
> > > > > to make it match the expected behavior again.
> > > > >
> > > > > Cc: Rafael Ávila de Espíndola 
> > > > > Link:
> > >
> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > > /amdgpu_acpi.c#L1060
> > > > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > > > Signed-off-by: Mario Limonciello 
> > > >
> > > > Patch 1 is:
> > > > Reviewed-by: Alex Deucher 
> > > > Patch 2 seems a bit much, but I could be convinced if you think it
> > > > will actually help more than a warn would.  Users already assume warn
> > > > is a kernel crash.  I'm not sure the average user makes a distinction
> > > > between warn and err.
> > > >
> > >
> > > You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> > > system is configured properly") as well, otherwise, we'll break
> > > runtime pm.
> > >
> >
> > Can you elaborate more on your thought process?  d2a197a45daacd was
> added in 5.18
> > and cf488dcd0ab7 was added in 6.3.  I can't imagine runtime PM is broken
> the whole time
> > on dGPUs.
>
> I tested this patch yesterday and it broke runtime pm because
> amdgpu_pmops_prepare() returned 1.  I haven't delved into what
> condition broke.  Reverting this patch restored runtime pm.  This is a
> Threadripper box that only supports S3.  The dGPUs were polaris and
> navi2x.
>

But runtime_suspend isn't supposed to run the prepare() callback AFACIT.
SMART_PREPARE is only used for system wide suspend/resume.

> Alex
>
>
> >
> > > Alex
> > >
> > > > Alex
> > > >
> > > > > ---
> > > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > index aeeec211861c..e1b01554e323 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > @@ -1092,16 +1092,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > > amdgpu_device *adev)
> > > > >  * S0ix even though the system is suspending to idle, so 
> > > > > return false
> > > > >  * in that case.
> > > > >  */
> > > > > -   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > > > +   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> > > > > dev_warn_once(adev->dev,
> > > > >   "Power consumption will be higher as 
> > > > > BIOS has not been
> > > configured for suspend-to-idle.\n"
> > > > >   "To use suspend-to-idle change the 
> > > > > sleep mode in BIOS
> > > setup.\n");
> > > > > +   return false;
> > > > > +   }
> > > > >
> > > > >  #if !IS_ENABLED(CONFIG_AMD_PMC)
> > > > > dev_warn_once(adev->dev,
> > > > >   "Power consumption will be higher as the kernel 
> > > > > has not
> been
> > > compiled with CONFIG_AMD_PMC.\n");
> > > > > -#endif

Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support again

[Alex Deucher]

On Thu, Jun 1, 2023 at 1:32 PM Limonciello, Mario
 wrote:
>
> [AMD Official Use Only - General]
>
> > -Original Message-
> > From: Alex Deucher 
> > Sent: Thursday, June 1, 2023 11:15 AM
> > To: Limonciello, Mario 
> > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > 
> > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > again
> >
> > On Thu, Jun 1, 2023 at 11:33 AM Limonciello, Mario
> >  wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > > > -Original Message-
> > > > From: Alex Deucher 
> > > > Sent: Wednesday, May 31, 2023 10:22 PM
> > > > To: Limonciello, Mario 
> > > > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > > > 
> > > > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > > > again
> > > >
> > > > On Wed, May 31, 2023 at 9:26 AM Alex Deucher
> > 
> > > > wrote:
> > > > >
> > > > > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> > > > >  wrote:
> > > > > >
> > > > > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > > > showed
> > > > > > improvements to power consumption over suspend when s0ix wasn't
> > > > enabled in
> > > > > > BIOS and the system didn't support S3.
> > > > > >
> > > > > > This patch however was misguided because the reason the system
> > didn't
> > > > > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > > > > This prevented the BIOS from allowing S3.
> > > > > >
> > > > > > Also allowing GPUs to use the s2idle path actually causes problems 
> > > > > > if
> > > > > > they're invoked on systems that may not support s2idle in the 
> > > > > > platform
> > > > > > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` 
> > > > > > fails
> > > > > > for any reason, which could lead to unexpected flows.
> > > > > >
> > > > > > The original commit also fixed a problem during resume from suspend
> > to
> > > > idle
> > > > > > without hardware support, but this is no longer necessary with 
> > > > > > commit
> > > > > > ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than
> > Raven")
> > > > > >
> > > > > > Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS
> > > > support")
> > > > > > to make it match the expected behavior again.
> > > > > >
> > > > > > Cc: Rafael Ávila de Espíndola 
> > > > > > Link:
> > > >
> > https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > > > /amdgpu_acpi.c#L1060
> > > > > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > > > > Signed-off-by: Mario Limonciello 
> > > > >
> > > > > Patch 1 is:
> > > > > Reviewed-by: Alex Deucher 
> > > > > Patch 2 seems a bit much, but I could be convinced if you think it
> > > > > will actually help more than a warn would.  Users already assume warn
> > > > > is a kernel crash.  I'm not sure the average user makes a distinction
> > > > > between warn and err.
> > > > >
> > > >
> > > > You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> > > > system is configured properly") as well, otherwise, we'll break
> > > > runtime pm.
> > > >
> > >
> > > Can you elaborate more on your thought process?  d2a197a45daacd was
> > added in 5.18
> > > and cf488dcd0ab7 was added in 6.3.  I can't imagine runtime PM is broken
> > the whole time
> > > on dGPUs.
> >
> > I tested this patch yesterday and it broke runtime pm because
> > amdgpu_pmops_prepare() returned 1.  I haven't delved into what
> > condition broke.  Reverting this patch restored runtime pm.  This is a
> > Threadripper box that only supports S3.  The dGPUs were polaris and
> > navi2x.
> >
>
> But runtime_suspend isn't supposed to run the prepare() callback AFACIT.
> SMART_PREPARE is only used for system wide suspend/resume.

Not sure what is happening then as amdgpu_acpi_is_s0ix_active() only
affects amdgpu_pmops_prepare(), amdgpu_pmops_suspend(), and
amdgpu_pmops_resume().  I'll try to trace what is going on.

Alex

>
> > Alex
> >
> >
> > >
> > > > Alex
> > > >
> > > > > Alex
> > > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > index aeeec211861c..e1b01554e323 100644
> > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > @@ -1092,16 +1092,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > > > amdgpu_device *adev)
> > > > > >  * S0ix even though the system is suspending to idle, so 
> > > > > > return false
> > > > > >  * in that case.
> > > > > >  */
> > > > > > -   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > > > > +   if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> > > > > > dev_warn_once(adev->dev,
> > > > > >   "Power co

Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support again

[Alex Deucher]

On Thu, Jun 1, 2023 at 2:23 PM Alex Deucher  wrote:
>
> On Thu, Jun 1, 2023 at 1:32 PM Limonciello, Mario
>  wrote:
> >
> > [AMD Official Use Only - General]
> >
> > > -Original Message-
> > > From: Alex Deucher 
> > > Sent: Thursday, June 1, 2023 11:15 AM
> > > To: Limonciello, Mario 
> > > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > > 
> > > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > > again
> > >
> > > On Thu, Jun 1, 2023 at 11:33 AM Limonciello, Mario
> > >  wrote:
> > > >
> > > > [AMD Official Use Only - General]
> > > >
> > > > > -Original Message-
> > > > > From: Alex Deucher 
> > > > > Sent: Wednesday, May 31, 2023 10:22 PM
> > > > > To: Limonciello, Mario 
> > > > > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > > > > 
> > > > > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS 
> > > > > support
> > > > > again
> > > > >
> > > > > On Wed, May 31, 2023 at 9:26 AM Alex Deucher
> > > 
> > > > > wrote:
> > > > > >
> > > > > > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> > > > > >  wrote:
> > > > > > >
> > > > > > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > > > > showed
> > > > > > > improvements to power consumption over suspend when s0ix wasn't
> > > > > enabled in
> > > > > > > BIOS and the system didn't support S3.
> > > > > > >
> > > > > > > This patch however was misguided because the reason the system
> > > didn't
> > > > > > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > > > > > This prevented the BIOS from allowing S3.
> > > > > > >
> > > > > > > Also allowing GPUs to use the s2idle path actually causes 
> > > > > > > problems if
> > > > > > > they're invoked on systems that may not support s2idle in the 
> > > > > > > platform
> > > > > > > firmware. `systemd` has a tendency to try to use `s2idle` if 
> > > > > > > `deep` fails
> > > > > > > for any reason, which could lead to unexpected flows.
> > > > > > >
> > > > > > > The original commit also fixed a problem during resume from 
> > > > > > > suspend
> > > to
> > > > > idle
> > > > > > > without hardware support, but this is no longer necessary with 
> > > > > > > commit
> > > > > > > ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than
> > > Raven")
> > > > > > >
> > > > > > > Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS
> > > > > support")
> > > > > > > to make it match the expected behavior again.
> > > > > > >
> > > > > > > Cc: Rafael Ávila de Espíndola 
> > > > > > > Link:
> > > > >
> > > https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > > > > /amdgpu_acpi.c#L1060
> > > > > > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > > > > > Signed-off-by: Mario Limonciello 
> > > > > >
> > > > > > Patch 1 is:
> > > > > > Reviewed-by: Alex Deucher 
> > > > > > Patch 2 seems a bit much, but I could be convinced if you think it
> > > > > > will actually help more than a warn would.  Users already assume 
> > > > > > warn
> > > > > > is a kernel crash.  I'm not sure the average user makes a 
> > > > > > distinction
> > > > > > between warn and err.
> > > > > >
> > > > >
> > > > > You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> > > > > system is configured properly") as well, otherwise, we'll break
> > > > > runtime pm.
> > > > >
> > > >
> > > > Can you elaborate more on your thought process?  d2a197a45daacd was
> > > added in 5.18
> > > > and cf488dcd0ab7 was added in 6.3.  I can't imagine runtime PM is broken
> > > the whole time
> > > > on dGPUs.
> > >
> > > I tested this patch yesterday and it broke runtime pm because
> > > amdgpu_pmops_prepare() returned 1.  I haven't delved into what
> > > condition broke.  Reverting this patch restored runtime pm.  This is a
> > > Threadripper box that only supports S3.  The dGPUs were polaris and
> > > navi2x.
> > >
> >
> > But runtime_suspend isn't supposed to run the prepare() callback AFACIT.
> > SMART_PREPARE is only used for system wide suspend/resume.
>
> Not sure what is happening then as amdgpu_acpi_is_s0ix_active() only
> affects amdgpu_pmops_prepare(), amdgpu_pmops_suspend(), and
> amdgpu_pmops_resume().  I'll try to trace what is going on.

Nevermind, now I can't repro it.

Alex


>
> Alex
>
> >
> > > Alex
> > >
> > >
> > > >
> > > > > Alex
> > > > >
> > > > > > Alex
> > > > > >
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> > > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > > index aeeec211861c..e1b01554e323 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > > > > > @@ -1092,16 +1092,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > > > > amdgpu_device *adev)
> > > > > > >

Re: [PATCH 06/36] drm/amd/display: add CRTC driver-specific property for gamma TF

[Harry Wentland]

On 5/23/23 18:14, Melissa Wen wrote:
> Hook up driver-specific atomic operations for managing AMD color
> properties and create AMD driver-specific color management properties
> and attach them according to HW capabilities defined by `struct
> dc_color_caps`. Add enumerated transfer function property to DRM CRTC
> gamma to convert to wire encoding with or without a user gamma LUT.
> Enumerated TFs are not supported yet by the DRM color pipeline,
> therefore, create a DRM enum list with the predefined TFs supported by
> the AMD display driver.
> 
> Co-developed-by: Joshua Ashton 
> Signed-off-by: Joshua Ashton 
> Signed-off-by: Melissa Wen 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   | 36 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h  |  8 +++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 22 ++
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 72 ++-
>  4 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 389396eac222..88af075e6c18 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1247,6 +1247,38 @@ amdgpu_display_user_framebuffer_create(struct 
> drm_device *dev,
>   return &amdgpu_fb->base;
>  }
>  
> +static const struct drm_prop_enum_list drm_transfer_function_enum_list[] = {
> + { DRM_TRANSFER_FUNCTION_DEFAULT, "Default" },
> + { DRM_TRANSFER_FUNCTION_SRGB, "sRGB" },
> + { DRM_TRANSFER_FUNCTION_BT709, "BT.709" },
> + { DRM_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
> + { DRM_TRANSFER_FUNCTION_LINEAR, "Linear" },
> + { DRM_TRANSFER_FUNCTION_UNITY, "Unity" },
> + { DRM_TRANSFER_FUNCTION_HLG, "HLG (Hybrid Log Gamma)" },
> + { DRM_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
> + { DRM_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
> + { DRM_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
> +};

Let's not use the DRM_/drm_ prefix here. It might clash later when
we introduce DRM_ core interfaces for enumerated transfer functions.

We'll want to specify whether something is an EOTF or an inverse EOTF,
or possibly an OETF. Of course that wouldn't apply to "Linear" or
"Unity" (I'm assuming the two are the same?).

EOTF = electro-optical transfer function
This is the transfer function to go from the encoded value to an
optical (linear) value.

Inverse EOTF = simply the inverse of the EOTF
This is usually intended to go from an optical/linear space (which
might have been used for blending) back to the encoded values.

OETF = opto-electronic transfer function
This is usually used for converting optical signals into encoded
values. Usually that's done on the camera side but I think HLG might
define the OETF instead of the EOTF. A bit fuzzy on that. If you're
unclear about HLG I recommend we don't include it yet.

It would also be good to document a bit more what each of the TFs
mean, but I'm fine if that comes later with a "driver-agnostic"
API. The key thing to clarify is whether we have an EOTF or inv_EOTF,
i.e. whether we use the TF to go from encoded to optical or vice
versa.

I know DC is sloppy and doesn't define those but it will still use
them as either EOTF or inv_EOTF, based on which block they're being
programmed, if I'm not mistaken.

> +
> +#ifdef AMD_PRIVATE_COLOR
> +static int
> +amdgpu_display_create_color_properties(struct amdgpu_device *adev)
> +{
> + struct drm_property *prop;
> +
> + prop = drm_property_create_enum(adev_to_drm(adev),
> + DRM_MODE_PROP_ENUM,
> + "AMD_REGAMMA_TF",
> + drm_transfer_function_enum_list,
> + 
> ARRAY_SIZE(drm_transfer_function_enum_list));
> + if (!prop)
> + return -ENOMEM;
> + adev->mode_info.regamma_tf_property = prop;
> +
> + return 0;
> +}
> +#endif
> +

It'd be nice if we have this function and the above enum_list
in amdgpu_dm, possibly in amdgpu_dm_color.c. You could call it
directly after the amdgpu_display_modeset_create_prop() call in 
amdgpu_dm_mode_config_init().

>  const struct drm_mode_config_funcs amdgpu_mode_funcs = {
>   .fb_create = amdgpu_display_user_framebuffer_create,
>  };
> @@ -1323,6 +1355,10 @@ int amdgpu_display_modeset_create_props(struct 
> amdgpu_device *adev)
>   return -ENOMEM;
>   }
>  
> +#ifdef AMD_PRIVATE_COLOR
> + if (amdgpu_display_create_color_properties(adev))
> + return -ENOMEM;
> +#endif
>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index b8633df418d4..881446c51b36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -344,6 +344,14 @@ struct amdgpu_mode_info {
>   int disp_prio

Re: [PATCH 07/36] drm/amd/display: add plane driver-specific properties for degamma LUT

[Harry Wentland]

On 5/23/23 18:14, Melissa Wen wrote:
> Create and attach driver-private properties for plane color management.
> First add plane degamma LUT properties that means user-blob and its
> size. We will add more plane color properties in the next commits. In
> addition, we keep these driver-private plane properties limited by
> defining AMD_PRIVATE_COLOR.
> 
> Co-developed-by: Joshua Ashton 
> Signed-off-by: Joshua Ashton 
> Signed-off-by: Melissa Wen 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   | 14 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h  |  8 ++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 77 +++
>  4 files changed, 108 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 88af075e6c18..fa67c84f5994 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1275,6 +1275,20 @@ amdgpu_display_create_color_properties(struct 
> amdgpu_device *adev)
>   return -ENOMEM;
>   adev->mode_info.regamma_tf_property = prop;
>  
> + prop = drm_property_create(adev_to_drm(adev),
> +DRM_MODE_PROP_BLOB,
> +"AMD_PLANE_DEGAMMA_LUT", 0);
> + if (!prop)
> + return -ENOMEM;
> + adev->mode_info.plane_degamma_lut_property = prop;
> +
> + prop = drm_property_create_range(adev_to_drm(adev),
> +  DRM_MODE_PROP_IMMUTABLE,
> +  "AMD_PLANE_DEGAMMA_LUT_SIZE", 0, 
> UINT_MAX);
> + if (!prop)
> + return -ENOMEM;
> + adev->mode_info.plane_degamma_lut_size_property = prop;
> +

Same as with previous patch and the following ones... Would be
great to have this sit in amdgpu_dm_color.c.

Harry

>   return 0;
>  }
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 881446c51b36..6c165ad9bdf0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -352,6 +352,14 @@ struct amdgpu_mode_info {
>* drm_transfer_function`.
>*/
>   struct drm_property *regamma_tf_property;
> + /* @plane_degamma_lut_property: Plane property to set a degamma LUT to
> +  * convert color space before blending.
> +  */
> + struct drm_property *plane_degamma_lut_property;
> + /* @plane_degamma_lut_size_property: Plane property to define the max
> +  * size of degamma LUT as supported by the driver (read-only).
> +  */
> + struct drm_property *plane_degamma_lut_size_property;
>  };
>  
>  #define AMDGPU_MAX_BL_LEVEL 0xFF
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index ad5ee28b83dc..22e126654767 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -716,6 +716,15 @@ enum drm_transfer_function {
>  struct dm_plane_state {
>   struct drm_plane_state base;
>   struct dc_plane_state *dc_state;
> +
> + /* Plane color mgmt */
> + /**
> +  * @degamma_lut:
> +  *
> +  * LUT for converting plane pixel data before going into plane merger.
> +  * The blob (if not NULL) is an array of &struct drm_color_lut.
> +  */
> + struct drm_property_blob *degamma_lut;
>  };
>  
>  struct dm_crtc_state {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 322668973747..e9cedc4068f1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1338,6 +1338,9 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane)
>   dc_plane_state_retain(dm_plane_state->dc_state);
>   }
>  
> + if (dm_plane_state->degamma_lut)
> + drm_property_blob_get(dm_plane_state->degamma_lut);
> +
>   return &dm_plane_state->base;
>  }
>  
> @@ -1405,12 +1408,79 @@ static void dm_drm_plane_destroy_state(struct 
> drm_plane *plane,
>  {
>   struct dm_plane_state *dm_plane_state = to_dm_plane_state(state);
>  
> + if (dm_plane_state->degamma_lut)
> + drm_property_blob_put(dm_plane_state->degamma_lut);
> +
>   if (dm_plane_state->dc_state)
>   dc_plane_state_release(dm_plane_state->dc_state);
>  
>   drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> +#ifdef AMD_PRIVATE_COLOR
> +static void
> +dm_atomic_plane_attach_color_mgmt_properties(struct amdgpu_display_manager 
> *dm,
> +  struct drm_plane *plane)
> +{
> + if (dm->dc->caps.color.dpp.dgam_ram || 
> dm->dc->caps.color.dpp.gamma_corr ) {
> + drm_object_attach_property(&plan

[PATCH 0/4] Page table fence

[Philip Yang]

This patch series to fix GPU generate random no-retry fault on APU with
XNACK on.

If updating GPU page table to use PDE0 as PTE, for example unmap 2MB
align virtual address, then map same virtual address using transparent
2MB huge page, we free the PTE BO first and then flush TLB.

If XNACK ON, H/W may access the freed old PTE page before TLB is flushed.
On APU, the freed PTE BO system memory page maybe used and the content
is changed, this causes H/W enerates unexpected no-retry fault.

The fix is to add fence to the freed page table BO, and then signal the
fence after TLB is flushed to really free the page table BO page.

Philip Yang (4):
  drm/amdgpu: Implement page table BO fence
  drm/amdkfd: Signal page table fence after KFD flush tlb
  drm/amdgpu: Signal page table fence after gfx vm flush
  drm/amdgpu: Add fence to the freed page table BOs

 drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 45 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c|  7 
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  4 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h|  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 33 +++--
 drivers/gpu/drm/amd/amdkfd/kfd_process.c  |  5 +++
 7 files changed, 86 insertions(+), 11 deletions(-)

-- 
2.35.1

[PATCH 1/4] drm/amdgpu: Implement page table BO fence

[Philip Yang]

Add pt_fence to amdgpu vm structure and implement helper functions. This
fence will be shared by all page table BOs of the same amdgpu vm.

Suggested-by: Felix Kuehling 
Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 45 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  4 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h|  1 +
 4 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 5af954abd5ba..09c116dfda31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1499,6 +1499,8 @@ int amdgpu_device_set_cg_state(struct amdgpu_device *adev,
 int amdgpu_device_set_pg_state(struct amdgpu_device *adev,
   enum amd_powergating_state state);
 
+struct dma_fence *amdgpu_pt_fence_create(void);
+
 static inline bool amdgpu_device_has_timeouts_enabled(struct amdgpu_device 
*adev)
 {
return amdgpu_gpu_recovery != 0 &&
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index c694b41f6461..d9bfb0af3a09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -57,6 +57,16 @@ struct amdgpu_fence {
ktime_t start_timestamp;
 };
 
+/*
+ * Page table BO fence
+ * Fence used to ensure page table BOs are freed after TLB is flushed, to avoid
+ * H/W access corrupted data if the freed BO page is reused.
+ */
+struct amdgpu_pt_fence {
+   struct dma_fence base;
+   spinlock_t lock;
+};
+
 static struct kmem_cache *amdgpu_fence_slab;
 
 int amdgpu_fence_slab_init(void)
@@ -79,6 +89,7 @@ void amdgpu_fence_slab_fini(void)
  */
 static const struct dma_fence_ops amdgpu_fence_ops;
 static const struct dma_fence_ops amdgpu_job_fence_ops;
+static const struct dma_fence_ops amdgpu_pt_fence_ops;
 static inline struct amdgpu_fence *to_amdgpu_fence(struct dma_fence *f)
 {
struct amdgpu_fence *__f = container_of(f, struct amdgpu_fence, base);
@@ -852,6 +863,40 @@ static const struct dma_fence_ops amdgpu_job_fence_ops = {
.release = amdgpu_job_fence_release,
 };
 
+static atomic_t pt_fence_seq = ATOMIC_INIT(0);
+
+struct dma_fence *amdgpu_pt_fence_create(void)
+{
+   struct amdgpu_pt_fence *fence;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return NULL;
+
+   spin_lock_init(&fence->lock);
+   dma_fence_init(&fence->base, &amdgpu_pt_fence_ops, &fence->lock,
+  dma_fence_context_alloc(1), 
atomic_inc_return(&pt_fence_seq));
+
+   dma_fence_get(&fence->base);
+   return &fence->base;
+}
+
+static const char *amdgpu_pt_fence_get_timeline_name(struct dma_fence *f)
+{
+   return "pt_fence_timeline";
+}
+
+static void amdgpu_pt_fence_release(struct dma_fence *f)
+{
+   kfree_rcu(f, rcu);
+}
+
+static const struct dma_fence_ops amdgpu_pt_fence_ops = {
+   .get_driver_name = amdgpu_fence_get_driver_name,
+   .get_timeline_name = amdgpu_pt_fence_get_timeline_name,
+   .release = amdgpu_pt_fence_release,
+};
+
 /*
  * Fence debugfs
  */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 37b9d8a8dbec..0219398e625c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2147,6 +2147,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct 
amdgpu_vm *vm)
vm->update_funcs = &amdgpu_vm_sdma_funcs;
 
vm->last_update = dma_fence_get_stub();
+   vm->pt_fence = amdgpu_pt_fence_create();
vm->last_unlocked = dma_fence_get_stub();
vm->last_tlb_flush = dma_fence_get_stub();
vm->generation = 0;
@@ -2270,6 +2271,8 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm)
 
dma_fence_put(vm->last_update);
vm->last_update = dma_fence_get_stub();
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();
vm->is_compute_context = true;
 
/* Free the shadow bo for compute VM */
@@ -2358,6 +2361,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct 
amdgpu_vm *vm)
}
 
dma_fence_put(vm->last_update);
+   dma_fence_put(vm->pt_fence);
for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
amdgpu_vmid_free_reserved(adev, vm, i);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d551fca1780e..9cc729358612 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -286,6 +286,7 @@ struct amdgpu_vm {
/* contains the page directory */
struct amdgpu_vm_bo_base root;
struct dma_fence*last_update;
+   struct dma_fence*pt_fence;
 
/* Scheduler entities for page table updates */

[PATCH 2/4] drm/amdkfd: Signal page table fence after KFD flush tlb

[Philip Yang]

To free page table BOs which are freed when updating page table, for
example PTE BOs when PDE0 used as PTE.

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

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index af0a4b5257cc..0ff007a74d03 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -2101,6 +2101,11 @@ void kfd_flush_tlb(struct kfd_process_device *pdd, enum 
TLB_FLUSH_TYPE type)
amdgpu_amdkfd_flush_gpu_tlb_pasid(
dev->adev, pdd->process->pasid, type, xcc);
}
+
+   /* Signal page table fence to free page table BOs */
+   dma_fence_signal(vm->pt_fence);
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();
 }
 
 struct kfd_process_device *kfd_process_device_data_by_id(struct kfd_process 
*p, uint32_t gpu_id)
-- 
2.35.1

[PATCH 3/4] drm/amdgpu: Signal page table fence after gfx vm flush

[Philip Yang]

To free page table BOs which are fenced and freed when updating page
table.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index e0d3e3aa2e31..10d63256d26b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -219,6 +219,13 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned 
num_ibs,
amdgpu_ring_undo(ring);
return r;
}
+
+   if (vm) {
+   /* Signal fence to free page table BO */
+   dma_fence_signal(vm->pt_fence);
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();
+   }
}
 
amdgpu_ring_ib_begin(ring);
-- 
2.35.1

[PATCH 4/4] drm/amdgpu: Add fence to the freed page table BOs

[Philip Yang]

If updating page table free the page table BOs, add fence to the BOs to
ensure the page table BOs are not freed and reused before TLB is
flushed.

Signed-off-by: Philip Yang 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 33 +++
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index dea1a64be44d..16eb9472d469 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -633,8 +633,10 @@ static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev,
  * amdgpu_vm_pt_free - free one PD/PT
  *
  * @entry: PDE to free
+ * @fence: fence added the freed page table BO
  */
-static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
+static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry,
+ struct dma_fence *fence)
 {
struct amdgpu_bo *shadow;
 
@@ -643,6 +645,9 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base 
*entry)
shadow = amdgpu_bo_shadowed(entry->bo);
if (shadow) {
ttm_bo_set_bulk_move(&shadow->tbo, NULL);
+   if (fence && !dma_resv_reserve_fences(shadow->tbo.base.resv, 1))
+   dma_resv_add_fence(shadow->tbo.base.resv, fence,
+  DMA_RESV_USAGE_BOOKKEEP);
amdgpu_bo_unref(&shadow);
}
ttm_bo_set_bulk_move(&entry->bo->tbo, NULL);
@@ -651,6 +656,9 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base 
*entry)
spin_lock(&entry->vm->status_lock);
list_del(&entry->vm_status);
spin_unlock(&entry->vm->status_lock);
+   if (fence && !dma_resv_reserve_fences(entry->bo->tbo.base.resv, 1))
+   dma_resv_add_fence(entry->bo->tbo.base.resv, fence,
+  DMA_RESV_USAGE_BOOKKEEP);
amdgpu_bo_unref(&entry->bo);
 }
 
@@ -670,7 +678,7 @@ void amdgpu_vm_pt_free_work(struct work_struct *work)
amdgpu_bo_reserve(vm->root.bo, true);
 
list_for_each_entry_safe(entry, next, &pt_freed, vm_status)
-   amdgpu_vm_pt_free(entry);
+   amdgpu_vm_pt_free(entry, NULL);
 
amdgpu_bo_unreserve(vm->root.bo);
 }
@@ -682,13 +690,15 @@ void amdgpu_vm_pt_free_work(struct work_struct *work)
  * @vm: amdgpu vm structure
  * @start: optional cursor where to start freeing PDs/PTs
  * @unlocked: vm resv unlock status
+ * @fence: page table fence added to the freed BOs
  *
  * Free the page directory or page table level and all sub levels.
  */
 static void amdgpu_vm_pt_free_dfs(struct amdgpu_device *adev,
  struct amdgpu_vm *vm,
  struct amdgpu_vm_pt_cursor *start,
- bool unlocked)
+ bool unlocked,
+ struct dma_fence *fence)
 {
struct amdgpu_vm_pt_cursor cursor;
struct amdgpu_vm_bo_base *entry;
@@ -706,10 +716,10 @@ static void amdgpu_vm_pt_free_dfs(struct amdgpu_device 
*adev,
}
 
for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-   amdgpu_vm_pt_free(entry);
+   amdgpu_vm_pt_free(entry, fence);
 
if (start)
-   amdgpu_vm_pt_free(start->entry);
+   amdgpu_vm_pt_free(start->entry, fence);
 }
 
 /**
@@ -721,7 +731,7 @@ static void amdgpu_vm_pt_free_dfs(struct amdgpu_device 
*adev,
  */
 void amdgpu_vm_pt_free_root(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
-   amdgpu_vm_pt_free_dfs(adev, vm, NULL, false);
+   amdgpu_vm_pt_free_dfs(adev, vm, NULL, false, NULL);
 }
 
 /**
@@ -905,6 +915,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params 
*params,
struct amdgpu_device *adev = params->adev;
struct amdgpu_vm_pt_cursor cursor;
uint64_t frag_start = start, frag_end;
+   struct amdgpu_vm *vm = params->vm;
unsigned int frag;
int r;
 
@@ -913,7 +924,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params 
*params,
   &frag_end);
 
/* walk over the address space and update the PTs */
-   amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
+   amdgpu_vm_pt_start(adev, vm, start, &cursor);
while (cursor.pfn < end) {
unsigned int shift, parent_shift, mask;
uint64_t incr, entry_end, pe_start;
@@ -923,7 +934,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params 
*params,
/* make sure that the page tables covering the
 * address range are actually allocated
 */
-   r = amdgpu_vm_pt_alloc(params->adev, params->vm,
+   r = amdgpu_vm_pt_alloc(params->adev, vm,
   &cursor, params->immediate);
if (r)
 

Re: [PATCH 09/36] drm/amd/display: add plane HDR multiplier driver-specific property

[Harry Wentland]

On 5/23/23 18:14, Melissa Wen wrote:
> From: Joshua Ashton 
> 
> Multiplier to 'gain' the plane. When PQ is decoded using the fixed func
> transfer function to the internal FP16 fb, 1.0 -> 80 nits (on AMD at
> least) When sRGB is decoded, 1.0 -> 1.0.  Therefore, 1.0 multiplier = 80
> nits for SDR content. So if you want, 203 nits for SDR content, pass in
> (203.0 / 80.0).
> 
> Signed-off-by: Joshua Ashton 
> Co-developed-by: Melissa Wen 
> Signed-off-by: Melissa Wen 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c |  6 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h|  4 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h   | 12 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 13 +
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index fd6c4078c53a..f0e12cca295d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1298,6 +1298,12 @@ amdgpu_display_create_color_properties(struct 
> amdgpu_device *adev)
>   return -ENOMEM;
>   adev->mode_info.plane_degamma_tf_property = prop;
>  
> + prop = drm_property_create_range(adev_to_drm(adev),
> +  0, "AMD_PLANE_HDR_MULT", 0, U64_MAX);
> + if (!prop)
> + return -ENOMEM;
> + adev->mode_info.plane_hdr_mult_property = prop;
> +
>   return 0;
>  }
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 9d7f47fe6303..c105f51b7b6d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -365,6 +365,10 @@ struct amdgpu_mode_info {
>* linearize content with or without LUT.
>*/
>   struct drm_property *plane_degamma_tf_property;
> + /**
> +  * @plane_hdr_mult_property:
> +  */
> + struct drm_property *plane_hdr_mult_property;
>  };
>  
>  #define AMDGPU_MAX_BL_LEVEL 0xFF
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index b8e432cc8078..dadbef561606 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -51,6 +51,7 @@
>  
>  #define AMDGPU_DMUB_NOTIFICATION_MAX 5
>  
> +#define AMDGPU_HDR_MULT_DEFAULT (0x1LL)
>  /*
>  #include "include/amdgpu_dal_power_if.h"
>  #include "amdgpu_dm_irq.h"
> @@ -732,6 +733,17 @@ struct dm_plane_state {
>* linearize.
>*/
>   enum drm_transfer_function degamma_tf;
> + /**
> +  * @hdr_mult:
> +  *
> +  * Multiplier to 'gain' the plane.  When PQ is decoded using the fixed
> +  * func transfer function to the internal FP16 fb, 1.0 -> 80 nits (on
> +  * AMD at least). When sRGB is decoded, 1.0 -> 1.0, obviously.
> +  * Therefore, 1.0 multiplier = 80 nits for SDR content.  So if you
> +  * want, 203 nits for SDR content, pass in (203.0 / 80.0).  Format is
> +  * S31.32 sign-magnitude.
> +  */

Good explanation.

Harry

> + __u64 hdr_mult;
>  };
>  
>  struct dm_crtc_state {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 6b71777a525c..bbbf25dd2515 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1322,6 +1322,7 @@ static void dm_drm_plane_reset(struct drm_plane *plane)
>  
>   __drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
>   amdgpu_state->degamma_tf = DRM_TRANSFER_FUNCTION_DEFAULT;
> + amdgpu_state->hdr_mult = AMDGPU_HDR_MULT_DEFAULT;
>  }
>  
>  static struct drm_plane_state *
> @@ -1345,6 +1346,7 @@ dm_drm_plane_duplicate_state(struct drm_plane *plane)
>   drm_property_blob_get(dm_plane_state->degamma_lut);
>  
>   dm_plane_state->degamma_tf = old_dm_plane_state->degamma_tf;
> + dm_plane_state->hdr_mult = old_dm_plane_state->hdr_mult;
>  
>   return &dm_plane_state->base;
>  }
> @@ -1450,6 +1452,10 @@ dm_atomic_plane_attach_color_mgmt_properties(struct 
> amdgpu_display_manager *dm,
>  
> dm->adev->mode_info.plane_degamma_tf_property,
>  DRM_TRANSFER_FUNCTION_DEFAULT);
>   }
> + /* HDR MULT is always available */
> + drm_object_attach_property(&plane->base,
> +dm->adev->mode_info.plane_hdr_mult_property,
> +AMDGPU_HDR_MULT_DEFAULT);
>  }
>  
>  static int
> @@ -1476,6 +1482,11 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
>   dm_plane_state->degamma_tf = val;
>   dm_plane_state->base.color_mgmt_changed = 1;
>   }
> + } else if

Re: [PATCH -next] drm/amdkfd: clean up one inconsistent indenting

[Felix Kuehling]

On 2023-05-30 22:08, Yang Li wrote:

drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device.c:1036 kgd2kfd_interrupt() 
warn: inconsistent indenting

Signed-off-by: Yang Li 


Reviewed-by: Felix Kuehling 

I'm applying the patch to amd-staging-drm-next. Thanks!



---
  drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 862a50f7b490..0398a8c52a44 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -1033,7 +1033,7 @@ void kgd2kfd_interrupt(struct kfd_dev *kfd, const void 
*ih_ring_entry)
is_patched ? patched_ihre : ih_ring_entry)) {
kfd_queue_work(node->ih_wq, &node->interrupt_work);
spin_unlock_irqrestore(&node->interrupt_lock, flags);
-   return;
+   return;
}
spin_unlock_irqrestore(&node->interrupt_lock, flags);
}

Re: [PATCH 1/4] drm/amdgpu: Implement page table BO fence

[Felix Kuehling]

On 2023-06-01 15:31, Philip Yang wrote:

Add pt_fence to amdgpu vm structure and implement helper functions. This
fence will be shared by all page table BOs of the same amdgpu vm.

Suggested-by: Felix Kuehling 
Signed-off-by: Philip Yang 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 45 +++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  4 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h|  1 +
  4 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 5af954abd5ba..09c116dfda31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1499,6 +1499,8 @@ int amdgpu_device_set_cg_state(struct amdgpu_device *adev,
  int amdgpu_device_set_pg_state(struct amdgpu_device *adev,
   enum amd_powergating_state state);
  
+struct dma_fence *amdgpu_pt_fence_create(void);

+
  static inline bool amdgpu_device_has_timeouts_enabled(struct amdgpu_device 
*adev)
  {
return amdgpu_gpu_recovery != 0 &&
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index c694b41f6461..d9bfb0af3a09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -57,6 +57,16 @@ struct amdgpu_fence {
ktime_t start_timestamp;
  };
  
+/*

+ * Page table BO fence
+ * Fence used to ensure page table BOs are freed after TLB is flushed, to avoid
+ * H/W access corrupted data if the freed BO page is reused.
+ */
+struct amdgpu_pt_fence {
+   struct dma_fence base;
+   spinlock_t lock;
+};
+
  static struct kmem_cache *amdgpu_fence_slab;
  
  int amdgpu_fence_slab_init(void)

@@ -79,6 +89,7 @@ void amdgpu_fence_slab_fini(void)
   */
  static const struct dma_fence_ops amdgpu_fence_ops;
  static const struct dma_fence_ops amdgpu_job_fence_ops;
+static const struct dma_fence_ops amdgpu_pt_fence_ops;
  static inline struct amdgpu_fence *to_amdgpu_fence(struct dma_fence *f)
  {
struct amdgpu_fence *__f = container_of(f, struct amdgpu_fence, base);
@@ -852,6 +863,40 @@ static const struct dma_fence_ops amdgpu_job_fence_ops = {
.release = amdgpu_job_fence_release,
  };
  
+static atomic_t pt_fence_seq = ATOMIC_INIT(0);

+
+struct dma_fence *amdgpu_pt_fence_create(void)
+{
+   struct amdgpu_pt_fence *fence;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return NULL;
+
+   spin_lock_init(&fence->lock);
+   dma_fence_init(&fence->base, &amdgpu_pt_fence_ops, &fence->lock,
+  dma_fence_context_alloc(1), 
atomic_inc_return(&pt_fence_seq));


Creating a new fence context per fence is probably wrong. I think we 
only need one PT fence context per GPU or per spatial partition, or 
maybe per VM.


Regards,
  Felix



+
+   dma_fence_get(&fence->base);
+   return &fence->base;
+}
+
+static const char *amdgpu_pt_fence_get_timeline_name(struct dma_fence *f)
+{
+   return "pt_fence_timeline";
+}
+
+static void amdgpu_pt_fence_release(struct dma_fence *f)
+{
+   kfree_rcu(f, rcu);
+}
+
+static const struct dma_fence_ops amdgpu_pt_fence_ops = {
+   .get_driver_name = amdgpu_fence_get_driver_name,
+   .get_timeline_name = amdgpu_pt_fence_get_timeline_name,
+   .release = amdgpu_pt_fence_release,
+};
+
  /*
   * Fence debugfs
   */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 37b9d8a8dbec..0219398e625c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2147,6 +2147,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct 
amdgpu_vm *vm)
vm->update_funcs = &amdgpu_vm_sdma_funcs;
  
  	vm->last_update = dma_fence_get_stub();

+   vm->pt_fence = amdgpu_pt_fence_create();
vm->last_unlocked = dma_fence_get_stub();
vm->last_tlb_flush = dma_fence_get_stub();
vm->generation = 0;
@@ -2270,6 +2271,8 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm)
  
  	dma_fence_put(vm->last_update);

vm->last_update = dma_fence_get_stub();
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();
vm->is_compute_context = true;
  
  	/* Free the shadow bo for compute VM */

@@ -2358,6 +2361,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct 
amdgpu_vm *vm)
}
  
  	dma_fence_put(vm->last_update);

+   dma_fence_put(vm->pt_fence);
for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
amdgpu_vmid_free_reserved(adev, vm, i);
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d551fca1780e..9cc729358612 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -286,6 +286,7 @@ struct amdgpu_vm {

Re: [PATCH 21/36] drm/amd/display: add CRTC 3D LUT support

[Harry Wentland]

On 5/23/23 18:15, Melissa Wen wrote:
> Wire up DC 3D LUT to DM CRTC color management (post-blending). On AMD
> display HW, we have to set a shaper LUT to delinearize or normalize the
> color space before applying a 3D LUT (since we have a reduced number of
> LUT entries). Therefore, we map DC shaper LUT to DM CRTC color mgmt in
> the next patch.
> 
> Signed-off-by: Melissa Wen 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   6 +
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  17 ++
>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 158 +-
>  3 files changed, 180 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 0be62fe436b0..a6dd982d7e77 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9976,6 +9976,12 @@ static int amdgpu_dm_atomic_check(struct drm_device 
> *dev,
>   goto fail;
>   }
>  
> + ret = amdgpu_dm_verify_lut3d_size(adev, new_crtc_state);
> + if (ret) {
> + drm_dbg_driver(dev, "amdgpu_dm_verify_lut_sizes() 
> failed\n");
> + goto fail;
> + }
> +
>   if (!new_crtc_state->enable)
>   continue;
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index e5f9db5a43f4..eebe12c353ad 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -797,6 +797,21 @@ struct dm_crtc_state {
>  
>   int abm_level;
>  
> + /* AMD driver-private CRTC color management
> +  *
> +  * DRM provides CRTC degamma/ctm/gamma color mgmt features, but AMD HW
> +  * has a larger set of post-blending color calibration. Here, DC MPC
> +  * color caps are wired up to DM CRTC state:
> +  */
> + /**
> +  * @lut3d:
> +  *
> +  * Post-blending 3D Lookup table for converting pixel data. When
> +  * supported by HW (DCN 3+), it is positioned just before post-blending
> +  * regamma and always assumes a preceding shaper LUT. The blob (if not
> +  * NULL) is an array of &struct drm_color_lut.
> +  */
> + struct drm_property_blob *lut3d;
>  /**
>* @regamma_tf:
>*
> @@ -868,6 +883,8 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device 
> *dev);
>  /* 3D LUT max size is 17x17x17 */
>  #define MAX_COLOR_3DLUT_ENTRIES 4913
>  #define MAX_COLOR_3DLUT_BITDEPTH 12
> +int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
> + const struct drm_crtc_state *crtc_state);
>  /* 1D LUT size */
>  #define MAX_COLOR_LUT_ENTRIES 4096
>  /* Legacy gamm LUT users such as X doesn't like large LUT sizes */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 161807e19886..cef8d0d7f37b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -364,6 +364,96 @@ static int __set_input_tf(struct dc_transfer_func *func,
>   return res ? 0 : -ENOMEM;
>  }
>  
> +static void __to_dc_lut3d_color(struct dc_rgb *rgb,
> + const struct drm_color_lut lut,
> + int bit_precision)
> +{
> + rgb->red = drm_color_lut_extract(lut.red, bit_precision);
> + rgb->green = drm_color_lut_extract(lut.green, bit_precision);
> + rgb->blue  = drm_color_lut_extract(lut.blue, bit_precision);
> +}
> +
> +static void __drm_3dlut_to_dc_3dlut(const struct drm_color_lut *lut,
> + uint32_t lut3d_size,
> + struct tetrahedral_params *params,
> + bool use_tetrahedral_9,
> + int bit_depth)
> +{
> + struct dc_rgb *lut0;
> + struct dc_rgb *lut1;
> + struct dc_rgb *lut2;
> + struct dc_rgb *lut3;
> + int lut_i, i;
> +
> +
> + if (use_tetrahedral_9) {
> + lut0 = params->tetrahedral_9.lut0;
> + lut1 = params->tetrahedral_9.lut1;
> + lut2 = params->tetrahedral_9.lut2;
> + lut3 = params->tetrahedral_9.lut3;
> + } else {
> + lut0 = params->tetrahedral_17.lut0;
> + lut1 = params->tetrahedral_17.lut1;
> + lut2 = params->tetrahedral_17.lut2;
> + lut3 = params->tetrahedral_17.lut3;
> + }
> +
> + for (lut_i = 0, i = 0; i < lut3d_size - 4; lut_i++, i += 4) {
> + /* We should consider the 3dlut RGB values are distributed
> +  * along four arrays lut0-3 where the first sizes 1229 and the
> +  * other 1228. The bit depth supported for 3dlut channel is
> +  * 12-bi

Re: [PATCH 2/4] drm/amdkfd: Signal page table fence after KFD flush tlb

[Felix Kuehling]

On 2023-06-01 15:31, Philip Yang wrote:

To free page table BOs which are freed when updating page table, for
example PTE BOs when PDE0 used as PTE.

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

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index af0a4b5257cc..0ff007a74d03 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -2101,6 +2101,11 @@ void kfd_flush_tlb(struct kfd_process_device *pdd, enum 
TLB_FLUSH_TYPE type)
amdgpu_amdkfd_flush_gpu_tlb_pasid(
dev->adev, pdd->process->pasid, type, xcc);
}
+
+   /* Signal page table fence to free page table BOs */
+   dma_fence_signal(vm->pt_fence);
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();


This is a bit weird to create a fence here when we have no idea when it 
will be signaled or whether it will be signaled at all. But I think it's 
OK in this case. You add this fence to PT BOs before they get freed, and 
at that point those BOs must wait for the next TLB flush before the BO 
can be freed.


The only important thing is that fences in the same context get signaled 
in the same order they are created. I think if you allocate a fence 
context per VM, that should be guaranteed by the VM root reservation 
lock that serializes all page table operations in the same VM.


You may need some locking here to prevent concurrent access to the fence 
while you're signaling and replacing it.


Regards,
  Felix



  }
  
  struct kfd_process_device *kfd_process_device_data_by_id(struct kfd_process *p, uint32_t gpu_id)

Re: [PATCH 3/4] drm/amdgpu: Signal page table fence after gfx vm flush

[Felix Kuehling]

On 2023-06-01 15:31, Philip Yang wrote:

To free page table BOs which are fenced and freed when updating page
table.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index e0d3e3aa2e31..10d63256d26b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -219,6 +219,13 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned 
num_ibs,
amdgpu_ring_undo(ring);
return r;
}
+
+   if (vm) {
+   /* Signal fence to free page table BO */
+   dma_fence_signal(vm->pt_fence);
+   dma_fence_put(vm->pt_fence);
+   vm->pt_fence = amdgpu_pt_fence_create();
+   }


I think this is too early. The TLB flush is not done at this point, it's 
only been emitted to the ring but not executed yet. You probably need to 
signal the PT fence in a fence callback from the fence "f" that signals 
when the IB completes.


Regards,
  Felix



}
  
  	amdgpu_ring_ib_begin(ring);

[PATCH 1/3] drm/amdkfd: add event age tracking

[James Zhu]

Add event age tracking

Signed-off-by: James Zhu 
---
 include/uapi/linux/kfd_ioctl.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 7e19a2d1e907..bfbe0006370e 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -38,9 +38,10 @@
  * - 1.10 - Add SMI profiler event log
  * - 1.11 - Add unified memory for ctx save/restore area
  * - 1.12 - Add DMA buf export ioctl
+ * - 1.13 - Update kfd_event_data
  */
 #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 12
+#define KFD_IOCTL_MINOR_VERSION 13
 
 /*
  * Debug revision change log
@@ -693,6 +694,7 @@ struct kfd_event_data {
union {
struct kfd_hsa_memory_exception_data memory_exception_data;
struct kfd_hsa_hw_exception_data hw_exception_data;
+   __u64 last_event_age;   /* to and from KFD */
};  /* From KFD */
__u64 kfd_event_data_ext;   /* pointer to an extension structure
   for future exception types */
-- 
2.34.1

[PATCH 2/3] drm/amdkfd: add event_age tracking when receiving interrupt

[James Zhu]

Add event_age tracking when receiving interrupt.

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 6 ++
 drivers/gpu/drm/amd/amdkfd/kfd_events.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index d23202613ed7..b27a79c5f826 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -439,6 +439,7 @@ int kfd_event_create(struct file *devkfd, struct 
kfd_process *p,
if (!ret) {
*event_id = ev->event_id;
*event_trigger_data = ev->event_id;
+   ev->event_age = 1;
} else {
kfree(ev);
}
@@ -637,6 +638,11 @@ static void set_event(struct kfd_event *ev)
 * updating the wait queues in kfd_wait_on_events.
 */
ev->signaled = !ev->auto_reset || !waitqueue_active(&ev->wq);
+   if (!(++ev->event_age)) {
+   /* Never wrap back to reserved/default event age 0/1 */
+   ev->event_age = 2;
+   WARN_ONCE(1, "event_age wrap back!");
+   }
 
 #if !defined(HAVE_WAIT_QUEUE_ENTRY)
list_for_each_entry(waiter, &ev->wq.task_list, wait.task_list)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
index 1c62c8dd6460..52ccfd397c2b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
@@ -53,6 +53,7 @@ struct signal_page;
 
 struct kfd_event {
u32 event_id;
+   u64 event_age;
 
bool signaled;
bool auto_reset;
-- 
2.34.1

[PATCH 3/3] drm/amdkfd: don't sleep when event age unmatch

[James Zhu]

Don't sleep when event age unmatch, and update last_event_age.
It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index b27a79c5f826..23e154811471 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -964,6 +964,19 @@ int kfd_wait_on_events(struct kfd_process *p,
event_data.event_id);
if (ret)
goto out_unlock;
+
+   /* last_event_age = 0 reserved for backward compatible */
+   if (event_data.last_event_age &&
+   event_waiters[i].event->event_age != 
event_data.last_event_age) {
+   event_data.last_event_age = 
event_waiters[i].event->event_age;
+   WRITE_ONCE(event_waiters[i].activated, true);
+
+   if (copy_to_user(&events[i], &event_data,
+   sizeof(struct kfd_event_data))) {
+   ret = -EFAULT;
+   goto out_unlock;
+   }
+   }
}
 
/* Check condition once. */
-- 
2.34.1

Re: [PATCH 1/3] drm/amdkfd: add event age tracking

[Felix Kuehling]

On 2023-06-01 16:47, James Zhu wrote:

Add event age tracking

Signed-off-by: James Zhu 
---
  include/uapi/linux/kfd_ioctl.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 7e19a2d1e907..bfbe0006370e 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -38,9 +38,10 @@
   * - 1.10 - Add SMI profiler event log
   * - 1.11 - Add unified memory for ctx save/restore area
   * - 1.12 - Add DMA buf export ioctl
+ * - 1.13 - Update kfd_event_data
   */
  #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 12
+#define KFD_IOCTL_MINOR_VERSION 13


I think minor version 13 is used for the debugger changes that are 
making their way into the upstream branch right now. You'll probably 
have to rebase this and use minor version 14 for this.



  
  /*

   * Debug revision change log
@@ -693,6 +694,7 @@ struct kfd_event_data {
union {
struct kfd_hsa_memory_exception_data memory_exception_data;
struct kfd_hsa_hw_exception_data hw_exception_data;
+   __u64 last_event_age;   /* to and from KFD */


It would be better to create a new struct kfd_hsa_signal_event_data for 
this to make it obvious what type of event it applies to. And to keep 
the union more uniform.


Regards,
  Felix



};  /* From KFD */
__u64 kfd_event_data_ext;   /* pointer to an extension structure
   for future exception types */

Re: [PATCH 1/3] drm/amdkfd: add event age tracking

[Felix Kuehling]

We'll also need a pointer to the user mode changes in some public repo, 
or a public email code review of the user mode changes.


Thanks,
  Felix


On 2023-06-01 16:58, Felix Kuehling wrote:

On 2023-06-01 16:47, James Zhu wrote:

Add event age tracking

Signed-off-by: James Zhu 
---
  include/uapi/linux/kfd_ioctl.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h 
b/include/uapi/linux/kfd_ioctl.h

index 7e19a2d1e907..bfbe0006370e 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -38,9 +38,10 @@
   * - 1.10 - Add SMI profiler event log
   * - 1.11 - Add unified memory for ctx save/restore area
   * - 1.12 - Add DMA buf export ioctl
+ * - 1.13 - Update kfd_event_data
   */
  #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 12
+#define KFD_IOCTL_MINOR_VERSION 13


I think minor version 13 is used for the debugger changes that are 
making their way into the upstream branch right now. You'll probably 
have to rebase this and use minor version 14 for this.




    /*
   * Debug revision change log
@@ -693,6 +694,7 @@ struct kfd_event_data {
  union {
  struct kfd_hsa_memory_exception_data memory_exception_data;
  struct kfd_hsa_hw_exception_data hw_exception_data;
+    __u64 last_event_age;    /* to and from KFD */


It would be better to create a new struct kfd_hsa_signal_event_data 
for this to make it obvious what type of event it applies to. And to 
keep the union more uniform.


Regards,
  Felix



  };    /* From KFD */
  __u64 kfd_event_data_ext;    /* pointer to an extension structure
 for future exception types */

Re: [PATCH 1/3] drm/amdkfd: add event age tracking

[James Zhu]

On 2023-06-01 16:58, Felix Kuehling wrote:

On 2023-06-01 16:47, James Zhu wrote:

Add event age tracking

Signed-off-by: James Zhu 
---
  include/uapi/linux/kfd_ioctl.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h 
b/include/uapi/linux/kfd_ioctl.h

index 7e19a2d1e907..bfbe0006370e 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -38,9 +38,10 @@
   * - 1.10 - Add SMI profiler event log
   * - 1.11 - Add unified memory for ctx save/restore area
   * - 1.12 - Add DMA buf export ioctl
+ * - 1.13 - Update kfd_event_data
   */
  #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 12
+#define KFD_IOCTL_MINOR_VERSION 13


I think minor version 13 is used for the debugger changes that are 
making their way into the upstream branch right now. You'll probably 
have to rebase this and use minor version 14 for this.

[JZ] Sure, I will change to version 14.




    /*
   * Debug revision change log
@@ -693,6 +694,7 @@ struct kfd_event_data {
  union {
  struct kfd_hsa_memory_exception_data memory_exception_data;
  struct kfd_hsa_hw_exception_data hw_exception_data;
+    __u64 last_event_age;    /* to and from KFD */


It would be better to create a new struct kfd_hsa_signal_event_data 
for this to make it obvious what type of event it applies to. And to 
keep the union more uniform.

[JZ]Sure. I will update it.


Regards,
  Felix



  };    /* From KFD */
  __u64 kfd_event_data_ext;    /* pointer to an extension structure
 for future exception types */

Re: [PATCH 3/3] drm/amdkfd: don't sleep when event age unmatch

[Felix Kuehling]

On 2023-06-01 16:47, James Zhu wrote:

Don't sleep when event age unmatch, and update last_event_age.
It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index b27a79c5f826..23e154811471 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -964,6 +964,19 @@ int kfd_wait_on_events(struct kfd_process *p,
event_data.event_id);
if (ret)
goto out_unlock;
+
+   /* last_event_age = 0 reserved for backward compatible */
+   if (event_data.last_event_age &&
+   event_waiters[i].event->event_age != 
event_data.last_event_age) {
+   event_data.last_event_age = 
event_waiters[i].event->event_age;
+   WRITE_ONCE(event_waiters[i].activated, true);


I think this is probably not necessary if you're returning before the 
first call to test_event_condition.




+
+   if (copy_to_user(&events[i], &event_data,
+   sizeof(struct kfd_event_data))) {
+   ret = -EFAULT;
+   goto out_unlock;
+   }
+   }


When waiting on multiple events, it would be more efficient to catch all 
events with outdated age in a single system call, instead of returning 
after finding the first one. Then return after the loop if it found one 
or more outdated events.


Also EFAULT is the wrong error code. It means "bad address". I think we 
could return 0 here because there is really no error. It's just a normal 
condition to allow user mode to update its event information before 
going to sleep. If you want a non-0 return code, I'd recommend -EDEADLK, 
because sleeping without outdated event information can lead to 
deadlocks. We'd also need to translate that into a meaningful status 
code in the Thunk to let ROCr know what's going on. I don't see a 
suitable status code in the current Thunk API for this.


Regards,
  Felix



}
  
  	/* Check condition once. */

[PATCH] drm/amdgpu: fix xclk freq on CHIP_STONEY

[Chia-I Wu]

According to Alex, most APUs from that time seem to have the same issue
(vbios says 48Mhz, actual is 100Mhz).  I only have a CHIP_STONEY so I
limit the fixup to CHIP_STONEY
---
 drivers/gpu/drm/amd/amdgpu/vi.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 770f2d7a371fc..6a8494f98d3ef 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -542,8 +542,15 @@ static u32 vi_get_xclk(struct amdgpu_device *adev)
u32 reference_clock = adev->clock.spll.reference_freq;
u32 tmp;
 
-   if (adev->flags & AMD_IS_APU)
-   return reference_clock;
+   if (adev->flags & AMD_IS_APU) {
+   switch (adev->asic_type) {
+   case CHIP_STONEY:
+   /* vbios says 48Mhz, but the actual freq is 100Mhz */
+   return 1;
+   default:
+   return reference_clock;
+   }
+   }
 
tmp = RREG32_SMC(ixCG_CLKPIN_CNTL_2);
if (REG_GET_FIELD(tmp, CG_CLKPIN_CNTL_2, MUX_TCLK_TO_XCLK))
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH 3/3] drm/amdkfd: don't sleep when event age unmatch

[James Zhu]

On 2023-06-01 17:17, Felix Kuehling wrote:

On 2023-06-01 16:47, James Zhu wrote:

Don't sleep when event age unmatch, and update last_event_age.
It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c

index b27a79c5f826..23e154811471 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -964,6 +964,19 @@ int kfd_wait_on_events(struct kfd_process *p,
  event_data.event_id);
  if (ret)
  goto out_unlock;
+
+    /* last_event_age = 0 reserved for backward compatible */
+    if (event_data.last_event_age &&
+    event_waiters[i].event->event_age != 
event_data.last_event_age) {
+    event_data.last_event_age = 
event_waiters[i].event->event_age;

+    WRITE_ONCE(event_waiters[i].activated, true);


I think this is probably not necessary if you're returning before the 
first call to test_event_condition.


[JZ] Currently, the returning is with test_event_condition. Since some 
conditions needs return with all events signaled.


so all waiters need check and update if any event interrupts are missing 
here(unmatched event age).






+
+    if (copy_to_user(&events[i], &event_data,
+    sizeof(struct kfd_event_data))) {
+    ret = -EFAULT;
+    goto out_unlock;
+    }
+    }


When waiting on multiple events, it would be more efficient to catch 
all events with outdated age in a single system call, instead of 
returning after finding the first one. Then return after the loop if 
it found one or more outdated events.
[JZ] Yes, the code is working in this way, when all events' waiters are 
activated, the following test_event_condition == 
KFD_IOC_WAIT_RESULT_COMPLETE, then return to user mode without sleep.



Also EFAULT is the wrong error code. It means "bad address". I think 
we could return 0 here because there is really no error. It's just a 
normal condition to allow user mode to update its event information 
before going to sleep. If you want a non-0 return code, I'd recommend 
-EDEADLK, because sleeping without outdated event information can lead 
to deadlocks. We'd also need to translate that into a meaningful 
status code in the Thunk to let ROCr know what's going on. I don't see 
a suitable status code in the current Thunk API for this.
[JZ] Basically, this failure is for copy_to_user. It will lead to busy 
wait instead of deadlock.

**
Regards,
  Felix



  }
    /* Check condition once. */

Re: [PATCH 3/3] drm/amdkfd: don't sleep when event age unmatch

[Felix Kuehling]

On 2023-06-01 18:06, James Zhu wrote:



On 2023-06-01 17:17, Felix Kuehling wrote:

On 2023-06-01 16:47, James Zhu wrote:

Don't sleep when event age unmatch, and update last_event_age.
It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_events.c

index b27a79c5f826..23e154811471 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -964,6 +964,19 @@ int kfd_wait_on_events(struct kfd_process *p,
  event_data.event_id);
  if (ret)
  goto out_unlock;
+
+    /* last_event_age = 0 reserved for backward compatible */
+    if (event_data.last_event_age &&
+    event_waiters[i].event->event_age != 
event_data.last_event_age) {
+    event_data.last_event_age = 
event_waiters[i].event->event_age;

+    WRITE_ONCE(event_waiters[i].activated, true);


I think this is probably not necessary if you're returning before the 
first call to test_event_condition.


[JZ] Currently, the returning is with test_event_condition. Since some 
conditions needs return with all events signaled.


so all waiters need check and update if any event interrupts are 
missing here(unmatched event age).






+
+    if (copy_to_user(&events[i], &event_data,
+    sizeof(struct kfd_event_data))) {
+    ret = -EFAULT;
+    goto out_unlock;
+    }
+    }


When waiting on multiple events, it would be more efficient to catch 
all events with outdated age in a single system call, instead of 
returning after finding the first one. Then return after the loop if 
it found one or more outdated events.
[JZ] Yes, the code is working in this way, when all events' waiters 
are activated, the following test_event_condition == 
KFD_IOC_WAIT_RESULT_COMPLETE, then return to user mode without sleep.



Also EFAULT is the wrong error code. It means "bad address". I think 
we could return 0 here because there is really no error. It's just a 
normal condition to allow user mode to update its event information 
before going to sleep. If you want a non-0 return code, I'd recommend 
-EDEADLK, because sleeping without outdated event information can 
lead to deadlocks. We'd also need to translate that into a meaningful 
status code in the Thunk to let ROCr know what's going on. I don't 
see a suitable status code in the current Thunk API for this.
[JZ] Basically, this failure is for copy_to_user. It will lead to busy 
wait instead of deadlock.


Oh, right, I missed that this is only for the case that copy_to_user 
fails. EFAULT is the correct error code for this. Then this patch looks 
good as is.


Regards,
  Felix



**
Regards,
  Felix



  }
    /* Check condition once. */

[PATCH v3] amdgpu: validate offset_in_bo of drm_amdgpu_gem_va

[Chia-I Wu]

This is motivated by OOB access in amdgpu_vm_update_range when
offset_in_bo+map_size overflows.

v2: keep the validations in amdgpu_vm_bo_map
v3: add the validations to amdgpu_vm_bo_map/amdgpu_vm_bo_replace_map
rather than to amdgpu_gem_va_ioctl

Fixes: 9f7eb5367d00 ("drm/amdgpu: actually use the VM map parameters")
Signed-off-by: Chia-I Wu 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 22f9a65ca0fc7..76d57bc7ac620 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1434,14 +1434,14 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
uint64_t eaddr;
 
/* validate the parameters */
-   if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK ||
-   size == 0 || size & ~PAGE_MASK)
+   if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK || size & ~PAGE_MASK)
+   return -EINVAL;
+   if (saddr + size <= saddr || offset + size <= offset)
return -EINVAL;
 
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
-   if (saddr >= eaddr ||
-   (bo && offset + size > amdgpu_bo_size(bo)) ||
+   if ((bo && offset + size > amdgpu_bo_size(bo)) ||
(eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
return -EINVAL;
 
@@ -1500,14 +1500,14 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
int r;
 
/* validate the parameters */
-   if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK ||
-   size == 0 || size & ~PAGE_MASK)
+   if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK || size & ~PAGE_MASK)
+   return -EINVAL;
+   if (saddr + size <= saddr || offset + size <= offset)
return -EINVAL;
 
/* make sure object fit at this offset */
eaddr = saddr + size - 1;
-   if (saddr >= eaddr ||
-   (bo && offset + size > amdgpu_bo_size(bo)) ||
+   if ((bo && offset + size > amdgpu_bo_size(bo)) ||
(eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
return -EINVAL;
 
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH] drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+

[Rodrigo Siqueira Jordao]

Hi Jay,

On 6/1/23 11:09, Aurabindo Pillai wrote:

Due to FPO, firmware will try to change OTG timings, which would only
latch if min/max selectors for vtotal are written by the driver.


Could you elaborate a little bit more about this issue? Right now, do we 
have some sort of race between firmware and driver? Is this situation 
causing some problems that we can reproduce? If so, could you also 
describe it?



Signed-off-by: Aurabindo Pillai 
---
  drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 15 +++
  drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c |  6 ++
  2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
index e1975991e075..633989fd2514 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
@@ -930,19 +930,10 @@ void optc1_set_drr(
OTG_FORCE_LOCK_ON_EVENT, 0,
OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
OTG_SET_V_TOTAL_MIN_MASK, 0);
-
-   // Setup manual flow control for EOF via TRIG_A
-   optc->funcs->setup_manual_trigger(optc);
-
-   } else {
-   REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
-   OTG_SET_V_TOTAL_MIN_MASK, 0,
-   OTG_V_TOTAL_MIN_SEL, 0,
-   OTG_V_TOTAL_MAX_SEL, 0,
-   OTG_FORCE_LOCK_ON_EVENT, 0);
-
-   optc->funcs->set_vtotal_min_max(optc, 0, 0);
}
+
+   // Setup manual flow control for EOF via TRIG_A
+   optc->funcs->setup_manual_trigger(optc);
  }
  
  void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, int vtotal_max)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
index e0edc163d767..042ce082965f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
@@ -455,6 +455,12 @@ void optc2_setup_manual_trigger(struct timing_generator 
*optc)
  {
struct optc *optc1 = DCN10TG_FROM_TG(optc);
  
+	REG_UPDATE_4(OTG_V_TOTAL_CONTROL,


Could you align the below registers right after the open parenthesis?


+OTG_V_TOTAL_MIN_SEL, 1,
+OTG_V_TOTAL_MAX_SEL, 1,
+OTG_FORCE_LOCK_ON_EVENT, 0,


Could you add a comment that describes why we want to set the above values?


+OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA 
*/


Why do you use (1 << 1)? Why not set the value directly here? Also, in 
the comment, I guess it should be TRIG_A.



+
REG_SET_8(OTG_TRIGA_CNTL, 0,
OTG_TRIGA_SOURCE_SELECT, 21,
OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,

[PATCH 1/1] drm/amdgpu/pm: notify driver unloading to PMFW for SMU v13.0.6 dGPU

[Le Ma]

Per requested, follow the same sequence as APU to send only
PPSMC_MSG_PrepareForDriverUnload to PMFW during driver unloading.

Change-Id: I2dc8495572b0bce6e21eafb51b215c83d94ac647
Signed-off-by: Le Ma 
Reviewed-by: Shiwu Zhang 
Reviewed-by: Lijo Lazar 
---
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 3da614faf75d..392ccebc8dac 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -1409,18 +1409,16 @@ static int smu_v13_0_6_system_features_control(struct 
smu_context *smu,
  bool enable)
 {
struct amdgpu_device *adev = smu->adev;
-   int ret;
-
-   /* On APUs, notify FW that the device is no longer driver managed */
-   if (adev->flags & AMD_IS_APU) {
-   if (!enable)
-   smu_v13_0_6_notify_unload(smu);
+   int ret = 0;
 
-   return 0;
+   if (enable) {
+   if (!(adev->flags & AMD_IS_APU))
+   ret = smu_v13_0_system_features_control(smu, enable);
+   } else {
+   /* Notify FW that the device is no longer driver managed */
+   smu_v13_0_6_notify_unload(smu);
}
 
-   ret = smu_v13_0_system_features_control(smu, enable);
-
return ret;
 }
 
-- 
2.38.1

RE: [PATCH 1/1] drm/amdgpu/pm: notify driver unloading to PMFW for SMU v13.0.6 dGPU

[Zhang, Hawking]

[AMD Official Use Only - General]

Reviewed-by: Hawking Zhang 

Regards,
Hawking
-Original Message-
From: amd-gfx  On Behalf Of Le Ma
Sent: Friday, June 2, 2023 10:42
To: amd-gfx@lists.freedesktop.org
Cc: Ma, Le ; Lazar, Lijo ; Zhang, Morris 

Subject: [PATCH 1/1] drm/amdgpu/pm: notify driver unloading to PMFW for SMU 
v13.0.6 dGPU

Per requested, follow the same sequence as APU to send only 
PPSMC_MSG_PrepareForDriverUnload to PMFW during driver unloading.

Change-Id: I2dc8495572b0bce6e21eafb51b215c83d94ac647
Signed-off-by: Le Ma 
Reviewed-by: Shiwu Zhang 
Reviewed-by: Lijo Lazar 
---
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 3da614faf75d..392ccebc8dac 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -1409,18 +1409,16 @@ static int smu_v13_0_6_system_features_control(struct 
smu_context *smu,
  bool enable)
 {
struct amdgpu_device *adev = smu->adev;
-   int ret;
-
-   /* On APUs, notify FW that the device is no longer driver managed */
-   if (adev->flags & AMD_IS_APU) {
-   if (!enable)
-   smu_v13_0_6_notify_unload(smu);
+   int ret = 0;

-   return 0;
+   if (enable) {
+   if (!(adev->flags & AMD_IS_APU))
+   ret = smu_v13_0_system_features_control(smu, enable);
+   } else {
+   /* Notify FW that the device is no longer driver managed */
+   smu_v13_0_6_notify_unload(smu);
}

-   ret = smu_v13_0_system_features_control(smu, enable);
-
return ret;
 }

--
2.38.1

[PATCH] drm/amd/pm: Fill metrics data for SMUv13.0.6

[Lijo Lazar]

Populate metrics data table for SMU v13.0.6. Add PCIe link speed/width
information also.

Signed-off-by: Lijo Lazar 
---
 .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  | 108 +++---
 1 file changed, 67 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 75255e0baf91..4ff5a66d446a 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -80,7 +80,10 @@
 /* possible frequency drift (1Mhz) */
 #define EPSILON 1
 
-#define smnPCIE_ESM_CTRL 0x111003D0
+#define smnPCIE_ESM_CTRL 0x193D0
+#define smnPCIE_LC_LINK_WIDTH_CNTL 0x1ab40288
+#define PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK 0x0070L
+#define PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT 0x4
 
 static const struct cmn2asic_msg_mapping 
smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
MSG_MAP(TestMessage, PPSMC_MSG_TestMessage, 
0),
@@ -197,6 +200,7 @@ struct PPTable_t {
 };
 
 #define SMUQ10_TO_UINT(x) ((x) >> 10)
+#define SMUQ16_TO_UINT(x) ((x) >> 16)
 
 struct smu_v13_0_6_dpm_map {
enum smu_clk_type clk_type;
@@ -1935,6 +1939,16 @@ static void 
smu_v13_0_6_log_thermal_throttling_event(struct smu_context *smu)
   smu_v13_0_6_throttler_map));
 }
 
+static int
+smu_v13_0_6_get_current_pcie_link_width_level(struct smu_context *smu)
+{
+   struct amdgpu_device *adev = smu->adev;
+
+   return (RREG32_PCIE(smnPCIE_LC_LINK_WIDTH_CNTL) &
+   PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK) >>
+  PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT;
+}
+
 static int smu_v13_0_6_get_current_pcie_link_speed(struct smu_context *smu)
 {
struct amdgpu_device *adev = smu->adev;
@@ -1953,8 +1967,12 @@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct 
smu_context *smu, void **table
struct smu_table_context *smu_table = &smu->smu_table;
struct gpu_metrics_v1_3 *gpu_metrics =
(struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
+   struct amdgpu_device *adev = smu->adev;
+   int ret = 0, inst0, xcc0;
MetricsTable_t *metrics;
-   int i, ret = 0;
+
+   inst0 = adev->sdma.instance[0].aid_id;
+   xcc0 = GET_INST(GC, 0);
 
metrics = kzalloc(sizeof(MetricsTable_t), GFP_KERNEL);
ret = smu_v13_0_6_get_metrics_table(smu, metrics, true);
@@ -1963,51 +1981,59 @@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct 
smu_context *smu, void **table
 
smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
 
-   /* TODO: Decide on how to fill in zero value fields */
-   gpu_metrics->temperature_edge = 0;
-   gpu_metrics->temperature_hotspot = 0;
-   gpu_metrics->temperature_mem = 0;
-   gpu_metrics->temperature_vrgfx = 0;
-   gpu_metrics->temperature_vrsoc = 0;
-   gpu_metrics->temperature_vrmem = 0;
-
-   gpu_metrics->average_gfx_activity = 0;
-   gpu_metrics->average_umc_activity = 0;
-   gpu_metrics->average_mm_activity = 0;
-
-   gpu_metrics->average_socket_power = 0;
-   gpu_metrics->energy_accumulator = 0;
-
-   gpu_metrics->average_gfxclk_frequency = 0;
-   gpu_metrics->average_socclk_frequency = 0;
-   gpu_metrics->average_uclk_frequency = 0;
-   gpu_metrics->average_vclk0_frequency = 0;
-   gpu_metrics->average_dclk0_frequency = 0;
-
-   gpu_metrics->current_gfxclk = 0;
-   gpu_metrics->current_socclk = 0;
-   gpu_metrics->current_uclk = 0;
-   gpu_metrics->current_vclk0 = 0;
-   gpu_metrics->current_dclk0 = 0;
-
+   gpu_metrics->temperature_hotspot =
+   SMUQ10_TO_UINT(metrics->MaxSocketTemperature);
+   /* Individual HBM stack temperature is not reported */
+   gpu_metrics->temperature_mem =
+   SMUQ10_TO_UINT(metrics->MaxHbmTemperature);
+   /* Reports max temperature of all voltage rails */
+   gpu_metrics->temperature_vrsoc =
+   SMUQ10_TO_UINT(metrics->MaxVrTemperature);
+
+   gpu_metrics->average_gfx_activity =
+   SMUQ10_TO_UINT(metrics->SocketGfxBusy);
+   gpu_metrics->average_umc_activity =
+   SMUQ10_TO_UINT(metrics->DramBandwidthUtilization);
+
+   gpu_metrics->average_socket_power =
+   SMUQ10_TO_UINT(metrics->SocketPower);
+   gpu_metrics->energy_accumulator =
+   SMUQ16_TO_UINT(metrics->SocketEnergyAcc);
+
+   gpu_metrics->current_gfxclk =
+   SMUQ10_TO_UINT(metrics->GfxclkFrequency[xcc0]);
+   gpu_metrics->current_socclk =
+   SMUQ10_TO_UINT(metrics->SocclkFrequency[inst0]);
+   gpu_metrics->current_uclk = SMUQ10_TO_UINT(metrics->UclkFrequency);
+   gpu_metrics->current_vclk0 =
+   SMUQ10_TO_UINT(metrics->VclkFrequency[inst0]);
+   gpu_metrics->current_dclk0 =
+   SMUQ10_TO_UINT

[PATCH v2] drm/amd/display: Avoid disabling GCC specific flag with clang for snprintf_count()

[Srinivasan Shanmugam]

These warning can cause build failure:

display/dc/dcn10/dcn10_hw_sequencer_debug.c: In function ‘snprintf_count’:
display/dc/dcn10/dcn10_hw_sequencer_debug.c:56:2: warning: function 
‘snprintf_count’ might be a candidate for ‘gnu_printf’ format attribute 
[-Wsuggest-attribute=format]

The warning being disabled by this pragma is GCC specific. Guard its use
with CONFIG_CC_IS_GCC so that it is not used with clang to clear up the
error.

Cc: Hamza Mahfooz 
Cc: Rodrigo Siqueira 
Cc: Harry Wentland 
Cc: Aurabindo Pillai 
Signed-off-by: Srinivasan Shanmugam 
---

v2:
 - Alternate Solution 2: for proposed
   https://patchwork.freedesktop.org/patch/540285/8, suspect that
   code is in DC, to see if it compiles even on windows also.

 .../gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer_debug.c   | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer_debug.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer_debug.c
index a0f8e31d2adc..e14b6747bbcc 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer_debug.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer_debug.c
@@ -45,6 +45,10 @@
 #include "dcn10_cm_common.h"
 #include "clk_mgr.h"
 
+#ifdef CONFIG_CC_IS_GCC
+#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
+#endif
+
 unsigned int snprintf_count(char *pBuf, unsigned int bufSize, char *fmt, ...)
 {
int ret_vsnprintf;
-- 
2.25.1

RE: [PATCH] drm/amd/pm: Fill metrics data for SMUv13.0.6

[Chen, Guchun]

[AMD Official Use Only - General]

> -Original Message-
> From: amd-gfx  On Behalf Of Lijo
> Lazar
> Sent: Friday, June 2, 2023 12:00 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander ; Zhang, Hawking
> 
> Subject: [PATCH] drm/amd/pm: Fill metrics data for SMUv13.0.6
>
> Populate metrics data table for SMU v13.0.6. Add PCIe link speed/width
> information also.
>
> Signed-off-by: Lijo Lazar 
> ---
>  .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  | 108 +++---
> 
>  1 file changed, 67 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> index 75255e0baf91..4ff5a66d446a 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> @@ -80,7 +80,10 @@
>  /* possible frequency drift (1Mhz) */
>  #define EPSILON 1
>
> -#define smnPCIE_ESM_CTRL 0x111003D0
> +#define smnPCIE_ESM_CTRL 0x193D0
> +#define smnPCIE_LC_LINK_WIDTH_CNTL 0x1ab40288 #define
> +PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK 0x0070L
> #define
> +PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT 0x4

I see in smu_v13_0.c and smu_v11_0.c, the same macro definitions are present. 
So is it better to put it into a common place which is scalable for later asics 
as well?

Regards,
Guchun

>  static const struct cmn2asic_msg_mapping
> smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
>   MSG_MAP(TestMessage,
> PPSMC_MSG_TestMessage,0),
> @@ -197,6 +200,7 @@ struct PPTable_t {
>  };
>
>  #define SMUQ10_TO_UINT(x) ((x) >> 10)
> +#define SMUQ16_TO_UINT(x) ((x) >> 16)
>
>  struct smu_v13_0_6_dpm_map {
>   enum smu_clk_type clk_type;
> @@ -1935,6 +1939,16 @@ static void
> smu_v13_0_6_log_thermal_throttling_event(struct smu_context *smu)
>
> smu_v13_0_6_throttler_map));
>  }
>
> +static int
> +smu_v13_0_6_get_current_pcie_link_width_level(struct smu_context *smu)
> +{
> + struct amdgpu_device *adev = smu->adev;
> +
> + return (RREG32_PCIE(smnPCIE_LC_LINK_WIDTH_CNTL) &
> + PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK) >>
> +PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT;
> +}
> +
>  static int smu_v13_0_6_get_current_pcie_link_speed(struct smu_context
> *smu)  {
>   struct amdgpu_device *adev = smu->adev; @@ -1953,8 +1967,12
> @@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct smu_context *smu,
> void **table
>   struct smu_table_context *smu_table = &smu->smu_table;
>   struct gpu_metrics_v1_3 *gpu_metrics =
>   (struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
> + struct amdgpu_device *adev = smu->adev;
> + int ret = 0, inst0, xcc0;
>   MetricsTable_t *metrics;
> - int i, ret = 0;
> +
> + inst0 = adev->sdma.instance[0].aid_id;
> + xcc0 = GET_INST(GC, 0);
>
>   metrics = kzalloc(sizeof(MetricsTable_t), GFP_KERNEL);
>   ret = smu_v13_0_6_get_metrics_table(smu, metrics, true); @@ -
> 1963,51 +1981,59 @@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct
> smu_context *smu, void **table
>
>   smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>
> - /* TODO: Decide on how to fill in zero value fields */
> - gpu_metrics->temperature_edge = 0;
> - gpu_metrics->temperature_hotspot = 0;
> - gpu_metrics->temperature_mem = 0;
> - gpu_metrics->temperature_vrgfx = 0;
> - gpu_metrics->temperature_vrsoc = 0;
> - gpu_metrics->temperature_vrmem = 0;
> -
> - gpu_metrics->average_gfx_activity = 0;
> - gpu_metrics->average_umc_activity = 0;
> - gpu_metrics->average_mm_activity = 0;
> -
> - gpu_metrics->average_socket_power = 0;
> - gpu_metrics->energy_accumulator = 0;
> -
> - gpu_metrics->average_gfxclk_frequency = 0;
> - gpu_metrics->average_socclk_frequency = 0;
> - gpu_metrics->average_uclk_frequency = 0;
> - gpu_metrics->average_vclk0_frequency = 0;
> - gpu_metrics->average_dclk0_frequency = 0;
> -
> - gpu_metrics->current_gfxclk = 0;
> - gpu_metrics->current_socclk = 0;
> - gpu_metrics->current_uclk = 0;
> - gpu_metrics->current_vclk0 = 0;
> - gpu_metrics->current_dclk0 = 0;
> -
> + gpu_metrics->temperature_hotspot =
> + SMUQ10_TO_UINT(metrics->MaxSocketTemperature);
> + /* Individual HBM stack temperature is not reported */
> + gpu_metrics->temperature_mem =
> + SMUQ10_TO_UINT(metrics->MaxHbmTemperature);
> + /* Reports max temperature of all voltage rails */
> + gpu_metrics->temperature_vrsoc =
> + SMUQ10_TO_UINT(metrics->MaxVrTemperature);
> +
> + gpu_metrics->average_gfx_activity =
> + SMUQ10_TO_UINT(metrics->SocketGfxBusy);
> + gpu_metrics->average_umc_activity =
> + SMUQ10_TO_UINT(metrics->DramBandwidthUtilization);
> +
> + gpu_metrics->average_socket_power =
> + SMUQ10_TO_UINT(metrics->SocketPower);
> + gpu_metrics->energy_accumulato

Re: [PATCH] drm/amd/pm: Fill metrics data for SMUv13.0.6

[Lazar, Lijo]

On 6/2/2023 11:26 AM, Chen, Guchun wrote:

[AMD Official Use Only - General]


-Original Message-
From: amd-gfx  On Behalf Of Lijo
Lazar
Sent: Friday, June 2, 2023 12:00 PM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Zhang, Hawking

Subject: [PATCH] drm/amd/pm: Fill metrics data for SMUv13.0.6

Populate metrics data table for SMU v13.0.6. Add PCIe link speed/width
information also.

Signed-off-by: Lijo Lazar 
---
  .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c  | 108 +++---

  1 file changed, 67 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index 75255e0baf91..4ff5a66d446a 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -80,7 +80,10 @@
  /* possible frequency drift (1Mhz) */
  #define EPSILON 1

-#define smnPCIE_ESM_CTRL 0x111003D0
+#define smnPCIE_ESM_CTRL 0x193D0
+#define smnPCIE_LC_LINK_WIDTH_CNTL 0x1ab40288 #define
+PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK 0x0070L
#define
+PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT 0x4


I see in smu_v13_0.c and smu_v11_0.c, the same macro definitions are present. 
So is it better to put it into a common place which is scalable for later asics 
as well?



These are reg offsets and reg field definitions. If there is no change 
to those offsets/fields we reuse the common smu_v13 or v11 versions for 
SMU13 or SMU11 family.


In this case, there is a change.

Thanks,
Lijo


Regards,
Guchun


  static const struct cmn2asic_msg_mapping
smu_v13_0_6_message_map[SMU_MSG_MAX_COUNT] = {
   MSG_MAP(TestMessage,
PPSMC_MSG_TestMessage,0),
@@ -197,6 +200,7 @@ struct PPTable_t {
  };

  #define SMUQ10_TO_UINT(x) ((x) >> 10)
+#define SMUQ16_TO_UINT(x) ((x) >> 16)

  struct smu_v13_0_6_dpm_map {
   enum smu_clk_type clk_type;
@@ -1935,6 +1939,16 @@ static void
smu_v13_0_6_log_thermal_throttling_event(struct smu_context *smu)

smu_v13_0_6_throttler_map));
  }

+static int
+smu_v13_0_6_get_current_pcie_link_width_level(struct smu_context *smu)
+{
+ struct amdgpu_device *adev = smu->adev;
+
+ return (RREG32_PCIE(smnPCIE_LC_LINK_WIDTH_CNTL) &
+ PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD_MASK) >>
+PCIE_LC_LINK_WIDTH_CNTL__LC_LINK_WIDTH_RD__SHIFT;
+}
+
  static int smu_v13_0_6_get_current_pcie_link_speed(struct smu_context
*smu)  {
   struct amdgpu_device *adev = smu->adev; @@ -1953,8 +1967,12
@@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct smu_context *smu,
void **table
   struct smu_table_context *smu_table = &smu->smu_table;
   struct gpu_metrics_v1_3 *gpu_metrics =
   (struct gpu_metrics_v1_3 *)smu_table->gpu_metrics_table;
+ struct amdgpu_device *adev = smu->adev;
+ int ret = 0, inst0, xcc0;
   MetricsTable_t *metrics;
- int i, ret = 0;
+
+ inst0 = adev->sdma.instance[0].aid_id;
+ xcc0 = GET_INST(GC, 0);

   metrics = kzalloc(sizeof(MetricsTable_t), GFP_KERNEL);
   ret = smu_v13_0_6_get_metrics_table(smu, metrics, true); @@ -
1963,51 +1981,59 @@ static ssize_t smu_v13_0_6_get_gpu_metrics(struct
smu_context *smu, void **table

   smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);

- /* TODO: Decide on how to fill in zero value fields */
- gpu_metrics->temperature_edge = 0;
- gpu_metrics->temperature_hotspot = 0;
- gpu_metrics->temperature_mem = 0;
- gpu_metrics->temperature_vrgfx = 0;
- gpu_metrics->temperature_vrsoc = 0;
- gpu_metrics->temperature_vrmem = 0;
-
- gpu_metrics->average_gfx_activity = 0;
- gpu_metrics->average_umc_activity = 0;
- gpu_metrics->average_mm_activity = 0;
-
- gpu_metrics->average_socket_power = 0;
- gpu_metrics->energy_accumulator = 0;
-
- gpu_metrics->average_gfxclk_frequency = 0;
- gpu_metrics->average_socclk_frequency = 0;
- gpu_metrics->average_uclk_frequency = 0;
- gpu_metrics->average_vclk0_frequency = 0;
- gpu_metrics->average_dclk0_frequency = 0;
-
- gpu_metrics->current_gfxclk = 0;
- gpu_metrics->current_socclk = 0;
- gpu_metrics->current_uclk = 0;
- gpu_metrics->current_vclk0 = 0;
- gpu_metrics->current_dclk0 = 0;
-
+ gpu_metrics->temperature_hotspot =
+ SMUQ10_TO_UINT(metrics->MaxSocketTemperature);
+ /* Individual HBM stack temperature is not reported */
+ gpu_metrics->temperature_mem =
+ SMUQ10_TO_UINT(metrics->MaxHbmTemperature);
+ /* Reports max temperature of all voltage rails */
+ gpu_metrics->temperature_vrsoc =
+ SMUQ10_TO_UINT(metrics->MaxVrTemperature);
+
+ gpu_metrics->average_gfx_activity =
+ SMUQ10_TO_UINT(metrics->SocketGfxBusy);
+ gpu_metrics->average_umc_activity =
+ SMUQ10_TO_UINT(metrics->DramBandwidthUtilization);
+
+ gpu_metrics->average_socket_power =
+ SMUQ10_TO_UINT(metrics->Sock