On 2018-09-21 10:52 AM, Alex Deucher wrote:
> On Fri, Sep 21, 2018 at 10:51 AM James Zhu <jamesz at amd.com> wrote:
>>
>>
>> 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.
> Picasso support has been merged into drm-next as of last week.
I see, then I will rebase it to drm-next also.
James.
>
> Alex
>
>> 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
