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@xxxxxxxxxx>
[...]
> +#[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.
[...]
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
