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

[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. 

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;
> >>>    }
> >>>