dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: "Iddamsetty, Aravind" <aravind.iddamsetty@intel.com>
To: Tomer Tayar <ttayar@habana.ai>,
	"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	Oded Gabbay <ogabbay@kernel.org>
Subject: Re: [Intel-xe] [RFC 2/5] drm/xe/RAS: Register a genl netlink family
Date: Mon, 5 Jun 2023 22:51:51 +0530	[thread overview]
Message-ID: <cdfe8b1b-641b-029b-4dc5-170835cab011@intel.com> (raw)
In-Reply-To: <d22fbece-e6f3-cc6a-789b-bdbe99c70ced@habana.ai>



On 04-06-2023 22:39, Tomer Tayar wrote:
> On 26/05/2023 19:20, Aravind Iddamsetty wrote:
>> Use the generic netlink(genl) subsystem to expose the RAS counters to
>> userspace. We define a family structure and operations and register to
>> genl subsystem and these callbacks will be invoked by genl subsystem when
>> userspace sends a registered command with a family identifier to genl
>> subsystem.
>>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
>> ---
>>   drivers/gpu/drm/xe/Makefile          |  1 +
>>   drivers/gpu/drm/xe/xe_device.c       |  3 +
>>   drivers/gpu/drm/xe/xe_device_types.h |  2 +
>>   drivers/gpu/drm/xe/xe_module.c       |  2 +
>>   drivers/gpu/drm/xe/xe_netlink.c      | 89 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_netlink.h      | 14 +++++
>>   6 files changed, 111 insertions(+)
>>   create mode 100644 drivers/gpu/drm/xe/xe_netlink.c
>>   create mode 100644 drivers/gpu/drm/xe/xe_netlink.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index b84e191ba14f..2b42165bc824 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -67,6 +67,7 @@ xe-y += xe_bb.o \
>>   	xe_mmio.o \
>>   	xe_mocs.o \
>>   	xe_module.o \
>> +	xe_netlink.o \
>>   	xe_pat.o \
>>   	xe_pci.o \
>>   	xe_pcode.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 323356a44e7f..aa12ef12d9dc 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -24,6 +24,7 @@
>>   #include "xe_irq.h"
>>   #include "xe_mmio.h"
>>   #include "xe_module.h"
>> +#include "xe_netlink.h"
>>   #include "xe_pcode.h"
>>   #include "xe_pm.h"
>>   #include "xe_query.h"
>> @@ -317,6 +318,8 @@ int xe_device_probe(struct xe_device *xe)
>>   
>>   	xe_display_register(xe);
>>   
>> +	xe_genl_register(xe);
> 
> xe_genl_register() can fail

That is right but I didn't want to fail the driver load as it would not
impact any device functionality but doesn't provide observability. hence
a warning would be printed "xe genl family registration failed".
> 
>> +
>>   	xe_debugfs_register(xe);
>>   
>>   	err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 682ebdd1c09e..c9612a54c48f 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -10,6 +10,7 @@
>>   
>>   #include <drm/drm_device.h>
>>   #include <drm/drm_file.h>
>> +#include <drm/drm_netlink.h>
>>   #include <drm/ttm/ttm_device.h>
>>   
>>   #include "xe_gt_types.h"
>> @@ -347,6 +348,7 @@ struct xe_device {
>>   		u32 lvds_channel_mode;
>>   	} params;
>>   #endif
>> +	struct genl_family xe_genl_family;
> 
> Should it be added above, before the "private" section?
> Maybe add a kernel-doc comment for it?

thanks for pointing out will move it there.

> 
>>   };
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
>> index 6860586ce7f8..1eb73eb9a9a5 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -11,6 +11,7 @@
>>   #include "xe_drv.h"
>>   #include "xe_hw_fence.h"
>>   #include "xe_module.h"
>> +#include "xe_netlink.h"
>>   #include "xe_pci.h"
>>   #include "xe_sched_job.h"
>>   
>> @@ -67,6 +68,7 @@ static void __exit xe_exit(void)
>>   {
>>   	int i;
>>   
>> +	xe_genl_cleanup();
>>   	xe_unregister_pci_driver();
>>   
>>   	for (i = ARRAY_SIZE(init_funcs) - 1; i >= 0; i--)
>> diff --git a/drivers/gpu/drm/xe/xe_netlink.c b/drivers/gpu/drm/xe/xe_netlink.c
>> new file mode 100644
>> index 000000000000..63ef238ebc27
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_netlink.c
>> @@ -0,0 +1,89 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_managed.h>
>> +
>> +#include "xe_device.h"
>> +
>> +DEFINE_XARRAY(xe_xarray);
> 
> xe_array sounds too generic. Maybe it should be more specific like 
> xe_genl_xarray?
> In addition, it should be probably static.

Ok.

Thanks,
Aravind.
> 
> Thanks,
> Tomer
> 
>> +
>> +static int xe_genl_list_errors(struct sk_buff *msg, struct genl_info *info)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int xe_genl_read_error(struct sk_buff *msg, struct genl_info *info)
>> +{
>> +	return 0;
>> +}
>> +
>> +/* operations definition */
>> +static const struct genl_ops xe_genl_ops[] = {
>> +	{
>> +		.cmd = DRM_CMD_QUERY,
>> +		.doit = xe_genl_list_errors,
>> +		.policy = drm_attr_policy_query,
>> +	},
>> +	{
>> +		.cmd = DRM_CMD_READ_ONE,
>> +		.doit = xe_genl_read_error,
>> +		.policy = drm_attr_policy_read_one,
>> +	},
>> +	{
>> +		.cmd = DRM_CMD_READ_ALL,
>> +		.doit = xe_genl_list_errors,
>> +		.policy = drm_attr_policy_query,
>> +	},
>> +};
>> +
>> +static void xe_genl_deregister(struct drm_device *dev,  void *arg)
>> +{
>> +	struct xe_device *xe = arg;
>> +
>> +	xa_erase(&xe_xarray, xe->xe_genl_family.id);
>> +
>> +	drm_dbg_driver(&xe->drm, "unregistering genl family %s\n", xe->xe_genl_family.name);
>> +
>> +	genl_unregister_family(&xe->xe_genl_family);
>> +}
>> +
>> +static void xe_genl_family_init(struct xe_device *xe)
>> +{
>> +	/* Use drm primary node name eg: card0 to name the genl family */
>> +	snprintf(xe->xe_genl_family.name, sizeof(xe->xe_genl_family.name), "%s", xe->drm.primary->kdev->kobj.name);
>> +	xe->xe_genl_family.version = DRM_GENL_VERSION;
>> +	xe->xe_genl_family.parallel_ops = true;
>> +	xe->xe_genl_family.ops = xe_genl_ops;
>> +	xe->xe_genl_family.n_ops = ARRAY_SIZE(xe_genl_ops);
>> +	xe->xe_genl_family.maxattr = DRM_ATTR_MAX;
>> +	xe->xe_genl_family.module = THIS_MODULE;
>> +}
>> +
>> +int xe_genl_register(struct xe_device *xe)
>> +{
>> +	int ret;
>> +
>> +	xe_genl_family_init(xe);
>> +
>> +	ret = genl_register_family(&xe->xe_genl_family);
>> +	if (ret < 0) {
>> +		drm_warn(&xe->drm, "xe genl family registration failed\n");
>> +		return ret;
>> +	}
>> +
>> +	drm_dbg_driver(&xe->drm, "genl family id %d and name %s\n", xe->xe_genl_family.id, xe->xe_genl_family.name);
>> +
>> +	xa_store(&xe_xarray, xe->xe_genl_family.id, xe, GFP_KERNEL);
>> +
>> +	ret = drmm_add_action_or_reset(&xe->drm, xe_genl_deregister, xe);
>> +
>> +	return ret;
>> +}
>> +
>> +void xe_genl_cleanup(void)
>> +{
>> +	/* destroy xarray */
>> +	xa_destroy(&xe_xarray);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_netlink.h b/drivers/gpu/drm/xe/xe_netlink.h
>> new file mode 100644
>> index 000000000000..3bbddb620539
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_netlink.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2021 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_GENL_H_
>> +#define _XE_GENL_H_
>> +
>> +#include "xe_device.h"
>> +
>> +int xe_genl_register(struct xe_device *xe);
>> +void xe_genl_cleanup(void);
>> +
>> +#endif
> 
> 

  reply	other threads:[~2023-06-05 17:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-26 16:20 [RFC 0/5] Proposal to use netlink for RAS and Telemetry across drm subsystem Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 1/5] drm/netlink: Add netlink infrastructure Aravind Iddamsetty
2023-06-04 17:07   ` [Intel-xe] " Tomer Tayar
2023-06-05 17:18     ` Iddamsetty, Aravind
2023-06-06 14:04       ` Tomer Tayar
2023-06-21  6:40         ` Iddamsetty, Aravind
2023-05-26 16:20 ` [RFC 2/5] drm/xe/RAS: Register a genl netlink family Aravind Iddamsetty
2023-06-04 17:09   ` [Intel-xe] " Tomer Tayar
2023-06-05 17:21     ` Iddamsetty, Aravind [this message]
2023-05-26 16:20 ` [RFC 3/5] drm/xe/RAS: Expose the error counters Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 4/5] drm/netlink: define multicast groups Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 5/5] drm/xe/RAS: send multicast event on occurrence of an error Aravind Iddamsetty
2023-06-04 17:07 ` [Intel-xe] [RFC 0/5] Proposal to use netlink for RAS and Telemetry across drm subsystem Tomer Tayar
2023-06-05 17:17   ` Iddamsetty, Aravind
2023-06-05 16:47 ` Alex Deucher
2023-06-06 11:56   ` Iddamsetty, Aravind
2023-06-21 17:24 ` Sebastian Wick
2023-07-17 12:02   ` Oded Gabbay

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cdfe8b1b-641b-029b-4dc5-170835cab011@intel.com \
    --to=aravind.iddamsetty@intel.com \
    --cc=alexander.deucher@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=ogabbay@kernel.org \
    --cc=ttayar@habana.ai \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).