On Thu, Sep 24, 2020 at 3:06 PM Roy Spliet <nouv...@spliet.org> wrote: > > > Op 23-09-2020 om 22:36 schreef Karol Herbst: > > On Wed, Sep 23, 2020 at 10:39 PM Jeremy Cline <jcl...@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 <jcl...@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 :) > > Risking to turn this into a "let's fix everything in one go" project > that ends up never getting finished, I just want to make sure that > everybody is also aware of the documentation generated from Envytools > [0]. Especially "architecture overview" (that is, if we're talking about > hardware architecture and not driver/software architecture) might be > better suited in envytools. > > As for module parameters, IMHO modinfo is supposed to be the source of > information. It's sadly lacking any "sub-"option inside nouveau.config > and nouveau.debug, which may be by design. Perhaps expanding the modinfo > explanations can help cover at least all the other options in a way that > never gets out of sync with source code. >
technically true, but 99% of the users won't know that modinfo shows what parameters there are. I don't like this "config" parameter and I think we should split it up and have a nice line telling what they do for modinfo, but for user facing documentation we need something more detailed and something people find when searching for it on the web. > Roy > > [0] https://envytools.readthedocs.io/en/latest/ > > > > >>>> 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 > >> > > > > _______________________________________________ > > Nouveau mailing list > > Nouveau@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/nouveau > > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau > _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
