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

[Christian König]

Am 16.10.2018 um 21:03 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Wednesday, October 17, 2018 2:59 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: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.
> This is also requested by Chrome project. Customer wish all the patches are cherry-pick from drm-next/linux kernel tree.
> Maybe we need to fix the vce issue on time.

Yeah, completely agree.

Exactly that's the kind of stuff why Chrome has the restriction to only 
cherry-pick patches from upstream.

So we can either go ahead and disable VCE upstream as well or we sit 
down and fix the issue.

Do you have a bug tracker for that?

Regards,
Christian.

>
> Best Regards
> Rex
>
>> 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;
>>>>>     }
>>>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx