[Mesa-dev] [PATCH v2 1/7] nouveau: implement the nvif hardware performance counters interface
Samuel Pitoiset
samuel.pitoiset at gmail.com
Thu Jul 23 07:57:56 PDT 2015
On 07/22/2015 10:29 PM, Martin Peres wrote:
>
>
> On 01/07/15 01:01, Samuel Pitoiset wrote:
>> This commit implements the base interface for hardware performance
>> counters that will be shared between nv50 and nvc0 drivers.
>>
>> TODO: Bump libdrm version of mesa when nvif will be merged.
>>
>> Changes since v2:
>> - remove double-query thing for domains, signals and sources
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>> src/gallium/drivers/nouveau/Makefile.sources | 2 +
>> src/gallium/drivers/nouveau/nouveau_perfmon.c | 290
>> ++++++++++++++++++++++++++
>> src/gallium/drivers/nouveau/nouveau_perfmon.h | 58 ++++++
>> src/gallium/drivers/nouveau/nouveau_screen.c | 5 +
>> src/gallium/drivers/nouveau/nouveau_screen.h | 1 +
>> 5 files changed, 356 insertions(+)
>> create mode 100644 src/gallium/drivers/nouveau/nouveau_perfmon.c
>> create mode 100644 src/gallium/drivers/nouveau/nouveau_perfmon.h
>>
>> diff --git a/src/gallium/drivers/nouveau/Makefile.sources
>> b/src/gallium/drivers/nouveau/Makefile.sources
>> index 3fae3bc..3da0bdc 100644
>> --- a/src/gallium/drivers/nouveau/Makefile.sources
>> +++ b/src/gallium/drivers/nouveau/Makefile.sources
>> @@ -10,6 +10,8 @@ C_SOURCES := \
>> nouveau_heap.h \
>> nouveau_mm.c \
>> nouveau_mm.h \
>> + nouveau_perfmon.c \
>> + nouveau_perfmon.h \
>> nouveau_screen.c \
>> nouveau_screen.h \
>> nouveau_statebuf.h \
>> diff --git a/src/gallium/drivers/nouveau/nouveau_perfmon.c
>> b/src/gallium/drivers/nouveau/nouveau_perfmon.c
>> new file mode 100644
>> index 0000000..e1d4546
>> --- /dev/null
>> +++ b/src/gallium/drivers/nouveau/nouveau_perfmon.c
>> @@ -0,0 +1,290 @@
>> +/*
>> + * Copyright 2015 Samuel Pitoiset
>> + *
>> + * 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 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
>> + * THE AUTHORS OR COPYRIGHT HOLDERS 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.
>> + */
>> +
>> +#include <errno.h>
>> +
>> +#include "util/u_memory.h"
>> +
>> +#include "nouveau_debug.h"
>> +#include "nouveau_winsys.h"
>> +#include "nouveau_perfmon.h"
>> +
>> +static int
>> +nouveau_perfmon_query_sources(struct nouveau_perfmon *pm,
>> + struct nouveau_perfmon_dom *dom,
>> + struct nouveau_perfmon_sig *sig)
>> +{
>> + struct nvif_perfmon_query_source_v0 args = {};
>> +
>> + args.iter = 1;
>
> Why start iterating from 1 and not 0?
Starting from 1 will give you the first source of the signal (because -1
is performed on the kernel side).
>
>> + args.domain = dom->id;
>> + args.signal = sig->signal;
>> + do {
>> + struct nouveau_perfmon_src *src;
>> + int ret;
>> +
>> + ret = nouveau_object_mthd(pm->object,
>> NVIF_PERFMON_V0_QUERY_SOURCE,
>> + &args, sizeof(args));
>> + if (ret)
>> + return ret;
>
> You do not check what happens if you do not expose any source for
> this signal. A test on args.iter != 0xffff with a return if not the
> case would be nice!
If no sources are exposed for a signal, it will return -EINVAL.
But we don't care to handle this because we check if a signal exposes
sources before querying them. (cf. perfmon_query_signals()).
>
>> +
>> + src = CALLOC_STRUCT(nouveau_perfmon_src);
>> + if (!src)
>> + return -ENOMEM;
>> +
>> +#if 0
>> + debug_printf("id = %d\n", args.source);
>> + debug_printf("name = %s\n", args.name);
>> + debug_printf("mask = %08x\n", args.mask);
>> + debug_printf("\n");
>> +#endif
>> +
>> + src->id = args.source;
>> + strncpy(src->name, args.name, sizeof(src->name));
>> + list_addtail(&src->head, &sig->sources);
>> + } while (args.iter != 0xff);
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +nouveau_perfmon_query_signals(struct nouveau_perfmon *pm,
>> + struct nouveau_perfmon_dom *dom)
>> +{
>> + struct nvif_perfmon_query_signal_v0 args = {};
>> +
>> + args.iter = 1;
>> + args.domain = dom->id;
>> + do {
>> + struct nouveau_perfmon_sig *sig;
>> + int ret;
>> +
>> + ret = nouveau_object_mthd(pm->object,
>> NVIF_PERFMON_V0_QUERY_SIGNAL,
>> + &args, sizeof(args));
>> + if (ret)
>> + return ret;
>
> You do not check what happens if you do not expose any source for this
> signal. A test on args.iter != 0xffff with a return if not the case
> would be nice!
Same as above.
>
>> +
>> + sig = CALLOC_STRUCT(nouveau_perfmon_sig);
>> + if (!sig)
>> + return -ENOMEM;
>> + list_inithead(&sig->sources);
>> +
>> +#if 0
>> + debug_printf("name = %s\n", args.name);
>> + debug_printf("signal = 0x%02x\n", args.signal);
>> + debug_printf("source_nr = %d\n", args.source_nr);
>> + debug_printf("\n");
>> +#endif
>> +
>> + sig->signal = args.signal;
>> + strncpy(sig->name, args.name, sizeof(sig->name));
>> + list_addtail(&sig->head, &dom->signals);
>> +
>> + /* Query all available sources for this signal. */
>> + if (args.source_nr > 0) {
>> + ret = nouveau_perfmon_query_sources(pm, dom, sig);
>> + if (ret)
>> + return ret;
>> + }
>> + } while (args.iter != 0xffff);
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +nouveau_perfmon_query_domains(struct nouveau_perfmon *pm)
>> +{
>> + struct nvif_perfmon_query_domain_v0 args = {};
>> + int ret;
>> +
>> + /* Before querying domains, check if the device exposes some of
>> them. */
>> + ret = nouveau_object_mthd(pm->object, NVIF_PERFMON_V0_QUERY_DOMAIN,
>> + &args, sizeof(args));
>> + if (ret)
>> + return ret;
>
> Why did you unrol this test? Delete the block above and move the block
> under this comment right before the allocation of dom.
At beginning we don't know if the device exposes domains. Therefore, we
have to check if it exposes them.
Notice that here, args.iter is initialized to 0, so if no domains are
exposed, args.iter will be set 0xff.
I don't clearly understand why you want to change this block.
>> +
>> + if (args.iter == 0xff) {
>> + /* No domains are exposed by the device. */
>> + return 0;
>> + }
>> +
>> + args.iter = 1;
>> + do {
>> + struct nouveau_perfmon_dom *dom;
>> + int ret;
>> +
>> + ret = nouveau_object_mthd(pm->object,
>> NVIF_PERFMON_V0_QUERY_DOMAIN,
>> + &args, sizeof(args));
>> + if (ret)
>> + return ret;
>> +
>> + dom = CALLOC_STRUCT(nouveau_perfmon_dom);
>> + if (!dom)
>> + return -ENOMEM;
>> + list_inithead(&dom->signals);
>> +
>> +#if 0
>> + debug_printf("id = %d\n", args.id);
>> + debug_printf("name = %s\n", args.name);
>> + debug_printf("counter_nr = %d\n", args.counter_nr);
>> + debug_printf("signal_nr = %d\n", args.signal_nr);
>> + debug_printf("\n");
>> +#endif
>> +
>> + dom->id = args.id;
>> + dom->max_active_cntr = args.counter_nr;
>> + strncpy(dom->name, args.name, sizeof(dom->name));
>> + list_addtail(&dom->head, &pm->domains);
>> +
>> + /* Query all available signals for this domain. */
>> + if (args.signal_nr > 0) {
>> + ret = nouveau_perfmon_query_signals(pm, dom);
>> + if (ret)
>> + return ret;
>> + }
>> + } while (args.iter != 0xff);
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +nouveau_perfmon_free_sources(struct nouveau_perfmon_sig *sig)
>> +{
>> + struct nouveau_perfmon_src *src, *next;
>> +
>> + LIST_FOR_EACH_ENTRY_SAFE(src, next, &sig->sources, head) {
>> + list_del(&src->head);
>> + free(src);
>> + }
>> +}
>> +
>> +static void
>> +nouveau_perfmon_free_signals(struct nouveau_perfmon_dom *dom)
>> +{
>> + struct nouveau_perfmon_sig *sig, *next;
>> +
>> + LIST_FOR_EACH_ENTRY_SAFE(sig, next, &dom->signals, head) {
>> + nouveau_perfmon_free_sources(sig);
>> + list_del(&sig->head);
>> + free(sig);
>> + }
>> +}
>> +
>> +static void
>> +nouveau_perfmon_free_domains(struct nouveau_perfmon *pm)
>> +{
>> + struct nouveau_perfmon_dom *dom, *next;
>> +
>> + LIST_FOR_EACH_ENTRY_SAFE(dom, next, &pm->domains, head) {
>> + nouveau_perfmon_free_signals(dom);
>> + list_del(&dom->head);
>> + free(dom);
> Free(dom), nice one :D
Yeah! :)
>> + }
>> +}
>> +
>> +struct nouveau_perfmon *
>> +nouveau_perfmon_create(struct nouveau_device *dev)
>> +{
>> + struct nouveau_perfmon *pm = NULL;
>> + int ret;
>> +
>> + pm = CALLOC_STRUCT(nouveau_perfmon);
>> + if (!pm) {
>> + NOUVEAU_ERR("Failed to allocate pm struct!\n");
>> + return NULL;
>> + }
>> + list_inithead(&pm->domains);
>> +
>> + /* init handle for perfdom objects */
>> + pm->handle = 0xbeef9751;
>> +
>> + ret = nouveau_object_new(&dev->object, 0xbeef9750,
> Shouldn't those handles be in libdrm?
No. All handles seem to be hardcoded in mesa directly.
I just followed the same convention.
>> + NVIF_IOCTL_NEW_V0_PERFMON, NULL, 0, &pm->object);
>> + if (ret)
>> + goto fail;
>> +
>> + /* Query all available domains, signals and sources for this
>> device. */
>> + ret = nouveau_perfmon_query_domains(pm);
>> + if (ret)
>> + goto fail;
>> +
>> + return pm;
>> +
>> +fail:
>> + nouveau_perfmon_destroy(pm);
>> + return NULL;
>> +}
>> +
>> +void
>> +nouveau_perfmon_destroy(struct nouveau_perfmon *pm)
>> +{
>> + if (!pm)
>> + return;
>> +
>> + nouveau_perfmon_free_domains(pm);
>> + nouveau_object_del(&pm->object);
>> + FREE(pm);
>> +}
>> +
>> +struct nouveau_perfmon_dom *
>> +nouveau_perfmon_get_dom_by_id(struct nouveau_perfmon *pm, uint8_t
>> dom_id)
>> +{
>> + struct nouveau_perfmon_dom *dom;
>> +
>> + if (pm) {
>> + LIST_FOR_EACH_ENTRY(dom, &pm->domains, head) {
>> + if (dom->id == dom_id)
>> + return dom;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +struct nouveau_perfmon_sig *
>> +nouveau_perfmon_get_sig_by_name(struct nouveau_perfmon_dom *dom,
>> + const char *name)
>> +{
>> + struct nouveau_perfmon_sig *sig;
>> +
>> + if (dom) {
>> + LIST_FOR_EACH_ENTRY(sig, &dom->signals, head) {
>> + if (!strcmp(sig->name, name))
>> + return sig;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +struct nouveau_perfmon_src *
>> +nouveau_perfmon_get_src_by_name(struct nouveau_perfmon_sig *sig,
>> + const char *name)
>> +{
>> + struct nouveau_perfmon_src *src;
>> +
>> + if (sig) {
>> + LIST_FOR_EACH_ENTRY(src, &sig->sources, head) {
>> + if (!strcmp(src->name, name))
>> + return src;
>> + }
>> + }
>> + return NULL;
>> +}
>> diff --git a/src/gallium/drivers/nouveau/nouveau_perfmon.h
>> b/src/gallium/drivers/nouveau/nouveau_perfmon.h
>> new file mode 100644
>> index 0000000..c18addc
>> --- /dev/null
>> +++ b/src/gallium/drivers/nouveau/nouveau_perfmon.h
>> @@ -0,0 +1,58 @@
>> +#ifndef __NOUVEAU_PERFMON_H__
>> +#define __NOUVEAU_PERFMON_H__
>> +
>> +#include <drm.h>
>> +#include <nouveau_class.h>
>> +#include <nouveau_ioctl.h>
>> +
>> +#include "util/list.h"
>> +
>> +struct nouveau_perfmon
>> +{
>> + struct nouveau_object *object;
>> + struct list_head domains;
>> + uint64_t handle;
>> +};
>> +
>> +struct nouveau_perfmon_dom
>> +{
>> + struct list_head head;
>> + struct list_head signals;
>> + uint8_t id;
>> + char name[64];
>> + uint8_t max_active_cntr;
>> +};
>> +
>> +struct nouveau_perfmon_sig
>> +{
>> + struct list_head head;
>> + struct list_head sources;
>> + uint8_t signal;
>> + char name[64];
>> +};
>> +
>> +struct nouveau_perfmon_src
>> +{
>> + struct list_head head;
>> + uint8_t id;
>> + char name[64];
>> +};
>> +
>> +struct nouveau_perfmon *
>> +nouveau_perfmon_create(struct nouveau_device *dev);
>> +
>> +void
>> +nouveau_perfmon_destroy(struct nouveau_perfmon *pm);
>> +
>> +struct nouveau_perfmon_dom *
>> +nouveau_perfmon_get_dom_by_id(struct nouveau_perfmon *pm, uint8_t
>> dom_id);
>> +
>> +struct nouveau_perfmon_sig *
>> +nouveau_perfmon_get_sig_by_name(struct nouveau_perfmon_dom *dom,
>> + const char *name);
>> +
>> +struct nouveau_perfmon_src *
>> +nouveau_perfmon_get_src_by_name(struct nouveau_perfmon_sig *sig,
>> + const char *name);
>> +
>> +#endif /* __NOUVEAU_PERFMON_H__ */
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
>> b/src/gallium/drivers/nouveau/nouveau_screen.c
>> index c6e5074..3c14a77 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
>> @@ -21,6 +21,7 @@
>> #include "nouveau_fence.h"
>> #include "nouveau_mm.h"
>> #include "nouveau_buffer.h"
>> +#include "nouveau_perfmon.h"
>> /* XXX this should go away */
>> #include "state_tracker/drm_driver.h"
>> @@ -226,6 +227,8 @@ nouveau_screen_init(struct nouveau_screen
>> *screen, struct nouveau_device *dev)
>> NOUVEAU_BO_GART | NOUVEAU_BO_MAP,
>> &mm_config);
>> screen->mm_VRAM = nouveau_mm_create(dev, NOUVEAU_BO_VRAM,
>> &mm_config);
>> +
>> + screen->perfmon = nouveau_perfmon_create(dev);
>> return 0;
>> }
>> @@ -235,6 +238,8 @@ nouveau_screen_fini(struct nouveau_screen *screen)
>> nouveau_mm_destroy(screen->mm_GART);
>> nouveau_mm_destroy(screen->mm_VRAM);
>> + nouveau_perfmon_destroy(screen->perfmon);
>> +
>> nouveau_pushbuf_del(&screen->pushbuf);
>> nouveau_client_del(&screen->client);
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.h
>> b/src/gallium/drivers/nouveau/nouveau_screen.h
>> index 30041b2..fd7ecdb 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.h
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.h
>> @@ -21,6 +21,7 @@ struct nouveau_screen {
>> struct nouveau_object *channel;
>> struct nouveau_client *client;
>> struct nouveau_pushbuf *pushbuf;
>> + struct nouveau_perfmon *perfmon;
>> int refcount;
>
> With the checks in the allocations added, this is:
>
> Reviewed-by: Martin Peres <martin.peres at free.fr>
More information about the mesa-dev
mailing list