Re: [PATCH v3 12/33] iommu/mediatek: Always tlb_flush_all when each PM resume

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

On 10.11.21 09:50, Yong Wu wrote:
> On Wed, 2021-11-10 at 07:29 +0200, Dafna Hirschfeld wrote:
>>
>> On 10.11.21 04:20, Yong Wu wrote:
>>> On Tue, 2021-11-09 at 14:21 +0200, Dafna Hirschfeld wrote:
>>>> Hi
>>>> This patch is needed in order to update the tlb when a device is
>>>> powered on.
>>>> Could you send this patch alone without the whole series so it
>>>> get
>>>> accepted easier?
>>>
>>> Which SoC are you testing on? In previous SoC, the IOMMU HW don't
>>> have
>>> power-domain, and we have a "has_pm"[1] in the tlb function for
>>> that
>>> case. The "has_pm" should be always 0 for the previous SoC like
>>> mt8173,
>>> it should always tlb synchronize.
>>>
>>> thus, Could you help share more about your issue? In which case it
>>> lack
>>> the necessary tlb operation. At least, We need confirm if it needs
>>> a
>>> "Fixes" tags if sending this patch alone.
>>
>> Hi,
>> I work with the mtk-vcodec driver on mt8173. As you wrote, the iommu
>> doesn't
>> have a power-domain and so when allocating buffers before the device
>> is powered
>> on, there is the warning
>> "Partial TLB flush timed out, falling back to full flush"
>> flooding the log buf.
> 
> oh. Thanks very much for your information. Get it now.
> 
> This issue should be introduced by the:
> 
> b34ea31fe013 ("iommu/mediatek: Always enable the clk on resume")

Hi, reverting this commit didn't solve those warnings,
I think this is because in the function mtk_iommu_attach_device
the first call to pm_runtime_resume_and_get does not turn the clks on
since m4u_dom is not yet initialize. And then mtk_iommu_attach_device
calls pm_runtime_put right after mtk_iommu_hw_init is called
(where the clks are turned on)

thanks,
Dafna


> 
> tlb failed due to the bclk is not enabled. Could you help try that
> after reverting this?
> 
>>
>> Sebastian Reichel suggested to remove the 'if(has_pm)' check to avoid
>> this warning,
>> and avoid flushing the tlb if the device is off:
>>
>> [1] http://ix.io/3Eyr
>>
>> This fixes the warning, but then the tlb is not flushed in sync,
>> Therefore the tlb should be flushed when the device is resumed.
>>
>> So the two patches (the one suggested in the link [1] and this patch)
>> should be sent together as a 2-patch series.
> 
> then this is reasonable. You could help this into a new patchset if you
> are free(add Fixes tag).
> 
> Thanks.
> 
>>
>> Thanks,
>> Dafna
>>
>>>
>>> Thanks.
>>>
>>> [1]
>>>
> https://elixir.bootlin.com/linux/v5.15/source/drivers/iommu/mtk_iommu.c#L236
>>>
>>>> I can resend the patch on your behalf if you want.
>>>>
>>>> Thanks,
>>>> Dafna
>>>>
>>>> On 23.09.21 14:58, Yong Wu wrote:
>>>>> Prepare for 2 HWs that sharing pgtable in different power-
>>>>> domains.
>>>>>
>>>>> When there are 2 M4U HWs, it may has problem in the flush_range
>>>>> in
>>>>> which
>>>>> we get the pm_status via the m4u dev, BUT that function don't
>>>>> reflect the
>>>>> real power-domain status of the HW since there may be other HW
>>>>> also
>>>>> use
>>>>> that power-domain.
>>>>>
>>>>> The function dma_alloc_attrs help allocate the iommu buffer
>>>>> which
>>>>> need the corresponding power domain since tlb flush is needed
>>>>> when
>>>>> preparing iova. BUT this function only is for allocating
>>>>> buffer,
>>>>> we have no good reason to request the user always call
>>>>> pm_runtime_get
>>>>> before calling dma_alloc_xxx. Therefore, we add a tlb_flush_all
>>>>> in the pm_runtime_resume to make sure the tlb always is clean.
>>>>>
>>>>> Another solution is always call pm_runtime_get in the
>>>>> tlb_flush_range.
>>>>> This will trigger pm runtime resume/backup so often when the
>>>>> iommu
>>>>> power is not active at some time(means user don't call
>>>>> pm_runtime_get
>>>>> before calling dma_alloc_xxx), This may cause the performance
>>>>> drop.
>>>>> thus we don't use this.
>>>>>
>>>>> In other case, the iommu's power should always be active via
>>>>> device
>>>>> link with smi.
>>>>>
>>>>> The previous SoC don't have PM except mt8192. the mt8192 IOMMU
>>>>> is
>>>>> display's
>>>>> power-domain which nearly always is enabled. thus no need fix
>>>>> tags
>>>>> here.
>>>>> Prepare for mt8195.
>>>>>
>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>> ---
>>>>>     drivers/iommu/mtk_iommu.c | 11 +++++++++++
>>>>>     1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/mtk_iommu.c
>>>>> b/drivers/iommu/mtk_iommu.c
>>>>> index 44cf5547d084..e9e94944ed91 100644
>>>>> --- a/drivers/iommu/mtk_iommu.c
>>>>> +++ b/drivers/iommu/mtk_iommu.c
>>>>> @@ -984,6 +984,17 @@ static int __maybe_unused
>>>>> mtk_iommu_runtime_resume(struct device *dev)
>>>>>     		return ret;
>>>>>     	}
>>>>>     
>>>>> +	/*
>>>>> +	 * Users may allocate dma buffer before they call
>>>>> pm_runtime_get, then
>>>>> +	 * it will lack the necessary tlb flush.
>>>>> +	 *
>>>>> +	 * We have no good reason to request the users always
>>>>> call
>>>>> dma_alloc_xx
>>>>> +	 * after pm_runtime_get_sync.
>>>>> +	 *
>>>>> +	 * Thus, Make sure the tlb always is clean after each
>>>>> PM
>>>>> resume.
>>>>> +	 */
>>>>> +	mtk_iommu_tlb_do_flush_all(data);
>>>>> +
>>>>>     	/*
>>>>>     	 * Uppon first resume, only enable the clk and return,
>>>>> since
>>>>> the values of the
>>>>>     	 * registers are not yet set.
>>>>>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek