Re: [Intel-xe] [PATCH 1/7] drm/xe: Add drm-client infrastructure

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8787 bytes --]


On 07-09-2023 14:27, Upadhyay, Tejas wrote:
>
> *From:*Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>
> *Sent:* Wednesday, September 6, 2023 9:45 AM
> *To:* Upadhyay, Tejas <tejas.upadhyay@intel.com>; 
> intel-xe@lists.freedesktop.org
> *Cc:* Iddamsetty, Aravind <aravind.iddamsetty@intel.com>
> *Subject:* Re: [PATCH 1/7] drm/xe: Add drm-client infrastructure
>
> Hi Tejas,
>
> On 31-08-2023 14:35, Tejas Upadhyay wrote:
>
>     Add drm-client infrastructure to record stats of consumption
>
>     done by individual drm client.
>
>     Signed-off-by: Tejas Upadhyay<tejas.upadhyay@intel.com>  <mailto:tejas.upadhyay@intel.com>
>
>     ---
>
>       drivers/gpu/drm/xe/Makefile          |  1 +
>
>       drivers/gpu/drm/xe/xe_device.c       | 15 +++++++-
>
>       drivers/gpu/drm/xe/xe_device_types.h |  6 ++++
>
>       drivers/gpu/drm/xe/xe_drm_client.c   | 52 ++++++++++++++++++++++++++++
>
>       drivers/gpu/drm/xe/xe_drm_client.h   | 43 +++++++++++++++++++++++
>
>       5 files changed, 116 insertions(+), 1 deletion(-)
>
>       create mode 100644 drivers/gpu/drm/xe/xe_drm_client.c
>
>       create mode 100644 drivers/gpu/drm/xe/xe_drm_client.h
>
>     diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>
>     index 9d2311f8141f..f9c25cb2f890 100644
>
>     --- a/drivers/gpu/drm/xe/Makefile
>
>     +++ b/drivers/gpu/drm/xe/Makefile
>
>     @@ -51,6 +51,7 @@ xe-y += xe_bb.o \
>
>              xe_device.o \
>
>              xe_device_sysfs.o \
>
>              xe_dma_buf.o \
>
>     +       xe_drm_client.o \
>
>              xe_exec.o \
>
>              xe_execlist.o \
>
>              xe_exec_queue.o \
>
>     diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>
>     index 109aeb25d19c..cf59c7b74eaf 100644
>
>     --- a/drivers/gpu/drm/xe/xe_device.c
>
>     +++ b/drivers/gpu/drm/xe/xe_device.c
>
>     @@ -18,6 +18,7 @@
>
>       #include "xe_debugfs.h"
>
>       #include "xe_display.h"
>
>       #include "xe_dma_buf.h"
>
>     +#include "xe_drm_client.h"
>
>       #include "xe_drv.h"
>
>       #include "xe_exec_queue.h"
>
>       #include "xe_exec.h"
>
>     @@ -43,13 +44,24 @@ struct lockdep_map xe_device_mem_access_lockdep_map = {
>
>       
>
>       static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>
>       {
>
>     +       struct xe_device *xe = to_xe_device(dev);
>
> Don't see xe_device being used in this patch. Better to make this 
> assignment in patch where the xe will be used to reference ttm 
> device.[Patch 7]
>
>     +       struct xe_drm_client *client;
>
>              struct xe_file *xef;
>
>     +       int ret = -ENOMEM;
>
>       
>
>              xef = kzalloc(sizeof(*xef), GFP_KERNEL);
>
>              if (!xef)
>
>     -               return -ENOMEM;
>
>     +               return ret;
>
>     +
>
>     +       client = xe_drm_client_alloc();
>
>     +       if (!client) {
>
>     +               kfree(xef);
>
>     +               return ret;
>
>     +       }
>
>       
>
>              xef->drm = file;
>
>     +       xef->client = client;
>
>     +       xef->xe = xe;
>
>       
>
>              mutex_init(&xef->vm.lock);
>
>              xa_init_flags(&xef->vm.xa, XA_FLAGS_ALLOC1);
>
>     @@ -89,6 +101,7 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>
>              xa_destroy(&xef->vm.xa);
>
>              mutex_destroy(&xef->vm.lock);
>
>       
>
>     +       xe_drm_client_put(xef->client);
>
>              kfree(xef);
>
>       }
>
>       
>
>     diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>
>     index 750e1f0d3339..d210a535c703 100644
>
>     --- a/drivers/gpu/drm/xe/xe_device_types.h
>
>     +++ b/drivers/gpu/drm/xe/xe_device_types.h
>
>     @@ -462,6 +462,9 @@ struct xe_device {
>
>        * struct xe_file - file handle for XE driver
>
>        */
>
>       struct xe_file {
>
>     +       /** @xe: xe DEVICE **/
>
>     +       struct xe_device *xe;
>
>     +
>
> same comment as above.
>
>              /** @drm: base DRM file */
>
>              struct drm_file *drm;
>
>       
>
>     @@ -480,6 +483,9 @@ struct xe_file {
>
>                      /** @lock: protects file engine state */
>
>                      struct mutex lock;
>
>              } exec_queue;
>
>     +
>
>     +       /** @client: drm client */
>
>     +       struct xe_drm_client *client;
>
>       };
>
>       
>
>       #endif
>
>     diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
>
>     new file mode 100644
>
>     index 000000000000..ea7993338076
>
>     --- /dev/null
>
>     +++ b/drivers/gpu/drm/xe/xe_drm_client.c
>
>     @@ -0,0 +1,52 @@
>
>     +// SPDX-License-Identifier: MIT
>
>     +/*
>
>     + * Copyright © 2023 Intel Corporation
>
>     + */
>
>     +
>
>     +#include <drm/drm_print.h>
>
>     +#include <linux/kernel.h>
>
>     +#include <linux/slab.h>
>
>     +#include <linux/types.h>
>
>     +
>
>     +#include "xe_device_types.h"
>
>     +#include "xe_drm_client.h"
>
>     +
>
>     +/**
>
>     + * xe_drm_client_alloc() - Allocate drm client
>
>     + * @void: No arg
>
>     + *
>
>     + * Allocate drm client struct to track client memory against
>
>     + * same till client life. Call this API whenever new client
>
>     + * has opened xe device.
>
>     + *
>
>     + * Return: pointer to client struct or NULL if cant allocate
>
>     + */
>
>     +struct xe_drm_client *xe_drm_client_alloc(void)
>
>     +{
>
>     +       struct xe_drm_client *client;
>
>     +
>
>     +       client = kzalloc(sizeof(*client), GFP_KERNEL);
>
>     +       if (!client)
>
>     +               return NULL;
>
>     +
>
>     +       kref_init(&client->kref);
>
>     +
>
>     +       return client;
>
>     +}
>
>     +
>
>     +/**
>
>     + * __xe_drm_client_free() - Free client struct
>
>     + * @kref: The reference
>
>     + *
>
>     + * This frees client struct. Call this API when xe device is closed
>
>     + * by drm client.
>
>     + *
>
>     + * Return: void
>
>     + */
>
>     +void __xe_drm_client_free(struct kref *kref)
>
>     +{
>
>     +       struct xe_drm_client *client =
>
>     +               container_of(kref, typeof(*client), kref);
>
>     +
>
>     +       kfree(client);
>
>     +}
>
>     diff --git a/drivers/gpu/drm/xe/xe_drm_client.h b/drivers/gpu/drm/xe/xe_drm_client.h
>
>     new file mode 100644
>
>     index 000000000000..be097cdf5d12
>
>     --- /dev/null
>
>     +++ b/drivers/gpu/drm/xe/xe_drm_client.h
>
>     @@ -0,0 +1,43 @@
>
>     +/* SPDX-License-Identifier: MIT */
>
>     +/*
>
>     + * Copyright © 2023 Intel Corporation
>
>     + */
>
>     +
>
>     +#ifndef _XE_DRM_CLIENT_H_
>
>     +#define _XE_DRM_CLIENT_H_
>
>     +
>
>     +#include <linux/kref.h>
>
>     +#include <linux/list.h>
>
>     +#include <linux/pid.h>
>
>     +#include <linux/rcupdate.h>
>
>     +#include <linux/sched.h>
>
>     +#include <linux/spinlock.h>
>
>     +
>
>     +struct drm_file;
>
>     +struct drm_printer;
>
>     +
>
>     +struct xe_drm_client {
>
>     +       struct kref kref;
>
>     +       unsigned int id;
>
>     +};
>
>     +
>
>     +       static inline struct xe_drm_client *
>
>     +xe_drm_client_get(struct xe_drm_client *client)
>
>     +{
>
>     +       kref_get(&client->kref);
>
>     +       return client;
>
>     +}
>
>     +
>
>     +void __xe_drm_client_free(struct kref *kref);
>
>     +
>
>     +static inline void xe_drm_client_put(struct xe_drm_client *client)
>
>     +{
>
>     +       kref_put(&client->kref, __xe_drm_client_free);
>
>     +}
>
>     +
>
>     +struct xe_drm_client *xe_drm_client_alloc(void);
>
>     +static inline struct xe_drm_client *
>
>     +xe_drm_client_get(struct xe_drm_client *client);
>
> kref_get/xe_drm_client_get can be moved to the patch where we are 
> introducing xe_drm_client_add_bo to maintain refcount.
>
> Sure. Let me check. But as we introduce infra related to client in 
> this patch. I thought of keeping all basic APIs and xe->xef as part of 
> this patch. Let me know if it does not make sense.
>
Hmm. Makes sense. RB from my end

Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

> Thanks,
>
> Tejas
>
> BR
>
> Himal
>
>     +static inline void xe_drm_client_put(struct xe_drm_client *client);
>
>     +
>
>     +#endif
>

[-- Attachment #2: Type: text/html, Size: 20269 bytes --]