[Nouveau] [PATCH RFC 05/20] pm: reorganize the nvif interface
Samuel Pitoiset
samuel.pitoiset at gmail.com
Sun Jun 14 04:54:22 PDT 2015
On 06/14/2015 04:32 AM, Ben Skeggs wrote:
> 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.
Sounds good to me. :-)
>
>>
>>>> __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.
Well, I submitted two patches which fix some minor stuff to the series.
Let met know if they are okay for you.
By the way, I also submitted a patch which adds compute signals/sources
on Fermi.
Thanks,
Samuel.
>
> 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