Re: [PATCH v5 1/5] drm/msm/adreno: Implement SMEM-based speed bin

[Akhil P Oommen <quic_akhilpo@quicinc.com>]

On 4/25/2025 2:35 PM, Konrad Dybcio wrote:
> On 7/15/24 10:04 PM, Akhil P Oommen wrote:
>> On Tue, Jul 09, 2024 at 12:45:29PM +0200, Konrad Dybcio wrote:
>>> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
>>> abstracted through SMEM, instead of being directly available in a fuse.
>>>
>>> Add support for SMEM-based speed binning, which includes getting
>>> "feature code" and "product code" from said source and parsing them
>>> to form something that lets us match OPPs against.
>>>
>>> Due to the product code being ignored in the context of Adreno on
>>> production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
> 
> [...]
> 
>>>  
>>>  int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>> @@ -1102,9 +1136,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>  			devm_pm_opp_set_clkname(dev, "core");
>>>  	}
>>>  
>>> -	if (adreno_read_speedbin(dev, &speedbin) || !speedbin)
>>> +	if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin)
>>>  		speedbin = 0xffff;
>>> -	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
>>> +	adreno_gpu->speedbin = speedbin;
>>
>> There are some chipsets which uses both Speedbin and Socinfo data for
>> SKU detection [1]. We don't need to worry about that logic for now. But
>> I am worried about mixing Speedbin and SKU_ID in the UABI with this patch.
>> It will be difficult when we have to expose both to userspace.
>>
>> I think we can use a separate bitfield to expose FCODE/PCODE. Currently,
>> the lower 32 bit is reserved for chipid and 33-48 is reserved for speedbin,
>> so I think we can use the rest of the 16 bits for SKU_ID. And within that
>> 16bits, 12 bits should be sufficient for FCODE and the rest 8 bits
>> reserved for future PCODE.
> 
> So I revisited this patchset and had another thought - maybe we could just
> extend the speedbin bitfield to cover [63:32] and worry about filling these
> higher bits in the future, when adding the platforms you mentioned?

Sorry for the late response, I needed sometime to think this through.

My concern is about mixing speedbin and socinfo in the same UABI. It
will be complicated to handle this correctly later. We have to track the
set of chipsets which exposes socinfo data via speedbin fields and those
do not. I think it is be simpler to handle this correctly from the start.

If you are worried about breaking backward compatibility with Mesa, we
can probably use a separate property for socinfo data which can be
implemented later? Just don't expose socinfo via speedbin UABI.

> 
> Do we still have plans to use *both* SMEM and fuse encoding on them?

It is better to keep that option open since there are chipsets using
both production. And there will be for foreseeable future because
automotive team stick with fuse register and mobile team stick with
socinfo data.

-Akhil

> 
> Konrad