[Nouveau] [PATCH RFC 05/20] pm: reorganize the nvif interface
Samuel Pitoiset
samuel.pitoiset at gmail.com
Tue Jun 9 14:53:26 PDT 2015
On 06/09/2015 12:02 AM, Ben Skeggs wrote:
> On 8 June 2015 at 06:40, Samuel Pitoiset <samuel.pitoiset at gmail.com> wrote:
>> This commit introduces the NVIF_IOCTL_NEW_V0_PERFMON class which will be
>> used in order to query domains, signals and sources. This separates the
>> querying and the counting interface.
> Hey Samuel,
>
> I've merged patches 1-4 already, I've got some comments on this one,
> but after they're solved I'm happy to merge up to (and including)
> patch 18. Patches 19/20, I need to think about some more.
>
Hey Ben,
Thanks for reviewing this series so quickly. :-)
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>> bin/nv_perfmon.c | 12 ++++++------
>> drm/nouveau/include/nvif/class.h | 26 ++++++++++++++++----------
>> drm/nouveau/include/nvif/ioctl.h | 5 +++--
>> drm/nouveau/nvkm/engine/pm/base.c | 38 ++++++++++++++++++++++++++++++++------
>> 4 files changed, 57 insertions(+), 24 deletions(-)
>>
>> diff --git a/bin/nv_perfmon.c b/bin/nv_perfmon.c
>> index a8c5838..30a3138 100644
>> --- a/bin/nv_perfmon.c
>> +++ b/bin/nv_perfmon.c
>> @@ -600,7 +600,7 @@ main(int argc, char **argv)
>> const char *cfg = NULL;
>> const char *dbg = "error";
>> u64 dev = ~0ULL;
>> - struct nvif_perfctr_query_v0 args = {};
>> + struct nvif_perfmon_query_signal_v0 args = {};
>> struct nvif_client *client;
>> struct nvif_object object;
>> int ret, c, k;
>> @@ -644,15 +644,14 @@ main(int argc, char **argv)
>> }
>>
>> ret = nvif_object_init(nvif_object(device), NULL, 0xdeadbeef,
>> - NVIF_IOCTL_NEW_V0_PERFCTR,
>> - &(struct nvif_perfctr_v0) {
>> - }, sizeof(struct nvif_perfctr_v0), &object);
>> + NVIF_IOCTL_NEW_V0_PERFMON, NULL, 0, &object);
>> assert(ret == 0);
>> do {
>> u32 prev_iter = args.iter;
>>
>> args.name[0] = '\0';
>> - ret = nvif_mthd(&object, NVIF_PERFCTR_V0_QUERY, &args, sizeof(args));
>> + ret = nvif_mthd(&object, NVIF_PERFMON_V0_QUERY_SIGNAL,
>> + &args, sizeof(args));
>> assert(ret == 0);
>>
>> if (prev_iter) {
>> @@ -663,7 +662,8 @@ main(int argc, char **argv)
>> args.iter = prev_iter;
>> strncpy(signals[nr_signals - 1], args.name,
>> sizeof(args.name));
>> - ret = nvif_mthd(&object, NVIF_PERFCTR_V0_QUERY, &args, sizeof(args));
>> + ret = nvif_mthd(&object, NVIF_PERFMON_V0_QUERY_SIGNAL,
>> + &args, sizeof(args));
>> assert(ret == 0);
>> }
>> } while (args.iter != 0xffffffff);
>> diff --git a/drm/nouveau/include/nvif/class.h b/drm/nouveau/include/nvif/class.h
>> index 64f8b2f..11935a0 100644
>> --- a/drm/nouveau/include/nvif/class.h
>> +++ b/drm/nouveau/include/nvif/class.h
>> @@ -251,6 +251,20 @@ struct gf110_dma_v0 {
>> * perfmon
>> ******************************************************************************/
>>
>> +#define NVIF_PERFMON_V0_QUERY_SIGNAL 0x00
>> +
>> +struct nvif_perfmon_query_signal_v0 {
>> + __u8 version;
>> + __u8 pad01[3];
>> + __u32 iter;
>> + char name[64];
>> +};
>> +
>> +
>> +/*******************************************************************************
>> + * perfctr
>> + ******************************************************************************/
>> +
>> struct nvif_perfctr_v0 {
>> __u8 version;
>> __u8 pad01[1];
>> @@ -259,16 +273,8 @@ struct nvif_perfctr_v0 {
>> char name[4][64];
>> };
>>
>> -#define NVIF_PERFCTR_V0_QUERY 0x00
>> -#define NVIF_PERFCTR_V0_SAMPLE 0x01
>> -#define NVIF_PERFCTR_V0_READ 0x02
>> -
>> -struct nvif_perfctr_query_v0 {
>> - __u8 version;
>> - __u8 pad01[3];
>> - __u32 iter;
>> - char name[64];
>> -};
>> +#define NVIF_PERFCTR_V0_SAMPLE 0x00
>> +#define NVIF_PERFCTR_V0_READ 0x01
>>
>> struct nvif_perfctr_sample {
>> };
>> diff --git a/drm/nouveau/include/nvif/ioctl.h b/drm/nouveau/include/nvif/ioctl.h
>> index 4cd8e32..517cd27 100644
>> --- a/drm/nouveau/include/nvif/ioctl.h
>> +++ b/drm/nouveau/include/nvif/ioctl.h
>> @@ -49,8 +49,9 @@ struct nvif_ioctl_new_v0 {
>> __u64 token;
>> __u32 handle;
>> /* these class numbers are made up by us, and not nvidia-assigned */
>> -#define NVIF_IOCTL_NEW_V0_PERFCTR 0x0000ffff
>> -#define NVIF_IOCTL_NEW_V0_CONTROL 0x0000fffe
>> +#define NVIF_IOCTL_NEW_V0_PERFMON 0x0000ffff
>> +#define NVIF_IOCTL_NEW_V0_PERFCTR 0x0000fffe
>> +#define NVIF_IOCTL_NEW_V0_CONTROL 0x0000fffd
> It doesn't matter this time, because we're technically breaking ABI
> already anyway and current userspace won't be effected, but best to
> avoid changing class numbers like this :) It's fine this time though.
Sure, since the nvif interface is still not exposed through libdrm, this
is not going to affect userspace.
Btw, what is your plan about this? Are you going to merge nvif support
to libdrm ?
>
>> __u32 oclass;
>> __u8 data[]; /* class data (class.h) */
>> };
>> diff --git a/drm/nouveau/nvkm/engine/pm/base.c b/drm/nouveau/nvkm/engine/pm/base.c
>> index 7b07e8b..cb88170 100644
>> --- a/drm/nouveau/nvkm/engine/pm/base.c
>> +++ b/drm/nouveau/nvkm/engine/pm/base.c
>> @@ -83,10 +83,10 @@ nvkm_perfsig_find(struct nvkm_pm *ppm, const char *name, u32 size,
>> * Perfmon object classes
>> ******************************************************************************/
>> static int
>> -nvkm_perfctr_query(struct nvkm_object *object, void *data, u32 size)
>> +nvkm_perfmon_mthd_query_signal(struct nvkm_object *object, void *data, u32 size)
>> {
>> union {
>> - struct nvif_perfctr_query_v0 v0;
>> + struct nvif_perfmon_query_signal_v0 v0;
>> } *args = data;
>> struct nvkm_device *device = nv_device(object);
>> struct nvkm_pm *ppm = (void *)object->engine;
>> @@ -97,9 +97,9 @@ nvkm_perfctr_query(struct nvkm_object *object, void *data, u32 size)
>> int tmp = 0, di, si;
>> int ret;
>>
>> - nv_ioctl(object, "perfctr query size %d\n", size);
>> + nv_ioctl(object, "perfmon query signal size %d\n", size);
>> if (nvif_unpack(args->v0, 0, 0, false)) {
>> - nv_ioctl(object, "perfctr query vers %d iter %08x\n",
>> + nv_ioctl(object, "perfmon query signal vers %d iter %08x\n",
>> args->v0.version, args->v0.iter);
>> di = (args->v0.iter & 0xff000000) >> 24;
>> si = (args->v0.iter & 0x00ffffff) - 1;
>> @@ -142,6 +142,30 @@ nvkm_perfctr_query(struct nvkm_object *object, void *data, u32 size)
>> }
>>
>> static int
>> +nvkm_perfmon_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
>> +{
>> + switch (mthd) {
>> + case NVIF_PERFMON_V0_QUERY_SIGNAL:
>> + return nvkm_perfmon_mthd_query_signal(object, data, size);
>> + default:
>> + break;
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static struct nvkm_ofuncs
>> +nvkm_perfmon_ofuncs = {
>> + .ctor = _nvkm_object_ctor,
>> + .dtor = nvkm_object_destroy,
>> + .init = nvkm_object_init,
>> + .fini = nvkm_object_fini,
>> + .mthd = nvkm_perfmon_mthd,
>> +};
>> +
>> +/*******************************************************************************
>> + * Perfctr object classes
>> + ******************************************************************************/
>> +static int
>> nvkm_perfctr_sample(struct nvkm_object *object, void *data, u32 size)
>> {
>> union {
>> @@ -221,8 +245,6 @@ static int
>> nvkm_perfctr_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
>> {
>> switch (mthd) {
>> - case NVIF_PERFCTR_V0_QUERY:
>> - return nvkm_perfctr_query(object, data, size);
>> case NVIF_PERFCTR_V0_SAMPLE:
>> return nvkm_perfctr_sample(object, data, size);
>> case NVIF_PERFCTR_V0_READ:
>> @@ -299,6 +321,10 @@ nvkm_perfctr_ofuncs = {
>>
>> struct nvkm_oclass
>> nvkm_pm_sclass[] = {
>> + {
>> + .handle = NVIF_IOCTL_NEW_V0_PERFMON,
>> + .ofuncs = &nvkm_perfmon_ofuncs,
>> + },
>> { .handle = NVIF_IOCTL_NEW_V0_PERFCTR,
>> .ofuncs = &nvkm_perfctr_ofuncs,
>> },
> What I'd like to see here is that nvkm_pm_sclass() only has PERFMON,
> and PERFCTR is created as a child of it (like how all the PDISP
> channel objects are children of the main disp class). To do this, you
> need to create PERFMON as a nvkm_parent instead of an nvkm_object, and
> have its parent.sclass contain PERFMON. This will perhaps look a bit
> yucky for the moment, but I'm working on making things cleaner so I'll
> fix it later. Feel free to ping me on IRC if you need a hand making
> the change though.
>
> I actually have a large patch series with the first part of this
> cleanup work queued up, but if you can make the above change first,
> I'll rebase my work on yours so you don't have to deal with that.
Okay, I'll take a look at the PDISP engine and make this change for the
PM engine this week.
And if I have some other questions, I'll ping you on IRC.
Thanks,
Samuel.
>
> Thanks,
> Ben.
>
>> --
>> 2.4.2
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
More information about the Nouveau
mailing list