On 2018-09-21 10:48 AM, Zhu, Rex wrote:
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>> James Zhu
>> Sent: Friday, September 21, 2018 10:22 PM
>> To: Zhu, Rex <Rex.Zhu at amd.com>; Koenig, Christian
>> <Christian.Koenig at amd.com>; Zhu, James <James.Zhu at amd.com>; Alex
>> Deucher <alexdeucher at gmail.com>
>> Cc: James Zhu <jzhums at gmail.com>; amd-gfx list <amd-
>> gfx at lists.freedesktop.org>
>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is
>> unchanged
>>
>>
>>
>> On 2018-09-21 10:05 AM, Zhu, Rex wrote:
>>> In my understand, when dpg mode enabled, we don't need to power
>>> up/down VCN via SMU and Also don't need to set vcn power gate/ungate
>> state.
>>> Just need to enable the dpg mode.
>>>
>>> If not true, please correct me.
>> You are right. Under DPG mode, vcn_v1_0_start is used to setup DPG mode
>> instead of ungating power.
>> For code reuse purpose, the function/variable name may have different
>> connotation under different mode.
> Did your patch base on tip drm-next code?
No based on amd-temp-pco. Since DPG mode is enabled only on PCO at this
moment.
James
>
> Rex
>
>> James
>>> Best Regards
>>> Rex
>>>
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>>> Christian König
>>>> Sent: Friday, September 21, 2018 8:53 PM
>>>> To: Zhu, James <James.Zhu at amd.com>; Alex Deucher
>>>> <alexdeucher at gmail.com>
>>>> Cc: James Zhu <jzhums at gmail.com>; Zhu, James <James.Zhu at amd.com>;
>>>> amd-gfx list <amd-gfx at lists.freedesktop.org>
>>>> Subject: Re: [PATCH v2] drm/amdgpu:No action when VCN PG state is
>>>> unchanged
>>>>
>>>> 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
>>>> _______________________________________________
>>>> 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
