On Thu Mar 12, 2026 at 8:03 AM JST, Deborah Brouwer wrote:
> From: Daniel Almeida <[email protected]>
>
> Convert the GPU_CONTROL register definitions to use the `register!` macro.
>
> Using the `register!` macro allows us to replace manual bit masks and
> shifts with typed register and field accessors, which makes the code
> easier to read and avoids errors from bit manipulation.
>
> Signed-off-by: Daniel Almeida <[email protected]>
> Co-developed-by: Deborah Brouwer <[email protected]>
> Signed-off-by: Deborah Brouwer <[email protected]>
> ---
> drivers/gpu/drm/tyr/driver.rs | 26 +-
> drivers/gpu/drm/tyr/gpu.rs | 211 ++++++--------
> drivers/gpu/drm/tyr/regs.rs | 644
> ++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 687 insertions(+), 194 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index
> 611434641580574ec6b5afa49a8fe79888bb7ace..10c212a3a01910858f02c6d637edff8a263f017b
> 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -13,7 +13,10 @@
> devres::Devres,
> drm,
> drm::ioctl,
> - io::poll,
> + io::{
> + poll,
> + Io, //
> + },
> new_mutex,
> of,
> platform,
> @@ -33,8 +36,11 @@
> file::TyrDrmFileData,
> gem::TyrObject,
> gpu,
> - gpu::GpuInfo,
> - regs, //
> + gpu::{
> + gpu_info_log, //
> + GpuInfo,
> + },
> + regs::*, //
> };
>
> pub(crate) type IoMem = kernel::io::mem::IoMem<SZ_2M>;
> @@ -78,11 +84,17 @@ unsafe impl Send for TyrDrmDeviceData {}
> unsafe impl Sync for TyrDrmDeviceData {}
>
> 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)?;
> + io.write_val(
> + GPU_COMMAND_RESET::zeroed().with_const_reset_mode::<{
> GPU_COMMAND_RESET::SOFT_RESET }>(),
> + );
My biggest piece of feedback for this patchset: replace the const values
with enums and use the `?=>` (or `=>` where possible) syntax to directly
convert your fields from and into them. It associates each field with
its possible set of values and guarantees you won't use a given constant
with the wrong field (and also that you cannot set invalid field values
at all).
It is a bit cumbersome at the moment because you will need to provide
`TryFrom` and `Into` implementations between the enum and the
corresponding bounded type, but it makes setting fields easier and is
the way the API was designed to be used. The `TryFrom/Into` derive macro
work will remove all the boilerplace once it is merged.
>
> poll::read_poll_timeout(
> - || regs::GPU_IRQ_RAWSTAT.read(dev, iomem),
> - |status| *status & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED != 0,
> + || {
> + let io = (*iomem).access(dev)?;
> + Ok(io.read(GPU_IRQ_RAWSTAT))
> + },
> + |status| status.reset_completed().get() != 0,
Here you can do the following in the declaration of `GPU_IRQ_RAWSTAT`:
/// Reset has completed.
8:8 reset_completed => bool;
and change this line to just
|status| status.reset_completed(),
You will probably want to do that for most of the single-bit fields,
unless their semantic is different from a boolean value.
An alternative is to use `into_bool` instead of `get` to at least get a
boolean value from the single-bit field.
<snip>
> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) ->
> Result {
> + let io = (*iomem).access(dev)?;
> + let gpu_id = io.read(GPU_ID);
> +
> + let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
> + f.arch_major == gpu_id.arch_major().get() && f.prod_major ==
> gpu_id.prod_major().get()
> + }) {
> + model.name
> + } else {
> + "unknown"
> + };
> +
> + // Create canonical product ID with only arch/product fields, excluding
> version
> + // fields. This ensures the same product at different revisions has the
> same ID.
> + let id = GPU_ID::zeroed()
> + .with_arch_major(gpu_id.arch_major())
> + .with_arch_minor(gpu_id.arch_minor())
> + .with_arch_rev(gpu_id.arch_rev())
> + .with_prod_major(gpu_id.prod_major())
> + .into_raw();
It might be simpler to just clear the values of the fields you don't want.
> +
> + dev_info!(
> + dev,
> + "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> + model_name,
> + id,
> + gpu_id.ver_major().get(),
> + gpu_id.ver_minor().get(),
> + gpu_id.ver_status().get()
> + );
Note that the `Debug` implementation of register types will display
their raw hex value and the individual values of all their fields - you
might want to leverage that.
> +
> + dev_info!(
> + dev,
> + "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> + io.read(L2_FEATURES).into_raw(),
> + io.read(TILER_FEATURES).into_raw(),
> + io.read(MEM_FEATURES).into_raw(),
> + io.read(MMU_FEATURES).into_raw(),
> + io.read(AS_PRESENT).into_raw(),
> + );
> +
> + dev_info!(
> + dev,
> + "shader_present=0x{:016x} l2_present=0x{:016x}
> tiler_present=0x{:016x}",
> + io.read(SHADER_PRESENT).into_raw(),
> + io.read(L2_PRESENT).into_raw(),
> + io.read(TILER_PRESENT).into_raw(),
> + );
> + Ok(())
> }
>
> /// Powers on the l2 block.
> pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &Devres<IoMem>) ->
> Result {
> - regs::L2_PWRON_LO.write(dev, iomem, 1)?;
> + let io = (*iomem).access(dev)?;
> + io.write_val(L2_PWRON::zeroed().with_const_request::<1>());
>
> poll::read_poll_timeout(
> - || regs::L2_READY_LO.read(dev, iomem),
> + || {
> + let io = (*iomem).access(dev)?;
> + Ok(io.read(L2_READY).into_raw())
> + },
> |status| *status == 1,
Why not
poll::read_poll_timeout(
|| regs::L2_READY_LO.read(dev, iomem),
|| {
let io = (*iomem).access(dev)?;
Ok(io.read(L2_READY))
},
|status| status.ready() == 1,
?
<snip>
> + /// Layout is interpreted differently depending on the command value.
> + /// Default command is [`GPU_COMMAND::NOP`] with no payload.
> + pub(crate) GPU_COMMAND (u32) @ 0x30 {
> + 7:0 command;
> + }
> + }
> +
> + impl GPU_COMMAND {
> + /// No operation. This is the default value.
> + pub(crate) const NOP: u32 = 0;
> + /// Reset the GPU.
> + pub(crate) const RESET: u32 = 1;
> + /// Flush caches.
> + pub(crate) const FLUSH_CACHES: u32 = 4;
> + /// Clear GPU faults.
> + pub(crate) const CLEAR_FAULT: u32 = 7;
> + }
So this should really be turned into an enum:
#[derive(Copy, Clone, Debug)]
#[repr(u32)]
enum GpuCommand {
Nop = 0,
Reset = 1,
FlushCaches = 4,
ClearFault = 7,
}
impl TryFrom<Bounded<u32, 8>> for GpuCommand {
...
}
impl From<GpuCommand> for Bounded<u32, 8> {
...
}
Then `GPU_COMMAND` becomes:
pub(crate) GPU_COMMAND (u32) @ 0x30 {
7:0 command ?=> GpuCommand;
}
... and everything becomes easier, as you can only set valid commands.
I see you also define aliases that reassign the roles of bitfields
depending on the command. I think you can harden that by having an
`impl` block for `GPU_COMMAND` with constructor methods for each command
taking exactly the arguments it needs. These constructors could then
build the proper value using one of the aliases register, otherwise you
are at risk of setting e.g. the `Reset` command on the
`GPU_COMMAND_FLUSH` alias.
> +
> + register! {
> + /// GPU command register in reset mode.
> + /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
> + pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> + 7:0 command;
> + 11:8 reset_mode;
> + }
> + }
> +
> + impl GPU_COMMAND_RESET {
> + /// Stop all external bus interfaces, then reset the entire GPU.
> + pub(crate) const SOFT_RESET: u32 = 1;
> + /// Force a full GPU reset.
> + pub(crate) const HARD_RESET: u32 = 2;
> + }
> +
> + register! {
> + /// GPU command register in cache flush mode.
> + /// Set command to [`GPU_COMMAND::FLUSH_CACHES`] to set flush modes.
> + pub(crate) GPU_COMMAND_FLUSH (u32) => GPU_COMMAND {
> + 7:0 command;
> + /// L2 cache flush mode.
> + 11:8 l2_flush;
> + /// Shader core load/store cache flush mode.
> + 15:12 lsc_flush;
> + /// Shader core other caches flush mode.
> + 19:16 other_flush;
> + }
> + }
> +
> + impl GPU_COMMAND_FLUSH {
> + /// No flush.
> + pub(crate) const NONE: u32 = 0;
> + /// Clean the caches.
> + pub(crate) const CLEAN: u32 = 1;
> + /// Invalidate the caches.
> + pub(crate) const INVALIDATE: u32 = 2;
> + /// Clean and invalidate the caches.
> + pub(crate) const CLEAN_INVALIDATE: u32 = 3;
> + }
> +
> + register! {
> + /// GPU status register. Read only.
> + pub(crate) GPU_STATUS(u32) @ 0x34 {
> + /// GPU active.
> + 0:0 gpu_active;
> + /// Power manager active.
> + 1:1 pwr_active;
> + /// Page fault active.
> + 4:4 page_fault;
> + /// Protected mode active.
> + 7:7 protected_mode_active;
> + /// Debug mode active.
> + 8:8 gpu_dbg_enabled;
> + }
> +
> + /// GPU fault status register. Read only.
> + pub(crate) GPU_FAULTSTATUS(u32) @ 0x3c {
> + /// Exception type.
> + 7:0 exception_type;
> + /// Access type.
> + 9:8 access_type;
> + /// The GPU_FAULTADDRESS is valid.
> + 10:10 address_valid;
> + /// The JASID field is valid.
> + 11:11 jasid_valid;
> + /// JASID of the fault, if known.
> + 15:12 jasid;
> + /// ID of the source that triggered the fault.
> + 31:16 source_id;
> + }
> + }
> +
> + impl GPU_FAULTSTATUS {
> + /// Exception type: No error.
> + pub(crate) const EXCEPTION_OK: u32 = 0x00;
> + /// Exception type: GPU external bus error.
> + pub(crate) const EXCEPTION_GPU_BUS_FAULT: u32 = 0x80;
> + /// Exception type: GPU shareability error.
> + pub(crate) const EXCEPTION_GPU_SHAREABILITY_FAULT: u32 = 0x88;
> + /// Exception type: System shareability error.
> + pub(crate) const EXCEPTION_SYSTEM_SHAREABILITY_FAULT: u32 = 0x89;
> + /// Exception type: GPU cacheability error.
> + pub(crate) const EXCEPTION_GPU_CACHEABILITY_FAULT: u32 = 0x8A;
> +
> + /// Access type: An atomic (read/write) transaction.
> + pub(crate) const ACCESS_ATOMIC: u32 = 0;
> + /// Access type: An execute transaction.
> + pub(crate) const ACCESS_EXECUTE: u32 = 1;
> + /// Access type: A read transaction.
> + pub(crate) const ACCESS_READ: u32 = 2;
> + /// Access type: A write transaction.
> + pub(crate) const ACCESS_WRITE: u32 = 3;
Since these consts cover all the possible values of `access_type`, you
can use the `=>` syntax for it and implement `From<Bounded<u32, 2>>`
instead of `TryFrom`. This will make the getter infallible as there are
no invalid values.
> + }
> +
> + register! {
> + /// GPU fault address. Read only.
> + /// Once a fault is reported, it must be manually cleared by issuing
> a
> + /// [`GPU_COMMAND::CLEAR_FAULT`] command to the [`GPU_COMMAND`]
> register. No further GPU
> + /// faults will be reported until the previous fault has been
> cleared.
> + pub(crate) GPU_FAULTADDRESS(u64) @ 0x40 {
> + 63:0 pointer;
> + }
> +
> + /// Level 2 cache configuration.
> + pub(crate) L2_CONFIG(u32) @ 0x48 {
> + /// Requested cache size.
> + 23:16 cache_size;
> + /// Requested hash function index.
> + 31:24 hash_function;
> + }
> +
> + /// Power state key. Write only.
> + pub(crate) PWR_KEY(u32) @ 0x50 {
> + /// Set to [`PWR_KEY::KEY_UNLOCK`] to unlock writes to other
> power state registers.
> + 31:0 key;
> + }
> + }
> +
> + impl PWR_KEY {
> + /// Key value to unlock writes to other power state registers.
> + /// This value was generated at random.
> + pub(crate) const KEY_UNLOCK: u32 = 0x2968A819;
Note that you can also create constants of the register type directly,
so you don't need to reconstruct one using this value.
> + }
> +
> + register! {
> + /// Power manager override settings.
> + pub(crate) PWR_OVERRIDE0(u32) @ 0x54 {
> + /// Override the PWRUP signal.
> + 1:0 pwrup_override;
> + /// Override the ISOLATE signal.
> + 3:2 isolate_override;
> + /// Override the RESET signal.
> + 5:4 reset_override;
> + /// Override the PWRUP_ACK signal.
> + 9:8 pwrup_ack_override;
> + /// Override the ISOLATE_ACK signal.
> + 11:10 isolate_ack_override;
> + /// Override the FUNC_ISOLATE signal.
> + 13:12 func_iso_override;
> + /// Override the FUNC_ISOLATE_ACK signal.
> + 15:14 func_iso_ack_override;
> + /// Maximum number of power transitions.
> + 21:16 pwrtrans_limit;
> + /// Core startup throttling enabled.
> + 23:23 throttle_enable;
> + /// Maximum number of simultaneous core startups.
> + 29:24 throttle_limit;
> + }
> + }
> +
> + /// Power override mode constants (`pwr_override_t` in hardware spec).
> + ///
> + /// These constants can be used with any field in [`PWR_OVERRIDE0`] that
> ends with
> + /// the `_override` suffix.
> + impl PWR_OVERRIDE0 {
> + /// The signal behaves normally.
> + pub(crate) const NONE: u32 = 0;
> + /// The signal is inverted (on when normally off, and off when
> normally on).
> + pub(crate) const INVERT: u32 = 1;
> + /// The signal is always kept on.
> + pub(crate) const ON: u32 = 2;
> + /// The signal is always kept off.
> + pub(crate) const OFF: u32 = 3;
> + }
> +
> + register! {
> + /// Power manager override settings for device manufacturer.
> + pub(crate) PWR_OVERRIDE1(u32) @ 0x58 {
> + 31:0 pwrtrans_vendor;
> + }
> +
> + /// Global time stamp offset.
> + pub(crate) TIMESTAMP_OFFSET(u64) @ 0x88 {
> + 63:0 offset;
> + }
> +
> + /// GPU cycle counter. Read only.
> + pub(crate) CYCLE_COUNT(u64) @ 0x90 {
> + 63:0 count;
> + }
> +
> + /// Global time stamp. Read only.
> + pub(crate) TIMESTAMP(u64) @ 0x98 {
> + 63:0 timestamp;
> + }
> +
> + /// Maximum number of threads per core. Read only constant.
> + pub(crate) THREAD_MAX_THREADS(u32) @ 0xa0 {
> + 31:0 threads;
> + }
> +
> + /// Maximum number of threads per workgroup. Read only constant.
> + pub(crate) THREAD_MAX_WORKGROUP_SIZE(u32) @ 0xa4 {
> + 31:0 threads;
> + }
> +
> + /// Maximum number of threads per barrier. Read only constant.
> + pub(crate) THREAD_MAX_BARRIER_SIZE(u32) @ 0xa8 {
> + 31:0 threads;
> + }
> +
> + /// Thread features. Read only constant.
> + pub(crate) THREAD_FEATURES(u32) @ 0xac {
> + /// Total number of registers per core.
> + 21:0 max_registers;
> + /// Implementation technology type.
> + 23:22 implementation_technology;
Here as well you will probably want an enum type for the values.
