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 <[email protected]> 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 <[email protected]> >>> --- >>> 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/[email protected]/ >>> --- >>> 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;
