[PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged
Alex Deucher
alexdeucher at gmail.com
Fri Sep 21 14:52:00 UTC 2018
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.
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
>
More information about the amd-gfx
mailing list