Re: [PATCH v8 04/12] iommu/mediatek: Add device_link between the consumer and the larb devices
On Mon, 2021-10-18 at 09:13 +0200, Dafna Hirschfeld wrote:
>
> On 16.10.21 04:23, Yong Wu wrote:
> > On Mon, 2021-10-11 at 14:36 +0200, Dafna Hirschfeld wrote:
> > >
> > > On 29.09.21 03:37, Yong Wu wrote:
> > > > MediaTek IOMMU-SMI diagram is like below. all the consumer
> > > > connect
> > > > with
> > > > smi-larb, then connect with smi-common.
> > > >
> > > > M4U
> > > >|
> > > > smi-common
> > > >|
> > > > -
> > > > | |...
> > > > | |
> > > > larb1 larb2
> > > > | |
> > > > vdec venc
> > > >
> > > > When the consumer works, it should enable the smi-larb's power
> > > > which
> > > > also need enable the smi-common's power firstly.
> > > >
> > > > Thus, First of all, use the device link connect the consumer
> > > > and
> > > > the
> > > > smi-larbs. then add device link between the smi-larb and smi-
> > > > common.
> > > >
> > > > This patch adds device_link between the consumer and the larbs.
> > > >
> > > > When device_link_add, I add the flag DL_FLAG_STATELESS to avoid
> > > > calling
> > > > pm_runtime_xx to keep the original status of clocks. It can
> > > > avoid
> > > > two
> > > > issues:
> > > > 1) Display HW show fastlogo abnormally reported in [1]. At the
> > > > beggining,
> > > > all the clocks are enabled before entering kernel, but the
> > > > clocks
> > > > for
> > > > display HW(always in larb0) will be gated after clk_enable and
> > > > clk_disable
> > > > called from device_link_add(->pm_runtime_resume) and rpm_idle.
> > > > The
> > > > clock
> > > > operation happened before display driver probe. At that time,
> > > > the
> > > > display
> > > > HW will be abnormal.
> > > >
> > > > 2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to
> > > > skip
> > > > pm_runtime_xx to avoid the deadlock.
> > > >
> > > > Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
> > > > device_link_removed should be added explicitly.
> > > >
> > > > [1]
> > > >
https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
> > > > [2] https://lore.kernel.org/patchwork/patch/1086569/
> > > >
> > > > Suggested-by: Tomasz Figa
> > > > Signed-off-by: Yong Wu
> > > > Tested-by: Frank Wunderlich # BPI-
> > > > R2/MT7623
> > > > ---
> > > >drivers/iommu/mtk_iommu.c| 22 ++
> > > >drivers/iommu/mtk_iommu_v1.c | 20 +++-
> > > >2 files changed, 41 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iommu/mtk_iommu.c
> > > > b/drivers/iommu/mtk_iommu.c
> > > > index d5848f78a677..a2fa55899434 100644
> > > > --- a/drivers/iommu/mtk_iommu.c
> > > > +++ b/drivers/iommu/mtk_iommu.c
> > > > @@ -560,22 +560,44 @@ static struct iommu_device
> > > > *mtk_iommu_probe_device(struct device *dev)
> > > >{
> > > > struct iommu_fwspec *fwspec =
> > > > dev_iommu_fwspec_get(dev);
> > > > struct mtk_iommu_data *data;
> > > > + struct device_link *link;
> > > > + struct device *larbdev;
> > > > + unsigned int larbid;
> > > >
> > > > if (!fwspec || fwspec->ops != _iommu_ops)
> > > > return ERR_PTR(-ENODEV); /* Not a iommu client
> > > > device
> > > > */
> > > >
> > > > data = dev_iommu_priv_get(dev);
> > > >
> > > > + /*
> > > > +* Link the consumer device with the smi-larb
> > > > device(supplier)
> > > > +* The device in each a larb is a independent HW. thus
> > > > only
> > > > link
> > > > +* one larb here.
> > > > +*/
> > > > + larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> > >
> > > so larbid is always the same for all the ids of a device?
> >
> > Yes. For me, each a dtsi node should represent a HW unit which can
> > only
> > connect one larb.
> >
> > > If so maybe it worth testing it and return error if this is not
> > > the
> > > case.
> >
> > Thanks for the suggestion. This is very helpful. I did see someone
> > put
> > the different larbs in one node. I will check this, and add return
>
> I am working on bugs found on media drivers, could you please point
> me to
> that wrong node?
> Will you send a fix to that node in the dtsi?
sorry. I mean it happened in the internal branch and it has already
been fixed internally, all the upstream nodes are ok for this.
Thanks
>
>
> Thanks,
> Dafna
>
> > EINVAL for this case.
>
>
>
> >
> > >
> > > Thanks,
> > > Dafna
> >
> >
> > > >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 04/12] iommu/mediatek: Add device_link between the consumer and the larb devices
On 16.10.21 04:23, Yong Wu wrote:
On Mon, 2021-10-11 at 14:36 +0200, Dafna Hirschfeld wrote:
On 29.09.21 03:37, Yong Wu wrote:
MediaTek IOMMU-SMI diagram is like below. all the consumer connect
with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power
which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and
the
smi-larbs. then add device link between the smi-larb and smi-
common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid
calling
pm_runtime_xx to keep the original status of clocks. It can avoid
two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the
beggining,
all the clocks are enabled before entering kernel, but the clocks
for
display HW(always in larb0) will be gated after clk_enable and
clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The
clock
operation happened before display driver probe. At that time, the
display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
[1]
https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-
R2/MT7623
---
drivers/iommu/mtk_iommu.c| 22 ++
drivers/iommu/mtk_iommu_v1.c | 20 +++-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index d5848f78a677..a2fa55899434 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -560,22 +560,44 @@ static struct iommu_device
*mtk_iommu_probe_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device
*/
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier)
+* The device in each a larb is a independent HW. thus only
link
+* one larb here.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
so larbid is always the same for all the ids of a device?
Yes. For me, each a dtsi node should represent a HW unit which can only
connect one larb.
If so maybe it worth testing it and return error if this is not the
case.
Thanks for the suggestion. This is very helpful. I did see someone put
the different larbs in one node. I will check this, and add return
I am working on bugs found on media drivers, could you please point me to
that wrong node?
Will you send a fix to that node in the dtsi?
Thanks,
Dafna
EINVAL for this case.
Thanks,
Dafna
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 04/12] iommu/mediatek: Add device_link between the consumer and the larb devices
On Mon, 2021-10-11 at 14:36 +0200, Dafna Hirschfeld wrote:
>
> On 29.09.21 03:37, Yong Wu wrote:
> > MediaTek IOMMU-SMI diagram is like below. all the consumer connect
> > with
> > smi-larb, then connect with smi-common.
> >
> > M4U
> > |
> > smi-common
> > |
> >-
> >| |...
> >| |
> > larb1 larb2
> >| |
> > vdec venc
> >
> > When the consumer works, it should enable the smi-larb's power
> > which
> > also need enable the smi-common's power firstly.
> >
> > Thus, First of all, use the device link connect the consumer and
> > the
> > smi-larbs. then add device link between the smi-larb and smi-
> > common.
> >
> > This patch adds device_link between the consumer and the larbs.
> >
> > When device_link_add, I add the flag DL_FLAG_STATELESS to avoid
> > calling
> > pm_runtime_xx to keep the original status of clocks. It can avoid
> > two
> > issues:
> > 1) Display HW show fastlogo abnormally reported in [1]. At the
> > beggining,
> > all the clocks are enabled before entering kernel, but the clocks
> > for
> > display HW(always in larb0) will be gated after clk_enable and
> > clk_disable
> > called from device_link_add(->pm_runtime_resume) and rpm_idle. The
> > clock
> > operation happened before display driver probe. At that time, the
> > display
> > HW will be abnormal.
> >
> > 2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
> > pm_runtime_xx to avoid the deadlock.
> >
> > Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
> > device_link_removed should be added explicitly.
> >
> > [1]
> > https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
> > [2] https://lore.kernel.org/patchwork/patch/1086569/
> >
> > Suggested-by: Tomasz Figa
> > Signed-off-by: Yong Wu
> > Tested-by: Frank Wunderlich # BPI-
> > R2/MT7623
> > ---
> > drivers/iommu/mtk_iommu.c| 22 ++
> > drivers/iommu/mtk_iommu_v1.c | 20 +++-
> > 2 files changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index d5848f78a677..a2fa55899434 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -560,22 +560,44 @@ static struct iommu_device
> > *mtk_iommu_probe_device(struct device *dev)
> > {
> > struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct mtk_iommu_data *data;
> > + struct device_link *link;
> > + struct device *larbdev;
> > + unsigned int larbid;
> >
> > if (!fwspec || fwspec->ops != _iommu_ops)
> > return ERR_PTR(-ENODEV); /* Not a iommu client device
> > */
> >
> > data = dev_iommu_priv_get(dev);
> >
> > + /*
> > +* Link the consumer device with the smi-larb device(supplier)
> > +* The device in each a larb is a independent HW. thus only
> > link
> > +* one larb here.
> > +*/
> > + larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>
> so larbid is always the same for all the ids of a device?
Yes. For me, each a dtsi node should represent a HW unit which can only
connect one larb.
> If so maybe it worth testing it and return error if this is not the
> case.
Thanks for the suggestion. This is very helpful. I did see someone put
the different larbs in one node. I will check this, and add return
EINVAL for this case.
>
> Thanks,
> Dafna
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 04/12] iommu/mediatek: Add device_link between the consumer and the larb devices
On 29.09.21 03:37, Yong Wu wrote:
MediaTek IOMMU-SMI diagram is like below. all the consumer connect with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and the
smi-larbs. then add device link between the smi-larb and smi-common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
pm_runtime_xx to keep the original status of clocks. It can avoid two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
all the clocks are enabled before entering kernel, but the clocks for
display HW(always in larb0) will be gated after clk_enable and clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
operation happened before display driver probe. At that time, the display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
[1] https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
---
drivers/iommu/mtk_iommu.c| 22 ++
drivers/iommu/mtk_iommu_v1.c | 20 +++-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index d5848f78a677..a2fa55899434 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -560,22 +560,44 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device */
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier)
+* The device in each a larb is a independent HW. thus only link
+* one larb here.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
so larbid is always the same for all the ids of a device? If so
maybe it worth testing it and return error if this is not the case.
Thanks,
Dafna
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
return >iommu;
}
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct mtk_iommu_data *data;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return;
+ data = dev_iommu_priv_get(dev);
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ device_link_remove(dev, larbdev);
+
iommu_fwspec_free(dev);
}
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 4d7809432239..e6f13459470e 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -423,7 +423,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct of_phandle_args iommu_spec;
struct mtk_iommu_data *data;
- int err, idx = 0;
+ int err, idx = 0, larbid;
+ struct device_link *link;
+ struct device *larbdev;
/*
* In the deferred case, free the existed fwspec.
@@ -453,6 +455,14 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
data = dev_iommu_priv_get(dev);
+ /* Link the consumer device with the smi-larb device(supplier) */
+ larbid = mt2701_m4u_to_larb(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
+
return >iommu;
}
@@ -473,10 +483,18 @@ static void mtk_iommu_probe_finalize(struct device *dev)
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct
