RE: [PATCH Review 1/1] drm/amdgpu: Fix potential null pointer derefernce
[AMD Official Use Only - General]
Reviewed-by: Tao Zhou
> -Original Message-
> From: amd-gfx On Behalf Of
> Stanley.Yang
> Sent: Thursday, September 28, 2023 11:46 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Yang, Stanley
> Subject: [PATCH Review 1/1] drm/amdgpu: Fix potential null pointer derefernce
>
> The amdgpu_ras_get_context may return NULL if device not support ras feature,
> so add check before using.
>
> Signed-off-by: Stanley.Yang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index cca3faf4dc23..60f8a18592b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5391,7 +5391,8 @@ int amdgpu_device_gpu_recover(struct
> amdgpu_device *adev,
>* Flush RAM to disk so that after reboot
>* the user can read log and see why the system rebooted.
>*/
> - if (need_emergency_restart && amdgpu_ras_get_context(adev)-
> >reboot) {
> + if (need_emergency_restart && amdgpu_ras_get_context(adev) &&
> + amdgpu_ras_get_context(adev)->reboot) {
> DRM_WARN("Emergency reboot.");
>
> ksys_sync_helper();
> --
> 2.25.1
RE: [PATCH Review 1/1] drm/amdgpu: Skip ring test during ras in recovery
[AMD Official Use Only - General]
Reviewed-by: Tao Zhou
> -Original Message-
> From: amd-gfx On Behalf Of
> Stanley.Yang
> Sent: Thursday, September 28, 2023 11:42 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Yang, Stanley
> Subject: [PATCH Review 1/1] drm/amdgpu: Skip ring test during ras in recovery
>
> This is workaround due to ring test failed during ras do gpu recovery for aqua
> vanjaram.
>
> Signed-off-by: Stanley.Yang
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> index fbfe0a1c4b19..9fff58d073a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
> @@ -248,10 +248,16 @@ static int gfx_v9_4_3_ring_test_ring(struct
> amdgpu_ring *ring) {
> uint32_t scratch_reg0_offset, xcc_offset;
> struct amdgpu_device *adev = ring->adev;
> + struct amdgpu_ras *ras;
> uint32_t tmp = 0;
> unsigned i;
> int r;
>
> + /* This is workaround: ring test failed during ras recovery */
> + ras = amdgpu_ras_get_context(adev);
> + if (ras && atomic_read(>in_recovery))
> + return 0;
> +
> /* Use register offset which is local to XCC in the packet */
> xcc_offset = SOC15_REG_OFFSET(GC, 0, regSCRATCH_REG0);
> scratch_reg0_offset = SOC15_REG_OFFSET(GC, GET_INST(GC, ring-
> >xcc_id), regSCRATCH_REG0);
> --
> 2.25.1
[PATCH Review 1/1] drm/amdgpu: Fix potential null pointer derefernce
The amdgpu_ras_get_context may return NULL if device
not support ras feature, so add check before using.
Signed-off-by: Stanley.Yang
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index cca3faf4dc23..60f8a18592b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5391,7 +5391,8 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
* Flush RAM to disk so that after reboot
* the user can read log and see why the system rebooted.
*/
- if (need_emergency_restart && amdgpu_ras_get_context(adev)->reboot) {
+ if (need_emergency_restart && amdgpu_ras_get_context(adev) &&
+ amdgpu_ras_get_context(adev)->reboot) {
DRM_WARN("Emergency reboot.");
ksys_sync_helper();
--
2.25.1
[PATCH Review 1/1] drm/amdgpu: Skip ring test during ras in recovery
This is workaround due to ring test failed during ras
do gpu recovery for aqua vanjaram.
Signed-off-by: Stanley.Yang
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
index fbfe0a1c4b19..9fff58d073a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
@@ -248,10 +248,16 @@ static int gfx_v9_4_3_ring_test_ring(struct amdgpu_ring
*ring)
{
uint32_t scratch_reg0_offset, xcc_offset;
struct amdgpu_device *adev = ring->adev;
+ struct amdgpu_ras *ras;
uint32_t tmp = 0;
unsigned i;
int r;
+ /* This is workaround: ring test failed during ras recovery */
+ ras = amdgpu_ras_get_context(adev);
+ if (ras && atomic_read(>in_recovery))
+ return 0;
+
/* Use register offset which is local to XCC in the packet */
xcc_offset = SOC15_REG_OFFSET(GC, 0, regSCRATCH_REG0);
scratch_reg0_offset = SOC15_REG_OFFSET(GC, GET_INST(GC, ring->xcc_id),
regSCRATCH_REG0);
--
2.25.1
Re: [PATCH] drm/amd/pm: delete dead code
Applied. Thanks! Alex On Wed, Sep 27, 2023 at 2:57 PM Wang, Yang(Kevin) wrote: > > [AMD Official Use Only - General] > > Thanks. > > Reviewed-by: Yang Wang > > Best Regards, > Kevin > > -Original Message- > From: Dan Carpenter > Sent: Wednesday, September 27, 2023 8:38 PM > To: Evan Quan ; Wang, Yang(Kevin) > Cc: Deucher, Alexander ; Koenig, Christian > ; Pan, Xinhui ; David Airlie > ; Daniel Vetter ; Lazar, Lijo > ; Kamal, Asad ; Zhang, Hawking > ; Wang, Yang(Kevin) ; > amd-gfx@lists.freedesktop.org; kernel-janit...@vger.kernel.org > Subject: [PATCH] drm/amd/pm: delete dead code > > "ret" was checked earlier inside the loop, so we know it is zero here. > No need to check a second time. > > Signed-off-by: Dan Carpenter > --- > drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 3 --- > 1 file changed, 3 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 11a6cd96c601..0ffe55e713f3 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 > @@ -2346,9 +2346,6 @@ static int mca_get_mca_entry(struct amdgpu_device > *adev, enum amdgpu_mca_error_t > return ret; > } > > - if (ret) > - return ret; > - > entry->idx = idx; > entry->type = type; > > -- > 2.39.2 >
[PATCH -next] drm/amd/display: clean up some inconsistent indentings
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn35/dcn35_fpu.c:261
dcn35_update_bw_bounding_box_fpu() warn: inconsistent indenting
Signed-off-by: Yang Li
---
.../drm/amd/display/dc/dml/dcn35/dcn35_fpu.c | 144 +-
1 file changed, 72 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn35/dcn35_fpu.c
b/drivers/gpu/drm/amd/display/dc/dml/dcn35/dcn35_fpu.c
index 4d5ee2aad9e4..4f284c31de5d 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/dcn35/dcn35_fpu.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/dcn35/dcn35_fpu.c
@@ -258,85 +258,85 @@ void dcn35_update_bw_bounding_box_fpu(struct dc *dc,
dc_assert_fp_enabled();
- dcn3_5_ip.max_num_otg =
- dc->res_pool->res_cap->num_timing_generator;
- dcn3_5_ip.max_num_dpp = dc->res_pool->pipe_count;
- dcn3_5_soc.num_chans = bw_params->num_channels;
-
- ASSERT(clk_table->num_entries);
-
- /* Prepass to find max clocks independent of voltage level. */
- for (i = 0; i < clk_table->num_entries; ++i) {
- if (clk_table->entries[i].dispclk_mhz > max_dispclk_mhz)
- max_dispclk_mhz =
clk_table->entries[i].dispclk_mhz;
- if (clk_table->entries[i].dppclk_mhz > max_dppclk_mhz)
- max_dppclk_mhz =
clk_table->entries[i].dppclk_mhz;
- }
+ dcn3_5_ip.max_num_otg =
+ dc->res_pool->res_cap->num_timing_generator;
+ dcn3_5_ip.max_num_dpp = dc->res_pool->pipe_count;
+ dcn3_5_soc.num_chans = bw_params->num_channels;
+
+ ASSERT(clk_table->num_entries);
+
+ /* Prepass to find max clocks independent of voltage level. */
+ for (i = 0; i < clk_table->num_entries; ++i) {
+ if (clk_table->entries[i].dispclk_mhz > max_dispclk_mhz)
+ max_dispclk_mhz = clk_table->entries[i].dispclk_mhz;
+ if (clk_table->entries[i].dppclk_mhz > max_dppclk_mhz)
+ max_dppclk_mhz = clk_table->entries[i].dppclk_mhz;
+ }
- for (i = 0; i < clk_table->num_entries; i++) {
- /* loop backwards*/
- for (closest_clk_lvl = 0, j = dcn3_5_soc.num_states - 1;
-j >= 0; j--) {
- if (dcn3_5_soc.clock_limits[j].dcfclk_mhz <=
- clk_table->entries[i].dcfclk_mhz) {
- closest_clk_lvl = j;
- break;
- }
- }
- if (clk_table->num_entries == 1) {
- /*smu gives one DPM level, let's take the
highest one*/
- closest_clk_lvl = dcn3_5_soc.num_states - 1;
+ for (i = 0; i < clk_table->num_entries; i++) {
+ /* loop backwards*/
+ for (closest_clk_lvl = 0, j = dcn3_5_soc.num_states - 1;
+ j >= 0; j--) {
+ if (dcn3_5_soc.clock_limits[j].dcfclk_mhz <=
+ clk_table->entries[i].dcfclk_mhz) {
+ closest_clk_lvl = j;
+ break;
}
+ }
+ if (clk_table->num_entries == 1) {
+ /*smu gives one DPM level, let's take the highest one*/
+ closest_clk_lvl = dcn3_5_soc.num_states - 1;
+ }
- clock_limits[i].state = i;
-
- /* Clocks dependent on voltage level. */
- clock_limits[i].dcfclk_mhz =
clk_table->entries[i].dcfclk_mhz;
- if (clk_table->num_entries == 1 &&
- clock_limits[i].dcfclk_mhz <
-
dcn3_5_soc.clock_limits[closest_clk_lvl].dcfclk_mhz) {
- /*SMU fix not released yet*/
- clock_limits[i].dcfclk_mhz =
-
dcn3_5_soc.clock_limits[closest_clk_lvl].dcfclk_mhz;
- }
+ clock_limits[i].state = i;
- clock_limits[i].fabricclk_mhz =
- clk_table->entries[i].fclk_mhz;
- clock_limits[i].socclk_mhz =
- clk_table->entries[i].socclk_mhz;
-
- if (clk_table->entries[i].memclk_mhz &&
- clk_table->entries[i].wck_ratio)
- clock_limits[i].dram_speed_mts =
- clk_table->entries[i].memclk_mhz * 2 *
- clk_table->entries[i].wck_ratio;
-
- /* Clocks independent of voltage level. */
-
Re: [PATCH] drm/amd/pm: Disallow managing power profiles on SRIOV for gc11.0.3
[Public]
Acked-by: Alex Deucher
From: amd-gfx on behalf of Victor Zhao
Sent: Monday, September 25, 2023 11:08 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhao, Victor
Subject: [PATCH] drm/amd/pm: Disallow managing power profiles on SRIOV for
gc11.0.3
disable pp_power_profile_mode for sriov on gc11.0.3 as not supported
by smu
Signed-off-by: Victor Zhao
---
drivers/gpu/drm/amd/pm/amdgpu_pm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 2d19282e4fbe..b6f32d57b81f 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2122,7 +2122,8 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
} else if (DEVICE_ATTR_IS(pp_power_profile_mode)) {
if (amdgpu_dpm_get_power_profile_mode(adev, NULL) ==
-EOPNOTSUPP)
*states = ATTR_STATE_UNSUPPORTED;
- else if (gc_ver == IP_VERSION(10, 3, 0) &&
amdgpu_sriov_vf(adev))
+ else if ((gc_ver == IP_VERSION(10, 3, 0) ||
+ gc_ver == IP_VERSION(11, 0, 3)) &&
amdgpu_sriov_vf(adev))
*states = ATTR_STATE_UNSUPPORTED;
}
--
2.34.1
Re: [PATCH v3] drm/amdkfd: Use partial migrations in GPU page faults
ping for review.
On 9/20/2023 12:32 PM, Xiaogang.Chen wrote:
From: Xiaogang Chen
This patch implements partial migration in gpu page fault according to migration
granularity(default 2MB) and not split svm range in cpu page fault handling.
A svm range may include pages from both system ram and vram of one gpu now.
These chagnes are expected to improve migration performance and reduce mmu
callback and TLB flush workloads.
Signed-off-by: xiaogang chen
---
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 156 +--
drivers/gpu/drm/amd/amdkfd/kfd_migrate.h | 6 +-
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 104 +++
drivers/gpu/drm/amd/amdkfd/kfd_svm.h | 6 +-
4 files changed, 178 insertions(+), 94 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index 6c25dab051d5..e886f9ce40ac 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -442,10 +442,10 @@ svm_migrate_vma_to_vram(struct kfd_node *node, struct
svm_range *prange,
goto out_free;
}
if (cpages != npages)
- pr_debug("partial migration, 0x%lx/0x%llx pages migrated\n",
+ pr_debug("partial migration, 0x%lx/0x%llx pages collected\n",
cpages, npages);
else
- pr_debug("0x%lx pages migrated\n", cpages);
+ pr_debug("0x%lx pages collected\n", cpages);
r = svm_migrate_copy_to_vram(node, prange, , , scratch, ttm_res_offset);
migrate_vma_pages();
@@ -479,6 +479,8 @@ svm_migrate_vma_to_vram(struct kfd_node *node, struct
svm_range *prange,
* svm_migrate_ram_to_vram - migrate svm range from system to device
* @prange: range structure
* @best_loc: the device to migrate to
+ * @start_mgr: start page to migrate
+ * @last_mgr: last page to migrate
* @mm: the process mm structure
* @trigger: reason of migration
*
@@ -489,6 +491,7 @@ svm_migrate_vma_to_vram(struct kfd_node *node, struct
svm_range *prange,
*/
static int
svm_migrate_ram_to_vram(struct svm_range *prange, uint32_t best_loc,
+ unsigned long start_mgr, unsigned long last_mgr,
struct mm_struct *mm, uint32_t trigger)
{
unsigned long addr, start, end;
@@ -498,23 +501,30 @@ svm_migrate_ram_to_vram(struct svm_range *prange,
uint32_t best_loc,
unsigned long cpages = 0;
long r = 0;
- if (prange->actual_loc == best_loc) {
- pr_debug("svms 0x%p [0x%lx 0x%lx] already on best_loc 0x%x\n",
-prange->svms, prange->start, prange->last, best_loc);
+ if (!best_loc) {
+ pr_debug("svms 0x%p [0x%lx 0x%lx] migrate to sys ram\n",
+ prange->svms, start_mgr, last_mgr);
return 0;
}
+ if (start_mgr < prange->start || last_mgr > prange->last) {
+ pr_debug("range [0x%lx 0x%lx] out prange [0x%lx 0x%lx]\n",
+start_mgr, last_mgr, prange->start,
prange->last);
+ return -EFAULT;
+ }
+
node = svm_range_get_node_by_id(prange, best_loc);
if (!node) {
pr_debug("failed to get kfd node by id 0x%x\n", best_loc);
return -ENODEV;
}
- pr_debug("svms 0x%p [0x%lx 0x%lx] to gpu 0x%x\n", prange->svms,
-prange->start, prange->last, best_loc);
+ pr_debug("svms 0x%p [0x%lx 0x%lx] in [0x%lx 0x%lx] to gpu 0x%x\n",
+ prange->svms, start_mgr, last_mgr, prange->start, prange->last,
+ best_loc);
- start = prange->start << PAGE_SHIFT;
- end = (prange->last + 1) << PAGE_SHIFT;
+ start = start_mgr << PAGE_SHIFT;
+ end = (last_mgr + 1) << PAGE_SHIFT;
r = svm_range_vram_node_new(node, prange, true);
if (r) {
@@ -544,8 +554,11 @@ svm_migrate_ram_to_vram(struct svm_range *prange, uint32_t
best_loc,
if (cpages) {
prange->actual_loc = best_loc;
- svm_range_dma_unmap(prange);
- } else {
+ prange->vram_pages = prange->vram_pages + cpages;
+ } else if (!prange->actual_loc) {
+ /* if no page migrated and all pages from prange are at
+* sys ram drop svm_bo got from svm_range_vram_node_new
+*/
svm_range_vram_node_free(prange);
}
@@ -663,19 +676,19 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
* Context: Process context, caller hold mmap read lock, prange->migrate_mutex
*
* Return:
- * 0 - success with all pages migrated
* negative values - indicate error
- * positive values - partial migration, number of pages not migrated
+ * positive values or zero - number of pages got migrated
*/
static long
svm_migrate_vma_to_ram(struct kfd_node *node, struct svm_range *prange,
-
Re: [PATCH v3 32/32] drm/amd/display: Add 3x4 CTM support for plane CTM
On 2023-09-25 15:49, Melissa Wen wrote:
> From: Joshua Ashton
>
> Create drm_color_ctm_3x4 to support 3x4-dimension plane CTM matrix and
> convert DRM CTM to DC CSC float matrix.
>
> v3:
> - rename ctm2 to ctm_3x4 (Harry)
>
> Signed-off-by: Joshua Ashton
Reviewed-by: Harry Wentland
Harry
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 28 +--
> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +-
> include/uapi/drm/drm_mode.h | 8 ++
> 3 files changed, 34 insertions(+), 4 deletions(-)
>
> 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 944dccd483de..774a83a9dee7 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
> @@ -410,6 +410,28 @@ static void __drm_ctm_to_dc_matrix(const struct
> drm_color_ctm *ctm,
> }
> }
>
> +/**
> + * __drm_ctm_3x4_to_dc_matrix - converts a DRM CTM 3x4 to a DC CSC float
> matrix
> + * @ctm: DRM color transformation matrix with 3x4 dimensions
> + * @matrix: DC CSC float matrix
> + *
> + * The matrix needs to be a 3x4 (12 entry) matrix.
> + */
> +static void __drm_ctm_3x4_to_dc_matrix(const struct drm_color_ctm_3x4 *ctm,
> +struct fixed31_32 *matrix)
> +{
> + int i;
> +
> + /* The format provided is S31.32, using signed-magnitude representation.
> + * Our fixed31_32 is also S31.32, but is using 2's complement. We have
> + * to convert from signed-magnitude to 2's complement.
> + */
> + for (i = 0; i < 12; i++) {
> + /* gamut_remap_matrix[i] = ctm[i - floor(i/4)] */
> + matrix[i] = dc_fixpt_from_s3132(ctm->matrix[i]);
> + }
> +}
> +
> /**
> * __set_legacy_tf - Calculates the legacy transfer function
> * @func: transfer function
> @@ -1154,7 +1176,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> dm_crtc_state *crtc,
> {
> struct amdgpu_device *adev = drm_to_adev(crtc->base.state->dev);
> struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
> - struct drm_color_ctm *ctm = NULL;
> + struct drm_color_ctm_3x4 *ctm = NULL;
> struct dc_color_caps *color_caps = NULL;
> bool has_crtc_cm_degamma;
> int ret;
> @@ -1208,7 +1230,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> dm_crtc_state *crtc,
>
> /* Setup CRTC CTM. */
> if (dm_plane_state->ctm) {
> - ctm = (struct drm_color_ctm *)dm_plane_state->ctm->data;
> + ctm = (struct drm_color_ctm_3x4 *)dm_plane_state->ctm->data;
> /* DCN2 and older don't support both pre-blending and
>* post-blending gamut remap. For this HW family, if we have
>* the plane and CRTC CTMs simultaneously, CRTC CTM takes
> @@ -1219,7 +1241,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> dm_crtc_state *crtc,
>* mapping CRTC CTM to MPC and keeping plane CTM setup at DPP,
>* as it's done by dcn30_program_gamut_remap().
>*/
> - __drm_ctm_to_dc_matrix(ctm,
> dc_plane_state->gamut_remap_matrix.matrix);
> + __drm_ctm_3x4_to_dc_matrix(ctm,
> dc_plane_state->gamut_remap_matrix.matrix);
>
> dc_plane_state->gamut_remap_matrix.enable_remap = true;
> dc_plane_state->input_csc_color_matrix.enable_adjustment =
> false;
> 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 98501f5044c2..a1f087b0f7e5 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
> @@ -1551,7 +1551,7 @@ dm_atomic_plane_set_property(struct drm_plane *plane,
> ret = drm_property_replace_blob_from_id(plane->dev,
> _plane_state->ctm,
> val,
> - sizeof(struct
> drm_color_ctm), -1,
> + sizeof(struct
> drm_color_ctm_3x4), -1,
> );
> dm_plane_state->base.color_mgmt_changed |= replaced;
> return ret;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 46becedf5b2f..a811d24e8ed5 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -838,6 +838,14 @@ struct drm_color_ctm {
> __u64 matrix[9];
> };
>
> +struct drm_color_ctm_3x4 {
> + /*
> + * Conversion matrix with 3x4 dimensions in S31.32 sign-magnitude
> + * (not two's complement!) format.
> + */
> + __u64 matrix[12];
> +};
> +
> struct drm_color_lut {
> /*
>* Values are mapped linearly to 0.0 - 1.0 range, with
Re: [PATCH v3 31/32] drm/amd/display: add plane CTM support
On 2023-09-25 15:49, Melissa Wen wrote:
> Map the plane CTM driver-specific property to DC plane, instead of DC
> stream. The remaining steps to program DPP block are already implemented
> on DC shared-code.
>
> v3:
> - fix comment about plane and CRTC CTMs priorities (Harry)
>
> Signed-off-by: Melissa Wen
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 +
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 25 +++
> 2 files changed, 26 insertions(+)
>
> 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 581afa7c5f8c..e4edd97982a6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9714,6 +9714,7 @@ static bool should_reset_plane(struct drm_atomic_state
> *state,
> if (dm_old_other_state->degamma_tf !=
> dm_new_other_state->degamma_tf ||
> dm_old_other_state->degamma_lut !=
> dm_new_other_state->degamma_lut ||
> dm_old_other_state->hdr_mult !=
> dm_new_other_state->hdr_mult ||
> + dm_old_other_state->ctm != dm_new_other_state->ctm ||
> dm_old_other_state->shaper_lut !=
> dm_new_other_state->shaper_lut ||
> dm_old_other_state->shaper_tf !=
> dm_new_other_state->shaper_tf ||
> dm_old_other_state->lut3d != dm_new_other_state->lut3d ||
> 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 26338f565574..944dccd483de 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
> @@ -1153,6 +1153,8 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> dm_crtc_state *crtc,
> struct dc_plane_state *dc_plane_state)
> {
> struct amdgpu_device *adev = drm_to_adev(crtc->base.state->dev);
> + struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
> + struct drm_color_ctm *ctm = NULL;
> struct dc_color_caps *color_caps = NULL;
> bool has_crtc_cm_degamma;
> int ret;
> @@ -1204,5 +1206,28 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> dm_crtc_state *crtc,
> return ret;
> }
>
> + /* Setup CRTC CTM. */
> + if (dm_plane_state->ctm) {
> + ctm = (struct drm_color_ctm *)dm_plane_state->ctm->data;
> + /* DCN2 and older don't support both pre-blending and
nit: formatting this with a newline before the comment and then:
> + /*
> + * DCN2 and older don't support both pre-blending and
would make this more readable.
Reviewed-by: Harry Wentland
Harry
> + * post-blending gamut remap. For this HW family, if we have
> + * the plane and CRTC CTMs simultaneously, CRTC CTM takes
> + * priority, and we discard plane CTM, as implemented in
> + * dcn10_program_gamut_remap(). However, DCN3+ has DPP
> + * (pre-blending) and MPC (post-blending) `gamut remap` blocks;
> + * therefore, we can program plane and CRTC CTMs together by
> + * mapping CRTC CTM to MPC and keeping plane CTM setup at DPP,
> + * as it's done by dcn30_program_gamut_remap().
> + */
> + __drm_ctm_to_dc_matrix(ctm,
> dc_plane_state->gamut_remap_matrix.matrix);
> +
> + dc_plane_state->gamut_remap_matrix.enable_remap = true;
> + dc_plane_state->input_csc_color_matrix.enable_adjustment =
> false;
> + } else {
> + /* Bypass CTM. */
> + dc_plane_state->gamut_remap_matrix.enable_remap = false;
> + dc_plane_state->input_csc_color_matrix.enable_adjustment =
> false;
> + }
> +
> return amdgpu_dm_plane_set_color_properties(plane_state,
> dc_plane_state);
> }
Re: [PATCH v3 28/32] drm/amd/display: allow newer DC hardware to use degamma ROM for PQ/HLG
On 2023-09-25 15:49, Melissa Wen wrote:
> From: Joshua Ashton
>
> Need to funnel the color caps through to these functions so it can check
> that the hardware is capable.
>
> v2:
> - remove redundant color caps assignment on plane degamma map (Harry)
> - pass color caps to degamma params
>
> v3:
> - remove unused color_caps parameter from set_color_properties (Harry)
>
> Signed-off-by: Joshua Ashton
> Signed-off-by: Melissa Wen
Reviewed-by: Harry Wentland
Harry
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 29 ---
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> 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 15590677f209..7871256f0e5f 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
> @@ -536,6 +536,7 @@ static int amdgpu_dm_set_atomic_regamma(struct
> dc_stream_state *stream,
> /**
> * __set_input_tf - calculates the input transfer function based on expected
> * input space.
> + * @caps: dc color capabilities
> * @func: transfer function
> * @lut: lookup table that defines the color space
> * @lut_size: size of respective lut.
> @@ -543,7 +544,7 @@ static int amdgpu_dm_set_atomic_regamma(struct
> dc_stream_state *stream,
> * Returns:
> * 0 in case of success. -ENOMEM if fails.
> */
> -static int __set_input_tf(struct dc_transfer_func *func,
> +static int __set_input_tf(struct dc_color_caps *caps, struct
> dc_transfer_func *func,
> const struct drm_color_lut *lut, uint32_t lut_size)
> {
> struct dc_gamma *gamma = NULL;
> @@ -560,7 +561,7 @@ static int __set_input_tf(struct dc_transfer_func *func,
> __drm_lut_to_dc_gamma(lut, gamma, false);
> }
>
> - res = mod_color_calculate_degamma_params(NULL, func, gamma, gamma !=
> NULL);
> + res = mod_color_calculate_degamma_params(caps, func, gamma, gamma !=
> NULL);
>
> if (gamma)
> dc_gamma_release();
> @@ -721,7 +722,7 @@ static int amdgpu_dm_atomic_blend_lut(const struct
> drm_color_lut *blend_lut,
> func_blend->tf = tf;
> func_blend->sdr_ref_white_level = SDR_WHITE_LEVEL_INIT_VALUE;
>
> - ret = __set_input_tf(func_blend, blend_lut, blend_size);
> + ret = __set_input_tf(NULL, func_blend, blend_lut, blend_size);
> } else {
> func_blend->type = TF_TYPE_BYPASS;
> func_blend->tf = TRANSFER_FUNCTION_LINEAR;
> @@ -946,7 +947,8 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state
> *crtc)
>
> static int
> map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
> - struct dc_plane_state *dc_plane_state)
> + struct dc_plane_state *dc_plane_state,
> + struct dc_color_caps *caps)
> {
> const struct drm_color_lut *degamma_lut;
> enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_SRGB;
> @@ -1001,7 +1003,7 @@ map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
> dc_plane_state->in_transfer_func->tf =
> TRANSFER_FUNCTION_LINEAR;
>
> - r = __set_input_tf(dc_plane_state->in_transfer_func,
> + r = __set_input_tf(caps, dc_plane_state->in_transfer_func,
> degamma_lut, degamma_size);
> if (r)
> return r;
> @@ -1014,7 +1016,7 @@ map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
> dc_plane_state->in_transfer_func->tf = tf;
>
> if (tf != TRANSFER_FUNCTION_SRGB &&
> - !mod_color_calculate_degamma_params(NULL,
> + !mod_color_calculate_degamma_params(caps,
>
> dc_plane_state->in_transfer_func,
> NULL, false))
> return -ENOMEM;
> @@ -1025,7 +1027,8 @@ map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
>
> static int
> __set_dm_plane_degamma(struct drm_plane_state *plane_state,
> -struct dc_plane_state *dc_plane_state)
> +struct dc_plane_state *dc_plane_state,
> +struct dc_color_caps *color_caps)
> {
> struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
> const struct drm_color_lut *degamma_lut;
> @@ -1056,7 +1059,7 @@ __set_dm_plane_degamma(struct drm_plane_state
> *plane_state,
> dc_plane_state->in_transfer_func->type =
> TF_TYPE_DISTRIBUTED_POINTS;
>
> - ret = __set_input_tf(dc_plane_state->in_transfer_func,
> + ret = __set_input_tf(color_caps,
> dc_plane_state->in_transfer_func,
>degamma_lut, degamma_size);
> if
Re: [PATCH v3 23/32] drm/amd/display: add plane shaper LUT support
On 2023-09-25 15:49, Melissa Wen wrote:
> Map DC shaper LUT to DM plane color management. Shaper LUT can be used
> to delinearize and/or normalize the color space for computational
> efficiency and achiving specific visual styles. If a plane degamma is
> apply to linearize the color space, a custom shaper 1D LUT can be used
> just before applying 3D LUT.
>
> v2:
> - use DPP color caps to verify plane 3D LUT support
> - add debug message if shaper LUT programming fails
>
> Reviewed-by: Harry Wentland
> Signed-off-by: Melissa Wen
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 +
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 108 +-
> 3 files changed, 107 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 455b690d6185..af18c523c431 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8185,6 +8185,7 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
> bundle->surface_updates[planes_count].in_transfer_func
> = dc_plane->in_transfer_func;
>
> bundle->surface_updates[planes_count].gamut_remap_matrix =
> _plane->gamut_remap_matrix;
> bundle->surface_updates[planes_count].hdr_mult =
> dc_plane->hdr_mult;
> + bundle->surface_updates[planes_count].func_shaper =
> dc_plane->in_shaper_func;
> }
>
> amdgpu_dm_plane_fill_dc_scaling_info(dm->adev, new_plane_state,
> 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 b7b1d67f87a0..ad583cc97f15 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -897,6 +897,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,
> + struct drm_plane_state *plane_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 577a9dc669a5..8c991b5cf473 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
> @@ -594,6 +594,74 @@ amdgpu_tf_to_dc_tf(enum amdgpu_transfer_function tf)
> }
> }
>
> +static int amdgpu_dm_atomic_shaper_lut(const struct drm_color_lut
> *shaper_lut,
> +uint32_t shaper_size,
> +struct dc_transfer_func *func_shaper)
> +{
> + int ret = 0;
> +
> + if (shaper_size) {
> + /* If DRM shaper LUT is set, we assume a linear color space
> + * (linearized by DRM degamma 1D LUT or not)
> + */
> + func_shaper->type = TF_TYPE_DISTRIBUTED_POINTS;
> + func_shaper->tf = TRANSFER_FUNCTION_LINEAR;
> +
> + ret = __set_output_tf(func_shaper, shaper_lut, shaper_size,
> false);
> + } else {
> + func_shaper->type = TF_TYPE_BYPASS;
> + func_shaper->tf = TRANSFER_FUNCTION_LINEAR;
> + }
> +
> + return ret;
> +}
> +
> +/* amdgpu_dm_lut3d_size - get expected size according to hw color caps
> + * @adev: amdgpu device
> + * @lut_size: default size
> + *
> + * Return:
> + * lut_size if DC 3D LUT is supported, zero otherwise.
> + */
> +static uint32_t amdgpu_dm_get_lut3d_size(struct amdgpu_device *adev,
The function name is a bit confusing since this just returns the
lut_size if we have a hw_3d_lut caps field. Might be clearer to
simply drop the function and do the below line of code in its
place directly.
No need to drop the RB, just a minor thing.
Harry
> + uint32_t lut_size)
> +{
> + return adev->dm.dc->caps.color.dpp.hw_3d_lut ? lut_size : 0;
> +}
> +
> +/**
> + * amdgpu_dm_verify_lut3d_size - verifies if 3D LUT is supported and if DRM
> 3D
> + * LUT matches the hw supported size
> + * @adev: amdgpu device
> + * @crtc_state: the DRM CRTC state
> + *
> + * Verifies if post-blending (MPC) 3D LUT is supported by the HW (DCN 3.0 or
> + * newer) and if the DRM 3D LUT matches the supported size.
> + *
> + * Returns:
> + * 0 on success. -EINVAL if lut size are invalid.
> + */
> +int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
> + struct drm_plane_state *plane_state)
> +{
> + struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
> +
Re: [PATCH v3 09/32] drm/amd/display: add plane 3D LUT driver-specific properties
On 2023-09-25 15:49, Melissa Wen wrote:
> Add 3D LUT property for plane color transformations using a 3D lookup
> table. 3D LUT allows for highly accurate and complex color
> transformations and is suitable to adjust the balance between color
> channels. It's also more complex to manage and require more
> computational resources. Since a 3D LUT has a limited number of entries
> in each dimension we want to use them in an optimal fashion. This means
> using the 3D LUT in a colorspace that is optimized for human vision,
> such as sRGB, PQ, or another non-linear space. Therefore, userpace may
> need one 1D LUT (shaper) before it to delinearize content and another 1D
> LUT after 3D LUT (blend) to linearize content again for blending. The
> next patches add these 1D LUTs to the plane color mgmt pipeline.
>
> v3:
> - improve commit message about 3D LUT
> - describe the 3D LUT entries and size (Harry)
>
> Signed-off-by: Melissa Wen
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 17 ++
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 9
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 14 +++
> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 23 +++
> 4 files changed, 63 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 66bae0eed80c..1b5f25989f7f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -363,6 +363,23 @@ struct amdgpu_mode_info {
>* @plane_hdr_mult_property:
>*/
> struct drm_property *plane_hdr_mult_property;
> + /**
> + * @plane_lut3d_property: Plane property for color transformation using
> + * a 3D LUT (pre-blending), a three-dimensional array where each
> + * element is an RGB triplet. Each dimension has a size of the cubed
> + * root of lut3d_size. The array contains samples from the approximated
> + * function. On AMD, values between samples are estimated by
> + * tetrahedral interpolation. The array is accessed with three indices,
> + * one for each input dimension (color channel), blue being the
> + * outermost dimension, red the innermost.
> + */
> + struct drm_property *plane_lut3d_property;
> + /**
> + * @plane_degamma_lut_size_property: Plane property to define the max
> + * size of 3D LUT as supported by the driver (read-only). The max size
> + * is the max size of one dimension cubed.
> + */
I've been thinking about this some more and don't particulary
like that we're reporting the size as the dimension cubed, e.g.,
4913 (17^3) instead of 17. This works for an AMD private API
(and I'm okay with keeping it as-is if changing it is a lot of
effort at this point) but in a generic API it would be a source
of bugs or undefined behavior if a driver mistakenly reported
a size that doesn't have an even cubed root.
Reporting the size of a single dimension (e.g., 17 in the case
of the current AMD driver) would be clearer.
Could we still change that?
Harry
> + struct drm_property *plane_lut3d_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 7ca594c7dfbe..dbd36fc24eca 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -773,6 +773,11 @@ struct dm_plane_state {
>* S31.32 sign-magnitude.
>*/
> __u64 hdr_mult;
> + /**
> + * @lut3d: 3D lookup table blob. The blob (if not NULL) is an array of
> + * drm_color_lut.
> + */
> + struct drm_property_blob *lut3d;
> };
>
> struct dm_crtc_state {
> @@ -858,6 +863,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector
> *connector,
>
> 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
> +/* 1D LUT size */
> #define MAX_COLOR_LUT_ENTRIES 4096
> /* Legacy gamm LUT users such as X doesn't like large LUT sizes */
> #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
> 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 f274909c0c7e..e2f3f2099cac 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
> @@ -207,6 +207,20 @@ amdgpu_dm_create_color_properties(struct amdgpu_device
> *adev)
> return -ENOMEM;
> adev->mode_info.plane_hdr_mult_property = prop;
>
> + prop = drm_property_create(adev_to_drm(adev),
> +DRM_MODE_PROP_BLOB,
> +"AMD_PLANE_LUT3D", 0);
> + if (!prop)
> + return -ENOMEM;
> +
[PATCH v2 1/2] drm/amd: Move `enum drm_amdgpu_ta_fw_type` to uapi
Enum values used by the ioctl `AMDGPU_INFO_FW_VERSION`/`AMDGPU_INFO_FW_TA`
are not exported so clients need to keep their own copy of the definitions
while looking up firmware versions for the TA.
Move this to uapi instead.
Signed-off-by: Mario Limonciello
--
v1->v2:
* Rename while moving (Alex)
---
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 18 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 12
include/uapi/drm/amdgpu_drm.h | 12
4 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 081bd28e2443..6eff7eb18322 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -311,32 +311,32 @@ static int amdgpu_firmware_info(struct
drm_amdgpu_info_firmware *fw_info,
break;
case AMDGPU_INFO_FW_TA:
switch (query_fw->index) {
- case TA_FW_TYPE_PSP_XGMI:
+ case AMDGPU_TA_FW_TYPE_PSP_XGMI:
fw_info->ver =
adev->psp.xgmi_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.xgmi_context.context
.bin_desc.feature_version;
break;
- case TA_FW_TYPE_PSP_RAS:
+ case AMDGPU_TA_FW_TYPE_PSP_RAS:
fw_info->ver =
adev->psp.ras_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.ras_context.context
.bin_desc.feature_version;
break;
- case TA_FW_TYPE_PSP_HDCP:
+ case AMDGPU_TA_FW_TYPE_PSP_HDCP:
fw_info->ver =
adev->psp.hdcp_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.hdcp_context.context
.bin_desc.feature_version;
break;
- case TA_FW_TYPE_PSP_DTM:
+ case AMDGPU_TA_FW_TYPE_PSP_DTM:
fw_info->ver =
adev->psp.dtm_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.dtm_context.context
.bin_desc.feature_version;
break;
- case TA_FW_TYPE_PSP_RAP:
+ case AMDGPU_TA_FW_TYPE_PSP_RAP:
fw_info->ver =
adev->psp.rap_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.rap_context.context
.bin_desc.feature_version;
break;
- case TA_FW_TYPE_PSP_SECUREDISPLAY:
+ case AMDGPU_TA_FW_TYPE_PSP_SECUREDISPLAY:
fw_info->ver =
adev->psp.securedisplay_context.context.bin_desc.fw_version;
fw_info->feature =
adev->psp.securedisplay_context.context.bin_desc
@@ -1536,8 +1536,8 @@ static int amdgpu_debugfs_firmware_info_show(struct
seq_file *m, void *unused)
uint8_t smu_program, smu_major, smu_minor, smu_debug;
int ret, i;
- static const char *ta_fw_name[TA_FW_TYPE_MAX_INDEX] = {
-#define TA_FW_NAME(type)[TA_FW_TYPE_PSP_##type] = #type
+ static const char *ta_fw_name[AMDGPU_TA_FW_TYPE_MAX_INDEX] = {
+#define TA_FW_NAME(type)[AMDGPU_TA_FW_TYPE_PSP_##type] = #type
TA_FW_NAME(XGMI),
TA_FW_NAME(RAS),
TA_FW_NAME(HDCP),
@@ -1689,7 +1689,7 @@ static int amdgpu_debugfs_firmware_info_show(struct
seq_file *m, void *unused)
fw_info.feature, fw_info.ver);
query_fw.fw_type = AMDGPU_INFO_FW_TA;
- for (i = TA_FW_TYPE_PSP_XGMI; i < TA_FW_TYPE_MAX_INDEX; i++) {
+ for (i = AMDGPU_TA_FW_TYPE_PSP_XGMI; i < AMDGPU_TA_FW_TYPE_MAX_INDEX;
i++) {
query_fw.index = i;
ret = amdgpu_firmware_info(_info, _fw, adev);
if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 72ee66db182c..bf9eb53cd4fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -3291,38 +3291,38 @@ static int parse_ta_bin_descriptor(struct psp_context
*psp,
le32_to_cpu(ta_hdr->header.ucode_array_offset_bytes);
switch (desc->fw_type) {
- case TA_FW_TYPE_PSP_ASD:
+ case AMDGPU_TA_FW_TYPE_PSP_ASD:
psp->asd_context.bin_desc.fw_version=
le32_to_cpu(desc->fw_version);
psp->asd_context.bin_desc.feature_version =
le32_to_cpu(desc->fw_version);
psp->asd_context.bin_desc.size_bytes=
le32_to_cpu(desc->size_bytes);
[PATCH v2 2/2] drm/amd: Also export `AMDGPU_TA_FW_TYPE_PSP_ASD` for `AMDGPU_INFO_FW_TA`
All other `drm_amdgpu_ta_fw_type` enums are exported for
`AMDGPU_INFO_FW_TA`. ASD is available via `AMDGPU_INFO_FW_ASD` but
for symmetry in other values export as part of `AMDGPU_INFO_FW_TA` as well.
Signed-off-by: Mario Limonciello
---
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 6eff7eb18322..77863d6af6c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -311,6 +311,11 @@ static int amdgpu_firmware_info(struct
drm_amdgpu_info_firmware *fw_info,
break;
case AMDGPU_INFO_FW_TA:
switch (query_fw->index) {
+ case AMDGPU_TA_FW_TYPE_PSP_ASD:
+ fw_info->ver =
adev->psp.asd_context.bin_desc.fw_version;
+ fw_info->feature = adev->psp.asd_context
+ .bin_desc.feature_version;
+ break;
case AMDGPU_TA_FW_TYPE_PSP_XGMI:
fw_info->ver =
adev->psp.xgmi_context.context.bin_desc.fw_version;
fw_info->feature = adev->psp.xgmi_context.context
--
2.34.1
Re: [PATCH] drm/amd: Move `enum drm_amdgpu_ta_fw_type` to uapi
On Wed, Sep 27, 2023 at 2:21 PM Mario Limonciello
wrote:
>
> Enum values used by the ioctl `AMDGPU_INFO_FW_VERSION`/`AMDGPU_INFO_FW_TA`
> are not exported so clients need to keep their own copy of the definitions
> while looking up firmware versions for the TA.
>
> Move this to uapi instead.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 12
> include/uapi/drm/amdgpu_drm.h | 12
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> index ae5fa61d2890..73a84af54d70 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> @@ -145,18 +145,6 @@ struct ta_firmware_header_v1_0 {
> struct psp_fw_legacy_bin_desc securedisplay;
> };
>
> -enum ta_fw_type {
> - TA_FW_TYPE_UNKOWN,
> - TA_FW_TYPE_PSP_ASD,
> - TA_FW_TYPE_PSP_XGMI,
> - TA_FW_TYPE_PSP_RAS,
> - TA_FW_TYPE_PSP_HDCP,
> - TA_FW_TYPE_PSP_DTM,
> - TA_FW_TYPE_PSP_RAP,
> - TA_FW_TYPE_PSP_SECUREDISPLAY,
> - TA_FW_TYPE_MAX_INDEX,
> -};
> -
> /* version_major=2, version_minor=0 */
> struct ta_firmware_header_v2_0 {
> struct common_firmware_header header;
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 984fc16577ca..225dec3634f0 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -912,6 +912,18 @@ struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> #define AMDGPU_INFO_MMR_SH_INDEX_SHIFT 8
> #define AMDGPU_INFO_MMR_SH_INDEX_MASK 0xff
>
> +enum drm_amdgpu_ta_fw_type {
> + TA_FW_TYPE_UNKOWN,
> + TA_FW_TYPE_PSP_ASD,
> + TA_FW_TYPE_PSP_XGMI,
> + TA_FW_TYPE_PSP_RAS,
> + TA_FW_TYPE_PSP_HDCP,
> + TA_FW_TYPE_PSP_DTM,
> + TA_FW_TYPE_PSP_RAP,
> + TA_FW_TYPE_PSP_SECUREDISPLAY,
> + TA_FW_TYPE_MAX_INDEX,
> +};
Prefix these with AMDGPU_?
Alex
> +
> struct drm_amdgpu_query_fw {
> /** AMDGPU_INFO_FW_* */
> __u32 fw_type;
> --
> 2.34.1
>
Re: [PATCH] drm/amd/display: Check all enabled planes in dm_check_crtc_cursor
On 9/12/23 06:22, Michel Dänzer wrote:
From: Michel Dänzer
It was only checking planes which had any state changes in the same
commit. However, it also needs to check other enabled planes.
Not doing this meant that a commit might spuriously "succeed", resulting
in the cursor plane displaying with incorrect scaling. See
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3177#note_1824263
for an example.
Fixes: d1bfbe8a3202 ("amd/display: check cursor plane matches underlying plane")
Signed-off-by: Michel Dänzer
Applied, thanks!
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 88ba8b66de1f..81c9d39567db 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9836,14 +9836,24 @@ static int dm_check_crtc_cursor(struct drm_atomic_state
*state,
* blending properties match the underlying planes'.
*/
- new_cursor_state = drm_atomic_get_new_plane_state(state, cursor);
- if (!new_cursor_state || !new_cursor_state->fb)
+ new_cursor_state = drm_atomic_get_plane_state(state, cursor);
+ if (IS_ERR(new_cursor_state))
+ return PTR_ERR(new_cursor_state);
+
+ if (!new_cursor_state->fb)
return 0;
dm_get_oriented_plane_size(new_cursor_state, _src_w, _src_h);
cursor_scale_w = new_cursor_state->crtc_w * 1000 / cursor_src_w;
cursor_scale_h = new_cursor_state->crtc_h * 1000 / cursor_src_h;
+ /* Need to check all enabled planes, even if this commit doesn't change
+* their state
+*/
+ i = drm_atomic_add_affected_planes(state, crtc);
+ if (i)
+ return i;
+
for_each_new_plane_in_state_reverse(state, underlying,
new_underlying_state, i) {
/* Narrow down to non-cursor planes on the same CRTC as the
cursor */
if (new_underlying_state->crtc != crtc || underlying ==
crtc->cursor)
--
Hamza
Re: [PATCH] drm/amdkfd: Wait vm update fence after retry fault recovered
On 9/27/2023 9:29 AM, Philip Yang wrote:
On 2023-09-26 16:43, Chen, Xiaogang wrote:
On 9/22/2023 4:37 PM, Philip Yang wrote:
Caution: This message originated from an External Source. Use proper
caution when opening attachments, clicking links, or responding.
Otherwise kfd flush tlb does nothing if vm update fence callback
doesn't
update vm->tlb_seq. H/W will generate retry fault again.
This works now because retry fault keep coming, recover will update
page
table again after AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING timeout and
flush
tlb.
I think what this patch does is waiting vm->last_update fence at gpu
page fault retry handler. I do not know what bug it tries to fix. h/w
will keep generating retry fault as long as vm page table is not
setup correctly, no matter kfd driver waits the fence or not. vm page
table eventually will be setup.
This issue was there, I notice it when implementing the granularity
bitmap_mapped flag for mGPUs, to skip the retry fault if prange mapped
on the GPU. The retry fault keep coming after updating GPU page table,
because restore_pages -> svm_range_validate_and_map doesn't wait for
vm update fence before kfd_flush_tlb.
Now I start knowing your change: it is a general issue for gpu retry
fault handling that tlb flush did not happen actually since vm->tlb_seq
was not updated as we did not give fence to amdgpu_vm_update_range.
It is working now because we handle the same retry fault again after
timeout AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING, and kfd_flush_tlb does
flush on the second time
The issue only exist if using sdma update GPU page table, as no fence
if cpu update GPU page table.
There are several todo items to optimize this further:
A. After updating GPU page table, we only wait for fence and flush tlb
if updating existing mapping, or vm params.table_freed (this needs
amdgpu vm interface change).
B. Use sync to wait mGPUs update fences.
C. Use multiple workers to handle restore_pages.
There is a consequence I saw: if we wait vm page table update fence
it will delay gpu page fault handler exit. Then more h/w interrupt
vectors will be sent to sw ring, potentially cause the ring overflow.
retry CAM filter, or sw filter drop the duplicate retry fault, to
prevent sw ring overflow.
Inside sw ring there is not only gpu retry fault type, also has other
interrupt types, right? These filters just drop interrupt, not make sure
all interrupts needed to handle got handled. I mean delay gpu retry
handler exist may drop more interrupts, some of them never got handled.
But I think this risk already existed, and we may need increase sw ring.
Regards
Xiaogang
Regards,
Philip
Regards
Xiaogang
Remove wait parameter in svm_range_validate_and_map because it is
always called with true.
Signed-off-by: Philip Yang
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 70aa882636ab..61f4de1633a8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1447,7 +1447,7 @@ svm_range_map_to_gpu(struct kfd_process_device
*pdd, struct svm_range *prange,
static int
svm_range_map_to_gpus(struct svm_range *prange, unsigned long offset,
unsigned long npages, bool readonly,
- unsigned long *bitmap, bool wait, bool flush_tlb)
+ unsigned long *bitmap, bool flush_tlb)
{
struct kfd_process_device *pdd;
struct amdgpu_device *bo_adev = NULL;
@@ -1480,8 +1480,7 @@ svm_range_map_to_gpus(struct svm_range
*prange, unsigned long offset,
r = svm_range_map_to_gpu(pdd, prange, offset,
npages, readonly,
prange->dma_addr[gpuidx],
- bo_adev, wait ? : NULL,
- flush_tlb);
+ bo_adev, , flush_tlb);
if (r)
break;
@@ -1605,7 +1604,7 @@ static void *kfd_svm_page_owner(struct
kfd_process *p, int32_t gpuidx)
*/
static int svm_range_validate_and_map(struct mm_struct *mm,
struct svm_range *prange,
int32_t gpuidx,
- bool intr, bool wait, bool
flush_tlb)
+ bool intr, bool flush_tlb)
{
struct svm_validate_context *ctx;
unsigned long start, end, addr;
@@ -1729,7 +1728,7 @@ static int svm_range_validate_and_map(struct
mm_struct *mm,
if (!r)
r = svm_range_map_to_gpus(prange, offset,
npages, readonly,
- ctx->bitmap, wait, flush_tlb);
+ ctx->bitmap, flush_tlb);
if (!r && next == end)
prange->mapped_to_gpu = true;
@@ -1823,7 +1822,7 @@ static void svm_range_restore_work(struct
[PATCH 1/2] drm/amdgpu/gmc: add a way to force a particular placement for GART
We normally place GART based on the location of VRAM and the
available address space around that, but provide an option
to force a particular location for hardware that needs it.
v2: Switch to passing the placement via parameter
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 22 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 9 -
drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
8 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2bfeaacd050c..60c81c3d29d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -269,7 +269,8 @@ void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev,
struct amdgpu_gmc *mc
* If GART size is bigger than space left then we ajust GART size.
* Thus function will never fails.
*/
-void amdgpu_gmc_gart_location(struct amdgpu_device *adev, struct amdgpu_gmc
*mc)
+void amdgpu_gmc_gart_location(struct amdgpu_device *adev, struct amdgpu_gmc
*mc,
+ enum amdgpu_gart_placement gart_placement)
{
const uint64_t four_gb = 0x1ULL;
u64 size_af, size_bf;
@@ -287,11 +288,22 @@ void amdgpu_gmc_gart_location(struct amdgpu_device *adev,
struct amdgpu_gmc *mc)
mc->gart_size = max(size_bf, size_af);
}
- if ((size_bf >= mc->gart_size && size_bf < size_af) ||
- (size_af < mc->gart_size))
- mc->gart_start = 0;
- else
+ switch (gart_placement) {
+ case AMDGPU_GART_PLACEMENT_HIGH:
mc->gart_start = max_mc_address - mc->gart_size + 1;
+ break;
+ case AMDGPU_GART_PLACEMENT_LOW:
+ mc->gart_start = 0;
+ break;
+ case AMDGPU_GART_PLACEMENT_BEST_FIT:
+ default:
+ if ((size_bf >= mc->gart_size && size_bf < size_af) ||
+ (size_af < mc->gart_size))
+ mc->gart_start = 0;
+ else
+ mc->gart_start = max_mc_address - mc->gart_size + 1;
+ break;
+ }
mc->gart_start &= ~(four_gb - 1);
mc->gart_end = mc->gart_start + mc->gart_size - 1;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index f593259a66c3..e699d1ca8deb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -199,6 +199,12 @@ struct amdgpu_mem_partition_info {
#define INVALID_PFN-1
+enum amdgpu_gart_placement {
+ AMDGPU_GART_PLACEMENT_BEST_FIT = 0,
+ AMDGPU_GART_PLACEMENT_HIGH,
+ AMDGPU_GART_PLACEMENT_LOW,
+};
+
struct amdgpu_gmc {
/* FB's physical address in MMIO space (for CPU to
* map FB). This is different compared to the agp/
@@ -391,7 +397,8 @@ void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev,
struct amdgpu_gmc *mc
void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc
*mc,
u64 base);
void amdgpu_gmc_gart_location(struct amdgpu_device *adev,
- struct amdgpu_gmc *mc);
+ struct amdgpu_gmc *mc,
+ enum amdgpu_gart_placement gart_placement);
void amdgpu_gmc_agp_location(struct amdgpu_device *adev,
struct amdgpu_gmc *mc);
void amdgpu_gmc_set_agp_default(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index 70370b412d24..8e6e36279389 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -670,7 +670,7 @@ static void gmc_v10_0_vram_gtt_location(struct
amdgpu_device *adev,
base += adev->gmc.xgmi.physical_node_id *
adev->gmc.xgmi.node_segment_size;
amdgpu_gmc_vram_location(adev, >gmc, base);
- amdgpu_gmc_gart_location(adev, mc);
+ amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
if (!amdgpu_sriov_vf(adev))
amdgpu_gmc_agp_location(adev, mc);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
index d0a9ee2f12d3..d611d2efce3b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
@@ -634,7 +634,7 @@ static void gmc_v11_0_vram_gtt_location(struct
amdgpu_device *adev,
base = adev->mmhub.funcs->get_fb_location(adev);
amdgpu_gmc_vram_location(adev, >gmc, base);
- amdgpu_gmc_gart_location(adev, mc);
+ amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
[PATCH 2/2] drm/amdgpu/gmc11: set gart placement GC11
Needed to avoid a hardware issue. v2: force high for all GC11 parts for consistency (Alex) v3: rebase Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c index d611d2efce3b..07f50ca8d481 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c @@ -634,7 +634,7 @@ static void gmc_v11_0_vram_gtt_location(struct amdgpu_device *adev, base = adev->mmhub.funcs->get_fb_location(adev); amdgpu_gmc_vram_location(adev, >gmc, base); - amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT); + amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_HIGH); if (!amdgpu_sriov_vf(adev) || (amdgpu_ip_version(adev, GC_HWIP, 0) < IP_VERSION(11, 5, 0))) amdgpu_gmc_agp_location(adev, mc); -- 2.41.0
[PATCH] drm/amd: Move `enum drm_amdgpu_ta_fw_type` to uapi
Enum values used by the ioctl `AMDGPU_INFO_FW_VERSION`/`AMDGPU_INFO_FW_TA`
are not exported so clients need to keep their own copy of the definitions
while looking up firmware versions for the TA.
Move this to uapi instead.
Signed-off-by: Mario Limonciello
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 12
include/uapi/drm/amdgpu_drm.h | 12
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
index ae5fa61d2890..73a84af54d70 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
@@ -145,18 +145,6 @@ struct ta_firmware_header_v1_0 {
struct psp_fw_legacy_bin_desc securedisplay;
};
-enum ta_fw_type {
- TA_FW_TYPE_UNKOWN,
- TA_FW_TYPE_PSP_ASD,
- TA_FW_TYPE_PSP_XGMI,
- TA_FW_TYPE_PSP_RAS,
- TA_FW_TYPE_PSP_HDCP,
- TA_FW_TYPE_PSP_DTM,
- TA_FW_TYPE_PSP_RAP,
- TA_FW_TYPE_PSP_SECUREDISPLAY,
- TA_FW_TYPE_MAX_INDEX,
-};
-
/* version_major=2, version_minor=0 */
struct ta_firmware_header_v2_0 {
struct common_firmware_header header;
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 984fc16577ca..225dec3634f0 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -912,6 +912,18 @@ struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
#define AMDGPU_INFO_MMR_SH_INDEX_SHIFT 8
#define AMDGPU_INFO_MMR_SH_INDEX_MASK 0xff
+enum drm_amdgpu_ta_fw_type {
+ TA_FW_TYPE_UNKOWN,
+ TA_FW_TYPE_PSP_ASD,
+ TA_FW_TYPE_PSP_XGMI,
+ TA_FW_TYPE_PSP_RAS,
+ TA_FW_TYPE_PSP_HDCP,
+ TA_FW_TYPE_PSP_DTM,
+ TA_FW_TYPE_PSP_RAP,
+ TA_FW_TYPE_PSP_SECUREDISPLAY,
+ TA_FW_TYPE_MAX_INDEX,
+};
+
struct drm_amdgpu_query_fw {
/** AMDGPU_INFO_FW_* */
__u32 fw_type;
--
2.34.1
Re: [PATCH 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
On 27/09/2023 19:27, Felix Kuehling wrote: [+Mukul] On 2023-09-27 12:16, Arvind Yadav wrote: This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Cc: Christian Koenig Cc: Alex Deucher Signed-off-by: Shashank Sharma Signed-off-by: Arvind Yadav --- drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index 0d3d538b64eb..c327f7f604d7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -407,7 +407,16 @@ static int allocate_doorbell(struct qcm_process_device *qpd, q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev->adev, qpd->proc_doorbells, - q->doorbell_id); + 0); Looks like we're now always calling amdgpu_doorbell_index_on_bar with the third parameter = 0. So we could remove that parameter. It doesn't do us any good and only causes bugs if we use any non-0 value. Hey Felix, Actually this was happening because in usermode KFD library the doorbell-size is for non-SOC15() hardware is hard coded to 4 bytes. We added this fix just so that the library code doesn't need to be changed, but can still get aligned to kernel code. GFX Usermode queue code uses this doorbell-index parameter, and it gives us the right offset. Regards Shashank + + /* Adjust the absolute doorbell offset against the doorbell id considering +* the doorbell size of 32/64 bit. +*/ + if (!KFD_IS_SOC15(dev)) + q->properties.doorbell_off += q->doorbell_id; + else + q->properties.doorbell_off += q->doorbell_id * 2; This could be simplified and generalized as q->properties.doorbell_off += q->doorbell_id * dev->kfd->device_info.doorbell_size / 4; Agree, we can do this simplification. - Shashank Regards, Felix + return 0; }
Re: [PATCH 2/2] drm/amd/display: Fix null pointer dereference in error message
On 2023-09-27 10:07, Harry Wentland wrote:
>
>
> On 2023-09-27 01:23, Christian König wrote:
>> Am 26.09.23 um 15:09 schrieb Harry Wentland:
>>>
>>> On 2023-09-26 01:56, Cong Liu wrote:
This patch fixes a null pointer dereference in the error message that is
printed when the Display Core (DC) fails to initialize. The original
message includes the DC version number, which is undefined if the DC is
not initialized.
Fixes: 9788d087caff ("drm/amd/display: improve the message printed when
loading DC")
Signed-off-by: Cong Liu
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 8e98dda1e084..bf52a909f558 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1703,8 +1703,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
DRM_INFO("Display Core v%s initialized on %s\n", DC_VER,
dce_version_to_string(adev->dm.dc->ctx->dce_version));
} else {
- DRM_INFO("Display Core v%s failed to initialize on %s\n", DC_VER,
- dce_version_to_string(adev->dm.dc->ctx->dce_version));
+ DRM_INFO("Display Core failed to initialize with v%s!\n", DC_VER);
>>> There is value in printing the version number. Let's not remove it.
>>>
>>> Instead you can probably fix it by doing a NULL check on adev->dm.dc->ctx.
>>
>> But as far as I understand it adev->dm.dc->ctx will always be NULL in this
>> case.
>>
>
> Thanks, Christian. We're in the else of the NULL check for
> adev-dm.dc, so obviously we can't get the version. Silly me.
>
> Your patch is
> Reviewed-by: Harry Wentland
>
Patch is merged.
Harry
> Harry
>
>> Regards,
>> Christian.
>>
>>>
>>> Harry
>>>
goto error;
}
>>
>
Re: [PATCH 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
[+Mukul] On 2023-09-27 12:16, Arvind Yadav wrote: This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Cc: Christian Koenig Cc: Alex Deucher Signed-off-by: Shashank Sharma Signed-off-by: Arvind Yadav --- drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index 0d3d538b64eb..c327f7f604d7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -407,7 +407,16 @@ static int allocate_doorbell(struct qcm_process_device *qpd, q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev->adev, qpd->proc_doorbells, - q->doorbell_id); + 0); Looks like we're now always calling amdgpu_doorbell_index_on_bar with the third parameter = 0. So we could remove that parameter. It doesn't do us any good and only causes bugs if we use any non-0 value. + + /* Adjust the absolute doorbell offset against the doorbell id considering +* the doorbell size of 32/64 bit. +*/ + if (!KFD_IS_SOC15(dev)) + q->properties.doorbell_off += q->doorbell_id; + else + q->properties.doorbell_off += q->doorbell_id * 2; This could be simplified and generalized as q->properties.doorbell_off += q->doorbell_id * dev->kfd->device_info.doorbell_size / 4; Regards, Felix + return 0; }
[linux-next:master] BUILD REGRESSION 18030226a48de1fbfabf4ae16aaa2695a484254f
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 18030226a48de1fbfabf4ae16aaa2695a484254f Add linux-next specific files for 20230927 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202308282000.2xnh0k6d-...@intel.com https://lore.kernel.org/oe-kbuild-all/202308301211.2hhbgs2n-...@intel.com https://lore.kernel.org/oe-kbuild-all/202308301542.li3khkjl-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309122047.cri9yjrq-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309130213.msr7x2jz-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309192154.njnpfiy5-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309192314.vbsjiim5-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309212121.cul1ptra-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309212339.hxhbu2f1-...@intel.com https://lore.kernel.org/oe-kbuild-all/202309271719.sxq960r2-...@intel.com Error/Warning: (recently discovered and may have been fixed) aarch64-linux-ld: ice_dpll.c:(.text+0x1124): undefined reference to `ice_cgu_get_pin_type' aarch64-linux-ld: ice_dpll.c:(.text+0x122c): undefined reference to `ice_cgu_get_pin_freq_supp' aarch64-linux-ld: ice_lib.c:(.text+0x85a0): undefined reference to `ice_is_cgu_present' aarch64-linux-ld: ice_lib.c:(.text+0x85d0): undefined reference to `ice_is_clock_mux_present_e810t' arc-elf-ld: xfrm_algo.c:(.text+0x46c): undefined reference to `crypto_has_aead' arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' writing 3 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] drivers/cpufreq/sti-cpufreq.c:215:50: warning: '%d' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=] drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:3928: warning: Function parameter or member 'srf_updates' not described in 'could_mpcc_tree_change_for_active_pipes' drivers/net/ethernet/intel/ice/ice_dpll.c:1066: undefined reference to `ice_get_cgu_state' drivers/net/ethernet/intel/ice/ice_dpll.c:1666: undefined reference to `ice_cgu_get_pin_freq_supp' drivers/net/ethernet/intel/ice/ice_dpll.c:1802: undefined reference to `ice_get_cgu_rclk_pin_info' drivers/net/ethernet/intel/ice/ice_lib.c:3992: undefined reference to `ice_is_phy_rclk_present' drivers/net/ethernet/sfc/ethtool_common.c:278:32: warning: '%-24s' directive output may be truncated writing between 24 and 31 bytes into a region of size 25 [-Wformat-truncation=] drivers/net/ethernet/sfc/falcon/ethtool.c:229:32: warning: '%-24s' directive output may be truncated writing between 24 and 31 bytes into a region of size 25 [-Wformat-truncation=] drivers/net/ethernet/sfc/siena/ethtool_common.c:229:32: warning: '%-24s' directive output may be truncated writing between 24 and 31 bytes into a region of size 25 [-Wformat-truncation=] fs/bcachefs/bcachefs_format.h:215:25: warning: 'p' offset 3 in 'struct bkey' isn't aligned to 4 [-Wpacked-not-aligned] fs/bcachefs/bcachefs_format.h:217:25: warning: 'version' offset 27 in 'struct bkey' isn't aligned to 4 [-Wpacked-not-aligned] fs/proc/task_mmu.c:2105:3: error: implicit declaration of function 'pagemap_scan_backout_range'; did you mean 'pagemap_scan_push_range'? [-Werror=implicit-function-declaration] ice_dpll.c:(.text+0x1104): undefined reference to `ice_cgu_get_pin_name' ice_dpll.c:(.text+0x15b4): undefined reference to `ice_get_cgu_state' ice_dpll.c:(.text+0x2758): undefined reference to `ice_get_cgu_rclk_pin_info' ice_lib.c:(.text+0x855c): undefined reference to `ice_is_phy_rclk_present' include/linux/fortify-string.h:57:33: warning: writing 8 bytes into a region of size 0 [-Wstringop-overflow=] include/linux/fortify-string.h:65:33: warning: '__builtin_strcpy' source argument is the same as destination [-Wrestrict] include/linux/netlink.h:116:13: warning: ') out of range, only support...' directive output truncated writing 60 bytes into a region of size between 46 and 55 [-Wformat-truncation=] include/linux/netlink.h:116:13: warning: 'sfc: Unsupported: only suppo...' directive output truncated writing 104 bytes into a region of size 80 [-Wformat-truncation=] include/linux/netlink.h:116:6: warning: ') out of range, only support...' directive output truncated writing 60 bytes into a region of size between 46 and 55 [-Wformat-truncation=] include/linux/netlink.h:116:6: warning: 'sfc: Unsupported: only suppo...' directive output truncated writing 104 bytes into a region of size 80 [-Wformat-truncation=] kernel/bpf/helpers.c:1906:19: warning: no previous declaration for 'bpf_percpu_obj_new_impl' [-Wmissing-declarations] kernel/bpf/helpers.c:1942:18: warning: no previous declaration for 'bpf_percpu_obj_drop_impl' [-Wmissing-declarations] kernel/bpf/helpers.c:2477:18: warning: no previous declaration for 'bpf_throw' [-Wmissing-declarations] ld: drivers/net/ethernet/intel/ice/ice_dpll.c:1646: undefined reference
Re: [PATCH 2/2] drm/amdkfd: drop struct kfd_cu_info
On 2023-09-26 12:39, Alex Deucher wrote:
I think this was an abstraction back from when
kfd supported both radeon and amdgpu. Since we just
support amdgpu now, there is no more need for this and
we can use the amdgpu structures directly.
This also avoids having the kfd_cu_info structures on
the stack when inlining which can blow up the stack.
Cc: Arnd Bergmann
Signed-off-by: Alex Deucher
Thanks for this cleanup. The patch is
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c| 22 -
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h| 2 -
drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 28 +--
drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c | 28 +--
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 49 ---
.../gpu/drm/amd/include/kgd_kfd_interface.h | 14 --
6 files changed, 48 insertions(+), 95 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index 38b5457baded..d95fd76102d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
@@ -467,28 +467,6 @@ uint32_t amdgpu_amdkfd_get_max_engine_clock_in_mhz(struct
amdgpu_device *adev)
return 100;
}
-void amdgpu_amdkfd_get_cu_info(struct amdgpu_device *adev, struct kfd_cu_info *cu_info)
-{
- struct amdgpu_cu_info acu_info = adev->gfx.cu_info;
-
- memset(cu_info, 0, sizeof(*cu_info));
- if (sizeof(cu_info->cu_bitmap) != sizeof(acu_info.bitmap))
- return;
-
- cu_info->cu_active_number = acu_info.number;
- cu_info->cu_ao_mask = acu_info.ao_cu_mask;
- memcpy(_info->cu_bitmap[0], _info.bitmap[0],
- sizeof(cu_info->cu_bitmap));
- cu_info->num_shader_engines = adev->gfx.config.max_shader_engines;
- cu_info->num_shader_arrays_per_engine = adev->gfx.config.max_sh_per_se;
- cu_info->num_cu_per_sh = adev->gfx.config.max_cu_per_sh;
- cu_info->simd_per_cu = acu_info.simd_per_cu;
- cu_info->max_waves_per_simd = acu_info.max_waves_per_simd;
- cu_info->wave_front_size = acu_info.wave_front_size;
- cu_info->max_scratch_slots_per_cu = acu_info.max_scratch_slots_per_cu;
- cu_info->lds_size = acu_info.lds_size;
-}
-
int amdgpu_amdkfd_get_dmabuf_info(struct amdgpu_device *adev, int dma_buf_fd,
struct amdgpu_device **dmabuf_adev,
uint64_t *bo_size, void *metadata_buffer,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 609a6fefd85f..3ad8dc523b42 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -236,8 +236,6 @@ void amdgpu_amdkfd_get_local_mem_info(struct amdgpu_device
*adev,
uint64_t amdgpu_amdkfd_get_gpu_clock_counter(struct amdgpu_device *adev);
uint32_t amdgpu_amdkfd_get_max_engine_clock_in_mhz(struct amdgpu_device *adev);
-void amdgpu_amdkfd_get_cu_info(struct amdgpu_device *adev,
- struct kfd_cu_info *cu_info);
int amdgpu_amdkfd_get_dmabuf_info(struct amdgpu_device *adev, int dma_buf_fd,
struct amdgpu_device **dmabuf_adev,
uint64_t *bo_size, void *metadata_buffer,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 9459603804b9..0e792a8496d6 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -2038,11 +2038,12 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
uint32_t proximity_domain)
{
struct crat_header *crat_table = (struct crat_header *)pcrat_image;
+ struct amdgpu_gfx_config *gfx_info = >adev->gfx.config;
+ struct amdgpu_cu_info *cu_info = >adev->gfx.cu_info;
struct crat_subtype_generic *sub_type_hdr;
struct kfd_local_mem_info local_mem_info;
struct kfd_topology_device *peer_dev;
struct crat_subtype_computeunit *cu;
- struct kfd_cu_info cu_info;
int avail_size = *size;
uint32_t total_num_of_cu;
uint32_t nid = 0;
@@ -2086,21 +2087,20 @@ static int kfd_create_vcrat_image_gpu(void *pcrat_image,
cu->flags |= CRAT_CU_FLAGS_GPU_PRESENT;
cu->proximity_domain = proximity_domain;
- amdgpu_amdkfd_get_cu_info(kdev->adev, _info);
- cu->num_simd_per_cu = cu_info.simd_per_cu;
- cu->num_simd_cores = cu_info.simd_per_cu *
- (cu_info.cu_active_number / kdev->kfd->num_nodes);
- cu->max_waves_simd = cu_info.max_waves_per_simd;
+ cu->num_simd_per_cu = cu_info->simd_per_cu;
+ cu->num_simd_cores = cu_info->simd_per_cu *
+ (cu_info->number / kdev->kfd->num_nodes);
+ cu->max_waves_simd = cu_info->max_waves_per_simd;
- cu->wave_front_size =
Re: [PATCH 1/2] drm/amdkfd: reduce stack size in kfd_topology_add_device()
On 2023-09-26 12:39, Alex Deucher wrote:
kfd_topology.c:2082:1: warning: the frame size of 1440 bytes is larger than
1024 bytes
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2866
Cc: Arnd Bergmann
Signed-off-by: Alex Deucher
Reviewed-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index c8c75ff7cea8..3f9f882d3f5c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1918,7 +1918,7 @@ int kfd_topology_add_device(struct kfd_node *gpu)
{
uint32_t gpu_id;
struct kfd_topology_device *dev;
- struct kfd_cu_info cu_info;
+ struct kfd_cu_info *cu_info;
int res = 0;
int i;
const char *asic_name = amdgpu_asic_name[gpu->adev->asic_type];
@@ -1959,8 +1959,11 @@ int kfd_topology_add_device(struct kfd_node *gpu)
/* Fill-in additional information that is not available in CRAT but
* needed for the topology
*/
+ cu_info = kzalloc(sizeof(struct kfd_cu_info), GFP_KERNEL);
+ if (!cu_info)
+ return -ENOMEM;
- amdgpu_amdkfd_get_cu_info(dev->gpu->adev, _info);
+ amdgpu_amdkfd_get_cu_info(dev->gpu->adev, cu_info);
for (i = 0; i < KFD_TOPOLOGY_PUBLIC_NAME_SIZE-1; i++) {
dev->node_props.name[i] = __tolower(asic_name[i]);
@@ -1970,7 +1973,7 @@ int kfd_topology_add_device(struct kfd_node *gpu)
dev->node_props.name[i] = '\0';
dev->node_props.simd_arrays_per_engine =
- cu_info.num_shader_arrays_per_engine;
+ cu_info->num_shader_arrays_per_engine;
dev->node_props.gfx_target_version =
gpu->kfd->device_info.gfx_target_version;
@@ -2051,7 +2054,7 @@ int kfd_topology_add_device(struct kfd_node *gpu)
*/
if (dev->gpu->adev->asic_type == CHIP_CARRIZO) {
dev->node_props.simd_count =
- cu_info.simd_per_cu * cu_info.cu_active_number;
+ cu_info->simd_per_cu * cu_info->cu_active_number;
dev->node_props.max_waves_per_simd = 10;
}
@@ -2078,6 +2081,8 @@ int kfd_topology_add_device(struct kfd_node *gpu)
kfd_notify_gpu_change(gpu_id, 1);
+ kfree(cu_info);
+
return 0;
}
Re: [PATCH 2/2] drm/amdkfd: drop struct kfd_cu_info
On 2023-09-26 15:29, Arnd Bergmann wrote: On Tue, Sep 26, 2023, at 20:47, Deucher, Alexander wrote: From: Arnd Bergmann Subject: Re: [PATCH 2/2] drm/amdkfd: drop struct kfd_cu_info On Tue, Sep 26, 2023, at 18:39, Alex Deucher wrote: I think this was an abstraction back from when kfd supported both radeon and amdgpu. Since we just support amdgpu now, there is no more need for this and we can use the amdgpu structures directly. This also avoids having the kfd_cu_info structures on the stack when inlining which can blow up the stack. Cc: Arnd Bergmann Signed-off-by: Alex Deucher Nice cleanup! Acked-by: Arnd Bergmann I guess you could fold patch 1/2 into this as it removes all the added code from that anyway. I left it as a separate patch as I didn't get a chance to see when the stack warning appeared and figured it might be a good way to mitigate that on stable kernels if necessary without pulling in the whole rework, but if not, I can just squash it into the second patch. Makes sense. FWIW, I had never seen the warning before updating to linux-next this week from an older snapshot from last month. My guess is that one of the recent changes made gcc take different inlining decisions so we end up with two copies of the cu_info in the same stack frame, even though the fundamental problem was there already. I've seen this type of problem before because our data structures keep growing. When we need to support more GPUs, or bigger GPUs with more CUs, the arrays in those structures grow, and start blowing up the stack in functions that didn't have a problem before. Regards, Felix Arnd
Re: [PATCH 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
Adding felix.kuehl...@amd.com for review. Thanks ~Arvind On 9/27/2023 9:46 PM, Arvind Yadav wrote: This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Cc: Christian Koenig Cc: Alex Deucher Signed-off-by: Shashank Sharma Signed-off-by: Arvind Yadav --- drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index 0d3d538b64eb..c327f7f604d7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -407,7 +407,16 @@ static int allocate_doorbell(struct qcm_process_device *qpd, q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev->adev, qpd->proc_doorbells, - q->doorbell_id); + 0); + + /* Adjust the absolute doorbell offset against the doorbell id considering +* the doorbell size of 32/64 bit. +*/ + if (!KFD_IS_SOC15(dev)) + q->properties.doorbell_off += q->doorbell_id; + else + q->properties.doorbell_off += q->doorbell_id * 2; + return 0; }
Re: [PATCH 0/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
Adding felix.kuehl...@amd.com for review. Thanks ~Arvind On 9/27/2023 9:46 PM, Arvind Yadav wrote: On older chips, the absolute doorbell offset within the doorbell page is based on the queue ID. KFD is using queue ID and doorbell size to get an absolute doorbell offset in userspace. This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Arvind Yadav (1): drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-)
[PATCH 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Cc: Christian Koenig Cc: Alex Deucher Signed-off-by: Shashank Sharma Signed-off-by: Arvind Yadav --- drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c index 0d3d538b64eb..c327f7f604d7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c @@ -407,7 +407,16 @@ static int allocate_doorbell(struct qcm_process_device *qpd, q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev->adev, qpd->proc_doorbells, - q->doorbell_id); + 0); + + /* Adjust the absolute doorbell offset against the doorbell id considering +* the doorbell size of 32/64 bit. +*/ + if (!KFD_IS_SOC15(dev)) + q->properties.doorbell_off += q->doorbell_id; + else + q->properties.doorbell_off += q->doorbell_id * 2; + return 0; } -- 2.34.1
[PATCH 0/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8
On older chips, the absolute doorbell offset within the doorbell page is based on the queue ID. KFD is using queue ID and doorbell size to get an absolute doorbell offset in userspace. This patch is to adjust the absolute doorbell offset against the doorbell id considering the doorbell size of 32/64 bit. Arvind Yadav (1): drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) -- 2.34.1
Re: [PATCH 1/2] drm/amdgpu/gmc: add a way to force a particular placement for GART
On Wed, Sep 27, 2023 at 1:37 AM Christian König
wrote:
>
> I'm still not happy with moving the GART fixed to the end. We abandoned
> this for good reasons.
We didn't abandon it, there is still code which decides where to put
the gart based on the relative sizes of the address space before and
after vram. It really comes down to where the vbios decides to put
vram. If it starts somewhere lower in the future, gart will naturally
end up higher. Also windows always puts gart above vram.
>
> If we really go this way I would prefer to have this as parameter to the
> amdgpu_gmc_gart_location() function and not in the gmc structure.
I can do that.
Alex
>
> Regards,
> Christian.
>
> Am 26.09.23 um 19:30 schrieb Alex Deucher:
> > Ping on this series?
> >
> > On Thu, Sep 21, 2023 at 5:46 PM Alex Deucher
> > wrote:
> >> We normally place GART based on the location of VRAM and the
> >> available address space around that, but provide an option
> >> to force a particular location for hardware that needs it.
> >>
> >> Signed-off-by: Alex Deucher
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 19 +++
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 8
> >> 2 files changed, 23 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >> index f74a51a93ebb..d1d98488373b 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> >> @@ -287,11 +287,22 @@ void amdgpu_gmc_gart_location(struct amdgpu_device
> >> *adev, struct amdgpu_gmc *mc)
> >> mc->gart_size = max(size_bf, size_af);
> >> }
> >>
> >> - if ((size_bf >= mc->gart_size && size_bf < size_af) ||
> >> - (size_af < mc->gart_size))
> >> - mc->gart_start = 0;
> >> - else
> >> + switch (mc->gart_placement) {
> >> + case AMDGPU_GART_PLACEMENT_HIGH:
> >> mc->gart_start = max_mc_address - mc->gart_size + 1;
> >> + break;
> >> + case AMDGPU_GART_PLACEMENT_LOW:
> >> + mc->gart_start = 0;
> >> + break;
> >> + case AMDGPU_GART_PLACEMENT_BEST_FIT:
> >> + default:
> >> + if ((size_bf >= mc->gart_size && size_bf < size_af) ||
> >> + (size_af < mc->gart_size))
> >> + mc->gart_start = 0;
> >> + else
> >> + mc->gart_start = max_mc_address - mc->gart_size +
> >> 1;
> >> + break;
> >> + }
> >>
> >> mc->gart_start &= ~(four_gb - 1);
> >> mc->gart_end = mc->gart_start + mc->gart_size - 1;
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >> index dd0ede75e5d7..fcef057b9213 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> >> @@ -199,6 +199,12 @@ struct amdgpu_mem_partition_info {
> >>
> >> #define INVALID_PFN-1
> >>
> >> +enum amdgpu_gart_placement {
> >> + AMDGPU_GART_PLACEMENT_BEST_FIT = 0,
> >> + AMDGPU_GART_PLACEMENT_HIGH,
> >> + AMDGPU_GART_PLACEMENT_LOW,
> >> +};
> >> +
> >> struct amdgpu_gmc {
> >> /* FB's physical address in MMIO space (for CPU to
> >> * map FB). This is different compared to the agp/
> >> @@ -339,6 +345,8 @@ struct amdgpu_gmc {
> >> bool flush_tlb_needs_extra_type_0;
> >> bool flush_tlb_needs_extra_type_2;
> >> bool flush_pasid_uses_kiq;
> >> +
> >> + enum amdgpu_gart_placement gart_placement;
> >> };
> >>
> >> #define amdgpu_gmc_emit_flush_gpu_tlb(r, vmid, addr)
> >> (r)->adev->gmc.gmc_funcs->emit_flush_gpu_tlb((r), (vmid), (addr))
> >> --
> >> 2.41.0
> >>
>
Re: [PATCH] drm/amdkfd: Fix a race condition of vram buffer unref in svm code
On 9/27/2023 9:19 AM, Eric Huang wrote:
Caution: This message originated from an External Source. Use proper
caution when opening attachments, clicking links, or responding.
On 2023-09-26 23:00, Xiaogang.Chen wrote:
From: Xiaogang Chen
prange->svm_bo unref can happen in both mmu callback and a callback
after
migrate to system ram. Both are async call in different tasks. Sync
svm_bo
unref operation to avoid random "use-after-free".
Signed-off-by: Xiaogang.Chen
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 70aa882636ab..8e246e848018 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -637,6 +637,15 @@ void svm_range_vram_node_free(struct svm_range
*prange)
{
svm_range_bo_unref(prange->svm_bo);
prange->ttm_res = NULL;
Are above two lines not removed?
you are right, It was caused by copy-paste when edit. I tested it
without these two lines. I will remove these two lines when send to gerrit.
Thanks
Xiaogang
Regards,
Eric
+ /* serialize prange->svm_bo unref */
+ mutex_lock(>lock);
+ /* prange->svm_bo has not been unref */
+ if (prange->ttm_res) {
+ prange->ttm_res = NULL;
+ mutex_unlock(>lock);
+ svm_range_bo_unref(prange->svm_bo);
+ } else
+ mutex_unlock(>lock);
}
struct kfd_node *
Re: [PATCH] drm/amdkfd: Wait vm update fence after retry fault recovered
On 2023-09-26 16:43, Chen, Xiaogang
wrote:
On 9/22/2023 4:37 PM, Philip Yang wrote:
Caution: This message originated from an
External Source. Use proper caution when opening attachments,
clicking links, or responding.
Otherwise kfd flush tlb does nothing if vm update fence callback
doesn't
update vm->tlb_seq. H/W will generate retry fault again.
This works now because retry fault keep coming, recover will
update page
table again after AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING timeout
and flush
tlb.
I think what this patch does is waiting vm->last_update fence
at gpu page fault retry handler. I do not know what bug it tries
to fix. h/w will keep generating retry fault as long as vm page
table is not setup correctly, no matter kfd driver waits the fence
or not. vm page table eventually will be setup.
This issue was there, I notice it when implementing the
granularity bitmap_mapped flag for mGPUs, to skip the retry fault
if prange mapped on the GPU. The retry fault keep coming after
updating GPU page table, because restore_pages ->
svm_range_validate_and_map doesn't wait for vm update fence before
kfd_flush_tlb.
It is working now because we handle the same retry fault again
after timeout AMDGPU_SVM_RANGE_RETRY_FAULT_PENDING, and
kfd_flush_tlb does flush on the second time.
The issue only exist if using sdma update GPU page table, as no
fence if cpu update GPU page table.
There are several todo items to optimize this further:
A. After updating GPU page table, we only wait for fence and
flush tlb if updating existing mapping, or vm params.table_freed
(this needs amdgpu vm interface change).
B. Use sync to wait mGPUs update fences.
C. Use multiple workers to handle restore_pages.
There is a consequence I saw: if we wait vm page table update
fence it will delay gpu page fault handler exit. Then more h/w
interrupt vectors will be sent to sw ring, potentially cause the
ring overflow.
retry CAM filter, or sw filter drop the duplicate retry fault, to
prevent sw ring overflow.
Regards,
Philip
Regards
Xiaogang
Remove wait parameter in
svm_range_validate_and_map because it is
always called with true.
Signed-off-by: Philip Yang
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 70aa882636ab..61f4de1633a8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1447,7 +1447,7 @@ svm_range_map_to_gpu(struct
kfd_process_device *pdd, struct svm_range *prange,
static int
svm_range_map_to_gpus(struct svm_range *prange, unsigned long
offset,
unsigned long npages, bool readonly,
- unsigned long *bitmap, bool wait, bool
flush_tlb)
+ unsigned long *bitmap, bool flush_tlb)
{
struct kfd_process_device *pdd;
struct amdgpu_device *bo_adev = NULL;
@@ -1480,8 +1480,7 @@ svm_range_map_to_gpus(struct svm_range
*prange, unsigned long offset,
r = svm_range_map_to_gpu(pdd, prange, offset,
npages, readonly,
prange->dma_addr[gpuidx],
- bo_adev, wait ?
: NULL,
- flush_tlb);
+ bo_adev, ,
flush_tlb);
if (r)
break;
@@ -1605,7 +1604,7 @@ static void *kfd_svm_page_owner(struct
kfd_process *p, int32_t gpuidx)
*/
static int svm_range_validate_and_map(struct mm_struct *mm,
struct svm_range *prange,
int32_t gpuidx,
- bool intr, bool wait, bool
flush_tlb)
+ bool intr,
Re: [PATCH] drm/amdkfd: Fix a race condition of vram buffer unref in svm code
On 2023-09-26 23:00, Xiaogang.Chen wrote:
From: Xiaogang Chen
prange->svm_bo unref can happen in both mmu callback and a callback after
migrate to system ram. Both are async call in different tasks. Sync svm_bo
unref operation to avoid random "use-after-free".
Signed-off-by: Xiaogang.Chen
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 70aa882636ab..8e246e848018 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -637,6 +637,15 @@ void svm_range_vram_node_free(struct svm_range *prange)
{
svm_range_bo_unref(prange->svm_bo);
prange->ttm_res = NULL;
Are above two lines not removed?
Regards,
Eric
+ /* serialize prange->svm_bo unref */
+ mutex_lock(>lock);
+ /* prange->svm_bo has not been unref */
+ if (prange->ttm_res) {
+ prange->ttm_res = NULL;
+ mutex_unlock(>lock);
+ svm_range_bo_unref(prange->svm_bo);
+ } else
+ mutex_unlock(>lock);
}
struct kfd_node *
RE: [PATCH 1/2] drm/amdgpu/gmc: add a way to force a particular placement for GART
[AMD Official Use Only - General]
Series is
Reviewed-by: Yang Wang
Best Regards,
Kevin
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Wednesday, September 27, 2023 1:30 AM
To: Deucher, Alexander
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/amdgpu/gmc: add a way to force a particular
placement for GART
Ping on this series?
On Thu, Sep 21, 2023 at 5:46 PM Alex Deucher wrote:
>
> We normally place GART based on the location of VRAM and the available
> address space around that, but provide an option to force a particular
> location for hardware that needs it.
>
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 19 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 8
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index f74a51a93ebb..d1d98488373b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -287,11 +287,22 @@ void amdgpu_gmc_gart_location(struct amdgpu_device
> *adev, struct amdgpu_gmc *mc)
> mc->gart_size = max(size_bf, size_af);
> }
>
> - if ((size_bf >= mc->gart_size && size_bf < size_af) ||
> - (size_af < mc->gart_size))
> - mc->gart_start = 0;
> - else
> + switch (mc->gart_placement) {
> + case AMDGPU_GART_PLACEMENT_HIGH:
> mc->gart_start = max_mc_address - mc->gart_size + 1;
> + break;
> + case AMDGPU_GART_PLACEMENT_LOW:
> + mc->gart_start = 0;
> + break;
> + case AMDGPU_GART_PLACEMENT_BEST_FIT:
> + default:
> + if ((size_bf >= mc->gart_size && size_bf < size_af) ||
> + (size_af < mc->gart_size))
> + mc->gart_start = 0;
> + else
> + mc->gart_start = max_mc_address - mc->gart_size + 1;
> + break;
> + }
>
> mc->gart_start &= ~(four_gb - 1);
> mc->gart_end = mc->gart_start + mc->gart_size - 1; diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index dd0ede75e5d7..fcef057b9213 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -199,6 +199,12 @@ struct amdgpu_mem_partition_info {
>
> #define INVALID_PFN-1
>
> +enum amdgpu_gart_placement {
> + AMDGPU_GART_PLACEMENT_BEST_FIT = 0,
> + AMDGPU_GART_PLACEMENT_HIGH,
> + AMDGPU_GART_PLACEMENT_LOW,
> +};
> +
> struct amdgpu_gmc {
> /* FB's physical address in MMIO space (for CPU to
> * map FB). This is different compared to the agp/ @@ -339,6
> +345,8 @@ struct amdgpu_gmc {
> bool flush_tlb_needs_extra_type_0;
> bool flush_tlb_needs_extra_type_2;
> bool flush_pasid_uses_kiq;
> +
> + enum amdgpu_gart_placement gart_placement;
> };
>
> #define amdgpu_gmc_emit_flush_gpu_tlb(r, vmid, addr)
> (r)->adev->gmc.gmc_funcs->emit_flush_gpu_tlb((r), (vmid), (addr))
> --
> 2.41.0
>
RE: [PATCH] drm/amd/pm: delete dead code
[AMD Official Use Only - General] Thanks. Reviewed-by: Yang Wang Best Regards, Kevin -Original Message- From: Dan Carpenter Sent: Wednesday, September 27, 2023 8:38 PM To: Evan Quan ; Wang, Yang(Kevin) Cc: Deucher, Alexander ; Koenig, Christian ; Pan, Xinhui ; David Airlie ; Daniel Vetter ; Lazar, Lijo ; Kamal, Asad ; Zhang, Hawking ; Wang, Yang(Kevin) ; amd-gfx@lists.freedesktop.org; kernel-janit...@vger.kernel.org Subject: [PATCH] drm/amd/pm: delete dead code "ret" was checked earlier inside the loop, so we know it is zero here. No need to check a second time. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 3 --- 1 file changed, 3 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 11a6cd96c601..0ffe55e713f3 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 @@ -2346,9 +2346,6 @@ static int mca_get_mca_entry(struct amdgpu_device *adev, enum amdgpu_mca_error_t return ret; } - if (ret) - return ret; - entry->idx = idx; entry->type = type; -- 2.39.2
Re: [PATCH 2/2] drm/amd/display: Fix null pointer dereference in error message
On 2023-09-27 01:23, Christian König wrote:
> Am 26.09.23 um 15:09 schrieb Harry Wentland:
>>
>> On 2023-09-26 01:56, Cong Liu wrote:
>>> This patch fixes a null pointer dereference in the error message that is
>>> printed when the Display Core (DC) fails to initialize. The original
>>> message includes the DC version number, which is undefined if the DC is
>>> not initialized.
>>>
>>> Fixes: 9788d087caff ("drm/amd/display: improve the message printed when
>>> loading DC")
>>> Signed-off-by: Cong Liu
>>> ---
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 8e98dda1e084..bf52a909f558 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -1703,8 +1703,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>> DRM_INFO("Display Core v%s initialized on %s\n", DC_VER,
>>> dce_version_to_string(adev->dm.dc->ctx->dce_version));
>>> } else {
>>> - DRM_INFO("Display Core v%s failed to initialize on %s\n", DC_VER,
>>> - dce_version_to_string(adev->dm.dc->ctx->dce_version));
>>> + DRM_INFO("Display Core failed to initialize with v%s!\n", DC_VER);
>> There is value in printing the version number. Let's not remove it.
>>
>> Instead you can probably fix it by doing a NULL check on adev->dm.dc->ctx.
>
> But as far as I understand it adev->dm.dc->ctx will always be NULL in this
> case.
>
Thanks, Christian. We're in the else of the NULL check for
adev-dm.dc, so obviously we can't get the version. Silly me.
Your patch is
Reviewed-by: Harry Wentland
Harry
> Regards,
> Christian.
>
>>
>> Harry
>>
>>> goto error;
>>> }
>>>
>
Re: [PATCH] drm/amdkfd: Fix a race condition of vram buffer unref in svm code
On 2023-09-26 23:00, Xiaogang.Chen
wrote:
From: Xiaogang Chen
prange->svm_bo unref can happen in both mmu callback and a callback after
migrate to system ram. Both are async call in different tasks. Sync svm_bo
unref operation to avoid random "use-after-free".
Signed-off-by: Xiaogang.Chen
Reviewed-by: Philip Yang
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 70aa882636ab..8e246e848018 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -637,6 +637,15 @@ void svm_range_vram_node_free(struct svm_range *prange)
{
svm_range_bo_unref(prange->svm_bo);
prange->ttm_res = NULL;
+ /* serialize prange->svm_bo unref */
+ mutex_lock(>lock);
+ /* prange->svm_bo has not been unref */
+ if (prange->ttm_res) {
+ prange->ttm_res = NULL;
+ mutex_unlock(>lock);
+ svm_range_bo_unref(prange->svm_bo);
+ } else
+ mutex_unlock(>lock);
}
struct kfd_node *
Re: [PATCH 15/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for ALS
Hi Ilpo,
On 9/27/2023 7:03 PM, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
>
>> From: Basavaraj Natikar
>>
>> AMDSFH has information about the Ambient light via the Ambient
>> Light Sensor (ALS) which is part of the AMD sensor fusion hub.
>> Add PMF and AMDSFH interface to get this information.
>>
>> Co-developed-by: Shyam Sundar S K
>> Signed-off-by: Shyam Sundar S K
>> Signed-off-by: Basavaraj Natikar
>> ---
>> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 1 +
>> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 6 ++
>> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c| 20 +++
>> drivers/platform/x86/amd/pmf/spc.c| 5 +
>> include/linux/amd-pmf-io.h| 2 ++
>> 5 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> index cd57037bf217..a1950bc6e6ce 100644
>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> @@ -39,6 +39,7 @@ struct amd_mp2_sensor_info {
>>
>> struct sfh_dev_status {
>> bool is_hpd_present;
>> +bool is_als_present;
>> };
>>
>> struct amd_mp2_dev {
>> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> index 9c623456ee12..d8dad39d68b5 100644
>> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> @@ -77,6 +77,9 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev
>> *privdata)
>> case HPD_IDX:
>> privdata->dev_en.is_hpd_present = false;
>> break;
>> +case ALS_IDX:
>> +privdata->dev_en.is_als_present = false;
>> +break;
>> }
>>
>> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
>> @@ -188,6 +191,9 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev
>> *privdata)
>> case HPD_IDX:
>> privdata->dev_en.is_hpd_present = true;
>> break;
>> +case ALS_IDX:
>> +privdata->dev_en.is_als_present = true;
>> +break;
>> }
>> }
>> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
>> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> index 63a5bbca5a09..2f8200fc3062 100644
>> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> @@ -94,12 +94,32 @@ static int amd_sfh_hpd_info(u8 *user_present)
>> return -ENODEV;
>> }
>>
>> +static int amd_sfh_als_info(u32 *ambient_light)
>> +{
>> +if (emp2 && emp2->dev_en.is_als_present) {
>> +struct sfh_als_data als_data;
>> +void __iomem *sensoraddr;
>> +
>> +sensoraddr = emp2->vsbase +
>> +(ALS_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) +
>> +OFFSET_SENSOR_DATA_DEFAULT;
>> +memcpy_fromio(_data, sensoraddr, sizeof(struct
>> sfh_als_data));
>> +*ambient_light = float_to_int(als_data.lux);
>> +
>> +return 0;
>> +}
>> +
>> +return -ENODEV;
>> +}
>> +
>> int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type
>> op)
>> {
>> if (sfh_info) {
>> switch (op) {
>> case MT_HPD:
>> return amd_sfh_hpd_info(_info->user_present);
>> +case MT_ALS:
>> +return amd_sfh_als_info(_info->ambient_light);
>> }
>> }
>> return -1;
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c
>> b/drivers/platform/x86/amd/pmf/spc.c
>> index 97293ae25cf5..8e19b351e76f 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -49,6 +49,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev,
>> struct ta_pmf_enact_table *
>> "Connected" : "disconnected/unknown");
>> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" :
>> "Open");
>> dev_dbg(dev->dev, "User Presence : %s\n", in->ev_info.user_present ?
>> "Present" : "Away");
>> +dev_dbg(dev->dev, "Ambient Light : %d\n", in->ev_info.ambient_light);
>
> %d vs u32
Thank you very much for your feedback. I will respin a new version
soon addressing your review remarks.
Thanks,
Shyam
>
>> dev_dbg(dev->dev, " TA inputs END \n");
>> }
>> #else
>> @@ -161,6 +162,10 @@ static void amd_pmf_get_sensor_info(struct amd_pmf_dev
>> *dev, struct ta_pmf_enact
>> {
>> struct amd_sfh_info sfh_info;
>>
>> +/* get ALS data */
>> +amd_get_sfh_info(_info, MT_ALS);
>> +in->ev_info.ambient_light = sfh_info.ambient_light;
>> +
>> /* get HPD data */
>>
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
Hi Hans,
On 9/27/2023 6:34 PM, Hans de Goede wrote:
> HI,
>
> On 9/26/23 15:17, Christian König wrote:
>> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 9/26/23 13:24, Shyam Sundar S K wrote:
Hi Hans,
On 9/26/2023 4:05 PM, Hans de Goede wrote:
> Hi,
>
> On 9/22/23 19:50, Shyam Sundar S K wrote:
>> For the Smart PC Solution to fully work, it has to enact to the actions
>> coming from TA. Add the initial code path for set interface to AMDGPU.
>>
>> Co-developed-by: Mario Limonciello
>> Signed-off-by: Mario Limonciello
>> Signed-off-by: Shyam Sundar S K
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
>> include/linux/amd-pmf-io.h | 1 +
>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> index 232d11833ddc..5c567bff0548 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>> +
>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
>> +{
>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>> + struct backlight_device *bd;
>> +
>> + if (!(adev->flags & AMD_IS_APU)) {
>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>> + return -ENODEV;
>> + }
>> +
>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>> + if (!bd)
>> + return -ENODEV;
> This assumes that the backlight is always controller by the amdgpu's
> native backlight driver, but it might e.g. also be handled by
> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
> nvidia dgpu).
PMF is meant for AMD APUs(atleast for now) and the _HID will only be
made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
should be safe, right?
>>> Users can pass say acpi_backlight=video and use the acpi_video
>>> driver for backlight control instead of the native GPU backlight
>>> control.
>>>
> For now what should be done here is to call
> acpi_video_get_backlight_type()
> and then translate the return value from this into a backlight-type:
>
> acpi_backlight_video -> BACKLIGHT_FIRMWARE
> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
> acpi_backlight_native, -> BACKLIGHT_RAW
> acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>
I can add this change in the v2, do you insist on this?
>>> Insist is a strong word, but I think that it is a good idea to have
>>> this. Evenutally it looks like this code will need to either integrate with
>>> the drm drivers lot more; or the drm core needs to export some special
>>> hooks for this which the PMF code can then call.
>>>
>>> Actually thinking more about this, I think that the right thing to do
>>> here is make some code register brightness control as a cooling device
>>> (which I think is already done in some cases) and then have the PMF
>>> code use the cooling-device APIs for this.
>>>
>>> IMHO that would be a much cleaner solution then this hack.
>>
>> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
>
> Shyam, the cooling device interface is defined in:
>
> include/linux/thermal.h
>
> And then look for cooling_device .
>
> An example of code registering a cooling_device for backlight control is:
>
> drivers/acpi/acpi_video.c
>
> and then specifically the code starting around line 257 with:
>
> video_get_max_state()
>
> until
>
> static const struct thermal_cooling_device_ops video_cooling_ops = {
> ...
>
> And the code around line 1750 for actually registering the cooling-dev.
>
> To use the cooling_device interface witt amdgpu's native backlight control
> you will need to make the amdgpu backlight control register a cooling-device
> for this in a similar manner.
>
Thank you for pointing to the references. I will address the remarks
from Ilpo and respin a new version soon.
Thanks,
Shyam
> Regards,
>
> Hans
>
>
>
>
>>
>> Apart from that what exactly is this thing supposed to do? Prevent
>> overheating by reducing the brightness?
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
Thanks,
Shyam
> Also I'm worried about probe order here, this code currently assumes
> that the GPU or other backlight driver has loaded before this
Re: [PATCH 15/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for ALS
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> From: Basavaraj Natikar
>
> AMDSFH has information about the Ambient light via the Ambient
> Light Sensor (ALS) which is part of the AMD sensor fusion hub.
> Add PMF and AMDSFH interface to get this information.
>
> Co-developed-by: Shyam Sundar S K
> Signed-off-by: Shyam Sundar S K
> Signed-off-by: Basavaraj Natikar
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 1 +
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 6 ++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c| 20 +++
> drivers/platform/x86/amd/pmf/spc.c| 5 +
> include/linux/amd-pmf-io.h| 2 ++
> 5 files changed, 34 insertions(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> index cd57037bf217..a1950bc6e6ce 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> @@ -39,6 +39,7 @@ struct amd_mp2_sensor_info {
>
> struct sfh_dev_status {
> bool is_hpd_present;
> + bool is_als_present;
> };
>
> struct amd_mp2_dev {
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> index 9c623456ee12..d8dad39d68b5 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> @@ -77,6 +77,9 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev
> *privdata)
> case HPD_IDX:
> privdata->dev_en.is_hpd_present = false;
> break;
> + case ALS_IDX:
> + privdata->dev_en.is_als_present = false;
> + break;
> }
>
> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
> @@ -188,6 +191,9 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev
> *privdata)
> case HPD_IDX:
> privdata->dev_en.is_hpd_present = true;
> break;
> + case ALS_IDX:
> + privdata->dev_en.is_als_present = true;
> + break;
> }
> }
> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> index 63a5bbca5a09..2f8200fc3062 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> @@ -94,12 +94,32 @@ static int amd_sfh_hpd_info(u8 *user_present)
> return -ENODEV;
> }
>
> +static int amd_sfh_als_info(u32 *ambient_light)
> +{
> + if (emp2 && emp2->dev_en.is_als_present) {
> + struct sfh_als_data als_data;
> + void __iomem *sensoraddr;
> +
> + sensoraddr = emp2->vsbase +
> + (ALS_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) +
> + OFFSET_SENSOR_DATA_DEFAULT;
> + memcpy_fromio(_data, sensoraddr, sizeof(struct
> sfh_als_data));
> + *ambient_light = float_to_int(als_data.lux);
> +
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
> +
> int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op)
> {
> if (sfh_info) {
> switch (op) {
> case MT_HPD:
> return amd_sfh_hpd_info(_info->user_present);
> + case MT_ALS:
> + return amd_sfh_als_info(_info->ambient_light);
> }
> }
> return -1;
> diff --git a/drivers/platform/x86/amd/pmf/spc.c
> b/drivers/platform/x86/amd/pmf/spc.c
> index 97293ae25cf5..8e19b351e76f 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -49,6 +49,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *
> "Connected" : "disconnected/unknown");
> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" :
> "Open");
> dev_dbg(dev->dev, "User Presence : %s\n", in->ev_info.user_present ?
> "Present" : "Away");
> + dev_dbg(dev->dev, "Ambient Light : %d\n", in->ev_info.ambient_light);
%d vs u32
> dev_dbg(dev->dev, " TA inputs END \n");
> }
> #else
> @@ -161,6 +162,10 @@ static void amd_pmf_get_sensor_info(struct amd_pmf_dev
> *dev, struct ta_pmf_enact
> {
> struct amd_sfh_info sfh_info;
>
> + /* get ALS data */
> + amd_get_sfh_info(_info, MT_ALS);
> + in->ev_info.ambient_light = sfh_info.ambient_light;
> +
> /* get HPD data */
> amd_get_sfh_info(_info, MT_HPD);
> switch (sfh_info.user_present) {
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> index 4f82973f6ad2..dac0af573a16 100644
> --- a/include/linux/amd-pmf-io.h
> +++ b/include/linux/amd-pmf-io.h
> @@ -31,6 +31,7 @@
Re: [PATCH 14/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for HPD
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> From: Basavaraj Natikar
>
> AMDSFH has information about the User presence information via the Human
> Presence Detection (HPD) sensor which is part of the AMD sensor fusion hub.
> Add PMF and AMDSFH interface to get this information.
>
> Co-developed-by: Shyam Sundar S K
> Signed-off-by: Shyam Sundar S K
> Signed-off-by: Basavaraj Natikar
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 5
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 2 +-
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 11
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c| 28 +++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h| 1 +
> drivers/platform/x86/amd/pmf/spc.c| 21 ++
> include/linux/amd-pmf-io.h| 22 ++-
> 7 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> index 2643bb14fee2..cd57037bf217 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> @@ -37,6 +37,10 @@ struct amd_mp2_sensor_info {
> dma_addr_t dma_address;
> };
>
> +struct sfh_dev_status {
> + bool is_hpd_present;
> +};
> +
> struct amd_mp2_dev {
> struct pci_dev *pdev;
> struct amdtp_cl_data *cl_data;
> @@ -47,6 +51,7 @@ struct amd_mp2_dev {
> struct amd_input_data in_data;
> /* mp2 active control status */
> u32 mp2_acs;
> + struct sfh_dev_status dev_en;
> };
>
> struct amd_mp2_ops {
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> index 06bdcf072d10..d7467c41ad3b 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> @@ -132,7 +132,7 @@ static void get_common_inputs(struct
> common_input_property *common, int report_i
> common->event_type = HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM;
> }
>
> -static int float_to_int(u32 flt32_val)
> +int float_to_int(u32 flt32_val)
This is way too generic name to be exposed by a driver, add proper
prefix for it to make sure it never hits a compile problem.
> {
> int fraction, shift, mantissa, sign, exp, zeropre;
>
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> index e9c6413af24a..9c623456ee12 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> @@ -73,6 +73,12 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev
> *privdata)
> int i, status;
>
> for (i = 0; i < cl_data->num_hid_devices; i++) {
> + switch (cl_data->sensor_idx[i]) {
> + case HPD_IDX:
> + privdata->dev_en.is_hpd_present = false;
> + break;
> + }
> +
> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
> privdata->mp2_ops->stop(privdata,
> cl_data->sensor_idx[i]);
> status = amd_sfh_wait_for_response
> @@ -178,6 +184,11 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev
> *privdata)
> rc = amdtp_hid_probe(i, cl_data);
> if (rc)
> goto cleanup;
> + switch (cl_data->sensor_idx[i]) {
> + case HPD_IDX:
> + privdata->dev_en.is_hpd_present = true;
> + break;
> + }
> }
> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> cl_data->sensor_idx[i],
> get_sensor_name(cl_data->sensor_idx[i]),
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> index 4f81ef2d4f56..63a5bbca5a09 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> @@ -7,11 +7,14 @@
> *
> * Author: Basavaraj Natikar
> */
> +#include
> #include
> #include
>
> #include "amd_sfh_interface.h"
>
> +static struct amd_mp2_dev *emp2;
> +
> static int amd_sfh_wait_response(struct amd_mp2_dev *mp2, u8 sid, u32 cmd_id)
> {
> struct sfh_cmd_response cmd_resp;
> @@ -76,4 +79,29 @@ static struct amd_mp2_ops amd_sfh_ops = {
> void sfh_interface_init(struct amd_mp2_dev *mp2)
> {
> mp2->mp2_ops = _sfh_ops;
> + emp2 = mp2;
> +}
> +
> +static int amd_sfh_hpd_info(u8 *user_present)
> +{
> + if (emp2 && emp2->dev_en.is_hpd_present) {
> + struct hpd_status hpdstatus;
> +
> + hpdstatus.val = readl(emp2->mmio + AMD_C2P_MSG(4));
> + *user_present = hpdstatus.shpd.presence;
> + return 0;
> + }
> + return -ENODEV;
> +}
> +
> +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> For the Smart PC Solution to fully work, it has to enact to the actions
> coming from TA. Add the initial code path for set interface to AMDGPU.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
> include/linux/amd-pmf-io.h | 1 +
> 4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> index 232d11833ddc..5c567bff0548 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> return 0;
> }
> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> +
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct backlight_device *bd;
> +
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + backlight_device_set_brightness(bd, pmf->brightness);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 9032df4ba48a..ce89cc0daa5a 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -73,6 +73,7 @@
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> #define PMF_POLICY_SYSTEM_STATE 9
> +#define PMF_POLICY_DISPLAY_BRIGHTNESS12
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + unsigned long display_brightness;
> enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1608996654e8..eef83a4c 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev
> *dev, u16 event)
> return 0;
> }
>
> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
Changing return type but no caller is changed to handle the error??
> {
> u32 val, event = 0;
> - int idx;
> + int idx, ret;
>
> for (idx = 0; idx < out->actions_count; idx++) {
> val = out->actions_list[idx].value;
> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev
> *dev, struct ta_pmf_enact_
> dev->prev_data->system_state = 0;
> }
> break;
> +
> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
> + ret = amd_pmf_get_gfx_data(>gfx_data);
> + if (ret)
> + return ret;
> +
> + dev->prev_data->display_brightness =
> dev->gfx_data.brightness;
> + if (dev->prev_data->display_brightness != val) {
> + dev->gfx_data.brightness = val;
> + amd_pmf_set_gfx_data(>gfx_data);
> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS :
> %d\n", val);
> + }
> + break;
> }
> }
> +
> + return 0;
> }
>
> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> index a2d4af231362..ecae387ddaa6 100644
> --- a/include/linux/amd-pmf-io.h
> +++ b/include/linux/amd-pmf-io.h
> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data {
> };
>
> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
> #endif
>
--
i.
RE: [PATCH] drm/amd/display: enable S/G display for for recent APUs by default
[AMD Official Use Only - General]
Good idea ! Thanks.
-Original Message-
From: Alex Deucher
Sent: Wednesday, September 27, 2023 9:13 PM
To: Koenig, Christian
Cc: Zhang, Yifan ; amd-gfx@lists.freedesktop.org;
Deucher, Alexander ; Wentland, Harry
Subject: Re: [PATCH] drm/amd/display: enable S/G display for for recent APUs by
default
On Wed, Sep 27, 2023 at 8:26 AM Christian König
wrote:
>
> Am 27.09.23 um 07:41 schrieb Yifan Zhang:
> > With S/G display becomes stable, enable S/G display for recent APUs
> > by default rather than white list.
> >
> > Signed-off-by: Yifan Zhang
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 43 +--
> > 1 file changed, 10 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 7e6a693d6369..241fd8defdee 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -1626,41 +1626,18 @@ static int amdgpu_dm_init(struct amdgpu_device
> > *adev)
> > break;
> > }
> >
> > - switch (adev->asic_type) {
> > - case CHIP_CARRIZO:
> > - case CHIP_STONEY:
> > + if ((adev->asic_type == CHIP_CARRIZO ||
> > + adev->asic_type == CHIP_STONEY ||
> > + ((adev->flags & AMD_IS_APU) &&
> > + amdgpu_ip_version(adev, DCE_HWIP, 0) >= IP_VERSION(1, 0, 0) &&
> > + !(adev->apu_flags & AMD_APU_IS_RAVEN))) &&
> > + amdgpu_sg_display != 0) {
>
> Looks like a good idea to me, but please double check your coding
> style settings. The second line of an "if (" should be indented so
> that it starts after the "(" of the previous line.
>
> Additional to that this check actually looks quite messy to me and
> should probably be converted to always using the IP version instead of
> the asic_type.
Or just explicitly disable it on the handful or pre-CZ chips that don't
actually support it.
E.g.,
switch (asic_type) {
case CHIP_KAVERI:
case CHIP_KABINI:
case CHIP_MULLINS:
vm = false;
break;
case CHIP_RAVEN:
if (apu_flags & AMD_APU_IS_RAVEN)
vm = false;
else
vm = amdgpu_sg_display != 0;
break;
default:
vm = (amdgpu_sg_display != 0) && (flags && AMD_IS_APU);
break;
}
>
> Regards,
> Christian.
>
> > init_data.flags.gpu_vm_support = true;
> > - break;
> > - default:
> > - switch (amdgpu_ip_version(adev, DCE_HWIP, 0)) {
> > - case IP_VERSION(1, 0, 0):
> > - case IP_VERSION(1, 0, 1):
> > - /* enable S/G on PCO and RV2 */
> > - if ((adev->apu_flags & AMD_APU_IS_RAVEN2) ||
> > - (adev->apu_flags & AMD_APU_IS_PICASSO))
> > - init_data.flags.gpu_vm_support = true;
> > - break;
> > - case IP_VERSION(2, 1, 0):
> > - case IP_VERSION(3, 0, 1):
> > - case IP_VERSION(3, 1, 2):
> > - case IP_VERSION(3, 1, 3):
> > - case IP_VERSION(3, 1, 4):
> > - case IP_VERSION(3, 1, 5):
> > - case IP_VERSION(3, 1, 6):
> > - case IP_VERSION(3, 5, 0):
> > - init_data.flags.gpu_vm_support = true;
> > - break;
> > - default:
> > - break;
> > - }
> > - break;
> > - }
> > - if (init_data.flags.gpu_vm_support &&
> > - (amdgpu_sg_display == 0))
> > - init_data.flags.gpu_vm_support = false;
> > -
> > - if (init_data.flags.gpu_vm_support)
> > adev->mode_info.gpu_vm_support = true;
> > + } else {
> > + init_data.flags.gpu_vm_support = false;
> > + adev->mode_info.gpu_vm_support = false;
> > + }
> >
> > if (amdgpu_dc_feature_mask & DC_FBC_MASK)
> > init_data.flags.fbc_support = true;
>
Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
HI,
On 9/26/23 15:17, Christian König wrote:
> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>> Hi,
>>
>> On 9/26/23 13:24, Shyam Sundar S K wrote:
>>> Hi Hans,
>>>
>>> On 9/26/2023 4:05 PM, Hans de Goede wrote:
Hi,
On 9/22/23 19:50, Shyam Sundar S K wrote:
> For the Smart PC Solution to fully work, it has to enact to the actions
> coming from TA. Add the initial code path for set interface to AMDGPU.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +
> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 19 +--
> include/linux/amd-pmf-io.h | 1 +
> 4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> index 232d11833ddc..5c567bff0548 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> return 0;
> }
> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> +
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct backlight_device *bd;
> +
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
This assumes that the backlight is always controller by the amdgpu's
native backlight driver, but it might e.g. also be handled by
eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
nvidia dgpu).
>>> PMF is meant for AMD APUs(atleast for now) and the _HID will only be
>>> made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
>>> should be safe, right?
>> Users can pass say acpi_backlight=video and use the acpi_video
>> driver for backlight control instead of the native GPU backlight
>> control.
>>
For now what should be done here is to call acpi_video_get_backlight_type()
and then translate the return value from this into a backlight-type:
acpi_backlight_video -> BACKLIGHT_FIRMWARE
acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
acpi_backlight_native, -> BACKLIGHT_RAW
acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>>> I can add this change in the v2, do you insist on this?
>> Insist is a strong word, but I think that it is a good idea to have
>> this. Evenutally it looks like this code will need to either integrate with
>> the drm drivers lot more; or the drm core needs to export some special
>> hooks for this which the PMF code can then call.
>>
>> Actually thinking more about this, I think that the right thing to do
>> here is make some code register brightness control as a cooling device
>> (which I think is already done in some cases) and then have the PMF
>> code use the cooling-device APIs for this.
>>
>> IMHO that would be a much cleaner solution then this hack.
>
> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
Shyam, the cooling device interface is defined in:
include/linux/thermal.h
And then look for cooling_device .
An example of code registering a cooling_device for backlight control is:
drivers/acpi/acpi_video.c
and then specifically the code starting around line 257 with:
video_get_max_state()
until
static const struct thermal_cooling_device_ops video_cooling_ops = {
...
And the code around line 1750 for actually registering the cooling-dev.
To use the cooling_device interface witt amdgpu's native backlight control
you will need to make the amdgpu backlight control register a cooling-device
for this in a similar manner.
Regards,
Hans
>
> Apart from that what exactly is this thing supposed to do? Prevent
> overheating by reducing the brightness?
>
> Regards,
> Christian.
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> Thanks,
>>> Shyam
>>>
Also I'm worried about probe order here, this code currently assumes
that the GPU or other backlight driver has loaded before this runs,
which is not necessarily the case.
I think that if the backlight_device_get_by_type() fails this
should be retried say every 10 seconds from some delayed workqueue
for at least a couple of minutes after boot.
Regards,
Hans
> +
> + backlight_device_set_brightness(bd,
Re: [PATCH 12/15] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> In order to provide GPU inputs to TA for the Smart PC solution to work, we
> need to have interface between the PMF driver and the AMDGPU driver.
>
> Add the initial code path for get interface from AMDGPU.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 70 +
> drivers/platform/x86/amd/pmf/Kconfig| 1 +
> drivers/platform/x86/amd/pmf/core.c | 1 +
> drivers/platform/x86/amd/pmf/pmf.h | 4 ++
> drivers/platform/x86/amd/pmf/spc.c | 13 +
> drivers/platform/x86/amd/pmf/tee-if.c | 22
> include/linux/amd-pmf-io.h | 28 ++
> 9 files changed, 142 insertions(+)
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> create mode 100644 include/linux/amd-pmf-io.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 384b798a9bad..7fafccefbd7a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>
> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o
>
> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o
> +
> # add asic specific block
> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \
> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index dc2d53081e80..475f3e248f35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -50,6 +50,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> new file mode 100644
> index ..232d11833ddc
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> +
> + * * Author: Shyam Sundar S K
> + */
> +
> +#include
> +#include "amdgpu.h"
> +
> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct drm_mode_config *mode_config = _dev->mode_config;
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct drm_connector_list_iter iter;
> + struct drm_connector *connector;
> + struct backlight_device *bd;
> + int i = 0;
> +
> + /* reset the count to zero */
> + pmf->display_count = 0;
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + pmf->brightness = backlight_get_brightness(bd);
> +
> + mutex_lock(_config->mutex);
> + drm_connector_list_iter_begin(drm_dev, );
> +
> + drm_for_each_connector_iter(connector, ) {
> + if (i > MAX_SUPPORTED)
> + break;
I'd put this below right after i++.
> + if (connector->status == connector_status_connected) {
> + pmf->con_status[i] = connector->status;
> + pmf->connector_type[i] = connector->connector_type;
> + pmf->display_count++;
> + }
> + i++;
> + }
> + drm_connector_list_iter_end();
> + mutex_unlock(_config->mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
> b/drivers/platform/x86/amd/pmf/Kconfig
>
Re: [PATCH] drm/amd/display: enable S/G display for for recent APUs by default
On Wed, Sep 27, 2023 at 8:26 AM Christian König
wrote:
>
> Am 27.09.23 um 07:41 schrieb Yifan Zhang:
> > With S/G display becomes stable, enable S/G display for recent APUs
> > by default rather than white list.
> >
> > Signed-off-by: Yifan Zhang
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 43 +--
> > 1 file changed, 10 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 7e6a693d6369..241fd8defdee 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -1626,41 +1626,18 @@ static int amdgpu_dm_init(struct amdgpu_device
> > *adev)
> > break;
> > }
> >
> > - switch (adev->asic_type) {
> > - case CHIP_CARRIZO:
> > - case CHIP_STONEY:
> > + if ((adev->asic_type == CHIP_CARRIZO ||
> > + adev->asic_type == CHIP_STONEY ||
> > + ((adev->flags & AMD_IS_APU) &&
> > + amdgpu_ip_version(adev, DCE_HWIP, 0) >= IP_VERSION(1, 0, 0) &&
> > + !(adev->apu_flags & AMD_APU_IS_RAVEN))) &&
> > + amdgpu_sg_display != 0) {
>
> Looks like a good idea to me, but please double check your coding style
> settings. The second line of an "if (" should be indented so that it
> starts after the "(" of the previous line.
>
> Additional to that this check actually looks quite messy to me and
> should probably be converted to always using the IP version instead of
> the asic_type.
Or just explicitly disable it on the handful or pre-CZ chips that
don't actually support it.
E.g.,
switch (asic_type) {
case CHIP_KAVERI:
case CHIP_KABINI:
case CHIP_MULLINS:
vm = false;
break;
case CHIP_RAVEN:
if (apu_flags & AMD_APU_IS_RAVEN)
vm = false;
else
vm = amdgpu_sg_display != 0;
break;
default:
vm = (amdgpu_sg_display != 0) && (flags && AMD_IS_APU);
break;
}
>
> Regards,
> Christian.
>
> > init_data.flags.gpu_vm_support = true;
> > - break;
> > - default:
> > - switch (amdgpu_ip_version(adev, DCE_HWIP, 0)) {
> > - case IP_VERSION(1, 0, 0):
> > - case IP_VERSION(1, 0, 1):
> > - /* enable S/G on PCO and RV2 */
> > - if ((adev->apu_flags & AMD_APU_IS_RAVEN2) ||
> > - (adev->apu_flags & AMD_APU_IS_PICASSO))
> > - init_data.flags.gpu_vm_support = true;
> > - break;
> > - case IP_VERSION(2, 1, 0):
> > - case IP_VERSION(3, 0, 1):
> > - case IP_VERSION(3, 1, 2):
> > - case IP_VERSION(3, 1, 3):
> > - case IP_VERSION(3, 1, 4):
> > - case IP_VERSION(3, 1, 5):
> > - case IP_VERSION(3, 1, 6):
> > - case IP_VERSION(3, 5, 0):
> > - init_data.flags.gpu_vm_support = true;
> > - break;
> > - default:
> > - break;
> > - }
> > - break;
> > - }
> > - if (init_data.flags.gpu_vm_support &&
> > - (amdgpu_sg_display == 0))
> > - init_data.flags.gpu_vm_support = false;
> > -
> > - if (init_data.flags.gpu_vm_support)
> > adev->mode_info.gpu_vm_support = true;
> > + } else {
> > + init_data.flags.gpu_vm_support = false;
> > + adev->mode_info.gpu_vm_support = false;
> > + }
> >
> > if (amdgpu_dc_feature_mask & DC_FBC_MASK)
> > init_data.flags.fbc_support = true;
>
Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> A policy binary is OS agnostic, and the same policies are expected to work
> across the OSes. At times it becomes difficult to debug when the policies
> inside the policy binaries starts to misbehave. Add a way to sideload such
> policies independently to debug them via a debugfs entry.
>
> Reviewed-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/platform/x86/amd/pmf/pmf.h| 1 +
> drivers/platform/x86/amd/pmf/tee-if.c | 60 +++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 61a0f3225b62..780c442239e3 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -215,6 +215,7 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + struct dentry *esbin;
> unsigned char *policy_buf;
> u32 policy_sz;
> struct tee_context *tee_ctx;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 4844782d93c7..fa37cfab2dc7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -8,6 +8,7 @@
> * Author: Shyam Sundar S K
> */
>
> +#include
> #include
> #include
> #include "pmf.h"
> @@ -21,6 +22,13 @@ module_param(pb_actions_ms, int, 0644);
> MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency
> (default = 1000ms)");
> #endif
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +/* Sideload policy binaries to debug policy failures */
> +static bool pb_side_load;
> +module_param(pb_side_load, bool, 0444);
> +MODULE_PARM_DESC(pb_side_load, "Sideload policy binaries debug policy
> failures");
> +#endif
> +
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1,
> 0x3d, 0x85, 0x43);
>
> @@ -267,6 +275,49 @@ static int amd_pmf_start_policy_engine(struct
> amd_pmf_dev *dev)
> return 0;
> }
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
> +size_t length, loff_t *pos)
> +{
> + struct amd_pmf_dev *dev = filp->private_data;
> + int ret;
> +
> + /* policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> + if (length > POLICY_BUF_MAX_SZ || length == 0)
> + return -EINVAL;
> +
> + dev->policy_sz = length;
> + if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
> + return -EFAULT;
> +
> + ret = amd_pmf_start_policy_engine(dev);
> + if (ret)
> + return -EINVAL;
> +
> + return length;
> +}
> +
> +static const struct file_operations pb_fops = {
> + .write = amd_pmf_get_pb_data,
> + .open = simple_open,
> +};
> +
> +int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
> +{
> + struct dentry *file = NULL;
> +
> + dev->esbin = debugfs_create_dir("pb", debugfs_root);
> + if (IS_ERR(dev->esbin))
> + return -EINVAL;
> +
> + file = debugfs_create_file("update_policy", 0644, dev->esbin, dev,
> _fops);
> + if (!file)
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
> +
> static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> {
> dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> @@ -279,6 +330,11 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev
> *dev)
>
> memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
Can't this go into amd_pmf_open_pb() as early return?
> + amd_pmf_open_pb(dev, dev->dbgfs_dir);
If you provide #else and empty amd_pmf_open_pb() above, you don't need to
do ifdefs here.
> +#endif
> +
> return amd_pmf_start_policy_engine(dev);
> }
>
> @@ -381,6 +437,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
> + debugfs_remove_recursive(dev->esbin);
> +#endif
Likewise here, if you add amd_pmf_remove_pb() into the above #ifdef + new
#else block with empty body, you can just call it w/o #ifdefs here.
> kfree(dev->prev_data);
> kfree(dev->policy_buf);
> cancel_delayed_work_sync(>pb_work);
>
--
i.
Re: [PATCH 07/15] platform/x86/amd/pmf: Add support update p3t limit
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> P3T (Peak Package Power Limit) is a metric within the SMU controller
> that can influence the power limits. Add support from the driver
> to update P3T limits accordingly.
>
> Reviewed-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/platform/x86/amd/pmf/pmf.h| 3 +++
> drivers/platform/x86/amd/pmf/tee-if.c | 8
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index e64b4d285624..897f61b75e2f 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -46,6 +46,7 @@
> #define GET_STT_MIN_LIMIT0x1F
> #define GET_STT_LIMIT_APU0x20
> #define GET_STT_LIMIT_HS20x21
> +#define SET_P3T 0x23 /* P3T: Peak Package Power
> Limit */
>
> /* OS slider update notification */
> #define DC_BEST_PERF 0
> @@ -69,6 +70,7 @@
> #define PMF_POLICY_STT_MIN 6
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> +#define PMF_POLICY_P3T 38
>
> /* TA macros */
> #define PMF_TA_IF_VERSION__MAJOR 1
> @@ -472,6 +474,7 @@ struct pmf_action_table {
> unsigned long stt_minlimit; /* in mW */
> unsigned long stt_skintemp_apu; /* in C */
> unsigned long stt_skintemp_hs2; /* in C */
> + unsigned long p3t_limit; /* in mW */
> };
>
> /* Input conditions */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index eb25d5ce3a9a..883dd143375a 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -105,6 +105,14 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev
> *dev, struct ta_pmf_enact_
> dev->prev_data->stt_skintemp_hs2 = val;
> }
> break;
> +
> + case PMF_POLICY_P3T:
> + if (dev->prev_data->p3t_limit != val) {
> + amd_pmf_send_cmd(dev, SET_P3T, false, val,
> NULL);
> + dev_dbg(dev->dev, "update P3T : %d\n", val);
%d vs u32
> + dev->prev_data->p3t_limit = val;
unsigned long vs u32 ? (as in the other patch)
--
i.
[PATCH] drm/amd/pm: delete dead code
"ret" was checked earlier inside the loop, so we know it is zero here. No need to check a second time. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 3 --- 1 file changed, 3 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 11a6cd96c601..0ffe55e713f3 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 @@ -2346,9 +2346,6 @@ static int mca_get_mca_entry(struct amdgpu_device *adev, enum amdgpu_mca_error_t return ret; } - if (ret) - return ret; - entry->idx = idx; entry->type = type; -- 2.39.2
Re: [PATCH 08/15] platform/x86/amd/pmf: Add support to update system state
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF driver based on the output actions from the TA can request to update
> the system states like entering s0i3, lock screen etc. by generating
> an uevent. Based on the udev rules set in the userspace the event id
> matching the uevent shall get updated accordingly using the systemctl.
>
> Sample udev rules under Documentation/admin-guide/pmf.rst.
>
> Signed-off-by: Shyam Sundar S K
> ---
> Documentation/admin-guide/pmf.rst | 24
> drivers/platform/x86/amd/pmf/pmf.h| 9 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 40 ++-
> 3 files changed, 72 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/admin-guide/pmf.rst
>
> diff --git a/Documentation/admin-guide/pmf.rst
> b/Documentation/admin-guide/pmf.rst
> new file mode 100644
> index ..b60f381410c3
> --- /dev/null
> +++ b/Documentation/admin-guide/pmf.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Set udev rules for PMF Smart PC Builder
> +---
> +
> +AMD PMF(Platform Management Framework) Smart PC Solution builder has to set
> the system states
> +like S0i3, Screen lock, hibernate etc, based on the output actions provided
> by the PMF
> +TA (Trusted Application).
> +
> +In order for this to work the PMF driver generates a uevent for userspace to
> react to. Below are
> +sample udev rules that can facilitate this experience when a machine has PMF
> Smart PC solution builder
> +enabled.
> +
> +Please add the following line(s) to
> +``/etc/udev/rules.d/99-local.rules``::
> +DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1",
> RUN+="/usr/bin/systemctl suspend"
> +DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2",
> RUN+="/usr/bin/systemctl hibernate"
> +DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="3",
> RUN+="/bin/loginctl lock-sessions"
> +
> +EVENT_ID values:
> +1= Put the system to S0i3/S2Idle
> +2= Put the system to hibernate
> +3= Lock the screen
> +
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 897f61b75e2f..c5334f1177a4 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -70,6 +70,7 @@
> #define PMF_POLICY_STT_MIN 6
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> +#define PMF_POLICY_SYSTEM_STATE 9
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -436,6 +437,13 @@ struct apmf_dyn_slider_output {
> } __packed;
>
> /* Smart PC - TA internals */
> +enum system_state {
> + SYSTEM_STATE__S0i3 = 1,
> + SYSTEM_STATE__S4,
> + SYSTEM_STATE__SCREEN_LOCK,
> + SYSTEM_STATE__MAX
> +};
> +
> enum ta_slider {
> TA_BEST_BATTERY, /* Best Battery */
> TA_BETTER_BATTERY, /* Better Battery */
> @@ -467,6 +475,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> unsigned long sppt_apuonly; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 883dd143375a..1629856c20b4 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions
> sampling frequency (defau
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1,
> 0x3d, 0x85, 0x43);
>
> +static const char *amd_pmf_uevent_as_str(unsigned int state)
> +{
> + switch (state) {
> + case SYSTEM_STATE__S0i3:
> + return "S0i3";
> + case SYSTEM_STATE__S4:
> + return "S4";
> + case SYSTEM_STATE__SCREEN_LOCK:
> + return "SCREEN_LOCK";
> + default:
> + return "Unknown Smart PC event";
> + }
> +}
> +
> static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>struct tee_ioctl_invoke_arg *arg,
>struct tee_param *param)
> @@ -42,9 +56,23 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev,
> int cmd,
> param[0].u.memref.shm_offs = 0;
> }
>
> +static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> +{
> + char *envp[2] = {};
> +
> + envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
> + if (!envp[0])
> + return -EINVAL;
> +
> + kobject_uevent_env(>dev->kobj, KOBJ_CHANGE, envp);
> +
> + kfree(envp[0]);
> + return 0;
> +}
> +
> static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
> {
> -
Re: [PATCH 09/15] platform/x86/amd/pmf: Add facility to dump TA inputs
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF driver sends constant inputs to TA which its gets via the other
> subsystems in the kernel. To debug certain TA issues knowing what inputs
> being sent to TA becomes critical. Add debug facility to the driver which
> can isolate Smart PC and TA related issues.
>
> Reviewed-by: Mario Limonciello
> Signed-off-by: Shyam Sundar S K
> ---
> drivers/platform/x86/amd/pmf/pmf.h| 3 +++
> drivers/platform/x86/amd/pmf/spc.c| 37 +++
> drivers/platform/x86/amd/pmf/sps.c| 2 +-
> drivers/platform/x86/amd/pmf/tee-if.c | 1 +
> 4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index c5334f1177a4..61a0f3225b62 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -592,6 +592,7 @@ int apmf_get_static_slider_granular(struct amd_pmf_dev
> *pdev,
> bool is_pprof_balanced(struct amd_pmf_dev *pmf);
> int amd_pmf_power_slider_update_event(struct amd_pmf_dev *dev);
>
> +const char *source_as_str(unsigned int state);
>
> int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
> int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf);
> @@ -622,4 +623,6 @@ int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
>
> /* Smart PC - TA interfaces */
> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in);
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in);
> +
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c
> b/drivers/platform/x86/amd/pmf/spc.c
> index 08159cd5f853..5c6745f56ed1 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -13,6 +13,43 @@
> #include
> #include "pmf.h"
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static const char *ta_slider_as_str(unsigned int state)
> +{
> + switch (state) {
> + case TA_BEST_PERFORMANCE:
> + return "PERFORMANCE";
> + case TA_BETTER_PERFORMANCE:
> + return "BALANCED";
> + case TA_BEST_BATTERY:
> + return "POWER_SAVER";
> + default:
> + return "Unknown TA Slider State";
> + }
> +}
> +
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in)
> +{
> + dev_dbg(dev->dev, " TA inputs START \n");
> + dev_dbg(dev->dev, "Slider State : %s\n",
> ta_slider_as_str(in->ev_info.power_slider));
> + dev_dbg(dev->dev, "Power Source : %s\n",
> source_as_str(in->ev_info.power_source));
> + dev_dbg(dev->dev, "Battery Percentage : %d\n",
> in->ev_info.bat_percentage);
> + dev_dbg(dev->dev, "Designed Battery Capacity : %d\n",
> in->ev_info.bat_design);
> + dev_dbg(dev->dev, "Fully Charged Capacity : %d\n",
> in->ev_info.full_charge_capacity);
> + dev_dbg(dev->dev, "Drain Rate : %d\n", in->ev_info.drain_rate);
> + dev_dbg(dev->dev, "Socket Power : %d\n", in->ev_info.socket_power);
> + dev_dbg(dev->dev, "Skin Temperature : %d\n",
> in->ev_info.skin_temperature);
> + dev_dbg(dev->dev, "Avg C0 Residency : %d\n",
> in->ev_info.avg_c0residency);
> + dev_dbg(dev->dev, "Max C0 Residency : %d\n",
> in->ev_info.max_c0residency);
> + dev_dbg(dev->dev, "GFX Busy : %d\n", in->ev_info.gfx_busy);
> + dev_dbg(dev->dev, "Connected Display Count : %d\n",
> in->ev_info.monitor_count);
> + dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" :
> "Open");
> + dev_dbg(dev->dev, " TA inputs END \n");
Again, the printf format specifiers are wrong, shouldn't the compiler warn
about them?
> +}
> +#else
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in) {}
> +#endif
> +
> static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_table *in)
> {
> u16 max, avg = 0;
> diff --git a/drivers/platform/x86/amd/pmf/sps.c
> b/drivers/platform/x86/amd/pmf/sps.c
> index a70e67749be3..13e36b52dfe8 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -27,7 +27,7 @@ static const char *slider_as_str(unsigned int state)
> }
> }
>
> -static const char *source_as_str(unsigned int state)
> +const char *source_as_str(unsigned int state)
> {
> switch (state) {
> case POWER_SOURCE_AC:
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
> b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1629856c20b4..4844782d93c7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -186,6 +186,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev
> *dev)
> }
>
> if (ta_sm->pmf_result == TA_PMF_TYPE__SUCCESS && out->actions_count) {
> + amd_pmf_dump_ta_inputs(dev, in);
> dev_dbg(dev->dev, "action count:%d result:%x\n",
> out->actions_count,
>
Re: [PATCH 04/15] platform/x86/amd/pmf: Add support for PMF Policy Binary
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF Policy binary is a encrypted and signed binary that will be part
> of the BIOS. PMF driver via the ACPI interface checks the existence
> of Smart PC bit. If the advertised bit is found, PMF driver walks
> the acpi namespace to find out the policy binary size and the address
> which has to be passed to the TA during the TA init sequence.
>
> The policy binary is comprised of inputs (or the events) and outputs
> (or the actions). With the PMF ecosystem, OEMs generate the policy
> binary (or could be multiple binaries) that contains a supported set
> of inputs and outputs which could be specifically carved out for each
> usage segment (or for each user also) that could influence the system
> behavior either by enriching the user experience or/and boost/throttle
> power limits.
>
> Once the TA init command succeeds, the PMF driver sends the changing
> events in the current environment to the TA for a constant sampling
> frequency time (the event here could be a lid close or open) and
> if the policy binary has corresponding action built within it, the
> TA sends the action for it in the subsequent enact command.
>
> If the inputs sent to the TA has no output defined in the policy
> binary generated by OEMs, there will be no action to be performed
> by the PMF driver.
>
> Example policies:
>
> 1) if slider is performance ; set the SPL to 40W
> Here PMF driver registers with the platform profile interface and
> when the slider position is changed, PMF driver lets the TA know
> about this. TA sends back an action to update the Sustained
> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
>
> 2) if user_away ; then lock the system
> Here PMF driver hooks to the AMD SFH driver to know the user presence
> and send the inputs to TA and if the condition is met, the TA sends
> the action of locking the system. PMF driver generates a uevent and
> based on the udev rule in the userland the system gets locked with
> systemctl.
>
> The intent here is to provide the OEM's to make a policy to lock the
> system when the user is away ; but the userland can make a choice to
> ignore it.
>
> and so on.
>
> The OEMs will have an utility to create numerous such policies and
> the policies shall be reviewed by AMD before signing and encrypting
> them. Policies are shared between operating systems to have seemless user
> experience.
>
> Since all this action has to happen via the "amdtee" driver, currently
> there is no caller for it in the kernel which can load the amdtee driver.
> Without amdtee driver loading onto the system the "tee" calls shall fail
> from the PMF driver. Hence an explicit "request_module" has been added
> to address this.
>
> Signed-off-by: Shyam Sundar S K
> ---
> +struct pmf_action_table {
> + unsigned long spl; /* in mW */
> + unsigned long sppt; /* in mW */
> + unsigned long sppt_apuonly; /* in mW */
> + unsigned long fppt; /* in mW */
> + unsigned long stt_minlimit; /* in mW */
> + unsigned long stt_skintemp_apu; /* in C */
> + unsigned long stt_skintemp_hs2; /* in C */
> +};
> +static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct
> ta_pmf_enact_result *out)
> +{
> + u32 val;
> + int idx;
> +
> + for (idx = 0; idx < out->actions_count; idx++) {
> + val = out->actions_list[idx].value;
> + switch (out->actions_list[idx].action_index) {
> + case PMF_POLICY_SPL:
> + if (dev->prev_data->spl != val) {
> + amd_pmf_send_cmd(dev, SET_SPL, false, val,
> NULL);
> + dev_dbg(dev->dev, "update SPL : %d\n", val);
The %d does not match u32.
> + dev->prev_data->spl = val;
Why is ->spl (and the others too) unsigned long if it's only assigned u32?
> + }
> + break;
> +
> + case PMF_POLICY_SPPT:
> + if (dev->prev_data->sppt != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT, false, val,
> NULL);
> + dev_dbg(dev->dev, "update SPPT : %d\n", val);
> + dev->prev_data->sppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_FPPT:
> + if (dev->prev_data->fppt != val) {
> + amd_pmf_send_cmd(dev, SET_FPPT, false, val,
> NULL);
> + dev_dbg(dev->dev, "update FPPT : %d\n", val);
> + dev->prev_data->fppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_SPPT_APU_ONLY:
> + if (dev->prev_data->sppt_apuonly != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
> val, NULL);
> + dev_dbg(dev->dev, "update SPPT_APU_ONLY :
>
RE: [PATCH 2/2] drm/amdgpu: update retry times for psp vmbx wait
[AMD Official Use Only - General]
Series is Reviewed-by: Hawking Zhang
Regards,
Hawking
-Original Message-
From: amd-gfx On Behalf Of Tao Zhou
Sent: Wednesday, September 27, 2023 18:20
To: amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao
Subject: [PATCH 2/2] drm/amdgpu: update retry times for psp vmbx wait
Increase the retry loops and replace the constant number with macro.
Signed-off-by: Tao Zhou
---
drivers/gpu/drm/amd/amdgpu/psp_v13_0.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
b/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
index 54008a8991fc..b7bc00d4c696 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
@@ -59,6 +59,9 @@ MODULE_FIRMWARE("amdgpu/psp_14_0_0_ta.bin");
/* Read USB-PD from LFB */
#define GFX_CMD_USB_PD_USE_LFB 0x480
+/* Retry times for vmbx ready wait */
+#define PSP_VMBX_POLLING_LIMIT 2
+
/* VBIOS gfl defines */
#define MBOX_READY_MASK 0x8000
#define MBOX_STATUS_MASK 0x
@@ -138,7 +141,7 @@ static int psp_v13_0_wait_for_vmbx_ready(struct psp_context
*psp)
struct amdgpu_device *adev = psp->adev;
int retry_loop, ret;
- for (retry_loop = 0; retry_loop < 70; retry_loop++) {
+ for (retry_loop = 0; retry_loop < PSP_VMBX_POLLING_LIMIT; retry_loop++)
{
/* Wait for bootloader to signify that is
ready having bit 31 of C2PMSG_33 set to 1 */
ret = psp_wait_for(
--
2.35.1
Re: [PATCH] drm/amd/display: enable S/G display for for recent APUs by default
Am 27.09.23 um 07:41 schrieb Yifan Zhang:
With S/G display becomes stable, enable S/G display for recent APUs
by default rather than white list.
Signed-off-by: Yifan Zhang
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 43 +--
1 file changed, 10 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 7e6a693d6369..241fd8defdee 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1626,41 +1626,18 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
break;
}
- switch (adev->asic_type) {
- case CHIP_CARRIZO:
- case CHIP_STONEY:
+ if ((adev->asic_type == CHIP_CARRIZO ||
+ adev->asic_type == CHIP_STONEY ||
+ ((adev->flags & AMD_IS_APU) &&
+ amdgpu_ip_version(adev, DCE_HWIP, 0) >= IP_VERSION(1, 0, 0) &&
+ !(adev->apu_flags & AMD_APU_IS_RAVEN))) &&
+ amdgpu_sg_display != 0) {
Looks like a good idea to me, but please double check your coding style
settings. The second line of an "if (" should be indented so that it
starts after the "(" of the previous line.
Additional to that this check actually looks quite messy to me and
should probably be converted to always using the IP version instead of
the asic_type.
Regards,
Christian.
init_data.flags.gpu_vm_support = true;
- break;
- default:
- switch (amdgpu_ip_version(adev, DCE_HWIP, 0)) {
- case IP_VERSION(1, 0, 0):
- case IP_VERSION(1, 0, 1):
- /* enable S/G on PCO and RV2 */
- if ((adev->apu_flags & AMD_APU_IS_RAVEN2) ||
- (adev->apu_flags & AMD_APU_IS_PICASSO))
- init_data.flags.gpu_vm_support = true;
- break;
- case IP_VERSION(2, 1, 0):
- case IP_VERSION(3, 0, 1):
- case IP_VERSION(3, 1, 2):
- case IP_VERSION(3, 1, 3):
- case IP_VERSION(3, 1, 4):
- case IP_VERSION(3, 1, 5):
- case IP_VERSION(3, 1, 6):
- case IP_VERSION(3, 5, 0):
- init_data.flags.gpu_vm_support = true;
- break;
- default:
- break;
- }
- break;
- }
- if (init_data.flags.gpu_vm_support &&
- (amdgpu_sg_display == 0))
- init_data.flags.gpu_vm_support = false;
-
- if (init_data.flags.gpu_vm_support)
adev->mode_info.gpu_vm_support = true;
+ } else {
+ init_data.flags.gpu_vm_support = false;
+ adev->mode_info.gpu_vm_support = false;
+ }
if (amdgpu_dc_feature_mask & DC_FBC_MASK)
init_data.flags.fbc_support = true;
RE: [PATCH] drm/amd/pm: Disallow managing power profiles on SRIOV for gc11.0.3
[AMD Official Use Only - General]
Ping on this patch. Please help review.
Thanks,
Victor
-Original Message-
From: Victor Zhao
Sent: Tuesday, September 26, 2023 11:09 AM
To: amd-gfx@lists.freedesktop.org
Cc: Zhao, Victor
Subject: [PATCH] drm/amd/pm: Disallow managing power profiles on SRIOV for
gc11.0.3
disable pp_power_profile_mode for sriov on gc11.0.3 as not supported by smu
Signed-off-by: Victor Zhao
---
drivers/gpu/drm/amd/pm/amdgpu_pm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 2d19282e4fbe..b6f32d57b81f 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2122,7 +2122,8 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
} else if (DEVICE_ATTR_IS(pp_power_profile_mode)) {
if (amdgpu_dpm_get_power_profile_mode(adev, NULL) ==
-EOPNOTSUPP)
*states = ATTR_STATE_UNSUPPORTED;
- else if (gc_ver == IP_VERSION(10, 3, 0) &&
amdgpu_sriov_vf(adev))
+ else if ((gc_ver == IP_VERSION(10, 3, 0) ||
+ gc_ver == IP_VERSION(11, 0, 3)) &&
amdgpu_sriov_vf(adev))
*states = ATTR_STATE_UNSUPPORTED;
}
--
2.34.1
[PATCH 1/2] drm/amdgpu: exit directly if gpu reset fails
No need to perform the full reset operation in case of gpu reset
failure.
Signed-off-by: Tao Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5436d7a34014..e4627d92e1d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5075,7 +5075,7 @@ int amdgpu_do_asic_reset(struct list_head
*device_list_handle,
if (r) {
dev_err(tmp_adev->dev, "ASIC reset failed with
error, %d for drm dev, %s",
r, adev_to_drm(tmp_adev)->unique);
- break;
+ goto out;
}
}
--
2.35.1
[PATCH 2/2] drm/amdgpu: update retry times for psp vmbx wait
Increase the retry loops and replace the constant number with macro.
Signed-off-by: Tao Zhou
---
drivers/gpu/drm/amd/amdgpu/psp_v13_0.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
b/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
index 54008a8991fc..b7bc00d4c696 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v13_0.c
@@ -59,6 +59,9 @@ MODULE_FIRMWARE("amdgpu/psp_14_0_0_ta.bin");
/* Read USB-PD from LFB */
#define GFX_CMD_USB_PD_USE_LFB 0x480
+/* Retry times for vmbx ready wait */
+#define PSP_VMBX_POLLING_LIMIT 2
+
/* VBIOS gfl defines */
#define MBOX_READY_MASK 0x8000
#define MBOX_STATUS_MASK 0x
@@ -138,7 +141,7 @@ static int psp_v13_0_wait_for_vmbx_ready(struct psp_context
*psp)
struct amdgpu_device *adev = psp->adev;
int retry_loop, ret;
- for (retry_loop = 0; retry_loop < 70; retry_loop++) {
+ for (retry_loop = 0; retry_loop < PSP_VMBX_POLLING_LIMIT; retry_loop++)
{
/* Wait for bootloader to signify that is
ready having bit 31 of C2PMSG_33 set to 1 */
ret = psp_wait_for(
--
2.35.1
Re: [PATCH 2/2] drm/amd/display: Fix null pointer dereference in error message
On 2023-09-26 01:56, Cong Liu wrote:
This patch fixes a null pointer dereference in the error message that is
printed when the Display Core (DC) fails to initialize. The original
message includes the DC version number, which is undefined if the DC is
not initialized.
Fixes: 9788d087caff ("drm/amd/display: improve the message printed when loading
DC")
Signed-off-by: Cong Liu
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 8e98dda1e084..bf52a909f558 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1703,8 +1703,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
DRM_INFO("Display Core v%s initialized on %s\n", DC_VER,
dce_version_to_string(adev->dm.dc->ctx->dce_version));
} else {
- DRM_INFO("Display Core v%s failed to initialize on %s\n",
DC_VER,
-dce_version_to_string(adev->dm.dc->ctx->dce_version));
+ DRM_INFO("Display Core failed to initialize with v%s!\n",
DC_VER);
There is value in printing the version number. Let's not remove it.
Instead you can probably fix it by doing a NULL check on adev->dm.dc->ctx.
Hi Harry
I don't understand what you mean. Are you saying that I need to add a
NULL check in the if statement (i.e. if(adev->dm.dc &&
adev->dm.dc->ctx)), because adev->dm.dc is NULL in the else statement
and there is no way to print adev->dm.dc->ctx->dce_version.
Regards
Cong
Harry
goto error;
}
