Message-ID: <Zv5Wv4wQTYFN3yyu@louis-chauvet-laptop>
Date: Thu, 3 Oct 2024 10:33:03 +0200
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
	Asahi Lina <lina@...hilina.net>, Danilo Krummrich <dakr@...nel.org>,
	mcanal@...lia.com, airlied@...hat.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...hat.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [WIP RFC v2 01/35] WIP: rust/drm: Add fourcc bindings

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@...hat.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.

[...]

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists