| Message ID | 20240930233257.1189730-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust bindings for KMS + RVKMS | expand |
On Mon, 30 Sep 2024, Lyude Paul <lyude@redhat.com> wrote: > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index b2e05f8c2ee7d..04898f70ef1b8 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -9,6 +9,7 @@ > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > #include <drm/drm_file.h> > +#include <drm/drm_fourcc.h> > #include <drm/drm_gem.h> > #include <drm/drm_gem_shmem_helper.h> > #include <drm/drm_ioctl.h> Unrelated to the patch, sorry, but... what's the idea with putting all the bindings in the same file? Does it mean every time any of the files or their dependencies get changed, *all* the rust bindings get regenerated? Should there be more granularity? BR, Jani.
On Tue, Oct 1, 2024 at 11:26 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > regenerated? Should there be more granularity? Indeed, eventually this will need to be split, like we did for `helpers.c`. Cheers, Miguel
Hi Lyude, Le 30/09/24 - 19:09, Lyude Paul a écrit : > This adds some very basic rust bindings for fourcc. We only have a single > format code added for the moment, but this is enough to get a driver > registered. > > TODO: > * Write up something to automatically generate constants from the fourcc > headers > > Signed-off-by: Lyude Paul <lyude@redhat.com> [...] > +#[derive(Copy, Clone)] > +#[repr(C)] > +pub struct FormatList<const COUNT: usize> { > + list: [u32; COUNT], > + _sentinel: u32, > +} > + > +impl<const COUNT: usize> FormatList<COUNT> { > + /// Create a new [`FormatList`] > + pub const fn new(list: [u32; COUNT]) -> Self { > + Self { > + list, > + _sentinel: 0 > + } > + } Can you explain what does the sentinel here? I don't think the DRM core requires this sentinel, and you don't use it in your API. > + /// Returns the number of entries in the list, including the sentinel. > + /// > + /// This is generally only useful for passing [`FormatList`] to C bindings. > + pub const fn raw_len(&self) -> usize { > + COUNT + 1 > + } > +} I don't think the C side requires to have this extra 0 field. For example in "C"VKMS there is no such "sentinel" at the end of the list [1]. Do you think I need to add one in VKMS? [1]:https://elixir.bootlin.com/linux/v6.11.1/source/drivers/gpu/drm/vkms/vkms_plane.c#L15 > +impl<const COUNT: usize> Deref for FormatList<COUNT> { > + type Target = [u32; COUNT]; > + > + fn deref(&self) -> &Self::Target { > + &self.list > + } > +} > + > +impl<const COUNT: usize> DerefMut for FormatList<COUNT> { > + fn deref_mut(&mut self) -> &mut Self::Target { > + &mut self.list > + } > +} > + > +#[derive(Copy, Clone)] > +#[repr(C)] > +pub struct ModifierList<const COUNT: usize> { > + list: [u64; COUNT], > + _sentinel: u64 > +} Same here [...] > +impl FormatInfo { > + // SAFETY: `ptr` must point to a valid instance of a `bindings::drm_format_info` > + pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_format_info) -> &'a Self { > + // SAFETY: Our data layout is identical > + unsafe { &*ptr.cast() } > + } > + > + /// The number of color planes (1 to 3) > + pub const fn num_planes(&self) -> u8 { > + self.inner.num_planes > + } > + > + /// Does the format embed an alpha component? > + pub const fn has_alpha(&self) -> bool { > + self.inner.has_alpha > + } > + > + /// The total number of components (color planes + alpha channel, if there is one) > + pub const fn num_components(&self) -> u8 { > + self.num_planes() + self.has_alpha() as u8 > + } I don't understand this "num_components" and why the alpha channel is added to the result? For me a component could be "plane count" or "color channels count", but your function is not returning any of this. For example in the table [1], BGRA5551 have 4 color components (R, G, B and A), but only have one plane, so your function will return two, what does this two means? [1]:https://elixir.bootlin.com/linux/v6.11.1/source/drivers/gpu/drm/drm_fourcc.c#L147 > + /// Number of bytes per block (per plane), where blocks are defined as a rectangle of pixels > + /// which are stored next to each other in a byte aligned memory region. > + pub fn char_per_block(&self) -> &[u8] { > + // SAFETY: The union we access here is just for descriptive purposes on the C side, both > + // members are identical in data layout > + unsafe { &self.inner.__bindgen_anon_1.char_per_block[..self.num_components() as _] } > + } > +} And here, I think there is an issue, again with BGRA5551 for example, one plane, with alpha channel, you are returning a slice with two members, instead of only one. [...]
On Thu, 2024-10-03 at 10:33 +0200, Louis Chauvet wrote: > Hi Lyude, > > Le 30/09/24 - 19:09, Lyude Paul a écrit : > > This adds some very basic rust bindings for fourcc. We only have a single > > format code added for the moment, but this is enough to get a driver > > registered. > > > > TODO: > > * Write up something to automatically generate constants from the fourcc > > headers > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > [...] > > > +#[derive(Copy, Clone)] > > +#[repr(C)] > > +pub struct FormatList<const COUNT: usize> { > > + list: [u32; COUNT], > > + _sentinel: u32, > > +} > > + > > +impl<const COUNT: usize> FormatList<COUNT> { > > + /// Create a new [`FormatList`] > > + pub const fn new(list: [u32; COUNT]) -> Self { > > + Self { > > + list, > > + _sentinel: 0 > > + } > > + } > > Can you explain what does the sentinel here? I don't think the DRM core > requires this sentinel, and you don't use it in your API. > > > + /// Returns the number of entries in the list, including the sentinel. > > + /// > > + /// This is generally only useful for passing [`FormatList`] to C bindings. > > + pub const fn raw_len(&self) -> usize { > > + COUNT + 1 > > + } > > +} > > I don't think the C side requires to have this extra 0 field. For example > in "C"VKMS there is no such "sentinel" at the end of the list [1]. Do you > think I need to add one in VKMS? > > [1]:https://elixir.bootlin.com/linux/v6.11.1/source/drivers/gpu/drm/vkms/vkms_plane.c#L15 Ah good catch - honestly what likely happened is I just got the two arguments mixed up with each other. Confusingly: the first formats argument does not require a sentinel, but the modifier list does require a sentinel. I would fix this but I think we already concluded we don't need either FormatList or ModifierList if we just use array slices so it shouldn't be an issue next patch version. > > > +impl<const COUNT: usize> Deref for FormatList<COUNT> { > > + type Target = [u32; COUNT]; > > + > > + fn deref(&self) -> &Self::Target { > > + &self.list > > + } > > +} > > + > > +impl<const COUNT: usize> DerefMut for FormatList<COUNT> { > > + fn deref_mut(&mut self) -> &mut Self::Target { > > + &mut self.list > > + } > > +} > > + > > +#[derive(Copy, Clone)] > > +#[repr(C)] > > +pub struct ModifierList<const COUNT: usize> { > > + list: [u64; COUNT], > > + _sentinel: u64 > > +} > > Same here Format modifiers does need a sentinel: if (format_modifiers) { const uint64_t *temp_modifiers = format_modifiers; while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID) format_modifier_count++; } else { if (!dev->mode_config.fb_modifiers_not_supported) { format_modifiers = default_modifiers; format_modifier_count = ARRAY_SIZE(default_modifiers); } } And 0 should be equivalent to DRM_FORMAT_MOD_INVALID, though I shouldn't have hardcoded that value. > > [...] > > > +impl FormatInfo { > > + // SAFETY: `ptr` must point to a valid instance of a `bindings::drm_format_info` > > + pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_format_info) -> &'a Self { > > + // SAFETY: Our data layout is identical > > + unsafe { &*ptr.cast() } > > + } > > + > > + /// The number of color planes (1 to 3) > > + pub const fn num_planes(&self) -> u8 { > > + self.inner.num_planes > > + } > > + > > + /// Does the format embed an alpha component? > > + pub const fn has_alpha(&self) -> bool { > > + self.inner.has_alpha > > + } > > + > > + /// The total number of components (color planes + alpha channel, if there is one) > > + pub const fn num_components(&self) -> u8 { > > + self.num_planes() + self.has_alpha() as u8 > > + } > > I don't understand this "num_components" and why the alpha channel > is added to the result? For me a component could be "plane count" or > "color channels count", but your function is not returning any of this. > > For example in the table [1], BGRA5551 have 4 color components (R, G, B > and A), but only have one plane, so your function will return two, what > does this two means? > > [1]:https://elixir.bootlin.com/linux/v6.11.1/source/drivers/gpu/drm/drm_fourcc.c#L147 Ah yeah - you're right, I will make sure to fix this as well. > > > + /// Number of bytes per block (per plane), where blocks are defined as a rectangle of pixels > > + /// which are stored next to each other in a byte aligned memory region. > > + pub fn char_per_block(&self) -> &[u8] { > > + // SAFETY: The union we access here is just for descriptive purposes on the C side, both > > + // members are identical in data layout > > + unsafe { &self.inner.__bindgen_anon_1.char_per_block[..self.num_components() as _] } > > + } > > +} > > And here, I think there is an issue, again with BGRA5551 for example, one > plane, with alpha channel, you are returning a slice with two members, > instead of only one. > > [...] >
Hi Lyude, sorry for the late review! > On 30 Sep 2024, at 20:09, Lyude Paul <lyude@redhat.com> wrote: > > This adds some very basic rust bindings for fourcc. We only have a single > format code added for the moment, but this is enough to get a driver > registered. > > TODO: > * Write up something to automatically generate constants from the fourcc > headers I assume this is blocked on [0], right? > > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/drm/fourcc.rs | 127 ++++++++++++++++++++++++++++++++ > rust/kernel/drm/mod.rs | 1 + > 3 files changed, 129 insertions(+) > create mode 100644 rust/kernel/drm/fourcc.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index b2e05f8c2ee7d..04898f70ef1b8 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -9,6 +9,7 @@ > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > #include <drm/drm_file.h> > +#include <drm/drm_fourcc.h> > #include <drm/drm_gem.h> > #include <drm/drm_gem_shmem_helper.h> > #include <drm/drm_ioctl.h> > diff --git a/rust/kernel/drm/fourcc.rs b/rust/kernel/drm/fourcc.rs > new file mode 100644 > index 0000000000000..b80eba99aa7e4 > --- /dev/null > +++ b/rust/kernel/drm/fourcc.rs > @@ -0,0 +1,127 @@ > +use bindings; > +use core::{ops::*, slice, ptr}; > + > +const fn fourcc_code(a: u8, b: u8, c: u8, d: u8) -> u32 { > + (a as u32) | (b as u32) << 8 | (c as u32) << 16 | (d as u32) << 24 > +} > + > +// TODO: Figure out a more automated way of importing this > +pub const XRGB888: u32 = fourcc_code(b'X', b'R', b'2', b'4'); > + > +#[derive(Copy, Clone)] > +#[repr(C)] > +pub struct FormatList<const COUNT: usize> { > + list: [u32; COUNT], > + _sentinel: u32, > +} > + > +impl<const COUNT: usize> FormatList<COUNT> { > + /// Create a new [`FormatList`] > + pub const fn new(list: [u32; COUNT]) -> Self { > + Self { > + list, > + _sentinel: 0 > + } > + } > + > + /// Returns the number of entries in the list, including the sentinel. > + /// > + /// This is generally only useful for passing [`FormatList`] to C bindings. > + pub const fn raw_len(&self) -> usize { > + COUNT + 1 > + } > +} > + > +impl<const COUNT: usize> Deref for FormatList<COUNT> { > + type Target = [u32; COUNT]; > + > + fn deref(&self) -> &Self::Target { > + &self.list > + } > +} > + > +impl<const COUNT: usize> DerefMut for FormatList<COUNT> { > + fn deref_mut(&mut self) -> &mut Self::Target { > + &mut self.list > + } > +} > + > +#[derive(Copy, Clone)] > +#[repr(C)] > +pub struct ModifierList<const COUNT: usize> { > + list: [u64; COUNT], > + _sentinel: u64 > +} > + > +impl<const COUNT: usize> ModifierList<COUNT> { > + /// Create a new [`ModifierList`] > + pub const fn new(list: [u64; COUNT]) -> Self { > + Self { > + list, > + _sentinel: 0 > + } > + } > +} > + > +impl<const COUNT: usize> Deref for ModifierList<COUNT> { > + type Target = [u64; COUNT]; > + > + fn deref(&self) -> &Self::Target { > + &self.list > + } > +} > + > +impl<const COUNT: usize> DerefMut for ModifierList<COUNT> { > + fn deref_mut(&mut self) -> &mut Self::Target { > + &mut self.list > + } > +} > + > +#[repr(transparent)] > +#[derive(Copy, Clone)] > +pub struct FormatInfo { > + inner: bindings::drm_format_info, > +} > + > +impl FormatInfo { > + // SAFETY: `ptr` must point to a valid instance of a `bindings::drm_format_info` > + pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_format_info) -> &'a Self { I think FormatInfoRef would be more appropriate, since you seem to be creating a reference type (IIUC) for a type that can also be owned. This would be more in line with the GEM [1] patch, for example. In other words, using `Ref` here will allow for both an owned `FormatInfo` and a `FormatInfoRef<‘_>`. I am not sure about the role of lifetime ‘a here. If you wanted to tie the lifetime of &Self to that of the pointer, this does not do it, specially considering that pointers do not have lifetimes associated with them. > + // SAFETY: Our data layout is identical > + unsafe { &*ptr.cast() } It’s hard to know what is going on with both the reborrow and the cast in the same statement. I am assuming that cast() is transforming to *Self, and the reborrow to &Self. To be honest, I dislike this approach. My suggestion here is to rework it to be similar to, e.g., what Alice did here for `ShrinkControl` [2]. +/// This struct is used to pass information from page reclaim to the shrinkers. +/// +/// # Invariants +/// +/// `ptr` has exclusive access to a valid `struct shrink_control`. +pub struct ShrinkControl<'a> { + ptr: NonNull<bindings::shrink_control>, + _phantom: PhantomData<&'a bindings::shrink_control>, +} + +impl<'a> ShrinkControl<'a> { + /// Create a `ShrinkControl` from a raw pointer. + /// + /// # Safety + /// + /// The pointer should point at a valid `shrink_control` for the duration of 'a. + pub unsafe fn from_raw(ptr: *mut bindings::shrink_control) -> Self { + Self { + // SAFETY: Caller promises that this pointer is valid. + ptr: unsafe { NonNull::new_unchecked(ptr) }, + _phantom: PhantomData, + } + } Notice the use of PhantomData in her patch. By the way, Alice, I wonder if we can just use Opaque here? > + } > + > + /// The number of color planes (1 to 3) > + pub const fn num_planes(&self) -> u8 { > + self.inner.num_planes > + } > + > + /// Does the format embed an alpha component? > + pub const fn has_alpha(&self) -> bool { > + self.inner.has_alpha > + } > + > + /// The total number of components (color planes + alpha channel, if there is one) > + pub const fn num_components(&self) -> u8 { > + self.num_planes() + self.has_alpha() as u8 > + } > + > + /// Number of bytes per block (per plane), where blocks are defined as a rectangle of pixels > + /// which are stored next to each other in a byte aligned memory region. > + pub fn char_per_block(&self) -> &[u8] { > + // SAFETY: The union we access here is just for descriptive purposes on the C side, both > + // members are identical in data layout > + unsafe { &self.inner.__bindgen_anon_1.char_per_block[..self.num_components() as _] } > + } > +} > + > +impl AsRef<bindings::drm_format_info> for FormatInfo { > + fn as_ref(&self) -> &bindings::drm_format_info { > + &self.inner > + } > +} > + > +impl From<bindings::drm_format_info> for FormatInfo { > + fn from(value: bindings::drm_format_info) -> Self { > + Self { inner: value } > + } > +} > diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs > index c44760a1332fa..2c12dbd181997 100644 > --- a/rust/kernel/drm/mod.rs > +++ b/rust/kernel/drm/mod.rs > @@ -5,5 +5,6 @@ > pub mod device; > pub mod drv; > pub mod file; > +pub mod fourcc; > pub mod gem; > pub mod ioctl; > -- > 2.46.1 — Daniel [0]: https://github.com/rust-lang/rust-bindgen/issues/753 [1]: https://gitlab.freedesktop.org/drm/nova/-/commit/cfd66f531af29e0616c58b4cd4c72770a5ac4081#71321381cbaa87053942373244bffe230e69392a_0_306 [2]: https://lore.kernel.org/rust-for-linux/20241014-shrinker-v2-1-04719efd2342@google.com/
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index b2e05f8c2ee7d..04898f70ef1b8 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -9,6 +9,7 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_fourcc.h> #include <drm/drm_gem.h> #include <drm/drm_gem_shmem_helper.h> #include <drm/drm_ioctl.h> diff --git a/rust/kernel/drm/fourcc.rs b/rust/kernel/drm/fourcc.rs new file mode 100644 index 0000000000000..b80eba99aa7e4 --- /dev/null +++ b/rust/kernel/drm/fourcc.rs @@ -0,0 +1,127 @@ +use bindings; +use core::{ops::*, slice, ptr}; + +const fn fourcc_code(a: u8, b: u8, c: u8, d: u8) -> u32 { + (a as u32) | (b as u32) << 8 | (c as u32) << 16 | (d as u32) << 24 +} + +// TODO: Figure out a more automated way of importing this +pub const XRGB888: u32 = fourcc_code(b'X', b'R', b'2', b'4'); + +#[derive(Copy, Clone)] +#[repr(C)] +pub struct FormatList<const COUNT: usize> { + list: [u32; COUNT], + _sentinel: u32, +} + +impl<const COUNT: usize> FormatList<COUNT> { + /// Create a new [`FormatList`] + pub const fn new(list: [u32; COUNT]) -> Self { + Self { + list, + _sentinel: 0 + } + } + + /// Returns the number of entries in the list, including the sentinel. + /// + /// This is generally only useful for passing [`FormatList`] to C bindings. + pub const fn raw_len(&self) -> usize { + COUNT + 1 + } +} + +impl<const COUNT: usize> Deref for FormatList<COUNT> { + type Target = [u32; COUNT]; + + fn deref(&self) -> &Self::Target { + &self.list + } +} + +impl<const COUNT: usize> DerefMut for FormatList<COUNT> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.list + } +} + +#[derive(Copy, Clone)] +#[repr(C)] +pub struct ModifierList<const COUNT: usize> { + list: [u64; COUNT], + _sentinel: u64 +} + +impl<const COUNT: usize> ModifierList<COUNT> { + /// Create a new [`ModifierList`] + pub const fn new(list: [u64; COUNT]) -> Self { + Self { + list, + _sentinel: 0 + } + } +} + +impl<const COUNT: usize> Deref for ModifierList<COUNT> { + type Target = [u64; COUNT]; + + fn deref(&self) -> &Self::Target { + &self.list + } +} + +impl<const COUNT: usize> DerefMut for ModifierList<COUNT> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.list + } +} + +#[repr(transparent)] +#[derive(Copy, Clone)] +pub struct FormatInfo { + inner: bindings::drm_format_info, +} + +impl FormatInfo { + // SAFETY: `ptr` must point to a valid instance of a `bindings::drm_format_info` + pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_format_info) -> &'a Self { + // SAFETY: Our data layout is identical + unsafe { &*ptr.cast() } + } + + /// The number of color planes (1 to 3) + pub const fn num_planes(&self) -> u8 { + self.inner.num_planes + } + + /// Does the format embed an alpha component? + pub const fn has_alpha(&self) -> bool { + self.inner.has_alpha + } + + /// The total number of components (color planes + alpha channel, if there is one) + pub const fn num_components(&self) -> u8 { + self.num_planes() + self.has_alpha() as u8 + } + + /// Number of bytes per block (per plane), where blocks are defined as a rectangle of pixels + /// which are stored next to each other in a byte aligned memory region. + pub fn char_per_block(&self) -> &[u8] { + // SAFETY: The union we access here is just for descriptive purposes on the C side, both + // members are identical in data layout + unsafe { &self.inner.__bindgen_anon_1.char_per_block[..self.num_components() as _] } + } +} + +impl AsRef<bindings::drm_format_info> for FormatInfo { + fn as_ref(&self) -> &bindings::drm_format_info { + &self.inner + } +} + +impl From<bindings::drm_format_info> for FormatInfo { + fn from(value: bindings::drm_format_info) -> Self { + Self { inner: value } + } +} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index c44760a1332fa..2c12dbd181997 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -5,5 +5,6 @@ pub mod device; pub mod drv; pub mod file; +pub mod fourcc; pub mod gem; pub mod ioctl;
This adds some very basic rust bindings for fourcc. We only have a single format code added for the moment, but this is enough to get a driver registered. TODO: * Write up something to automatically generate constants from the fourcc headers Signed-off-by: Lyude Paul <lyude@redhat.com> --- rust/bindings/bindings_helper.h | 1 + rust/kernel/drm/fourcc.rs | 127 ++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 1 + 3 files changed, 129 insertions(+) create mode 100644 rust/kernel/drm/fourcc.rs