Sorry for miss this. With the Alex's comment addressed, the patch is reviewed-by: Evan Quan <evan.quan@xxxxxxx>
-----Original Message-----
From: Wu, Hersen <hersenxs.wu@xxxxxxx>
Sent: Tuesday, March 3, 2020 9:19 PM
To: Alex Deucher <alexdeucher@xxxxxxxxx>
Cc: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Feng, Kenneth <Kenneth.Feng@xxxxxxx>
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 <alexdeucher@xxxxxxxxx>
Sent: Monday, March 2, 2020 9:27 AM
To: Wu, Hersen <hersenxs.wu@xxxxxxx>
Cc: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Feng, Kenneth <Kenneth.Feng@xxxxxxx>
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 <hersenxs.wu@xxxxxxx> 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 <hersenxs.wu@xxxxxxx>
> ---
> .../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 = &adev->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(&smu->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(&smu->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 <Evan.Quan@xxxxxxx>
> Sent: February 27, 2020 9:58 PM
> To: Wu, Hersen <hersenxs.wu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Feng, Kenneth <Kenneth.Feng@xxxxxxx>; Wu, Hersen
> <hersenxs.wu@xxxxxxx>
> 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(&smu->mutex)"?
>
> -----Original Message-----
> From: Hersen Wu <hersenxs.wu@xxxxxxx>
> Sent: Thursday, February 27, 2020 11:54 PM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Quan, Evan <Evan.Quan@xxxxxxx>; Feng, Kenneth
> <Kenneth.Feng@xxxxxxx>; Wu, Hersen <hersenxs.wu@xxxxxxx>
> Subject: [PATCH 2/2] drm/amdgpu/display: navi1x copy dcn watermark
> clock settings to smu resume from s3
>
> 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 <hersenxs.wu@xxxxxxx>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64
> +++++++++++++++++++
> 1 file changed, 64 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..c58c0e95735e 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,68 @@ 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 = &adev->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;
> + }
> +
> + /* 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;
> + }
> + smu->watermarks_bitmap |= WATERMARKS_LOADED;
> + }
> +
> + return 0;
> +}
> +
> /**
> * dm_hw_init() - Initialize DC device
> * @handle: The base driver device containing the amdgpu_dm device.
> @@ -1713,6 +1775,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
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Che
> rsenxs.wu%40amd.com%7C4709bedf60fc41d1ae0508d7beb5c120%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637187560164248086&sdata=6wH59U4RtXQ
> lL6Z2EcIKQWvBNfV0shDAUD05ARVs2MA%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
