diff mbox series

[v2,11/29] iommu/mediatek: Always pm_runtime_get while tlb flush

Message ID 20210813065324.29220-12-yong.wu@mediatek.com
State New
Headers show
Series MT8195 IOMMU SUPPORT | expand

Commit Message

Yong Wu (吴勇) Aug. 13, 2021, 6:53 a.m. UTC
Prepare for 2 HWs that sharing pgtable in different power-domains.

The previous SoC don't have PM. Only mt8192 has power-domain,
and it is display's power-domain which nearly always is enabled.

When there are 2 M4U HWs, it may has problem.
In this function, we get the pm_status via the m4u dev, but it don't
reflect the real power-domain status of the HW since there may be other
HW also use that power-domain.

Currently we could not get the real power-domain status, thus always
pm_runtime_get here.

Prepare for mt8195, thus, no need fix tags here.

This patch may drop the performance, we expect the user could
pm_runtime_get_sync before dma_alloc_attrs which need tlb ops.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Hsin-Yi Wang Aug. 24, 2021, 7:10 a.m. UTC | #1
On Fri, Aug 13, 2021 at 2:57 PM Yong Wu <yong.wu@mediatek.com> wrote:
>

> Prepare for 2 HWs that sharing pgtable in different power-domains.

>

> The previous SoC don't have PM. Only mt8192 has power-domain,

> and it is display's power-domain which nearly always is enabled.

>

> When there are 2 M4U HWs, it may has problem.

> In this function, we get the pm_status via the m4u dev, but it don't

> reflect the real power-domain status of the HW since there may be other

> HW also use that power-domain.

>

> Currently we could not get the real power-domain status, thus always

> pm_runtime_get here.

>

> Prepare for mt8195, thus, no need fix tags here.

>

> This patch may drop the performance, we expect the user could

> pm_runtime_get_sync before dma_alloc_attrs which need tlb ops.


Can you check if there are existing users that need to add this change?


>

> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> ---

<snip>
Yong Wu (吴勇) Sept. 1, 2021, 12:10 p.m. UTC | #2
On Tue, 2021-08-24 at 15:10 +0800, Hsin-Yi Wang wrote:
> On Fri, Aug 13, 2021 at 2:57 PM Yong Wu <yong.wu@mediatek.com> wrote:

> > 

> > Prepare for 2 HWs that sharing pgtable in different power-domains.

> > 

> > The previous SoC don't have PM. Only mt8192 has power-domain,

> > and it is display's power-domain which nearly always is enabled.

> > 

> > When there are 2 M4U HWs, it may has problem.

> > In this function, we get the pm_status via the m4u dev, but it

> > don't

> > reflect the real power-domain status of the HW since there may be

> > other

> > HW also use that power-domain.

> > 

> > Currently we could not get the real power-domain status, thus

> > always

> > pm_runtime_get here.

> > 

> > Prepare for mt8195, thus, no need fix tags here.

> > 

> > This patch may drop the performance, we expect the user could

> > pm_runtime_get_sync before dma_alloc_attrs which need tlb ops.

> 

> Can you check if there are existing users that need to add this

> change?


The issue may exist in our most users. Our users mainly are in v4l2.
normally their flow like this:
a) VIDIOC_REQBUFS: call dma_alloc_attrs or dma_buf_map_attachment.
b) VIDIOC_STREAMON. 
c) VIDIOC_QBUF: device_run: pm_runtime_get_sync.

Requesting they call pm_runtime_get before dma_alloc_attrs looks not
reasonable. It seems that they should not care about this.

This patch mainly make sure the flow is right. Locally I have a TODO to
try get the real power-domain status here, the sample code like below:

static struct notifier_block mtk_penpd_notifier;

/* Register the genpd notifier. */
mtk_penpd_notifier.notifier_call = mtk_iommu_pd_callback;
ret = dev_pm_genpd_add_notifier(dev, &mtk_penpd_notifier);

/* Then get the real power domain status in the notifier */
 static int mtk_iommu_pd_callback(struct notifier_block *nb,
                        unsigned long flags, void *data) 
 {
       if (flags == GENPD_NOTIFY_ON)
           /* the real power domain is power on */
       else if (flags == GENPD_NOTIFY_PRE_OFF)
           /* the real power domain are going to power off. Take it as
power off.
            * Skip the tlb ops after receivice this flag.
            */
 }
 
 How about this? or any other suggestion to get the real power-domain
rather than the iommu device's power domain status.
 Thanks.

> 

> 

> > 

> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> > ---

> 

> <snip>
Dafna Hirschfeld Sept. 30, 2021, 11:26 a.m. UTC | #3
On 13.08.21 08:53, Yong Wu wrote:
> Prepare for 2 HWs that sharing pgtable in different power-domains.

> 

