[Intel-xe] [RFC v4 1/5] drm/netlink: Add netlink infrastructure

Tomer Tayar ttayar at habana.ai
Thu Nov 23 07:26:48 UTC 2023


On 22/11/2023 16:32, Aravind Iddamsetty wrote:
> On 11/10/23 17:54, Tomer Tayar wrote:
>> On 20/10/2023 18:58, Aravind Iddamsetty wrote:
>>> Define the netlink registration interface and commands, attributes that
>>> can be commonly used across by drm drivers. This patch intends to use
>>> the generic netlink family to expose various stats of device. At present
>>> it defines some commands that shall be used to expose RAS error counters.
>>>
>>> v2:
>>> define common interfaces to genl netlink subsystem that all drm drivers
>>> can leverage.(Tomer Tayar)
>>>
>>> v3: drop DRIVER_NETLINK flag and use the driver_genl_ops structure to
>>> register to netlink subsystem (Daniel Vetter)
>>>
>>> v4:(Michael J. Ruhl)
>>> 1. rename drm_genl_send to drm_genl_reply
>>> 2. catch error from xa_store and handle appropriately
>>>
>>> Cc: Tomer Tayar<ttayar at habana.ai>
>>> Cc: Daniel Vetter<daniel at ffwll.ch>
>>> Cc: Michael J. Ruhl<michael.j.ruhl at intel.com>
>>>
>>> Signed-off-by: Aravind Iddamsetty<aravind.iddamsetty at linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/Makefile       |   1 +
>>>    drivers/gpu/drm/drm_drv.c      |   7 ++
>>>    drivers/gpu/drm/drm_netlink.c  | 188 +++++++++++++++++++++++++++++++++
>>>    include/drm/drm_device.h       |   8 ++
>>>    include/drm/drm_drv.h          |   7 ++
>>>    include/drm/drm_netlink.h      |  30 ++++++
>>>    include/uapi/drm/drm_netlink.h |  83 +++++++++++++++
>>>    7 files changed, 324 insertions(+)
>>>    create mode 100644 drivers/gpu/drm/drm_netlink.c
>>>    create mode 100644 include/drm/drm_netlink.h
>>>    create mode 100644 include/uapi/drm/drm_netlink.h
>>>
>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>> index ee64c51274ad..60864369adaa 100644
>>> --- a/drivers/gpu/drm/Makefile
>>> +++ b/drivers/gpu/drm/Makefile
>>> @@ -35,6 +35,7 @@ drm-y := \
>>>    	drm_mode_object.o \
>>>    	drm_modes.o \
>>>    	drm_modeset_lock.o \
>>> +	drm_netlink.o \
>>>    	drm_plane.o \
>>>    	drm_prime.o \
>>>    	drm_print.o \
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 535f16e7882e..31f55c1c7524 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -937,6 +937,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>>>    	if (ret)
>>>    		goto err_minors;
>>>    
>>> +	if (driver->genl_ops) {
>>> +		ret = drm_genl_register(dev);
>>> +		if (ret)
>>> +			goto err_minors;
>>> +	}
>>> +
>>>    	ret = create_compat_control_link(dev);
>>>    	if (ret)
>>>    		goto err_minors;
>>> @@ -1074,6 +1080,7 @@ static void drm_core_exit(void)
>>>    {
>>>    	drm_privacy_screen_lookup_exit();
>>>    	accel_core_exit();
>>> +	drm_genl_exit();
>>>    	unregister_chrdev(DRM_MAJOR, "drm");
>>>    	debugfs_remove(drm_debugfs_root);
>>>    	drm_sysfs_destroy();
>>> diff --git a/drivers/gpu/drm/drm_netlink.c b/drivers/gpu/drm/drm_netlink.c
>>> new file mode 100644
>>> index 000000000000..8add249c1da3
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/drm_netlink.c
>>> @@ -0,0 +1,188 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#include <drm/drm_device.h>
>>> +#include <drm/drm_drv.h>
>>> +#include <drm/drm_file.h>
>>> +#include <drm/drm_managed.h>
>>> +#include <drm/drm_netlink.h>
>>> +#include <drm/drm_print.h>
>>> +
>>> +DEFINE_XARRAY(drm_dev_xarray);
>>> +
>>> +/**
>>> + * drm_genl_reply - response to a request
>>> + * @msg: socket buffer
>>> + * @info: receiver information
>>> + * @usrhdr: pointer to user specific header in the message buffer
>>> + *
>>> + * RETURNS:
>>> + * 0 on success and negative error code on failure
>>> + */
>>> +int drm_genl_reply(struct sk_buff *msg, struct genl_info *info, void *usrhdr)
>>> +{
>>> +	int ret;
>>> +
>>> +	genlmsg_end(msg, usrhdr);
>>> +
>>> +	ret = genlmsg_reply(msg, info);
>>> +	if (ret)
>>> +		nlmsg_free(msg);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(drm_genl_reply);
>>> +
>>> +/**
>>> + * drm_genl_alloc_msg - allocate genl message buffer
>>> + * @dev: drm_device for which the message is being allocated
>>> + * @info: receiver information
>> a description for msg_size is missing
> Thanks for catching it will add.
>>> + * @usrhdr: pointer to user specific header in the message buffer
>>> + *
>>> + * RETURNS:
>>> + * pointer to new allocated buffer on success, NULL on failure
>>> + */
>>> +struct sk_buff *
>>> +drm_genl_alloc_msg(struct drm_device *dev,
>>> +		   struct genl_info *info,
>>> +		   size_t msg_size, void **usrhdr)
>>> +{
>>> +	struct sk_buff *new_msg;
>>> +
>>> +	new_msg = genlmsg_new(msg_size, GFP_KERNEL);
>>> +	if (!new_msg)
>>> +		return new_msg;
>>> +
>>> +	*usrhdr = genlmsg_put_reply(new_msg, info, &dev->drm_genl_family, 0, info->genlhdr->cmd);
>>> +	if (!*usrhdr) {
>>> +		nlmsg_free(new_msg);
>>> +		new_msg = NULL;
>>> +	}
>>> +
>>> +	return new_msg;
>>> +}
>>> +EXPORT_SYMBOL(drm_genl_alloc_msg);
>>> +
>>> +static struct drm_device *genl_to_dev(struct genl_info *info)
>>> +{
>>> +	return xa_load(&drm_dev_xarray, info->nlhdr->nlmsg_type);
>>> +}
>>> +
>>> +static int drm_genl_list_errors(struct sk_buff *msg, struct genl_info *info)
>>> +{
>>> +	struct drm_device *dev = genl_to_dev(info);
>>> +
>>> +	if (GENL_REQ_ATTR_CHECK(info, DRM_RAS_ATTR_REQUEST))
>>> +		return -EINVAL;
>>> +
>>> +	if (WARN_ON(!dev->driver->genl_ops[info->genlhdr->cmd].doit))
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	return dev->driver->genl_ops[info->genlhdr->cmd].doit(dev, msg, info);
>>> +}
>>> +
>>> +static int drm_genl_read_error(struct sk_buff *msg, struct genl_info *info)
>>> +{
>>> +	struct drm_device *dev = genl_to_dev(info);
>>> +
>>> +	if (GENL_REQ_ATTR_CHECK(info, DRM_RAS_ATTR_ERROR_ID))
>>> +		return -EINVAL;
>>> +
>>> +	if (WARN_ON(!dev->driver->genl_ops[info->genlhdr->cmd].doit))
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	return dev->driver->genl_ops[info->genlhdr->cmd].doit(dev, msg, info);
>>> +}
>>> +
>>> +/* attribute policies */
>>> +static const struct nla_policy drm_attr_policy_query[DRM_ATTR_MAX + 1] = {
>>> +	[DRM_RAS_ATTR_REQUEST] = { .type = NLA_U8 },
>>> +};
>>> +
>>> +static const struct nla_policy drm_attr_policy_read_one[DRM_ATTR_MAX + 1] = {
>>> +	[DRM_RAS_ATTR_ERROR_ID] = { .type = NLA_U64 },
>>> +};
>>> +
>>> +/* drm genl operations definition */
>>> +const struct genl_ops drm_genl_ops[] = {
>>> +	{
>>> +		.cmd = DRM_RAS_CMD_QUERY,
>>> +		.doit = drm_genl_list_errors,
>>> +		.policy = drm_attr_policy_query,
>>> +	},
>>> +	{
>>> +		.cmd = DRM_RAS_CMD_READ_ONE,
>>> +		.doit = drm_genl_read_error,
>>> +		.policy = drm_attr_policy_read_one,
>>> +	},
>>> +	{
>>> +		.cmd = DRM_RAS_CMD_READ_ALL,
>>> +		.doit = drm_genl_list_errors,
>>> +		.policy = drm_attr_policy_query,
>>> +	},
>>> +};
>>> +
>>> +static void drm_genl_family_init(struct drm_device *dev)
>>> +{
>>> +	/* Use drm primary node name eg: card0 to name the genl family */
>>> +	snprintf(dev->drm_genl_family.name, sizeof(dev->drm_genl_family.name), "%s", dev->primary->kdev->kobj.name);
>> dev_name() can be used.
>> Also, what about accel? Maybe check dev->primary and use primary/accel
>> accordingly?
> the present series is adding this feature for primary device only and has
> no knowledge how it will be used for accel device, so when accel device
> start using this infra should make that particular change or do you think
> it should be added as part of this series only?

I think that accel is considered a part of the drm subsystem, so we can 
refer to all minor types when adding a general drm feature.
But I understand your argument and if you prefer to postpone it until it 
is used for some accel device then no problem.

Thanks,
Tomer

>
>>> +	dev->drm_genl_family.version = DRM_GENL_VERSION;
>>> +	dev->drm_genl_family.parallel_ops = true;
>>> +	dev->drm_genl_family.ops = drm_genl_ops;
>>> +	dev->drm_genl_family.n_ops = ARRAY_SIZE(drm_genl_ops);
>>> +	dev->drm_genl_family.maxattr = DRM_ATTR_MAX;
>>> +	dev->drm_genl_family.module = dev->dev->driver->owner;
>>> +}
>>> +
>>> +static void drm_genl_deregister(struct drm_device *dev,  void *arg)
>> Redundant space before "void *arg"
> will clean it.
>>> +{
>>> +	drm_dbg_driver(dev, "unregistering genl family %s\n", dev->drm_genl_family.name);
>>> +
>>> +	xa_erase(&drm_dev_xarray, dev->drm_genl_family.id);
>>> +
>>> +	genl_unregister_family(&dev->drm_genl_family);
>>> +}
>>> +
>>> +/**
>>> + * drm_genl_register - Register genl family
>>> + * @dev: drm_device for which genl family needs to be registered
>>> + *
>>> + * RETURNS:
>>> + * 0 on success and negative error code on failure
>>> + */
>>> +int drm_genl_register(struct drm_device *dev)
>>> +{
>>> +	int ret;
>>> +
>>> +	drm_genl_family_init(dev);
>>> +
>>> +	ret = genl_register_family(&dev->drm_genl_family);
>>> +	if (ret < 0) {
>>> +		drm_warn(dev, "genl family registration failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	drm_dbg_driver(dev, "genl family id %d and name %s\n", dev->drm_genl_family.id, dev->drm_genl_family.name);
>>> +
>>> +	ret = xa_err(xa_store(&drm_dev_xarray, dev->drm_genl_family.id, dev, GFP_KERNEL));
>>> +	if (ret)
>>> +		goto genl_unregister;
>>> +
>>> +	ret = drmm_add_action_or_reset(dev, drm_genl_deregister, NULL);
>>> +
>>> +	return ret;
>>> +
>>> +genl_unregister:
>>> +	genl_unregister_family(&dev->drm_genl_family);
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * drm_genl_exit: destroy drm_dev_xarray
>>> + */
>>> +void drm_genl_exit(void)
>>> +{
>>> +	xa_destroy(&drm_dev_xarray);
>>> +}
>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>> index c490977ee250..d3ae91b7714d 100644
>>> --- a/include/drm/drm_device.h
>>> +++ b/include/drm/drm_device.h
>>> @@ -8,6 +8,7 @@
>>>    
>>>    #include <drm/drm_legacy.h>
>>>    #include <drm/drm_mode_config.h>
>>> +#include <drm/drm_netlink.h>
>>>    
>>>    struct drm_driver;
>>>    struct drm_minor;
>>> @@ -318,6 +319,13 @@ struct drm_device {
>>>    	 */
>>>    	struct dentry *debugfs_root;
>>>    
>>> +	/**
>>> +	 * @drm_genl_family:
>>> +	 *
>>> +	 * Generic netlink family registration structure.
>>> +	 */
>>> +	struct genl_family drm_genl_family;
>>> +
>>>    	/* Everything below here is for legacy driver, never use! */
>>>    	/* private: */
>>>    #if IS_ENABLED(CONFIG_DRM_LEGACY)
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index e2640dc64e08..ebdb7850d235 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -434,6 +434,13 @@ struct drm_driver {
>>>    	 */
>>>    	const struct file_operations *fops;
>>>    
>>> +	/**
>>> +	 * @genl_ops:
>>> +	 *
>>> +	 * Drivers private callback to genl commands
>>> +	 */
>>> +	const struct driver_genl_ops *genl_ops;
>>> +
>>>    #ifdef CONFIG_DRM_LEGACY
>>>    	/* Everything below here is for legacy driver, never use! */
>>>    	/* private: */
>>> diff --git a/include/drm/drm_netlink.h b/include/drm/drm_netlink.h
>>> new file mode 100644
>>> index 000000000000..54527dae7847
>>> --- /dev/null
>>> +++ b/include/drm/drm_netlink.h
>>> @@ -0,0 +1,30 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __DRM_NETLINK_H__
>>> +#define __DRM_NETLINK_H__
>>> +
>>> +#include <linux/netdevice.h>
>>> +#include <net/genetlink.h>
>>> +#include <net/sock.h>
>>> +#include <uapi/drm/drm_netlink.h>
>>> +
>>> +struct drm_device;
>>> +
>>> +struct driver_genl_ops {
>>> +	int		       (*doit)(struct drm_device *dev,
>>> +				       struct sk_buff *skb,
>> The skb parameter is currently not used (both xe_genl_list_errors() and
>> xe_genl_read_error() allocate a new skb).
>> Did you add because it might be needed for future ops?
> well I wanted to pass on the details the netlink subsystem sends and leave it to the driver
> if it wants to use it anyway.
>>> +				       struct genl_info *info);
>>> +};
>>> +
>>> +int drm_genl_register(struct drm_device *dev);
>>> +void drm_genl_exit(void);
>>> +int drm_genl_reply(struct sk_buff *msg, struct genl_info *info, void *usrhdr);
>>> +struct sk_buff *
>>> +drm_genl_alloc_msg(struct drm_device *dev,
>>> +		   struct genl_info *info,
>>> +		   size_t msg_size, void **usrhdr);
>>> +#endif
>>> +
>>> diff --git a/include/uapi/drm/drm_netlink.h b/include/uapi/drm/drm_netlink.h
>>> new file mode 100644
>>> index 000000000000..aab42147a20e
>>> --- /dev/null
>>> +++ b/include/uapi/drm/drm_netlink.h
>>> @@ -0,0 +1,83 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright 2023 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the next
>>> + * paragraph) shall be included in all copies or substantial portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef _DRM_NETLINK_H_
>>> +#define _DRM_NETLINK_H_
>>> +
>>> +#define DRM_GENL_VERSION 1
>>> +
>>> +#if defined(__cplusplus)
>>> +extern "C" {
>>> +#endif
>>> +
>>> +/**
>>> + * enum drm_genl_error_cmds - Supported error commands
>>> + *
>>> + */
>>> +enum drm_genl_error_cmds {
>>> +	DRM_CMD_UNSPEC,
>>> +	/** @DRM_RAS_CMD_QUERY: Command to list all errors names with config-id */
>>> +	DRM_RAS_CMD_QUERY,
>>> +	/** @DRM_RAS_CMD_READ_ONE: Command to get a counter for a specific error */
>>> +	DRM_RAS_CMD_READ_ONE,
>>> +	/** @DRM_RAS_CMD_READ_ALL: Command to get counters of all errors */
>>> +	DRM_RAS_CMD_READ_ALL,
>>> +
>>> +	__DRM_CMD_MAX,
>>> +	DRM_CMD_MAX = __DRM_CMD_MAX - 1,
>>> +};
>>> +
>>> +/**
>>> + * enum drm_error_attr - Attributes to use with drm_genl_error_cmds
>>> + *
>>> + */
>>> +enum drm_error_attr {
>>> +	DRM_ATTR_UNSPEC,
>>> +	DRM_ATTR_PAD = DRM_ATTR_UNSPEC,
>>> +	/**
>>> +	 * @DRM_RAS_ATTR_REQUEST: Should be used with DRM_RAS_CMD_QUERY,
>>> +	 * DRM_RAS_CMD_READ_ALL
>>> +	 */
>>> +	DRM_RAS_ATTR_REQUEST, /* NLA_U8 */
>>> +	/**
>>> +	 * @DRM_RAS_ATTR_QUERY_REPLY: First Nested attributed sent as a
>>> +	 * response to DRM_RAS_CMD_QUERY, DRM_RAS_CMD_READ_ALL commands.
>>> +	 */
>>> +	DRM_RAS_ATTR_QUERY_REPLY, /*NLA_NESTED*/
>> Maybe a space before and after NLA_NESTED?
> right missed that.
>
> Thanks,
> Aravind.
>> Thanks,
>> Tomer
>>
>>> +	/** @DRM_RAS_ATTR_ERROR_NAME: Used to pass error name */
>>> +	DRM_RAS_ATTR_ERROR_NAME, /* NLA_NUL_STRING */
>>> +	/** @DRM_RAS_ATTR_ERROR_ID: Used to pass error id */
>>> +	DRM_RAS_ATTR_ERROR_ID, /* NLA_U64 */
>>> +	/** @DRM_RAS_ATTR_ERROR_VALUE: Used to pass error value */
>>> +	DRM_RAS_ATTR_ERROR_VALUE, /* NLA_U64 */
>>> +
>>> +	__DRM_ATTR_MAX,
>>> +	DRM_ATTR_MAX = __DRM_ATTR_MAX - 1,
>>> +};
>>> +
>>> +#if defined(__cplusplus)
>>> +}
>>> +#endif
>>> +
>>> +#endif




More information about the Intel-xe mailing list