[PATCH v2 2/4] drm/amdgpu: Put enable gfx off feature to a delay thread

[Felix Kuehling]

Sorry, I just saw your discussion with Evan. Checking the conditions in
the worker function should work. Feel free to add my Reviewed-by.

Regards,
  Felix


On 2018-07-30 03:33 PM, Felix Kuehling wrote:
> NACK.
>
> See inline ...
>
>
> On 2018-07-30 05:23 AM, Rex Zhu wrote:
>> delay to enable gfx off feature to avoid gfx on/off frequently
>> suggested by Alex and Evan.
>>
>> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  2 ++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 15 +++++++++++++++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c    |  8 ++++++--
>>  3 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 188bc53..ff16689 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -992,6 +992,8 @@ struct amdgpu_gfx {
>>  	bool                            gfx_off_state; /* true: enabled, false: disabled */
>>  	struct mutex                    gfx_off_mutex;
>>  	uint32_t                        gfx_off_req_count; /* default 1, enable gfx off: dec 1, disable gfx off: add 1 */
>> +	struct delayed_work             gfx_off_delay_work;
>> +
>>  	/* pipe reservation */
>>  	struct mutex			pipe_reserve_mutex;
>>  	DECLARE_BITMAP			(pipe_reserve_bitmap, AMDGPU_MAX_COMPUTE_QUEUES);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 122653f..f6a6fb5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1925,6 +1925,19 @@ static void amdgpu_device_ip_late_init_func_handler(struct work_struct *work)
>>  		DRM_ERROR("ib ring test failed (%d).\n", r);
>>  }
>>  
>> +static void amdgpu_device_delay_enable_gfx_off(struct work_struct *work)
>> +{
>> +	struct amdgpu_device *adev =
>> +		container_of(work, struct amdgpu_device, gfx.gfx_off_delay_work.work);
>> +
>> +	mutex_lock(&adev->gfx.gfx_off_mutex);
>> +	if (!adev->gfx.gfx_off_state && !adev->gfx.gfx_off_req_count) {
>> +		if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, true))
>> +			adev->gfx.gfx_off_state = true;
>> +	}
>> +	mutex_unlock(&adev->gfx.gfx_off_mutex);
>> +}
>> +
>>  /**
>>   * amdgpu_device_ip_suspend_phase1 - run suspend for hardware IPs (phase 1)
>>   *
>> @@ -2394,6 +2407,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>  
>>  	INIT_DELAYED_WORK(&adev->late_init_work,
>>  			  amdgpu_device_ip_late_init_func_handler);
>> +	INIT_DELAYED_WORK(&adev->gfx.gfx_off_delay_work,
>> +			  amdgpu_device_delay_enable_gfx_off);
>>  
>>  	adev->gfx.gfx_off_req_count = 1;
>>  	adev->pm.ac_power = power_supply_is_system_supplied() > 0 ? true : false;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index 1cdb264..11d4d9f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -26,6 +26,9 @@
>>  #include "amdgpu.h"
>>  #include "amdgpu_gfx.h"
>>  
>> +/* 0.5 second timeout */
>> +#define GFX_OFF_DELAY_ENABLE         msecs_to_jiffies(500)
>> +
>>  /*
>>   * GPU scratch registers helpers function.
>>   */
>> @@ -360,6 +363,7 @@ void amdgpu_gfx_off_ctrl(struct amdgpu_device *adev, bool enable)
>>  	if (!adev->powerplay.pp_funcs->set_powergating_by_smu)
>>  		return;
>>  
>> +
>>  	mutex_lock(&adev->gfx.gfx_off_mutex);
>>  
>>  	if (!enable)
>> @@ -368,11 +372,11 @@ void amdgpu_gfx_off_ctrl(struct amdgpu_device *adev, bool enable)
>>  		adev->gfx.gfx_off_req_count--;
>>  
>>  	if (enable && !adev->gfx.gfx_off_state && !adev->gfx.gfx_off_req_count) {
>> -		if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, true))
>> -			adev->gfx.gfx_off_state = true;
>> +		schedule_delayed_work(&adev->gfx.gfx_off_delay_work, GFX_OFF_DELAY_ENABLE);
>>  	} else if (!enable && adev->gfx.gfx_off_state) {
> There may still be delayed work pending to enable GFXOFF. You need to
> use cancel_delayed_work_sync to cancel it and make sure GFXOFF doesn't
> get enabled again while your caller still needs it disabled.
>
> That may also wait for a worker function that has already started.
> Therefore you need to cancel before taking the gfx_off_mutex, otherwise
> you can deadlock with the worker function trying to take the lock as well.
>
> Regards,
>   Felix
>
>>  		if (!amdgpu_dpm_set_powergating_by_smu(adev, AMD_IP_BLOCK_TYPE_GFX, false))
>>  			adev->gfx.gfx_off_state = false;
>>  	}
>> +
>>  	mutex_unlock(&adev->gfx.gfx_off_mutex);
>>  }
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx