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

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

On 22.11.21 09:05, Yong Wu wrote:
> Hi Dafna,
> 
> On Wed, 2021-11-10 at 15:50 +0800, 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")
>>
>> 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.
> 
> What's your plan here?
> 
> If you send the two as a independent patchset on v5.16, I will rebase
> yours.
> If you have no time for this, I could help this.

Hi, just sent it in the patchset ` iommu/mediatek: fix tlb flush logic`

Thanks,
Dafna

> 
> Thanks.
> 
>>>>>
>>>>> 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.
>>>>>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu

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