Re: [PATCH 1/1] drm/amdgpu: export gfx capability by gpu info

["Christian König" <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]

Am 16.02.2017 um 10:24 schrieb Zhang, Jerry:
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Thursday, February 16, 2017 17:12
>> To: Zhang, Jerry; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 1/1] drm/amdgpu: export gfx capability by gpu info
>>
>> Am 16.02.2017 um 03:53 schrieb Junwei Zhang:
>>> Change-Id: Ibf3e4dbb7deb83271adabc275c9b7a0e0652541a
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> Complete NAK on this. The drm_amdgpu_capability structure is specific to the
>> pro driver.
> Thanks to reminder it.
> I neglect it.
>   
>> The Vulkan stack on the other hand is supposed to work on the open stack as
>> well.
>>
>> So please but that into the drm_amdgpu_info_device structure and the new field
>> in the amdgpu_gca_config structure (you might as well want to rename the
>> amdgpu_gca_config structure).
> So you mean add the feature or config structure in amdgpu_device directly from now on.
> Right?

No, just forget about the config or feature structure. Add gfx features 
to amdgpu_gca_config.

> Could you explain the meaning about gca?

I just think that the naming isn't the best, something like 
amdgpu_gfx_config would probably be better.

>
> BTW,
> 1) in the future we may add more feature/config in the same structure, but may not for gfx.

Stop, that is really bad design. The features/config should be kept on a 
per IP basis.

So gfx config/features should only be grouped together with gfx 
features. Don't mix features from different block inside the kernel module.

The IOCTL interface is a different story, here we have the 
drm_amdgpu_info_device structure for information about the whole GPU as 
well as individual structures about each IP block (like firmware 
versions etc...).

For the current case adding that to drm_amdgpu_info_device sounds like 
the right approach to me.

> 2) Shall we add a version to control these features? UMD could know the features status according to the version.

No, the size of the drm_amdgpu_info_device structure is already version 
controlled.

Just make sure that the default value is zero, this way you keep 
backward compatible with older kernel interfaces.

Regards,
Christian.

