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

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