RE: [PATCH] drm/amdgpu/sriov: Use VF-accessible register for gpu_clock_count

[Liu, Monk]

You can put my RB.
_
Monk Liu|GPU Virtualization Team |AMD


-Original Message-
From: Zhao, Jiange  
Sent: Wednesday, March 4, 2020 1:39 PM
To: amd-gfx@lists.freedesktop.org
Cc: Deng, Emily ; Liu, Monk ; Deucher, 
Alexander ; Zhao, Jiange 
Subject: [PATCH] drm/amdgpu/sriov: Use VF-accessible register for 
gpu_clock_count

Navi12 VK CTS subtest timestamp.calibrated.dev_domain_test failed because 
mmRLC_CAPTURE_GPU_CLOCK_COUNT register cannot be written in VF due to security 
policy.

Solution: use a VF-accessible timestamp register pair 
mmGOLDEN_TSC_COUNT_LOWER/UPPER for SRIOV case.

v2: according to Deucher Alexander's advice, switch to 
mmGOLDEN_TSC_COUNT_LOWER/UPPER for both bare metal and SRIOV.

Signed-off-by: jianzh 
---
 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 03655c3..22a07ad 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -35,6 +35,8 @@
 
 #include "gc/gc_10_1_0_offset.h"
 #include "gc/gc_10_1_0_sh_mask.h"
+#include "smuio/smuio_11_0_0_offset.h"
+#include "smuio/smuio_11_0_0_sh_mask.h"
 #include "navi10_enum.h"
 #include "hdp/hdp_5_0_0_offset.h"
 #include "ivsrcid/gfx/irqsrcs_gfx_10_1.h"
@@ -3925,9 +3927,8 @@ static uint64_t gfx_v10_0_get_gpu_clock_counter(struct 
amdgpu_device *adev)
 
amdgpu_gfx_off_ctrl(adev, false);
mutex_lock(>gfx.gpu_clock_mutex);
-   WREG32_SOC15(GC, 0, mmRLC_CAPTURE_GPU_CLOCK_COUNT, 1);
-   clock = (uint64_t)RREG32_SOC15(GC, 0, mmRLC_GPU_CLOCK_COUNT_LSB) |
-   ((uint64_t)RREG32_SOC15(GC, 0, mmRLC_GPU_CLOCK_COUNT_MSB) << 
32ULL);
+   clock = (uint64_t)RREG32_SOC15(SMUIO, 0, mmGOLDEN_TSC_COUNT_LOWER) |
+   ((uint64_t)RREG32_SOC15(SMUIO, 0, mmGOLDEN_TSC_COUNT_UPPER) << 
+32ULL);
mutex_unlock(>gfx.gpu_clock_mutex);
amdgpu_gfx_off_ctrl(adev, true);
return clock;
--
2.7.4

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

[PATCH] drm/amdgpu/sriov: Use VF-accessible register for gpu_clock_count

[jianzh]

Navi12 VK CTS subtest timestamp.calibrated.dev_domain_test failed
because mmRLC_CAPTURE_GPU_CLOCK_COUNT register cannot be
written in VF due to security policy.

Solution: use a VF-accessible timestamp register pair
mmGOLDEN_TSC_COUNT_LOWER/UPPER for SRIOV case.

v2: according to Deucher Alexander's advice, switch to
mmGOLDEN_TSC_COUNT_LOWER/UPPER for both bare metal and SRIOV.

Signed-off-by: jianzh 
---
 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 03655c3..22a07ad 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -35,6 +35,8 @@
 
 #include "gc/gc_10_1_0_offset.h"
 #include "gc/gc_10_1_0_sh_mask.h"
+#include "smuio/smuio_11_0_0_offset.h"
+#include "smuio/smuio_11_0_0_sh_mask.h"
 #include "navi10_enum.h"
 #include "hdp/hdp_5_0_0_offset.h"
 #include "ivsrcid/gfx/irqsrcs_gfx_10_1.h"
@@ -3925,9 +3927,8 @@ static uint64_t gfx_v10_0_get_gpu_clock_counter(struct 
amdgpu_device *adev)
 
amdgpu_gfx_off_ctrl(adev, false);
mutex_lock(>gfx.gpu_clock_mutex);
-   WREG32_SOC15(GC, 0, mmRLC_CAPTURE_GPU_CLOCK_COUNT, 1);
-   clock = (uint64_t)RREG32_SOC15(GC, 0, mmRLC_GPU_CLOCK_COUNT_LSB) |
-   ((uint64_t)RREG32_SOC15(GC, 0, mmRLC_GPU_CLOCK_COUNT_MSB) << 
32ULL);
+   clock = (uint64_t)RREG32_SOC15(SMUIO, 0, mmGOLDEN_TSC_COUNT_LOWER) |
+   ((uint64_t)RREG32_SOC15(SMUIO, 0, mmGOLDEN_TSC_COUNT_UPPER) << 
32ULL);
mutex_unlock(>gfx.gpu_clock_mutex);
amdgpu_gfx_off_ctrl(adev, true);
return clock;
-- 
2.7.4

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

[PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Jacob He]

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..f08effb033a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
amdgpu_job *job,
struct dma_fence *fence = NULL;
bool pasid_mapping_needed = false;
unsigned patch_offset = 0;
+   bool update_spm_vmid_needed = (job->vm && 
(job->vm->reserved_vmid[vmhub] != NULL));
int r;
 
+   if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
+   adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+
if (amdgpu_vmid_had_gpu_reset(adev, id)) {
gds_switch_needed = true;
vm_flush_needed = true;
@@ -3213,6 +3217,7 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, 
struct drm_file *filp)
union drm_amdgpu_vm *args = data;
struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_fpriv *fpriv = filp->driver_priv;
+   long timeout = msecs_to_jiffies(2000);
int r;
 
switch (args->in.op) {
@@ -3224,6 +3229,15 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void *data, 
struct drm_file *filp)
return r;
break;
case AMDGPU_VM_OP_UNRESERVE_VMID:
+   if (amdgpu_sriov_runtime(adev))
+   timeout = 8 * timeout;
+
+   /* Wait vm idle to make sure the vmid set in SPM_VMID is
+* not referenced anymore.
+*/
+   r = amdgpu_vm_wait_idle(>vm, timeout);
+   if (r < 0)
+   return r;
amdgpu_vmid_free_reserved(adev, >vm, AMDGPU_GFXHUB_0);
break;
default:
-- 
2.17.1

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

Re: [PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Mikel Rychliski]

On Tuesday, March 3, 2020 9:38:27 AM EST Bjorn Helgaas wrote:
> Cosmetics:
> 
> s/ioremap/ioremap()/ (also in commit log)
> s/phys_to_virt/phys_to_virt()/ (also in commit log)
> s/pci_platform_rom/pci_platform_rom()/ (commit log)
> s/rom/ROM/

> This changes the interface of pci_platform_rom() (the caller is now
> responsible for doing an iounmap()).  That should be mentioned in the
> function comment, and I think the subsequent patches should be
> squashed into this one so the interface change and the caller changes
> are done simultaneously.
> 
> Also, it looks like nvbios_platform.init() (platform_init()) needs a
> similar change?

Hi Bjorn,

Thank you for your comments. I'll make the suggested changes, add an iounmap() 
in the nouveau usage, and provide a new two-patch series.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm/amd/powerplay: map mclk to fclk for COMBINATIONAL_BYPASS case

[Quan, Evan]

Series is reviewed-by: Evan Quan 

-Original Message-
From: Liang, Prike  
Sent: Wednesday, March 4, 2020 10:56 AM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan ; Huang, Ray ; Liang, 
Prike 
Subject: [PATCH 2/2] drm/amd/powerplay: map mclk to fclk for 
COMBINATIONAL_BYPASS case

When hit COMBINATIONAL_BYPASS the mclk will be bypass and can export
fclk frequency to user usage.

Signed-off-by: Prike Liang 
---
 drivers/gpu/drm/amd/powerplay/renoir_ppt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c 
b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
index cca4820..653faad 100644
--- a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
@@ -111,8 +111,8 @@ static struct smu_12_0_cmn2aisc_mapping 
renoir_clk_map[SMU_CLK_COUNT] = {
CLK_MAP(GFXCLK, CLOCK_GFXCLK),
CLK_MAP(SCLK,   CLOCK_GFXCLK),
CLK_MAP(SOCCLK, CLOCK_SOCCLK),
-   CLK_MAP(UCLK, CLOCK_UMCCLK),
-   CLK_MAP(MCLK, CLOCK_UMCCLK),
+   CLK_MAP(UCLK, CLOCK_FCLK),
+   CLK_MAP(MCLK, CLOCK_FCLK),
 };
 
 static struct smu_12_0_cmn2aisc_mapping renoir_table_map[SMU_TABLE_COUNT] = {
@@ -280,7 +280,7 @@ static int renoir_print_clk_levels(struct smu_context *smu,
break;
case SMU_MCLK:
count = NUM_MEMCLK_DPM_LEVELS;
-   cur_value = metrics.ClockFrequency[CLOCK_UMCCLK];
+   cur_value = metrics.ClockFrequency[CLOCK_FCLK];
break;
case SMU_DCEFCLK:
count = NUM_DCFCLK_DPM_LEVELS;
-- 
2.7.4

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

RE: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark clock settings to smu resume from s3

[Quan, Evan]

Sorry for miss this. With the Alex's comment addressed, the patch is 
reviewed-by: Evan Quan 

-Original Message-
From: Wu, Hersen  
Sent: Tuesday, March 3, 2020 9:19 PM
To: Alex Deucher 
Cc: Quan, Evan ; amd-gfx@lists.freedesktop.org; Feng, 
Kenneth 
Subject: RE: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark clock 
settings to smu resume from s3

[AMD Official Use Only - Internal Distribution Only]

Hi Evan, Kenneth,

Would you please help review this patch again?

Thanks!
Hersen


-Original Message-
From: Alex Deucher 
Sent: Monday, March 2, 2020 9:27 AM
To: Wu, Hersen 
Cc: Quan, Evan ; amd-gfx@lists.freedesktop.org; Feng, 
Kenneth 
Subject: Re: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark clock 
settings to smu resume from s3

On Fri, Feb 28, 2020 at 3:59 PM Wu, Hersen  wrote:
>
> Follow Evan's review, add smu->mutex.
>
>
> This interface is for dGPU Navi1x. Linux dc-pplib interface depends on 
> window driver dc implementation.
>
>  For Navi1x, clock settings of dcn watermarks are fixed. the settings 
> should be passed to smu during boot up and resume from s3.
>  boot up: dc calculate dcn watermark clock settings within dc_create, 
> dcn20_resource_construct, then call pplib functions below to pass  the 
> settings to smu:
>  smu_set_watermarks_for_clock_ranges
>  smu_set_watermarks_table
>  navi10_set_watermarks_table
>  smu_write_watermarks_table
>
>  For Renoir, clock settings of dcn watermark are also fixed values.
>  dc has implemented different flow for window driver:
>  dc_hardware_init / dc_set_power_state  dcn10_init_hw notify_wm_ranges  
> set_wm_ranges
>
>  For Linux
>  smu_set_watermarks_for_clock_ranges
>  renoir_set_watermarks_table
>  smu_write_watermarks_table
>
>  dc_hardware_init -> amdgpu_dm_init
>  dc_set_power_state --> dm_resume
>
>  therefore, linux dc-pplib interface of navi10/12/14 is different from 
> that of Renoir.
>
> Signed-off-by: Hersen Wu 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68
> +++
>  1 file changed, 68 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 931cbd7b372e..1ee1d6ff2782 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1435,6 +1435,72 @@ static void s3_handle_mst(struct drm_device *dev, bool 
> suspend)
>   drm_kms_helper_hotplug_event(dev);
>  }
>
> +static int amdgpu_dm_smu_write_watermarks_table(struct amdgpu_device
> +*adev) {  struct smu_context *smu = >smu;  int ret = 0;
> +
> + if (!is_support_sw_smu(adev))
> + return 0;
> +
> + /* This interface is for dGPU Navi1x.Linux dc-pplib interface 
> + depends
> + * on window driver dc implementation.
> + * For Navi1x, clock settings of dcn watermarks are fixed. the 
> + settings
> + * should be passed to smu during boot up and resume from s3.
> + * boot up: dc calculate dcn watermark clock settings within 
> + dc_create,
> + * dcn20_resource_construct
> + * then call pplib functions below to pass the settings to smu:
> + * smu_set_watermarks_for_clock_ranges
> + * smu_set_watermarks_table
> + * navi10_set_watermarks_table
> + * smu_write_watermarks_table
> + *
> + * For Renoir, clock settings of dcn watermark are also fixed values.
> + * dc has implemented different flow for window driver:
> + * dc_hardware_init / dc_set_power_state
> + * dcn10_init_hw
> + * notify_wm_ranges
> + * set_wm_ranges
> + * -- Linux
> + * smu_set_watermarks_for_clock_ranges
> + * renoir_set_watermarks_table
> + * smu_write_watermarks_table
> + *
> + * For Linux,
> + * dc_hardware_init -> amdgpu_dm_init
> + * dc_set_power_state --> dm_resume
> + *
> + * therefore, this function apply to navi10/12/14 but not Renoir
> + * *
> + */
> + switch(adev->asic_type) {
> + case CHIP_NAVI10:
> + case CHIP_NAVI14:
> + case CHIP_NAVI12:
> + break;
> + default:
> + return 0;
> + }
> +
> + mutex_lock(>mutex);
> +
> + /* pass data to smu controller */
> + if ((smu->watermarks_bitmap & WATERMARKS_EXIST) && 
> + !(smu->watermarks_bitmap & WATERMARKS_LOADED)) { ret = 
> + smu_write_watermarks_table(smu);
> +
> + if (ret) {
> + DRM_ERROR("Failed to update WMTABLE!\n"); return ret;

You need to unlock the mutex here in the failure case.

Alex

> + }
> + smu->watermarks_bitmap |= WATERMARKS_LOADED;
> + }
> +
> + mutex_unlock(>mutex);
> +
> + return 0;
> +}
> +
>  /**
>   * dm_hw_init() - Initialize DC device
>   * @handle: The base driver device containing the amdgpu_dm device.
> @@ -1713,6 +1779,8 @@ static int dm_resume(void *handle)
>
>   amdgpu_dm_irq_resume_late(adev);
>
> + amdgpu_dm_smu_write_watermarks_table(adev);
> +
>   return 0;
>  }
>
> --
> 2.17.1
>
> 
> From: Quan, Evan 
> Sent: February 27, 2020 9:58 PM
> To: Wu, Hersen ; amd-gfx@lists.freedesktop.org 
> 
> Cc: Feng, Kenneth ; Wu, Hersen 
> 
> Subject: RE: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn 

[PATCH 2/2] drm/amd/powerplay: map mclk to fclk for COMBINATIONAL_BYPASS case

[Prike Liang]

When hit COMBINATIONAL_BYPASS the mclk will be bypass and can export
fclk frequency to user usage.

Signed-off-by: Prike Liang 
---
 drivers/gpu/drm/amd/powerplay/renoir_ppt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c 
b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
index cca4820..653faad 100644
--- a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
@@ -111,8 +111,8 @@ static struct smu_12_0_cmn2aisc_mapping 
renoir_clk_map[SMU_CLK_COUNT] = {
CLK_MAP(GFXCLK, CLOCK_GFXCLK),
CLK_MAP(SCLK,   CLOCK_GFXCLK),
CLK_MAP(SOCCLK, CLOCK_SOCCLK),
-   CLK_MAP(UCLK, CLOCK_UMCCLK),
-   CLK_MAP(MCLK, CLOCK_UMCCLK),
+   CLK_MAP(UCLK, CLOCK_FCLK),
+   CLK_MAP(MCLK, CLOCK_FCLK),
 };
 
 static struct smu_12_0_cmn2aisc_mapping renoir_table_map[SMU_TABLE_COUNT] = {
@@ -280,7 +280,7 @@ static int renoir_print_clk_levels(struct smu_context *smu,
break;
case SMU_MCLK:
count = NUM_MEMCLK_DPM_LEVELS;
-   cur_value = metrics.ClockFrequency[CLOCK_UMCCLK];
+   cur_value = metrics.ClockFrequency[CLOCK_FCLK];
break;
case SMU_DCEFCLK:
count = NUM_DCFCLK_DPM_LEVELS;
-- 
2.7.4

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

[PATCH 1/2] drm/amd/powerplay: fix pre-check condition for setting clock range

[Prike Liang]

This fix will handle some MP1 FW issue like as mclk dpm table in renoir has a 
reverse
dpm clock layout and a zero frequency dpm level as following case.

cat pp_dpm_mclk
0: 1200Mhz
1: 1200Mhz
2: 800Mhz
3: 0Mhz

Signed-off-by: Prike Liang 
---
 drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 2 +-
 drivers/gpu/drm/amd/powerplay/smu_v12_0.c  | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c 
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index e3398f9..d454493 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -214,7 +214,7 @@ int smu_set_soft_freq_range(struct smu_context *smu, enum 
smu_clk_type clk_type,
 {
int ret = 0;
 
-   if (min <= 0 && max <= 0)
+   if (min < 0 && max < 0)
return -EINVAL;
 
if (!smu_clk_dpm_is_enabled(smu, clk_type))
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v12_0.c 
b/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
index 93b8558..d52e624 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
@@ -461,9 +461,6 @@ int smu_v12_0_set_soft_freq_limited_range(struct 
smu_context *smu, enum smu_clk_
 {
int ret = 0;
 
-   if (max < min)
-   return -EINVAL;
-
switch (clk_type) {
case SMU_GFXCLK:
case SMU_SCLK:
-- 
2.7.4

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

[PATCH 01/12] drm/amd/display: update soc bb for nv14

[Rodrigo Siqueira]

From: Martin Leung 

[why]
nv14 previously inherited soc bb from generic dcn 2, did not match
watermark values according to memory team

[how]
add nv14 specific soc bb: copy nv2 generic that it was
using from before, but changed num channels to 8

Signed-off-by: Martin Leung 
Reviewed-by: Jun Lei 
Acked-by: Rodrigo Siqueira 
---
 .../drm/amd/display/dc/dcn20/dcn20_resource.c | 113 +-
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
index c629a7b45f56..c8b85f62ae95 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
@@ -337,6 +337,117 @@ struct _vcs_dpi_soc_bounding_box_st dcn2_0_soc = {
.use_urgent_burst_bw = 0
 };
 
+struct _vcs_dpi_soc_bounding_box_st dcn2_0_nv14_soc = {
+   .clock_limits = {
+   {
+   .state = 0,
+   .dcfclk_mhz = 560.0,
+   .fabricclk_mhz = 560.0,
+   .dispclk_mhz = 513.0,
+   .dppclk_mhz = 513.0,
+   .phyclk_mhz = 540.0,
+   .socclk_mhz = 560.0,
+   .dscclk_mhz = 171.0,
+   .dram_speed_mts = 8960.0,
+   },
+   {
+   .state = 1,
+   .dcfclk_mhz = 694.0,
+   .fabricclk_mhz = 694.0,
+   .dispclk_mhz = 642.0,
+   .dppclk_mhz = 642.0,
+   .phyclk_mhz = 600.0,
+   .socclk_mhz = 694.0,
+   .dscclk_mhz = 214.0,
+   .dram_speed_mts = 11104.0,
+   },
+   {
+   .state = 2,
+   .dcfclk_mhz = 875.0,
+   .fabricclk_mhz = 875.0,
+   .dispclk_mhz = 734.0,
+   .dppclk_mhz = 734.0,
+   .phyclk_mhz = 810.0,
+   .socclk_mhz = 875.0,
+   .dscclk_mhz = 245.0,
+   .dram_speed_mts = 14000.0,
+   },
+   {
+   .state = 3,
+   .dcfclk_mhz = 1000.0,
+   .fabricclk_mhz = 1000.0,
+   .dispclk_mhz = 1100.0,
+   .dppclk_mhz = 1100.0,
+   .phyclk_mhz = 810.0,
+   .socclk_mhz = 1000.0,
+   .dscclk_mhz = 367.0,
+   .dram_speed_mts = 16000.0,
+   },
+   {
+   .state = 4,
+   .dcfclk_mhz = 1200.0,
+   .fabricclk_mhz = 1200.0,
+   .dispclk_mhz = 1284.0,
+   .dppclk_mhz = 1284.0,
+   .phyclk_mhz = 810.0,
+   .socclk_mhz = 1200.0,
+   .dscclk_mhz = 428.0,
+   .dram_speed_mts = 16000.0,
+   },
+   /*Extra state, no dispclk ramping*/
+   {
+   .state = 5,
+   .dcfclk_mhz = 1200.0,
+   .fabricclk_mhz = 1200.0,
+   .dispclk_mhz = 1284.0,
+   .dppclk_mhz = 1284.0,
+   .phyclk_mhz = 810.0,
+   .socclk_mhz = 1200.0,
+   .dscclk_mhz = 428.0,
+   .dram_speed_mts = 16000.0,
+   },
+   },
+   .num_states = 5,
+   .sr_exit_time_us = 8.6,
+   .sr_enter_plus_exit_time_us = 10.9,
+   .urgent_latency_us = 4.0,
+   .urgent_latency_pixel_data_only_us = 4.0,
+   .urgent_latency_pixel_mixed_with_vm_data_us = 4.0,
+   .urgent_latency_vm_data_only_us = 4.0,
+   .urgent_out_of_order_return_per_channel_pixel_only_bytes = 4096,
+   .urgent_out_of_order_return_per_channel_pixel_and_vm_bytes = 4096,
+   .urgent_out_of_order_return_per_channel_vm_only_bytes = 4096,
+   .pct_ideal_dram_sdp_bw_after_urgent_pixel_only = 40.0,
+   .pct_ideal_dram_sdp_bw_after_urgent_pixel_and_vm = 40.0,
+   .pct_ideal_dram_sdp_bw_after_urgent_vm_only = 40.0,
+   .max_avg_sdp_bw_use_normal_percent = 40.0,
+   

[PATCH 11/12] drm/amd/display: separate FEC capability from fec debug flag

[Rodrigo Siqueira]

From: Wenjing Liu 

[why]
FEC capability query should not be affected by debugging decision on
whether to disable FEC. We should not determine if display supports FEC
by checking debug option.

Signed-off-by: Wenjing Liu 
Reviewed-by: Ashley Thomas 
Acked-by: Rodrigo Siqueira 
---
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c  | 16 
 drivers/gpu/drm/amd/display/dc/core/dc_link.c|  3 +--
 drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c |  4 ++--
 drivers/gpu/drm/amd/display/dc/dc.h  |  8 +++-
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 4837640530ad..cc1b52b72c0b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -207,7 +207,7 @@ static bool validate_dsc_caps_on_connector(struct 
amdgpu_dm_connector *aconnecto
 
if (!dc_dsc_parse_dsc_dpcd(aconnector->dc_link->ctx->dc,
   dsc_caps, NULL,
-  _sink->sink_dsc_caps.dsc_dec_caps))
+  _sink->dsc_caps.dsc_dec_caps))
return false;
 
return true;
@@ -262,8 +262,8 @@ static int dm_dp_mst_get_modes(struct drm_connector 
*connector)
 
 #if defined(CONFIG_DRM_AMD_DC_DCN)
if (!validate_dsc_caps_on_connector(aconnector))
-   memset(>dc_sink->sink_dsc_caps,
-  0, 
sizeof(aconnector->dc_sink->sink_dsc_caps));
+   memset(>dc_sink->dsc_caps,
+  0, 
sizeof(aconnector->dc_sink->dsc_caps));
 #endif
}
}
@@ -550,7 +550,7 @@ static void set_dsc_configs_from_fairness_vars(struct 
dsc_mst_fairness_params *p
memset([i].timing->dsc_cfg, 0, 
sizeof(params[i].timing->dsc_cfg));
if (vars[i].dsc_enabled && dc_dsc_compute_config(

params[i].sink->ctx->dc->res_pool->dscs[0],
-   
[i].sink->sink_dsc_caps.dsc_dec_caps,
+   [i].sink->dsc_caps.dsc_dec_caps,

params[i].sink->ctx->dc->debug.dsc_min_slice_height_override,
0,
params[i].timing,
@@ -571,7 +571,7 @@ static int bpp_x16_from_pbn(struct dsc_mst_fairness_params 
param, int pbn)
kbps = div_u64((u64)pbn * 994 * 8 * 54, 64);
dc_dsc_compute_config(
param.sink->ctx->dc->res_pool->dscs[0],
-   >sink_dsc_caps.dsc_dec_caps,
+   >dsc_caps.dsc_dec_caps,

param.sink->ctx->dc->debug.dsc_min_slice_height_override,
(int) kbps, param.timing, _config);
 
@@ -768,14 +768,14 @@ static bool compute_mst_dsc_configs_for_link(struct 
drm_atomic_state *state,
params[count].sink = stream->sink;
aconnector = (struct amdgpu_dm_connector 
*)stream->dm_stream_context;
params[count].port = aconnector->port;
-   params[count].compression_possible = 
stream->sink->sink_dsc_caps.dsc_dec_caps.is_dsc_supported;
+   params[count].compression_possible = 
stream->sink->dsc_caps.dsc_dec_caps.is_dsc_supported;
dc_dsc_get_policy_for_timing(params[count].timing, _policy);
if (!dc_dsc_compute_bandwidth_range(
stream->sink->ctx->dc->res_pool->dscs[0],

stream->sink->ctx->dc->debug.dsc_min_slice_height_override,
dsc_policy.min_target_bpp,
dsc_policy.max_target_bpp,
-   >sink->sink_dsc_caps.dsc_dec_caps,
+   >sink->dsc_caps.dsc_dec_caps,
>timing, [count].bw_range))
params[count].bw_range.stream_kbps = 
dc_bandwidth_in_kbps_from_timing(>timing);
 
@@ -857,7 +857,7 @@ bool compute_mst_dsc_configs_for_state(struct 
drm_atomic_state *state,
if (!aconnector || !aconnector->dc_sink)
continue;
 
-   if 
(!aconnector->dc_sink->sink_dsc_caps.dsc_dec_caps.is_dsc_supported)
+   if 
(!aconnector->dc_sink->dsc_caps.dsc_dec_caps.is_dsc_supported)
continue;
 
if (computed_streams[i])
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 83df17a17271..fb603bd46fac 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -3407,7 +3407,7 @@ uint32_t dc_link_bandwidth_kbps(

[PATCH 03/12] drm/amd/display: determine is mst hdcp based on stream instead of sink signal

[Rodrigo Siqueira]

From: Wenjing Liu 

[why]
It is possible even if sink signal is MST but driver enables SST stream.
We should not determine if we should do MST authentication based on
sink's capability.
Instead we should determine whether to do MST authentication based on
what we have enabled in stream.

Signed-off-by: Wenjing Liu 
Reviewed-by: Ashley Thomas 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c | 1 +
 drivers/gpu/drm/amd/display/dc/core/dc_link.c  | 2 ++
 drivers/gpu/drm/amd/display/dc/dm_cp_psp.h | 1 +
 drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c| 4 +---
 drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h| 6 +++---
 drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h | 4 ++--
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c
index c4fd148bf6e0..5b70ed3cdb88 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.c
@@ -412,6 +412,7 @@ static void update_config(void *handle, struct 
cp_psp_stream_config *config)
link->dig_be = config->link_enc_inst;
link->ddc_line = aconnector->dc_link->ddc_hw_inst + 1;
link->dp.rev = aconnector->dc_link->dpcd_caps.dpcd_rev.raw;
+   link->dp.mst_supported = config->mst_supported;
display->adjust.disable = 1;
link->adjust.auth_delay = 2;
 
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index a4680968c8f4..ddd4dca61cc3 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -2959,6 +2959,8 @@ static void update_psp_stream_config(struct pipe_ctx 
*pipe_ctx, bool dpms_off)
config.link_enc_inst = pipe_ctx->stream->link->link_enc_hw_inst;
config.dpms_off = dpms_off;
config.dm_stream_ctx = pipe_ctx->stream->dm_stream_context;
+   config.mst_supported = (pipe_ctx->stream->signal ==
+   SIGNAL_TYPE_DISPLAY_PORT_MST);
cp_psp->funcs.update_stream_config(cp_psp->handle, );
}
 }
diff --git a/drivers/gpu/drm/amd/display/dc/dm_cp_psp.h 
b/drivers/gpu/drm/amd/display/dc/dm_cp_psp.h
index 626d22d437f4..968c46dfb506 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_cp_psp.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_cp_psp.h
@@ -32,6 +32,7 @@ struct cp_psp_stream_config {
uint8_t otg_inst;
uint8_t link_enc_inst;
uint8_t stream_enc_inst;
+   uint8_t mst_supported;
void *dm_stream_ctx;
bool dpms_off;
 };
diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c 
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
index bcba93d3b195..7a571b3f62d6 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
@@ -481,10 +481,8 @@ enum mod_hdcp_operation_mode 
mod_hdcp_signal_type_to_operation_mode(
break;
case SIGNAL_TYPE_EDP:
case SIGNAL_TYPE_DISPLAY_PORT:
-   mode = MOD_HDCP_MODE_DP;
-   break;
case SIGNAL_TYPE_DISPLAY_PORT_MST:
-   mode = MOD_HDCP_MODE_DP_MST;
+   mode = MOD_HDCP_MODE_DP;
break;
default:
break;
diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h 
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
index 77fdcec4263e..5cb4546be0ef 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
@@ -392,13 +392,13 @@ enum mod_hdcp_status mod_hdcp_write_content_type(struct 
mod_hdcp *hdcp);
 /* hdcp version helpers */
 static inline uint8_t is_dp_hdcp(struct mod_hdcp *hdcp)
 {
-   return (hdcp->connection.link.mode == MOD_HDCP_MODE_DP ||
-   hdcp->connection.link.mode == MOD_HDCP_MODE_DP_MST);
+   return (hdcp->connection.link.mode == MOD_HDCP_MODE_DP);
 }
 
 static inline uint8_t is_dp_mst_hdcp(struct mod_hdcp *hdcp)
 {
-   return (hdcp->connection.link.mode == MOD_HDCP_MODE_DP_MST);
+   return (hdcp->connection.link.mode == MOD_HDCP_MODE_DP &&
+   hdcp->connection.link.dp.mst_supported);
 }
 
 static inline uint8_t is_hdmi_dvi_sl_hdcp(struct mod_hdcp *hdcp)
diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h 
b/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
index bb855ea5d5a3..c088602bc1a0 100644
--- a/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
+++ b/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
@@ -102,6 +102,7 @@ enum mod_hdcp_status {
 struct mod_hdcp_displayport {
uint8_t rev;
uint8_t assr_supported;
+   uint8_t mst_supported;
 };
 
 struct mod_hdcp_hdmi {
@@ -110,8 +111,7 @@ struct mod_hdcp_hdmi {
 enum mod_hdcp_operation_mode {
MOD_HDCP_MODE_OFF,

[PATCH 09/12] drm/amd/display: Stop if retimer is not available

[Rodrigo Siqueira]

Raven provides retimer feature support that requires i2c interaction in
order to make it work well, all settings required for this configuration
are loaded from the Atom bios which include the i2c address. If the
retimer feature is not available, we should abort the attempt to set
this feature, otherwise, it makes the following line return
I2C_CHANNEL_OPERATION_NO_RESPONSE:

 i2c_success = i2c_write(pipe_ctx, slave_address, buffer, sizeof(buffer));
 ...
 if (!i2c_success)
   ASSERT(i2c_success);

This ends up causing problems with hotplugging HDMI displays on Raven,
and causes retimer settings to warn like so:

WARNING: CPU: 1 PID: 429 at
drivers/gpu/drm/amd/amdgpu/../dal/dc/core/dc_link.c:1998
write_i2c_retimer_setting+0xc2/0x3c0 [amdgpu] Modules linked in:
edac_mce_amd ccp kvm irqbypass binfmt_misc crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel snd_hda_codec_realtek snd_hda_codec_generic
ledtrig_audio snd_hda_codec_hdmi snd_hda_intel amdgpu(+) snd_hda_codec
snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event
snd_rawmidi aesni_intel snd_seq amd_iommu_v2 gpu_sched aes_x86_64
crypto_simd cryptd glue_helper snd_seq_device ttm drm_kms_helper
snd_timer eeepc_wmi wmi_bmof asus_wmi sparse_keymap drm mxm_wmi snd
k10temp fb_sys_fops syscopyarea sysfillrect sysimgblt soundcore joydev
input_leds mac_hid sch_fq_codel parport_pc ppdev lp parport ip_tables
x_tables autofs4 igb i2c_algo_bit hid_generic usbhid i2c_piix4 dca ahci
hid libahci video wmi gpio_amdpt gpio_generic CPU: 1 PID: 429 Comm:
systemd-udevd Tainted: GW 5.2.0-rc1sept162019+ #1
Hardware name: System manufacturer System Product Name/ROG STRIX B450-F
GAMING, BIOS 2605 08/06/2019
RIP: 0010:write_i2c_retimer_setting+0xc2/0x3c0 [amdgpu]
Code: ff 0f b6 4d ce 44 0f b6 45 cf 44 0f b6 c8 45 89 cf 44 89 e2 48 c7
c6 f0 34 bc c0 bf 04 00 00 00 e8 63 b0 90 ff 45 84 ff 75 02 <0f> 0b 42
0f b6 04 73 8d 50 f6 80 fa 02 77 8c 3c 0a 0f 85 c8 00 00 RSP:
0018:a99d02726fd0 EFLAGS: 00010246
RAX:  RBX: a99d02727035 RCX: 0006
RDX:  RSI: 0002 RDI: 976acc857440
RBP: a99d02727018 R08: 0002 R09: 0002a600
R10: e90610193680 R11: 05e3 R12: 005d
R13: 976ac4b201b8 R14: 0001 R15: 
FS:  7f14f99e1680() GS:976acc84() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 7fdf212843b8 CR3: 000408906000 CR4: 003406e0
Call Trace:
 core_link_enable_stream+0x626/0x680 [amdgpu]
 dce110_apply_ctx_to_hw+0x414/0x4e0 [amdgpu]
 dc_commit_state+0x331/0x5e0 [amdgpu]
 ? drm_calc_timestamping_constants+0xf9/0x150 [drm]
 amdgpu_dm_atomic_commit_tail+0x395/0x1e00 [amdgpu]
 ? dm_plane_helper_prepare_fb+0x20c/0x280 [amdgpu]
 commit_tail+0x42/0x70 [drm_kms_helper]
 drm_atomic_helper_commit+0x10c/0x120 [drm_kms_helper]
 amdgpu_dm_atomic_commit+0x95/0xa0 [amdgpu]
 drm_atomic_commit+0x4a/0x50 [drm]
 restore_fbdev_mode_atomic+0x1c0/0x1e0 [drm_kms_helper]
 restore_fbdev_mode+0x4c/0x160 [drm_kms_helper]
 ? _cond_resched+0x19/0x40
 drm_fb_helper_restore_fbdev_mode_unlocked+0x4e/0xa0 [drm_kms_helper]
 drm_fb_helper_set_par+0x2d/0x50 [drm_kms_helper]
 fbcon_init+0x471/0x630
 visual_init+0xd5/0x130
 do_bind_con_driver+0x20a/0x430
 do_take_over_console+0x7d/0x1b0
 do_fbcon_takeover+0x5c/0xb0
 fbcon_event_notify+0x6cd/0x8a0
 notifier_call_chain+0x4c/0x70
 blocking_notifier_call_chain+0x43/0x60
 fb_notifier_call_chain+0x1b/0x20
 register_framebuffer+0x254/0x360
 __drm_fb_helper_initial_config_and_unlock+0x2c5/0x510 [drm_kms_helper]
 drm_fb_helper_initial_config+0x35/0x40 [drm_kms_helper]
 amdgpu_fbdev_init+0xcd/0x100 [amdgpu]
 amdgpu_device_init+0x1156/0x1930 [amdgpu]
 amdgpu_driver_load_kms+0x8d/0x2e0 [amdgpu]
 drm_dev_register+0x12b/0x1c0 [drm]
 amdgpu_pci_probe+0xd3/0x160 [amdgpu]
 local_pci_probe+0x47/0xa0
 pci_device_probe+0x142/0x1b0
 really_probe+0xf5/0x3d0
 driver_probe_device+0x11b/0x130
 device_driver_attach+0x58/0x60
 __driver_attach+0xa3/0x140
 ? device_driver_attach+0x60/0x60
 ? device_driver_attach+0x60/0x60
 bus_for_each_dev+0x74/0xb0
 ? kmem_cache_alloc_trace+0x1a3/0x1c0
 driver_attach+0x1e/0x20
 bus_add_driver+0x147/0x220
 ? 0xc0cb9000
 driver_register+0x60/0x100
 ? 0xc0cb9000
 __pci_register_driver+0x5a/0x60
 amdgpu_init+0x74/0x83 [amdgpu]
 do_one_initcall+0x4a/0x1fa
 ? _cond_resched+0x19/0x40
 ? kmem_cache_alloc_trace+0x3f/0x1c0
 ? __vunmap+0x1cc/0x200
 do_init_module+0x5f/0x227
 load_module+0x2330/0x2b40
 __do_sys_finit_module+0xfc/0x120
 ? __do_sys_finit_module+0xfc/0x120
 __x64_sys_finit_module+0x1a/0x20
 do_syscall_64+0x5a/0x130
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f14f9500839
Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89
f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 1f f6 2c 00 f7 d8 64 89 01 48
RSP: 002b:7fff9bc4f5a8 EFLAGS: 0246 ORIG_RAX: 

[PATCH 08/12] drm/amd/display: fix a minor HDCP logging error

[Rodrigo Siqueira]

From: Wenjing Liu 

[why]
In HDCP Uninitialzed State, a CPIRQ event would cause log output
internal policy error because the CPIRQ event is not recognized as
unexpected event.

[how]
CPIRQ is issued in HDCP uninitialized state is unexpected.  We should
set unexpected event flag in event ctx.

Signed-off-by: Wenjing Liu 
Reviewed-by: Ashley Thomas 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c 
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
index 7a571b3f62d6..cc1d3f470b99 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
@@ -114,6 +114,9 @@ static enum mod_hdcp_status execution(struct mod_hdcp *hdcp,
} else if (is_in_hdcp2_dp_states(hdcp)) {
status = mod_hdcp_hdcp2_dp_execution(hdcp,
event_ctx, >hdcp2);
+   } else {
+   event_ctx->unexpected_event = 1;
+   goto out;
}
 out:
return status;
-- 
2.25.1

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

[PATCH 10/12] drm/amd/display: writing stereo polarity register if swapped

[Rodrigo Siqueira]

From: Martin Leung 

[why]
on some displays that prefer swapped polarity we were seeing L/R images
swapped because OTG_STEREO_SYNC_OUTPUT_POLARITY would always be mapped
to 0

[how]
fix initial dal3 implementation to properly update the polarity field
according to the crtc_stereo_flags (same as
OTG_STEREO_EYE_FLAG_POLARITY)

Signed-off-by: Martin Leung 
Reviewed-by: Aric Cyr 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
index 94ac34106776..63acb8ff7462 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
@@ -1193,7 +1193,7 @@ static void optc1_enable_stereo(struct timing_generator 
*optc,
REG_UPDATE_3(OTG_STEREO_CONTROL,
OTG_STEREO_EN, stereo_en,
OTG_STEREO_SYNC_OUTPUT_LINE_NUM, 0,
-   OTG_STEREO_SYNC_OUTPUT_POLARITY, 0);
+   OTG_STEREO_SYNC_OUTPUT_POLARITY, 
flags->RIGHT_EYE_POLARITY == 0 ? 0 : 1);
 
if (flags->PROGRAM_POLARITY)
REG_UPDATE(OTG_STEREO_CONTROL,
-- 
2.25.1

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

[PATCH 12/12] drm/amd/display: 3.2.76

[Rodrigo Siqueira]

From: Aric Cyr 

Signed-off-by: Aric Cyr 
Reviewed-by: Aric Cyr 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/dc/dc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dc.h 
b/drivers/gpu/drm/amd/display/dc/dc.h
index 5508c32f4484..1e6413a79d47 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -39,7 +39,7 @@
 #include "inc/hw/dmcu.h"
 #include "dml/display_mode_lib.h"
 
-#define DC_VER "3.2.75"
+#define DC_VER "3.2.76"
 
 #define MAX_SURFACES 3
 #define MAX_PLANES 6
-- 
2.25.1

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

[PATCH 06/12] drm/amd/display: Program DSC during timing programming

[Rodrigo Siqueira]

From: Nikola Cornij 

[why]
Link or DIG BE can't be exposed to a higher stream bandwidth than they
can handle. When DSC is required to fit the stream into the link
bandwidth, DSC has to be programmed during timing programming to ensure
this. Without it, intermittent issues such as black screen after S3 or a
hot-plug can be seen.

[how]
Move DSC programming from enabling stream on link to timing setup.

Signed-off-by: Nikola Cornij 
Reviewed-by: Tony Cheng 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c  | 11 ---
 drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c |  2 +-
 drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c |  7 +++
 drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h|  1 +
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index ddd4dca61cc3..114f77759ebf 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -3078,9 +3078,14 @@ void core_link_enable_stream(
 
if (pipe_ctx->stream->timing.flags.DSC) {
if (dc_is_dp_signal(pipe_ctx->stream->signal) ||
-   
dc_is_virtual_signal(pipe_ctx->stream->signal))
-   dp_set_dsc_enable(pipe_ctx, true);
+   
dc_is_virtual_signal(pipe_ctx->stream->signal)) {
+   /* Here we only need to enable DSC on RX. DSC 
HW programming
+* was done earlier, as part of timing 
programming.
+*/
+   dp_set_dsc_on_rx(pipe_ctx, true);
+   }
}
+
dc->hwss.enable_stream(pipe_ctx);
 
/* Set DPS PPS SDP (AKA "info frames") */
@@ -3107,7 +3112,7 @@ void core_link_enable_stream(
} else { // if (IS_FPGA_MAXIMUS_DC(dc->ctx->dce_environment))
if (dc_is_dp_signal(pipe_ctx->stream->signal) ||
dc_is_virtual_signal(pipe_ctx->stream->signal))
-   dp_set_dsc_enable(pipe_ctx, true);
+   dp_set_dsc_on_rx(pipe_ctx, true);
 
}
 }
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c 
b/drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c
index 51e0ee6e7695..ac2103dec9e7 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c
@@ -394,7 +394,7 @@ static void dsc_optc_config_log(struct 
display_stream_compressor *dsc,
DC_LOG_DSC("\tslice_width %d", config->slice_width);
 }
 
-static bool dp_set_dsc_on_rx(struct pipe_ctx *pipe_ctx, bool enable)
+bool dp_set_dsc_on_rx(struct pipe_ctx *pipe_ctx, bool enable)
 {
struct dc *dc = pipe_ctx->stream->ctx->dc;
struct dc_stream_state *stream = pipe_ctx->stream;
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index b0f61bd7c208..03f0c9914520 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -623,6 +623,13 @@ enum dc_status dcn20_enable_stream_timing(
 
/* TODO check if timing_changed, disable stream if timing changed */
 
+   /* Have to setup DSC here to make sure the bandwidth sent to DIG BE 
won't be bigger than
+* what the link and/or DIG BE can handle. 
VBID[6]/CompressedStream_flag will be automatically
+* set at a later time when the video is enabled (DP_VID_STREAM_EN = 1).
+*/
+   if (pipe_ctx->stream->timing.flags.DSC)
+   dp_set_dsc_on_stream(pipe_ctx, true);
+
for (odm_pipe = pipe_ctx->next_odm_pipe; odm_pipe; odm_pipe = 
odm_pipe->next_odm_pipe) {
opp_inst[opp_cnt] = odm_pipe->stream_res.opp->inst;
opp_cnt++;
diff --git a/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h 
b/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
index e94e5fbf2aa2..64f401e4db54 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
@@ -85,6 +85,7 @@ void dp_set_fec_enable(struct dc_link *link, bool enable);
 bool dp_set_dsc_enable(struct pipe_ctx *pipe_ctx, bool enable);
 bool dp_set_dsc_pps_sdp(struct pipe_ctx *pipe_ctx, bool enable);
 void dp_set_dsc_on_stream(struct pipe_ctx *pipe_ctx, bool enable);
+bool dp_set_dsc_on_rx(struct pipe_ctx *pipe_ctx, bool enable);
 bool dp_update_dsc_config(struct pipe_ctx *pipe_ctx);
 
 #endif /* __DC_LINK_DP_H__ */
-- 
2.25.1

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

[PATCH 05/12] drm/amd/display: Not check wm and clk change flag in optimized bandwidth.

[Rodrigo Siqueira]

From: Yongqiang Sun 

[Why]
System isn't able to enter S0i3 due to not send display count 0 to smu.
When dpms off, clk changed flag is cleared alreay, and it is checked
when doing optimized bandwidth, and update clocks is bypassed due to the
flag is unset.

[How]
Remove check flag incide the function since watermark values and clocks
values are checked during update to determine whether to perform it, no
need to check it again outside the function.

Signed-off-by: Yongqiang Sun 
Reviewed-by: Tony Cheng 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/dc/core/dc.c  |  4 +++
 .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 28 ++-
 .../drm/amd/display/dc/dcn20/dcn20_hwseq.c| 26 +++--
 3 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 6dece1ee30bf..df285f57fe92 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -1378,6 +1378,10 @@ bool dc_post_update_surfaces_to_stream(struct dc *dc)
}
 
dc->hwss.optimize_bandwidth(dc, context);
+
+   dc->clk_optimized_required = false;
+   dc->wm_optimized_required = false;
+
return true;
 }
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c 
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
index 385250e1e3fd..21c7c1b010ec 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
@@ -2717,30 +2717,20 @@ void dcn10_optimize_bandwidth(
hws->funcs.verify_allow_pstate_change_high(dc);
 
if (!IS_FPGA_MAXIMUS_DC(dc->ctx->dce_environment)) {
-   if (context->stream_count == 0) {
+   if (context->stream_count == 0)
context->bw_ctx.bw.dcn.clk.phyclk_khz = 0;
 
-   dc->clk_mgr->funcs->update_clocks(
-   dc->clk_mgr,
-   context,
-   true);
-   } else if (dc->clk_optimized_required || 
IS_DIAG_DC(dc->ctx->dce_environment)) {
-   dc->clk_mgr->funcs->update_clocks(
-   dc->clk_mgr,
-   context,
-   true);
-   }
-   }
-
-   if (dc->wm_optimized_required || IS_DIAG_DC(dc->ctx->dce_environment)) {
-   hubbub->funcs->program_watermarks(hubbub,
-   >bw_ctx.bw.dcn.watermarks,
-   dc->res_pool->ref_clocks.dchub_ref_clock_inKhz 
/ 1000,
+   dc->clk_mgr->funcs->update_clocks(
+   dc->clk_mgr,
+   context,
true);
}
 
-   dc->clk_optimized_required = false;
-   dc->wm_optimized_required = false;
+   hubbub->funcs->program_watermarks(hubbub,
+   >bw_ctx.bw.dcn.watermarks,
+   dc->res_pool->ref_clocks.dchub_ref_clock_inKhz / 1000,
+   true);
+
dcn10_stereo_hw_frame_pack_wa(dc, context);
 
if (dc->debug.pplib_wm_report_mode == WM_REPORT_OVERRIDE)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c 
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index 045ba08c85b4..b0f61bd7c208 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -1660,22 +1660,16 @@ void dcn20_optimize_bandwidth(
 {
struct hubbub *hubbub = dc->res_pool->hubbub;
 
-   if (dc->wm_optimized_required || IS_DIAG_DC(dc->ctx->dce_environment)) {
-   /* program dchubbub watermarks */
-   hubbub->funcs->program_watermarks(hubbub,
-   
>bw_ctx.bw.dcn.watermarks,
-   
dc->res_pool->ref_clocks.dchub_ref_clock_inKhz / 1000,
-   true);
-   dc->wm_optimized_required = false;
-   }
-
-   if (dc->clk_optimized_required || IS_DIAG_DC(dc->ctx->dce_environment)) 
{
-   dc->clk_mgr->funcs->update_clocks(
-   dc->clk_mgr,
-   context,
-   true);
-   dc->clk_optimized_required = false;
-   }
+   /* program dchubbub watermarks */
+   hubbub->funcs->program_watermarks(hubbub,
+   >bw_ctx.bw.dcn.watermarks,
+   
dc->res_pool->ref_clocks.dchub_ref_clock_inKhz / 1000,
+   true);
+
+   dc->clk_mgr->funcs->update_clocks(
+  

[PATCH 07/12] drm/amd/display: determine rx id list bytes to read based on device count

[Rodrigo Siqueira]

From: Wenjing Liu 

[why]
Some RX doesn't like us to read rx id list upto max rx id list size.  As
discussed, we decided to read rx id list based on device count.

[how]
According to HDCP specs the actual size of rx id list is calculated as
rx id list size = 2+3+16+5*device_count.  We will read 16 bytes at a
time until it reached or exceeded rx id list size.

Signed-off-by: Wenjing Liu 
Reviewed-by: Ashley Thomas 
Acked-by: Rodrigo Siqueira 
---
 .../drm/amd/display/modules/hdcp/hdcp_ddc.c   | 24 +++
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c 
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c
index ff9d54812e62..816759d10cbc 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c
@@ -65,6 +65,7 @@ enum mod_hdcp_ddc_message_id {
MOD_HDCP_MESSAGE_ID_READ_LC_SEND_L_PRIME,
MOD_HDCP_MESSAGE_ID_WRITE_SKE_SEND_EKS,
MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST,
+   MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST_PART2,
MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_SEND_ACK,
MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_STREAM_MANAGE,
MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_STREAM_READY,
@@ -101,6 +102,7 @@ static const uint8_t hdcp_i2c_offsets[] = {
[MOD_HDCP_MESSAGE_ID_READ_LC_SEND_L_PRIME] = 0x80,
[MOD_HDCP_MESSAGE_ID_WRITE_SKE_SEND_EKS] = 0x60,
[MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST] = 0x80,
+   [MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST_PART2] = 
0x80,
[MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_SEND_ACK] = 0x60,
[MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_STREAM_MANAGE] = 0x60,
[MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_STREAM_READY] = 0x80,
@@ -135,6 +137,7 @@ static const uint32_t hdcp_dpcd_addrs[] = {
[MOD_HDCP_MESSAGE_ID_READ_LC_SEND_L_PRIME] = 0x692f8,
[MOD_HDCP_MESSAGE_ID_WRITE_SKE_SEND_EKS] = 0x69318,
[MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST] = 0x69330,
+   [MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST_PART2] = 
0x69340,
[MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_SEND_ACK] = 0x693e0,
[MOD_HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_STREAM_MANAGE] = 0x693f0,
[MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_STREAM_READY] = 0x69473,
@@ -474,14 +477,27 @@ enum mod_hdcp_status mod_hdcp_read_l_prime(struct 
mod_hdcp *hdcp)
 
 enum mod_hdcp_status mod_hdcp_read_rx_id_list(struct mod_hdcp *hdcp)
 {
-   enum mod_hdcp_status status;
+   enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS;
 
if (is_dp_hdcp(hdcp)) {
+   uint32_t device_count = 0;
+   uint32_t rx_id_list_size = 0;
+   uint32_t bytes_read = 0;
+
hdcp->auth.msg.hdcp2.rx_id_list[0] = 12;
status = read(hdcp, 
MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST,
-   hdcp->auth.msg.hdcp2.rx_id_list+1,
-   sizeof(hdcp->auth.msg.hdcp2.rx_id_list)-1);
-
+   
hdcp->auth.msg.hdcp2.rx_id_list+1,
+   HDCP_MAX_AUX_TRANSACTION_SIZE);
+   if (status == MOD_HDCP_STATUS_SUCCESS) {
+   bytes_read = HDCP_MAX_AUX_TRANSACTION_SIZE;
+   device_count = 
HDCP_2_2_DEV_COUNT_LO(hdcp->auth.msg.hdcp2.rx_id_list[2]) +
+   
(HDCP_2_2_DEV_COUNT_HI(hdcp->auth.msg.hdcp2.rx_id_list[1]) << 4);
+   rx_id_list_size = MIN((21 + 5 * device_count),
+   
(sizeof(hdcp->auth.msg.hdcp2.rx_id_list) - 1));
+   status = read(hdcp, 
MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST_PART2,
+   hdcp->auth.msg.hdcp2.rx_id_list + 1 + 
bytes_read,
+   (rx_id_list_size - 1) / 
HDCP_MAX_AUX_TRANSACTION_SIZE * HDCP_MAX_AUX_TRANSACTION_SIZE);
+   }
} else {
status = read(hdcp, 
MOD_HDCP_MESSAGE_ID_READ_REPEATER_AUTH_SEND_RECEIVERID_LIST,
hdcp->auth.msg.hdcp2.rx_id_list,
-- 
2.25.1

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

[PATCH 00/12] DC Patches March 03, 2020

[Rodrigo Siqueira]

This DC patchset brings improvements in multiple areas. In summary, we
highlight:
 
* Fix i2c_write issue
* Fix HDCP issues
* Update nv14 soc

Aric Cyr (1):
  drm/amd/display: 3.2.76

Isabel Zhang (1):
  drm/amd/display: Add stay count and bstatus to HDCP log

Martin Leung (2):
  drm/amd/display: update soc bb for nv14
  drm/amd/display: writing stereo polarity register if swapped

Nikola Cornij (1):
  drm/amd/display: Program DSC during timing programming

Rodrigo Siqueira (1):
  drm/amd/display: Stop if retimer is not available

Wenjing Liu (4):
  drm/amd/display: determine is mst hdcp based on stream instead of sink
signal
  drm/amd/display: determine rx id list bytes to read based on device
count
  drm/amd/display: fix a minor HDCP logging error
  drm/amd/display: separate FEC capability from fec debug flag

Yongqiang Sun (1):
  drm/amd/display: Not check wm and clk change flag in optimized
bandwidth.

bradenbakker (1):
  drm/amd/display: Add registry for mem pwr control

 .../amd/display/amdgpu_dm/amdgpu_dm_hdcp.c|   1 +
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  16 +--
 drivers/gpu/drm/amd/display/dc/core/dc.c  |   4 +
 drivers/gpu/drm/amd/display/dc/core/dc_link.c |  83 +++--
 .../gpu/drm/amd/display/dc/core/dc_link_dp.c  |   4 +-
 .../drm/amd/display/dc/core/dc_link_hwss.c|   2 +-
 drivers/gpu/drm/amd/display/dc/dc.h   |  11 +-
 .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  28 ++---
 .../gpu/drm/amd/display/dc/dcn10/dcn10_optc.c |   2 +-
 .../drm/amd/display/dc/dcn20/dcn20_hwseq.c|  31 ++---
 .../drm/amd/display/dc/dcn20/dcn20_resource.c | 113 +-
 drivers/gpu/drm/amd/display/dc/dm_cp_psp.h|   1 +
 .../gpu/drm/amd/display/dc/inc/dc_link_dp.h   |   1 +
 .../gpu/drm/amd/display/modules/hdcp/hdcp.c   |   7 +-
 .../gpu/drm/amd/display/modules/hdcp/hdcp.h   |   6 +-
 .../drm/amd/display/modules/hdcp/hdcp_ddc.c   |  24 +++-
 .../drm/amd/display/modules/hdcp/hdcp_log.h   |   8 +-
 .../drm/amd/display/modules/inc/mod_hdcp.h|   4 +-
 18 files changed, 240 insertions(+), 106 deletions(-)

-- 
2.25.1

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

[PATCH 04/12] drm/amd/display: Add registry for mem pwr control

[Rodrigo Siqueira]

From: bradenbakker 

[What]
Need debug options to control lightl/deep sleep
[How]
Add registry for memory power control

Signed-off-by: Braden Bakker 
Reviewed-by: Charlene Liu 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/dc/dc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dc.h 
b/drivers/gpu/drm/amd/display/dc/dc.h
index abff0da945e7..2b538f477c82 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -390,6 +390,7 @@ struct dc_debug_options {
int always_scale;
bool disable_pplib_clock_request;
bool disable_clock_gate;
+   bool disable_mem_low_power;
bool disable_dmcu;
bool disable_psr;
bool force_abm_enable;
-- 
2.25.1

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

[PATCH 02/12] drm/amd/display: Add stay count and bstatus to HDCP log

[Rodrigo Siqueira]

From: Isabel Zhang 

[Why]
So the values of stay count and bstatus can be easily viewed during
debugging.

[How]
Add stay count and bstatus values to be outputted in HDCP log

Signed-off-by: Isabel Zhang 
Reviewed-by: Wenjing Liu 
Acked-by: Rodrigo Siqueira 
---
 drivers/gpu/drm/amd/display/modules/hdcp/hdcp_log.h | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_log.h 
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_log.h
index 6e844825ad23..d3192b9d0c3d 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_log.h
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_log.h
@@ -37,10 +37,11 @@
 /* default logs */
 #define HDCP_ERROR_TRACE(hdcp, status) \
HDCP_LOG_ERR(hdcp, \
-   "[Link %d] WARNING %s IN STATE %s", \
+   "[Link %d] WARNING %s IN STATE %s STAY COUNT %d", \
hdcp->config.index, \
mod_hdcp_status_to_str(status), \
-   mod_hdcp_state_id_to_str(hdcp->state.id))
+   mod_hdcp_state_id_to_str(hdcp->state.id), \
+   hdcp->state.stay_count)
 #define HDCP_HDCP1_ENABLED_TRACE(hdcp, displayIndex) \
HDCP_LOG_VER(hdcp, \
"[Link %d] HDCP 1.4 enabled on display %d", \
@@ -111,6 +112,9 @@
sizeof(hdcp->auth.msg.hdcp1.bksv)); \
HDCP_DDC_READ_TRACE(hdcp, "BCAPS", >auth.msg.hdcp1.bcaps, 
\
sizeof(hdcp->auth.msg.hdcp1.bcaps)); \
+   HDCP_DDC_READ_TRACE(hdcp, "BSTATUS", \
+   (uint8_t *)>auth.msg.hdcp1.bstatus, \
+   sizeof(hdcp->auth.msg.hdcp1.bstatus)); \
HDCP_DDC_WRITE_TRACE(hdcp, "AN", hdcp->auth.msg.hdcp1.an, \
sizeof(hdcp->auth.msg.hdcp1.an)); \
HDCP_DDC_WRITE_TRACE(hdcp, "AKSV", hdcp->auth.msg.hdcp1.aksv, \
-- 
2.25.1

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

Re: [PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Luben Tuikov]

On 2020-03-03 17:02, Andrey Grodzovsky wrote:
> Add the programming sequence.
> 
> v2:
> Change donwload wait loop to more efficient.
> Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion
> 
> v3: Fix lack of loop counter increment typo
> 
> Signed-off-by: Andrey Grodzovsky 
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 
> +
>  2 files changed, 85 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h 
> b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> index 36b6579..a44fd60 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
> @@ -31,6 +31,9 @@
>  #define GFX_CMD_RESERVED_MASK   0x7FF0
>  #define GFX_CMD_RESPONSE_MASK   0x8000
>  
> +/* USBC PD FW version retrieval command */
> +#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x200
> +
>  /* TEE Gfx Command IDs for the register interface.
>  *  Command ID must be between 0x0001 and 0x000F.
>  */
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 8ab3bf3..3db1c0d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> @@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
>  /* memory training timeout define */
>  #define MEM_TRAIN_SEND_MSG_TIMEOUT_US300
>  
> +/* For large FW files the time to complete can be very long */
> +#define USBC_PD_POLLING_LIMIT_S 240
> +
>  static int psp_v11_0_init_microcode(struct psp_context *psp)
>  {
>   struct amdgpu_device *adev = psp->adev;
> @@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context 
> *psp, uint32_t value)
>   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
>  }
>  
> +static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t 
> dma_addr)
> +{
> + struct amdgpu_device *adev = psp->adev;
> + uint32_t reg_status;
> + int ret, i = 0;
> +
> + /* Write lower 32-bit address of the PD Controller FW */
> + WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
> + ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +  0x8000, 0x8000, false);
> + if (ret)
> + return ret;
> +
> + /* Fireup interrupt so PSP can pick up the lower address */
> + WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x80);
> + ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +  0x8000, 0x8000, false);
> + if (ret)
> + return ret;
> +
> + reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
> +
> + if ((reg_status & 0x) != 0) {
> + DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits 
> [15:0] = %02x...\n",
> + reg_status & 0x);
> + return -EIO;
> + }
> +
> + /* Write upper 32-bit address of the PD Controller FW */
> + WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
> +
> + ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
> +  0x8000, 0x8000, false);
> + if (ret)
> + return ret;
> +
> + /* Fireup interrupt so PSP can pick up the upper address */
> + WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x400);
> +
> + /* FW load takes very long time */
> + do {
> + msleep(1000);
> + reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
> +
> + if (reg_status & 0x8000)
> + goto done;
> +
> + } while(i++ < USBC_PD_POLLING_LIMIT_S);

1. The LKCS wants a space after a keyword ("while") and before parenthesis "(".

2. In do-while loops, you want to pre-increment, in order to account for the
   already executed iteration of the loop, since the check is done _after_
   the loop executes, in contrast to "for" and "while" loops. So you'll need
   to do this:

   } while (++i < USBC_PD_POLLING_LIMIT_S);

> +
> + return -ETIME;
> +done:
> + reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);

3. You had _just_ read the register, right before the goto jump.
   You do not need to read it again. (Else a race would exist,
   and you'd need to poll, _again_.) You shouldn't have to
   read it again.

> +
> + if ((reg_status & 0x) != 0) {
> + DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits 
> [15:0] = x%04x\n",
> + reg_status & 0x);
> + return -EIO;
> + }

With those fixed, this patch is,
Reviewed-by: Luben Tuikov 

Regards,
Luben



> +
> + return 0;
> +}
> +
> +static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t 
> *fw_ver)
> +{
> + struct amdgpu_device *adev = psp->adev;
> + int ret;
> +
> + WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 

Re: [PATCH 1/4] drm/amdgpu/vcn: fix race condition issue for vcn start

[James Zhu]

On 2020-03-03 2:03 p.m., James Zhu wrote:


On 2020-03-03 1:44 p.m., Christian König wrote:

Am 03.03.20 um 19:16 schrieb James Zhu:

Fix race condition issue when multiple vcn starts are called.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 +
  2 files changed, 5 insertions(+)

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

index f96464e..aa7663f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -63,6 +63,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
  int i, r;
    INIT_DELAYED_WORK(>vcn.idle_work, 
amdgpu_vcn_idle_work_handler);

+    mutex_init(>vcn.vcn_pg_lock);
    switch (adev->asic_type) {
  case CHIP_RAVEN:
@@ -210,6 +211,7 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
  }
    release_firmware(adev->vcn.fw);
+    mutex_destroy(>vcn.vcn_pg_lock);
    return 0;
  }
@@ -321,6 +323,7 @@ void amdgpu_vcn_ring_begin_use(struct 
amdgpu_ring *ring)

  struct amdgpu_device *adev = ring->adev;
  bool set_clocks = 
!cancel_delayed_work_sync(>vcn.idle_work);

  +    mutex_lock(>vcn.vcn_pg_lock);


That still won't work correctly here.

The whole idea of the cancel_delayed_work_sync() and 
schedule_delayed_work() dance is that you have exactly one user of 
that. If you have multiple rings that whole thing won't work correctly.


To fix this you need to call mutex_lock() before 
cancel_delayed_work_sync() and schedule_delayed_work() before 
mutex_unlock().


Big lock definitely works. I am trying to use as smaller lock as 
possible here. the share resource which needs protect here are power 
gate process and dpg mode switch process.


if we move mutex_unlock() before schedule_delayed_work(. I am 
wondering what are the other necessary resources which need protect.


By the way, cancel_delayed_work_sync() supports multiple thread itself, 
so I didn't put it into protection area.  power  gate is shared by all 
VCN IP instances and different rings , so it  needs be put into 
protection area.


each ring's job itself is serialized by scheduler. it doesn't need be  
put into this protection area.




Thanks!

James



Regards,
Christian.


  if (set_clocks) {
  amdgpu_gfx_off_ctrl(adev, false);
  amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCN,
@@ -345,6 +348,7 @@ void amdgpu_vcn_ring_begin_use(struct 
amdgpu_ring *ring)

    adev->vcn.pause_dpg_mode(adev, ring->me, _state);
  }
+    mutex_unlock(>vcn.vcn_pg_lock);
  }
    void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h

index 6fe0573..2ae110d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -200,6 +200,7 @@ struct amdgpu_vcn {
  struct drm_gpu_scheduler 
*vcn_dec_sched[AMDGPU_MAX_VCN_INSTANCES];

  uint32_t num_vcn_enc_sched;
  uint32_t num_vcn_dec_sched;
+    struct mutex vcn_pg_lock;
    unsigned    harvest_config;
  int (*pause_dpg_mode)(struct amdgpu_device *adev,



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

[PATCH v3 1/3] drm/amdgpu: Add USBC PD FW load interface to PSP.

[Andrey Grodzovsky]

Used to load power Delivery FW to PSP.

Signed-off-by: Andrey Grodzovsky 
Reviewed-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 37fa184..297435c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -114,6 +114,8 @@ struct psp_funcs
int (*mem_training)(struct psp_context *psp, uint32_t ops);
uint32_t (*ring_get_wptr)(struct psp_context *psp);
void (*ring_set_wptr)(struct psp_context *psp, uint32_t value);
+   int (*load_usbc_pd_fw)(struct psp_context *psp, dma_addr_t dma_addr);
+   int (*read_usbc_pd_fw)(struct psp_context *psp, uint32_t *fw_ver);
 };
 
 #define AMDGPU_XGMI_MAX_CONNECTED_NODES64
@@ -351,6 +353,14 @@ struct amdgpu_psp_funcs {
 #define psp_ring_get_wptr(psp) (psp)->funcs->ring_get_wptr((psp))
 #define psp_ring_set_wptr(psp, value) (psp)->funcs->ring_set_wptr((psp), 
(value))
 
+#define psp_load_usbc_pd_fw(psp, dma_addr) \
+   ((psp)->funcs->load_usbc_pd_fw ? \
+   (psp)->funcs->load_usbc_pd_fw((psp), (dma_addr)) : -EINVAL)
+
+#define psp_read_usbc_pd_fw(psp, fw_ver) \
+   ((psp)->funcs->read_usbc_pd_fw ? \
+   (psp)->funcs->read_usbc_pd_fw((psp), fw_ver) : -EINVAL)
+
 extern const struct amd_ip_funcs psp_ip_funcs;
 
 extern const struct amdgpu_ip_block_version psp_v3_1_ip_block;
-- 
2.7.4

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

[PATCH v3 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

Add the programming sequence.

v2:
Change donwload wait loop to more efficient.
Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion

v3: Fix lack of loop counter increment typo

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +
 2 files changed, 85 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h 
b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
index 36b6579..a44fd60 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
+++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
@@ -31,6 +31,9 @@
 #define GFX_CMD_RESERVED_MASK   0x7FF0
 #define GFX_CMD_RESPONSE_MASK   0x8000
 
+/* USBC PD FW version retrieval command */
+#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x200
+
 /* TEE Gfx Command IDs for the register interface.
 *  Command ID must be between 0x0001 and 0x000F.
 */
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 8ab3bf3..3db1c0d 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
 /* memory training timeout define */
 #define MEM_TRAIN_SEND_MSG_TIMEOUT_US  300
 
+/* For large FW files the time to complete can be very long */
+#define USBC_PD_POLLING_LIMIT_S 240
+
 static int psp_v11_0_init_microcode(struct psp_context *psp)
 {
struct amdgpu_device *adev = psp->adev;
@@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context 
*psp, uint32_t value)
WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
 }
 
+static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t 
dma_addr)
+{
+   struct amdgpu_device *adev = psp->adev;
+   uint32_t reg_status;
+   int ret, i = 0;
+
+   /* Write lower 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   /* Fireup interrupt so PSP can pick up the lower address */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x80);
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits 
[15:0] = %02x...\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   /* Write upper 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
+
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   /* Fireup interrupt so PSP can pick up the upper address */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x400);
+
+   /* FW load takes very long time */
+   do {
+   msleep(1000);
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if (reg_status & 0x8000)
+   goto done;
+
+   } while(i++ < USBC_PD_POLLING_LIMIT_S);
+
+   return -ETIME;
+done:
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits 
[15:0] = x%04x\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   return 0;
+}
+
+static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
+{
+   struct amdgpu_device *adev = psp->adev;
+   int ret;
+
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
+
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (!ret)
+   *fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
+
+   return ret;
+}
+
 static const struct psp_funcs psp_v11_0_funcs = {
.init_microcode = psp_v11_0_init_microcode,
.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
@@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
.mem_training = psp_v11_0_memory_training,
.ring_get_wptr = psp_v11_0_ring_get_wptr,
.ring_set_wptr = psp_v11_0_ring_set_wptr,
+   .load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
+   .read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
 };
 
 void psp_v11_0_set_psp_funcs(struct psp_context *psp)
-- 
2.7.4

[PATCH v3 3/3] drm/amdgpu: Add support for USBC PD FW download

[Andrey Grodzovsky]

Starts USBC PD FW download and reads back the latest FW version.

v2:
Move sysfs file creation to late init
Add locking around PSP calls to avoid concurrent access to PSP's C2P registers

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index d33f741..cff0fd2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -24,6 +24,7 @@
  */
 
 #include 
+#include 
 
 #include "amdgpu.h"
 #include "amdgpu_psp.h"
@@ -38,6 +39,9 @@
 
 static void psp_set_funcs(struct amdgpu_device *adev);
 
+static int psp_sysfs_init(struct amdgpu_device *adev);
+static void psp_sysfs_fini(struct amdgpu_device *adev);
+
 /*
  * Due to DF Cstate management centralized to PMFW, the firmware
  * loading sequence will be updated as below:
@@ -113,6 +117,16 @@ static int psp_early_init(void *handle)
return 0;
 }
 
+static int psp_late_init(void *handle)
+{
+   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+
+   if (adev->asic_type == CHIP_NAVI10)
+   return psp_sysfs_init(adev);
+
+   return 0;
+}
+
 static int psp_sw_init(void *handle)
 {
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
@@ -152,6 +166,10 @@ static int psp_sw_fini(void *handle)
release_firmware(adev->psp.ta_fw);
adev->psp.ta_fw = NULL;
}
+
+   if (adev->asic_type == CHIP_NAVI10)
+   psp_sysfs_fini(adev);
+
return 0;
 }
 
@@ -1816,10 +1834,85 @@ static int psp_set_powergating_state(void *handle,
return 0;
 }
 
+static ssize_t psp_usbc_pd_fw_sysfs_read(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct drm_device *ddev = dev_get_drvdata(dev);
+   struct amdgpu_device *adev = ddev->dev_private;
+   uint32_t fw_ver;
+   int ret;
+
+   mutex_lock(>psp.mutex);
+   ret = psp_read_usbc_pd_fw(>psp, _ver);
+   mutex_unlock(>psp.mutex);
+
+   if (ret) {
+   DRM_ERROR("Failed to read USBC PD FW, err = %d", ret);
+   return ret;
+   }
+
+   return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver);
+}
+
+static ssize_t psp_usbc_pd_fw_sysfs_write(struct device *dev,
+  struct device_attribute 
*attr,
+  const char *buf,
+  size_t count)
+{
+   struct drm_device *ddev = dev_get_drvdata(dev);
+   struct amdgpu_device *adev = ddev->dev_private;
+   void *cpu_addr;
+   dma_addr_t dma_addr;
+   int ret;
+   char fw_name[100];
+   const struct firmware *usbc_pd_fw;
+
+
+   snprintf(fw_name, sizeof(fw_name), "amdgpu/%s", buf);
+   ret = request_firmware(_pd_fw, fw_name, adev->dev);
+   if (ret)
+   goto fail;
+
+   /* We need contiguous physical mem to place the FW  for psp to access */
+   cpu_addr = dma_alloc_coherent(adev->dev, usbc_pd_fw->size, _addr, 
GFP_KERNEL);
+
+   ret = dma_mapping_error(adev->dev, dma_addr);
+   if (ret)
+   goto rel_buf;
+
+   memcpy_toio(cpu_addr, usbc_pd_fw->data, usbc_pd_fw->size);
+
+   /*TODO Remove once PSP starts snooping CPU cache */
+   clflush_cache_range(cpu_addr, (usbc_pd_fw->size & ~(L1_CACHE_BYTES - 
1)));
+
+   mutex_lock(>psp.mutex);
+   ret = psp_load_usbc_pd_fw(>psp, dma_addr);
+   mutex_unlock(>psp.mutex);
+
+rel_buf:
+   dma_free_coherent(adev->dev, usbc_pd_fw->size, cpu_addr, dma_addr);
+   release_firmware(usbc_pd_fw);
+
+fail:
+   if (ret) {
+   DRM_ERROR("Failed to load USBC PD FW, err = %d", ret);
+   return ret;
+   }
+
+   return count;
+}
+
+static DEVICE_ATTR(usbc_pd_fw, S_IRUGO | S_IWUSR,
+  psp_usbc_pd_fw_sysfs_read,
+  psp_usbc_pd_fw_sysfs_write);
+
+
+
 const struct amd_ip_funcs psp_ip_funcs = {
.name = "psp",
.early_init = psp_early_init,
-   .late_init = NULL,
+   .late_init = psp_late_init,
.sw_init = psp_sw_init,
.sw_fini = psp_sw_fini,
.hw_init = psp_hw_init,
@@ -1834,6 +1927,21 @@ const struct amd_ip_funcs psp_ip_funcs = {
.set_powergating_state = psp_set_powergating_state,
 };
 
+static int psp_sysfs_init(struct amdgpu_device *adev)
+{
+   int ret = device_create_file(adev->dev, _attr_usbc_pd_fw);
+
+   if (ret)
+   DRM_ERROR("Failed to create USBC PD FW control file!");
+
+   return ret;
+}
+
+static void psp_sysfs_fini(struct amdgpu_device *adev)
+{
+   device_remove_file(adev->dev, _attr_usbc_pd_fw);
+}
+
 static const struct amdgpu_psp_funcs psp_funcs = {

[PATCH v3 0/3] Add support for USBC PD FW download

[Andrey Grodzovsky]

This patchset adds the kernel driver part of supporting USBC power delivery
firmware downloading to USBC chip on the ASIC. The FW is placed in DMA buffer
visible to PSP which then proceeds and copies the FW over I2C to the USBC chip.


Andrey Grodzovsky (3):
  drm/amdgpu: Add USBC PD FW load interface to PSP.
  drm/amdgpu: Add USBC PD FW load to PSP 11
  drm/amdgpu: Add support for USBC PD FW download

 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  10 +++
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |   3 +
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  |  82 
 4 files changed, 204 insertions(+), 1 deletion(-)

-- 
2.7.4

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

Re: [PATCH v2 0/3] Add support for USBC PD FW download

[Andrey Grodzovsky]

Please ignore this patchset as there is a typo error in second patch, 
will resend fixed V3 in a moment.


Andrey

On 3/3/20 4:54 PM, Andrey Grodzovsky wrote:

This patchset adds the kernel driver part of supporting USBC power delivery
firmware downloading to USBC chip on the ASIC. The FW is placed in DMA buffer
visible to PSP which then proceeds and copies the FW over I2C to the USBC chip.

Andrey Grodzovsky (3):
   drm/amdgpu: Add USBC PD FW load interface to PSP.
   drm/amdgpu: Add USBC PD FW load to PSP 11
   drm/amdgpu: Add support for USBC PD FW download

  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  10 +++
  drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |   3 +
  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  |  82 
  4 files changed, 204 insertions(+), 1 deletion(-)


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

[PATCH v2 3/3] drm/amdgpu: Add support for USBC PD FW download

[Andrey Grodzovsky]

Starts USBC PD FW download and reads back the latest FW version.

v2:
Move sysfs file creation to late init
Add locking around PSP calls to avoid concurrent access to PSP's C2P registers

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index d33f741..cff0fd2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -24,6 +24,7 @@
  */
 
 #include 
+#include 
 
 #include "amdgpu.h"
 #include "amdgpu_psp.h"
@@ -38,6 +39,9 @@
 
 static void psp_set_funcs(struct amdgpu_device *adev);
 
+static int psp_sysfs_init(struct amdgpu_device *adev);
+static void psp_sysfs_fini(struct amdgpu_device *adev);
+
 /*
  * Due to DF Cstate management centralized to PMFW, the firmware
  * loading sequence will be updated as below:
@@ -113,6 +117,16 @@ static int psp_early_init(void *handle)
return 0;
 }
 
+static int psp_late_init(void *handle)
+{
+   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
+
+   if (adev->asic_type == CHIP_NAVI10)
+   return psp_sysfs_init(adev);
+
+   return 0;
+}
+
 static int psp_sw_init(void *handle)
 {
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
@@ -152,6 +166,10 @@ static int psp_sw_fini(void *handle)
release_firmware(adev->psp.ta_fw);
adev->psp.ta_fw = NULL;
}
+
+   if (adev->asic_type == CHIP_NAVI10)
+   psp_sysfs_fini(adev);
+
return 0;
 }
 
@@ -1816,10 +1834,85 @@ static int psp_set_powergating_state(void *handle,
return 0;
 }
 
+static ssize_t psp_usbc_pd_fw_sysfs_read(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+   struct drm_device *ddev = dev_get_drvdata(dev);
+   struct amdgpu_device *adev = ddev->dev_private;
+   uint32_t fw_ver;
+   int ret;
+
+   mutex_lock(>psp.mutex);
+   ret = psp_read_usbc_pd_fw(>psp, _ver);
+   mutex_unlock(>psp.mutex);
+
+   if (ret) {
+   DRM_ERROR("Failed to read USBC PD FW, err = %d", ret);
+   return ret;
+   }
+
+   return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver);
+}
+
+static ssize_t psp_usbc_pd_fw_sysfs_write(struct device *dev,
+  struct device_attribute 
*attr,
+  const char *buf,
+  size_t count)
+{
+   struct drm_device *ddev = dev_get_drvdata(dev);
+   struct amdgpu_device *adev = ddev->dev_private;
+   void *cpu_addr;
+   dma_addr_t dma_addr;
+   int ret;
+   char fw_name[100];
+   const struct firmware *usbc_pd_fw;
+
+
+   snprintf(fw_name, sizeof(fw_name), "amdgpu/%s", buf);
+   ret = request_firmware(_pd_fw, fw_name, adev->dev);
+   if (ret)
+   goto fail;
+
+   /* We need contiguous physical mem to place the FW  for psp to access */
+   cpu_addr = dma_alloc_coherent(adev->dev, usbc_pd_fw->size, _addr, 
GFP_KERNEL);
+
+   ret = dma_mapping_error(adev->dev, dma_addr);
+   if (ret)
+   goto rel_buf;
+
+   memcpy_toio(cpu_addr, usbc_pd_fw->data, usbc_pd_fw->size);
+
+   /*TODO Remove once PSP starts snooping CPU cache */
+   clflush_cache_range(cpu_addr, (usbc_pd_fw->size & ~(L1_CACHE_BYTES - 
1)));
+
+   mutex_lock(>psp.mutex);
+   ret = psp_load_usbc_pd_fw(>psp, dma_addr);
+   mutex_unlock(>psp.mutex);
+
+rel_buf:
+   dma_free_coherent(adev->dev, usbc_pd_fw->size, cpu_addr, dma_addr);
+   release_firmware(usbc_pd_fw);
+
+fail:
+   if (ret) {
+   DRM_ERROR("Failed to load USBC PD FW, err = %d", ret);
+   return ret;
+   }
+
+   return count;
+}
+
+static DEVICE_ATTR(usbc_pd_fw, S_IRUGO | S_IWUSR,
+  psp_usbc_pd_fw_sysfs_read,
+  psp_usbc_pd_fw_sysfs_write);
+
+
+
 const struct amd_ip_funcs psp_ip_funcs = {
.name = "psp",
.early_init = psp_early_init,
-   .late_init = NULL,
+   .late_init = psp_late_init,
.sw_init = psp_sw_init,
.sw_fini = psp_sw_fini,
.hw_init = psp_hw_init,
@@ -1834,6 +1927,21 @@ const struct amd_ip_funcs psp_ip_funcs = {
.set_powergating_state = psp_set_powergating_state,
 };
 
+static int psp_sysfs_init(struct amdgpu_device *adev)
+{
+   int ret = device_create_file(adev->dev, _attr_usbc_pd_fw);
+
+   if (ret)
+   DRM_ERROR("Failed to create USBC PD FW control file!");
+
+   return ret;
+}
+
+static void psp_sysfs_fini(struct amdgpu_device *adev)
+{
+   device_remove_file(adev->dev, _attr_usbc_pd_fw);
+}
+
 static const struct amdgpu_psp_funcs psp_funcs = {

[PATCH v2 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

Add the programming sequence.

v2:
Change donwload wait loop to more efficient.
Move C2PMSG_CMD_GFX_USB_PD_FW_VER defintion

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |  3 ++
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  | 82 +
 2 files changed, 85 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h 
b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
index 36b6579..a44fd60 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
+++ b/drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h
@@ -31,6 +31,9 @@
 #define GFX_CMD_RESERVED_MASK   0x7FF0
 #define GFX_CMD_RESPONSE_MASK   0x8000
 
+/* USBC PD FW version retrieval command */
+#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x200
+
 /* TEE Gfx Command IDs for the register interface.
 *  Command ID must be between 0x0001 and 0x000F.
 */
diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 8ab3bf3..2ff5f2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
 /* memory training timeout define */
 #define MEM_TRAIN_SEND_MSG_TIMEOUT_US  300
 
+/* For large FW files the time to complete can be very long */
+#define USBC_PD_POLLING_LIMIT_S 240
+
 static int psp_v11_0_init_microcode(struct psp_context *psp)
 {
struct amdgpu_device *adev = psp->adev;
@@ -1109,6 +1112,83 @@ static void psp_v11_0_ring_set_wptr(struct psp_context 
*psp, uint32_t value)
WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
 }
 
+static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t 
dma_addr)
+{
+   struct amdgpu_device *adev = psp->adev;
+   uint32_t reg_status;
+   int ret, i = 0;
+
+   /* Write lower 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   /* Fireup interrupt so PSP can pick up the lower address */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x80);
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits 
[15:0] = %02x...\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   /* Write upper 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
+
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   /* Fireup interrupt so PSP can pick up the upper address */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x400);
+
+   /* FW load takes very long time */
+   do {
+   msleep(1000);
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if (reg_status & 0x8000)
+   goto done;
+
+   } while(i < USBC_PD_POLLING_LIMIT_S);
+
+   return -ETIME;
+done:
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits 
[15:0] = x%04x\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   return 0;
+}
+
+static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
+{
+   struct amdgpu_device *adev = psp->adev;
+   int ret;
+
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
+
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (!ret)
+   *fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
+
+   return ret;
+}
+
 static const struct psp_funcs psp_v11_0_funcs = {
.init_microcode = psp_v11_0_init_microcode,
.bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
@@ -1133,6 +1213,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
.mem_training = psp_v11_0_memory_training,
.ring_get_wptr = psp_v11_0_ring_get_wptr,
.ring_set_wptr = psp_v11_0_ring_set_wptr,
+   .load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
+   .read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
 };
 
 void psp_v11_0_set_psp_funcs(struct psp_context *psp)
-- 
2.7.4

___
amd-gfx mailing list

[PATCH v2 0/3] Add support for USBC PD FW download

[Andrey Grodzovsky]

This patchset adds the kernel driver part of supporting USBC power delivery
firmware downloading to USBC chip on the ASIC. The FW is placed in DMA buffer
visible to PSP which then proceeds and copies the FW over I2C to the USBC chip.

Andrey Grodzovsky (3):
  drm/amdgpu: Add USBC PD FW load interface to PSP.
  drm/amdgpu: Add USBC PD FW load to PSP 11
  drm/amdgpu: Add support for USBC PD FW download

 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 110 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  10 +++
 drivers/gpu/drm/amd/amdgpu/psp_gfx_if.h |   3 +
 drivers/gpu/drm/amd/amdgpu/psp_v11_0.c  |  82 
 4 files changed, 204 insertions(+), 1 deletion(-)

-- 
2.7.4

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

[PATCH v2 1/3] drm/amdgpu: Add USBC PD FW load interface to PSP.

[Andrey Grodzovsky]

Used to load power Delivery FW to PSP.

Signed-off-by: Andrey Grodzovsky 
Reviewed-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index 37fa184..297435c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -114,6 +114,8 @@ struct psp_funcs
int (*mem_training)(struct psp_context *psp, uint32_t ops);
uint32_t (*ring_get_wptr)(struct psp_context *psp);
void (*ring_set_wptr)(struct psp_context *psp, uint32_t value);
+   int (*load_usbc_pd_fw)(struct psp_context *psp, dma_addr_t dma_addr);
+   int (*read_usbc_pd_fw)(struct psp_context *psp, uint32_t *fw_ver);
 };
 
 #define AMDGPU_XGMI_MAX_CONNECTED_NODES64
@@ -351,6 +353,14 @@ struct amdgpu_psp_funcs {
 #define psp_ring_get_wptr(psp) (psp)->funcs->ring_get_wptr((psp))
 #define psp_ring_set_wptr(psp, value) (psp)->funcs->ring_set_wptr((psp), 
(value))
 
+#define psp_load_usbc_pd_fw(psp, dma_addr) \
+   ((psp)->funcs->load_usbc_pd_fw ? \
+   (psp)->funcs->load_usbc_pd_fw((psp), (dma_addr)) : -EINVAL)
+
+#define psp_read_usbc_pd_fw(psp, fw_ver) \
+   ((psp)->funcs->read_usbc_pd_fw ? \
+   (psp)->funcs->read_usbc_pd_fw((psp), fw_ver) : -EINVAL)
+
 extern const struct amd_ip_funcs psp_ip_funcs;
 
 extern const struct amdgpu_ip_block_version psp_v3_1_ip_block;
-- 
2.7.4

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

Re: [PATCH] drm/amdgpu/sriov: skip programing some regs with new L1 policy

[Deucher, Alexander]

[AMD Public Use]

Acked-by: Alex Deucher 


From: amd-gfx  on behalf of Zhou, 
Tiecheng 
Sent: Tuesday, March 3, 2020 6:04 AM
To: Zhou, Tiecheng ; amd-gfx@lists.freedesktop.org 

Subject: RE: [PATCH] drm/amdgpu/sriov: skip programing some regs with new L1 
policy

[AMD Official Use Only - Internal Distribution Only]

Ping

-Original Message-
From: Tiecheng Zhou 
Sent: Monday, March 2, 2020 3:08 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou, Tiecheng 
Subject: [PATCH] drm/amdgpu/sriov: skip programing some regs with new L1 policy

With new L1 policy, some regs are blocked at guest and they are programed at 
host side. So skip programing the regs under sriov.

the regs are:
GCMC_VM_FB_LOCATION_TOP
GCMC_VM_FB_LOCATION_BASE
MMMC_VM_FB_LOCATION_TOP
MMMC_VM_FB_LOCATION_BASE
GCMC_VM_SYSTEM_APERTURE_HIGH_ADDR
GCMC_VM_SYSTEM_APERTURE_LOW_ADDR
MMMC_VM_SYSTEM_APERTURE_HIGH_ADDR
MMMC_VM_SYSTEM_APERTURE_LOW_ADDR
HDP_NONSURFACE_BASE
HDP_NONSURFACE_BASE_HI
GCMC_VM_AGP_TOP
GCMC_VM_AGP_BOT
GCMC_VM_AGP_BASE

Signed-off-by: Tiecheng Zhou 
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c | 55 +++-  
drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c  | 29 ++---
 2 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
index e0654a216ab5..cc866c367939 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
@@ -81,24 +81,31 @@ static void gfxhub_v2_0_init_system_aperture_regs(struct 
amdgpu_device *adev)  {
 uint64_t value;

-   /* Disable AGP. */
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BOT, 0x00FF);
-
-   /* Program the system aperture low logical page number. */
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
-
-   /* Set default page address. */
-   value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
-   + adev->vm_manager.vram_base_offset;
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_LSB,
-(u32)(value >> 12));
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_MSB,
-(u32)(value >> 44));
+   if (!amdgpu_sriov_vf(adev)) {
+   /*
+* the new L1 policy will block SRIOV guest from writing
+* these regs, and they will be programed at host.
+* so skip programing these regs.
+*/
+   /* Disable AGP. */
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BASE, 0);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_TOP, 0);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BOT, 0x00FF);
+
+   /* Program the system aperture low logical page number. */
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_LOW_ADDR,
+adev->gmc.vram_start >> 18);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
+adev->gmc.vram_end >> 18);
+
+   /* Set default page address. */
+   value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
+   + adev->vm_manager.vram_base_offset;
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_LSB,
+(u32)(value >> 12));
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_MSB,
+(u32)(value >> 44));
+   }

 /* Program "protection fault". */
 WREG32_SOC15(GC, 0, mmGCVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32,
@@ -260,18 +267,6 @@ static void gfxhub_v2_0_program_invalidation(struct 
amdgpu_device *adev)

 int gfxhub_v2_0_gart_enable(struct amdgpu_device *adev)  {
-   if (amdgpu_sriov_vf(adev)) {
-   /*
-* GCMC_VM_FB_LOCATION_BASE/TOP is NULL for VF, becuase they are
-* VF copy registers so vbios post doesn't program them, for
-* SRIOV driver need to program them
-*/
-   WREG32_SOC15(GC, 0, mmGCMC_VM_FB_LOCATION_BASE,
-adev->gmc.vram_start >> 24);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_FB_LOCATION_TOP,
-adev->gmc.vram_end >> 24);
-   }
-
 /* GART Enable. */
 gfxhub_v2_0_init_gart_aperture_regs(adev);
 gfxhub_v2_0_init_system_aperture_regs(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c 
b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
index bde189680521..fb3f228458e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
@@ -72,11 +72,18 @@ static void 

[PATCH 5.4 080/152] drm/radeon: Inline drm_get_pci_dev

[Greg Kroah-Hartman]

From: Daniel Vetter 

commit eb12c957735b582607e5842a06d1f4c62e185c1d upstream.

It's the last user, and more importantly, it's the last non-legacy
user of anything in drm_pci.c.

The only tricky bit is the agp initialization. But a close look shows
that radeon does not use the drm_agp midlayer (the main use of that is
drm_bufs for legacy drivers), and instead could use the agp subsystem
directly (like nouveau does already). Hence we can just pull this in
too.

A further step would be to entirely drop the use of drm_device->agp,
but feels like too much churn just for this patch.

Signed-off-by: Daniel Vetter 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "David (ChunMing) Zhou" 
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Alex Deucher 
Reviewed-by: Emil Velikov 
Signed-off-by: Alex Deucher 
Cc: sta...@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/gpu/drm/radeon/radeon_drv.c |   43 ++--
 drivers/gpu/drm/radeon/radeon_kms.c |6 +
 2 files changed, 47 insertions(+), 2 deletions(-)

--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -325,6 +326,7 @@ static int radeon_pci_probe(struct pci_d
const struct pci_device_id *ent)
 {
unsigned long flags = 0;
+   struct drm_device *dev;
int ret;
 
if (!ent)
@@ -365,7 +367,44 @@ static int radeon_pci_probe(struct pci_d
if (ret)
return ret;
 
-   return drm_get_pci_dev(pdev, ent, _driver);
+   dev = drm_dev_alloc(_driver, >dev);
+   if (IS_ERR(dev))
+   return PTR_ERR(dev);
+
+   ret = pci_enable_device(pdev);
+   if (ret)
+   goto err_free;
+
+   dev->pdev = pdev;
+#ifdef __alpha__
+   dev->hose = pdev->sysdata;
+#endif
+
+   pci_set_drvdata(pdev, dev);
+
+   if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
+   dev->agp = drm_agp_init(dev);
+   if (dev->agp) {
+   dev->agp->agp_mtrr = arch_phys_wc_add(
+   dev->agp->agp_info.aper_base,
+   dev->agp->agp_info.aper_size *
+   1024 * 1024);
+   }
+
+   ret = drm_dev_register(dev, ent->driver_data);
+   if (ret)
+   goto err_agp;
+
+   return 0;
+
+err_agp:
+   if (dev->agp)
+   arch_phys_wc_del(dev->agp->agp_mtrr);
+   kfree(dev->agp);
+   pci_disable_device(pdev);
+err_free:
+   drm_dev_put(dev);
+   return ret;
 }
 
 static void
@@ -578,7 +617,7 @@ radeon_get_crtc_scanout_position(struct
 
 static struct drm_driver kms_driver = {
.driver_features =
-   DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER,
+   DRIVER_GEM | DRIVER_RENDER,
.load = radeon_driver_load_kms,
.open = radeon_driver_open_kms,
.postclose = radeon_driver_postclose_kms,
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -77,6 +78,11 @@ void radeon_driver_unload_kms(struct drm
radeon_modeset_fini(rdev);
radeon_device_fini(rdev);
 
+   if (dev->agp)
+   arch_phys_wc_del(dev->agp->agp_mtrr);
+   kfree(dev->agp);
+   dev->agp = NULL;
+
 done_free:
kfree(rdev);
dev->dev_private = NULL;


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

[PATCH 5.5 094/176] drm/radeon: Inline drm_get_pci_dev

[Greg Kroah-Hartman]

From: Daniel Vetter 

commit eb12c957735b582607e5842a06d1f4c62e185c1d upstream.

It's the last user, and more importantly, it's the last non-legacy
user of anything in drm_pci.c.

The only tricky bit is the agp initialization. But a close look shows
that radeon does not use the drm_agp midlayer (the main use of that is
drm_bufs for legacy drivers), and instead could use the agp subsystem
directly (like nouveau does already). Hence we can just pull this in
too.

A further step would be to entirely drop the use of drm_device->agp,
but feels like too much churn just for this patch.

Signed-off-by: Daniel Vetter 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "David (ChunMing) Zhou" 
Cc: amd-gfx@lists.freedesktop.org
Reviewed-by: Alex Deucher 
Reviewed-by: Emil Velikov 
Signed-off-by: Alex Deucher 
Cc: sta...@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/gpu/drm/radeon/radeon_drv.c |   43 ++--
 drivers/gpu/drm/radeon/radeon_kms.c |6 +
 2 files changed, 47 insertions(+), 2 deletions(-)

--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -325,6 +326,7 @@ static int radeon_pci_probe(struct pci_d
const struct pci_device_id *ent)
 {
unsigned long flags = 0;
+   struct drm_device *dev;
int ret;
 
if (!ent)
@@ -365,7 +367,44 @@ static int radeon_pci_probe(struct pci_d
if (ret)
return ret;
 
-   return drm_get_pci_dev(pdev, ent, _driver);
+   dev = drm_dev_alloc(_driver, >dev);
+   if (IS_ERR(dev))
+   return PTR_ERR(dev);
+
+   ret = pci_enable_device(pdev);
+   if (ret)
+   goto err_free;
+
+   dev->pdev = pdev;
+#ifdef __alpha__
+   dev->hose = pdev->sysdata;
+#endif
+
+   pci_set_drvdata(pdev, dev);
+
+   if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
+   dev->agp = drm_agp_init(dev);
+   if (dev->agp) {
+   dev->agp->agp_mtrr = arch_phys_wc_add(
+   dev->agp->agp_info.aper_base,
+   dev->agp->agp_info.aper_size *
+   1024 * 1024);
+   }
+
+   ret = drm_dev_register(dev, ent->driver_data);
+   if (ret)
+   goto err_agp;
+
+   return 0;
+
+err_agp:
+   if (dev->agp)
+   arch_phys_wc_del(dev->agp->agp_mtrr);
+   kfree(dev->agp);
+   pci_disable_device(pdev);
+err_free:
+   drm_dev_put(dev);
+   return ret;
 }
 
 static void
@@ -575,7 +614,7 @@ radeon_get_crtc_scanout_position(struct
 
 static struct drm_driver kms_driver = {
.driver_features =
-   DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER,
+   DRIVER_GEM | DRIVER_RENDER,
.load = radeon_driver_load_kms,
.open = radeon_driver_open_kms,
.postclose = radeon_driver_postclose_kms,
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -77,6 +78,11 @@ void radeon_driver_unload_kms(struct drm
radeon_modeset_fini(rdev);
radeon_device_fini(rdev);
 
+   if (dev->agp)
+   arch_phys_wc_del(dev->agp->agp_mtrr);
+   kfree(dev->agp);
+   dev->agp = NULL;
+
 done_free:
kfree(rdev);
dev->dev_private = NULL;


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

Re: [RFC PATCH 2/4] drm/scheduler: implement a function to modify sched list

[Luben Tuikov]

On 2020-03-03 14:06, Christian König wrote:
> Am 02.03.20 um 21:47 schrieb Luben Tuikov:
>> On 2020-02-28 2:47 a.m., Christian König wrote:
>>> Am 28.02.20 um 06:08 schrieb Luben Tuikov:
 On 2020-02-27 4:40 p.m., Nirmoy Das wrote:
> [SNIP]
> + if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
> + return -EINVAL;
 This seems unmaintainable. I'd write it in its natural form:
>>> This is probably just copy & pasted from the init function and complete
>>> overkill here.
>> It should be an easy rejection then. Statements like this make
>> the code unmaintainable. Regardless of whether it was copy-and-pasted
>> I wanted to emphasize the lack of simplification of what
>> was being done.
> 
> The problem is even deeper. As you noticed as well this is checking for 
> in kernel coding error and not application errors.

We shouldn't check the in-kernel implementation itself. If the kernel
did this everywhere, it'd be twice the size. The kernel shouldn't be
second-guessing itself.

Another way to see this is to ask "what would the kernel do here
if XYZ was NULL", for instance?  For userspace, it's clear. For
the kernel, it shows inconsistent and incomplete implmentation--something
which should be fixed.

Another way of seeing this is, if you break out a function into separate
smaller functions, checking the input parameters in those leaf/helper
functions (code which had been part of the original big function), would
make the code larger and second-guessing itself.

That check shouldn't be there. The implementation should be consistent
and complete in order to show an elegant code.

> 
> That check shouldn't have been in the init function in the first place, 
> but nobody had time to look into that so far.
> 
>> We need to put intention and sense into what we're doing, scrutinizing
>> every line we put into a patch. This is why I suggested
>> the simplification here:
>>
 int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
  struct drm_gpu_scheduler **sched_list,
  unsigned int num_sched_list)
 {
if (entity && sched_list && (num_sched_list == 0 || sched_list[0] != 
 NULL)) {
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
entity->num_sched_list = num_sched_list;
return 0;
} else {
return -EINVAL;
}
 }
>>> Actually that's a rather bad idea. Error handling should always be in
>> I actually don't think that it is a "rather bad idea". At all.
>> I actually think that it makes this leaf function more clear to
>> understand as the conditional would read like a sentence in prose.
> 
> The condition is indeed easier to read, but for the sacrifice of earlier 
> return and keeping prerequisite checking out of the code.

You know the compiler can reorder such code and invert
this simple-for-the-compiler conditional. It is easier
for human readers who might need to maintain it.

For instance, when you asked "Do you guys remember why we
might not have a job here?" for amdgpu_ib_schedule()
in a recent email--if that code had been split into code for 
test purposes and real IB processing code, then that question
wouldn't even be on the table.

We need to achieve a balance of breaking out code and if-else
statements. At the moment the code show everything bunched
up into a single function and a ton of if-else statements,
with the pretense "to avoid duplication". Such duplication can be
avoided architecturally by redefining structures, what's in them,
and what arguments functions take.

> 
>> [SNIP]
>>> What we should do instead is just: WARN_ON(!num_sched_list || !sched_list);
>> Again, what does that *mean*? What does the check mean and what
>> does the num_sched_list == 0 or sched_list == NULL mean?
>> And how did we get into a situation like this where either or both
>> could be nil?
> 
> It's an in kernel coding error to do this. The caller should at least 
> always provide a list with some entries in it.
> 
> A WARN_ON() is appropriate since it helps to narrows down the incorrect 
> behavior following from that.
> 
>> Wouldn't it be better to simplify or re-architecture this (we only recently
>> decided to hide physical rings from user-space), so that the code
>> is elegant (meaning no if-else) yet flexible and straightforward?
> 
> That was not recently at all, hiding physical rings was done nearly 5 
> years ago shortly after the driver was initially released.
> 
 Why not fix the architecture so that this is simply copied?
>>> We had that and moved away from it because the scheduler list is
>>> actually const and shouldn't be allocated with each entity (which we can
>>> easily have thousands of).
>> I think that peppering the code with if-else conditionals
>> everywhere as these patch-series into the DRM scheduler have been,
>> would make the code unmaintainable in the long run.
> 
> 

Re: [RFC PATCH 2/4] drm/scheduler: implement a function to modify sched list

[Christian König]

Am 02.03.20 um 21:47 schrieb Luben Tuikov:

On 2020-02-28 2:47 a.m., Christian König wrote:

Am 28.02.20 um 06:08 schrieb Luben Tuikov:

On 2020-02-27 4:40 p.m., Nirmoy Das wrote:

[SNIP]
+   if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
+   return -EINVAL;

This seems unmaintainable. I'd write it in its natural form:

This is probably just copy & pasted from the init function and complete
overkill here.

It should be an easy rejection then. Statements like this make
the code unmaintainable. Regardless of whether it was copy-and-pasted
I wanted to emphasize the lack of simplification of what
was being done.


The problem is even deeper. As you noticed as well this is checking for 
in kernel coding error and not application errors.


That check shouldn't have been in the init function in the first place, 
but nobody had time to look into that so far.



We need to put intention and sense into what we're doing, scrutinizing
every line we put into a patch. This is why I suggested
the simplification here:


int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
  struct drm_gpu_scheduler **sched_list,
  unsigned int num_sched_list)
{
if (entity && sched_list && (num_sched_list == 0 || sched_list[0] != 
NULL)) {
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
entity->num_sched_list = num_sched_list;
return 0;
} else {
return -EINVAL;
}
}

Actually that's a rather bad idea. Error handling should always be in

I actually don't think that it is a "rather bad idea". At all.
I actually think that it makes this leaf function more clear to
understand as the conditional would read like a sentence in prose.


The condition is indeed easier to read, but for the sacrifice of earlier 
return and keeping prerequisite checking out of the code.



[SNIP]

What we should do instead is just: WARN_ON(!num_sched_list || !sched_list);

Again, what does that *mean*? What does the check mean and what
does the num_sched_list == 0 or sched_list == NULL mean?
And how did we get into a situation like this where either or both
could be nil?


It's an in kernel coding error to do this. The caller should at least 
always provide a list with some entries in it.


A WARN_ON() is appropriate since it helps to narrows down the incorrect 
behavior following from that.



Wouldn't it be better to simplify or re-architecture this (we only recently
decided to hide physical rings from user-space), so that the code
is elegant (meaning no if-else) yet flexible and straightforward?


That was not recently at all, hiding physical rings was done nearly 5 
years ago shortly after the driver was initially released.



Why not fix the architecture so that this is simply copied?

We had that and moved away from it because the scheduler list is
actually const and shouldn't be allocated with each entity (which we can
easily have thousands of).

I think that peppering the code with if-else conditionals
everywhere as these patch-series into the DRM scheduler have been,
would make the code unmaintainable in the long run.


That's something I can agree on. Using a switch to map the priority to 
the backend implementation seems like the best idea to me.


E.g. function amdgpu_to_sched_priority() should not only map the IOCTL 
values to the scheduler values, but also return the array which hw rings 
to use.


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

Re: [PATCH 1/4] drm/amdgpu/vcn: fix race condition issue for vcn start

[James Zhu]

On 2020-03-03 1:44 p.m., Christian König wrote:

Am 03.03.20 um 19:16 schrieb James Zhu:

Fix race condition issue when multiple vcn starts are called.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 +
  2 files changed, 5 insertions(+)

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

index f96464e..aa7663f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -63,6 +63,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
  int i, r;
    INIT_DELAYED_WORK(>vcn.idle_work, 
amdgpu_vcn_idle_work_handler);

+    mutex_init(>vcn.vcn_pg_lock);
    switch (adev->asic_type) {
  case CHIP_RAVEN:
@@ -210,6 +211,7 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
  }
    release_firmware(adev->vcn.fw);
+    mutex_destroy(>vcn.vcn_pg_lock);
    return 0;
  }
@@ -321,6 +323,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring 
*ring)

  struct amdgpu_device *adev = ring->adev;
  bool set_clocks = !cancel_delayed_work_sync(>vcn.idle_work);
  +    mutex_lock(>vcn.vcn_pg_lock);


That still won't work correctly here.

The whole idea of the cancel_delayed_work_sync() and 
schedule_delayed_work() dance is that you have exactly one user of 
that. If you have multiple rings that whole thing won't work correctly.


To fix this you need to call mutex_lock() before 
cancel_delayed_work_sync() and schedule_delayed_work() before 
mutex_unlock().


Big lock definitely works. I am trying to use as smaller lock as 
possible here. the share resource which needs protect here are power 
gate process and dpg mode switch process.


if we move mutex_unlock() before schedule_delayed_work(. I am wondering 
what are the other necessary resources which need protect.


Thanks!

James



Regards,
Christian.


  if (set_clocks) {
  amdgpu_gfx_off_ctrl(adev, false);
  amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCN,
@@ -345,6 +348,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring 
*ring)

    adev->vcn.pause_dpg_mode(adev, ring->me, _state);
  }
+    mutex_unlock(>vcn.vcn_pg_lock);
  }
    void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h

index 6fe0573..2ae110d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -200,6 +200,7 @@ struct amdgpu_vcn {
  struct drm_gpu_scheduler *vcn_dec_sched[AMDGPU_MAX_VCN_INSTANCES];
  uint32_t num_vcn_enc_sched;
  uint32_t num_vcn_dec_sched;
+    struct mutex vcn_pg_lock;
    unsigned    harvest_config;
  int (*pause_dpg_mode)(struct amdgpu_device *adev,



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

Re: [PATCH] drm/amd/display: dcn20: remove an unused function

[Rodrigo Siqueira]

Hi,

Thanks for your patch, everything lgtm.

Reviewed-by: Rodrigo Siqueira 

On 03/02, Melissa Wen wrote:
> The dpp2_get_optimal_number_of_taps function is never used. Removing just for
> code cleaning up.
> 
> Signed-off-by: Melissa Wen 
> ---
>  .../gpu/drm/amd/display/dc/dcn20/dcn20_dpp.c  | 78 ---
>  1 file changed, 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_dpp.c 
> b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_dpp.c
> index 13e057d7ee93..42bba7c9548b 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_dpp.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_dpp.c
> @@ -369,84 +369,6 @@ void dpp2_set_cursor_attributes(
>   }
>  }
>  
> -#define IDENTITY_RATIO(ratio) (dc_fixpt_u3d19(ratio) == (1 << 19))
> -
> -bool dpp2_get_optimal_number_of_taps(
> - struct dpp *dpp,
> - struct scaler_data *scl_data,
> - const struct scaling_taps *in_taps)
> -{
> - /* Some ASICs does not support  FP16 scaling, so we reject modes 
> require this*/
> - if (scl_data->viewport.width  != scl_data->h_active &&
> - scl_data->viewport.height != scl_data->v_active &&
> - dpp->caps->dscl_data_proc_format == 
> DSCL_DATA_PRCESSING_FIXED_FORMAT &&
> - scl_data->format == PIXEL_FORMAT_FP16)
> - return false;
> -
> - if (scl_data->viewport.width > scl_data->h_active &&
> - dpp->ctx->dc->debug.max_downscale_src_width != 0 &&
> - scl_data->viewport.width > 
> dpp->ctx->dc->debug.max_downscale_src_width)
> - return false;
> -
> - /* TODO: add lb check */
> -
> - /* No support for programming ratio of 8, drop to 7.9.. */
> - if (scl_data->ratios.horz.value == (8ll << 32))
> - scl_data->ratios.horz.value--;
> - if (scl_data->ratios.vert.value == (8ll << 32))
> - scl_data->ratios.vert.value--;
> - if (scl_data->ratios.horz_c.value == (8ll << 32))
> - scl_data->ratios.horz_c.value--;
> - if (scl_data->ratios.vert_c.value == (8ll << 32))
> - scl_data->ratios.vert_c.value--;
> -
> - /* Set default taps if none are provided */
> - if (in_taps->h_taps == 0) {
> - if (dc_fixpt_ceil(scl_data->ratios.horz) > 4)
> - scl_data->taps.h_taps = 8;
> - else
> - scl_data->taps.h_taps = 4;
> - } else
> - scl_data->taps.h_taps = in_taps->h_taps;
> - if (in_taps->v_taps == 0) {
> - if (dc_fixpt_ceil(scl_data->ratios.vert) > 4)
> - scl_data->taps.v_taps = 8;
> - else
> - scl_data->taps.v_taps = 4;
> - } else
> - scl_data->taps.v_taps = in_taps->v_taps;
> - if (in_taps->v_taps_c == 0) {
> - if (dc_fixpt_ceil(scl_data->ratios.vert_c) > 4)
> - scl_data->taps.v_taps_c = 4;
> - else
> - scl_data->taps.v_taps_c = 2;
> - } else
> - scl_data->taps.v_taps_c = in_taps->v_taps_c;
> - if (in_taps->h_taps_c == 0) {
> - if (dc_fixpt_ceil(scl_data->ratios.horz_c) > 4)
> - scl_data->taps.h_taps_c = 4;
> - else
> - scl_data->taps.h_taps_c = 2;
> - } else if ((in_taps->h_taps_c % 2) != 0 && in_taps->h_taps_c != 1)
> - /* Only 1 and even h_taps_c are supported by hw */
> - scl_data->taps.h_taps_c = in_taps->h_taps_c - 1;
> - else
> - scl_data->taps.h_taps_c = in_taps->h_taps_c;
> -
> - if (!dpp->ctx->dc->debug.always_scale) {
> - if (IDENTITY_RATIO(scl_data->ratios.horz))
> - scl_data->taps.h_taps = 1;
> - if (IDENTITY_RATIO(scl_data->ratios.vert))
> - scl_data->taps.v_taps = 1;
> - if (IDENTITY_RATIO(scl_data->ratios.horz_c))
> - scl_data->taps.h_taps_c = 1;
> - if (IDENTITY_RATIO(scl_data->ratios.vert_c))
> - scl_data->taps.v_taps_c = 1;
> - }
> -
> - return true;
> -}
> -
>  void oppn20_dummy_program_regamma_pwl(
>   struct dpp *dpp,
>   const struct pwl_params *params,
> -- 
> 2.25.1
> 

-- 
Rodrigo Siqueira
https://siqueira.tech


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

Re: [PATCH 1/4] drm/amdgpu/vcn: fix race condition issue for vcn start

[Christian König]

Am 03.03.20 um 19:16 schrieb James Zhu:

Fix race condition issue when multiple vcn starts are called.

Signed-off-by: James Zhu 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 
  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 +
  2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index f96464e..aa7663f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -63,6 +63,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
int i, r;
  
  	INIT_DELAYED_WORK(>vcn.idle_work, amdgpu_vcn_idle_work_handler);

+   mutex_init(>vcn.vcn_pg_lock);
  
  	switch (adev->asic_type) {

case CHIP_RAVEN:
@@ -210,6 +211,7 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
}
  
  	release_firmware(adev->vcn.fw);

+   mutex_destroy(>vcn.vcn_pg_lock);
  
  	return 0;

  }
@@ -321,6 +323,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
struct amdgpu_device *adev = ring->adev;
bool set_clocks = !cancel_delayed_work_sync(>vcn.idle_work);
  
+	mutex_lock(>vcn.vcn_pg_lock);


That still won't work correctly here.

The whole idea of the cancel_delayed_work_sync() and 
schedule_delayed_work() dance is that you have exactly one user of that. 
If you have multiple rings that whole thing won't work correctly.


To fix this you need to call mutex_lock() before 
cancel_delayed_work_sync() and schedule_delayed_work() before 
mutex_unlock().


Regards,
Christian.


if (set_clocks) {
amdgpu_gfx_off_ctrl(adev, false);
amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCN,
@@ -345,6 +348,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
  
  		adev->vcn.pause_dpg_mode(adev, ring->me, _state);

}
+   mutex_unlock(>vcn.vcn_pg_lock);
  }
  
  void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index 6fe0573..2ae110d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -200,6 +200,7 @@ struct amdgpu_vcn {
struct drm_gpu_scheduler *vcn_dec_sched[AMDGPU_MAX_VCN_INSTANCES];
uint32_t num_vcn_enc_sched;
uint32_t num_vcn_dec_sched;
+   struct mutex vcn_pg_lock;
  
  	unsigned	harvest_config;

int (*pause_dpg_mode)(struct amdgpu_device *adev,


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

[PATCH 1/4] drm/amdgpu/vcn: fix race condition issue for vcn start

[James Zhu]

Fix race condition issue when multiple vcn starts are called.

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index f96464e..aa7663f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -63,6 +63,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
int i, r;
 
INIT_DELAYED_WORK(>vcn.idle_work, amdgpu_vcn_idle_work_handler);
+   mutex_init(>vcn.vcn_pg_lock);
 
switch (adev->asic_type) {
case CHIP_RAVEN:
@@ -210,6 +211,7 @@ int amdgpu_vcn_sw_fini(struct amdgpu_device *adev)
}
 
release_firmware(adev->vcn.fw);
+   mutex_destroy(>vcn.vcn_pg_lock);
 
return 0;
 }
@@ -321,6 +323,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
struct amdgpu_device *adev = ring->adev;
bool set_clocks = !cancel_delayed_work_sync(>vcn.idle_work);
 
+   mutex_lock(>vcn.vcn_pg_lock);
if (set_clocks) {
amdgpu_gfx_off_ctrl(adev, false);
amdgpu_device_ip_set_powergating_state(adev, 
AMD_IP_BLOCK_TYPE_VCN,
@@ -345,6 +348,7 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 
adev->vcn.pause_dpg_mode(adev, ring->me, _state);
}
+   mutex_unlock(>vcn.vcn_pg_lock);
 }
 
 void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index 6fe0573..2ae110d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -200,6 +200,7 @@ struct amdgpu_vcn {
struct drm_gpu_scheduler *vcn_dec_sched[AMDGPU_MAX_VCN_INSTANCES];
uint32_t num_vcn_enc_sched;
uint32_t num_vcn_dec_sched;
+   struct mutex vcn_pg_lock;
 
unsignedharvest_config;
int (*pause_dpg_mode)(struct amdgpu_device *adev,
-- 
2.7.4

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

[PATCH 4/4] drm/amdgpu/vcn2.5: stall DPG when WPTR/RPTR reset

[James Zhu]

Add vcn dpg harware and firmware synchronization to fix race condition
issue among vcn driver, hardware and firmware

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c 
b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index 2d64ba1..189c816 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -1388,8 +1388,11 @@ static int vcn_v2_5_pause_dpg_mode(struct amdgpu_device 
*adev,
   UVD_DPG_PAUSE__NJ_PAUSE_DPG_ACK_MASK,
   
UVD_DPG_PAUSE__NJ_PAUSE_DPG_ACK_MASK, ret_code);
 
+   /* Stall DPG before WPTR/RPTR reset */
+   WREG32_P(SOC15_REG_OFFSET(UVD, inst_idx, 
mmUVD_POWER_STATUS), UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK, 
~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
/* Restore */
ring = >vcn.inst[inst_idx].ring_enc[0];
+   ring->wptr = 0;
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_BASE_LO, 
ring->gpu_addr);
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_BASE_HI, 
upper_32_bits(ring->gpu_addr));
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_SIZE, 
ring->ring_size / 4);
@@ -1397,6 +1400,7 @@ static int vcn_v2_5_pause_dpg_mode(struct amdgpu_device 
*adev,
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_WPTR, 
lower_32_bits(ring->wptr));
 
ring = >vcn.inst[inst_idx].ring_enc[1];
+   ring->wptr = 0;
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_BASE_LO2, 
ring->gpu_addr);
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_BASE_HI2, 
upper_32_bits(ring->gpu_addr));
WREG32_SOC15(UVD, inst_idx, mmUVD_RB_SIZE2, 
ring->ring_size / 4);
@@ -1405,6 +1409,8 @@ static int vcn_v2_5_pause_dpg_mode(struct amdgpu_device 
*adev,
 
WREG32_SOC15(UVD, inst_idx, mmUVD_RBC_RB_WPTR,
   RREG32_SOC15(UVD, inst_idx, 
mmUVD_SCRATCH2) & 0x7FFF);
+   /* Unstall DPG */
+   WREG32_P(SOC15_REG_OFFSET(UVD, inst_idx, 
mmUVD_POWER_STATUS), 0, ~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
 
SOC15_WAIT_ON_RREG(UVD, inst_idx, 
mmUVD_POWER_STATUS,
   UVD_PGFSM_CONFIG__UVDM_UVDU_PWR_ON, 
UVD_POWER_STATUS__UVD_POWER_STATUS_MASK, ret_code);
-- 
2.7.4

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

[PATCH 2/4] drm/amdgpu/vcn: fix race condition issue for dpg unpause mode switch

[James Zhu]

Couldn't only rely on enc fence to decide switching to dpg unpaude mode.
Since a enc thread may not schedule a fence in time during multiple
threads running situation.

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 28 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |  1 +
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
index aa7663f..74cefc7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
@@ -64,6 +64,8 @@ int amdgpu_vcn_sw_init(struct amdgpu_device *adev)
 
INIT_DELAYED_WORK(>vcn.idle_work, amdgpu_vcn_idle_work_handler);
mutex_init(>vcn.vcn_pg_lock);
+   for (i = 0; i < adev->vcn.num_vcn_inst; i++)
+   atomic_set(>vcn.inst[i].enc_submission_cnt, 0);
 
switch (adev->asic_type) {
case CHIP_RAVEN:
@@ -332,19 +334,22 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 
if (adev->pg_flags & AMD_PG_SUPPORT_VCN_DPG){
struct dpg_pause_state new_state;
-   unsigned int fences = 0;
-   unsigned int i;
 
-   for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
-   fences += 
amdgpu_fence_count_emitted(>vcn.inst[ring->me].ring_enc[i]);
-   }
-   if (fences)
+   if (ring->funcs->type == AMDGPU_RING_TYPE_VCN_ENC) {
+   
atomic_inc(>vcn.inst[ring->me].enc_submission_cnt);
new_state.fw_based = VCN_DPG_STATE__PAUSE;
-   else
-   new_state.fw_based = VCN_DPG_STATE__UNPAUSE;
+   } else {
+   unsigned int fences = 0;
+   unsigned int i;
 
-   if (ring->funcs->type == AMDGPU_RING_TYPE_VCN_ENC)
-   new_state.fw_based = VCN_DPG_STATE__PAUSE;
+   for (i = 0; i < adev->vcn.num_enc_rings; ++i)
+   fences += 
amdgpu_fence_count_emitted(>vcn.inst[ring->me].ring_enc[i]);
+
+   if (fences || 
atomic_read(>vcn.inst[ring->me].enc_submission_cnt))
+   new_state.fw_based = VCN_DPG_STATE__PAUSE;
+   else
+   new_state.fw_based = VCN_DPG_STATE__UNPAUSE;
+   }
 
adev->vcn.pause_dpg_mode(adev, ring->me, _state);
}
@@ -354,6 +359,9 @@ void amdgpu_vcn_ring_begin_use(struct amdgpu_ring *ring)
 void amdgpu_vcn_ring_end_use(struct amdgpu_ring *ring)
 {
schedule_delayed_work(>adev->vcn.idle_work, VCN_IDLE_TIMEOUT);
+   if (ring->funcs->type == AMDGPU_RING_TYPE_VCN_ENC &&
+   
atomic_dec_return(>adev->vcn.inst[ring->me].enc_submission_cnt) < 0)
+   atomic_set(>adev->vcn.inst[ring->me].enc_submission_cnt, 
0);
 }
 
 int amdgpu_vcn_dec_ring_test_ring(struct amdgpu_ring *ring)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
index 2ae110d..4ca76c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
@@ -183,6 +183,7 @@ struct amdgpu_vcn_inst {
void*dpg_sram_cpu_addr;
uint64_tdpg_sram_gpu_addr;
uint32_t*dpg_sram_curr_addr;
+   atomic_tenc_submission_cnt;
 };
 
 struct amdgpu_vcn {
-- 
2.7.4

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

[PATCH 3/4] drm/amdgpu/vcn2.0: stall DPG when WPTR/RPTR reset

[James Zhu]

Add vcn dpg harware and firmware synchronization to fix race condition
issue among vcn driver, hardware and firmware

Signed-off-by: James Zhu 
---
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c 
b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index c387c81..7719f56 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -1158,8 +1158,12 @@ static int vcn_v2_0_pause_dpg_mode(struct amdgpu_device 
*adev,
   UVD_DPG_PAUSE__NJ_PAUSE_DPG_ACK_MASK,
   
UVD_DPG_PAUSE__NJ_PAUSE_DPG_ACK_MASK, ret_code);
 
+
+   /* Stall DPG before WPTR/RPTR reset */
+   WREG32_P(SOC15_REG_OFFSET(UVD, 0, 
mmUVD_POWER_STATUS), UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK, 
~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
/* Restore */
ring = >vcn.inst->ring_enc[0];
+   ring->wptr = 0;
WREG32_SOC15(UVD, 0, mmUVD_RB_BASE_LO, 
ring->gpu_addr);
WREG32_SOC15(UVD, 0, mmUVD_RB_BASE_HI, 
upper_32_bits(ring->gpu_addr));
WREG32_SOC15(UVD, 0, mmUVD_RB_SIZE, 
ring->ring_size / 4);
@@ -1167,6 +1171,7 @@ static int vcn_v2_0_pause_dpg_mode(struct amdgpu_device 
*adev,
WREG32_SOC15(UVD, 0, mmUVD_RB_WPTR, 
lower_32_bits(ring->wptr));
 
ring = >vcn.inst->ring_enc[1];
+   ring->wptr = 0;
WREG32_SOC15(UVD, 0, mmUVD_RB_BASE_LO2, 
ring->gpu_addr);
WREG32_SOC15(UVD, 0, mmUVD_RB_BASE_HI2, 
upper_32_bits(ring->gpu_addr));
WREG32_SOC15(UVD, 0, mmUVD_RB_SIZE2, 
ring->ring_size / 4);
@@ -1175,6 +1180,8 @@ static int vcn_v2_0_pause_dpg_mode(struct amdgpu_device 
*adev,
 
WREG32_SOC15(UVD, 0, mmUVD_RBC_RB_WPTR,
   RREG32_SOC15(UVD, 0, mmUVD_SCRATCH2) 
& 0x7FFF);
+   /* Unstall DPG */
+   WREG32_P(SOC15_REG_OFFSET(UVD, 0, 
mmUVD_POWER_STATUS), 0, ~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
 
SOC15_WAIT_ON_RREG(UVD, 0, mmUVD_POWER_STATUS,
   UVD_PGFSM_CONFIG__UVDM_UVDU_PWR_ON,
-- 
2.7.4

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

Re: [PATCH 2/3] drm/amdgpu: Add USBC PD FW load to PSP 11

[Andrey Grodzovsky]

On 3/2/20 4:30 PM, Alex Deucher wrote:

On Mon, Mar 2, 2020 at 2:24 PM Andrey Grodzovsky
 wrote:

Add the programming sequence.

Signed-off-by: Andrey Grodzovsky 
---
  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 76 ++
  1 file changed, 76 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c 
b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
index 8ab3bf3..621b99c 100644
--- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
@@ -65,6 +65,9 @@ MODULE_FIRMWARE("amdgpu/arcturus_ta.bin");
  /* memory training timeout define */
  #define MEM_TRAIN_SEND_MSG_TIMEOUT_US  300

+/* USBC PD FW version retrieval command */
+#define C2PMSG_CMD_GFX_USB_PD_FW_VER 0x200
+

This probably belongs in psp_gfx_if.h.


  static int psp_v11_0_init_microcode(struct psp_context *psp)
  {
 struct amdgpu_device *adev = psp->adev;
@@ -1109,6 +1112,77 @@ static void psp_v11_0_ring_set_wptr(struct psp_context 
*psp, uint32_t value)
 WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_67, value);
  }

+static int psp_v11_0_load_usbc_pd_fw(struct psp_context *psp, dma_addr_t 
dma_addr)
+{
+   struct amdgpu_device *adev = psp->adev;
+   uint32_t reg_status;
+   int ret;
+
+   /* Write lower 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, lower_32_bits(dma_addr));
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+// Fireup interrupt so PSP can pick up the lower address

C style comments please.


+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x80);
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Lower address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] 
= %02x...\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   /* Write upper 32-bit address of the PD Controller FW */
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36, upper_32_bits(dma_addr));
+
+
+   ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false);
+   if (ret)
+   return ret;
+
+// Fireup interrupt so PSP can pick up the upper address

C style comments.


+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, 0x400);
+
+   /* FW load takes long time */
+   while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false))
+   msleep(1000);
+

Are we actually waiting for the full loading here or just the ack of
the command?


+   reg_status = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35);
+
+   if ((reg_status & 0x) != 0) {
+   DRM_ERROR("Upper address load failed - MP0_SMN_C2PMSG_35.Bits [15:0] 
= %02x...\n",
+   reg_status & 0x);
+   return -EIO;
+   }
+
+   return 0;
+}
+
+static int psp_v11_0_read_usbc_pd_fw(struct psp_context *psp, uint32_t *fw_ver)
+{
+   struct amdgpu_device *adev = psp->adev;
+
+   WREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_35, C2PMSG_CMD_GFX_USB_PD_FW_VER);
+
+   while(psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_35),
+0x8000, 0x8000, false))
+   msleep(1);
+
+   *fw_ver = RREG32_SOC15(MP0, 0, mmMP0_SMN_C2PMSG_36);
+
+   return 0;
+}

