On 21/08/2023 17:09, Robin Murphy 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. 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.
Sorry I should have been clearer here. Other than the message printed
(using drm_info_once) the only use of core_mask in this function is in
the call to panthor_gpu_power_on:
+ return panthor_gpu_power_on(ptdev, L2,
+ ptdev->gpu_info.l2_present & core_mask,
+ 20000);
Here the core_mask variable is anded with l2_present. So using the value
1 is equivalent to the actual core mask which is being calculated.
Obviously '1' isn't likely to be the real core mask (it's an "L2 mask").
Mostly it just seemed odd to calculate the core_mask and then
effectively throw the value away.
>> 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] ;)
I do often have problems with the code that guy wrote ;)
Steve
>
> Cheers,
> Robin.
>
> [1]
> https://lore.kernel.org/dri-devel/b009b4c4-0396-58c2-7779-30c844f36f04@xxxxxxx/
