[Nouveau] [RFC] Documentation: nouveau: Introduce some nouveau documentation

Wed Sep 23 21:36:54 UTC 2020

On Wed, Sep 23, 2020 at 10:39 PM Jeremy Cline <jcline at redhat.com> wrote:
>
> On Wed, Sep 23, 2020 at 09:02:45PM +0200, Karol Herbst wrote:
> > On Fri, Sep 11, 2020 at 6:21 PM Jeremy Cline <jcline at redhat.com> wrote:
>
> <snip>
>
> > yeah, I think overall this file is a good idea and being able to get a
> > quick overview over the driver is helpful. I think if we focus on the
> > user facing things first, like the hwmon or other things users
> > generally interact with would be helpful. I think if we start to
> > document areas where there are many low hanging fruits, this could
> > help random people to start with easier tasks and get more used to the
> > driver overall, so I'd probably ignore most of the stuff which really
> > requires a fundamental understanding on how things work and focus more
> > on vbios parsing (which has annoying interfaces anyway and it might
> > make sense to make it more consistent and nicer to use) and/or simple
> > code interfacing with the mmio space.
> >
>
> I'll admit to being motivated by entirely selfish reasons. I know
> practically nothing about nouveau and I'm the type of person who likes
> to keep notes about how things work together, both free form and
> structured in-line docs. All that to say, I think focusing on the
> "low-hanging fruit" stuff will be very beneficial and I'm happy to do
> that, but I'm also interested in documenting everything else I run
> across.
>

yeah, that's fine. I was just giving a suggestion on where the initial
focus should be on.

> > Eg some users have problems with their fans as they are either always
> > ramping up to max, or not running at all... GPUs temperature or power
> > consumption is reporting incorrectly... all those things users hit
> > regularly, but which isn't really important enough so it just falls
> > under the table even if it gets reported.
> >
>
> This does bring up an interesting point about organization and target
> audiences. Typically when I'm writing documentation I like to organize
> things by target audiences, so we could have a layout like:
>
> * General Introduction
>
> * User Guide
>     - Overview of supported hardware/features/etc

That's best to document in a wiki though. And we had plans to convert
the existing old wiki over to gitlab. And maybe It think we really
should do that and clean it up while we work on that. It's just a huge
project but maybe just starting with whatever you want to do would be
fine and after a while nothing is left. Anyway, I think we should
discuss this more openly with the others as well. i don't like the
current wiki anyway, as only approved developers can change things and
with a gitlab wiki we could even take MRs on stuff.

>     - Installation

well.. I think this can be skipped ;) But still, also belongs more
into a wiki. I think what we could cover here is to how to clean up
remaining stuff from the blob driver as this is something which comes
up quite a lot on IRC though.

>     - Configuration (module parameters and such)
>     - Troubleshooting

that would be cool to have in the wiki as well. Just collecting the
most common issue and document it there. Especially if it is on
gitlab, users can just do that as well :)

>     - Getting Involved (bug reporting, running tests, etc)

yeah, and we have some stuff on that on the old wiki already, it's
just very outdated and needs updating, which as said above can only be
done by developers and developers sadly have other things to do :)

>
> * Developer Guide
>     - Architecture Overview
>     - External APIs (include/uapi/drm/nouveau_drm.h, any sysfs stuff)?
>     - Internal APIs

Right, those things I'd like to see in the kernel tree actually.

>     - Debugging and Development Tools
>     - Contribution Guide
>

Those I think belong more into the wiki again. The latter is a bit
hard to split as there are kernel guides, but also community and
project guides. Mesa does things differently than let's say the
kernel. And Nouveau is still in this limbo state being on the old
infra, but also on the new one with mesa...

> I'm not sure how much stuff people want to keep on the
> nouveau.freedesktop.org wiki vs here.
>

I think the first step actually is to set up a proper nouveau project
on gitlab for dealing with issues and the wiki. I would be fine to do
that and we can also move the code there late. But maybe let's start
with the wiki :)

> > > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/engine/disp.h b/drivers/gpu/drm/nouveau/include/nvkm/engine/disp.h
> > > index 5a96c942d912..76b90d7ddfc6 100644
> > > --- a/drivers/gpu/drm/nouveau/include/nvkm/engine/disp.h
> > > +++ b/drivers/gpu/drm/nouveau/include/nvkm/engine/disp.h
> > > @@ -1,22 +1,57 @@
> > >  /* SPDX-License-Identifier: MIT */
> > > +
> > > +/**
> > > + * DOC: Overview
> > > + *
> > > + * Interfaces for working with the display engine.
> > > + */
> > > +
> > >  #ifndef __NVKM_DISP_H__
> > >  #define __NVKM_DISP_H__
> > > +
> > > +/**
> > > + * nvkm_disp() - Get a &struct nvkm_disp from a &struct nvkm_engine.
> > > + *
> > > + * @p: A &struct nvkm_engine reference.
> > > + *
> > > + * Return: The &struct nvkm_disp that contains the given engine.
> > > + */
> > >  #define nvkm_disp(p) container_of((p), struct nvkm_disp, engine)
> > >  #include <core/engine.h>
> > >  #include <core/event.h>
> > >
> > > +/**
> > > + * struct nvkm_disp - Represents a display engine.
> > > + *
> > > + * This structure is for <some abstraction here>. It has <some assumptions
> > > + * about its usage here>.
> > > + */
> > >  struct nvkm_disp {
> > > +    /** @func: Chipset-specific vtable for manipulating the display. */
> > >         const struct nvkm_disp_func *func;
> > > +
> > > +    /** @engine: The engine for this display. */
> > >         struct nvkm_engine engine;
> > >
> > > +    /** @head: list of heads attached to this display. */
> > >         struct list_head head;
> > > +
> > > +    /** @ior: List of IO resources for this display. */
> > >         struct list_head ior;
> > > +
> > > +    /** @outp: List of outputs for this display. */
> > >         struct list_head outp;
> > > +
> > > +    /** @conn: List of connectors for this display. */
> > >         struct list_head conn;
> > >
> > > +    /** @hpd: An event that fires when something happens I guess. */
> > >         struct nvkm_event hpd;
> > > +
> > > +    /** @vblank: An event that fires and has some relation to the vblank. */
> > >         struct nvkm_event vblank;
> > >
> > > +    /** @client: The oproxy (?) client for this display. */
> > >         struct nvkm_oproxy *client;
> > >  };
> >
> > generally not a big fan of "int a; // a is an int" kind of
> > documentation. But if it would specify constraints or details on how
> > it's valid to use those fields, then it makes totally sense to add
> > stuff.
>
> Definitely, I think that is not particularly helpful documentation. Of
> course, the what and why of a function parameter or struct member is
> very likely to be more interesting than that, but it's true that every
> once in a while that the variable name can be so clear there's not much
> else to say.
>
> I think it's fair to say the documentation I added for the above struct
> is not good. I think it's also fair to say that a new-comer such as
> myself who stumbles upon this structure has practically no chance of
> guessing what it's all about without reading a bunch of additional code.
> My first guess was that it represented a display I had plugged in, which
> at this point I'm fairly confident is not at all correct. It required me
> to look at many users of this struct along with perusing envytools
> documentation to guess it represented a display engine.
>

yeah, but given that you run into the confusion you can actually
document this and leave a comment addressing that. So describing a
little bit what the engine does, what are heads, iors and outputs,
etc... I think getting the high level overview should be the focus
atm. We can always deal with the technical details later as those are
usually easier to get once you have a rough idea on what's going on.

> It may well be I'm an exceptionally slow learner, but even short notes
> can be extremely helpful.
>
> >
> > not sure if you were aware of it, but we have some documentation on
> > the module options here:
> > https://nouveau.freedesktop.org/wiki/KernelModuleParameters/
> >
> > But I think it makes sense to move it into the source code and link to
> > the new thing from the wiki then.
> >
>
> Indeed, and in fact I started this documentation from the wiki, but
> tried my best to fill in the missing parameters and config options (you
> don't happen to know what the NvAcrWpr* config options do, do you?)
>

I only know that this option specifies the version of the ACR WPR
firmware to load, but I don't know what that actually is :)

> Thanks!
>
> - Jeremy
>