[Nouveau] [PATCH RFC 05/20] pm: reorganize the nvif interface
Ben Skeggs
skeggsb at gmail.com
Sat Jun 13 19:32:01 PDT 2015
On 10 June 2015 at 07:53, Samuel Pitoiset <samuel.pitoiset at gmail.com> wrote:
>
>
> 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 ?
Well, Ilia hasn't quite reviewed the patches yet still ;) But, you've
been using them successfully so far, so that counts as review.
I have some changes that I haven't quite pushed yet that'll need some
slight re-jigging of the libdrm series, but I'll push to merge it
(finally) once that's done.
>
>
>>
>>> __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.
Some work I've got queued up would conflict with these, so I've merged
your patches ahead of time and made that change myself to avoid you
having to rebase your whole series. Any fixes etc that'd been done
since can be done as separate patches before going to Linus.
Thanks,
Ben.
>
> 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