Re: [PATCH] Introduce Tyr

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 30 Jun 2025 18:12:02 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On 6/30/25 6:06 PM, Boris Brezillon wrote:
> > On Sat, 28 Jun 2025 01:12:34 +0200
> > Danilo Krummrich <dakr@kernel.org> wrote:
> >   
> >>> +    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.  
> > 
> > We do have the same level of verbosity in Panthor, and it's proven
> > useful when people are filling bug reports. Asking them to reload
> > the module with debug prints enabled is kinda annoying, and I don't
> > think I've heard anyone complaining that this was too verbose or slowing
> > down the boot, so I'd be tempted to keep it like that, and least for
> > the information printed in this function.  
> 
> Yeah, I think for the GPU revision bits that's reasonable, but do you really
> also need the other prints to be dev_info()? Don't you know this information
> from the combination of the GPU revision bits and the kernel version?

Sure, we could have a tool extracting most of that from the driver info
and DEV_QUERY ioctl(), but those info have been printed in Panfrost
since the early days. I picked those traces up in Panthor because devs
were used to it, and I honestly see no good reason to not print those as
dev_info() in Tyr too. What's your concern here? Is this about boot
time, not bloating the kernel logs or something else? I mean, we're
talking about less than 10 lines printed at boot/module-load-time.