>
>> Regards,
>> Christian.
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  6 ++++++
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  2 ++
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c   |  6 ++++++
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c   |  6 ++++++
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c   | 14 ++++++++++++++
>>>    include/uapi/drm/amdgpu_drm.h           |  1 +
>>>    6 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 1ad3f08..cdc2b2a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -869,6 +869,10 @@ struct amdgpu_gfx_funcs {
>>>    	void (*read_wave_sgprs)(struct amdgpu_device *adev, uint32_t simd,
>> uint32_t wave, uint32_t start, uint32_t size, uint32_t *dst);
>>>    };
>>>
>>> +struct amdgpu_gfx_cap {
>>> +	uint32_t gc_double_offchip_lds_buf;
>>> +};
>>> +
>>>    struct amdgpu_gfx {
>>>    	struct mutex			gpu_clock_mutex;
>>>    	struct amdgpu_gca_config	config;
>>> @@ -911,6 +915,8 @@ struct amdgpu_gfx {
>>>    	/* reset mask */
>>>    	uint32_t                        grbm_soft_reset;
>>>    	uint32_t                        srbm_soft_reset;
>>> +
>>> +	struct amdgpu_gfx_cap		cap;
>>>    };
>>>
>>>    int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 7f59608..e7aa382 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -618,6 +618,8 @@ static int amdgpu_info_ioctl(struct drm_device *dev,
>> void *data, struct drm_file
>>>    			cap.flag |= AMDGPU_CAPABILITY_DIRECT_GMA_FLAG;
>>>    			cap.direct_gma_size = amdgpu_direct_gma_size;
>>>    		}
>>> +		cap.gc_double_offchip_lds_buf =
>>> +			adev->gfx.cap.gc_double_offchip_lds_buf;
>>>    		return copy_to_user(out, &cap,
>>>    				    min((size_t)size, sizeof(cap))) ? -EFAULT : 0;
>>>    	}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> index ce75d46..a1e221b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>>> @@ -1534,6 +1534,11 @@ static void gfx_v6_0_setup_spi(struct
>> amdgpu_device *adev,
>>>    	mutex_unlock(&adev->grbm_idx_mutex);
>>>    }
>>>
>>> +static void gfx_v6_0_cap_init(struct amdgpu_device *adev) {
>>> +	adev->gfx.cap.gc_double_offchip_lds_buf = 1; }
>>> +
>>>    static void gfx_v6_0_gpu_init(struct amdgpu_device *adev)
>>>    {
>>>    	u32 gb_addr_config = 0;
>>> @@ -1692,6 +1697,7 @@ static void gfx_v6_0_gpu_init(struct amdgpu_device
>> *adev)
>>>    		     adev->gfx.config.max_cu_per_sh);
>>>
>>>    	gfx_v6_0_get_cu_info(adev);
>>> +	gfx_v6_0_cap_init(adev);
>>>
>>>    	WREG32(mmCP_QUEUE_THRESHOLDS, ((0x16 <<
>> CP_QUEUE_THRESHOLDS__ROQ_IB1_START__SHIFT) |
>>>    				       (0x2b <<
>> CP_QUEUE_THRESHOLDS__ROQ_IB2_START__SHIFT)));
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> index aaf66fe..e1e97ae 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -1876,6 +1876,11 @@ static void gmc_v7_0_init_compute_vmid(struct
>> amdgpu_device *adev)
>>>    	mutex_unlock(&adev->srbm_mutex);
>>>    }
>>>
>>> +static void gfx_v7_0_cap_init(struct amdgpu_device *adev) {
>>> +	adev->gfx.cap.gc_double_offchip_lds_buf = 1; }
>>> +
>>>    /**
>>>     * gfx_v7_0_gpu_init - setup the 3D engine
>>>     *
>>> @@ -1900,6 +1905,7 @@ static void gfx_v7_0_gpu_init(struct
>>> amdgpu_device *adev)
>>>
>>>    	gfx_v7_0_setup_rb(adev);
>>>    	gfx_v7_0_get_cu_info(adev);
>>> +	gfx_v7_0_cap_init(adev);
>>>
>>>    	/* set HW defaults for 3D engine */
>>>    	WREG32(mmCP_MEQ_THRESHOLDS,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index ce05e38..934fc71 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -3826,6 +3826,19 @@ static void gfx_v8_0_init_compute_vmid(struct
>> amdgpu_device *adev)
>>>    	mutex_unlock(&adev->srbm_mutex);
>>>    }
>>>
>>> +static void gfx_v8_0_cap_init(struct amdgpu_device *adev) {
>>> +	switch (adev->asic_type) {
>>> +	default:
>>> +		adev->gfx.cap.gc_double_offchip_lds_buf = 1;
>>> +		break;
>>> +	case CHIP_CARRIZO:
>>> +	case CHIP_STONEY:
>>> +		adev->gfx.cap.gc_double_offchip_lds_buf = 0;
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>    static void gfx_v8_0_gpu_init(struct amdgpu_device *adev)
>>>    {
>>>    	u32 tmp, sh_static_mem_cfg;
>>> @@ -3839,6 +3852,7 @@ static void gfx_v8_0_gpu_init(struct amdgpu_device
>> *adev)
>>>    	gfx_v8_0_tiling_mode_table_init(adev);
>>>    	gfx_v8_0_setup_rb(adev);
>>>    	gfx_v8_0_get_cu_info(adev);
>>> +	gfx_v8_0_cap_init(adev);
>>>
>>>    	/* XXX SH_MEM regs */
>>>    	/* where to put LDS, scratch, GPUVM in FSA64 space */ diff --git
>>> a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h index
>>> 04daab3..7064fdc 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -853,6 +853,7 @@ struct drm_amdgpu_virtual_range {
>>>    struct drm_amdgpu_capability {
>>>    	__u32 flag;
>>>    	__u32 direct_gma_size;
>>> +	__u32 gc_double_offchip_lds_buf;
>>>    };
>>>
>>>    /*
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx