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