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

From: Robin Murphy <robin.murphy@arm.com>
To: Steven Price <steven.price@arm.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	dri-devel@lists.freedesktop.org
Cc: "Nicolas Boichat" <drinkcat@chromium.org>,
	"Daniel Stone" <daniels@collabora.com>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Liviu Dudau" <Liviu.Dudau@arm.com>,
	"Clément Péron" <peron.clem@gmail.com>,
	"Marty E . Plummer" <hanetzer@startmail.com>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>
Subject: Re: [PATCH v2 05/15] drm/panthor: Add the GPU logical block
Date: Mon, 21 Aug 2023 17:09:49 +0100	[thread overview]
Message-ID: <13815072-9403-0a9a-e6a8-d97fd727e2a0@arm.com> (raw)
In-Reply-To: <b1c4d260-93a5-d1f0-7f4b-4dcb444e7a24@arm.com>

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

> 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] ;)

Cheers,
Robin.

[1] 
https://lore.kernel.org/dri-devel/b009b4c4-0396-58c2-7779-30c844f36f04@arm.com/