On Wed, Jan 31, 2018 at 6:27 PM, Felix Kuehling <felix.kuehling at amd.com> wrote:
> On 2018-01-31 10:29 AM, Oded Gabbay wrote:
>> On Wed, Jan 31, 2018 at 5:23 PM, Deucher, Alexander
>> <Alexander.Deucher at amd.com> wrote:
>>> ________________________________
>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of Oded
>>> Gabbay <oded.gabbay at gmail.com>
>>> Sent: Wednesday, January 31, 2018 10:17 AM
>>> To: Kuehling, Felix
>>> Cc: amd-gfx list
>>> Subject: Re: [PATCH 7/9] drm/amdkfd: Add dGPU support to kernel_queue_init
>>>
>>> On Fri, Jan 5, 2018 at 12:17 AM, Felix Kuehling <Felix.Kuehling at amd.com>
>>> wrote:
>>>> Recognize dGPU ASIC families.
>>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>>>> index 5dc6567..69f4964 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
>>>> @@ -297,10 +297,15 @@ struct kernel_queue *kernel_queue_init(struct
>>>> kfd_dev *dev,
>>>>
>>>> switch (dev->device_info->asic_family) {
>>>> case CHIP_CARRIZO:
>>>> + case CHIP_TONGA:
>>>> + case CHIP_FIJI:
>>>> + case CHIP_POLARIS10:
>>>> + case CHIP_POLARIS11:
>>> I believe POLARIS is from arcatic islands, no ?
>>> Maybe rename kernel_queue_init_vi to kernel_queue_init_vi_ai ?
>>> or create a new function kernel_queue_init_ai() and assign same
>>> functions as vi ?
>>> Either way, I think you need to address that.
>>>
>>> They are all gfx8. adding ai just confuses things.
>
> Internally we use VI and GFX8 interchangably. I think what's confusing
> is, that internal code names are used for marketing purposes and applied
> to the wrong chip generation.
>
> Another precedent for that is Hawaii. It was called the first "volcanic
> island" GPU when it was launched at an event on Hawaii (a volcanic
> island), but as far as the driver is concerned, it belongs to the CIK
> generation.
>
> It's really hard to keep consistent naming, when naming conventions get
> misappropriated to mean different things over time.
>
>>>
>>> Alex
>> In that case, I think it is better maybe to change it to
>> kernel_queue_init_gfx_7 and kernel_queue_init_gfx_8, to be consistent
>> with the calls to amdgpu_amdkfd_gfx_7_0_get_functions and
>> amdgpu_amdkfd_gfx_8_0_get_functions.
>>
>> Leaving as cik and vi as the identifier when it clearly isn't seems
>> confusing to me as well.
>
> For Vega10 we use the suffix _v9 instead of _cik or _vi. For consistency
> and brevity I could rename _cik->v7 and _vi->v8. However, that would be
> a lot of churn and, in my eyes, a waste of time.
I agree its a lot of churn but I don't think its a total waste of
time, even if those devices are not really important anymore.
I'll try to find some time to do it.
Oded
>
> Regards,
> Felix
>
>>
>> Oded
>>
>>>
>>>> kernel_queue_init_vi(&kq->ops_asic_specific);
>>>> break;
>>>>
>>>> case CHIP_KAVERI:
>>>> + case CHIP_HAWAII:
>>>> kernel_queue_init_cik(&kq->ops_asic_specific);
>>>> break;
>>>> default:
>>>> --
>>>> 2.7.4
>>>>
>>> Other then that, This patch is:
>>> Reviewed-by: Oded Gabbay <oded.gabbay at gmail.com>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
