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 1/5] drm/netlink: Add netlink infrastructure
Date: Mon, 5 Jun 2023 22:48:19 +0530	[thread overview]
Message-ID: <4418f353-18c6-b74d-dc19-f6edc624cd52@intel.com> (raw)
In-Reply-To: <98184112-ca7d-65c5-0b98-94abb418a2a5@habana.ai>



On 04-06-2023 22:37, Tomer Tayar wrote:
> On 26/05/2023 19:20, Aravind Iddamsetty wrote:
>> Define the netlink commands and attributes that can be commonly used
>> across by drm drivers.
>>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
>> ---
>>   include/uapi/drm/drm_netlink.h | 68 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 include/uapi/drm/drm_netlink.h
>>
>> diff --git a/include/uapi/drm/drm_netlink.h b/include/uapi/drm/drm_netlink.h
>> new file mode 100644
>> index 000000000000..28e7a334d0c7
>> --- /dev/null
>> +++ b/include/uapi/drm/drm_netlink.h
>> @@ -0,0 +1,68 @@
>> +/*
>> + * 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_
>> +
>> +#include <linux/netdevice.h>
>> +#include <net/genetlink.h>
>> +#include <net/sock.h>
> 
> This is a uapi header.
> Are all header files here available for user?

no they are not, I later came to know that we should not have any of
that user can't use so will split the header into 2.
> Also, should we add here "#if defined(__cplusplus) extern "C" { ..."?

ya will add that
> 
>> +
>> +#define DRM_GENL_VERSION 1
>> +
>> +enum error_cmds {
>> +	DRM_CMD_UNSPEC,
>> +	/* command to list all errors names with config-id */
>> +	DRM_CMD_QUERY,
>> +	/* command to get a counter for a specific error */
>> +	DRM_CMD_READ_ONE,
>> +	/* command to get counters of all errors */
>> +	DRM_CMD_READ_ALL,
>> +
>> +	__DRM_CMD_MAX,
>> +	DRM_CMD_MAX = __DRM_CMD_MAX - 1,
>> +};
>> +
>> +enum error_attr {
>> +	DRM_ATTR_UNSPEC,
>> +	DRM_ATTR_PAD = DRM_ATTR_UNSPEC,
>> +	DRM_ATTR_REQUEST, /* NLA_U8 */
>> +	DRM_ATTR_QUERY_REPLY, /*NLA_NESTED*/
> 
> Missing spaces in /*NLA_NESTED*/
> 
>> +	DRM_ATTR_ERROR_NAME, /* NLA_NUL_STRING */
>> +	DRM_ATTR_ERROR_ID, /* NLA_U64 */
>> +	DRM_ATTR_ERROR_VALUE, /* NLA_U64 */
>> +
>> +	__DRM_ATTR_MAX,
>> +	DRM_ATTR_MAX = __DRM_ATTR_MAX - 1,
>> +};
>> +
>> +/* attribute policies */
>> +static const struct nla_policy drm_attr_policy_query[DRM_ATTR_MAX + 1] = {
>> +	[DRM_ATTR_REQUEST] = { .type = NLA_U8 },
>> +};
> 
> Should these policies structures be in uapi?

so ya these will also likely move into a separate drm header as
userspace would define there own policy.
> 
>> +
>> +static const struct nla_policy drm_attr_policy_read_one[DRM_ATTR_MAX + 1] = {
>> +	[DRM_ATTR_ERROR_ID] = { .type = NLA_U64 },
>> +};
> 
> I might miss something here, but why it is not a single policy structure 
> with entries for DRM_ATTR_REQUEST and DRM_ATTR_ERROR_ID?

so each command can have it's own policy defined, i.e what attributes it
expects we could define only those, that way we can have a check as
well. So, in the present implementation DRM_CMD_QUERY and
DRM_CMD_READ_ALL expect only DRM_ATTR_REQUEST and while DRM_CMD_READ_ONE
expects only DRM_ATTR_ERROR_ID as part of the incoming message from user.

Thanks,
Aravind.
> 
> Thanks,
> Tomer
> 
>> +
>> +#endif
> 
> 

  reply	other threads:[~2023-06-05 17:18 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 [this message]
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
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=4418f353-18c6-b74d-dc19-f6edc624cd52@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).