I think we need locking for mmMP0_SMN_C2PMSG_35/mmMP0_SMN_C2PMSG_36
since they are the mailbox registers for communicating with the PSP.
Maybe just a generic psp lock since I don't think we need fine grained
locking for psp.

Alex



I wonder why we don't lock in any other place we access them (e.g. 
psp_v11_0_memory_training_send_msg or psp_v11_0_bootloader_load_sos) ?


Andrey




+
  static const struct psp_funcs psp_v11_0_funcs = {
 .init_microcode = psp_v11_0_init_microcode,
 .bootloader_load_kdb = psp_v11_0_bootloader_load_kdb,
@@ -1133,6 +1207,8 @@ static const struct psp_funcs psp_v11_0_funcs = {
 .mem_training = psp_v11_0_memory_training,
 .ring_get_wptr = psp_v11_0_ring_get_wptr,
 .ring_set_wptr = psp_v11_0_ring_set_wptr,
+   .load_usbc_pd_fw = psp_v11_0_load_usbc_pd_fw,
+   .read_usbc_pd_fw = psp_v11_0_read_usbc_pd_fw
  };

  void psp_v11_0_set_psp_funcs(struct psp_context *psp)
--
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

[AMD Official Use Only - Internal Distribution Only]

Thanks million! I’ll change the patch.

-Jacob


From: Koenig, Christian 
Sent: Wednesday, March 4, 2020 12:09:36 AM
To: He, Jacob ; amd-gfx@lists.freedesktop.org 

Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 03.03.20 um 17:07 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]


Oh, you are right! If SPM_VMID is updated by other process while the SPM 
enabled commands is executing, that will cause VM fault.



Is the wait vm idle right before unreserve vmid still necessary if using 
asynchroneously setting SPM_VMID?

No, that are alternative approaches.

Updating the VMID asynchronously sounds a bit cleaner to me, but feel free to 
pick whatever is easier for you to implement.

Regards,
Christian.




Thanks

Jacob



From: Koenig, Christian
Sent: Tuesday, March 3, 2020 11:36 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid



See the SPM buffer address is set using CP commands as well, right? And those 
execute asynchronously.

When we now synchronously update the SPM VMID we risk that we switch from one 
process to another while the new process is not ready yet with its setup.

That could have quite a bunch of unforeseen consequences, including 
accidentally writing SPM data into the new process address space at whatever 
buffer address was used before.

This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]



Thanks!  Could you please take an example of trouble  “This way we avoid a 
bunch of trouble when one process drops the VMID reservation and another one 
grabs it.”?



Thanks

Jacob



From: Koenig, Christian
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid



Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]



It would be better if we could do that asynchronously with a register
write on the ring.

Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the 
write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.

Regards,
Christian.






Thanks

Jacob



From: Christian König
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid



Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
> amdgpu_job *job,
>struct dma_fence *fence = NULL;
>bool pasid_mapping_needed = false;
>unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm && 
> (job->vm->reserved_vmid[vmhub] != NULL));
>int r;
>
> + if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>gds_switch_needed = true;
>vm_flush_needed = true;











___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org

Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 03.03.20 um 17:07 schrieb He, Jacob:


[AMD Official Use Only - Internal Distribution Only]


Oh, you are right! If SPM_VMID is updated by other process while the 
SPM enabled commands is executing, that will cause VM fault.


Is the wait vm idle right before unreserve vmid still necessary if 
using asynchroneously setting SPM_VMID?




No, that are alternative approaches.

Updating the VMID asynchronously sounds a bit cleaner to me, but feel 
free to pick whatever is easier for you to implement.


Regards,
Christian.


Thanks

Jacob

*From: *Koenig, Christian 
*Sent: *Tuesday, March 3, 2020 11:36 PM
*To: *He, Jacob ; 
amd-gfx@lists.freedesktop.org 
*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
when application reserves the vmid


See the SPM buffer address is set using CP commands as well, right? 
And those execute asynchronously.


When we now synchronously update the SPM VMID we risk that we switch 
from one process to another while the new process is not ready yet 
with its setup.


That could have quite a bunch of unforeseen consequences, including 
accidentally writing SPM data into the new process address space at 
whatever buffer address was used before.


This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

Thanks!  Could you please take an example of trouble  “This way we
avoid a bunch of trouble when one process drops the VMID
reservation and another one grabs it.”?

Thanks

Jacob

*From: *Koenig, Christian 
*Sent: *Tuesday, March 3, 2020 11:03 PM
*To: *He, Jacob ;
amd-gfx@lists.freedesktop.org 
*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's
vmid when application reserves the vmid

Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

/It would be better if we could do that asynchronously with a
register
write on the ring./

Sorry, I don’t get your point. Could you please elaborate more?


You pass the ring from amdgpu_vm_flush() to the
*_update_spm_vmid() functions.

And then instead of using WREG32() you call
amdgpu_ring_emit_wreg() to make the write asynchronously on the
ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the
VMID reservation and another one grabs it.

Regards,
Christian.



Thanks

Jacob

*From: *Christian König 
*Sent: *Tuesday, March 3, 2020 10:16 PM
*To: *He, Jacob ;
amd-gfx@lists.freedesktop.org

*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the
job's vmid when application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should
be updated
> with the job's vmid right before the job is scheduled.
SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct
amdgpu_ring *ring, struct amdgpu_job *job,
>    struct dma_fence *fence = NULL;
>    bool pasid_mapping_needed = false;
>    unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm &&
(job->vm->reserved_vmid[vmhub] != NULL));
>    int r;
>
> + if (update_spm_vmid_needed &&
adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a
register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and
call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>    if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>    

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

[AMD Official Use Only - Internal Distribution Only]

Oh, you are right! If SPM_VMID is updated by other process while the SPM 
enabled commands is executing, that will cause VM fault.

Is the wait vm idle right before unreserve vmid still necessary if using 
asynchroneously setting SPM_VMID?

Thanks
Jacob

From: Koenig, Christian
Sent: Tuesday, March 3, 2020 11:36 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

See the SPM buffer address is set using CP commands as well, right? And those 
execute asynchronously.

When we now synchronously update the SPM VMID we risk that we switch from one 
process to another while the new process is not ready yet with its setup.

That could have quite a bunch of unforeseen consequences, including 
accidentally writing SPM data into the new process address space at whatever 
buffer address was used before.

This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

Thanks!  Could you please take an example of trouble  “This way we avoid a 
bunch of trouble when one process drops the VMID reservation and another one 
grabs it.”?

Thanks
Jacob

From: Koenig, Christian
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.
Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the 
write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.

Regards,
Christian.




Thanks
Jacob

From: Christian König
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
> amdgpu_job *job,
>struct dma_fence *fence = NULL;
>bool pasid_mapping_needed = false;
>unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm && 
> (job->vm->reserved_vmid[vmhub] != NULL));
>int r;
>
> + if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>gds_switch_needed = true;
>vm_flush_needed = true;





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

Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

See the SPM buffer address is set using CP commands as well, right? And 
those execute asynchronously.


When we now synchronously update the SPM VMID we risk that we switch 
from one process to another while the new process is not ready yet with 
its setup.


That could have quite a bunch of unforeseen consequences, including 
accidentally writing SPM data into the new process address space at 
whatever buffer address was used before.


This is something we at least should try to avoid.

Regards,
Christian.

Am 03.03.20 um 16:28 schrieb He, Jacob:


[AMD Official Use Only - Internal Distribution Only]


Thanks!  Could you please take an example of trouble  “This way we 
avoid a bunch of trouble when one process drops the VMID reservation 
and another one grabs it.”?


Thanks

Jacob

*From: *Koenig, Christian 
*Sent: *Tuesday, March 3, 2020 11:03 PM
*To: *He, Jacob ; 
amd-gfx@lists.freedesktop.org 
*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
when application reserves the vmid


Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

