Alexandre Courbot
2015-Apr-17 06:26 UTC
[Nouveau] [PATCH 2/6] instmem/gk20a: refer to IOMMU physical translation bit
On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh at nvidia.com> wrote:> Instead of hard-coding the translation bit in subdev driver, we refer to > the platform data. > > Signed-off-by: Vince Hsu <vinceh at nvidia.com> > --- > drm/nouveau/nvkm/subdev/instmem/gk20a.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drm/nouveau/nvkm/subdev/instmem/gk20a.c > index dd0994d9ebfc..69ef5eae3279 100644 > --- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c > +++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c > @@ -89,6 +89,7 @@ struct gk20a_instmem_priv { > struct nvkm_mm *mm; > struct iommu_domain *domain; > unsigned long iommu_pgshift; > + unsigned long iommu_phys_addr_bit; > > /* Only used by DMA API */ > struct dma_attrs attrs; > @@ -169,8 +170,8 @@ gk20a_instobj_dtor_iommu(struct gk20a_instobj_priv *_node) > r = list_first_entry(&_node->mem->regions, struct nvkm_mm_node, > rl_entry); > > - /* clear bit 34 to unmap pages */ > - r->offset &= ~BIT(34 - priv->iommu_pgshift); > + /* clear IOMMU translation bit to unmap pages */ > + r->offset &= ~BIT(priv->iommu_phys_addr_bit - priv->iommu_pgshift); > > /* Unmap pages from GPU address space and free them */ > for (i = 0; i < _node->mem->size; i++) { > @@ -298,8 +299,11 @@ gk20a_instobj_ctor_iommu(struct nvkm_object *parent, struct nvkm_object *engine, > } > } > > - /* Bit 34 tells that an address is to be resolved through the IOMMU */ > - r->offset |= BIT(34 - priv->iommu_pgshift); > + /* > + * The iommu_phys_addr_bit tells that an address is to be resolved > + * through the IOMMU > + */ > + r->offset |= BIT(priv->iommu_phys_addr_bit - priv->iommu_pgshift); > > node->base._mem.offset = ((u64)r->offset) << priv->iommu_pgshift; > > @@ -407,6 +411,7 @@ gk20a_instmem_ctor(struct nvkm_object *parent, struct nvkm_object *engine, > priv->domain = plat->gpu->iommu.domain; > priv->mm = plat->gpu->iommu.mm; > priv->iommu_pgshift = plat->gpu->iommu.pgshift; > + priv->iommu_phys_addr_bit = plat->gpu->iommu.phys_addr_bit;Looks good, but I think I would definitely prefer this to be a mask instead of a bit index, i.e: r->offset &= ~(priv->iommu_addr_mask >> priv->iommu_pgshift); and r->offset |= (priv->iommu_addr_mask >> priv->iommu_pgshift);
Vince Hsu
2015-Apr-17 07:19 UTC
[Nouveau] [PATCH 2/6] instmem/gk20a: refer to IOMMU physical translation bit
On 04/17/2015 02:26 PM, Alexandre Courbot wrote:> On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh at nvidia.com> wrote: >> Instead of hard-coding the translation bit in subdev driver, we refer to >> the platform data. >> >> Signed-off-by: Vince Hsu <vinceh at nvidia.com> >> --- >> drm/nouveau/nvkm/subdev/instmem/gk20a.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> index dd0994d9ebfc..69ef5eae3279 100644 >> --- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> +++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c >> @@ -89,6 +89,7 @@ struct gk20a_instmem_priv { >> struct nvkm_mm *mm; >> struct iommu_domain *domain; >> unsigned long iommu_pgshift; >> + unsigned long iommu_phys_addr_bit; >> >> /* Only used by DMA API */ >> struct dma_attrs attrs; >> @@ -169,8 +170,8 @@ gk20a_instobj_dtor_iommu(struct gk20a_instobj_priv *_node) >> r = list_first_entry(&_node->mem->regions, struct nvkm_mm_node, >> rl_entry); >> >> - /* clear bit 34 to unmap pages */ >> - r->offset &= ~BIT(34 - priv->iommu_pgshift); >> + /* clear IOMMU translation bit to unmap pages */ >> + r->offset &= ~BIT(priv->iommu_phys_addr_bit - priv->iommu_pgshift); >> >> /* Unmap pages from GPU address space and free them */ >> for (i = 0; i < _node->mem->size; i++) { >> @@ -298,8 +299,11 @@ gk20a_instobj_ctor_iommu(struct nvkm_object *parent, struct nvkm_object *engine, >> } >> } >> >> - /* Bit 34 tells that an address is to be resolved through the IOMMU */ >> - r->offset |= BIT(34 - priv->iommu_pgshift); >> + /* >> + * The iommu_phys_addr_bit tells that an address is to be resolved >> + * through the IOMMU >> + */ >> + r->offset |= BIT(priv->iommu_phys_addr_bit - priv->iommu_pgshift); >> >> node->base._mem.offset = ((u64)r->offset) << priv->iommu_pgshift; >> >> @@ -407,6 +411,7 @@ gk20a_instmem_ctor(struct nvkm_object *parent, struct nvkm_object *engine, >> priv->domain = plat->gpu->iommu.domain; >> priv->mm = plat->gpu->iommu.mm; >> priv->iommu_pgshift = plat->gpu->iommu.pgshift; >> + priv->iommu_phys_addr_bit = plat->gpu->iommu.phys_addr_bit; > Looks good, but I think I would definitely prefer this to be a mask > instead of a bit index, i.e: > > r->offset &= ~(priv->iommu_addr_mask >> priv->iommu_pgshift); > > and > > r->offset |= (priv->iommu_addr_mask >> priv->iommu_pgshift);Which one is for setting up an IOMMU address If the iommu_addr_mask is defined as ~BIT_ULL(34)? Thanks, Vince ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
Alexandre Courbot
2015-Apr-17 07:40 UTC
[Nouveau] [PATCH 2/6] instmem/gk20a: refer to IOMMU physical translation bit
On Fri, Apr 17, 2015 at 4:19 PM, Vince Hsu <vinceh at nvidia.com> wrote:> > On 04/17/2015 02:26 PM, Alexandre Courbot wrote: >> >> On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh at nvidia.com> wrote: >>> >>> Instead of hard-coding the translation bit in subdev driver, we refer to >>> the platform data. >>> >>> Signed-off-by: Vince Hsu <vinceh at nvidia.com> >>> --- >>> drm/nouveau/nvkm/subdev/instmem/gk20a.c | 13 +++++++++---- >>> 1 file changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> b/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> index dd0994d9ebfc..69ef5eae3279 100644 >>> --- a/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> +++ b/drm/nouveau/nvkm/subdev/instmem/gk20a.c >>> @@ -89,6 +89,7 @@ struct gk20a_instmem_priv { >>> struct nvkm_mm *mm; >>> struct iommu_domain *domain; >>> unsigned long iommu_pgshift; >>> + unsigned long iommu_phys_addr_bit; >>> >>> /* Only used by DMA API */ >>> struct dma_attrs attrs; >>> @@ -169,8 +170,8 @@ gk20a_instobj_dtor_iommu(struct gk20a_instobj_priv >>> *_node) >>> r = list_first_entry(&_node->mem->regions, struct nvkm_mm_node, >>> rl_entry); >>> >>> - /* clear bit 34 to unmap pages */ >>> - r->offset &= ~BIT(34 - priv->iommu_pgshift); >>> + /* clear IOMMU translation bit to unmap pages */ >>> + r->offset &= ~BIT(priv->iommu_phys_addr_bit - >>> priv->iommu_pgshift); >>> >>> /* Unmap pages from GPU address space and free them */ >>> for (i = 0; i < _node->mem->size; i++) { >>> @@ -298,8 +299,11 @@ gk20a_instobj_ctor_iommu(struct nvkm_object *parent, >>> struct nvkm_object *engine, >>> } >>> } >>> >>> - /* Bit 34 tells that an address is to be resolved through the >>> IOMMU */ >>> - r->offset |= BIT(34 - priv->iommu_pgshift); >>> + /* >>> + * The iommu_phys_addr_bit tells that an address is to be >>> resolved >>> + * through the IOMMU >>> + */ >>> + r->offset |= BIT(priv->iommu_phys_addr_bit - >>> priv->iommu_pgshift); >>> >>> node->base._mem.offset = ((u64)r->offset) << >>> priv->iommu_pgshift; >>> >>> @@ -407,6 +411,7 @@ gk20a_instmem_ctor(struct nvkm_object *parent, struct >>> nvkm_object *engine, >>> priv->domain = plat->gpu->iommu.domain; >>> priv->mm = plat->gpu->iommu.mm; >>> priv->iommu_pgshift = plat->gpu->iommu.pgshift; >>> + priv->iommu_phys_addr_bit >>> plat->gpu->iommu.phys_addr_bit; >> >> Looks good, but I think I would definitely prefer this to be a mask >> instead of a bit index, i.e: >> >> r->offset &= ~(priv->iommu_addr_mask >> priv->iommu_pgshift); >> >> and >> >> r->offset |= (priv->iommu_addr_mask >> priv->iommu_pgshift); > > Which one is for setting up an IOMMU address If the iommu_addr_mask is > defined as ~BIT_ULL(34)?I would define iomm_addr_mask as BIT_ULL(34). Then the |= is for setting the mask and &= to remove it.
Terje Bergstrom
2015-Apr-17 15:14 UTC
[Nouveau] [PATCH 2/6] instmem/gk20a: refer to IOMMU physical translation bit
On 04/16/2015 11:26 PM, Alexandre Courbot wrote:> Looks good, but I think I would definitely prefer this to be a mask > instead of a bit index, i.e: > > r->offset &= ~(priv->iommu_addr_mask >> priv->iommu_pgshift); > > and > > r->offset |= (priv->iommu_addr_mask >> priv->iommu_pgshift);Wouldn't that be just a more complicated way of expressing the same thing?
Alexandre Courbot
2015-Apr-20 07:38 UTC
[Nouveau] [PATCH 2/6] instmem/gk20a: refer to IOMMU physical translation bit
On Sat, Apr 18, 2015 at 12:14 AM, Terje Bergstrom <tbergstrom at nvidia.com> wrote:> > On 04/16/2015 11:26 PM, Alexandre Courbot wrote: >> >> Looks good, but I think I would definitely prefer this to be a mask >> instead of a bit index, i.e: >> >> r->offset &= ~(priv->iommu_addr_mask >> priv->iommu_pgshift); >> >> and >> >> r->offset |= (priv->iommu_addr_mask >> priv->iommu_pgshift); > > Wouldn't that be just a more complicated way of expressing the same thing?Right now we have r->offset |= BIT(priv->iommu_phys_addr_bit - priv->iommu_pgshift), which doesn't look much simpler. :)