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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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&amp;data=02%7C01%7Che
> rsenxs.wu%40amd.com%7C4709bedf60fc41d1ae0508d7beb5c120%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637187560164248086&amp;sdata=6wH59U4RtXQ
> lL6Z2EcIKQWvBNfV0shDAUD05ARVs2MA%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux