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

[Zhu, Rex]

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> Sent: Wednesday, October 17, 2018 3:11 AM
> To: Zhu, Rex <Rex.Zhu at amd.com>; Koenig, Christian
> <Christian.Koenig 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 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?

Yes, there is a ticket: SWDEV-168119

Regards
Rex
 
> 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