Re: [PATCH] Introduce Tyr

[Danilo Krummrich <dakr@kernel.org>]

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?