Announcement

Collapse
No announcement yet.

Initial Rust DRM Abstractions, AGX Apple DRM Driver Posted For Review

Collapse
X
 
  • Filter
  • Time
  • Show
Clear All
new posts

  • #31
    Originally posted by Weasel View Post
    And I bet it's needed due to the way Rust handles "safety". And she'd have to use "unsafe" otherwise with the proposed solutions to the patch. Oh no.
    Reversing a gpu without good documentation. you are going to have dangerous behaviors and things happen when you are not expecting. Yes Rust safety is a factor but there is another factor.


    I actually don't know of any way to actively abort jobs on the firmware,
    so this is pretty much the only option I have. I've even seen
    long-running compute jobs on macOS run to completion even if you kill
    the submitting process, so there might be no way to do this at all.
    Though in practice since we unmap everything from the VM anyway when the
    userspace stuff gets torn down, almost any normal GPU work is going to
    immediately fault at that point (macOS doesn't do this because macOS
    effectively does implicit sync with BO tracking at the kernel level...).​​
    I even went to the effort to quoting it for you. These reversing gpu with lack of documentation there are many cases where you have given the instructions you think will say job die and that not what has happened or in the AGX case it simply going to only end once job has reached complete.

    Weasel it is rust unsafe is factor why person looked at doing this code but its also the nature of some GPUs as well.

    Hopefully apple has not made a mistake in the AGX and include means to infinity loop in the AGX yes halt and catch file. There are reasons why you want to be able to force terminate GPU processes so most GPU designs include this feature. Note I said most AGX is not the only exception.

    Weasel the prior stuff ups in these areas that caused kernel paincks don't after this modification.

    Weasel think about drm_sched_fini why is every driver having to do it own checks before calling drm_sched_fini to make sure no job is still using the device this is code duplication. Rust developers nature that C code should validate does cause some better C code design choices in the long term in lots of cases.

    Yes the one you said that it releates to the rust driver "can_run_job" in fact does not relate to safe and unsafe code.

    It relates to how in heck to you set what is the max number of jobs you can run on a GPU at anyone time this is a different method.
    But keep in mind none of this is documented, and there's no way for us
    driver authors to understand what we're supposed to do without
    documentation.​
    Do note the other ways suggested to do instead of can_run_job happen to be undocumented. Is the first one being done because the driver is being written in rust absolutely no. Is it being done because the developer believes this could be a higher efficiency solution than the existing method absolutely yes.

    Both of these solutions are author with different background looking at code and coming up with a different solution.

    Rust developers looking to have C code they can declare safe does cause some interesting design choices like drm_sched_fini one that could over time reduce lots of graphics driver code and make it simpler for people to write new people due to having less lines of code to get right so driver works correctly.

    Weasel the developer has gone into detailed reasons for both alterations its not just hey we want rust safe support. Rust safe support caused them to look at drm_sched_fini in more detail and notice hang on why are we doing this were a person can miss cleaning up all the jobs and then call drm_sched_fini and kernel panic the system it will not add much overhead adding auto clean up here particularly when this function is only called when you are basically shutdown a GPU.

    Weasel think about it how would you like shutdown your system to have it kernel panic due to some GPU job so not writing your files to disc correctly just because drm_sched_fini is done how it currently is and some driver developer did not pick up some job on the GPU was not closed before calling drm_sched_fini. Just because something is motivated by rust safe vs unsafe does not make the assessment that the current implementation of drm_sched_fini could be stupidly dangerous and a change should be suggested particularly considering drm_sched_fini is not path that need to be high performance and failure leading to panick when drm_sched_fini is normally will be called could be data loss. People make checklists for safety things for a reason. Making sure you don't terminate a scheduler with jobs still on list would be like safety checklist.

    Comment


    • #32
      Originally posted by Weasel View Post
      And I bet it's needed due to the way Rust handles "safety". And she'd have to use "unsafe" otherwise with the proposed solutions to the patch. Oh no.
      No, the first patch literally has nothing to do with Rust at all. It's a limitation of the Apple GPU firmware they are trying to model, and the question is how to do it in the DRM subsystem. Rust or no rust makes zero difference on that patch, and the fact that you can't see that doesn't speak very highly towards your programming skills.

      Now the second one is about Rust semantics, if you wanted to focus on that one.

      I notice you ignored my last response, so I'll try again. Otherwise I'll have to assume you're just shitposting and have no other changes you were actually looking at, in spite of saying you did.

      Please point out these other changes they wanted which show that Rust sucks.

      I'm sure we're all interested to see it, since the patches this article was about apparently don't cover it.​

      Comment


      • #33
        Originally posted by smitty3268 View Post
        No, the first patch literally has nothing to do with Rust at all. It's a limitation of the Apple GPU firmware they are trying to model, and the question is how to do it in the DRM subsystem. Rust or no rust makes zero difference on that patch, and the fact that you can't see that doesn't speak very highly towards your programming skills.

        Now the second one is about Rust semantics, if you wanted to focus on that one.

        I notice you ignored my last response, so I'll try again. Otherwise I'll have to assume you're just shitposting and have no other changes you were actually looking at, in spite of saying you did.
        I mean she did claim she did the bare minimum but would have liked more C code changes? Obviously the changes aren't released. What do you expect? I'm just taking her word.

        And by "Rust sucks" I mean "forcing you into a specific design, even with other APIs". A powerful language should be able to cleanly deal with any interfaces at all. Just like C can deal with C++ interfaces without issues (except name mangling will be terrible). Even though C has no notion of C++ and was designed before it, so it's not like they "dealt with it later". Here's an example of such.

        I mean... Rust technically can, but people hate using "unsafe", so there's that.

        Comment


        • #34
          Originally posted by Weasel View Post
          And I bet it's needed due to the way Rust handles "safety". And she'd have to use "unsafe" otherwise with the proposed solutions to the patch. Oh no.
          It's not. The existing way the C drivers do it is just dumb.

          It needed an unsafe block because the C code in other drivers (that the rust code would have to mimic) is itself stupid and error-prone. If you're pushing for a more reliable driver anyway (this one went from single-threaded to multi-thread multi-application far easier than any C driver ever did) then it makes sense to take the opportunity to clean some of this other junk up too.

          This happens all the time in mesa. There will be some task that many drivers need to do, and all of them will produce separate buggy implementations of it. Then eventually someone puts in the effort to implement it properly in the common code and each driver is converted to use the common implementation. Now if other drivers want to do it this way, they can.
          Last edited by Developer12; 09 March 2023, 08:09 PM.

          Comment


          • #35
            Originally posted by Weasel View Post
            I mean she did claim she did the bare minimum but would have liked more C code changes? Obviously the changes aren't released. What do you expect? I'm just taking her word.
            Except you have missed something. Linux kernel is requiring bare minimum changes to support rust. This is because Linux kernel C is not pure C. Sparse and gcc plugins that the Linux kernel development users forces all the Linux kernel C code to have rust like features.

            This is also brings up why a lot of rust developer proposed C changes in Linux kernel don't raise cause any questions because the changes to support rust happen to be this code was not correct in the first place by the existing Linux kernel documentation on how things should be done, Of course sections that are under-documented have caused some issues.

            For the Linux kernel there is no real loss to having this Rust work. It will bring missing documentation problems out and have interfaces re-looked at.

            Originally posted by Weasel View Post
            IAnd by "Rust sucks" I mean "forcing you into a specific design, even with other APIs". A powerful language should be able to cleanly deal with any interfaces at all. Just like C can deal with C++ interfaces without issues (except name mangling will be terrible). Even though C has no notion of C++ and was designed before it, so it's not like they "dealt with it later". Here's an example of such.

            I mean... Rust technically can, but people hate using "unsafe", so there's that.
            This is not as simple as one would think.
            I noticed linking against wlroots fails when using a C++ compiler. Including the headers with extern "C" { ... } makes this (expectedly) work. Would it be a bad idea to use this common macro around...


            Trying to use a Library with rust that happens to have issues using it with C++. There are even keyword conflits between wlroots code and C++.

            Weasel you were not thinking that you can create interfaces in C that C++ cannot use. wlroots in fact manages to pull that off in places. I would believe you would not be thinking that c code could be this bad. Most times C library developers make sure their libraries are at least C++ compatible there are rare exceptions that don't and wlroots is one of the rare exceptions.. Yes you have a person complained how bad making wlroots work with rust is this and to me this is why did the person expect any different you have core library that is successfully C++ incompatible in places things are going to be bad.

            The more complex a rust wrapper is for a third party library written in C the more likely that library is using undefined behavior or poorly defined design and possible both. Those making gtk wrappers for rust their code is way less than the rust wrapper required for wlroots. Yes GTK is a lot bigger library in function count and code base.

            So you do have to be careful of examples here Weasel wlroots is not a particularly good one due to how nasty it really is. GTK is sane because early on they created gobject being a object standard so OOP in C does not turn into a horrible nightmare to implement in other languages. wlroots C OOP without a solid OOP design when ever you hit a C library with this property attempt to use it with other languages is going to hurt and some languages it going to hurt a lot more. Yes the Linux kernel is C OOP with defined OOP this is what you want to see so you only need to solve the OOP problem once when connecting in a different language not 100+ of times due to stacks of different quirks yes it this feature why rust wrappers for the Linux kernel are also fairly light.

            Basically rust wrapper being insanely complex to make so is safe on a C library should be raise alarm bells as this normally means a design issue either the person writing the rust wrapper has screwed up badly or the C library contains lot of horrible things that could cause problems in future.

            Yes I do agree that rust unsafe should be less hated because unsafe rust does not disable all the rust detection for programmers making stupid mistakes. So a program written in unsafe rust will still normally be safer code than program written in C or C++. But developers attempting to make rust safe wrappers do tell us something critical about the library particularly when they fail to make a wrapper. Some places unsafe rust code may be totally the right thing.

            Rust bindings to libbpf from the Linux kernel. Contribute to libbpf/libbpf-sys development by creating an account on GitHub.


            With rust currently is like above yes you can use bindgen to automatically make a C to rust interface one problem this is never going to generate a safe rust interface done the current way..

            C headers are able to contain extra information for C++ and for static code checkers and so on. So there is no reason why C headers could not contain extra information for rust interface generation to use. Yes extra information for rust put into C headers could be used to static validation of C code. There is a possible win win here.





            Comment


            • #36
              Originally posted by oiaohm View Post
              Trying to use a Library with rust that happens to have issues using it with C++. There are even keyword conflits between wlroots code and C++.
              Who cares about keywords lmao, that's a source code problem that can be easily worked around.

              Originally posted by oiaohm View Post
              Weasel you were not thinking that you can create interfaces in C that C++ cannot use.
              Such as? extern "C" is all you need.

              Please don't say "argument names" using reserved keywords in C++. Those aren't in the binary, i.e. you can literally just change them, and have it work. It's just a way to refer to them. For example if an argument name is "this" you can just change it to "This" (capital T) and have it work just fine, identical to before. It compiles to the same thing.

              Can even do it without changing the source:
              Code:
              #define this This
              extern "C" {
              #include "c_lib_header.h"
              }
              #undef this
              Last edited by Weasel; 10 March 2023, 10:58 AM.

              Comment


              • #37
                Originally posted by Developer12 View Post
                It's not. The existing way the C drivers do it is just dumb.

                It needed an unsafe block because the C code in other drivers (that the rust code would have to mimic) is itself stupid and error-prone. If you're pushing for a more reliable driver anyway (this one went from single-threaded to multi-thread multi-application far easier than any C driver ever did) then it makes sense to take the opportunity to clean some of this other junk up too.

                This happens all the time in mesa. There will be some task that many drivers need to do, and all of them will produce separate buggy implementations of it. Then eventually someone puts in the effort to implement it properly in the common code and each driver is converted to use the common implementation. Now if other drivers want to do it this way, they can.
                Maybe. I'm not familiar with the code, but it does strike me as odd that nobody else had a need for this until a Rust driver came along. So... hence my assumption.

                Comment


                • #38
                  Originally posted by Weasel View Post
                  Please don't say "argument names" using reserved keywords in C++. Those aren't in the binary, i.e. you can literally just change them, and have it work. It's just a way to refer to them. For example if an argument name is "this" you can just change it to "This" (capital T) and have it work just fine, identical to before. It compiles to the same thing.​
                  What happens when it core macros in the headers that run straight into the C++ reserve words... Welcome to wlroots creativity in places. They did not stop at just plain function names.

                  Of course they included the classic NULL equal type void* by Macro that is forbid in C++ standard even inside extern "C".

                  Yes wlroots different versions are really good classic examples how to mess anyone trying to build C code with C++ compiler instead of a C compiler with macro stuff that extern "C" does not fix.

                  Also using capital changes to fix overlapping functions is starting to turn your code into obfuscated code. So dig yourself out of one problem only to make your code base have poor readability so leading to more programming errors. Like are you really sure when reading over the code base that you would get the difference between This and this right all the time and if you do everyone else working with you if the project you are working on grows.

                  Originally posted by Weasel View Post
                  Who cares about keywords lmao, that's a source code problem that can be easily worked around.
                  Are you sure its going to be that easy when you have just picked something that end up being a classical example is the program knows C and only cares about C so has no issue doing every feature that is C only spread over the complete code base. Welcome to game of wack a mole where new stuff comes in with every update.

                  Comment


                  • #39
                    Originally posted by Weasel View Post
                    Maybe. I'm not familiar with the code, but it does strike me as odd that nobody else had a need for this until a Rust driver came along. So... hence my assumption.
                    You have to be very careful with assumes. By the reported issues against the Linux kernel different drivers have had problems of calling drm_sched_fini with jobs still active leading to kernel panic. Everyone proceed to patch their driver code instead of altering drm_sched_fini so the event was not fatal kernel panic. "Yes we do not alter the core functions just in case someone else depends on it and the alteration causes a problem mindset at work applies to the prior fixes"

                    Rust developer of the AGX driver comes in looks at the function and goes this is dumb this function is unsafe by rust standard lets do alteration then look at all users in the kernel and see if it has any adverse effects notice no then submit the alteration.

                    Rust programmer nature to code quality over status quo is a good thing they apply to their C code reviews. So something that has been worked around in 50+ locations in the Linux kernel the rust developer here just submitted a possible generic code once fix every single time.

                    Weasel nobody else has need it that a dangerous assume. You could say you have given screws to a people who only knows how to use hammers and nails and you found them using hammers to drive in screws instead of using the supplied screwdriver to explain what was going on with the Linux kernel in this section. Person only knows they need better in a lot of cases if they know better exists. This is why having a few rust people looking at the Linux kernel code is a good thing.

                    Weasel you started with the idea that Rust had to be bad. There are cases were rust is bad but you have to be very careful there are cases that rust methods being applied to C just make bad C usage show out like nobody business so far all the rust developer modifications to C code appear to be they found bad C and that was front and centre to them due to rust background they also did not just keep on working around the bad C. Yes fixed C code cross all the drivers could possible get simpler as well as supporting rust.
                    Last edited by oiaohm; 10 March 2023, 11:37 AM.

                    Comment


                    • #40
                      Originally posted by oiaohm View Post
                      What happens when it core macros in the headers that run straight into the C++ reserve words... Welcome to wlroots creativity in places. They did not stop at just plain function names.

                      Of course they included the classic NULL equal type void* by Macro that is forbid in C++ standard even inside extern "C".

                      Yes wlroots different versions are really good classic examples how to mess anyone trying to build C code with C++ compiler instead of a C compiler with macro stuff that extern "C" does not fix.

                      Also using capital changes to fix overlapping functions is starting to turn your code into obfuscated code. So dig yourself out of one problem only to make your code base have poor readability so leading to more programming errors. Like are you really sure when reading over the code base that you would get the difference between This and this right all the time and if you do everyone else working with you if the project you are working on grows.

                      Are you sure its going to be that easy when you have just picked something that end up being a classical example is the program knows C and only cares about C so has no issue doing every feature that is C only spread over the complete code base. Welcome to game of wack a mole where new stuff comes in with every update.
                      You're missing the point. My example was that in many cases you don't even have to modify the source header.

                      Obviously, modifying the header should be done in some cases. In Rust, you can't even use C headers in the first place, you have to write your own compatible one with compatible interface. Heck you even have "asm" name for functions (GCC extension) in worst situation with symbol, if you can't use the linker to change it.

                      The point is that C++ can interface with C perfectly and I challenge you to give me an example where it can't. The reverse is also true, C can also interface with C++, even if it's a bit painful with C++ libraries, but it can. Yes, you might have to write your own header in C++, so? It's still much easier than Rust and Rust CANNOT do some of it without "unsafe", which rustaceans have a phobia of.

                      Comment

                      Working...
                      X