Re: [PATCH] Introduce Tyr

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Jun 27, 2025 at 07:34:04PM -0300, Daniel Almeida wrote:
> +#[pin_data]
> +pub(crate) struct TyrData {
> +    pub(crate) pdev: ARef<platform::Device>,
> +
> +    #[pin]
> +    clks: Mutex<Clocks>,
> +
> +    #[pin]
> +    regulators: Mutex<Regulators>,
> +
> +    // Some inforation on the GPU. This is mainly queried by userspace (mesa).
> +    pub(crate) gpu_info: GpuInfo,
> +}
> +
> +unsafe impl Send for TyrData {}
> +unsafe impl Sync for TyrData {}

What's the safety justification for those? Why do you need them? The fact that
you seem to need to implement those traits within a driver indicates an issue.

> +fn issue_soft_reset(iomem: &Devres<IoMem<0>>) -> Result<()> {
> +    let irq_enable_cmd = 1 | bit_u32(8);
> +    regs::GPU_CMD.write(iomem, irq_enable_cmd)?;
> +
> +    let op = || regs::GPU_INT_RAWSTAT.read(iomem);
> +    let cond = |raw_stat: &u32| -> bool { (*raw_stat >> 8) & 1 == 1 };
> +    let res = io::poll::read_poll_timeout(
> +        op,
> +        cond,
> +        time::Delta::from_millis(100),
> +        Some(time::Delta::from_micros(20000)),
> +    );
> +
> +    if let Err(e) = res {
> +        pr_err!("GPU reset failed with errno {}\n", e.to_errno());
> +        pr_err!(
> +            "GPU_INT_RAWSTAT is {}\n",
> +            regs::GPU_INT_RAWSTAT.read(iomem)?
> +        );

This is a driver, please use dev_err!().

> +    }
> +
> +    Ok(())
> +}
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <TyrDriver as platform::Driver>::IdInfo,
> +    [
> +        (of::DeviceId::new(c_str!("rockchip,rk3588-mali")), ()),
> +        (of::DeviceId::new(c_str!("arm,mali-valhall-csf")), ())
> +    ]
> +);
> +
> +impl platform::Driver for TyrDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        dev_dbg!(pdev.as_ref(), "Probed Tyr\n");
> +
> +        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> +        let stacks_clk = Clk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> +        let coregroup_clk = Clk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> +
> +        core_clk.prepare_enable()?;
> +        stacks_clk.prepare_enable()?;
> +        coregroup_clk.prepare_enable()?;
> +
> +        let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("mali"))?;
> +        let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("sram"))?;
> +
> +        let resource = pdev.resource_by_index(0).ok_or(EINVAL)?;
> +
> +        let iomem = Arc::new(pdev.iomap_resource(resource)?, GFP_KERNEL)?;

You can do

	let io = iomem.access(pdev.as_ref())?;

which gives you an &IoMem for the whole scope of probe() without any
limitations.

Also, why not use iomap_resource_sized()? Lots of offsets are known at compile
time. This allows you to use infallible accesses, e.g. write() instead of
try_write().

> +
> +        issue_soft_reset(&iomem)?;
> +        gpu::l2_power_on(&iomem)?;
> +
> +        let gpu_info = GpuInfo::new(&iomem)?;
> +        gpu_info.log(pdev);
> +
> +        let platform: ARef<platform::Device> = pdev.into();
> +
> +        let data = try_pin_init!(TyrData {
> +                pdev: platform.clone(),
> +                clks <- new_mutex!(Clocks {
> +                    core: core_clk,
> +                    stacks: stacks_clk,
> +                    coregroup: coregroup_clk,
> +                }),
> +                regulators <- new_mutex!(Regulators {
> +                    mali: mali_regulator,
> +                    sram: sram_regulator,
> +                }),
> +                gpu_info,
> +        });
> +
> +        let data = Arc::pin_init(data, GFP_KERNEL)?;
> +
> +        let tdev: ARef<TyrDevice> = drm::device::Device::new(pdev.as_ref(), data.clone())?;
> +        drm::driver::Registration::new_foreign_owned(&tdev, pdev.as_ref(), 0)?;
> +
> +        let driver = KBox::pin_init(try_pin_init!(TyrDriver { device: tdev }), GFP_KERNEL)?;
> +
> +        regs::MCU_CONTROL.write(&iomem, regs::MCU_CONTROL_AUTO)?;
> +
> +        dev_info!(pdev.as_ref(), "Tyr initialized correctly.\n");

Consider dev_dbg!() instead.

> +    pub(crate) fn log(&self, pdev: &platform::Device) {
> +        let major = (self.gpu_id >> 16) & 0xff;
> +        let minor = (self.gpu_id >> 8) & 0xff;
> +        let status = self.gpu_id & 0xff;
> +
> +        let model_name = if let Some(model) = GPU_MODELS
> +            .iter()
> +            .find(|&f| f.major == major && f.minor == minor)
> +        {
> +            model.name
> +        } else {
> +            "unknown"
> +        };
> +
> +        dev_info!(
> +            pdev.as_ref(),
> +            "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> +            model_name,
> +            self.gpu_id >> 16,
> +            major,
> +            minor,
> +            status
> +        );
> +
> +        dev_info!(
> +            pdev.as_ref(),
> +            "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> +            self.l2_features,
> +            self.tiler_features,
> +            self.mem_features,
> +            self.mmu_features,
> +            self.as_present
> +        );
> +
> +        dev_info!(
> +            pdev.as_ref(),
> +            "shader_present=0x{:016x} l2_present=0x{:016x} tiler_present=0x{:016x}",
> +            self.shader_present,
> +            self.l2_present,
> +            self.tiler_present
> +        );
> +
> +        dev_info!(
> +            pdev.as_ref(),
> +            "PA bits: {}, VA bits: {}",
> +            self.pa_bits(),
> +            self.va_bits()
> +        );
> +    }

This is called from probe() and seems way too verbose for dev_info!(), please
use dev_dbg!() instead.

> +/// Represents a register in the Register Set
> +pub(crate) struct Register<const OFFSET: usize>;
> +
> +impl<const OFFSET: usize> Register<OFFSET> {
> +    #[inline]
> +    pub(crate) fn read(&self, iomem: &Devres<IoMem>) -> Result<u32> {
> +        (*iomem).try_access().ok_or(ENODEV)?.try_read32(OFFSET)
> +    }
> +
> +    #[inline]
> +    pub(crate) fn write(&self, iomem: &Devres<IoMem>, value: u32) -> Result<()> {
> +        (*iomem)
> +            .try_access()
> +            .ok_or(ENODEV)?
> +            .try_write32(value, OFFSET)
> +    }
> +}

This seems like a bad idea. You really want to use Devres::access() from each
entry point where you have a &Device<Bound> (such as probe()) and use the
returned &IoMem instead. Otherwise every read() and write() does an atomic read
and RCU read-side critical section, due to try_access().

If you really run in a case where you don't have a &Device<Bound>, you can use
Devres::try_access_with(), which takes a closure that will have an &IoMem as
argument, such that you can do things like:

	io.try_access_with(|io| my_register.write(io, ...))

Also, you want accessors for read32() and write32() rather than always use
try_read32() and try_write32(). The latter you only want to use when the offset
isn't known at compile time.

I also recommend looking at what nova-core does for register accesses. Regarding
the register!() macro in nova-core, we're working on providing this as generic
infrastructure.