/It would be better if we could do that asynchronously with a
register
write on the ring./

Sorry, I don’t get your point. Could you please elaborate more?


You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() 
functions.


And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to 
make the write asynchronously on the ring buffer using a CP command.


This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.


Regards,
Christian.


Thanks

Jacob

*From: *Christian König 
*Sent: *Tuesday, March 3, 2020 10:16 PM
*To: *He, Jacob ;
amd-gfx@lists.freedesktop.org 
*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's
vmid when application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be
updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He  
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring
*ring, struct amdgpu_job *job,
>    struct dma_fence *fence = NULL;
>    bool pasid_mapping_needed = false;
>    unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm &&
(job->vm->reserved_vmid[vmhub] != NULL));
>    int r;
>
> + if (update_spm_vmid_needed &&
adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>    if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>    gds_switch_needed = true;
>    vm_flush_needed = true;



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

Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Christian König]

Am 03.03.20 um 16:22 schrieb Nirmoy:

Hi Christian,

On 3/3/20 4:14 PM, Christian König wrote:


I mean the drm_gpu_scheduler * array doesn't needs to be 
constructed by the context code in the first place.
Do you mean amdgpu_ctx_init_sched() should belong to somewhere else 
may be in amdgpu_ring.c ?


That's one possibility, yes. On the other hand feel free to go ahead 
with the boolean for now. \


Are you fine with struct drm_gpu_scheduler 
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX]; for now as well ?


Yeah, that should work anyway. My only concern was adding the boolean to 
the ring structure.


regards,
Christian.




Regards,

Nirmoy



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

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

[AMD Official Use Only - Internal Distribution Only]

Thanks!  Could you please take an example of trouble  “This way we avoid a 
bunch of trouble when one process drops the VMID reservation and another one 
grabs it.”?

Thanks
Jacob

From: Koenig, Christian
Sent: Tuesday, March 3, 2020 11:03 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 03.03.20 um 15:34 schrieb He, Jacob:

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.
Sorry, I don’t get your point. Could you please elaborate more?

You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() functions.

And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to make the 
write asynchronously on the ring buffer using a CP command.

This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.

Regards,
Christian.



Thanks
Jacob

From: Christian König
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
> amdgpu_job *job,
>struct dma_fence *fence = NULL;
>bool pasid_mapping_needed = false;
>unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm && 
> (job->vm->reserved_vmid[vmhub] != NULL));
>int r;
>
> + if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>gds_switch_needed = true;
>vm_flush_needed = true;



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

Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Nirmoy]

Hi Christian,

On 3/3/20 4:14 PM, Christian König wrote:


I mean the drm_gpu_scheduler * array doesn't needs to be constructed 
by the context code in the first place.
Do you mean amdgpu_ctx_init_sched() should belong to somewhere else 
may be in amdgpu_ring.c ?


That's one possibility, yes. On the other hand feel free to go ahead 
with the boolean for now. \


Are you fine with struct drm_gpu_scheduler 
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX]; for now as well ?



Regards,

Nirmoy

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

Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Christian König]

Am 03.03.20 um 15:29 schrieb Nirmoy:


On 3/3/20 3:03 PM, Christian König wrote:

Am 03.03.20 um 13:50 schrieb Nirmoy Das:

[SNIP]
  struct amdgpu_mec {
  struct amdgpu_bo    *hpd_eop_obj;
  u64    hpd_eop_gpu_addr;
@@ -280,8 +290,9 @@ struct amdgpu_gfx {
  uint32_t    num_gfx_sched;
  unsigned    num_gfx_rings;
  struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+    struct drm_gpu_scheduler 
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
  struct drm_gpu_scheduler 
*compute_sched[AMDGPU_MAX_COMPUTE_RINGS];

-    uint32_t    num_compute_sched;
+    uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
  unsigned    num_compute_rings;
  struct amdgpu_irq_src    eop_irq;
  struct amdgpu_irq_src    priv_reg_irq;


Well my question is why we we need compute_prio_sched here?


This one is so that I can leverage single sched array 
compute_sched[AMDGPU_MAX_COMPUTE_RINGS]


to store both priority  sched list  instead of

struct drm_gpu_scheduler 
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_COMPUTE_RINGS];


I guess this make ctx code unnecessarily complex.


Well, it would be good if the ctx code didn't need to fill in 
compute_sched in the first place.


E.g. that the hardware backends provide to the ctx handling which 
schedulers to use for a specific use case.



Can't we just design that as:
struct drm_gpu_scheduler 
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];

uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];

What should be the value of AMDGPU_MAX_HI_COMPUTE_RINGS ?


I mean the drm_gpu_scheduler * array doesn't needs to be constructed 
by the context code in the first place.
Do you mean amdgpu_ctx_init_sched() should belong to somewhere else 
may be in amdgpu_ring.c ?


That's one possibility, yes. On the other hand feel free to go ahead 
with the boolean for now.


This seems to be a to complex and to wide cleanup that we should do it 
as part of this patch set.


Regards,
Christian.



Or am I missing something?

Regards,
Christian.

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


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

Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 03.03.20 um 15:34 schrieb He, Jacob:


[AMD Official Use Only - Internal Distribution Only]


/It would be better if we could do that asynchronously with a register
write on the ring.

/

Sorry, I don’t get your point. Could you please elaborate more?



You pass the ring from amdgpu_vm_flush() to the *_update_spm_vmid() 
functions.


And then instead of using WREG32() you call amdgpu_ring_emit_wreg() to 
make the write asynchronously on the ring buffer using a CP command.


This way we avoid a bunch of trouble when one process drops the VMID 
reservation and another one grabs it.


Regards,
Christian.


Thanks

Jacob

*From: *Christian König 
*Sent: *Tuesday, March 3, 2020 10:16 PM
*To: *He, Jacob ; 
amd-gfx@lists.freedesktop.org 
*Subject: *Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid 
when application reserves the vmid


Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, 
struct amdgpu_job *job,

>    struct dma_fence *fence = NULL;
>    bool pasid_mapping_needed = false;
>    unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm && 
(job->vm->reserved_vmid[vmhub] != NULL));

>    int r;
>
> + if (update_spm_vmid_needed && 
adev->gfx.rlc.funcs->update_spm_vmid)

> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>    if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>    gds_switch_needed = true;
>    vm_flush_needed = true;



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

Re: [PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Bjorn Helgaas]

Cosmetics:

s/ioremap/ioremap()/ (also in commit log)
s/phys_to_virt/phys_to_virt()/ (also in commit log)
s/pci_platform_rom/pci_platform_rom()/ (commit log)
s/rom/ROM/

On Mon, Mar 02, 2020 at 10:34:55PM -0500, Mikel Rychliski wrote:
> On some EFI systems, the video BIOS is provided by the EFI firmware.  The
> boot stub code stores the physical address of the ROM image in pdev->rom.
> Currently we attempt to access this pointer using phys_to_virt, which
> doesn't work with CONFIG_HIGHMEM.
> 
> On these systems, attempting to load the radeon module on a x86_32 kernel
> can result in the following:
> 
> BUG: unable to handle page fault for address: 3e8ed03c
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x) - not-present page
> *pde = 
> Oops:  [#1] PREEMPT SMP
> CPU: 0 PID: 317 Comm: systemd-udevd Not tainted 5.6.0-rc3-next-20200228 #2
> Hardware name: Apple Computer, Inc. MacPro1,1/Mac-F4208DC8, BIOS 
> MP11.88Z.005C.B08.0707021221 07/02/07
> EIP: radeon_get_bios+0x5ed/0xe50 [radeon]
> Code: 00 00 84 c0 0f 85 12 fd ff ff c7 87 64 01 00 00 00 00 00 00 8b 47 
> 08 8b 55 b0 e8 1e 83 e1 d6 85 c0 74 1a 8b 55 c0 85 d2 74 13 <80> 38 55 75 0e 
> 80 78 01 aa 0f 84 a4 03 00 00 8d 74 26 00 68 dc 06
> EAX: 3e8ed03c EBX:  ECX: 3e8ed03c EDX: 0001
> ESI: 0004 EDI: eec04000 EBP: eef3fc60 ESP: eef3fbe0
> DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010206
> CR0: 80050033 CR2: 3e8ed03c CR3: 2ec77000 CR4: 06d0
> Call Trace:
>  ? register_client+0x34/0xe0
>  ? register_client+0xab/0xe0
>  r520_init+0x26/0x240 [radeon]
>  radeon_device_init+0x533/0xa50 [radeon]
>  radeon_driver_load_kms+0x80/0x220 [radeon]
>  drm_dev_register+0xa7/0x180 [drm]
>  radeon_pci_probe+0x10f/0x1a0 [radeon]
>  pci_device_probe+0xd4/0x140
>  really_probe+0x13d/0x3b0
>  driver_probe_device+0x56/0xd0
>  device_driver_attach+0x49/0x50
>  __driver_attach+0x79/0x130
>  ? device_driver_attach+0x50/0x50
>  bus_for_each_dev+0x5b/0xa0
>  driver_attach+0x19/0x20
>  ? device_driver_attach+0x50/0x50
>  bus_add_driver+0x117/0x1d0
>  ? pci_bus_num_vf+0x20/0x20
>  driver_register+0x66/0xb0
>  ? 0xf80f4000
>  __pci_register_driver+0x3d/0x40
>  radeon_init+0x82/0x1000 [radeon]
>  do_one_initcall+0x42/0x200
>  ? kvfree+0x25/0x30
>  ? __vunmap+0x206/0x230
>  ? kmem_cache_alloc_trace+0x16f/0x220
>  ? do_init_module+0x21/0x220
>  do_init_module+0x50/0x220
>  load_module+0x1f26/0x2200
>  sys_init_module+0x12d/0x160
>  do_fast_syscall_32+0x82/0x250
>  entry_SYSENTER_32+0xa5/0xf8
> 
> Fix the issue by using ioremap instead of phys_to_virt in pci_platform_rom.
> 
> Signed-off-by: Mikel Rychliski 
> ---
>  drivers/pci/rom.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index 137bf0cee897..e352798eed0c 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -197,8 +197,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem 
> *rom)
>  EXPORT_SYMBOL(pci_unmap_rom);
>  
>  /**
> - * pci_platform_rom - provides a pointer to any ROM image provided by the
> - * platform
> + * pci_platform_rom - ioremap the ROM image provided by the platform
>   * @pdev: pointer to pci device struct
>   * @size: pointer to receive size of pci window over ROM
>   */
> @@ -206,7 +205,7 @@ void __iomem *pci_platform_rom(struct pci_dev *pdev, 
> size_t *size)
>  {
>   if (pdev->rom && pdev->romlen) {
>   *size = pdev->romlen;
> - return phys_to_virt((phys_addr_t)pdev->rom);
> + return ioremap(pdev->rom, pdev->romlen);

This changes the interface of pci_platform_rom() (the caller is now
responsible for doing an iounmap()).  That should be mentioned in the
function comment, and I think the subsequent patches should be
squashed into this one so the interface change and the caller changes
are done simultaneously.

Also, it looks like nvbios_platform.init() (platform_init()) needs a
similar change?

>   }
>  
>   return NULL;
> -- 
> 2.13.7
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[He, Jacob]

[AMD Official Use Only - Internal Distribution Only]

It would be better if we could do that asynchronously with a register
write on the ring.

Sorry, I don’t get your point. Could you please elaborate more?

Thanks
Jacob

From: Christian König
Sent: Tuesday, March 3, 2020 10:16 PM
To: He, Jacob; 
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when 
application reserves the vmid

Am 02.03.20 um 06:35 schrieb Jacob He:
> SPM access the video memory according to SPM_VMID. It should be updated
> with the job's vmid right before the job is scheduled. SPM_VMID is a
> global resource
>
> Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
> Signed-off-by: Jacob He 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c00696f3017e..c761d3a0b6e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
> amdgpu_job *job,
>struct dma_fence *fence = NULL;
>bool pasid_mapping_needed = false;
>unsigned patch_offset = 0;
> + bool update_spm_vmid_needed = (job->vm && 
> (job->vm->reserved_vmid[vmhub] != NULL));
>int r;
>
> + if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> +

It would be better if we could do that asynchronously with a register
write on the ring.

The alternative is that we block for the VM to be idle in
amdgpu_vm_ioctl() before unreserving the VMID.

In other words lock the reservation object of the root PD and call
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().

Regards,
Christian.

>if (amdgpu_vmid_had_gpu_reset(adev, id)) {
>gds_switch_needed = true;
>vm_flush_needed = true;

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

Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Nirmoy]

On 3/3/20 3:03 PM, Christian König wrote:

Am 03.03.20 um 13:50 schrieb Nirmoy Das:

[SNIP]
  struct amdgpu_mec {
  struct amdgpu_bo    *hpd_eop_obj;
  u64    hpd_eop_gpu_addr;
@@ -280,8 +290,9 @@ struct amdgpu_gfx {
  uint32_t    num_gfx_sched;
  unsigned    num_gfx_rings;
  struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+    struct drm_gpu_scheduler 
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];

  struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
-    uint32_t    num_compute_sched;
+    uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
  unsigned    num_compute_rings;
  struct amdgpu_irq_src    eop_irq;
  struct amdgpu_irq_src    priv_reg_irq;


Well my question is why we we need compute_prio_sched here?


This one is so that I can leverage single sched array 
compute_sched[AMDGPU_MAX_COMPUTE_RINGS]


to store both priority  sched list  instead of

struct drm_gpu_scheduler 
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_COMPUTE_RINGS];


I guess this make ctx code unnecessarily complex.



Can't we just design that as:
struct drm_gpu_scheduler 
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];

uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];

What should be the value of AMDGPU_MAX_HI_COMPUTE_RINGS ?


I mean the drm_gpu_scheduler * array doesn't needs to be constructed 
by the context code in the first place.
Do you mean amdgpu_ctx_init_sched() should belong to somewhere else may 
be in amdgpu_ring.c ?


Or am I missing something?

Regards,
Christian.

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

Re: [PATCH] drm/amdgpu: Update SPM_VMID with the job's vmid when application reserves the vmid

[Christian König]

Am 02.03.20 um 06:35 schrieb Jacob He:

SPM access the video memory according to SPM_VMID. It should be updated
with the job's vmid right before the job is scheduled. SPM_VMID is a
global resource

Change-Id: Id3881908960398f87e7c95026a54ff83ff826700
Signed-off-by: Jacob He 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index c00696f3017e..c761d3a0b6e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1080,8 +1080,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct 
amdgpu_job *job,
struct dma_fence *fence = NULL;
bool pasid_mapping_needed = false;
unsigned patch_offset = 0;
+   bool update_spm_vmid_needed = (job->vm && 
(job->vm->reserved_vmid[vmhub] != NULL));
int r;
  
+	if (update_spm_vmid_needed && adev->gfx.rlc.funcs->update_spm_vmid)

+   adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
+


It would be better if we could do that asynchronously with a register 
write on the ring.


The alternative is that we block for the VM to be idle in 
amdgpu_vm_ioctl() before unreserving the VMID.


In other words lock the reservation object of the root PD and call 
amdgpu_vm_wait_idle() before calling amdgpu_vmid_free_reserved().


Regards,
Christian.


if (amdgpu_vmid_had_gpu_reset(adev, id)) {
gds_switch_needed = true;
vm_flush_needed = true;


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

Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Christian König]

Am 03.03.20 um 13:50 schrieb Nirmoy Das:

[SNIP]
  struct amdgpu_mec {
struct amdgpu_bo*hpd_eop_obj;
u64 hpd_eop_gpu_addr;
@@ -280,8 +290,9 @@ struct amdgpu_gfx {
uint32_tnum_gfx_sched;
unsignednum_gfx_rings;
struct amdgpu_ring  compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+   struct drm_gpu_scheduler
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
struct drm_gpu_scheduler
*compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
-   uint32_tnum_compute_sched;
+   uint32_t
num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
unsignednum_compute_rings;
struct amdgpu_irq_src   eop_irq;
struct amdgpu_irq_src   priv_reg_irq;


Well my question is why we we need compute_prio_sched here?

Can't we just design that as:
struct drm_gpu_scheduler 
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];

uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];

I mean the drm_gpu_scheduler * array doesn't needs to be constructed by 
the context code in the first place.


Or am I missing something?

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

RE: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark clock settings to smu resume from s3

[Wu, Hersen]

[AMD Official Use Only - Internal Distribution Only]

Hi Evan, Kenneth,

Would you please help review this patch again?

Thanks!
Hersen


-Original Message-
From: Alex Deucher  
Sent: Monday, March 2, 2020 9:27 AM
To: Wu, Hersen 
Cc: Quan, Evan ; amd-gfx@lists.freedesktop.org; Feng, 
Kenneth 
Subject: Re: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark clock 
settings to smu resume from s3

On Fri, Feb 28, 2020 at 3:59 PM Wu, Hersen  wrote:
>
> Follow Evan's review, add smu->mutex.
>
>
> This interface is for dGPU Navi1x. Linux dc-pplib interface depends  
> on window driver dc implementation.
>
>  For Navi1x, clock settings of dcn watermarks are fixed. the settings  
> should be passed to smu during boot up and resume from s3.
>  boot up: dc calculate dcn watermark clock settings within dc_create,  
> dcn20_resource_construct, then call pplib functions below to pass  the 
> settings to smu:
>  smu_set_watermarks_for_clock_ranges
>  smu_set_watermarks_table
>  navi10_set_watermarks_table
>  smu_write_watermarks_table
>
>  For Renoir, clock settings of dcn watermark are also fixed values.
>  dc has implemented different flow for window driver:
>  dc_hardware_init / dc_set_power_state  dcn10_init_hw  
> notify_wm_ranges  set_wm_ranges
>
>  For Linux
>  smu_set_watermarks_for_clock_ranges
>  renoir_set_watermarks_table
>  smu_write_watermarks_table
>
>  dc_hardware_init -> amdgpu_dm_init
>  dc_set_power_state --> dm_resume
>
>  therefore, linux dc-pplib interface of navi10/12/14 is different  
> from that of Renoir.
>
> Signed-off-by: Hersen Wu 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68 
> +++
>  1 file changed, 68 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 931cbd7b372e..1ee1d6ff2782 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1435,6 +1435,72 @@ static void s3_handle_mst(struct drm_device *dev, bool 
> suspend)
>   drm_kms_helper_hotplug_event(dev);
>  }
>
> +static int amdgpu_dm_smu_write_watermarks_table(struct amdgpu_device 
> +*adev) {  struct smu_context *smu = >smu;  int ret = 0;
> +
> + if (!is_support_sw_smu(adev))
> + return 0;
> +
> + /* This interface is for dGPU Navi1x.Linux dc-pplib interface 
> + depends
> + * on window driver dc implementation.
> + * For Navi1x, clock settings of dcn watermarks are fixed. the 
> + settings
> + * should be passed to smu during boot up and resume from s3.
> + * boot up: dc calculate dcn watermark clock settings within 
> + dc_create,
> + * dcn20_resource_construct
> + * then call pplib functions below to pass the settings to smu:
> + * smu_set_watermarks_for_clock_ranges
> + * smu_set_watermarks_table
> + * navi10_set_watermarks_table
> + * smu_write_watermarks_table
> + *
> + * For Renoir, clock settings of dcn watermark are also fixed values.
> + * dc has implemented different flow for window driver:
> + * dc_hardware_init / dc_set_power_state
> + * dcn10_init_hw
> + * notify_wm_ranges
> + * set_wm_ranges
> + * -- Linux
> + * smu_set_watermarks_for_clock_ranges
> + * renoir_set_watermarks_table
> + * smu_write_watermarks_table
> + *
> + * For Linux,
> + * dc_hardware_init -> amdgpu_dm_init
> + * dc_set_power_state --> dm_resume
> + *
> + * therefore, this function apply to navi10/12/14 but not Renoir
> + * *
> + */
> + switch(adev->asic_type) {
> + case CHIP_NAVI10:
> + case CHIP_NAVI14:
> + case CHIP_NAVI12:
> + break;
> + default:
> + return 0;
> + }
> +
> + mutex_lock(>mutex);
> +
> + /* pass data to smu controller */
> + if ((smu->watermarks_bitmap & WATERMARKS_EXIST) && 
> + !(smu->watermarks_bitmap & WATERMARKS_LOADED)) { ret = 
> + smu_write_watermarks_table(smu);
> +
> + if (ret) {
> + DRM_ERROR("Failed to update WMTABLE!\n"); return ret;

You need to unlock the mutex here in the failure case.

Alex

> + }
> + smu->watermarks_bitmap |= WATERMARKS_LOADED;
> + }
> +
> + mutex_unlock(>mutex);
> +
> + return 0;
> +}
> +
>  /**
>   * dm_hw_init() - Initialize DC device
>   * @handle: The base driver device containing the amdgpu_dm device.
> @@ -1713,6 +1779,8 @@ static int dm_resume(void *handle)
>
>   amdgpu_dm_irq_resume_late(adev);
>
> + amdgpu_dm_smu_write_watermarks_table(adev);
> +
>   return 0;
>  }
>
> --
> 2.17.1
>
> 
> From: Quan, Evan 
> Sent: February 27, 2020 9:58 PM
> To: Wu, Hersen ; amd-gfx@lists.freedesktop.org 
> 
> Cc: Feng, Kenneth ; Wu, Hersen 
> 
> Subject: RE: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark 
> clock settings to smu resume from s3
>
> Thanks. But could you help to confirm whether this is correctly protected by 
> "mutex_lock(>mutex)"?
>
> -Original Message-
> From: Hersen Wu 
> Sent: Thursday, February 27, 2020 11:54 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Quan, Evan ; Feng, Kenneth 
> ; Wu, Hersen 
> Subject: [PATCH 2/2] 

[PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy Das]

Switch to appropriate sched list for an entity on priority override.

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 8c52152e3a6e..a0bf14ab9d33 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx 
*ctx,
return fence;
 }

+static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
+  struct amdgpu_ctx_entity *aentity,
+  int hw_ip, enum drm_sched_priority priority)
+{
+   struct amdgpu_device *adev = ctx->adev;
+   enum gfx_pipe_priority hw_prio;
+   struct drm_gpu_scheduler **scheds = NULL;
+   unsigned num_scheds;
+
+   /* set sw priority */
+   drm_sched_entity_set_priority(>entity, priority);
+
+   /* set hw priority */
+   switch (hw_ip) {
+   case AMDGPU_HW_IP_COMPUTE:
+   hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
+   scheds = adev->gfx.compute_prio_sched[hw_prio];
+   num_scheds = adev->gfx.num_compute_sched[hw_prio];
+   break;
+   default:
+   return;
+   }
+
+   drm_sched_entity_modify_sched(>entity, scheds, num_scheds);
+}
+
 void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
  enum drm_sched_priority priority)
 {
@@ -534,13 +560,11 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
ctx->init_priority : ctx->override_priority;
for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
-   struct drm_sched_entity *entity;
-
if (!ctx->entities[i][j])
continue;

-   entity = >entities[i][j]->entity;
-   drm_sched_entity_set_priority(entity, ctx_prio);
+   amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j],
+  i, ctx_prio);
}
}
 }
