Am 21.09.2018 um 14:28 schrieb James Zhu:
>
>
> On 2018-09-20 01:54 PM, Christian König wrote:
>> Am 20.09.2018 um 18:24 schrieb James Zhu:
>>>
>>>
>>> On 2018-09-20 11:49 AM, Alex Deucher wrote:
>>>> On Thu, Sep 20, 2018 at 11:27 AM James Zhu <jamesz at amd.com> wrote:
>>>>>
>>>>>
>>>>> On 2018-09-20 11:14 AM, Alex Deucher wrote:
>>>>>> On Thu, Sep 13, 2018 at 4:56 PM James Zhu <jzhums at gmail.com> wrote:
>>>>>>> When VCN PG state is unchanged, it is unnecessary to reset power
>>>>>>> gate state
>>>>>>>
>>>>>>> Signed-off-by: James Zhu <James.Zhu at amd.com>
>>>>>>> ---
>>>>>>> Â Â drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h |Â 1 +
>>>>>>>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c  | 12 ++++++++++--
>>>>>>> Â Â 2 files changed, 11 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>>>> index 0b0b863..d2219ab 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
>>>>>>> @@ -69,6 +69,7 @@ struct amdgpu_vcn {
>>>>>>>          struct amdgpu_ring     ring_jpeg;
>>>>>>>          struct amdgpu_irq_src  irq;
>>>>>>>          unsigned               num_enc_rings;
>>>>>>> +Â Â Â Â Â Â enum amd_powergating_state cur_state;
>>>>>> Does the default value (0) at init time properly reflect the default
>>>>>> powergating state? If so,
>>>>>> Acked-by: Alex Deucher <alexander.deucher at amd.com>
>>>>> Yes, the below code shows it will be set to 0 during driver load
>>>>> stage.
>>>> Yes, I understand that. Is 0 (AMD_PG_STATE_GATE) what we want as the
>>>> default though? The first time the code runs are we going to do the
>>>> right thing or is the code going to return early? IIRC, the hw
>>>> default is ungated.
>>> cur_state is used for tracking driver SW PG state, not HWÂ PG state.
>>> I though no matter what HWÂ PG state is after device powers up, when
>>> first vcn ring is scheduled to run,
>>> begin_use->set_powergating_state->vcn_v1_0_start->ungate
>>> power/clock->Boot_VCPUÂ will be tried.
>>>
>>> For DPG mode, the ungate power/clock , boot VCPU will not actually
>>> be activated during start setup stage, and
>>> only be activated during ring run stage.
>>
>> Mhm, I wonder if it wouldn't be better to have that functionality one
>> layer up.
>>
>> E.g. in amdgpu_device_ip_set_powergating_state so that it applies to
>> all IP blocks in the same way.
> If necessary, I can add support for IP blocks later.
Yeah, agree. In general I'm not doing much with power management, so
can't judge if that is a good idea or not.
Anyway feel free to add my Acked-by to the patch.
Regards,
Christian.
> James
>>
>> But on the other hand the correct solution looks good to me as well,
>> so feel free to add my Acked-by as well.
>>
>> Christian.
>>
>>>
>>> James
>>>> Alex
>>>>> int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long
>>>>> flags)
>>>>> ....
>>>>> Â Â Â Â Â adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
>>>>>
>>>>> struct amdgpu_device {
>>>>> ....
>>>>>      struct amdgpu_vcn       vcn;
>>>>>
>>>>> Best Regards!
>>>>> James zhu
>>>>>>> Â Â };
>>>>>>>
>>>>>>> Â Â int amdgpu_vcn_sw_init(struct amdgpu_device *adev);
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>>> index 2664bb2..2cde0b4 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>>> @@ -1633,12 +1633,20 @@ static int
>>>>>>> vcn_v1_0_set_powergating_state(void *handle,
>>>>>>> Â Â Â Â Â Â Â Â Â Â * revisit this when there is a cleaner line between
>>>>>>> Â Â Â Â Â Â Â Â Â Â * the smc and the hw blocks
>>>>>>> Â Â Â Â Â Â Â Â Â Â */
>>>>>>> +Â Â Â Â Â Â int ret;
>>>>>>> Â Â Â Â Â Â Â Â Â struct amdgpu_device *adev = (struct amdgpu_device
>>>>>>> *)handle;
>>>>>>>
>>>>>>> +Â Â Â Â Â Â if(state == adev->vcn.cur_state)
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â return 0;
>>>>>>> +
>>>>>>> Â Â Â Â Â Â Â Â Â if (state == AMD_PG_STATE_GATE)
>>>>>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â return vcn_v1_0_stop(adev);
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = vcn_v1_0_stop(adev);
>>>>>>> Â Â Â Â Â Â Â Â Â else
>>>>>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â return vcn_v1_0_start(adev);
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = vcn_v1_0_start(adev);
>>>>>>> +
>>>>>>> +Â Â Â Â Â Â if(!ret)
>>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â adev->vcn.cur_state = state;
>>>>>>> +Â Â Â Â Â Â return ret;
>>>>>>> Â Â }
>>>>>>>
>>>>>>> Â Â static const struct amd_ip_funcs vcn_v1_0_ip_funcs = {
>>>>>>> --
>>>>>>> 2.7.4
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> amd-gfx mailing list
>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>
