[PATCH v2] drm/amdgpu:No action when VCN PG state is unchanged

[Christian König]

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.

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