Re: [PATCH] Partially revert "rust: drm: gem: Implement AlwaysRefCounted for all gem objects automatically"

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Jul 25, 2025 at 9:41 PM CEST, Lyude Paul wrote:
> a-ha, ok. I made a mistake here with misremembering where the compilation
> issue I saw here really was.
>
> It's not that multiple gem object implementations are triggering it, it's that
> it immediately breaks compilation if any other type tries to do a blanket
> implementation with AlwaysRefCounted like this.
>
> Here's a properly compiling example with rvkms:
>
> https://gitlab.freedesktop.org/lyudess/linux/-/commits/rvkms-slim
>
> This builds fine because IntoGEMObject is the only one with a blanket
> implementation of AlwaysRefCounted, and we implement AlwaysRefCounted using a
> macro for refcounted Kms objects.
>
> But if we apply this patch which adds the second blanket impl:
>
> https://gitlab.freedesktop.org/lyudess/linux/-/commit/ec094d4fc209a7122b00168e7293f365fe7fc16c
>
> Then compilation fails:
>
>    ➜  nouveau-gsp git:(rvkms-slim) ✗ nice make -j20
>      DESCEND objtool
>      DESCEND bpf/resolve_btfids
>      CALL    scripts/checksyscalls.sh
>      INSTALL libsubcmd_headers
>      INSTALL libsubcmd_headers
>      RUSTC L rust/kernel.o
>    warning: unused import: `pin_init`
>      --> rust/kernel/drm/driver.rs:18:5
>       |
>    18 | use pin_init;
>       |     ^^^^^^^^
>       |
>       = note: `#[warn(unused_imports)]` on by default
>    
>    warning: unused import: `prelude::*`
>     --> rust/kernel/drm/kms/modes.rs:4:13
>      |
>    4 | use crate::{prelude::*, types::Opaque};
>      |             ^^^^^^^^^^
>    
>    error[E0119]: conflicting implementations of trait `types::AlwaysRefCounted`
>       --> rust/kernel/drm/kms.rs:504:1
>        |
>    504 | unsafe impl<T: RcModeObject> AlwaysRefCounted for T {
>        | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation
>        |
>       ::: rust/kernel/drm/gem/mod.rs:97:1
>        |
>    97  | unsafe impl<T: IntoGEMObject> AlwaysRefCounted for T {
>        | ---------------------------------------------------- first implementation here
>    
>    warning: unused import: `Sealed`
>     --> rust/kernel/drm/kms/vblank.rs:7:44
>      |
>    7 | use super::{crtc::*, ModeObject, modes::*, Sealed};
>      |                                            ^^^^^^
>    
>    error: aborting due to 1 previous error; 3 warnings emitted
>    
>    For more information about this error, try `rustc --explain E0119`.
>    make[2]: *** [rust/Makefile:538: rust/kernel.o] Error 1
>    make[1]: *** [/home/lyudess/Projects/linux/worktrees/nouveau-gsp/Makefile:1280: prepare] Error 2
>    make: *** [Makefile:248: __sub-make] Error 2
>
> This is definitely part of the reason I didn't notice this problem until later
> too. My understanding is that this is a result of rust's orphan rule, which
> basically just disallows trait impls where it would be ambiguous which impl
> applies to a specific type. Here, the issue is that there's nothing stopping a
> type from implementing both RcModeObject and IntoGEMObject.

Yeah, this is pretty annoying. I don't think it's related to the orphan rule
though; see also the example in [1].

I think in this case we should just keep the generic
impl<T: IntoGEMObject> AlwaysRefCounted for T and not introduce the blanket one
for T: RcModeObject.

In theory it doesn't matter which one to drop, but I'd rather avoid the revert
and I think there's no reason for both to have the less nice macro solution.

[1] https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=23593da0e5e0ca0d9d2aa654e0c9bde6