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

[Mario Limonciello <mario.limonciello@xxxxxxx>]

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@xxxxxxx>
> > ---
> > Cc: Harry.Wentland@xxxxxxx
> >   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);