Re: [PATCH] drm/amd: Propagate failures in dc_set_power_state()

[Harry Wentland <harry.wentland@amd.com>]

On 2023-09-22 16:12, Mario Limonciello wrote:
> During the suspend process dc_set_power_state() will use kzalloc
> to allocate memory, but this potentially fails with memory pressure.
> If it fails, the suspend should be aborted.
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2362
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Cc: Harry.Wentland@amd.com
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 ++++++++-----
>  drivers/gpu/drm/amd/display/dc/core/dc.c          |  8 +++++---
>  drivers/gpu/drm/amd/display/dc/dc.h               |  2 +-
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 373884ca38b9..2acb555343ae 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2670,9 +2670,7 @@ static int dm_suspend(void *handle)
>  
>  	hpd_rx_irq_work_suspend(dm);
>  
> -	dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D3);
> -
> -	return 0;
> +	return dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D3);
>  }
>  
>  struct amdgpu_dm_connector *
> @@ -2865,7 +2863,10 @@ static int dm_resume(void *handle)
>  		if (r)
>  			DRM_ERROR("DMUB interface failed to initialize: status=%d\n", r);
>  
> -		dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D0);
> +		r = dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D0);
> +		if (r)
> +			return r;
> +
>  		dc_resume(dm->dc);
>  
>  		amdgpu_dm_irq_resume_early(adev);
> @@ -2914,7 +2915,9 @@ static int dm_resume(void *handle)
>  	}
>  
>  	/* power on hardware */
> -	dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D0);
> +	r = dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D0);
> +	if (r)
> +		return r;
>  
>  	/* program HPD filter */
>  	dc_resume(dm->dc);
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 293489c41086..a1593d550526 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -4711,7 +4711,7 @@ void dc_power_down_on_boot(struct dc *dc)
>  		dc->hwss.power_down_on_boot(dc);
>  }
>  
> -void dc_set_power_state(
> +int dc_set_power_state(
>  	struct dc *dc,
>  	enum dc_acpi_cm_power_state power_state)
>  {
> @@ -4719,7 +4719,7 @@ void dc_set_power_state(
>  	struct display_mode_lib *dml;
>  
>  	if (!dc->current_state)
> -		return;
> +		return 0;
>  
>  	switch (power_state) {
>  	case DC_ACPI_CM_POWER_STATE_D0:
> @@ -4746,7 +4746,7 @@ void dc_set_power_state(
>  
>  		ASSERT(dml);
>  		if (!dml)
> -			return;
> +			return -ENOMEM;
>  

DC code doesn't have a concept of Linux error codes and is
shared with other platforms. Can we follow a similar paradigm
to the other DC interface functions and return a bool, with
"false" on failure?

Harry

>  		/* Preserve refcount */
>  		refcount = dc->current_state->refcount;
> @@ -4764,6 +4764,8 @@ void dc_set_power_state(
>  
>  		break;
>  	}
> +
> +	return 0;
>  }
>  
>  void dc_resume(struct dc *dc)
> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index faf897ac75d8..82013ebcba91 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> @@ -2329,7 +2329,7 @@ void dc_notify_vsync_int_state(struct dc *dc, struct dc_stream_state *stream, bo
>  
>  /* Power Interfaces */
>  
> -void dc_set_power_state(
> +int dc_set_power_state(
>  		struct dc *dc,
>  		enum dc_acpi_cm_power_state power_state);
>  void dc_resume(struct dc *dc);