--
2.25.0

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

[PATCH v2 2/4] drm/scheduler: implement a function to modify sched list

[Nirmoy Das]

implement drm_sched_entity_modify_sched() which can modify existing
sched_list with a different one. This is going to be helpful when
userspace changes priority of a ctx/entity then driver can switch to
corresponding hw shced list for that priority

Signed-off-by: Nirmoy Das 
Reviewed-by: Christian König 
---
 drivers/gpu/drm/scheduler/sched_entity.c | 19 +++
 include/drm/gpu_scheduler.h  |  4 
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index 63bccd201b97..b94312154e56 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 }
 EXPORT_SYMBOL(drm_sched_entity_init);

+/**
+ * drm_sched_entity_modify_sched - Modify sched of an entity
+ *
+ * @entity: scheduler entity to init
+ * @sched_list: the list of new drm scheds which will replace
+ * existing entity->sched_list
+ * @num_sched_list: number of drm sched in sched_list
+ */
+void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list)
+{
+   WARN_ON(!num_sched_list || !sched_list);
+
+   entity->sched_list = sched_list;
+   entity->num_sched_list = num_sched_list;
+}
+EXPORT_SYMBOL(drm_sched_entity_modify_sched);
+
 /**
  * drm_sched_entity_is_idle - Check if entity is idle
  *
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 589be851f8a1..f70a84aaaf7a 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
 int drm_sched_job_init(struct drm_sched_job *job,
   struct drm_sched_entity *entity,
   void *owner);
+void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+ struct drm_gpu_scheduler **sched_list,
+  unsigned int num_sched_list);
+
 void drm_sched_job_cleanup(struct drm_sched_job *job);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
 void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job 
*bad);
--
2.25.0

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

[PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Nirmoy Das]

We were changing compute ring priority while rings were being used
before every job submission which is not recommended. This patch
sets compute queue priority at mqd initialization for gfx8, gfx9 and
gfx10.

Policy: make queue 0 of each pipe as high priority compute queue

High/normal priority compute sched lists are generated from set of high/normal
priority compute queues. At context creation, entity of compute queue
get a sched list from high or normal priority depending on ctx->priority

Signed-off-by: Nirmoy Das 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 60 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c  |  8 
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 15 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
 drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c   | 19 
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c| 23 +++--
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c| 20 
 9 files changed, 135 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index f397ff97b4e4..8304d0c87899 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
struct drm_sched_entity *entity = p->entity;
enum drm_sched_priority priority;
-   struct amdgpu_ring *ring;
struct amdgpu_bo_list_entry *e;
struct amdgpu_job *job;
uint64_t seq;
@@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
priority = job->base.s_priority;
drm_sched_entity_push_job(>base, entity);

-   ring = to_amdgpu_ring(entity->rq->sched);
-   amdgpu_ring_priority_get(ring, priority);
-
amdgpu_vm_move_to_lru_tail(p->adev, >vm);

ttm_eu_fence_buffer_objects(>ticket, >validated, p->fence);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 94a6c42f29ea..4ad944f85672 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
return -EACCES;
 }

+static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum 
drm_sched_priority prio)
+{
+   switch(prio) {
+   case DRM_SCHED_PRIORITY_MIN:
+   case DRM_SCHED_PRIORITY_NORMAL:
+   case DRM_SCHED_PRIORITY_HIGH_SW:
+   return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+   case DRM_SCHED_PRIORITY_HIGH_HW:
+   case DRM_SCHED_PRIORITY_KERNEL:
+   return AMDGPU_GFX_PIPE_PRIO_HIGH;
+   default:
+   return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+   }
+
+   return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+}
+
 static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, 
const u32 ring)
 {
struct amdgpu_device *adev = ctx->adev;
struct amdgpu_ctx_entity *entity;
struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
unsigned num_scheds = 0;
+   enum gfx_pipe_priority hw_prio;
enum drm_sched_priority priority;
int r;

@@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, 
const u32 hw_ip, const
num_scheds = 1;
break;
case AMDGPU_HW_IP_COMPUTE:
-   scheds = adev->gfx.compute_sched;
-   num_scheds = adev->gfx.num_compute_sched;
+   hw_prio = 
amdgpu_ctx_sched_prio_to_compute_prio(priority);
+   scheds = adev->gfx.compute_prio_sched[hw_prio];
+   num_scheds = adev->gfx.num_compute_sched[hw_prio];
break;
case AMDGPU_HW_IP_DMA:
scheds = adev->sdma.sdma_sched;
@@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
mutex_destroy(>lock);
 }

+
+static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
+{
+   int num_compute_sched_normal = 0;
+   int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
+   int i;
+
+   /* fill compute_sched array as: start from 0th index for normal 
priority scheds and
+* start from (last_index - num_compute_sched_normal) for high priority
+* scheds */
+   for (i = 0; i < adev->gfx.num_compute_rings; i++) {
+   if (!adev->gfx.compute_ring[i].has_high_prio)
+   adev->gfx.compute_sched[num_compute_sched_normal++] =
+   >gfx.compute_ring[i].sched;
+   else
+   adev->gfx.compute_sched[num_compute_sched_high--] =
+   >gfx.compute_ring[i].sched;
+   }
+
+   /* compute ring only has two 

[PATCH 4/4] drm/amdgpu: remove unused functions

[Nirmoy Das]

amdgpu statically set priority for compute queues
at initialization so remove all the functions
responsible changing compute queue priority dynamically

Signed-off-by: Nirmoy Das 
Reviewed-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c |  70 
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |   7 --
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c|  99 --
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c| 100 ---
 4 files changed, 276 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index ca6b52054b4b..a7e1d0425ed0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -150,76 +150,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring)
ring->funcs->end_use(ring);
 }
 
-/**
- * amdgpu_ring_priority_put - restore a ring's priority
- *
- * @ring: amdgpu_ring structure holding the information
- * @priority: target priority
- *
- * Release a request for executing at @priority
- */
-void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
- enum drm_sched_priority priority)
-{
-   int i;
-
-   if (!ring->funcs->set_priority)
-   return;
-
-   if (atomic_dec_return(>num_jobs[priority]) > 0)
-   return;
-
-   /* no need to restore if the job is already at the lowest priority */
-   if (priority == DRM_SCHED_PRIORITY_NORMAL)
-   return;
-
-   mutex_lock(>priority_mutex);
-   /* something higher prio is executing, no need to decay */
-   if (ring->priority > priority)
-   goto out_unlock;
-
-   /* decay priority to the next level with a job available */
-   for (i = priority; i >= DRM_SCHED_PRIORITY_MIN; i--) {
-   if (i == DRM_SCHED_PRIORITY_NORMAL
-   || atomic_read(>num_jobs[i])) {
-   ring->priority = i;
-   ring->funcs->set_priority(ring, i);
-   break;
-   }
-   }
-
-out_unlock:
-   mutex_unlock(>priority_mutex);
-}
-
-/**
- * amdgpu_ring_priority_get - change the ring's priority
- *
- * @ring: amdgpu_ring structure holding the information
- * @priority: target priority
- *
- * Request a ring's priority to be raised to @priority (refcounted).
- */
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
- enum drm_sched_priority priority)
-{
-   if (!ring->funcs->set_priority)
-   return;
-
-   if (atomic_inc_return(>num_jobs[priority]) <= 0)
-   return;
-
-   mutex_lock(>priority_mutex);
-   if (priority <= ring->priority)
-   goto out_unlock;
-
-   ring->priority = priority;
-   ring->funcs->set_priority(ring, priority);
-
-out_unlock:
-   mutex_unlock(>priority_mutex);
-}
-
 /**
  * amdgpu_ring_init - init driver ring struct.
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 201c6ac7bf9d..a75e2418a20e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -167,9 +167,6 @@ struct amdgpu_ring_funcs {
uint32_t reg0, uint32_t reg1,
uint32_t ref, uint32_t mask);
void (*emit_tmz)(struct amdgpu_ring *ring, bool start);
-   /* priority functions */
-   void (*set_priority) (struct amdgpu_ring *ring,
- enum drm_sched_priority priority);
/* Try to soft recover the ring to make the fence signal */
void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
int (*preempt_ib)(struct amdgpu_ring *ring);
@@ -259,10 +256,6 @@ void amdgpu_ring_insert_nop(struct amdgpu_ring *ring, 
uint32_t count);
 void amdgpu_ring_generic_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib 
*ib);
 void amdgpu_ring_commit(struct amdgpu_ring *ring);
 void amdgpu_ring_undo(struct amdgpu_ring *ring);
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
- enum drm_sched_priority priority);
-void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
- enum drm_sched_priority priority);
 int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 unsigned ring_size, struct amdgpu_irq_src *irq_src,
 unsigned irq_type);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 05b6f01e1228..f5029eb9ac12 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6275,104 +6275,6 @@ static void gfx_v8_0_ring_set_wptr_compute(struct 
amdgpu_ring *ring)
WDOORBELL32(ring->doorbell_index, lower_32_bits(ring->wptr));
 }
 
-static void gfx_v8_0_ring_set_pipe_percent(struct amdgpu_ring *ring,
-   

Re: [PATCH v5 1/4] drm/amdgpu: set compute queue priority at mqd_init

[Nirmoy]

On 3/2/20 6:16 PM, Christian König wrote:



or else

@@ -222,7 +229,8 @@ struct amdgpu_ring {
    struct mutex    priority_mutex;
    /* protected by priority_mutex */
    int priority;
-   bool    gfx_pipe_priority;
+
+   enum gfx_pipe_priority  pipe_priority;

doesn't work because of compilation error: " field ‘pipe_priority’ 
has incomplete type"


Mhm, let me ask from the other direction: What is that good for in the 
first place?


As far as I can see this is just to communicate to the ctx handling 
what priority a hw ring has, right?


But what we actually need in the ctx handling is an array of ring with 
normal and high priorty. So why don't we create that in the first place?


Do you mean to create two array ring for both priority ? We still need a 
way to detect ring priority in ctx to populate those array in 
amdgpu_ctx_init_sched.


I think the previous approach to have bool to indicate ring priority 
status should be good enough for ctx. Let me send the next version of 
the patch


to explain what I mean.

Regards,

Nirmoy



Regards,
Christian.


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

RE: [PATCH] drm/amdgpu/sriov: skip programing some regs with new L1 policy

[Zhou, Tiecheng]

[AMD Official Use Only - Internal Distribution Only]

Ping

-Original Message-
From: Tiecheng Zhou  
Sent: Monday, March 2, 2020 3:08 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhou, Tiecheng 
Subject: [PATCH] drm/amdgpu/sriov: skip programing some regs with new L1 policy

With new L1 policy, some regs are blocked at guest and they are programed at 
host side. So skip programing the regs under sriov.

the regs are:
GCMC_VM_FB_LOCATION_TOP
GCMC_VM_FB_LOCATION_BASE
MMMC_VM_FB_LOCATION_TOP
MMMC_VM_FB_LOCATION_BASE
GCMC_VM_SYSTEM_APERTURE_HIGH_ADDR
GCMC_VM_SYSTEM_APERTURE_LOW_ADDR
MMMC_VM_SYSTEM_APERTURE_HIGH_ADDR
MMMC_VM_SYSTEM_APERTURE_LOW_ADDR
HDP_NONSURFACE_BASE
HDP_NONSURFACE_BASE_HI
GCMC_VM_AGP_TOP
GCMC_VM_AGP_BOT
GCMC_VM_AGP_BASE

Signed-off-by: Tiecheng Zhou 
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c | 55 +++-  
drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c  | 29 ++---
 2 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
index e0654a216ab5..cc866c367939 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c
@@ -81,24 +81,31 @@ static void gfxhub_v2_0_init_system_aperture_regs(struct 
amdgpu_device *adev)  {
uint64_t value;
 
-   /* Disable AGP. */
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BOT, 0x00FF);
-
-   /* Program the system aperture low logical page number. */
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
-
-   /* Set default page address. */
-   value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
-   + adev->vm_manager.vram_base_offset;
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_LSB,
-(u32)(value >> 12));
-   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_MSB,
-(u32)(value >> 44));
+   if (!amdgpu_sriov_vf(adev)) {
+   /*
+* the new L1 policy will block SRIOV guest from writing
+* these regs, and they will be programed at host.
+* so skip programing these regs.
+*/
+   /* Disable AGP. */
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BASE, 0);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_TOP, 0);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_AGP_BOT, 0x00FF);
+
+   /* Program the system aperture low logical page number. */
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_LOW_ADDR,
+adev->gmc.vram_start >> 18);
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
+adev->gmc.vram_end >> 18);
+
+   /* Set default page address. */
+   value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
+   + adev->vm_manager.vram_base_offset;
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_LSB,
+(u32)(value >> 12));
+   WREG32_SOC15(GC, 0, mmGCMC_VM_SYSTEM_APERTURE_DEFAULT_ADDR_MSB,
+(u32)(value >> 44));
+   }
 
/* Program "protection fault". */
WREG32_SOC15(GC, 0, mmGCVM_L2_PROTECTION_FAULT_DEFAULT_ADDR_LO32,
@@ -260,18 +267,6 @@ static void gfxhub_v2_0_program_invalidation(struct 
amdgpu_device *adev)
 
 int gfxhub_v2_0_gart_enable(struct amdgpu_device *adev)  {
-   if (amdgpu_sriov_vf(adev)) {
-   /*
-* GCMC_VM_FB_LOCATION_BASE/TOP is NULL for VF, becuase they are
-* VF copy registers so vbios post doesn't program them, for
-* SRIOV driver need to program them
-*/
-   WREG32_SOC15(GC, 0, mmGCMC_VM_FB_LOCATION_BASE,
-adev->gmc.vram_start >> 24);
-   WREG32_SOC15(GC, 0, mmGCMC_VM_FB_LOCATION_TOP,
-adev->gmc.vram_end >> 24);
-   }
-
/* GART Enable. */
gfxhub_v2_0_init_gart_aperture_regs(adev);
gfxhub_v2_0_init_system_aperture_regs(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c 
b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
index bde189680521..fb3f228458e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c
@@ -72,11 +72,18 @@ static void mmhub_v2_0_init_system_aperture_regs(struct 
amdgpu_device *adev)
WREG32_SOC15(MMHUB, 0, mmMMMC_VM_AGP_TOP, 0);
WREG32_SOC15(MMHUB, 0, mmMMMC_VM_AGP_BOT, 0x00FF);
 
-   /* Program the system aperture low logical page number. */
-   WREG32_SOC15(MMHUB, 0, 

[PATCH 0/4] Fix loading of radeon BIOS from 32-bit EFI

[Mikel Rychliski]

This patch set fixes an opps when loading the radeon driver on old Macs
with a 32-bit EFI implementation.

Tested on a MacPro 1,1 with a Radeon X1900 XT card and 32-bit kernel.

Mikel Rychliski (4):
  drm/radeon: Stop directly referencing iomem
  PCI: Use ioremap, not phys_to_virt for platform rom
  drm/radeon: iounmap unused mapping
  drm/amdgpu: iounmap unused mapping

 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c |  1 +
 drivers/gpu/drm/radeon/radeon_bios.c | 12 
 drivers/pci/rom.c|  5 ++---
 3 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.13.7

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

[PATCH 4/4] drm/amdgpu: iounmap unused mapping

[Mikel Rychliski]

Now that pci_platform_rom creates a new mapping to access the ROM
image, we should remove this mapping after extracting the BIOS.

Signed-off-by: Mikel Rychliski 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index 50dff69a0f6e..ea6a1fa98ad9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -207,6 +207,7 @@ static bool amdgpu_read_platform_bios(struct amdgpu_device 
*adev)
return false;
 
memcpy_fromio(adev->bios, bios, size);
+   iounmap(bios);
 
if (!check_atom_bios(adev->bios, size)) {
kfree(adev->bios);
-- 
2.13.7

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

[PATCH 1/4] drm/radeon: Stop directly referencing iomem

[Mikel Rychliski]

pci_platform_rom returns an __iomem pointer which should not be accessed
directly. Change radeon_read_platform_bios to use memcpy_fromio instead of
calling kmemdup on the __iomem pointer.

Signed-off-by: Mikel Rychliski 
---
 drivers/gpu/drm/radeon/radeon_bios.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c 
b/drivers/gpu/drm/radeon/radeon_bios.c
index c42f73fad3e3..c3ae4c92a115 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -118,11 +118,14 @@ static bool radeon_read_platform_bios(struct 
radeon_device *rdev)
return false;
}
 
-   if (size == 0 || bios[0] != 0x55 || bios[1] != 0xaa) {
+   rdev->bios = kzalloc(size, GFP_KERNEL);
+   if (!rdev->bios)
return false;
-   }
-   rdev->bios = kmemdup(bios, size, GFP_KERNEL);
-   if (rdev->bios == NULL) {
+
+   memcpy_fromio(rdev->bios, bios, size);
+
+   if (size == 0 || rdev->bios[0] != 0x55 || rdev->bios[1] != 0xaa) {
+   kfree(rdev->bios);
return false;
}
 
-- 
2.13.7

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

[PATCH 3/4] drm/radeon: iounmap unused mapping

[Mikel Rychliski]

Now that pci_platform_rom creates a new mapping to access the ROM
image, we should remove this mapping after extracting the BIOS.

Signed-off-by: Mikel Rychliski 
---
 drivers/gpu/drm/radeon/radeon_bios.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c 
b/drivers/gpu/drm/radeon/radeon_bios.c
index c3ae4c92a115..712b88d88957 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -123,6 +123,7 @@ static bool radeon_read_platform_bios(struct radeon_device 
*rdev)
return false;
 
memcpy_fromio(rdev->bios, bios, size);
+   iounmap(bios);
 
if (size == 0 || rdev->bios[0] != 0x55 || rdev->bios[1] != 0xaa) {
kfree(rdev->bios);
-- 
2.13.7

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

[PATCH 2/4] PCI: Use ioremap, not phys_to_virt for platform rom

[Mikel Rychliski]

On some EFI systems, the video BIOS is provided by the EFI firmware.  The
boot stub code stores the physical address of the ROM image in pdev->rom.
Currently we attempt to access this pointer using phys_to_virt, which
doesn't work with CONFIG_HIGHMEM.

On these systems, attempting to load the radeon module on a x86_32 kernel
can result in the following:

BUG: unable to handle page fault for address: 3e8ed03c
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
*pde = 
Oops:  [#1] PREEMPT SMP
CPU: 0 PID: 317 Comm: systemd-udevd Not tainted 5.6.0-rc3-next-20200228 #2
Hardware name: Apple Computer, Inc. MacPro1,1/Mac-F4208DC8, BIOS 
MP11.88Z.005C.B08.0707021221 07/02/07
EIP: radeon_get_bios+0x5ed/0xe50 [radeon]
Code: 00 00 84 c0 0f 85 12 fd ff ff c7 87 64 01 00 00 00 00 00 00 8b 47 08 
8b 55 b0 e8 1e 83 e1 d6 85 c0 74 1a 8b 55 c0 85 d2 74 13 <80> 38 55 75 0e 80 78 
01 aa 0f 84 a4 03 00 00 8d 74 26 00 68 dc 06
EAX: 3e8ed03c EBX:  ECX: 3e8ed03c EDX: 0001
ESI: 0004 EDI: eec04000 EBP: eef3fc60 ESP: eef3fbe0
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010206
CR0: 80050033 CR2: 3e8ed03c CR3: 2ec77000 CR4: 06d0
Call Trace:
 ? register_client+0x34/0xe0
 ? register_client+0xab/0xe0
 r520_init+0x26/0x240 [radeon]
 radeon_device_init+0x533/0xa50 [radeon]
 radeon_driver_load_kms+0x80/0x220 [radeon]
 drm_dev_register+0xa7/0x180 [drm]
 radeon_pci_probe+0x10f/0x1a0 [radeon]
 pci_device_probe+0xd4/0x140
 really_probe+0x13d/0x3b0
 driver_probe_device+0x56/0xd0
 device_driver_attach+0x49/0x50
 __driver_attach+0x79/0x130
 ? device_driver_attach+0x50/0x50
 bus_for_each_dev+0x5b/0xa0
 driver_attach+0x19/0x20
 ? device_driver_attach+0x50/0x50
 bus_add_driver+0x117/0x1d0
 ? pci_bus_num_vf+0x20/0x20
 driver_register+0x66/0xb0
 ? 0xf80f4000
 __pci_register_driver+0x3d/0x40
 radeon_init+0x82/0x1000 [radeon]
 do_one_initcall+0x42/0x200
 ? kvfree+0x25/0x30
 ? __vunmap+0x206/0x230
 ? kmem_cache_alloc_trace+0x16f/0x220
 ? do_init_module+0x21/0x220
 do_init_module+0x50/0x220
 load_module+0x1f26/0x2200
 sys_init_module+0x12d/0x160
 do_fast_syscall_32+0x82/0x250
 entry_SYSENTER_32+0xa5/0xf8

Fix the issue by using ioremap instead of phys_to_virt in pci_platform_rom.

Signed-off-by: Mikel Rychliski 
---
 drivers/pci/rom.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 137bf0cee897..e352798eed0c 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -197,8 +197,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
 EXPORT_SYMBOL(pci_unmap_rom);
 
 /**
- * pci_platform_rom - provides a pointer to any ROM image provided by the
- * platform
+ * pci_platform_rom - ioremap the ROM image provided by the platform
  * @pdev: pointer to pci device struct
  * @size: pointer to receive size of pci window over ROM
  */
@@ -206,7 +205,7 @@ void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t 
*size)
 {
if (pdev->rom && pdev->romlen) {
*size = pdev->romlen;
-   return phys_to_virt((phys_addr_t)pdev->rom);
+   return ioremap(pdev->rom, pdev->romlen);
}
 
return NULL;
-- 
2.13.7

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