Re: [PATCH] rust/drm: tyr: Convert to the register!() macro

["Gary Guo" <gary@garyguo.net>]

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@gmail.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@collabora.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;