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