[PATCH v9 3/5] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Bibek Kumar Patro]

On 5/28/2024 9:38 PM, Rob Clark wrote:
> On Tue, May 28, 2024 at 6:06 AM Dmitry Baryshkov
> <dmitry.baryshkov at linaro.org> wrote:
>>
>> On Tue, May 28, 2024 at 02:59:51PM +0200, Konrad Dybcio wrote:
>>>
>>>
>>> On 5/15/24 15:59, Bibek Kumar Patro wrote:
>>>>
>>>>
>>>> On 5/10/2024 6:32 PM, Konrad Dybcio wrote:
>>>>> On 10.05.2024 2:52 PM, Bibek Kumar Patro wrote:
>>>>>>
>>>>>>
>>>>>> On 5/1/2024 12:30 AM, Rob Clark wrote:
>>>>>>> On Tue, Jan 23, 2024 at 7:00 AM Bibek Kumar Patro
>>>>>>> <quic_bibekkum at quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Currently in Qualcomm  SoCs the default prefetch is set to 1 which allows
>>>>>>>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>>>>>>>> register which is implementation defined and is used for Qualcomm SoCs
>>>>>>>> to have a custom prefetch setting enabling TLB to prefetch the next set
>>>>>>>> of page tables accordingly allowing for faster translations.
>>>>>>>>
>>>>>>>> ACTLR value is unique for each SMR (Stream matching register) and stored
>>>>>>>> in a pre-populated table. This value is set to the register during
>>>>>>>> context bank initialisation.
>>>>>>>>
>>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum at quicinc.com>
>>>>>>>> ---
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +
>>>>>>>> +               for_each_cfg_sme(cfg, fwspec, j, idx) {
>>>>>>>> +                       smr = &smmu->smrs[idx];
>>>>>>>> +                       if (smr_is_subset(smr, id, mask)) {
>>>>>>>> +                               arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR,
>>>>>>>> +                                               actlrcfg[i].actlr);
>>>>>>>
>>>>>>> So, this makes ACTLR look like kind of a FIFO.  But I'm looking at
>>>>>>> downstream kgsl's PRR thing (which we'll need to implement vulkan
>>>>>>> sparse residency), and it appears to be wanting to set BIT(5) in ACTLR
>>>>>>> to enable PRR.
>>>>>>>
>>>>>>>            val = KGSL_IOMMU_GET_CTX_REG(ctx, KGSL_IOMMU_CTX_ACTLR);
>>>>>>>            val |= FIELD_PREP(KGSL_IOMMU_ACTLR_PRR_ENABLE, 1);
>>>>>>>            KGSL_IOMMU_SET_CTX_REG(ctx, KGSL_IOMMU_CTX_ACTLR, val);
>>>>>>>
>>>>>>> Any idea how this works?  And does it need to be done before or after
>>>>>>> the ACTLR programming done in this patch?
>>>>>>>
>>>>>>> BR,
>>>>>>> -R
>>>>>>>
>>>>>>
>>>>>> Hi Rob,
>>>>>>
>>>>>> Can you please help provide some more clarification on the FIFO part? By FIFO are you referring to the storing of ACTLR data in the table?
>>>>>>
>>>>>> Thanks for pointing to the downstream implementation of kgsl driver for
>>>>>> the PRR bit. Since kgsl driver is already handling this PRR bit's
>>>>>> setting, this makes setting the PRR BIT(5) by SMMU driver redundant.
>>>>>
>>>>> The kgsl driver is not present upstream.
>>>>>
>>>>
>>>> Right kgsl is not present upstream, it would be better to avoid configuring the PRR bit and can be handled by kgsl directly in downstream.
>>>
>>> No! Upstream is not a dumping ground to reduce your technical debt.
>>>
>>> There is no kgsl driver upstream, so this ought to be handled here, in
>>> the iommu driver (as poking at hardware A from driver B is usually not good
>>> practice).
>>
>> I'd second the request here. If another driver has to control the
>> behaviour of another driver, please add corresponding API for that.
> 
> We have adreno_smmu_priv for this purpose ;-)
> 

Thanks Rob for pointing to this private interface structure between smmu
and gpu. I think it's similar to what you're trying to implement here
https://lore.kernel.org/all/CAF6AEGtm-KweFdMFvahH1pWmpOq7dW_p0Xe_13aHGWt0jSbg8w@mail.gmail.com/#t
I can add an api "set_actlr_prr()" with smmu_domain cookie, page pointer 
as two parameters. This api then can be used by drm/msm driver to carry 
out the prr implementation by simply calling this.
Would this be okay Rob,Konrad,Dmitry?
Let me know if any other suggestions you have in mind as well regarding
parameters and placement.

Thanks & regards,
Bibek

> BR,
> -R
> 
>>>
>>>>
>>>>>> Thanks for bringing up this point.
>>>>>> I will send v10 patch series removing this BIT(5) setting from the ACTLR
>>>>>> table.
>>>>>
>>>>> I think it's generally saner to configure the SMMU from the SMMU driver..
>>>>
>>>> Yes, agree on this. But since PRR bit is not directly related to SMMU
>>>> configuration so I think it would be better to remove this PRR bit
>>>> setting from SMMU driver based on my understanding.
>>>
>>> Why is it not related? We still don't know what it does.
>>>
>>> Konrad
>>
>> --
>> With best wishes
>> Dmitry