| Message ID | 20200810222657.1841322-5-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu: Add Adreno SMMU specific implementation | expand |
On Mon, Aug 10, 2020 at 04:26:48PM -0600, Jordan Crouse wrote: > Add domain attribute DOMAIN_ATTR_PGTABLE_CFG. This will be used by > arm-smmu to share the current pagetable configuration with the > leaf driver and to allow the leaf driver to set up a new pagetable > configuration under certain circumstances. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > > include/linux/iommu.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index fee209efb756..995ab8c47ef2 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -118,6 +118,7 @@ enum iommu_attr { > DOMAIN_ATTR_FSL_PAMUV1, > DOMAIN_ATTR_NESTING, /* two stages of translation */ > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > + DOMAIN_ATTR_PGTABLE_CFG, > DOMAIN_ATTR_MAX, > }; Nobody other than the adreno gpu uses this, so can we avoid exposing it in the IOMMU API, please? Given that you have a reference to the adreno GPU device in the SMMU implementation code thanks to .alloc_context_bank(), can you squirrel some function pointers away in the driver data (i.e. with dev_set_drvdata()) instead? Will
On Thu, Aug 13, 2020 at 6:14 AM Will Deacon <will@kernel.org> wrote: > > On Mon, Aug 10, 2020 at 04:26:48PM -0600, Jordan Crouse wrote: > > Add domain attribute DOMAIN_ATTR_PGTABLE_CFG. This will be used by > > arm-smmu to share the current pagetable configuration with the > > leaf driver and to allow the leaf driver to set up a new pagetable > > configuration under certain circumstances. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > > > include/linux/iommu.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index fee209efb756..995ab8c47ef2 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -118,6 +118,7 @@ enum iommu_attr { > > DOMAIN_ATTR_FSL_PAMUV1, > > DOMAIN_ATTR_NESTING, /* two stages of translation */ > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > > + DOMAIN_ATTR_PGTABLE_CFG, > > DOMAIN_ATTR_MAX, > > }; > > Nobody other than the adreno gpu uses this, so can we avoid exposing it > in the IOMMU API, please? Given that you have a reference to the adreno > GPU device in the SMMU implementation code thanks to .alloc_context_bank(), > can you squirrel some function pointers away in the driver data (i.e. with > dev_set_drvdata()) instead? > Hmm, we are already using drvdata on the gpu side, and it looks like arm-smmu is also using it. Could we get away with stashing an extra 'void *' in iommu_domain itself? Or alternatively, if we had a is_arm_smmu_domain(domain), then we could just directly call some exported private fxns with the domain ptr (which could then verify that the domain is actually an arm_smmu_domain, and then from there that the smmu is indeed qcom,adreno-smmu, to keep things sane) BR, -R
On Thu, Aug 13, 2020 at 08:11:02AM -0700, Rob Clark wrote: > On Thu, Aug 13, 2020 at 6:14 AM Will Deacon <will@kernel.org> wrote: > > > > On Mon, Aug 10, 2020 at 04:26:48PM -0600, Jordan Crouse wrote: > > > Add domain attribute DOMAIN_ATTR_PGTABLE_CFG. This will be used by > > > arm-smmu to share the current pagetable configuration with the > > > leaf driver and to allow the leaf driver to set up a new pagetable > > > configuration under certain circumstances. > > > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > > --- > > > > > > include/linux/iommu.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index fee209efb756..995ab8c47ef2 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -118,6 +118,7 @@ enum iommu_attr { > > > DOMAIN_ATTR_FSL_PAMUV1, > > > DOMAIN_ATTR_NESTING, /* two stages of translation */ > > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > > > + DOMAIN_ATTR_PGTABLE_CFG, > > > DOMAIN_ATTR_MAX, > > > }; > > > > Nobody other than the adreno gpu uses this, so can we avoid exposing it > > in the IOMMU API, please? Given that you have a reference to the adreno > > GPU device in the SMMU implementation code thanks to .alloc_context_bank(), > > can you squirrel some function pointers away in the driver data (i.e. with > > dev_set_drvdata()) instead? > > > > Hmm, we are already using drvdata on the gpu side, and it looks like > arm-smmu is also using it. Could we get away with stashing an extra > 'void *' in iommu_domain itself? What I meant was, expose the type of whatever you put in there on the GPU side so that the SMMU impl can install its function pointers into a field of that structure. As far as I'm concerned, the SMMU impl code and the GPU driver are the same entity and we should keep their communication private, rather than expose it up the stack. After all, the GPU writes to the SMMU registers! If you really don't want to expose all of your gubbins, I suppose you could have a structure just for the SMMU view and container_of() out of that on the GPU side. Will
On Thu, Aug 13, 2020 at 8:19 AM Will Deacon <will@kernel.org> wrote: > > On Thu, Aug 13, 2020 at 08:11:02AM -0700, Rob Clark wrote: > > On Thu, Aug 13, 2020 at 6:14 AM Will Deacon <will@kernel.org> wrote: > > > > > > On Mon, Aug 10, 2020 at 04:26:48PM -0600, Jordan Crouse wrote: > > > > Add domain attribute DOMAIN_ATTR_PGTABLE_CFG. This will be used by > > > > arm-smmu to share the current pagetable configuration with the > > > > leaf driver and to allow the leaf driver to set up a new pagetable > > > > configuration under certain circumstances. > > > > > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > > > --- > > > > > > > > include/linux/iommu.h | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > > index fee209efb756..995ab8c47ef2 100644 > > > > --- a/include/linux/iommu.h > > > > +++ b/include/linux/iommu.h > > > > @@ -118,6 +118,7 @@ enum iommu_attr { > > > > DOMAIN_ATTR_FSL_PAMUV1, > > > > DOMAIN_ATTR_NESTING, /* two stages of translation */ > > > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, > > > > + DOMAIN_ATTR_PGTABLE_CFG, > > > > DOMAIN_ATTR_MAX, > > > > }; > > > > > > Nobody other than the adreno gpu uses this, so can we avoid exposing it > > > in the IOMMU API, please? Given that you have a reference to the adreno > > > GPU device in the SMMU implementation code thanks to .alloc_context_bank(), > > > can you squirrel some function pointers away in the driver data (i.e. with > > > dev_set_drvdata()) instead? > > > > > > > Hmm, we are already using drvdata on the gpu side, and it looks like > > arm-smmu is also using it. Could we get away with stashing an extra > > 'void *' in iommu_domain itself? > > What I meant was, expose the type of whatever you put in there on the GPU > side so that the SMMU impl can install its function pointers into a field of > that structure. As far as I'm concerned, the SMMU impl code and the GPU > driver are the same entity and we should keep their communication private, > rather than expose it up the stack. After all, the GPU writes to the SMMU > registers! > > If you really don't want to expose all of your gubbins, I suppose you > could have a structure just for the SMMU view and container_of() out of > that on the GPU side. yeah, msm_gpu has a lot of internal state.. but I suppose we could define a 'struct adreno_smmu_priv' and embed that in msm_gpu, and throw in a get_gpu_drvdata() type wrapper for get_drvdata() to make this not totally horrible in the various cases places that use get_drvdata() currently. BR, -R
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index fee209efb756..995ab8c47ef2 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -118,6 +118,7 @@ enum iommu_attr { DOMAIN_ATTR_FSL_PAMUV1, DOMAIN_ATTR_NESTING, /* two stages of translation */ DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, + DOMAIN_ATTR_PGTABLE_CFG, DOMAIN_ATTR_MAX, };
Add domain attribute DOMAIN_ATTR_PGTABLE_CFG. This will be used by arm-smmu to share the current pagetable configuration with the leaf driver and to allow the leaf driver to set up a new pagetable configuration under certain circumstances. Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- include/linux/iommu.h | 1 + 1 file changed, 1 insertion(+)