| Message ID | 1404147116-4598-8-git-send-email-ohaugan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- > bounces@lists.linux-foundation.org] On Behalf Of Olav Haugan > Sent: Monday, June 30, 2014 10:22 PM > To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux- > foundation.org > Cc: linux-arm-msm@vger.kernel.org; will.deacon@arm.com; > thierry.reding@gmail.com; vgandhi@codeaurora.org > Subject: [RFC/PATCH 7/7] iommu-api: Add domain attribute to enable > coherent HTW > > Add a new iommu domain attribute that can be used to enable cache > coherent hardware table walks (HTW) by the SMMU. HTW might be supported > by the SMMU HW but depending on the use case and the usage of the SMMU in > the SoC it might not be always beneficial to always turn on coherent HTW > for all domains/iommu's. > [Sethi Varun-B16395] Why won't you want to use the coherent table walk feature? > Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> > --- > drivers/iommu/msm_iommu-v1.c | 16 ++++++++++++++++ > include/linux/iommu.h | 1 + > 2 files changed, 17 insertions(+) > > diff --git a/drivers/iommu/msm_iommu-v1.c b/drivers/iommu/msm_iommu-v1.c > index 2c574ef..e163ffc 100644 > --- a/drivers/iommu/msm_iommu-v1.c > +++ b/drivers/iommu/msm_iommu-v1.c > @@ -1456,8 +1456,16 @@ static int msm_domain_get_attr(struct iommu_domain > *domain, > enum iommu_attr attr, void *data) { > s32 ret = 0; > + struct msm_iommu_priv *priv = domain->priv; > > switch (attr) { > + case DOMAIN_ATTR_COHERENT_HTW: > + { > + s32 *int_ptr = (s32 *) data; > + > + *int_ptr = priv->pt.redirect; > + break; > + } > default: > pr_err("Unsupported attribute type\n"); > ret = -EINVAL; > @@ -1471,8 +1479,16 @@ static int msm_domain_set_attr(struct iommu_domain > *domain, > enum iommu_attr attr, void *data) { > s32 ret = 0; > + struct msm_iommu_priv *priv = domain->priv; > > switch (attr) { > + case DOMAIN_ATTR_COHERENT_HTW: > + { > + s32 *int_ptr = (s32 *) data; > + > + priv->pt.redirect = *int_ptr; > + break; > + } > default: > pr_err("Unsupported attribute type\n"); > ret = -EINVAL; > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index > 63dca6d..6d9596d 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -81,6 +81,7 @@ enum iommu_attr { > DOMAIN_ATTR_FSL_PAMU_STASH, > DOMAIN_ATTR_FSL_PAMU_ENABLE, > DOMAIN_ATTR_FSL_PAMUV1, > + DOMAIN_ATTR_COHERENT_HTW, [Sethi Varun-B16395] Would it make sense to represent this as DOMAIN_ATTR_SMMU_COHERENT_HTW? I believe this is specific to SMMU. -Varun -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/1/2014 1:49 AM, Varun Sethi wrote: > > >> -----Original Message----- >> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- >> bounces@lists.linux-foundation.org] On Behalf Of Olav Haugan >> Sent: Monday, June 30, 2014 10:22 PM >> To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux- >> foundation.org >> Cc: linux-arm-msm@vger.kernel.org; will.deacon@arm.com; >> thierry.reding@gmail.com; vgandhi@codeaurora.org >> Subject: [RFC/PATCH 7/7] iommu-api: Add domain attribute to enable >> coherent HTW >> >> Add a new iommu domain attribute that can be used to enable cache >> coherent hardware table walks (HTW) by the SMMU. HTW might be supported >> by the SMMU HW but depending on the use case and the usage of the SMMU in >> the SoC it might not be always beneficial to always turn on coherent HTW >> for all domains/iommu's. >> > [Sethi Varun-B16395] Why won't you want to use the coherent table walk feature? Very good question. We have found that turning on IOMMU coherent HTW is not always beneficial to performance (performance either the same or slightly worse in some cases). Even if the perf. is the same we would like to avoid using precious L2 cache for no benefit to the IOMMU. Although our HW supports this feature we don't always want to turn this on for a specific use case/domain (bus master). >> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> >> --- >> drivers/iommu/msm_iommu-v1.c | 16 ++++++++++++++++ >> include/linux/iommu.h | 1 + >> 2 files changed, 17 insertions(+) >> >> diff --git a/drivers/iommu/msm_iommu-v1.c b/drivers/iommu/msm_iommu-v1.c >> index 2c574ef..e163ffc 100644 >> --- a/drivers/iommu/msm_iommu-v1.c >> +++ b/drivers/iommu/msm_iommu-v1.c >> @@ -1456,8 +1456,16 @@ static int msm_domain_get_attr(struct iommu_domain >> *domain, >> enum iommu_attr attr, void *data) { >> s32 ret = 0; >> + struct msm_iommu_priv *priv = domain->priv; >> >> switch (attr) { >> + case DOMAIN_ATTR_COHERENT_HTW: >> + { >> + s32 *int_ptr = (s32 *) data; >> + >> + *int_ptr = priv->pt.redirect; >> + break; >> + } >> default: >> pr_err("Unsupported attribute type\n"); >> ret = -EINVAL; >> @@ -1471,8 +1479,16 @@ static int msm_domain_set_attr(struct iommu_domain >> *domain, >> enum iommu_attr attr, void *data) { >> s32 ret = 0; >> + struct msm_iommu_priv *priv = domain->priv; >> >> switch (attr) { >> + case DOMAIN_ATTR_COHERENT_HTW: >> + { >> + s32 *int_ptr = (s32 *) data; >> + >> + priv->pt.redirect = *int_ptr; >> + break; >> + } >> default: >> pr_err("Unsupported attribute type\n"); >> ret = -EINVAL; >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index >> 63dca6d..6d9596d 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -81,6 +81,7 @@ enum iommu_attr { >> DOMAIN_ATTR_FSL_PAMU_STASH, >> DOMAIN_ATTR_FSL_PAMU_ENABLE, >> DOMAIN_ATTR_FSL_PAMUV1, >> + DOMAIN_ATTR_COHERENT_HTW, > [Sethi Varun-B16395] Would it make sense to represent this as DOMAIN_ATTR_SMMU_COHERENT_HTW? I believe this is specific to SMMU. Yes, it does. Thanks, Olav Haugan
On Wed, Jul 02, 2014 at 11:11:13PM +0100, Olav Haugan wrote: > On 7/1/2014 1:49 AM, Varun Sethi wrote: > > > > > >> -----Original Message----- > >> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- > >> bounces@lists.linux-foundation.org] On Behalf Of Olav Haugan > >> Sent: Monday, June 30, 2014 10:22 PM > >> To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux- > >> foundation.org > >> Cc: linux-arm-msm@vger.kernel.org; will.deacon@arm.com; > >> thierry.reding@gmail.com; vgandhi@codeaurora.org > >> Subject: [RFC/PATCH 7/7] iommu-api: Add domain attribute to enable > >> coherent HTW > >> > >> Add a new iommu domain attribute that can be used to enable cache > >> coherent hardware table walks (HTW) by the SMMU. HTW might be supported > >> by the SMMU HW but depending on the use case and the usage of the SMMU in > >> the SoC it might not be always beneficial to always turn on coherent HTW > >> for all domains/iommu's. > >> > > [Sethi Varun-B16395] Why won't you want to use the coherent table walk feature? > > Very good question. We have found that turning on IOMMU coherent HTW is > not always beneficial to performance (performance either the same or > slightly worse in some cases). Even if the perf. is the same we would > like to avoid using precious L2 cache for no benefit to the IOMMU. > Although our HW supports this feature we don't always want to turn this > on for a specific use case/domain (bus master). Could we at least invert the feature flag, please? i.e. you set an attribute to *disable* coherent walks? I'd also be interested to see some performance numbers, as the added cacheflushing overhead from non-coherent walks is going to be non-trivial. Will -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/3/2014 10:43 AM, Will Deacon wrote: > On Wed, Jul 02, 2014 at 11:11:13PM +0100, Olav Haugan wrote: >> On 7/1/2014 1:49 AM, Varun Sethi wrote: >>> >>> >>>> -----Original Message----- >>>> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- >>>> bounces@lists.linux-foundation.org] On Behalf Of Olav Haugan >>>> Sent: Monday, June 30, 2014 10:22 PM >>>> To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux- >>>> foundation.org >>>> Cc: linux-arm-msm@vger.kernel.org; will.deacon@arm.com; >>>> thierry.reding@gmail.com; vgandhi@codeaurora.org >>>> Subject: [RFC/PATCH 7/7] iommu-api: Add domain attribute to enable >>>> coherent HTW >>>> >>>> Add a new iommu domain attribute that can be used to enable cache >>>> coherent hardware table walks (HTW) by the SMMU. HTW might be supported >>>> by the SMMU HW but depending on the use case and the usage of the SMMU in >>>> the SoC it might not be always beneficial to always turn on coherent HTW >>>> for all domains/iommu's. >>>> >>> [Sethi Varun-B16395] Why won't you want to use the coherent table walk feature? >> >> Very good question. We have found that turning on IOMMU coherent HTW is >> not always beneficial to performance (performance either the same or >> slightly worse in some cases). Even if the perf. is the same we would >> like to avoid using precious L2 cache for no benefit to the IOMMU. >> Although our HW supports this feature we don't always want to turn this >> on for a specific use case/domain (bus master). > > Could we at least invert the feature flag, please? i.e. you set an attribute > to *disable* coherent walks? I'd also be interested to see some performance > numbers, as the added cacheflushing overhead from non-coherent walks is > going to be non-trivial. > Yes, agree that we can do the inverse. On one SoC I saw about 5% degradation in performance with coherent table walk enabled for a specific bus master. However, we have seen improved performance also with other SMMUs/bus masters. It just depends on the SMMU/bus master and how it is being used. Hence the need to be able to disable this on a per-domain basis. Thanks, Olav Haugan
diff --git a/drivers/iommu/msm_iommu-v1.c b/drivers/iommu/msm_iommu-v1.c index 2c574ef..e163ffc 100644 --- a/drivers/iommu/msm_iommu-v1.c +++ b/drivers/iommu/msm_iommu-v1.c @@ -1456,8 +1456,16 @@ static int msm_domain_get_attr(struct iommu_domain *domain, enum iommu_attr attr, void *data) { s32 ret = 0; + struct msm_iommu_priv *priv = domain->priv; switch (attr) { + case DOMAIN_ATTR_COHERENT_HTW: + { + s32 *int_ptr = (s32 *) data; + + *int_ptr = priv->pt.redirect; + break; + } default: pr_err("Unsupported attribute type\n"); ret = -EINVAL; @@ -1471,8 +1479,16 @@ static int msm_domain_set_attr(struct iommu_domain *domain, enum iommu_attr attr, void *data) { s32 ret = 0; + struct msm_iommu_priv *priv = domain->priv; switch (attr) { + case DOMAIN_ATTR_COHERENT_HTW: + { + s32 *int_ptr = (s32 *) data; + + priv->pt.redirect = *int_ptr; + break; + } default: pr_err("Unsupported attribute type\n"); ret = -EINVAL; diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 63dca6d..6d9596d 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -81,6 +81,7 @@ enum iommu_attr { DOMAIN_ATTR_FSL_PAMU_STASH, DOMAIN_ATTR_FSL_PAMU_ENABLE, DOMAIN_ATTR_FSL_PAMUV1, + DOMAIN_ATTR_COHERENT_HTW, DOMAIN_ATTR_MAX, };
Add a new iommu domain attribute that can be used to enable cache coherent hardware table walks (HTW) by the SMMU. HTW might be supported by the SMMU HW but depending on the use case and the usage of the SMMU in the SoC it might not be always beneficial to always turn on coherent HTW for all domains/iommu's. Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> --- drivers/iommu/msm_iommu-v1.c | 16 ++++++++++++++++ include/linux/iommu.h | 1 + 2 files changed, 17 insertions(+)