[PATCH v2] drm/amdgpu: Poweroff uvd/vce/vcn/acp block if they were disabled by user

[Koenig, Christian]

Am 16.10.2018 um 20:54 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>> Sent: Wednesday, October 17, 2018 2:31 AM
>> To: Zhu, Rex <Rex.Zhu at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH v2] drm/amdgpu: Poweroff uvd/vce/vcn/acp block if
>> they were disabled by user
>>
>> Am 16.10.2018 um 20:25 schrieb Rex Zhu:
>>> If user disable uvd/vce/vcn/acp blocks via module parameter
>>> ip_block_mask, driver power off thoser blocks to save power.
>>>
>>> v2: power off uvd/vce/vcn/acp via smu.
>> I'm not sure if that is a good idea.
>>
>> The meaning of the flag is to NOT touch the blocks at all, e.g. pretend they
>> don't even exists.
>>
>> Additional to that the ip_block_mask is only for bringup and debugging and
>> not meant for production use and power saving shouldn't be an issue there.
> Hi Christian,
>
> This is requested by Chrome project. As there is an S3 issue caused by VCE, and they still don't find the root cause.
> And in Chrome project, they don't need to use vce engine.
> So as a workaround, they want to disable vce and power off the vce engine to save power.

Oh, please not that stupid idea again! In this case that is a clear NAK.

The mask isn't necessary stable and this module parameter is absolutely 
NOT intended for production use!

If we want to disable a specific block permanently in a project we 
should do so by adding a patch to the specific branch and NOT by abusing 
ip_block_mask for it.

Regards,
Christian.

>
> Best Regards
> Rex
>   
>> So I would rather say that this is a NAK.
>>
>> Christian.
>>
>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 19
>> +++++++++++++++++++
>>>    1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 6fe6ea9..ef9fe50 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1776,6 +1776,24 @@ static int amdgpu_device_set_pg_state(struct
>>> amdgpu_device *adev, enum amd_power
>>>
>>>    	for (j = 0; j < adev->num_ip_blocks; j++) {
>>>    		i = state == AMD_PG_STATE_GATE ? j : adev->num_ip_blocks
>> - j - 1;
>>> +
>>> +		/* try to power off VCE/UVD/VCN/ACP if they were disabled
>> by user */
>>> +		if ((adev->ip_blocks[i].version->type ==
>> AMD_IP_BLOCK_TYPE_UVD ||
>>> +		    adev->ip_blocks[i].version->type ==
>> AMD_IP_BLOCK_TYPE_VCE ||
>>> +		    adev->ip_blocks[i].version->type ==
>> AMD_IP_BLOCK_TYPE_VCN ||
>>> +		    adev->ip_blocks[i].version->type ==
>> AMD_IP_BLOCK_TYPE_ACP) &&
>>> +		    adev->powerplay.pp_funcs->set_powergating_by_smu) {
>>> +			if (!adev->ip_blocks[i].status.valid) {
>>> +
>> 	amdgpu_dpm_set_powergating_by_smu(adev, adev-
>>> ip_blocks[i].version->type, state == AMD_PG_STATE_GATE ?
>>> +
>> 					true : false);
>>> +				if (r) {
>>> +
>> 	DRM_ERROR("set_powergating_state(gate) of IP block <%s>
>> failed %d\n",
>>> +					  adev->ip_blocks[i].version->funcs-
>>> name, r);
>>> +					return r;
>>> +				}
>>> +			}
>>> +		}
>>> +
>>>    		if (!adev->ip_blocks[i].status.late_initialized)
>>>    			continue;
>>>    		/* skip CG for VCE/UVD, it's handled specially */ @@ -1793,6
>>> +1811,7 @@ static int amdgpu_device_set_pg_state(struct amdgpu_device
>> *adev, enum amd_power
>>>    			}
>>>    		}
>>>    	}
>>> +
>>>    	return 0;
>>>    }
>>>