| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DFQ16QS1RAEL.1Z0ESXE0MBA7G@garyguo.net>
Date: Fri, 16 Jan 2026 13:00:35 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Dirk Behme"
<dirk.behme@...il.com>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl"
<aliceryhl@...gle.com>, "Maarten Lankhorst"
<maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
"Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
<airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Miguel Ojeda"
<ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
Hindborg" <a.hindborg@...nel.org>, "Trevor Gross" <tmgross@...ch.edu>,
"Steven Price" <steven.price@....com>, <dri-devel@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust/drm: tyr: Convert to the register!() macro
On Fri Jan 16, 2026 at 12:23 PM GMT, Daniel Almeida wrote:
> Hi Dirk, thanks for the review!
>
>> On 15 Jan 2026, at 14:05, Dirk Behme <dirk.behme@...il.com> wrote:
>>
>> Hi Daniel,
>>
>> On 14.01.26 23:53, Daniel Almeida wrote:
>>> Replace regs::Register with kernel::register. This allow us to more
>>> succinctly express the register set by introducing the ability to describe
>>> fields and their documentation and to auto-generate the accessors. In
>>> particular, this is very helpful as it does away with a lot of manual masks
>>> and shifts.
>>
>>
>> As mentioned somewhere else already I really like switching to
>> register!(). Thanks!
>>
>> Some coments below:
>>
>>
>>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>>> reads and writes in kernel::register.
>>>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>>> ---
>>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>>
>>> That version is stale, so I ended up rebasing it locally myself for the
>>> purpose of developing this patch and gathering some reviews on the list. In
>>> other words, the current patch does not apply for the time being, but will
>>> once a v7 for Joel's series is out.
>>>
>>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>> ---
>>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>>> index 0389c558c036..8e06db5320bf 100644
>>> --- a/drivers/gpu/drm/tyr/driver.rs
>>> +++ b/drivers/gpu/drm/tyr/driver.rs
>>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>>> unsafe impl Sync for TyrData {}
>>>
>>> fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
>>> - regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
>>> + let io = iomem.access(dev)?;
>>> +
>>> + regs::GpuCommand::default()
>>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>>> + .write(io);
>>>
>>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>>
>>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>>> + let rawstat = regs::GpuIrqRawstat::read(io);
>>> + if !rawstat.reset_completed() {
>>> dev_err!(dev, "GPU reset failed with errno\n");
>>> - dev_err!(
>>> - dev,
>>> - "GPU_INT_RAWSTAT is {}\n",
>>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>>> - );
>>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>>
>>
>> This is pre-existing, but printing `... INT ...` for `...IRQ...`
>> register looks confusing (wrong?).
>
> Yeah, this needs to change indeed.
>
>>
>>
>>> return Err(EIO);
>>> }
>>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>>> index 6c582910dd5d..7c698fb1e36a 100644
>>> --- a/drivers/gpu/drm/tyr/gpu.rs
>>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>>
>>> impl GpuInfo {
>>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>>> -
>>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>>> -
>>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>>> -
>>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>>> + let io = (*iomem).access(dev)?;
>>> +
>>> + let gpu_id = regs::GpuId::read(io).into();
>>> + let csf_id = regs::CsfId::read(io).into();
>>> + let gpu_rev = regs::RevIdr::read(io).into();
>>> + let core_features = regs::CoreFeatures::read(io).into();
>>> + let l2_features = regs::L2Features::read(io).into();
>>> + let tiler_features = regs::TilerFeatures::read(io).into();
>>> + let mem_features = regs::MemFeatures::read(io).into();
>>> + let mmu_features = regs::MmuFeatures::read(io).into();
>>> + let thread_features = regs::ThreadFeatures::read(io).into();
>>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>>
>>
>> Is there any reason why you replace the UPPERCASE register names with
>> CamelCase ones?
>>
>> I was under the impression that we want to use UPPERCASE for register
>> names. Like in nova
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>
> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
> discussions, their registers are automatically generated from some internal
> docs.
>
> We have only a few, we can simply convert them to CamelCase.
Frankly, register names do look nicer in UPPER_CASE, especially that they're in
many cases, packed with acronyms.
Best,
Gary
>>
>>
>>
>>> + let texture_features = regs::TextureFeatures::read(io, 0).into();
>>> +
>>> + let as_present = regs::AsPresent::read(io).into();
>>> +
>>> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
>>> let shader_present =
>>> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
>>> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
Powered by blists - more mailing lists