[PATCH 03/11] drm/amdgpu/gfx: add generic handling for disable_kq
Khatri, Sunil
sukhatri at amd.com
Thu Mar 6 09:57:33 UTC 2025
On 3/6/2025 6:36 AM, Felix Kuehling wrote:
>
> On 2025-03-05 15:47, Alex Deucher wrote:
>> Add proper checks for disable_kq functionality in
>> gfx helper functions. Add special logic for families
>> that require the clear state setup.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 92 +++++++++++++++++--------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 +
>> 2 files changed, 67 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index a194bf3347cbc..af3f8b62f6fd5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -371,6 +371,18 @@ int amdgpu_gfx_kiq_init(struct amdgpu_device *adev,
>> return 0;
>> }
>> +static bool amdgpu_gfx_disable_gfx_kq(struct amdgpu_device *adev)
>> +{
>> + if (adev->gfx.disable_kq) {
>> + /* GFX11 needs the GFX ring for clear buffer */
>> + if (amdgpu_ip_version(adev, GC_HWIP, 0) <= IP_VERSION(12, 0,
>> 0))
Yes the check has to be < as gfx12 do not need the clear buffer based
on our discussions.
Regards
Sunil
>
> Should this be < instead of <=?
>
> Regards,
> Felix
>
>> + return false;
>> + else
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> /* create MQD for each compute/gfx queue */
>> int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>> unsigned int mqd_size, int xcc_id)
>> @@ -379,6 +391,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device
>> *adev,
>> struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>> struct amdgpu_ring *ring = &kiq->ring;
>> u32 domain = AMDGPU_GEM_DOMAIN_GTT;
>> + bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
name of variable and function could be in sync. disable_gfx_kq and
amdgpu_gfx_disable_gfx_kq or change function name according to variable.
Also another suggestion here is better to have one more variable in the
gfx struct or ring and read this amdgpu_gfx_disable_gfx_kq once and use
it in all the places. It does looks confusing
so many similar sounding names.
Regards
Sunil
>> #if !defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
>> /* Only enable on gfx10 and 11 for now to avoid changing
>> behavior on older chips */
>> @@ -413,7 +426,8 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device
>> *adev,
>> }
>> }
>> - if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring) {
>> + if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring &&
>> + !disable_kq_gfx) {
>> /* create MQD for each KGQ */
>> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>> ring = &adev->gfx.gfx_ring[i];
>> @@ -437,25 +451,28 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device
>> *adev,
>> }
>> }
>> - /* create MQD for each KCQ */
>> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> - j = i + xcc_id * adev->gfx.num_compute_rings;
>> - ring = &adev->gfx.compute_ring[j];
>> - if (!ring->mqd_obj) {
>> - r = amdgpu_bo_create_kernel(adev, mqd_size, PAGE_SIZE,
>> - domain, &ring->mqd_obj,
>> - &ring->mqd_gpu_addr, &ring->mqd_ptr);
>> - if (r) {
>> - dev_warn(adev->dev, "failed to create ring mqd bo
>> (%d)", r);
>> - return r;
>> - }
>> + if (!adev->gfx.disable_kq) {
>
> Maybe just set adev->gfx.num_compute_rings to 0 somewhere, then you
> don't need this condition.
>
>
>> + /* create MQD for each KCQ */
>> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> + j = i + xcc_id * adev->gfx.num_compute_rings;
>> + ring = &adev->gfx.compute_ring[j];
>> + if (!ring->mqd_obj) {
>> + r = amdgpu_bo_create_kernel(adev, mqd_size, PAGE_SIZE,
>> + domain, &ring->mqd_obj,
>> + &ring->mqd_gpu_addr, &ring->mqd_ptr);
>> + if (r) {
>> + dev_warn(adev->dev, "failed to create ring mqd
>> bo (%d)", r);
>> + return r;
>> + }
>> - ring->mqd_size = mqd_size;
>> - /* prepare MQD backup */
>> - adev->gfx.mec.mqd_backup[j] = kzalloc(mqd_size,
>> GFP_KERNEL);
>> - if (!adev->gfx.mec.mqd_backup[j]) {
>> - dev_warn(adev->dev, "no memory to create MQD backup
>> for ring %s\n", ring->name);
>> - return -ENOMEM;
>> + ring->mqd_size = mqd_size;
>> + /* prepare MQD backup */
>> + adev->gfx.mec.mqd_backup[j] = kzalloc(mqd_size,
>> GFP_KERNEL);
>> + if (!adev->gfx.mec.mqd_backup[j]) {
>> + dev_warn(adev->dev, "no memory to create MQD
>> backup for ring %s\n",
>> + ring->name);
>> + return -ENOMEM;
>> + }
>> }
>> }
>> }
>> @@ -468,8 +485,10 @@ void amdgpu_gfx_mqd_sw_fini(struct amdgpu_device
>> *adev, int xcc_id)
>> struct amdgpu_ring *ring = NULL;
>> int i, j;
>> struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>> + bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>> - if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring) {
>> + if (adev->asic_type >= CHIP_NAVI10 && amdgpu_async_gfx_ring &&
>> + !disable_kq_gfx) {
>> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>> ring = &adev->gfx.gfx_ring[i];
>> kfree(adev->gfx.me.mqd_backup[i]);
>> @@ -479,13 +498,15 @@ void amdgpu_gfx_mqd_sw_fini(struct
>> amdgpu_device *adev, int xcc_id)
>> }
>> }
>> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> - j = i + xcc_id * adev->gfx.num_compute_rings;
>> - ring = &adev->gfx.compute_ring[j];
>> - kfree(adev->gfx.mec.mqd_backup[j]);
>> - amdgpu_bo_free_kernel(&ring->mqd_obj,
>> - &ring->mqd_gpu_addr,
>> - &ring->mqd_ptr);
>> + if (!adev->gfx.disable_kq) {
>
> Same as above.
>
>
>> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> + j = i + xcc_id * adev->gfx.num_compute_rings;
>> + ring = &adev->gfx.compute_ring[j];
>> + kfree(adev->gfx.mec.mqd_backup[j]);
>> + amdgpu_bo_free_kernel(&ring->mqd_obj,
>> + &ring->mqd_gpu_addr,
>> + &ring->mqd_ptr);
>> + }
>> }
>> ring = &kiq->ring;
>> @@ -502,6 +523,9 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device
>> *adev, int xcc_id)
>> int i, r = 0;
>> int j;
>> + if (adev->gfx.disable_kq)
>
> Same as above.
>
>
>> + return 0;
>> +
>> if (adev->enable_mes) {
>> for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> j = i + xcc_id * adev->gfx.num_compute_rings;
>> @@ -547,11 +571,15 @@ int amdgpu_gfx_disable_kcq(struct amdgpu_device
>> *adev, int xcc_id)
>> int amdgpu_gfx_disable_kgq(struct amdgpu_device *adev, int xcc_id)
>> {
>> + bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>> struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>> struct amdgpu_ring *kiq_ring = &kiq->ring;
>> int i, r = 0;
>> int j;
>> + if (disable_kq_gfx)
>> + return 0;
> Maybe just set adev->gfx.num_gfx_rings to 0 somewhere, then you don't
> need this condition.
>
> Regards,
> Felix
>
>
>> +
>> if (adev->enable_mes) {
>> if (amdgpu_gfx_is_master_xcc(adev, xcc_id)) {
>> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>> @@ -657,6 +685,9 @@ int amdgpu_gfx_enable_kcq(struct amdgpu_device
>> *adev, int xcc_id)
>> uint64_t queue_mask = 0;
>> int r, i, j;
>> + if (adev->gfx.disable_kq)
>> + return 0;
>> +
>> if (adev->mes.enable_legacy_queue_map)
>> return amdgpu_gfx_mes_enable_kcq(adev, xcc_id);
>> @@ -716,10 +747,14 @@ int amdgpu_gfx_enable_kcq(struct
>> amdgpu_device *adev, int xcc_id)
>> int amdgpu_gfx_enable_kgq(struct amdgpu_device *adev, int xcc_id)
>> {
>> + bool disable_kq_gfx = amdgpu_gfx_disable_gfx_kq(adev);
>> struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>> struct amdgpu_ring *kiq_ring = &kiq->ring;
>> int r, i, j;
>> + if (disable_kq_gfx)
>> + return 0;
>> +
>> if (!kiq->pmf || !kiq->pmf->kiq_map_queues)
>> return -EINVAL;
>> @@ -1544,6 +1579,9 @@ static ssize_t
>> amdgpu_gfx_set_run_cleaner_shader(struct device *dev,
>> if (adev->in_suspend && !adev->in_runpm)
>> return -EPERM;
>> + if (adev->gfx.disable_kq)
>> + return -ENOTSUPP;
>> +
>> ret = kstrtol(buf, 0, &value);
>> if (ret)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> index ddf4533614bac..8fa68a4ac34f1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> @@ -483,6 +483,8 @@ struct amdgpu_gfx {
>> atomic_t total_submission_cnt;
>> struct delayed_work idle_work;
>> +
>> + bool disable_kq;
>> };
>> struct amdgpu_gfx_ras_reg_entry {
More information about the amd-gfx
mailing list