On Mon, 21 Aug 2023 17:09:49 +0100
Robin Murphy <robin.murphy@xxxxxxx> 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] ;)
:'-)
