Re: [PATCH 2/2] drm/amdkfd: Eliminate ops_asic_specific in kernel queue

[Yong Zhao <yong.zhao@xxxxxxx>]

I will change it to CHIP_MULLINS.

Yes , I also spotted the kq->ops cleanup, will send it out shortly.

Regards,

Yong

On 2019-11-13 2:31 p.m., Felix Kuehling wrote:
> See one comment inline. With that fixed, the series is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
>
> I could think of more follow-up cleanup while you're at it:
>
> 1. Can you see any reason why the kq->ops need to be function pointers.
>    Looks to me like they are the same for all kernel queues, so those
>    functions could be called without the pointer indirection.
> 2. The only think left in the ASIC-specific kfd_kernel_queue_*.c files
>    is the PM4 packet writer functions that are called by the
>    kfd_packet_manager. It may make sense to rename them to reflect
>    that. Maybe kfd_packet_manager_*.c
>
> Regards,
>   Felix
>
> On 2019-11-12 5:18 p.m., Yong Zhao wrote:
> > The ops_asic_specific function pointers are actually quite generic after
> > using a simple if condition. Eliminate it by code refactoring.
> >
> > Change-Id: Icb891289cca31acdbe2d2eea76a426f1738b9c08
> > Signed-off-by: Yong Zhao <Yong.Zhao@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 63 ++++++++-----------
> >   drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h |  4 --
> >   .../gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c  | 36 -----------
> >   .../gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c  | 48 --------------
> >   4 files changed, 26 insertions(+), 125 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> > index a750b1d110eb..59ee9053498c 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> > @@ -87,9 +87,17 @@ static bool initialize(struct kernel_queue *kq, 
> > struct kfd_dev *dev,
> >       kq->pq_kernel_addr = kq->pq->cpu_ptr;
> >       kq->pq_gpu_addr = kq->pq->gpu_addr;
> >   -    retval = kq->ops_asic_specific.initialize(kq, dev, type, 
> > queue_size);
> > -    if (!retval)
> > -        goto err_eop_allocate_vidmem;
> > +    /* For CIK family asics, kq->eop_mem is not needed */
> > +    if (dev->device_info->asic_family > CHIP_HAWAII) {
>
> This is not the correct condition to distinguish GFXv7 (CIK) vs v8 
> (VI). CHIP_MULLINS comes after Hawaii, but it is also GFXv7 (CIK), 
> even though KFD current doesn't support it.
>
>
> > +        retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
> > +        if (retval != 0)
> > +            goto err_eop_allocate_vidmem;
> > +
> > +        kq->eop_gpu_addr = kq->eop_mem->gpu_addr;
> > +        kq->eop_kernel_addr = kq->eop_mem->cpu_ptr;
> > +
> > +        memset(kq->eop_kernel_addr, 0, PAGE_SIZE);
> > +    }
> >         retval = kfd_gtt_sa_allocate(dev, sizeof(*kq->rptr_kernel),
> >                       &kq->rptr_mem);
> > @@ -200,7 +208,12 @@ static void uninitialize(struct kernel_queue *kq)
> >         kfd_gtt_sa_free(kq->dev, kq->rptr_mem);
> >       kfd_gtt_sa_free(kq->dev, kq->wptr_mem);
> > -    kq->ops_asic_specific.uninitialize(kq);
> > +
> > +    /* For CIK family asics, kq->eop_mem is Null, kfd_gtt_sa_free()
> > +     * is able to handle NULL properly.
> > +     */
> > +    kfd_gtt_sa_free(kq->dev, kq->eop_mem);
> > +
> >       kfd_gtt_sa_free(kq->dev, kq->pq);
> >       kfd_release_kernel_doorbell(kq->dev,
> >                       kq->queue->properties.doorbell_ptr);
> > @@ -280,8 +293,15 @@ static void submit_packet(struct kernel_queue *kq)
> >       }
> >       pr_debug("\n");
> >   #endif
> > -
> > -    kq->ops_asic_specific.submit_packet(kq);
> > +    if (kq->dev->device_info->doorbell_size == 8) {
> > +        *kq->wptr64_kernel = kq->pending_wptr64;
> > + write_kernel_doorbell64(kq->queue->properties.doorbell_ptr,
> > +                    kq->pending_wptr64);
> > +    } else {
> > +        *kq->wptr_kernel = kq->pending_wptr;
> > + write_kernel_doorbell(kq->queue->properties.doorbell_ptr,
> > +                    kq->pending_wptr);
> > +    }
> >   }
> >     static void rollback_packet(struct kernel_queue *kq)
> > @@ -310,42 +330,11 @@ struct kernel_queue *kernel_queue_init(struct 
> > kfd_dev *dev,
> >       kq->ops.submit_packet = submit_packet;
> >       kq->ops.rollback_packet = rollback_packet;
> >   -    switch (dev->device_info->asic_family) {
> > -    case CHIP_KAVERI:
> > -    case CHIP_HAWAII:
> > -    case CHIP_CARRIZO:
> > -    case CHIP_TONGA:
> > -    case CHIP_FIJI:
> > -    case CHIP_POLARIS10:
> > -    case CHIP_POLARIS11:
> > -    case CHIP_POLARIS12:
> > -    case CHIP_VEGAM:
> > -        kernel_queue_init_vi(&kq->ops_asic_specific);
> > -        break;
> > -
> > -    case CHIP_VEGA10:
> > -    case CHIP_VEGA12:
> > -    case CHIP_VEGA20:
> > -    case CHIP_RAVEN:
> > -    case CHIP_RENOIR:
> > -    case CHIP_ARCTURUS:
> > -    case CHIP_NAVI10:
> > -    case CHIP_NAVI12:
> > -    case CHIP_NAVI14:
> > -        kernel_queue_init_v9(&kq->ops_asic_specific);
> > -        break;
> > -    default:
> > -        WARN(1, "Unexpected ASIC family %u",
> > -             dev->device_info->asic_family);
> > -        goto out_free;
> > -    }
> > -
> >       if (kq->ops.initialize(kq, dev, type, KFD_KERNEL_QUEUE_SIZE))
> >           return kq;
> >         pr_err("Failed to init kernel queue\n");
> >   -out_free:
> >       kfree(kq);
> >       return NULL;
> >   }
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
> > index a9a35897d8b7..475e9499c0af 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.h
> > @@ -66,7 +66,6 @@ struct kernel_queue_ops {
> >     struct kernel_queue {
> >       struct kernel_queue_ops ops;
> > -    struct kernel_queue_ops ops_asic_specific;
> >         /* data */
> >       struct kfd_dev        *dev;
> > @@ -99,7 +98,4 @@ struct kernel_queue {
> >       struct list_head    list;
> >   };
> >   -void kernel_queue_init_vi(struct kernel_queue_ops *ops);
> > -void kernel_queue_init_v9(struct kernel_queue_ops *ops);
> > -
> >   #endif /* KFD_KERNEL_QUEUE_H_ */
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> > index 9e0eaf446bab..2de01009f1b6 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> > @@ -27,42 +27,6 @@
> >   #include "kfd_pm4_opcodes.h"
> >   #include "gc/gc_10_1_0_sh_mask.h"
> >   -static bool initialize_v9(struct kernel_queue *kq, struct kfd_dev 
> > *dev,
> > -            enum kfd_queue_type type, unsigned int queue_size)
> > -{
> > -    int retval;
> > -
> > -    retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
> > -    if (retval)
> > -        return false;
> > -
> > -    kq->eop_gpu_addr = kq->eop_mem->gpu_addr;
> > -    kq->eop_kernel_addr = kq->eop_mem->cpu_ptr;
> > -
> > -    memset(kq->eop_kernel_addr, 0, PAGE_SIZE);
> > -
> > -    return true;
> > -}
> > -
> > -static void uninitialize_v9(struct kernel_queue *kq)
> > -{
> > -    kfd_gtt_sa_free(kq->dev, kq->eop_mem);
> > -}
> > -
> > -static void submit_packet_v9(struct kernel_queue *kq)
> > -{
> > -    *kq->wptr64_kernel = kq->pending_wptr64;
> > - write_kernel_doorbell64(kq->queue->properties.doorbell_ptr,
> > -                kq->pending_wptr64);
> > -}
> > -
> > -void kernel_queue_init_v9(struct kernel_queue_ops *ops)
> > -{
> > -    ops->initialize = initialize_v9;
> > -    ops->uninitialize = uninitialize_v9;
> > -    ops->submit_packet = submit_packet_v9;
> > -}
> > -
> >   static int pm_map_process_v9(struct packet_manager *pm,
> >           uint32_t *buffer, struct qcm_process_device *qpd)
> >   {
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c 
> > b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
> > index 64f13f34d819..bed4d0ccb6b1 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_vi.c
> > @@ -26,54 +26,6 @@
> >   #include "kfd_pm4_headers_vi.h"
> >   #include "kfd_pm4_opcodes.h"
> >   -static bool initialize_vi(struct kernel_queue *kq, struct kfd_dev 
> > *dev,
> > -            enum kfd_queue_type type, unsigned int queue_size);
> > -static void uninitialize_vi(struct kernel_queue *kq);
> > -static void submit_packet_vi(struct kernel_queue *kq);
> > -
> > -void kernel_queue_init_vi(struct kernel_queue_ops *ops)
> > -{
> > -    ops->initialize = initialize_vi;
> > -    ops->uninitialize = uninitialize_vi;
> > -    ops->submit_packet = submit_packet_vi;
> > -}
> > -
> > -static bool initialize_vi(struct kernel_queue *kq, struct kfd_dev *dev,
> > -            enum kfd_queue_type type, unsigned int queue_size)
> > -{
> > -    int retval;
> > -
> > -    /*For CIK family asics, kq->eop_mem is not needed */
> > -    if (dev->device_info->asic_family <= CHIP_HAWAII)
> > -        return true;
> > -
> > -    retval = kfd_gtt_sa_allocate(dev, PAGE_SIZE, &kq->eop_mem);
> > -    if (retval != 0)
> > -        return false;
> > -
> > -    kq->eop_gpu_addr = kq->eop_mem->gpu_addr;
> > -    kq->eop_kernel_addr = kq->eop_mem->cpu_ptr;
> > -
> > -    memset(kq->eop_kernel_addr, 0, PAGE_SIZE);
> > -
> > -    return true;
> > -}
> > -
> > -static void uninitialize_vi(struct kernel_queue *kq)
> > -{
> > -    /* For CIK family asics, kq->eop_mem is Null, kfd_gtt_sa_free()
> > -     * is able to handle NULL properly.
> > -     */
> > -    kfd_gtt_sa_free(kq->dev, kq->eop_mem);
> > -}
> > -
> > -static void submit_packet_vi(struct kernel_queue *kq)
> > -{
> > -    *kq->wptr_kernel = kq->pending_wptr;
> > - write_kernel_doorbell(kq->queue->properties.doorbell_ptr,
> > -                kq->pending_wptr);
> > -}
> > -
> >   unsigned int pm_build_pm4_header(unsigned int opcode, size_t 
> > packet_size)
> >   {
> >       union PM4_MES_TYPE_3_HEADER header;
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx