Message-Id: <9D9AFE08-2CBB-4A89-866D-512D9080754C@collabora.com>
Date: Fri, 27 Jun 2025 21:12:29 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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>,
 Alex Gaynor <alex.gaynor@...il.com>,
 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>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Daniel Stone <daniels@...labora.com>,
 Rob Herring <robh@...nel.org>,
 Alice Ryhl <alice.ryhl@...gle.com>,
 Beata Michalska <beata.michalska@....com>,
 Carsten Haitzler <carsten.haitzler@...s.arm.com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Ashley Smith <ashley.smith@...labora.com>,
 linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH] Introduce Tyr

Hi Danilo, thank you an Boqun for having a look at this,


> On 27 Jun 2025, at 20:12, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> 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.

This was forgotten when scooped from the downstream code.

Although I think the problematic members are only Clk and Regulator
as Boqun pointed out.

In any case, my bad.

Also, for some reason the Clippy lint did not save me this time.

> 
>> +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().

Right, I did not even consider this. Should be possible indeed.

> 
>> +
>> +        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.

The problem with dev_dbg() is that it doesn't work, as Alex has also found out
recently. There was a thread on fixing it and I guess Tamir(?) or Andrew(?)
came up with a patch, but it hasn't seen any traction. I simply don't think
there is a way to get these to print for now (at least in upstream code)

> 
>> +    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.

Same comment as above. Although I don’t care about these printing.

I think that at this point we just need one dev_info!() at the end of probe,
just to make sure it worked. The rest can be converted to dev_dbg!().

OTOH, IIRC these are indeed printed for Panthor, so maybe Boris can
explain why this would be relevant.

> 
>> +/// 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, ...))

Right, thanks for pointing that out.

> 
> 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.

Oh we’ll definitely switch to the nova macro. We just didn’t get to
work on it yet, and IIUC it's not available atm?

In any case, if you guys post a patch to make the macro available to other
drivers I'll switch to that instead.

— Daniel

Powered by blists - more mailing lists