Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Hi Hiroshi, Fernando, On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Guzman Lugo, Fernando fernando.l...@ti.com Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Date: Tue, 8 Mar 2011 11:59:43 -0600 On Tue, Mar 8, 2011 at 6:46 AM, David Cohen daco...@gmail.com wrote: Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according to input 'da' address when mapping memory: da == 0: IOVMF_DA_ANON da != 0: IOVMF_DA_FIXED It prevents IOMMU to map first page with fixed 'da'. To avoid such issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now come from the user. IOVMF_DA_ANON will be automatically set if IOVMF_DA_FIXED isn't set. Signed-off-by: David Cohen daco...@gmail.com --- arch/arm/plat-omap/iovmm.c | 12 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c index 11c9b76..dde9cb0 100644 --- a/arch/arm/plat-omap/iovmm.c +++ b/arch/arm/plat-omap/iovmm.c @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt, flags = IOVMF_HW_MASK; flags |= IOVMF_DISCONT; flags |= IOVMF_MMIO; - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. Regards, David Regards, Fernando. .. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 12:57 PM, David Cohen daco...@gmail.com wrote: Hi Hiroshi, Fernando, On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: From: ext Guzman Lugo, Fernando fernando.l...@ti.com Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Date: Tue, 8 Mar 2011 11:59:43 -0600 On Tue, Mar 8, 2011 at 6:46 AM, David Cohen daco...@gmail.com wrote: Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according to input 'da' address when mapping memory: da == 0: IOVMF_DA_ANON da != 0: IOVMF_DA_FIXED It prevents IOMMU to map first page with fixed 'da'. To avoid such issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now come from the user. IOVMF_DA_ANON will be automatically set if IOVMF_DA_FIXED isn't set. Signed-off-by: David Cohen daco...@gmail.com --- arch/arm/plat-omap/iovmm.c | 12 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c index 11c9b76..dde9cb0 100644 --- a/arch/arm/plat-omap/iovmm.c +++ b/arch/arm/plat-omap/iovmm.c @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt, flags = IOVMF_HW_MASK; flags |= IOVMF_DISCONT; flags |= IOVMF_MMIO; - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. sounds perfect to me. Regards, Fernando. Regards, David Regards, Fernando. .. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
[snip] - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Br, David Cohen -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 9:46 PM, David Cohen daco...@gmail.com wrote: [snip] - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); + if (~flags IOVMF_DA_FIXED) + flags |= IOVMF_DA_ANON; could we use only one? both are mutual exclusive, what happen if flag is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of IOVMF_DA_ANON. Then, what about introducing some MACRO? Better names? #define set_iovmf_da_anon(flags) #define set_iovmf_da_fix(flags) #define set_iovmf_mmio(flags) will they be used by the users? I think people are more used to use iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. than set_iovmf_da_anon(flags) set_iovmf_mmio(flags) iommu_vmap(obj, da, sgt, flags); I don't have problem with the change, but I think how it is now is ok, just that we don't we two bits to handle anon/fixed da, it can be managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Indeed not. :) A new set is coming soon. Br, David Br, David Cohen -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
