Re: [PATCH v2 05/15] drm/panthor: Add the GPU logical block

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

On Mon, 21 Aug 2023 17:09:49 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2023-08-14 11:54, Steven Price wrote:
> [...]
> >> +/**
> >> + * panthor_gpu_l2_power_on() - Power-on the L2-cache
> >> + * @ptdev: Device.
> >> + *
> >> + * Return: 0 on success, a negative error code otherwise.
> >> + */
> >> +int panthor_gpu_l2_power_on(struct panthor_device *ptdev)
> >> +{
> >> +	u64 core_mask = U64_MAX;
> >> +
> >> +	if (ptdev->gpu_info.l2_present != 1) {
> >> +		/*
> >> +		 * Only support one core group now.
> >> +		 * ~(l2_present - 1) unsets all bits in l2_present except
> >> +		 * the bottom bit. (l2_present - 2) has all the bits in
> >> +		 * the first core group set. AND them together to generate
> >> +		 * a mask of cores in the first core group.
> >> +		 */
> >> +		core_mask = ~(ptdev->gpu_info.l2_present - 1) &
> >> +			     (ptdev->gpu_info.l2_present - 2);
> >> +		drm_info_once(&ptdev->base, "using only 1st core group (%lu cores from %lu)\n",
> >> +			      hweight64(core_mask),
> >> +			      hweight64(ptdev->gpu_info.shader_present));  
> > 
> > I'm not sure what the point of this complexity is. This boils down to
> > the equivalent of:
> > 
> > 	if (ptdev->gpu_info.l2_present != 1)
> > 		core_mask = 1;  
> 
> Hmm, that doesn't look right - the idiom here should be to set all bits 
> of the output below the *second* set bit of the input, i.e. 0x11 -> 
> 0x0f.

Ah ah, I should really read the whole thread before replying :-).

> However since panthor is (somewhat ironically) unlikely to ever 
> run on T628, and everything newer should pretend to have a single L2 
> because software-managed coherency is a terrible idea, I would agree 
> that ultimately it does all seem a bit pointless.

Okay, good to know.

> 
> > If we were doing shader-core power management manually (like on pre-CSF
> > GPUs, rather than letting the firmware control it) then the computed
> > core_mask would be useful. So I guess it comes down to the
> > drm_info_once() output and counting the cores - which is nice to have
> > but it took me some time figuring out what was going on here.  
> As for the complexity, I'd suggest you can have some choice words with 
> the guy who originally suggested that code[1] ;)

:'-)