> The previous SoC don't have PM. Only mt8192 has power-domain,

> and it is display's power-domain which nearly always is enabled.


hi, I see that in mt1873.dtsi, many devices that uses the iommu have the
'power-domains' property.

> 

> When there are 2 M4U HWs, it may has problem.

> In this function, we get the pm_status via the m4u dev, but it don't

> reflect the real power-domain status of the HW since there may be other

> HW also use that power-domain.

> 

> Currently we could not get the real power-domain status, thus always

> pm_runtime_get here.

> 

> Prepare for mt8195, thus, no need fix tags here.

> 

> This patch may drop the performance, we expect the user could

> pm_runtime_get_sync before dma_alloc_attrs which need tlb ops.

> 


Could you explain this sentence a bit? should the user call pm_runtime_get_sync
before calling dma_alloc_attrs?

Thanks,
Dafna

> Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> ---

>   drivers/iommu/mtk_iommu.c | 5 ++++-

>   1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c

> index add23a36a5e2..abc721a1da21 100644

> --- a/drivers/iommu/mtk_iommu.c

> +++ b/drivers/iommu/mtk_iommu.c

> @@ -238,8 +238,11 @@ static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size,

>   

>   	for_each_m4u(data, head) {

>   		if (has_pm) {

> -			if (pm_runtime_get_if_in_use(data->dev) <= 0)

> +			ret = pm_runtime_resume_and_get(data->dev);

> +			if (ret < 0) {

> +				dev_err(data->dev, "tlb flush: pm get fail %d.\n", ret);

>   				continue;

> +			}

>   		}

>   

>   		spin_lock_irqsave(&data->tlb_lock, flags);

>
Yong Wu (吴勇) Oct. 7, 2021, 3 a.m. UTC | #4
On Thu, 2021-09-30 at 13:26 +0200, Dafna Hirschfeld wrote:
> 

> On 13.08.21 08:53, Yong Wu wrote:

> > Prepare for 2 HWs that sharing pgtable in different power-domains.

> > 

> > The previous SoC don't have PM. Only mt8192 has power-domain,

> > and it is display's power-domain which nearly always is enabled.

> 

> hi, I see that in mt1873.dtsi, many devices that uses the iommu have

> the

> 'power-domains' property.


Sorry, I didn't clarify this clear. I mean the iommu device don't have
this property rather than the other device.

> 

> > 

> > When there are 2 M4U HWs, it may has problem.

> > In this function, we get the pm_status via the m4u dev, but it

> > don't

> > reflect the real power-domain status of the HW since there may be

> > other

> > HW also use that power-domain.

> > 

> > Currently we could not get the real power-domain status, thus

> > always

> > pm_runtime_get here.

> > 

> > Prepare for mt8195, thus, no need fix tags here.

> > 

> > This patch may drop the performance, we expect the user could

> > pm_runtime_get_sync before dma_alloc_attrs which need tlb ops.

> > 

> 

> Could you explain this sentence a bit? should the user call

> pm_runtime_get_sync

> before calling dma_alloc_attrs?


In v3, I have removed this patch. Use [1] instead.

[1] 
https://lore.kernel.org/linux-mediatek/20210923115840.17813-13-yong.wu@mediatek.com/

Thanks.

> 

> Thanks,

> Dafna

> 

> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>

> > ---

> >   drivers/iommu/mtk_iommu.c | 5 ++++-

> >   1 file changed, 4 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c

> > index add23a36a5e2..abc721a1da21 100644

> > --- a/drivers/iommu/mtk_iommu.c

> > +++ b/drivers/iommu/mtk_iommu.c

> > @@ -238,8 +238,11 @@ static void

> > mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size,

> >   

> >   	for_each_m4u(data, head) {

> >   		if (has_pm) {

> > -			if (pm_runtime_get_if_in_use(data->dev) <= 0)

> > +			ret = pm_runtime_resume_and_get(data->dev);

> > +			if (ret < 0) {

> > +				dev_err(data->dev, "tlb flush: pm get

> > fail %d.\n", ret);

> >   				continue;

> > +			}

> >   		}

> >   

> >   		spin_lock_irqsave(&data->tlb_lock, flags);

> >
diff mbox series

Patch

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index add23a36a5e2..abc721a1da21 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -238,8 +238,11 @@  static void mtk_iommu_tlb_flush_range_sync(unsigned long iova, size_t size,
 
 	for_each_m4u(data, head) {
 		if (has_pm) {
-			if (pm_runtime_get_if_in_use(data->dev) <= 0)
+			ret = pm_runtime_resume_and_get(data->dev);
+			if (ret < 0) {
+				dev_err(data->dev, "tlb flush: pm get fail %d.\n", ret);
 				continue;
+			}
 		}
 
 		spin_lock_irqsave(&data->tlb_lock, flags);