Re: [PATCH 03/11] drm/amdgpu/gfx: add generic handling for disable_kq

["Khatri, Sunil" <sukhatri@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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 {