[PATCH 2/3] drm/amdgpu: expose only the first UVD instance for now

[Christian König]

Am 18.07.2018 um 16:22 schrieb James Zhu:
>
>
> On 2018-07-18 10:11 AM, Christian König wrote:
>> Am 18.07.2018 um 15:40 schrieb James Zhu:
>>>
>>> On 2018-07-18 08:55 AM, Christian König wrote:
>>>> Going to completely rework the context to ring mapping with Nayan's 
>>>> GSoC
>>>> work, but for now just stopping to expose the second UVD instance 
>>>> should
>>>> do it.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 13 +++++--------
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c |  9 ++-------
>>>>   2 files changed, 7 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index e451a3f25beb..75da3c41f3b3 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -286,7 +286,7 @@ static int amdgpu_info_ioctl(struct drm_device 
>>>> *dev, void *data, struct drm_file
>>>>       struct drm_crtc *crtc;
>>>>       uint32_t ui32 = 0;
>>>>       uint64_t ui64 = 0;
>>>> -    int i, j, found;
>>>> +    int i, found;
>>>>       int ui32_size = sizeof(ui32);
>>>>         if (!info->return_size || !info->return_pointer)
>>>> @@ -348,8 +348,7 @@ static int amdgpu_info_ioctl(struct drm_device 
>>>> *dev, void *data, struct drm_file
>>>>               break;
>>>>           case AMDGPU_HW_IP_UVD:
>>>>               type = AMD_IP_BLOCK_TYPE_UVD;
>>>> -            for (i = 0; i < adev->uvd.num_uvd_inst; i++)
>>>> -                ring_mask |= adev->uvd.inst[i].ring.ready << i;
>>>> +            ring_mask |= adev->uvd.inst[0].ring.ready;
>>>>               ib_start_alignment = 64;
>>>>               ib_size_alignment = 64;
>>>>               break;
>>>> @@ -362,11 +361,9 @@ static int amdgpu_info_ioctl(struct drm_device 
>>>> *dev, void *data, struct drm_file
>>>>               break;
>>>>           case AMDGPU_HW_IP_UVD_ENC:
>>>>               type = AMD_IP_BLOCK_TYPE_UVD;
>>>> -            for (i = 0; i < adev->uvd.num_uvd_inst; i++)
>>>> -                for (j = 0; j < adev->uvd.num_enc_rings; j++)
>>>> -                    ring_mask |=
>>>> -                    adev->uvd.inst[i].ring_enc[j].ready <<
>>>> -                    (j + i * adev->uvd.num_enc_rings);
>>>> +            for (i = 0; i < adev->uvd.num_enc_rings; i++)
>>>> +                ring_mask |=
>>>> +                    adev->uvd.inst[0].ring_enc[i].ready << i;
>>>>               ib_start_alignment = 64;
>>>>               ib_size_alignment = 64;
>>>>               break;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
>>>> index ea9850c9224d..d8357290ad09 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
>>>> @@ -66,8 +66,6 @@ static int amdgpu_identity_map(struct 
>>>> amdgpu_device *adev,
>>>>                      u32 ring,
>>>>                      struct amdgpu_ring **out_ring)
>>>>   {
>>>> -    u32 instance;
>>>> -
>>>>       switch (mapper->hw_ip) {
>>>>       case AMDGPU_HW_IP_GFX:
>>>>           *out_ring = &adev->gfx.gfx_ring[ring];
>>>> @@ -79,16 +77,13 @@ static int amdgpu_identity_map(struct 
>>>> amdgpu_device *adev,
>>>>           *out_ring = &adev->sdma.instance[ring].ring;
>>>>           break;
>>>>       case AMDGPU_HW_IP_UVD:
>>>> -        instance = ring;
>>>> -        *out_ring = &adev->uvd.inst[instance].ring;
>>>> +        *out_ring = &adev->uvd.inst[0].ring;
>>>>           break;
>>>>       case AMDGPU_HW_IP_VCE:
>>>>           *out_ring = &adev->vce.ring[ring];
>>>>           break;
>>>>       case AMDGPU_HW_IP_UVD_ENC:
>>>> -        instance = ring / adev->uvd.num_enc_rings;
>>>> -        *out_ring =
>>>> - &adev->uvd.inst[instance].ring_enc[ring%adev->uvd.num_enc_rings];
>>>> +        *out_ring = &adev->uvd.inst[0].ring_enc[ring];
>>>
>>> Hi Christian,
>>>
>>> If here uses amdgpu_identity_map, I think only first instance's ring 
>>> is in use.
>>>
>>> In current Vega20 situation, should we change to use amdgpu_lru_map 
>>> or something else to schedule which instance/ring for use ?
>>
>> Yes and no. I'm going to completely remove the queue manager and 
>> replace it with Nayan's work on the scheduler.
>>
>> So no more lru map, but instead a function which picks the best ring 
>> based on actual load during context creation.
>>
>> Regards,
>> Christian.
> Got it. So do we totally remove instance and ring concept out of user 
> space scope?

Yes, that's at least the idea.

> This design is good for use, but it lost flexibility on fine resource
> management. It may be requested by cgroup's use case in the future.

I actually do this to allow simpler cgroup implementation on top of this.

In other words cgroup can still be used to limit certain applications to 
certain hardware engines and/or sw priorities, etc...

Regards,
Christian.

>
> Regards!
> James
>>
>>>
>>> James
>>>
>>>>           break;
>>>>       case AMDGPU_HW_IP_VCN_DEC:
>>>>           *out_ring = &adev->vcn.ring_dec;
>>>
>>
>