* [PATCH] drm/amd: Propagate failures in dc_set_power_state()
@ 2023-09-22 20:12 Mario Limonciello
2023-09-25 14:32 ` Alex Deucher
2023-09-25 17:50 ` Harry Wentland
0 siblings, 2 replies; 4+ messages in thread
From: Mario Limonciello @ 2023-09-22 20:12 UTC (permalink / raw)
To: amd-gfx; +Cc: Harry.Wentland, Mario Limonciello
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;
/* 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);
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd: Propagate failures in dc_set_power_state()
2023-09-22 20:12 [PATCH] drm/amd: Propagate failures in dc_set_power_state() Mario Limonciello
@ 2023-09-25 14:32 ` Alex Deucher
2023-09-25 17:50 ` Harry Wentland
1 sibling, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2023-09-25 14:32 UTC (permalink / raw)
To: Mario Limonciello; +Cc: Harry.Wentland, amd-gfx
On Fri, Sep 22, 2023 at 4:51 PM Mario Limonciello
<mario.limonciello@amd.com> 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>
Acked-by: Alex Deucher <alexander.deucher@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;
>
> /* 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);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd: Propagate failures in dc_set_power_state()
2023-09-22 20:12 [PATCH] drm/amd: Propagate failures in dc_set_power_state() Mario Limonciello
2023-09-25 14:32 ` Alex Deucher
@ 2023-09-25 17:50 ` Harry Wentland
2023-09-25 17:51 ` Mario Limonciello
1 sibling, 1 reply; 4+ messages in thread
From: Harry Wentland @ 2023-09-25 17:50 UTC (permalink / raw)
To: Mario Limonciello, amd-gfx
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd: Propagate failures in dc_set_power_state()
2023-09-25 17:50 ` Harry Wentland
@ 2023-09-25 17:51 ` Mario Limonciello
0 siblings, 0 replies; 4+ messages in thread
From: Mario Limonciello @ 2023-09-25 17:51 UTC (permalink / raw)
To: Harry Wentland, amd-gfx
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);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-25 17:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-22 20:12 [PATCH] drm/amd: Propagate failures in dc_set_power_state() Mario Limonciello
2023-09-25 14:32 ` Alex Deucher
2023-09-25 17:50 ` Harry Wentland
2023-09-25 17:51 ` Mario Limonciello
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox