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

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/25/2023 12:50, Harry Wentland wrote:
> 
> 
> 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?

Sure; I'll follow up with another patch to correct this.

> 
> 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);
>