[PATCH v9 3/5] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
Bibek Kumar Patro
quic_bibekkum at quicinc.com
Mon Jun 10 03:12:53 PDT 2024
On 6/6/2024 3:43 AM, Rob Clark wrote:
> On Wed, Jun 5, 2024 at 3:52 AM Bibek Kumar Patro
> <quic_bibekkum at quicinc.com> wrote:
>>
>> On 6/5/2024 12:19 AM, Rob Clark wrote:
>>> On Thu, May 30, 2024 at 2:22 AM Bibek Kumar Patro
>>> <quic_bibekkum at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>>
>>> Hey Bibek, quick question.. is ACTLR preserved across a suspend/resume
>>> cycle? Or does it need to be reprogrammed on resume? And same
>>> question for these two PRR related regs:
>>>
>>> /* Global SMMU register offsets */
>>> #define KGSL_IOMMU_PRR_CFG_LADDR 0x6008
>>> #define KGSL_IOMMU_PRR_CFG_UADDR 0x600c
>>>
>>> (ie. high/low 32b of the PRR page)
>>>
>>
>> Hey Rob, In suspend/resume, the register space power rails are not in
>> disabled state, so it won't go back to reset values and should retain
>> it's value. Only in hibernation cycle the registers' value would get reset.
>>
>> So the hi/low address bit register for PRR page would also retain it's
>> value along with the ACTLR registers.
>>
>>> I was starting to type up a patch to add PRR configuration, but
>>> depending on whether it interacts with suspend/resume, it might be
>>> better form arm-smmu-qcom.c to just always enable and configure PRR
>>> (including allocating a page to have an address to program into
>>> PRR_CFG_LADDR/UADDR), and instead add an interface to return the PRR
>>> page? I think there is no harm in unconditionally configuring PRR for
>>> gpu smmu.
>>
>> Sounds okay though since this would not interact with suspend/resume path.
>> But I think, suppose in-case this page would have some other references
>> as well before configuring the address to the registers for PRR
>> configuration, then GPU would be dependent on arm-smmu-qcom for this page.
>> So Instead an endpoint api in arm-smmu-qcom.c can recieve the just the
>> page-address, and bit set status from drm/msm driver and can set/reset
>> the bit along with any page-address they want ?
>> It would mean the interface will be smmu's , but the choice of
>> configuration data to the registers' will be still with gpu.
>>
>> I wrote up a small patch with this implementation, would you like to
>> review that?
>> Will send it in this v11 series as new patch.
>
> I think if there is no suspend/resume interaction, we should go back
> to the original idea of page allocation in drm/msm.
>
> Basically, I think the pros and cons are:
>
> allocate in arm-smmu
> pro: easy to sequence programming with suspend/resume
> con: there isn't a convenient place to free the page on driver unload
>
> allocate in drm/msm:
> pro: easy place to free the page in teardown
> con: harder to sequence with s/r
>
> But if ACTLR and PRR_CFG_LADDR/UADDR are retained, then the con isn't
> actually an issue ;-)
>
Sounds right, also in this case the ownership of the page stays with
drm/msm which might also make it easy to handle the page for them.
> Anyways, I can type that patch.. the rest of drm/msm and userspace
> changes (vm_bind + sparse) to get to the point where I can use PRR are
> a somewhat bigger task so it will take me a while to get the point
> where I can test any smmu patches.
>
Sure Rob get it. Previously in v11 I sent a patch adding a
adreno-smmu-priv api with similar "page allocation in drm" design as you
explained
above. Is that approach looking okay?
If it's okay can I add you in
suggested-by tag in that patch
<https://lore.kernel.org/all/20240605121713.3596499-7-quic_bibekkum@quicinc.com/>
?
Thanks & regards,
Bibek
> BR,
> -R
>
>
>> Thanks & regards,
>> Bibek
>>
>>>
>>> BR,
>>> -R
>>>
>>>> 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
More information about the linux-arm-kernel
mailing list