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@xxxxxxx/
