| Message ID | 20240227215146.46487-1-daniel.almeida@collabora.com |
|---|---|
| Headers | show |
| Series | Rewrite the VP9 codec library in Rust | expand |
On Tue, Feb 27, 2024 at 10:56 PM Daniel Almeida <daniel.almeida@collabora.com> wrote: > > +#[no_mangle] > +pub extern "C" fn v4l2_vp9_seg_feat_enabled_rs( > + feature_enabled: &mut u8, > + feature: u32, > + segid: u32, > +) -> bool { > + let feature_enabled = unsafe { core::slice::from_raw_parts_mut(feature_enabled, 8) }; > + let mask = 1 << feature; > + feature_enabled[segid as usize] & mask != 0 > +} This seems to be the only unsafe block. Impressive! I recommend taking an `*mut u8` here instead of `&mut u8` if you're going to use `slice::from_raw_parts_mut`. Or, if cbindgen allows it, take an `&mut [u8; 8]` instead and eliminate the unsafe block. Alice
Hi Alice! On Wednesday, February 28, 2024 11:13 -03, Alice Ryhl <aliceryhl@google.com> wrote: > I recommend taking an `*mut u8` here instead of `&mut u8` if you're > going to use `slice::from_raw_parts_mut`. Or, if cbindgen allows it, > take an `&mut [u8; 8]` instead and eliminate the unsafe block. You're right, the signature here can change to `&mut [u8; 8]` instead. This means no unsafe blocks at all. Thanks for the tip, -- Daniel
Hi Daniel, On Thu, Mar 07, 2024 at 04:08:14PM -0300, Daniel Almeida wrote: > Hi Mauro, Hans, > > While working on v1 for this patchset, I realized that we can go further by > converting the error-prone sections of our codec drivers to Rust. This also > does not need any bindings in order to work. > > As yet another proof-of-concept, I have converted parts of the rkvdec driver. > Refer to instructions in v1 to test this. I tested this on rk3399 rkvdec with: ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -ts VP9-TEST-VECTORS -j1 And I get the same result as on mainline (6.8-rc3) Ran 220/305 tests successfully in 913.877 secs So I can't say I understand all the Rust here but my testing didn't show any regressions in the VP9 decoding. :) Deb > > Notice how: > > 1) many problematic memcpy's go away, these become a simple assignment in Rust. > > 2) it can interop seamlessly with the code in rkvdec-vp9.c that was already > converted in v1 of this series. > > 3) it can use the Rust version of `seg_feat_enabled` directly in vp9.rs, while > also using the C APIs from the v4l2-vp9-rs library in rkvdec-vp9.c > > 4) more modern things become available for the programmer, like iterators and > their methods without a performance penalty. > > I want to propose the following: > > Let's merge a non-RFC version of this series and gate it behind some kconfigs > so that we can switch between the C and Rust implementations. Users get the C > version by default, while we continuously test the Rust components on a CI for > a few months. This will hopefully be enough time until the next Media Summit. > > My aim is to eventually deprecate the C parts once we're confident that the > Rust code is stable enough. I will keep my own tree, and send PRs to the media > tree if a rebase or fix is needed. > > I believe this will not be disruptive nor require any extra work from anyone > but me. > > -- Daniel > > > Again, applies on top of: > > commit d9c1fae3e5b225f2e45e0bca519f9a2967cd1062 > Author: Alice Ryhl <aliceryhl@google.com> > Date: Fri Feb 9 11:18:22 2024 +0000 > > rust: file: add abstraction for `poll_table` > > For those looking for a branch instead: https://gitlab.collabora.com/dwlsalmeida/for-upstream/-/tree/vp9-rs-rkvdec?ref_type=heads > > Daniel Almeida (2): > v4l2-core: rewrite the VP9 library in Rust > media: rkvdec: rewrite parts of the driver in Rust > > drivers/media/platform/verisilicon/Kconfig | 2 +- > .../platform/verisilicon/hantro_g2_vp9_dec.c | 38 +- > .../media/platform/verisilicon/hantro_hw.h | 8 +- > drivers/media/v4l2-core/Kconfig | 5 + > drivers/staging/media/rkvdec/Kconfig | 3 +- > drivers/staging/media/rkvdec/Makefile | 2 +- > drivers/staging/media/rkvdec/cbindgen.toml | 36 + > drivers/staging/media/rkvdec/common.rs | 19 + > drivers/staging/media/rkvdec/regs.rs | 237 ++ > drivers/staging/media/rkvdec/rkvdec-vp9.c | 607 +---- > drivers/staging/media/rkvdec/rkvdec_rs.h | 125 + > drivers/staging/media/rkvdec/rkvdec_rs.rs | 14 + > drivers/staging/media/rkvdec/vp9.rs | 636 +++++ > include/media/v4l2-vp9-rs.h | 99 + > rust/bindings/bindings_helper.h | 1 + > rust/helpers.c | 7 + > rust/kernel/lib.rs | 2 + > rust/kernel/media.rs | 5 + > rust/kernel/media/v4l2_core.rs | 6 + > rust/kernel/media/v4l2_core/cbindgen.toml | 26 + > rust/kernel/media/v4l2_core/vp9.rs | 2053 +++++++++++++++++ > 21 files changed, 3415 insertions(+), 516 deletions(-) > create mode 100644 drivers/staging/media/rkvdec/cbindgen.toml > create mode 100644 drivers/staging/media/rkvdec/common.rs > create mode 100644 drivers/staging/media/rkvdec/regs.rs > create mode 100644 drivers/staging/media/rkvdec/rkvdec_rs.h > create mode 100644 drivers/staging/media/rkvdec/rkvdec_rs.rs > create mode 100644 drivers/staging/media/rkvdec/vp9.rs > create mode 100644 include/media/v4l2-vp9-rs.h > create mode 100644 rust/kernel/media.rs > create mode 100644 rust/kernel/media/v4l2_core.rs > create mode 100644 rust/kernel/media/v4l2_core/cbindgen.toml > create mode 100644 rust/kernel/media/v4l2_core/vp9.rs > > -- > 2.43.0 > >
Hi Daniel, as I'm already sold to the idea, I decided to discuss other things ;-P see below. Le jeudi 07 mars 2024 à 16:08 -0300, Daniel Almeida a écrit : > +++ b/drivers/staging/media/rkvdec/regs.rs > @@ -0,0 +1,237 @@ > +#![allow(dead_code)] > +#![allow(unused_macros)] > + > +pub(crate) const RKVDEC_REG_INTERRUPT: u32 = 0x004; > +pub(crate) const RKVDEC_INTERRUPT_DEC_E: u32 = 1 << 0; > +pub(crate) const RKVDEC_CONFIG_DEC_CLK_GATE_E: u32 = 1 << 1; > +pub(crate) const RKVDEC_E_STRMD_CLKGATE_DIS: u32 = 1 << 2; Regs file are a bit our coded reference information on how the registers are layout in memory. So I believe the alignment, indent and readability of that file would at least need polishing. But to the light of your comment, being able to use more modern utility, isn't there something in Rust we could use to better map the registers ? These variables are just mask offset to help someone write specific bits within a list of 32bit registers (Hantro and RKVDEC have that in common). In downstream mpp userspace driver, they maps all the register with a C struct. struct reg123 { val1 :3 // bit 31-29 val2 :20 // bit 28-9 val3 :9 // bit 8-0 }; I seriously think it looks nicer, and when the compiler does not screw it up (the main reason we don't use that), it is also a lot safer and simpler to use. Now, lets forget about C, my question is just if there is something in Rust that could give us the safety to edit the right portion of a register, but also allow expressing that map in a readable form. Note that sometimes, we may want to read the register before editing it, something MPP giant C struct does not help with. Nicolas
Hi Nicolas, > struct reg123 { > val1 :3 // bit 31-29 > val2 :20 // bit 28-9 > val3 :9 // bit 8-0 > }; What you're describing can be modeled as Ranges in Rust: ``` use core::ops::Range; struct Foo { reg1: Range<u32>, reg2: Range<u32>, reg3: Range<u32> } const FOO_REGMAP: Foo = Foo { reg1: 0..3, reg2: 3..24, reg3: 24..32 }; ``` It becomes more useful when you pair that with a bit writer. For an example of previous art, see Faith's work: [0] This has asserts in the right places so that you do not shoot yourself in the foot. IMHO, such a data structure can be shared with the whole Rust code in the kernel. You can then describe your writes using the ranges, e.g.: [1] But as we've established, instead of writing the ranges down directly, you can simply refer to them as FOO_REGMAP.reg1, FOO_REGMAP.reg2 and so on. I believe this is both more readable and safer. [0]: https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/nouveau/compiler/bitview/lib.rs?ref_type=heads [1]: https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/nouveau/compiler/nak/encode_sm70.rs?ref_type=heads#L228
Hi everyone, This patch ports the VP9 library written by Andrzej into Rust as a proof-of-concept. This is so that we can evaluate the Rust in V4L2 initiative with source code in hand. It converts rkvdec and hantro to use the Rust version of the library. These two were chosen merely because that's the hardware I currently own. Fluster scores remain the same when using either the Rust or C version. Please test this. Much has been spoken at various occasions about drivers and I feel that the consensus is to wait for now. This is why I am proposing a different approach: porting our codec libraries to Rust. IMHO, these components can benefit greatly from Rust, as they implement complicated algorithms that also happen to process data received from userspace through V4L2 controls. These algorithms use the data received from userspace in order to index into a lot of arrays and thus benefit from Rust's memory safety. The first thing about the code is that it does not include any layer of bindings. This was pointed out as a blocker several times due to the need of keeping them in sync with the C code. The Rust code here also offers a C API for C drivers.This C API is automatically generated by cbindgen and I have provided instructions on how to do so. We can even use functions from both the C and Rust libraries at the same time since the ABI is the same. The above can come in handy because it means that we can convert a given codebase piece by piece if need be. C drivers will work as usual through the C API and any new Rust driver will get to benefit from a native Rust interface. Please take note that most of the code is *not* in the media tree, so I do not see how this can further stress our subsystem. I take responsibility for maintaining stuff in rust/kernel/media and honestly, this library will not need any further updates for the same reason we have never touched its C counterpart. I hope to convince the community that this is a feasible way to slowly experiment with Rust code without tying us up too much to it. Lastly, please note that this code is just a proof of concept, we can settle on a proper patch - with all that entails - if it is well received. Those with hardware can follow the steps below to test this patch: a) enable Rust (https://www.kernel.org/doc/html/latest/rust/quick-start.html). Make sure CONFIG_RUST=y. b) enable one of the converted drivers (CONFIG_VIDEO_HANTRO or CONFIG_VIDEO_ROCKCHIP_VDEC). This will select V4L2_VP9_RS. c) download the Fluster tool (https://github.com/fluendo/fluster) d) download the VP9 test suite from libvpx (fluster.py download VP9-TEST-VECTORS) e) make sure you have a recent version of GStreamer (`gst-inspect-1.0 v4l2codecs` must not be empty) f) run the test suite (fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -ts VP9-TEST-VECTORS) g) results should be the same both with and without this patch -- Daniel Applies on top of: commit d9c1fae3e5b225f2e45e0bca519f9a2967cd1062 Author: Alice Ryhl <aliceryhl@google.com> Date: Fri Feb 9 11:18:22 2024 +0000 rust: file: add abstraction for `poll_table` For those looking for a branch instead: https://gitlab.collabora.com/dwlsalmeida/for-upstream/-/tree/vp9-rs?ref_type=heads Daniel Almeida (1): v4l2-core: rewrite the VP9 library in Rust drivers/media/platform/verisilicon/Kconfig | 2 +- .../platform/verisilicon/hantro_g2_vp9_dec.c | 38 +- .../media/platform/verisilicon/hantro_hw.h | 8 +- drivers/media/v4l2-core/Kconfig | 4 + drivers/staging/media/rkvdec/Kconfig | 2 +- drivers/staging/media/rkvdec/rkvdec-vp9.c | 52 +- include/media/v4l2-vp9-rs.h | 97 + rust/bindings/bindings_helper.h | 1 + rust/kernel/lib.rs | 2 + rust/kernel/media.rs | 5 + rust/kernel/media/v4l2_core.rs | 6 + rust/kernel/media/v4l2_core/cbindgen.toml | 26 + rust/kernel/media/v4l2_core/vp9.rs | 1999 +++++++++++++++++ 13 files changed, 2192 insertions(+), 50 deletions(-) create mode 100644 include/media/v4l2-vp9-rs.h create mode 100644 rust/kernel/media.rs create mode 100644 rust/kernel/media/v4l2_core.rs create mode 100644 rust/kernel/media/v4l2_core/cbindgen.toml create mode 100644 rust/kernel/media/v4l2_core/vp9.rs