* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
The device address is assigned by tidspbridge no need for that parameter anymore.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
.../tidspbridge/include/dspbridge/dspapi-ioctl.h | 1 -
.../staging/tidspbridge/include/dspbridge/proc.h | 3 --
drivers/staging/tidspbridge/pmgr/dspapi.c | 2 +-
drivers/staging/tidspbridge/rmgr/node.c | 2 +-
drivers/staging/tidspbridge/rmgr/proc.c | 25 +++++++++----------
5 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h b/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
index 8da5bd8..db06468 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
@@ -138,7 +138,6 @@ union trapped_args {
void *hprocessor;
void *pmpu_addr;
u32 ul_size;
- void *req_addr;
void *__user *pp_map_addr;
u32 ul_map_attr;
} args_proc_mapmem;
diff --git a/drivers/staging/tidspbridge/include/dspbridge/proc.h b/drivers/staging/tidspbridge/include/dspbridge/proc.h
index 2d12aab..74344bd 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/proc.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/proc.h
@@ -524,8 +524,6 @@ extern int proc_invalidate_memory(void *hprocessor,
* hprocessor : The processor handle.
* pmpu_addr : Starting address of the memory region to map.
* ul_size : Size of the memory region to map.
- * req_addr : Requested DSP start address. Offset-adjusted actual
- * mapped address is in the last argument.
* pp_map_addr : Ptr to DSP side mapped u8 address.
* ul_map_attr : Optional endianness attributes, virt to phys flag.
* Returns:
@@ -546,7 +544,6 @@ extern int proc_invalidate_memory(void *hprocessor,
extern int proc_map(void *hprocessor,
void *pmpu_addr,
u32 ul_size,
- void *req_addr,
void **pp_map_addr, u32 ul_map_attr,
struct process_context *pr_ctxt);
diff --git a/drivers/staging/tidspbridge/pmgr/dspapi.c b/drivers/staging/tidspbridge/pmgr/dspapi.c
index 0187c47..d1e1185 100644
--- a/drivers/staging/tidspbridge/pmgr/dspapi.c
+++ b/drivers/staging/tidspbridge/pmgr/dspapi.c
@@ -952,7 +952,7 @@ u32 procwrap_map(union trapped_args *args, void *pr_ctxt)
status = proc_map(args->args_proc_mapmem.hprocessor,
args->args_proc_mapmem.pmpu_addr,
args->args_proc_mapmem.ul_size,
- args->args_proc_mapmem.req_addr, &map_addr,
+ &map_addr,
args->args_proc_mapmem.ul_map_attr, pr_ctxt);
if (!status) {
if (put_user(map_addr, args->args_proc_mapmem.pp_map_addr)) {
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index a660247..3f5abcf 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -430,7 +430,7 @@ int node_allocate(struct proc_object *hprocessor,
map_attrs |= DSP_MAPVIRTUALADDR;
status = proc_map(hprocessor, (void *)attr_in->pgpp_virt_addr,
pnode->create_args.asa.task_arg_obj.heap_size,
- NULL, (void **)&mapped_addr, map_attrs,
+ (void **)&mapped_addr, map_attrs,
pr_ctxt);
if (status)
pr_err("%s: Failed to map memory for Heap: 0x%x\n",
diff --git a/drivers/staging/tidspbridge/rmgr/proc.c b/drivers/staging/tidspbridge/rmgr/proc.c
index 7a15a02..5e5eb75 100644
--- a/drivers/staging/tidspbridge/rmgr/proc.c
+++ b/drivers/staging/tidspbridge/rmgr/proc.c
@@ -1314,10 +1314,10 @@ func_end:
* Maps a MPU buffer to DSP address space.
*/
int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
- void *req_addr, void **pp_map_addr, u32 ul_map_attr,
+ void **pp_map_addr, u32 ul_map_attr,
struct process_context *pr_ctxt)
{
- u32 va_align;
+ u32 da;
u32 pa_align;
u32 size_align;
int status = 0;
@@ -1336,7 +1336,6 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
#endif
/* Calculate the page-aligned PA, VA and size */
- va_align = PG_ALIGN_LOW((u32) req_addr, PG_SIZE4K);
pa_align = PG_ALIGN_LOW((u32) pmpu_addr, PG_SIZE4K);
size_align = PG_ALIGN_HIGH(ul_size + (u32) pmpu_addr - pa_align,
PG_SIZE4K);
@@ -1351,26 +1350,26 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
/* Add mapping to the page tables. */
if (!status) {
/* mapped memory resource tracking */
- map_obj = add_mapping_info(pr_ctxt, pa_align, va_align,
+ map_obj = add_mapping_info(pr_ctxt, pa_align, 0,
size_align);
if (!map_obj) {
status = -ENOMEM;
} else {
- va_align = user_to_dsp_map(
+ da = user_to_dsp_map(
p_proc_object->hbridge_context->dsp_mmu,
- pa_align, va_align, size_align,
+ pa_align, 0, size_align,
map_obj->pages);
- if (IS_ERR_VALUE(va_align))
- status = (int)va_align;
+ if (IS_ERR_VALUE(da))
+ status = (int)da;
}
}
if (!status) {
/* Mapped address = MSB of VA | LSB of PA */
- map_obj->dsp_addr = (va_align |
+ map_obj->dsp_addr = (da |
((u32)pmpu_addr & (PG_SIZE4K - 1)));
*pp_map_addr = (void *)map_obj->dsp_addr;
} else {
- remove_mapping_information(pr_ctxt, va_align);
+ remove_mapping_information(pr_ctxt, 0);
}
mutex_unlock(&proc_lock);
@@ -1379,10 +1378,10 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
func_end:
dev_dbg(bridge, "%s: hprocessor %p, pmpu_addr %p, ul_size %x, "
- "req_addr %p, ul_map_attr %x, pp_map_addr %p, va_align %x, "
+ "ul_map_attr %x, pp_map_addr %p, "
"pa_align %x, size_align %x status 0x%x\n", __func__,
- hprocessor, pmpu_addr, ul_size, req_addr, ul_map_attr,
- pp_map_addr, va_align, pa_align, size_align, status);
+ hprocessor, pmpu_addr, ul_size, ul_map_attr,
+ pp_map_addr, pa_align, size_align, status);
return status;
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
The device address is assigned by tidspbridge no need for that parameter anymore.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
.../tidspbridge/include/dspbridge/dspapi-ioctl.h | 1 -
.../staging/tidspbridge/include/dspbridge/proc.h | 3 --
drivers/staging/tidspbridge/pmgr/dspapi.c | 2 +-
drivers/staging/tidspbridge/rmgr/node.c | 2 +-
drivers/staging/tidspbridge/rmgr/proc.c | 25 +++++++++----------
5 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h b/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
index 8da5bd8..db06468 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/dspapi-ioctl.h
@@ -138,7 +138,6 @@ union trapped_args {
void *hprocessor;
void *pmpu_addr;
u32 ul_size;
- void *req_addr;
void *__user *pp_map_addr;
u32 ul_map_attr;
} args_proc_mapmem;
diff --git a/drivers/staging/tidspbridge/include/dspbridge/proc.h b/drivers/staging/tidspbridge/include/dspbridge/proc.h
index 2d12aab..74344bd 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/proc.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/proc.h
@@ -524,8 +524,6 @@ extern int proc_invalidate_memory(void *hprocessor,
* hprocessor : The processor handle.
* pmpu_addr : Starting address of the memory region to map.
* ul_size : Size of the memory region to map.
- * req_addr : Requested DSP start address. Offset-adjusted actual
- * mapped address is in the last argument.
* pp_map_addr : Ptr to DSP side mapped u8 address.
* ul_map_attr : Optional endianness attributes, virt to phys flag.
* Returns:
@@ -546,7 +544,6 @@ extern int proc_invalidate_memory(void *hprocessor,
extern int proc_map(void *hprocessor,
void *pmpu_addr,
u32 ul_size,
- void *req_addr,
void **pp_map_addr, u32 ul_map_attr,
struct process_context *pr_ctxt);
diff --git a/drivers/staging/tidspbridge/pmgr/dspapi.c b/drivers/staging/tidspbridge/pmgr/dspapi.c
index 0187c47..d1e1185 100644
--- a/drivers/staging/tidspbridge/pmgr/dspapi.c
+++ b/drivers/staging/tidspbridge/pmgr/dspapi.c
@@ -952,7 +952,7 @@ u32 procwrap_map(union trapped_args *args, void *pr_ctxt)
status = proc_map(args->args_proc_mapmem.hprocessor,
args->args_proc_mapmem.pmpu_addr,
args->args_proc_mapmem.ul_size,
- args->args_proc_mapmem.req_addr, &map_addr,
+ &map_addr,
args->args_proc_mapmem.ul_map_attr, pr_ctxt);
if (!status) {
if (put_user(map_addr, args->args_proc_mapmem.pp_map_addr)) {
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index a660247..3f5abcf 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -430,7 +430,7 @@ int node_allocate(struct proc_object *hprocessor,
map_attrs |= DSP_MAPVIRTUALADDR;
status = proc_map(hprocessor, (void *)attr_in->pgpp_virt_addr,
pnode->create_args.asa.task_arg_obj.heap_size,
- NULL, (void **)&mapped_addr, map_attrs,
+ (void **)&mapped_addr, map_attrs,
pr_ctxt);
if (status)
pr_err("%s: Failed to map memory for Heap: 0x%x\n",
diff --git a/drivers/staging/tidspbridge/rmgr/proc.c b/drivers/staging/tidspbridge/rmgr/proc.c
index 7a15a02..5e5eb75 100644
--- a/drivers/staging/tidspbridge/rmgr/proc.c
+++ b/drivers/staging/tidspbridge/rmgr/proc.c
@@ -1314,10 +1314,10 @@ func_end:
* Maps a MPU buffer to DSP address space.
*/
int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
- void *req_addr, void **pp_map_addr, u32 ul_map_attr,
+ void **pp_map_addr, u32 ul_map_attr,
struct process_context *pr_ctxt)
{
- u32 va_align;
+ u32 da;
u32 pa_align;
u32 size_align;
int status = 0;
@@ -1336,7 +1336,6 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
#endif
/* Calculate the page-aligned PA, VA and size */
- va_align = PG_ALIGN_LOW((u32) req_addr, PG_SIZE4K);
pa_align = PG_ALIGN_LOW((u32) pmpu_addr, PG_SIZE4K);
size_align = PG_ALIGN_HIGH(ul_size + (u32) pmpu_addr - pa_align,
PG_SIZE4K);
@@ -1351,26 +1350,26 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
/* Add mapping to the page tables. */
if (!status) {
/* mapped memory resource tracking */
- map_obj = add_mapping_info(pr_ctxt, pa_align, va_align,
+ map_obj = add_mapping_info(pr_ctxt, pa_align, 0,
size_align);
if (!map_obj) {
status = -ENOMEM;
} else {
- va_align = user_to_dsp_map(
+ da = user_to_dsp_map(
p_proc_object->hbridge_context->dsp_mmu,
- pa_align, va_align, size_align,
+ pa_align, 0, size_align,
map_obj->pages);
- if (IS_ERR_VALUE(va_align))
- status = (int)va_align;
+ if (IS_ERR_VALUE(da))
+ status = (int)da;
}
}
if (!status) {
/* Mapped address = MSB of VA | LSB of PA */
- map_obj->dsp_addr = (va_align |
+ map_obj->dsp_addr = (da |
((u32)pmpu_addr & (PG_SIZE4K - 1)));
*pp_map_addr = (void *)map_obj->dsp_addr;
} else {
- remove_mapping_information(pr_ctxt, va_align);
+ remove_mapping_information(pr_ctxt, 0);
}
mutex_unlock(&proc_lock);
@@ -1379,10 +1378,10 @@ int proc_map(void *hprocessor, void *pmpu_addr, u32 ul_size,
func_end:
dev_dbg(bridge, "%s: hprocessor %p, pmpu_addr %p, ul_size %x, "
- "req_addr %p, ul_map_attr %x, pp_map_addr %p, va_align %x, "
+ "ul_map_attr %x, pp_map_addr %p, "
"pa_align %x, size_align %x status 0x%x\n", __func__,
- hprocessor, pmpu_addr, ul_size, req_addr, ul_map_attr,
- pp_map_addr, va_align, pa_align, size_align, status);
+ hprocessor, pmpu_addr, ul_size, ul_map_attr,
+ pp_map_addr, pa_align, size_align, status);
return status;
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 2/8] staging: tidspbridge - add kconfig parameter for DMM size
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
A new kconfig parameter for DMM size is added. Also DMM is
allocated after the end of SHM area. So that the checks on
DSP are still valid and we avoid using areas between SHM
which are not mapped reducing the probability of shared
memory corruption.
NOTE: This patch has a dependency on this patch:
"omap: iommu - create new api to set valid da range"
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/Kconfig | 8 ++++++++
drivers/staging/tidspbridge/core/tiomap3430.c | 20 ++++++++++++++++++--
.../tidspbridge/include/dspbridge/dsp-mmu.h | 3 +++
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/Kconfig b/drivers/staging/tidspbridge/Kconfig
index 672008f..37e47f5 100644
--- a/drivers/staging/tidspbridge/Kconfig
+++ b/drivers/staging/tidspbridge/Kconfig
@@ -42,6 +42,14 @@ config TIDSPBRIDGE_MEMPOOL_SIZE
Allocate specified size of memory at booting time to avoid allocation
failure under heavy memory fragmentation after some use time.
+config TIDSPBRIDGE_DMM_SIZE
+ hex "DMM capable memory size (Byte)"
+ depends on TIDSPBRIDGE
+ default "0x10000000"
+ help
+ Memory size of DSP virtual address for Dynamic Memory Mapping.
+ Please make sure the size is 4K aligned.
+
config TIDSPBRIDGE_DEBUG
bool "Debug Support"
depends on TIDSPBRIDGE
diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index c28dcf1..88c8c71 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -345,7 +345,6 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
OMAP343X_CONTROL_IVA2_BOOTMOD));
}
}
-
if (!status) {
(*pdata->dsp_prm_rmw_bits)(OMAP3430_RST2_IVA2_MASK, 0,
OMAP3430_IVA2_MOD, OMAP2_RM_RSTCTRL);
@@ -362,6 +361,11 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
if (!status) {
dev_context->dsp_mmu = mmu;
sm_sg = &dev_context->sh_s;
+ /* Set valid range to map shared memory */
+ status = iommu_set_da_range(mmu, sm_sg->seg0_da,
+ sm_sg->seg1_da + sm_sg->seg1_size);
+ }
+ if (!status) {
sg0_da = iommu_kmap(mmu, sm_sg->seg0_da, sm_sg->seg0_pa,
sm_sg->seg0_size, IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_32);
if (IS_ERR_VALUE(sg0_da)) {
@@ -411,7 +415,19 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
l4_i++;
}
}
-
+ if (!status) {
+ /* Set valid range for DMM mappings */
+ if (MAX_DSP_MMU_DA - CONFIG_TIDSPBRIDGE_DMM_SIZE <
+ sm_sg->seg1_da + sm_sg->seg1_size) {
+ dev_err(bridge, "DMM size too big!\n");
+ status = -ENOMEM;
+ } else {
+ status = iommu_set_da_range(mmu,
+ sm_sg->seg1_da + sm_sg->seg1_size,
+ sm_sg->seg1_da + sm_sg->seg1_size +
+ CONFIG_TIDSPBRIDGE_DMM_SIZE);
+ }
+ }
/* Lock the above TLB entries and get the BIOS and load monitor timer
* information */
if (!status) {
diff --git a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
index cb38d4c..bbbe9e6 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
@@ -22,6 +22,9 @@
#include <plat/iommu.h>
#include <plat/iovmm.h>
+/* Last patch is not mapped to detect buffer overflow in DSP side */
+#define MAX_DSP_MMU_DA 0xFFFFF000
+
/**
* dsp_mmu_init() - initialize dsp_mmu module and returns a handle
*
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 2/8] staging: tidspbridge - add kconfig parameter for DMM size
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
A new kconfig parameter for DMM size is added. Also DMM is
allocated after the end of SHM area. So that the checks on
DSP are still valid and we avoid using areas between SHM
which are not mapped reducing the probability of shared
memory corruption.
NOTE: This patch has a dependency on this patch:
"omap: iommu - create new api to set valid da range"
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/Kconfig | 8 ++++++++
drivers/staging/tidspbridge/core/tiomap3430.c | 20 ++++++++++++++++++--
.../tidspbridge/include/dspbridge/dsp-mmu.h | 3 +++
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/Kconfig b/drivers/staging/tidspbridge/Kconfig
index 672008f..37e47f5 100644
--- a/drivers/staging/tidspbridge/Kconfig
+++ b/drivers/staging/tidspbridge/Kconfig
@@ -42,6 +42,14 @@ config TIDSPBRIDGE_MEMPOOL_SIZE
Allocate specified size of memory at booting time to avoid allocation
failure under heavy memory fragmentation after some use time.
+config TIDSPBRIDGE_DMM_SIZE
+ hex "DMM capable memory size (Byte)"
+ depends on TIDSPBRIDGE
+ default "0x10000000"
+ help
+ Memory size of DSP virtual address for Dynamic Memory Mapping.
+ Please make sure the size is 4K aligned.
+
config TIDSPBRIDGE_DEBUG
bool "Debug Support"
depends on TIDSPBRIDGE
diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index c28dcf1..88c8c71 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -345,7 +345,6 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
OMAP343X_CONTROL_IVA2_BOOTMOD));
}
}
-
if (!status) {
(*pdata->dsp_prm_rmw_bits)(OMAP3430_RST2_IVA2_MASK, 0,
OMAP3430_IVA2_MOD, OMAP2_RM_RSTCTRL);
@@ -362,6 +361,11 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
if (!status) {
dev_context->dsp_mmu = mmu;
sm_sg = &dev_context->sh_s;
+ /* Set valid range to map shared memory */
+ status = iommu_set_da_range(mmu, sm_sg->seg0_da,
+ sm_sg->seg1_da + sm_sg->seg1_size);
+ }
+ if (!status) {
sg0_da = iommu_kmap(mmu, sm_sg->seg0_da, sm_sg->seg0_pa,
sm_sg->seg0_size, IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_32);
if (IS_ERR_VALUE(sg0_da)) {
@@ -411,7 +415,19 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
l4_i++;
}
}
-
+ if (!status) {
+ /* Set valid range for DMM mappings */
+ if (MAX_DSP_MMU_DA - CONFIG_TIDSPBRIDGE_DMM_SIZE <
+ sm_sg->seg1_da + sm_sg->seg1_size) {
+ dev_err(bridge, "DMM size too big!\n");
+ status = -ENOMEM;
+ } else {
+ status = iommu_set_da_range(mmu,
+ sm_sg->seg1_da + sm_sg->seg1_size,
+ sm_sg->seg1_da + sm_sg->seg1_size +
+ CONFIG_TIDSPBRIDGE_DMM_SIZE);
+ }
+ }
/* Lock the above TLB entries and get the BIOS and load monitor timer
* information */
if (!status) {
diff --git a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
index cb38d4c..bbbe9e6 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
@@ -22,6 +22,9 @@
#include <plat/iommu.h>
#include <plat/iovmm.h>
+/* Last patch is not mapped to detect buffer overflow in DSP side */
+#define MAX_DSP_MMU_DA 0xFFFFF000
+
/**
* dsp_mmu_init() - initialize dsp_mmu module and returns a handle
*
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 2/8] staging: tidspbridge - add kconfig parameter for DMM size
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
A new kconfig parameter for DMM size is added. Also DMM is
allocated after the end of SHM area. So that the checks on
DSP are still valid and we avoid using areas between SHM
which are not mapped reducing the probability of shared
memory corruption.
NOTE: This patch has a dependency on this patch:
"omap: iommu - create new api to set valid da range"
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/Kconfig | 8 ++++++++
drivers/staging/tidspbridge/core/tiomap3430.c | 20 ++++++++++++++++++--
.../tidspbridge/include/dspbridge/dsp-mmu.h | 3 +++
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/Kconfig b/drivers/staging/tidspbridge/Kconfig
index 672008f..37e47f5 100644
--- a/drivers/staging/tidspbridge/Kconfig
+++ b/drivers/staging/tidspbridge/Kconfig
@@ -42,6 +42,14 @@ config TIDSPBRIDGE_MEMPOOL_SIZE
Allocate specified size of memory at booting time to avoid allocation
failure under heavy memory fragmentation after some use time.
+config TIDSPBRIDGE_DMM_SIZE
+ hex "DMM capable memory size (Byte)"
+ depends on TIDSPBRIDGE
+ default "0x10000000"
+ help
+ Memory size of DSP virtual address for Dynamic Memory Mapping.
+ Please make sure the size is 4K aligned.
+
config TIDSPBRIDGE_DEBUG
bool "Debug Support"
depends on TIDSPBRIDGE
diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index c28dcf1..88c8c71 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -345,7 +345,6 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
OMAP343X_CONTROL_IVA2_BOOTMOD));
}
}
-
if (!status) {
(*pdata->dsp_prm_rmw_bits)(OMAP3430_RST2_IVA2_MASK, 0,
OMAP3430_IVA2_MOD, OMAP2_RM_RSTCTRL);
@@ -362,6 +361,11 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
if (!status) {
dev_context->dsp_mmu = mmu;
sm_sg = &dev_context->sh_s;
+ /* Set valid range to map shared memory */
+ status = iommu_set_da_range(mmu, sm_sg->seg0_da,
+ sm_sg->seg1_da + sm_sg->seg1_size);
+ }
+ if (!status) {
sg0_da = iommu_kmap(mmu, sm_sg->seg0_da, sm_sg->seg0_pa,
sm_sg->seg0_size, IOVMF_ENDIAN_LITTLE | IOVMF_ELSZ_32);
if (IS_ERR_VALUE(sg0_da)) {
@@ -411,7 +415,19 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
l4_i++;
}
}
-
+ if (!status) {
+ /* Set valid range for DMM mappings */
+ if (MAX_DSP_MMU_DA - CONFIG_TIDSPBRIDGE_DMM_SIZE <
+ sm_sg->seg1_da + sm_sg->seg1_size) {
+ dev_err(bridge, "DMM size too big!\n");
+ status = -ENOMEM;
+ } else {
+ status = iommu_set_da_range(mmu,
+ sm_sg->seg1_da + sm_sg->seg1_size,
+ sm_sg->seg1_da + sm_sg->seg1_size +
+ CONFIG_TIDSPBRIDGE_DMM_SIZE);
+ }
+ }
/* Lock the above TLB entries and get the BIOS and load monitor timer
* information */
if (!status) {
diff --git a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
index cb38d4c..bbbe9e6 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/dsp-mmu.h
@@ -22,6 +22,9 @@
#include <plat/iommu.h>
#include <plat/iovmm.h>
+/* Last patch is not mapped to detect buffer overflow in DSP side */
+#define MAX_DSP_MMU_DA 0xFFFFF000
+
/**
* dsp_mmu_init() - initialize dsp_mmu module and returns a handle
*
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 3/8] staging: tidspbridge - change mmufault tasklet to a workqueue
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
We don't need to manage the mmufault inside a tasklet
it is safer using a workqueue.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 34 ++++++++++++++++++----------
1 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 983c95a..6d7501a 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -28,7 +28,8 @@
#define MMU_CNTL_TWL_EN (1 << 2)
-static struct tasklet_struct mmu_tasklet;
+static struct workqueue_struct *mmu_wq;
+static struct work_struct fault_work;
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
@@ -37,7 +38,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
u32 fa, tmp;
struct iotlb_entry e;
struct iommu *mmu = dev_context->dsp_mmu;
- dummy_addr = (void *)__get_free_page(GFP_ATOMIC);
+
+ dummy_addr = (void *)__get_free_page(GFP_KERNEL);
+ if (!dummy_addr)
+ return;
/*
* Before acking the MMU fault, let's make sure MMU can only
@@ -76,19 +80,19 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
#endif
-static void fault_tasklet(unsigned long data)
+static void mmu_fault_work(struct work_struct *work)
{
- struct iommu *mmu = (struct iommu *)data;
struct bridge_dev_context *dev_ctx;
struct deh_mgr *dm;
u32 fa;
+
dev_get_deh_mgr(dev_get_first(), &dm);
dev_get_bridge_context(dev_get_first(), &dev_ctx);
if (!dm || !dev_ctx)
return;
- fa = iommu_read_reg(mmu, MMU_FAULT_AD);
+ fa = iommu_read_reg(dev_ctx->dsp_mmu, MMU_FAULT_AD);
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
print_dsp_trace_buffer(dev_ctx);
@@ -109,7 +113,8 @@ static int mmu_fault_callback(struct iommu *mmu)
return -EPERM;
iommu_write_reg(mmu, 0, MMU_IRQENABLE);
- tasklet_schedule(&mmu_tasklet);
+ queue_work(mmu_wq, &fault_work);
+
return 0;
}
@@ -126,10 +131,16 @@ struct iommu *dsp_mmu_init()
mmu = iommu_get("iva2");
- if (!IS_ERR(mmu)) {
- tasklet_init(&mmu_tasklet, fault_tasklet, (unsigned long)mmu);
- mmu->isr = mmu_fault_callback;
+ if (IS_ERR(mmu))
+ return mmu;
+
+ mmu->isr = mmu_fault_callback;
+ mmu_wq = create_workqueue("dsp-mmu_wq");
+ if (!mmu_wq) {
+ iommu_put(mmu);
+ return ERR_PTR(-ENOMEM);
}
+ INIT_WORK(&fault_work, mmu_fault_work);
return mmu;
}
@@ -143,9 +154,8 @@ struct iommu *dsp_mmu_init()
*/
void dsp_mmu_exit(struct iommu *mmu)
{
- if (mmu)
- iommu_put(mmu);
- tasklet_kill(&mmu_tasklet);
+ iommu_put(mmu);
+ destroy_workqueue(mmu_wq);
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 3/8] staging: tidspbridge - change mmufault tasklet to a workqueue
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
We don't need to manage the mmufault inside a tasklet
it is safer using a workqueue.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 34 ++++++++++++++++++----------
1 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 983c95a..6d7501a 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -28,7 +28,8 @@
#define MMU_CNTL_TWL_EN (1 << 2)
-static struct tasklet_struct mmu_tasklet;
+static struct workqueue_struct *mmu_wq;
+static struct work_struct fault_work;
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
@@ -37,7 +38,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
u32 fa, tmp;
struct iotlb_entry e;
struct iommu *mmu = dev_context->dsp_mmu;
- dummy_addr = (void *)__get_free_page(GFP_ATOMIC);
+
+ dummy_addr = (void *)__get_free_page(GFP_KERNEL);
+ if (!dummy_addr)
+ return;
/*
* Before acking the MMU fault, let's make sure MMU can only
@@ -76,19 +80,19 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
#endif
-static void fault_tasklet(unsigned long data)
+static void mmu_fault_work(struct work_struct *work)
{
- struct iommu *mmu = (struct iommu *)data;
struct bridge_dev_context *dev_ctx;
struct deh_mgr *dm;
u32 fa;
+
dev_get_deh_mgr(dev_get_first(), &dm);
dev_get_bridge_context(dev_get_first(), &dev_ctx);
if (!dm || !dev_ctx)
return;
- fa = iommu_read_reg(mmu, MMU_FAULT_AD);
+ fa = iommu_read_reg(dev_ctx->dsp_mmu, MMU_FAULT_AD);
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
print_dsp_trace_buffer(dev_ctx);
@@ -109,7 +113,8 @@ static int mmu_fault_callback(struct iommu *mmu)
return -EPERM;
iommu_write_reg(mmu, 0, MMU_IRQENABLE);
- tasklet_schedule(&mmu_tasklet);
+ queue_work(mmu_wq, &fault_work);
+
return 0;
}
@@ -126,10 +131,16 @@ struct iommu *dsp_mmu_init()
mmu = iommu_get("iva2");
- if (!IS_ERR(mmu)) {
- tasklet_init(&mmu_tasklet, fault_tasklet, (unsigned long)mmu);
- mmu->isr = mmu_fault_callback;
+ if (IS_ERR(mmu))
+ return mmu;
+
+ mmu->isr = mmu_fault_callback;
+ mmu_wq = create_workqueue("dsp-mmu_wq");
+ if (!mmu_wq) {
+ iommu_put(mmu);
+ return ERR_PTR(-ENOMEM);
}
+ INIT_WORK(&fault_work, mmu_fault_work);
return mmu;
}
@@ -143,9 +154,8 @@ struct iommu *dsp_mmu_init()
*/
void dsp_mmu_exit(struct iommu *mmu)
{
- if (mmu)
- iommu_put(mmu);
- tasklet_kill(&mmu_tasklet);
+ iommu_put(mmu);
+ destroy_workqueue(mmu_wq);
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 3/8] staging: tidspbridge - change mmufault tasklet to a workqueue
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
We don't need to manage the mmufault inside a tasklet
it is safer using a workqueue.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 34 ++++++++++++++++++----------
1 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 983c95a..6d7501a 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -28,7 +28,8 @@
#define MMU_CNTL_TWL_EN (1 << 2)
-static struct tasklet_struct mmu_tasklet;
+static struct workqueue_struct *mmu_wq;
+static struct work_struct fault_work;
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
@@ -37,7 +38,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
u32 fa, tmp;
struct iotlb_entry e;
struct iommu *mmu = dev_context->dsp_mmu;
- dummy_addr = (void *)__get_free_page(GFP_ATOMIC);
+
+ dummy_addr = (void *)__get_free_page(GFP_KERNEL);
+ if (!dummy_addr)
+ return;
/*
* Before acking the MMU fault, let's make sure MMU can only
@@ -76,19 +80,19 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
#endif
-static void fault_tasklet(unsigned long data)
+static void mmu_fault_work(struct work_struct *work)
{
- struct iommu *mmu = (struct iommu *)data;
struct bridge_dev_context *dev_ctx;
struct deh_mgr *dm;
u32 fa;
+
dev_get_deh_mgr(dev_get_first(), &dm);
dev_get_bridge_context(dev_get_first(), &dev_ctx);
if (!dm || !dev_ctx)
return;
- fa = iommu_read_reg(mmu, MMU_FAULT_AD);
+ fa = iommu_read_reg(dev_ctx->dsp_mmu, MMU_FAULT_AD);
#ifdef CONFIG_TIDSPBRIDGE_BACKTRACE
print_dsp_trace_buffer(dev_ctx);
@@ -109,7 +113,8 @@ static int mmu_fault_callback(struct iommu *mmu)
return -EPERM;
iommu_write_reg(mmu, 0, MMU_IRQENABLE);
- tasklet_schedule(&mmu_tasklet);
+ queue_work(mmu_wq, &fault_work);
+
return 0;
}
@@ -126,10 +131,16 @@ struct iommu *dsp_mmu_init()
mmu = iommu_get("iva2");
- if (!IS_ERR(mmu)) {
- tasklet_init(&mmu_tasklet, fault_tasklet, (unsigned long)mmu);
- mmu->isr = mmu_fault_callback;
+ if (IS_ERR(mmu))
+ return mmu;
+
+ mmu->isr = mmu_fault_callback;
+ mmu_wq = create_workqueue("dsp-mmu_wq");
+ if (!mmu_wq) {
+ iommu_put(mmu);
+ return ERR_PTR(-ENOMEM);
}
+ INIT_WORK(&fault_work, mmu_fault_work);
return mmu;
}
@@ -143,9 +154,8 @@ struct iommu *dsp_mmu_init()
*/
void dsp_mmu_exit(struct iommu *mmu)
{
- if (mmu)
- iommu_put(mmu);
- tasklet_kill(&mmu_tasklet);
+ iommu_put(mmu);
+ destroy_workqueue(mmu_wq);
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 4/8] staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
Timeout was not being initialized correctly and should
use time_is-before_jiffies, also make it a parameter
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-clock.c | 13 +++++++------
drivers/staging/tidspbridge/core/dsp-mmu.c | 5 ++++-
.../staging/tidspbridge/include/dspbridge/clk.h | 4 +++-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index 46d17c7..8106d1e 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -202,13 +202,13 @@ static void mcbsp_clk_prepare(bool flag, u8 id)
* Sets an overflow interrupt for the desired GPT waiting for a timeout
* of 5 msecs for the interrupt to occur.
*/
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout)
{
struct omap_dm_timer *gpt = timer[clk_id - 1];
- unsigned long timeout;
if (!gpt)
- return;
+ return -EINVAL;
/* Enable overflow interrupt */
omap_dm_timer_set_int_enable(gpt, OMAP_TIMER_INT_OVERFLOW);
@@ -222,14 +222,15 @@ void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
/* Wait 80us for timer to overflow */
udelay(80);
- timeout = msecs_to_jiffies(5);
+ timeout = jiffies + msecs_to_jiffies(timeout);
/* Check interrupt status and wait for interrupt */
while (!(omap_dm_timer_read_status(gpt) & OMAP_TIMER_INT_OVERFLOW)) {
- if (time_is_after_jiffies(timeout)) {
+ if (time_is_before_jiffies(timeout)) {
pr_err("%s: GPTimer interrupt failed\n", __func__);
- break;
+ return -ETIME;
}
}
+ return 0;
}
/*
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 6d7501a..2d4e897 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -65,7 +65,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
dsp_clk_enable(DSP_CLK_GPT8);
- dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe);
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ pr_err("%s: error sending interrupt to DSP\n", __func__);
+ return;
+ }
/* Clear MMU interrupt */
tmp = iommu_read_reg(mmu, MMU_IRQSTATUS);
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index b239503..6fe1ff2 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,7 +62,9 @@ extern void dsp_clk_exit(void);
*/
extern void dsp_clk_init(void);
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout);
+
/*
* ======== dsp_clk_enable ========
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 4/8] staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
Timeout was not being initialized correctly and should
use time_is-before_jiffies, also make it a parameter
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-clock.c | 13 +++++++------
drivers/staging/tidspbridge/core/dsp-mmu.c | 5 ++++-
.../staging/tidspbridge/include/dspbridge/clk.h | 4 +++-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index 46d17c7..8106d1e 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -202,13 +202,13 @@ static void mcbsp_clk_prepare(bool flag, u8 id)
* Sets an overflow interrupt for the desired GPT waiting for a timeout
* of 5 msecs for the interrupt to occur.
*/
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout)
{
struct omap_dm_timer *gpt = timer[clk_id - 1];
- unsigned long timeout;
if (!gpt)
- return;
+ return -EINVAL;
/* Enable overflow interrupt */
omap_dm_timer_set_int_enable(gpt, OMAP_TIMER_INT_OVERFLOW);
@@ -222,14 +222,15 @@ void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
/* Wait 80us for timer to overflow */
udelay(80);
- timeout = msecs_to_jiffies(5);
+ timeout = jiffies + msecs_to_jiffies(timeout);
/* Check interrupt status and wait for interrupt */
while (!(omap_dm_timer_read_status(gpt) & OMAP_TIMER_INT_OVERFLOW)) {
- if (time_is_after_jiffies(timeout)) {
+ if (time_is_before_jiffies(timeout)) {
pr_err("%s: GPTimer interrupt failed\n", __func__);
- break;
+ return -ETIME;
}
}
+ return 0;
}
/*
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 6d7501a..2d4e897 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -65,7 +65,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
dsp_clk_enable(DSP_CLK_GPT8);
- dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe);
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ pr_err("%s: error sending interrupt to DSP\n", __func__);
+ return;
+ }
/* Clear MMU interrupt */
tmp = iommu_read_reg(mmu, MMU_IRQSTATUS);
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index b239503..6fe1ff2 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,7 +62,9 @@ extern void dsp_clk_exit(void);
*/
extern void dsp_clk_init(void);
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout);
+
/*
* ======== dsp_clk_enable ========
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 4/8] staging: tidspbridge - fix timeout in dsp_gpt_wait_overflow
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
Timeout was not being initialized correctly and should
use time_is-before_jiffies, also make it a parameter
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-clock.c | 13 +++++++------
drivers/staging/tidspbridge/core/dsp-mmu.c | 5 ++++-
.../staging/tidspbridge/include/dspbridge/clk.h | 4 +++-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index 46d17c7..8106d1e 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -202,13 +202,13 @@ static void mcbsp_clk_prepare(bool flag, u8 id)
* Sets an overflow interrupt for the desired GPT waiting for a timeout
* of 5 msecs for the interrupt to occur.
*/
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout)
{
struct omap_dm_timer *gpt = timer[clk_id - 1];
- unsigned long timeout;
if (!gpt)
- return;
+ return -EINVAL;
/* Enable overflow interrupt */
omap_dm_timer_set_int_enable(gpt, OMAP_TIMER_INT_OVERFLOW);
@@ -222,14 +222,15 @@ void dsp_gpt_wait_overflow(short int clk_id, unsigned int load)
/* Wait 80us for timer to overflow */
udelay(80);
- timeout = msecs_to_jiffies(5);
+ timeout = jiffies + msecs_to_jiffies(timeout);
/* Check interrupt status and wait for interrupt */
while (!(omap_dm_timer_read_status(gpt) & OMAP_TIMER_INT_OVERFLOW)) {
- if (time_is_after_jiffies(timeout)) {
+ if (time_is_before_jiffies(timeout)) {
pr_err("%s: GPTimer interrupt failed\n", __func__);
- break;
+ return -ETIME;
}
}
+ return 0;
}
/*
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 6d7501a..2d4e897 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -65,7 +65,10 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
dsp_clk_enable(DSP_CLK_GPT8);
- dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe);
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ pr_err("%s: error sending interrupt to DSP\n", __func__);
+ return;
+ }
/* Clear MMU interrupt */
tmp = iommu_read_reg(mmu, MMU_IRQSTATUS);
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index b239503..6fe1ff2 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,7 +62,9 @@ extern void dsp_clk_exit(void);
*/
extern void dsp_clk_init(void);
-void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
+int dsp_gpt_wait_overflow(short int clk_id, unsigned int load,
+ unsigned long timeout);
+
/*
* ======== dsp_clk_enable ========
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 5/8] staging: tidspbridge - use GTP7 for DSP stack dump
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
DSP stack dump is changed to GTP7 due to GPT8 is used
by DSP side apps
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 2d4e897..157b743 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -63,9 +63,9 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
load_iotlb_entry(mmu, &e);
- dsp_clk_enable(DSP_CLK_GPT8);
+ dsp_clk_enable(DSP_CLK_GPT7);
- if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe, 10)) {
pr_err("%s: error sending interrupt to DSP\n", __func__);
return;
}
@@ -75,7 +75,7 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
iommu_write_reg(mmu, tmp, MMU_IRQSTATUS);
dump_dsp_stack(dev_context);
- dsp_clk_disable(DSP_CLK_GPT8);
+ dsp_clk_disable(DSP_CLK_GPT7);
iopgtable_clear_entry(mmu, fa);
free_page((unsigned long)dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 5/8] staging: tidspbridge - use GTP7 for DSP stack dump
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
DSP stack dump is changed to GTP7 due to GPT8 is used
by DSP side apps
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 2d4e897..157b743 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -63,9 +63,9 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
load_iotlb_entry(mmu, &e);
- dsp_clk_enable(DSP_CLK_GPT8);
+ dsp_clk_enable(DSP_CLK_GPT7);
- if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe, 10)) {
pr_err("%s: error sending interrupt to DSP\n", __func__);
return;
}
@@ -75,7 +75,7 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
iommu_write_reg(mmu, tmp, MMU_IRQSTATUS);
dump_dsp_stack(dev_context);
- dsp_clk_disable(DSP_CLK_GPT8);
+ dsp_clk_disable(DSP_CLK_GPT7);
iopgtable_clear_entry(mmu, fa);
free_page((unsigned long)dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 5/8] staging: tidspbridge - use GTP7 for DSP stack dump
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
DSP stack dump is changed to GTP7 due to GPT8 is used
by DSP side apps
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 2d4e897..157b743 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -63,9 +63,9 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
load_iotlb_entry(mmu, &e);
- dsp_clk_enable(DSP_CLK_GPT8);
+ dsp_clk_enable(DSP_CLK_GPT7);
- if (dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe, 10)) {
+ if (dsp_gpt_wait_overflow(DSP_CLK_GPT7, 0xfffffffe, 10)) {
pr_err("%s: error sending interrupt to DSP\n", __func__);
return;
}
@@ -75,7 +75,7 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
iommu_write_reg(mmu, tmp, MMU_IRQSTATUS);
dump_dsp_stack(dev_context);
- dsp_clk_disable(DSP_CLK_GPT8);
+ dsp_clk_disable(DSP_CLK_GPT7);
iopgtable_clear_entry(mmu, fa);
free_page((unsigned long)dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 6/8] staging: tidspbridge - remove disabling twl when printing DSP stack
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
Now the SHM segments are not in lock tlbs, instead
they are in translation tables. So we cannot disable
twl in order to get the DSP stack dump.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 157b743..3a00087 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -43,14 +43,6 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
if (!dummy_addr)
return;
- /*
- * Before acking the MMU fault, let's make sure MMU can only
- * access entry #0. Then add a new entry so that the DSP OS
- * can continue in order to dump the stack.
- */
- tmp = iommu_read_reg(mmu, MMU_CNTL);
- tmp &= ~MMU_CNTL_TWL_EN;
- iommu_write_reg(mmu, tmp, MMU_CNTL);
fa = iommu_read_reg(mmu, MMU_FAULT_AD);
e.da = fa & PAGE_MASK;
e.pa = virt_to_phys(dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 6/8] staging: tidspbridge - remove disabling twl when printing DSP stack
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
Now the SHM segments are not in lock tlbs, instead
they are in translation tables. So we cannot disable
twl in order to get the DSP stack dump.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 157b743..3a00087 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -43,14 +43,6 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
if (!dummy_addr)
return;
- /*
- * Before acking the MMU fault, let's make sure MMU can only
- * access entry #0. Then add a new entry so that the DSP OS
- * can continue in order to dump the stack.
- */
- tmp = iommu_read_reg(mmu, MMU_CNTL);
- tmp &= ~MMU_CNTL_TWL_EN;
- iommu_write_reg(mmu, tmp, MMU_CNTL);
fa = iommu_read_reg(mmu, MMU_FAULT_AD);
e.da = fa & PAGE_MASK;
e.pa = virt_to_phys(dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 6/8] staging: tidspbridge - remove disabling twl when printing DSP stack
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
Now the SHM segments are not in lock tlbs, instead
they are in translation tables. So we cannot disable
twl in order to get the DSP stack dump.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 157b743..3a00087 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -43,14 +43,6 @@ static void mmu_fault_print_stack(struct bridge_dev_context *dev_context)
if (!dummy_addr)
return;
- /*
- * Before acking the MMU fault, let's make sure MMU can only
- * access entry #0. Then add a new entry so that the DSP OS
- * can continue in order to dump the stack.
- */
- tmp = iommu_read_reg(mmu, MMU_CNTL);
- tmp &= ~MMU_CNTL_TWL_EN;
- iommu_write_reg(mmu, tmp, MMU_CNTL);
fa = iommu_read_reg(mmu, MMU_FAULT_AD);
e.da = fa & PAGE_MASK;
e.pa = virt_to_phys(dummy_addr);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 7/8] staging: tidspbridge - fix some issues after iommu patches
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: Hiroshi.DOYU, linux-kernel, felipe.contreras, andy.shevchenko,
Fernando Guzman Lugo, linux-omap, linux-arm-kernel
This patch fixes:
* In delete_node() we need to check for udsp_heap_addr in order
to unmap the head instead of udsp_heap_res_addr which used
to be the reserved memory a not valid anymore.
* Fix in get_io_pages() as pointed by Felipe Contreras in this
mail:
http://marc.info/?l=linux-kernel&m=128735502205183&w=2
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 2 +-
drivers/staging/tidspbridge/rmgr/node.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 3a00087..54f3ba4 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -196,7 +196,7 @@ static int get_io_pages(struct mm_struct *mm, u32 uva, unsigned pages,
int i;
struct page *pg;
- for (i = 0; i < pages; i++) {
+ for (i = 0; i < pages; i++, uva += PAGE_SIZE) {
pa = user_va2_pa(mm, uva);
if (!pfn_valid(__phys_to_pfn(pa)))
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 3f5abcf..f7fe6c0 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2541,7 +2541,7 @@ static void delete_node(struct node_object *hnode,
kfree(task_arg_obj.strm_out_def);
task_arg_obj.strm_out_def = NULL;
}
- if (task_arg_obj.udsp_heap_res_addr) {
+ if (task_arg_obj.udsp_heap_addr) {
status = proc_un_map(hnode->hprocessor, (void *)
task_arg_obj.udsp_heap_addr,
pr_ctxt);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 7/8] staging: tidspbridge - fix some issues after iommu patches
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
This patch fixes:
* In delete_node() we need to check for udsp_heap_addr in order
to unmap the head instead of udsp_heap_res_addr which used
to be the reserved memory a not valid anymore.
* Fix in get_io_pages() as pointed by Felipe Contreras in this
mail:
http://marc.info/?l=linux-kernel&m=128735502205183&w=2
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 2 +-
drivers/staging/tidspbridge/rmgr/node.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 3a00087..54f3ba4 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -196,7 +196,7 @@ static int get_io_pages(struct mm_struct *mm, u32 uva, unsigned pages,
int i;
struct page *pg;
- for (i = 0; i < pages; i++) {
+ for (i = 0; i < pages; i++, uva += PAGE_SIZE) {
pa = user_va2_pa(mm, uva);
if (!pfn_valid(__phys_to_pfn(pa)))
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 3f5abcf..f7fe6c0 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2541,7 +2541,7 @@ static void delete_node(struct node_object *hnode,
kfree(task_arg_obj.strm_out_def);
task_arg_obj.strm_out_def = NULL;
}
- if (task_arg_obj.udsp_heap_res_addr) {
+ if (task_arg_obj.udsp_heap_addr) {
status = proc_un_map(hnode->hprocessor, (void *)
task_arg_obj.udsp_heap_addr,
pr_ctxt);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 7/8] staging: tidspbridge - fix some issues after iommu patches
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes:
* In delete_node() we need to check for udsp_heap_addr in order
to unmap the head instead of udsp_heap_res_addr which used
to be the reserved memory a not valid anymore.
* Fix in get_io_pages() as pointed by Felipe Contreras in this
mail:
http://marc.info/?l=linux-kernel&m=128735502205183&w=2
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
drivers/staging/tidspbridge/core/dsp-mmu.c | 2 +-
drivers/staging/tidspbridge/rmgr/node.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/core/dsp-mmu.c b/drivers/staging/tidspbridge/core/dsp-mmu.c
index 3a00087..54f3ba4 100644
--- a/drivers/staging/tidspbridge/core/dsp-mmu.c
+++ b/drivers/staging/tidspbridge/core/dsp-mmu.c
@@ -196,7 +196,7 @@ static int get_io_pages(struct mm_struct *mm, u32 uva, unsigned pages,
int i;
struct page *pg;
- for (i = 0; i < pages; i++) {
+ for (i = 0; i < pages; i++, uva += PAGE_SIZE) {
pa = user_va2_pa(mm, uva);
if (!pfn_valid(__phys_to_pfn(pa)))
diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 3f5abcf..f7fe6c0 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2541,7 +2541,7 @@ static void delete_node(struct node_object *hnode,
kfree(task_arg_obj.strm_out_def);
task_arg_obj.strm_out_def = NULL;
}
- if (task_arg_obj.udsp_heap_res_addr) {
+ if (task_arg_obj.udsp_heap_addr) {
status = proc_un_map(hnode->hprocessor, (void *)
task_arg_obj.udsp_heap_addr,
pr_ctxt);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 0:51 ` Fernando Guzman Lugo
(?)
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
-1 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: Hiroshi.DOYU, linux-kernel, felipe.contreras, andy.shevchenko,
Fernando Guzman Lugo, linux-omap, linux-arm-kernel
So that avoid non-killable process.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
.../staging/tidspbridge/include/dspbridge/sync.h | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/include/dspbridge/sync.h b/drivers/staging/tidspbridge/include/dspbridge/sync.h
index e2651e7..df05b8f 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/sync.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/sync.h
@@ -80,13 +80,22 @@ void sync_set_event(struct sync_object *event);
* This functios will wait until @event is set or until timeout. In case of
* success the function will return 0 and
* in case of timeout the function will return -ETIME
+ * in case of signal the function will return -ERESTARTSYS
*/
static inline int sync_wait_on_event(struct sync_object *event,
unsigned timeout)
{
- return wait_for_completion_timeout(&event->comp,
- msecs_to_jiffies(timeout)) ? 0 : -ETIME;
+ int res;
+
+ res = wait_for_completion_interruptible_timeout(&event->comp,
+ msecs_to_jiffies(timeout));
+ if (!res)
+ res = -ETIME;
+ else if (res > 0)
+ res = 0;
+
+ return res;
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: gregkh
Cc: felipe.contreras, Hiroshi.DOYU, linux-kernel, andy.shevchenko,
linux-omap, linux-arm-kernel, Fernando Guzman Lugo
So that avoid non-killable process.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
.../staging/tidspbridge/include/dspbridge/sync.h | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/include/dspbridge/sync.h b/drivers/staging/tidspbridge/include/dspbridge/sync.h
index e2651e7..df05b8f 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/sync.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/sync.h
@@ -80,13 +80,22 @@ void sync_set_event(struct sync_object *event);
* This functios will wait until @event is set or until timeout. In case of
* success the function will return 0 and
* in case of timeout the function will return -ETIME
+ * in case of signal the function will return -ERESTARTSYS
*/
static inline int sync_wait_on_event(struct sync_object *event,
unsigned timeout)
{
- return wait_for_completion_timeout(&event->comp,
- msecs_to_jiffies(timeout)) ? 0 : -ETIME;
+ int res;
+
+ res = wait_for_completion_interruptible_timeout(&event->comp,
+ msecs_to_jiffies(timeout));
+ if (!res)
+ res = -ETIME;
+ else if (res > 0)
+ res = 0;
+
+ return res;
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 0:51 ` Fernando Guzman Lugo
0 siblings, 0 replies; 70+ messages in thread
From: Fernando Guzman Lugo @ 2010-10-26 0:51 UTC (permalink / raw)
To: linux-arm-kernel
So that avoid non-killable process.
Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
.../staging/tidspbridge/include/dspbridge/sync.h | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/tidspbridge/include/dspbridge/sync.h b/drivers/staging/tidspbridge/include/dspbridge/sync.h
index e2651e7..df05b8f 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/sync.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/sync.h
@@ -80,13 +80,22 @@ void sync_set_event(struct sync_object *event);
* This functios will wait until @event is set or until timeout. In case of
* success the function will return 0 and
* in case of timeout the function will return -ETIME
+ * in case of signal the function will return -ERESTARTSYS
*/
static inline int sync_wait_on_event(struct sync_object *event,
unsigned timeout)
{
- return wait_for_completion_timeout(&event->comp,
- msecs_to_jiffies(timeout)) ? 0 : -ETIME;
+ int res;
+
+ res = wait_for_completion_interruptible_timeout(&event->comp,
+ msecs_to_jiffies(timeout));
+ if (!res)
+ res = -ETIME;
+ else if (res > 0)
+ res = 0;
+
+ return res;
}
/**
--
1.6.3.3
^ permalink raw reply related [flat|nested] 70+ messages in thread* Re: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 0:51 ` Fernando Guzman Lugo
@ 2010-10-26 0:58 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 0:58 UTC (permalink / raw)
To: Fernando Guzman Lugo
Cc: gregkh, felipe.contreras, Hiroshi.DOYU, linux-kernel,
andy.shevchenko, linux-omap, linux-arm-kernel
On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo <x0095840@ti.com> wrote:
> So that avoid non-killable process.
It would be useful to interrupt these tasks from user-space. A
separate ioctl to do that would be needed.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 0:58 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 0:58 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo <x0095840@ti.com> wrote:
> So that avoid non-killable process.
It would be useful to interrupt these tasks from user-space. A
separate ioctl to do that would be needed.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 0:58 ` Felipe Contreras
@ 2010-10-26 15:50 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 15:50 UTC (permalink / raw)
To: Felipe Contreras
Cc: gregkh@suse.de, felipe.contreras@nokia.com,
Hiroshi.DOYU@nokia.com, linux-kernel@vger.kernel.org,
andy.shevchenko@gmail.com, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Monday, October 25, 2010 7:59 PM
> To: Guzman Lugo, Fernando
> Cc: gregkh@suse.de; felipe.contreras@nokia.com;
> Hiroshi.DOYU@nokia.com; linux-kernel@vger.kernel.org;
> andy.shevchenko@gmail.com; linux-omap@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> <x0095840@ti.com> wrote:
> > So that avoid non-killable process.
>
> It would be useful to interrupt these tasks from user-space.
> A separate ioctl to do that would be needed.
I don't see use case where that could be needed. It is only
To avoid a nonkillable task in the case the user pass an infinite
Timeout.
If you have some test case where that ioctl would be needed
Please share it in order to find the best solution.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 15:50 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 15:50 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at gmail.com]
> Sent: Monday, October 25, 2010 7:59 PM
> To: Guzman Lugo, Fernando
> Cc: gregkh at suse.de; felipe.contreras at nokia.com;
> Hiroshi.DOYU at nokia.com; linux-kernel at vger.kernel.org;
> andy.shevchenko at gmail.com; linux-omap at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> <x0095840@ti.com> wrote:
> > So that avoid non-killable process.
>
> It would be useful to interrupt these tasks from user-space.
> A separate ioctl to do that would be needed.
I don't see use case where that could be needed. It is only
To avoid a nonkillable task in the case the user pass an infinite
Timeout.
If you have some test case where that ioctl would be needed
Please share it in order to find the best solution.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 15:50 ` Guzman Lugo, Fernando
@ 2010-10-26 17:03 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 17:03 UTC (permalink / raw)
To: fernando.lugo, felipe.contreras
Cc: gregkh, hiroshi.doyu, linux-kernel, andy.shevchenko, linux-omap,
linux-arm-kernel
fernando.lugo@ti.com wrote:
> > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > <x0095840@ti.com> wrote:
> > > So that avoid non-killable process.
> >
> > It would be useful to interrupt these tasks from user-space.
> > A separate ioctl to do that would be needed.
>
> I don't see use case where that could be needed. It is only
> To avoid a nonkillable task in the case the user pass an infinite
> Timeout.
>
> If you have some test case where that ioctl would be needed
> Please share it in order to find the best solution.
Well, imagine the application is using a library to access the DSP, and
the library has a dedicated thread listening for DSP events in a loop.
This happens to be how libomxil-ti and gst-dsp work.
Now, the thread received the last message, but has set a timeout of 10s,
or even worst, no timeout at all.
After realizing that was the last message, the main thread decides to
shut down, but it has to wait for the DSP thread to join. Unfortunately
the DSP thread is stuck waiting for events, and there's nothing that can
be done.
However, if we have a separate ioctl to interrupt that task, then the
main thread can issue that ioctl, and unlock the DSP thread without
having to wait 10s, or forever.
Does that make sense?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 17:03 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 17:03 UTC (permalink / raw)
To: linux-arm-kernel
fernando.lugo at ti.com wrote:
> > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > <x0095840@ti.com> wrote:
> > > So that avoid non-killable process.
> >
> > It would be useful to interrupt these tasks from user-space.
> > A separate ioctl to do that would be needed.
>
> I don't see use case where that could be needed. It is only
> To avoid a nonkillable task in the case the user pass an infinite
> Timeout.
>
> If you have some test case where that ioctl would be needed
> Please share it in order to find the best solution.
Well, imagine the application is using a library to access the DSP, and
the library has a dedicated thread listening for DSP events in a loop.
This happens to be how libomxil-ti and gst-dsp work.
Now, the thread received the last message, but has set a timeout of 10s,
or even worst, no timeout at all.
After realizing that was the last message, the main thread decides to
shut down, but it has to wait for the DSP thread to join. Unfortunately
the DSP thread is stuck waiting for events, and there's nothing that can
be done.
However, if we have a separate ioctl to interrupt that task, then the
main thread can issue that ioctl, and unlock the DSP thread without
having to wait 10s, or forever.
Does that make sense?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 17:03 ` Felipe Contreras
@ 2010-10-26 17:58 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 17:58 UTC (permalink / raw)
To: Felipe Contreras, felipe.contreras@gmail.com
Cc: gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> Sent: Tuesday, October 26, 2010 12:03 PM
> To: Guzman Lugo, Fernando; felipe.contreras@gmail.com
> Cc: gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> fernando.lugo@ti.com wrote:
> > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > <x0095840@ti.com> wrote:
> > > > So that avoid non-killable process.
> > >
> > > It would be useful to interrupt these tasks from user-space.
> > > A separate ioctl to do that would be needed.
> >
> > I don't see use case where that could be needed. It is only
> To avoid a
> > nonkillable task in the case the user pass an infinite Timeout.
> >
> > If you have some test case where that ioctl would be needed Please
> > share it in order to find the best solution.
>
> Well, imagine the application is using a library to access
> the DSP, and the library has a dedicated thread listening for
> DSP events in a loop.
> This happens to be how libomxil-ti and gst-dsp work.
>
> Now, the thread received the last message, but has set a
> timeout of 10s, or even worst, no timeout at all.
>
> After realizing that was the last message, the main thread
> decides to shut down, but it has to wait for the DSP thread
> to join. Unfortunately the DSP thread is stuck waiting for
> events, and there's nothing that can be done.
>
> However, if we have a separate ioctl to interrupt that task,
> then the main thread can issue that ioctl, and unlock the DSP
> thread without having to wait 10s, or forever.
>
> Does that make sense?
Maybe sending a signal to yourselft and having a dummy signal
Handle should work, it that would not like good. I am thinking
On having a ioctl to create and set an event the you could
Something like this:
struct dsp_notification events[3];
proc_register_notify(proc, event_type, &events[0]);
...
proc_register_notify(proc, event_type, &events[1]);
...
Sync_open_event(&events[2]);
second thread:
mgr_wait_for_bridge_events(proc, events, 3, index);
if (index == 2)
/* main thread force exit */
Main thread:
/* if some execption happened then finish the second thread */
sync_set_event(events[2]);
pthread_join(...);
However it is in progess a task for change replacing dspbridge sync.c
Module with event_fd to signal events to userspace. Where now simple
File descriptor will be used as event elements. So the mgr_wait_for_bridge_events
Will be implemented using "select" system call inside to wait for multiple events.
So you will be able to do something like this:
int events[3];
proc_register_notify(proc, event_type, &events[0]);
...
proc_register_notify(proc, event_type, &events[1]);
...
events[2] = eventfd(0, 0);
second thread:
mgr_wait_for_bridge_events(proc, events, 3, index);
if (index == 2)
/* main thread force exit */
Main thread:
/* if some execption happened then finish the second thread */
write(events[2], "s", 1);
pthread_join(...);
You won't need any aditional ioctl in order to do what you want to do.
So, I think it is not worth to make much changes to some module that will
Dissapear (my patch is just a fix it is not implementing something new),
It is just a matter of time to that task is finished and tested properly
And then send to LO.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 17:58 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 17:58 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at nokia.com]
> Sent: Tuesday, October 26, 2010 12:03 PM
> To: Guzman Lugo, Fernando; felipe.contreras at gmail.com
> Cc: gregkh at suse.de; hiroshi.doyu at nokia.com;
> linux-kernel at vger.kernel.org; andy.shevchenko at gmail.com;
> linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> fernando.lugo at ti.com wrote:
> > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > <x0095840@ti.com> wrote:
> > > > So that avoid non-killable process.
> > >
> > > It would be useful to interrupt these tasks from user-space.
> > > A separate ioctl to do that would be needed.
> >
> > I don't see use case where that could be needed. It is only
> To avoid a
> > nonkillable task in the case the user pass an infinite Timeout.
> >
> > If you have some test case where that ioctl would be needed Please
> > share it in order to find the best solution.
>
> Well, imagine the application is using a library to access
> the DSP, and the library has a dedicated thread listening for
> DSP events in a loop.
> This happens to be how libomxil-ti and gst-dsp work.
>
> Now, the thread received the last message, but has set a
> timeout of 10s, or even worst, no timeout at all.
>
> After realizing that was the last message, the main thread
> decides to shut down, but it has to wait for the DSP thread
> to join. Unfortunately the DSP thread is stuck waiting for
> events, and there's nothing that can be done.
>
> However, if we have a separate ioctl to interrupt that task,
> then the main thread can issue that ioctl, and unlock the DSP
> thread without having to wait 10s, or forever.
>
> Does that make sense?
Maybe sending a signal to yourselft and having a dummy signal
Handle should work, it that would not like good. I am thinking
On having a ioctl to create and set an event the you could
Something like this:
struct dsp_notification events[3];
proc_register_notify(proc, event_type, &events[0]);
...
proc_register_notify(proc, event_type, &events[1]);
...
Sync_open_event(&events[2]);
second thread:
mgr_wait_for_bridge_events(proc, events, 3, index);
if (index == 2)
/* main thread force exit */
Main thread:
/* if some execption happened then finish the second thread */
sync_set_event(events[2]);
pthread_join(...);
However it is in progess a task for change replacing dspbridge sync.c
Module with event_fd to signal events to userspace. Where now simple
File descriptor will be used as event elements. So the mgr_wait_for_bridge_events
Will be implemented using "select" system call inside to wait for multiple events.
So you will be able to do something like this:
int events[3];
proc_register_notify(proc, event_type, &events[0]);
...
proc_register_notify(proc, event_type, &events[1]);
...
events[2] = eventfd(0, 0);
second thread:
mgr_wait_for_bridge_events(proc, events, 3, index);
if (index == 2)
/* main thread force exit */
Main thread:
/* if some execption happened then finish the second thread */
write(events[2], "s", 1);
pthread_join(...);
You won't need any aditional ioctl in order to do what you want to do.
So, I think it is not worth to make much changes to some module that will
Dissapear (my patch is just a fix it is not implementing something new),
It is just a matter of time to that task is finished and tested properly
And then send to LO.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 17:58 ` Guzman Lugo, Fernando
@ 2010-10-26 19:27 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 19:27 UTC (permalink / raw)
To: fernando.lugo, felipe.contreras
Cc: gregkh, hiroshi.doyu, linux-kernel, andy.shevchenko, linux-omap,
linux-arm-kernel
fernando.lugo@ti.com wrote:
> > fernando.lugo@ti.com wrote:
> > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > <x0095840@ti.com> wrote:
> > > > > So that avoid non-killable process.
> > > >
> > > > It would be useful to interrupt these tasks from user-space.
> > > > A separate ioctl to do that would be needed.
> > >
> > > I don't see use case where that could be needed. It is only
> > To avoid a
> > > nonkillable task in the case the user pass an infinite Timeout.
> > >
> > > If you have some test case where that ioctl would be needed Please
> > > share it in order to find the best solution.
> >
> > Well, imagine the application is using a library to access
> > the DSP, and the library has a dedicated thread listening for
> > DSP events in a loop.
> > This happens to be how libomxil-ti and gst-dsp work.
> >
> > Now, the thread received the last message, but has set a
> > timeout of 10s, or even worst, no timeout at all.
> >
> > After realizing that was the last message, the main thread
> > decides to shut down, but it has to wait for the DSP thread
> > to join. Unfortunately the DSP thread is stuck waiting for
> > events, and there's nothing that can be done.
> >
> > However, if we have a separate ioctl to interrupt that task,
> > then the main thread can issue that ioctl, and unlock the DSP
> > thread without having to wait 10s, or forever.
> >
> > Does that make sense?
>
> Maybe sending a signal to yourselft and having a dummy signal
> Handle should work, it that would not like good.
Signals on libraries is a no-no.
> I am thinking On having a ioctl to create and set an event the you
> could Something like this:
>
> struct dsp_notification events[3];
>
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> Sync_open_event(&events[2]);
>
>
> second thread:
>
> mgr_wait_for_bridge_events(proc, events, 3, index);
>
> if (index == 2)
> /* main thread force exit */
>
>
> Main thread:
>
> /* if some execption happened then finish the second thread */
> sync_set_event(events[2]);
>
> pthread_join(...);
>
>
> However it is in progess a task for change replacing dspbridge sync.c
> Module with event_fd to signal events to userspace. Where now simple
> File descriptor will be used as event elements. So the mgr_wait_for_bridge_events
> Will be implemented using "select" system call inside to wait for multiple events.
> So you will be able to do something like this:
>
> int events[3];
>
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> events[2] = eventfd(0, 0);
>
>
> second thread:
>
> mgr_wait_for_bridge_events(proc, events, 3, index);
>
> if (index == 2)
> /* main thread force exit */
>
> Main thread:
>
> /* if some execption happened then finish the second thread */
> write(events[2], "s", 1);
>
> pthread_join(...);
>
> You won't need any aditional ioctl in order to do what you want to do.
>
> So, I think it is not worth to make much changes to some module that will
> Dissapear (my patch is just a fix it is not implementing something new),
> It is just a matter of time to that task is finished and tested properly
> And then send to LO.
All right, that makes sense. I just wanted to make suere you are aware
of that need.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 19:27 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 19:27 UTC (permalink / raw)
To: linux-arm-kernel
fernando.lugo at ti.com wrote:
> > fernando.lugo at ti.com wrote:
> > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > <x0095840@ti.com> wrote:
> > > > > So that avoid non-killable process.
> > > >
> > > > It would be useful to interrupt these tasks from user-space.
> > > > A separate ioctl to do that would be needed.
> > >
> > > I don't see use case where that could be needed. It is only
> > To avoid a
> > > nonkillable task in the case the user pass an infinite Timeout.
> > >
> > > If you have some test case where that ioctl would be needed Please
> > > share it in order to find the best solution.
> >
> > Well, imagine the application is using a library to access
> > the DSP, and the library has a dedicated thread listening for
> > DSP events in a loop.
> > This happens to be how libomxil-ti and gst-dsp work.
> >
> > Now, the thread received the last message, but has set a
> > timeout of 10s, or even worst, no timeout at all.
> >
> > After realizing that was the last message, the main thread
> > decides to shut down, but it has to wait for the DSP thread
> > to join. Unfortunately the DSP thread is stuck waiting for
> > events, and there's nothing that can be done.
> >
> > However, if we have a separate ioctl to interrupt that task,
> > then the main thread can issue that ioctl, and unlock the DSP
> > thread without having to wait 10s, or forever.
> >
> > Does that make sense?
>
> Maybe sending a signal to yourselft and having a dummy signal
> Handle should work, it that would not like good.
Signals on libraries is a no-no.
> I am thinking On having a ioctl to create and set an event the you
> could Something like this:
>
> struct dsp_notification events[3];
>
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> Sync_open_event(&events[2]);
>
>
> second thread:
>
> mgr_wait_for_bridge_events(proc, events, 3, index);
>
> if (index == 2)
> /* main thread force exit */
>
>
> Main thread:
>
> /* if some execption happened then finish the second thread */
> sync_set_event(events[2]);
>
> pthread_join(...);
>
>
> However it is in progess a task for change replacing dspbridge sync.c
> Module with event_fd to signal events to userspace. Where now simple
> File descriptor will be used as event elements. So the mgr_wait_for_bridge_events
> Will be implemented using "select" system call inside to wait for multiple events.
> So you will be able to do something like this:
>
> int events[3];
>
> proc_register_notify(proc, event_type, &events[0]);
> ...
> proc_register_notify(proc, event_type, &events[1]);
> ...
> events[2] = eventfd(0, 0);
>
>
> second thread:
>
> mgr_wait_for_bridge_events(proc, events, 3, index);
>
> if (index == 2)
> /* main thread force exit */
>
> Main thread:
>
> /* if some execption happened then finish the second thread */
> write(events[2], "s", 1);
>
> pthread_join(...);
>
> You won't need any aditional ioctl in order to do what you want to do.
>
> So, I think it is not worth to make much changes to some module that will
> Dissapear (my patch is just a fix it is not implementing something new),
> It is just a matter of time to that task is finished and tested properly
> And then send to LO.
All right, that makes sense. I just wanted to make suere you are aware
of that need.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 19:27 ` Felipe Contreras
@ 2010-10-26 20:01 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 20:01 UTC (permalink / raw)
To: Felipe Contreras, felipe.contreras@gmail.com
Cc: gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> Sent: Tuesday, October 26, 2010 2:27 PM
> To: Guzman Lugo, Fernando; felipe.contreras@gmail.com
> Cc: gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> fernando.lugo@ti.com wrote:
> > > fernando.lugo@ti.com wrote:
> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > > <x0095840@ti.com> wrote:
> > > > > > So that avoid non-killable process.
> > > > >
> > > > > It would be useful to interrupt these tasks from user-space.
> > > > > A separate ioctl to do that would be needed.
> > > >
> > > > I don't see use case where that could be needed. It is only
> > > To avoid a
> > > > nonkillable task in the case the user pass an infinite Timeout.
> > > >
> > > > If you have some test case where that ioctl would be
> needed Please
> > > > share it in order to find the best solution.
> > >
> > > Well, imagine the application is using a library to
> access the DSP,
> > > and the library has a dedicated thread listening for DSP
> events in a
> > > loop.
> > > This happens to be how libomxil-ti and gst-dsp work.
> > >
> > > Now, the thread received the last message, but has set a
> timeout of
> > > 10s, or even worst, no timeout at all.
> > >
> > > After realizing that was the last message, the main
> thread decides
> > > to shut down, but it has to wait for the DSP thread to join.
> > > Unfortunately the DSP thread is stuck waiting for events, and
> > > there's nothing that can be done.
> > >
> > > However, if we have a separate ioctl to interrupt that task, then
> > > the main thread can issue that ioctl, and unlock the DSP thread
> > > without having to wait 10s, or forever.
> > >
> > > Does that make sense?
> >
> > Maybe sending a signal to yourselft and having a dummy
> signal Handle
> > should work, it that would not like good.
>
> Signals on libraries is a no-no.
>
> > I am thinking On having a ioctl to create and set an event the you
> > could Something like this:
> >
> > struct dsp_notification events[3];
> >
> > proc_register_notify(proc, event_type, &events[0]); ...
> > proc_register_notify(proc, event_type, &events[1]); ...
> > Sync_open_event(&events[2]);
> >
> >
> > second thread:
> >
> > mgr_wait_for_bridge_events(proc, events, 3, index);
> >
> > if (index == 2)
> > /* main thread force exit */
> >
> >
> > Main thread:
> >
> > /* if some execption happened then finish the second thread */
> > sync_set_event(events[2]);
> >
> > pthread_join(...);
> >
> >
> > However it is in progess a task for change replacing
> dspbridge sync.c
> > Module with event_fd to signal events to userspace. Where
> now simple
> > File descriptor will be used as event elements. So the
> > mgr_wait_for_bridge_events Will be implemented using
> "select" system call inside to wait for multiple events.
> > So you will be able to do something like this:
> >
> > int events[3];
> >
> > proc_register_notify(proc, event_type, &events[0]); ...
> > proc_register_notify(proc, event_type, &events[1]); ...
> > events[2] = eventfd(0, 0);
> >
> >
> > second thread:
> >
> > mgr_wait_for_bridge_events(proc, events, 3, index);
> >
> > if (index == 2)
> > /* main thread force exit */
> >
> > Main thread:
> >
> > /* if some execption happened then finish the second thread */
> > write(events[2], "s", 1);
> >
> > pthread_join(...);
> >
> > You won't need any aditional ioctl in order to do what you
> want to do.
> >
> > So, I think it is not worth to make much changes to some
> module that
> > will Dissapear (my patch is just a fix it is not implementing
> > something new), It is just a matter of time to that task is
> finished
> > and tested properly And then send to LO.
>
> All right, that makes sense. I just wanted to make suere you
> are aware of that need.
Sure, that need will cover soon.
Thanks and regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-10-26 20:01 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 20:01 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at nokia.com]
> Sent: Tuesday, October 26, 2010 2:27 PM
> To: Guzman Lugo, Fernando; felipe.contreras at gmail.com
> Cc: gregkh at suse.de; hiroshi.doyu at nokia.com;
> linux-kernel at vger.kernel.org; andy.shevchenko at gmail.com;
> linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 8/8] staging: tidspbridge - make
> sync_wait_on_event interruptible
>
> fernando.lugo at ti.com wrote:
> > > fernando.lugo at ti.com wrote:
> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > > <x0095840@ti.com> wrote:
> > > > > > So that avoid non-killable process.
> > > > >
> > > > > It would be useful to interrupt these tasks from user-space.
> > > > > A separate ioctl to do that would be needed.
> > > >
> > > > I don't see use case where that could be needed. It is only
> > > To avoid a
> > > > nonkillable task in the case the user pass an infinite Timeout.
> > > >
> > > > If you have some test case where that ioctl would be
> needed Please
> > > > share it in order to find the best solution.
> > >
> > > Well, imagine the application is using a library to
> access the DSP,
> > > and the library has a dedicated thread listening for DSP
> events in a
> > > loop.
> > > This happens to be how libomxil-ti and gst-dsp work.
> > >
> > > Now, the thread received the last message, but has set a
> timeout of
> > > 10s, or even worst, no timeout at all.
> > >
> > > After realizing that was the last message, the main
> thread decides
> > > to shut down, but it has to wait for the DSP thread to join.
> > > Unfortunately the DSP thread is stuck waiting for events, and
> > > there's nothing that can be done.
> > >
> > > However, if we have a separate ioctl to interrupt that task, then
> > > the main thread can issue that ioctl, and unlock the DSP thread
> > > without having to wait 10s, or forever.
> > >
> > > Does that make sense?
> >
> > Maybe sending a signal to yourselft and having a dummy
> signal Handle
> > should work, it that would not like good.
>
> Signals on libraries is a no-no.
>
> > I am thinking On having a ioctl to create and set an event the you
> > could Something like this:
> >
> > struct dsp_notification events[3];
> >
> > proc_register_notify(proc, event_type, &events[0]); ...
> > proc_register_notify(proc, event_type, &events[1]); ...
> > Sync_open_event(&events[2]);
> >
> >
> > second thread:
> >
> > mgr_wait_for_bridge_events(proc, events, 3, index);
> >
> > if (index == 2)
> > /* main thread force exit */
> >
> >
> > Main thread:
> >
> > /* if some execption happened then finish the second thread */
> > sync_set_event(events[2]);
> >
> > pthread_join(...);
> >
> >
> > However it is in progess a task for change replacing
> dspbridge sync.c
> > Module with event_fd to signal events to userspace. Where
> now simple
> > File descriptor will be used as event elements. So the
> > mgr_wait_for_bridge_events Will be implemented using
> "select" system call inside to wait for multiple events.
> > So you will be able to do something like this:
> >
> > int events[3];
> >
> > proc_register_notify(proc, event_type, &events[0]); ...
> > proc_register_notify(proc, event_type, &events[1]); ...
> > events[2] = eventfd(0, 0);
> >
> >
> > second thread:
> >
> > mgr_wait_for_bridge_events(proc, events, 3, index);
> >
> > if (index == 2)
> > /* main thread force exit */
> >
> > Main thread:
> >
> > /* if some execption happened then finish the second thread */
> > write(events[2], "s", 1);
> >
> > pthread_join(...);
> >
> > You won't need any aditional ioctl in order to do what you
> want to do.
> >
> > So, I think it is not worth to make much changes to some
> module that
> > will Dissapear (my patch is just a fix it is not implementing
> > something new), It is just a matter of time to that task is
> finished
> > and tested properly And then send to LO.
>
> All right, that makes sense. I just wanted to make suere you
> are aware of that need.
Sure, that need will cover soon.
Thanks and regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
2010-10-26 0:58 ` Felipe Contreras
@ 2010-12-06 8:51 ` Ramirez Luna, Omar
-1 siblings, 0 replies; 70+ messages in thread
From: Ramirez Luna, Omar @ 2010-12-06 8:51 UTC (permalink / raw)
To: Guzman Lugo, Fernando
Cc: gregkh, felipe.contreras, Hiroshi.DOYU, linux-kernel,
andy.shevchenko, linux-omap, linux-arm-kernel, Felipe Contreras
On Mon, Oct 25, 2010 at 5:17 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> So that avoid nonkillable process.
>
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> ---
> .../staging/tidspbridge/include/dspbridge/sync.h | 13 +++++++++++--
> 1 files changed, 11 insertions(+), 2 deletions(-)
Pushed to dspbridge.
Regards,
Omar
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 8/8] staging: tidspbridge - make sync_wait_on_event interruptible
@ 2010-12-06 8:51 ` Ramirez Luna, Omar
0 siblings, 0 replies; 70+ messages in thread
From: Ramirez Luna, Omar @ 2010-12-06 8:51 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 25, 2010 at 5:17 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> So that avoid nonkillable process.
>
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> ---
> .../staging/tidspbridge/include/dspbridge/sync.h | 13 +++++++++++--
> 1 files changed, 11 insertions(+), 2 deletions(-)
Pushed to dspbridge.
Regards,
Omar
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 0:51 ` Fernando Guzman Lugo
@ 2010-10-26 11:46 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 11:46 UTC (permalink / raw)
To: Fernando Guzman Lugo
Cc: gregkh, felipe.contreras, Hiroshi.DOYU, linux-kernel,
andy.shevchenko, linux-omap, linux-arm-kernel
On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo <x0095840@ti.com> wrote:
> The device address is assigned by tidspbridge no need for that parameter anymore.
>
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
This would break the API with user-space, right?
I think this change should be delayed, preferably after we have a
working tidspbridge.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 11:46 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 11:46 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo <x0095840@ti.com> wrote:
> The device address is assigned by tidspbridge no need for that parameter anymore.
>
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
This would break the API with user-space, right?
I think this change should be delayed, preferably after we have a
working tidspbridge.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 11:46 ` Felipe Contreras
@ 2010-10-26 15:52 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 15:52 UTC (permalink / raw)
To: Felipe Contreras
Cc: gregkh@suse.de, felipe.contreras@nokia.com,
Hiroshi.DOYU@nokia.com, linux-kernel@vger.kernel.org,
andy.shevchenko@gmail.com, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Tuesday, October 26, 2010 6:46 AM
> To: Guzman Lugo, Fernando
> Cc: gregkh@suse.de; felipe.contreras@nokia.com;
> Hiroshi.DOYU@nokia.com; linux-kernel@vger.kernel.org;
> andy.shevchenko@gmail.com; linux-omap@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> <x0095840@ti.com> wrote:
> > The device address is assigned by tidspbridge no need for
> that parameter anymore.
> >
> > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>
> This would break the API with user-space, right?
Yes, user-space needs to be changed accordingly.
>
> I think this change should be delayed, preferably after we
> have a working tidspbridge.
The issue you were seeing must be fixed with patch 2/8, and
Having all the dependencies tidspbridge has to be working
Properly.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 15:52 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 15:52 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at gmail.com]
> Sent: Tuesday, October 26, 2010 6:46 AM
> To: Guzman Lugo, Fernando
> Cc: gregkh at suse.de; felipe.contreras at nokia.com;
> Hiroshi.DOYU at nokia.com; linux-kernel at vger.kernel.org;
> andy.shevchenko at gmail.com; linux-omap at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> <x0095840@ti.com> wrote:
> > The device address is assigned by tidspbridge no need for
> that parameter anymore.
> >
> > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>
> This would break the API with user-space, right?
Yes, user-space needs to be changed accordingly.
>
> I think this change should be delayed, preferably after we
> have a working tidspbridge.
The issue you were seeing must be fixed with patch 2/8, and
Having all the dependencies tidspbridge has to be working
Properly.
Regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 15:52 ` Guzman Lugo, Fernando
@ 2010-10-26 17:07 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 17:07 UTC (permalink / raw)
To: fernando.lugo, felipe.contreras
Cc: gregkh, hiroshi.doyu, linux-kernel, andy.shevchenko, linux-omap,
linux-arm-kernel
fernando.lugo@ti.com wrote:
> > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > <x0095840@ti.com> wrote:
> > > The device address is assigned by tidspbridge no need for
> > that parameter anymore.
> > >
> > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> >
> > This would break the API with user-space, right?
>
> Yes, user-space needs to be changed accordingly.
Wouldn't it make sense to avoid stuffing so many changes at once
including ABI breakage?
Does user-space really _needs_ to be changed? Can't you just ignore that
argument?
> > I think this change should be delayed, preferably after we
> > have a working tidspbridge.
>
> The issue you were seeing must be fixed with patch 2/8, and
> Having all the dependencies tidspbridge has to be working
> Properly.
Yes, which might not happen at .37. If we revert the iommu stuff, we
might have a chance at having a working bridge on .37.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 17:07 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 17:07 UTC (permalink / raw)
To: linux-arm-kernel
fernando.lugo at ti.com wrote:
> > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > <x0095840@ti.com> wrote:
> > > The device address is assigned by tidspbridge no need for
> > that parameter anymore.
> > >
> > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> >
> > This would break the API with user-space, right?
>
> Yes, user-space needs to be changed accordingly.
Wouldn't it make sense to avoid stuffing so many changes at once
including ABI breakage?
Does user-space really _needs_ to be changed? Can't you just ignore that
argument?
> > I think this change should be delayed, preferably after we
> > have a working tidspbridge.
>
> The issue you were seeing must be fixed with patch 2/8, and
> Having all the dependencies tidspbridge has to be working
> Properly.
Yes, which might not happen at .37. If we revert the iommu stuff, we
might have a chance at having a working bridge on .37.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 17:07 ` Felipe Contreras
@ 2010-10-26 18:08 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 18:08 UTC (permalink / raw)
To: Felipe Contreras, felipe.contreras@gmail.com
Cc: gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> Sent: Tuesday, October 26, 2010 12:08 PM
> To: Guzman Lugo, Fernando; felipe.contreras@gmail.com
> Cc: gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> fernando.lugo@ti.com wrote:
> > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > <x0095840@ti.com> wrote:
> > > > The device address is assigned by tidspbridge no need for
> > > that parameter anymore.
> > > >
> > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > >
> > > This would break the API with user-space, right?
> >
> > Yes, user-space needs to be changed accordingly.
>
> Wouldn't it make sense to avoid stuffing so many changes at
> once including ABI breakage?
>
> Does user-space really _needs_ to be changed? Can't you just
> ignore that argument?
Actually, I had a previous version of that patch where I only
Ignored that paramteter. But after thinking again and seeing
How the long time ago depreacted function are still there I
Removed the parameter in order to force apps to make the change.
You can ignore that argument at API level, so all users of the
API not need to have change (in that momment). That should be
Only few line change.
>
> > > I think this change should be delayed, preferably after we have a
> > > working tidspbridge.
> >
> > The issue you were seeing must be fixed with patch 2/8, and
> Having all
> > the dependencies tidspbridge has to be working Properly.
>
> Yes, which might not happen at .37. If we revert the iommu
> stuff, we might have a chance at having a working bridge on .37.
But havi that merged in tidspbridge make more pressure to merged
The dependencies, if not maybe that wont have event for .38. But
That is just my opinion if you all are agree revert is the best
Option I am ok, but I looks like more work reverting and rebaseing
specially because it is a big change.
Thanks and regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 18:08 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 18:08 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at nokia.com]
> Sent: Tuesday, October 26, 2010 12:08 PM
> To: Guzman Lugo, Fernando; felipe.contreras at gmail.com
> Cc: gregkh at suse.de; hiroshi.doyu at nokia.com;
> linux-kernel at vger.kernel.org; andy.shevchenko at gmail.com;
> linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> fernando.lugo at ti.com wrote:
> > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > <x0095840@ti.com> wrote:
> > > > The device address is assigned by tidspbridge no need for
> > > that parameter anymore.
> > > >
> > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > >
> > > This would break the API with user-space, right?
> >
> > Yes, user-space needs to be changed accordingly.
>
> Wouldn't it make sense to avoid stuffing so many changes at
> once including ABI breakage?
>
> Does user-space really _needs_ to be changed? Can't you just
> ignore that argument?
Actually, I had a previous version of that patch where I only
Ignored that paramteter. But after thinking again and seeing
How the long time ago depreacted function are still there I
Removed the parameter in order to force apps to make the change.
You can ignore that argument at API level, so all users of the
API not need to have change (in that momment). That should be
Only few line change.
>
> > > I think this change should be delayed, preferably after we have a
> > > working tidspbridge.
> >
> > The issue you were seeing must be fixed with patch 2/8, and
> Having all
> > the dependencies tidspbridge has to be working Properly.
>
> Yes, which might not happen at .37. If we revert the iommu
> stuff, we might have a chance at having a working bridge on .37.
But havi that merged in tidspbridge make more pressure to merged
The dependencies, if not maybe that wont have event for .38. But
That is just my opinion if you all are agree revert is the best
Option I am ok, but I looks like more work reverting and rebaseing
specially because it is a big change.
Thanks and regards,
Fernando.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 18:08 ` Guzman Lugo, Fernando
@ 2010-10-26 19:37 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 19:37 UTC (permalink / raw)
To: fernando.lugo, felipe.contreras
Cc: gregkh, hiroshi.doyu, linux-kernel, andy.shevchenko, linux-omap,
linux-arm-kernel
fernando.lugo@ti.com wrote:
> > fernando.lugo@ti.com wrote:
> > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > <x0095840@ti.com> wrote:
> > > > > The device address is assigned by tidspbridge no need for
> > > > that parameter anymore.
> > > > >
> > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > > >
> > > > This would break the API with user-space, right?
> > >
> > > Yes, user-space needs to be changed accordingly.
> >
> > Wouldn't it make sense to avoid stuffing so many changes at
> > once including ABI breakage?
> >
> > Does user-space really _needs_ to be changed? Can't you just
> > ignore that argument?
>
> Actually, I had a previous version of that patch where I only
> Ignored that paramteter. But after thinking again and seeing
> How the long time ago depreacted function are still there I
> Removed the parameter in order to force apps to make the change.
Again, can we concentrate on first getting this thing to work?
We can think on breaking things again later.
> You can ignore that argument at API level, so all users of the
> API not need to have change (in that momment). That should be
> Only few line change.
Yes, that's what I'm proposing.
> > > > I think this change should be delayed, preferably after we have a
> > > > working tidspbridge.
> > >
> > > The issue you were seeing must be fixed with patch 2/8, and
> > Having all
> > > the dependencies tidspbridge has to be working Properly.
> >
> > Yes, which might not happen at .37. If we revert the iommu
> > stuff, we might have a chance at having a working bridge on .37.
>
> But havi that merged in tidspbridge make more pressure to merged
> The dependencies, if not maybe that wont have event for .38.
Those patches have been there for a while and there's no sign of when
they'll be merged. Also, remember that the fact that the patches have
been sent doesn't guarantee they would be on this, or even the next
cycle.
Besides, the rule followed here is: it's ready when it's ready. In the
meantime the driver should work, and we can do that rather easily.
> But That is just my opinion if you all are agree revert is the best
> Option I am ok, but I looks like more work reverting and rebaseing
> specially because it is a big change.
git makes this easy.
The hard part is for the people out there that are trying to use this
driver and have to hunt mailing lists and multiple repos just to get
this to work. Moving to staging was supposed to make this easier, not
harder.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 19:37 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-26 19:37 UTC (permalink / raw)
To: linux-arm-kernel
fernando.lugo at ti.com wrote:
> > fernando.lugo at ti.com wrote:
> > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > <x0095840@ti.com> wrote:
> > > > > The device address is assigned by tidspbridge no need for
> > > > that parameter anymore.
> > > > >
> > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > > >
> > > > This would break the API with user-space, right?
> > >
> > > Yes, user-space needs to be changed accordingly.
> >
> > Wouldn't it make sense to avoid stuffing so many changes at
> > once including ABI breakage?
> >
> > Does user-space really _needs_ to be changed? Can't you just
> > ignore that argument?
>
> Actually, I had a previous version of that patch where I only
> Ignored that paramteter. But after thinking again and seeing
> How the long time ago depreacted function are still there I
> Removed the parameter in order to force apps to make the change.
Again, can we concentrate on first getting this thing to work?
We can think on breaking things again later.
> You can ignore that argument at API level, so all users of the
> API not need to have change (in that momment). That should be
> Only few line change.
Yes, that's what I'm proposing.
> > > > I think this change should be delayed, preferably after we have a
> > > > working tidspbridge.
> > >
> > > The issue you were seeing must be fixed with patch 2/8, and
> > Having all
> > > the dependencies tidspbridge has to be working Properly.
> >
> > Yes, which might not happen at .37. If we revert the iommu
> > stuff, we might have a chance at having a working bridge on .37.
>
> But havi that merged in tidspbridge make more pressure to merged
> The dependencies, if not maybe that wont have event for .38.
Those patches have been there for a while and there's no sign of when
they'll be merged. Also, remember that the fact that the patches have
been sent doesn't guarantee they would be on this, or even the next
cycle.
Besides, the rule followed here is: it's ready when it's ready. In the
meantime the driver should work, and we can do that rather easily.
> But That is just my opinion if you all are agree revert is the best
> Option I am ok, but I looks like more work reverting and rebaseing
> specially because it is a big change.
git makes this easy.
The hard part is for the people out there that are trying to use this
driver and have to hunt mailing lists and multiple repos just to get
this to work. Moving to staging was supposed to make this easier, not
harder.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread
* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 19:37 ` Felipe Contreras
@ 2010-10-26 20:39 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 20:39 UTC (permalink / raw)
To: Felipe Contreras, felipe.contreras@gmail.com
Cc: gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@nokia.com]
> Sent: Tuesday, October 26, 2010 2:38 PM
> To: Guzman Lugo, Fernando; felipe.contreras@gmail.com
> Cc: gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> fernando.lugo@ti.com wrote:
> > > fernando.lugo@ti.com wrote:
> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > > <x0095840@ti.com> wrote:
> > > > > > The device address is assigned by tidspbridge no need for
> > > > > that parameter anymore.
> > > > > >
> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > > > >
> > > > > This would break the API with user-space, right?
> > > >
> > > > Yes, user-space needs to be changed accordingly.
> > >
> > > Wouldn't it make sense to avoid stuffing so many changes at once
> > > including ABI breakage?
> > >
> > > Does user-space really _needs_ to be changed? Can't you
> just ignore
> > > that argument?
> >
> > Actually, I had a previous version of that patch where I
> only Ignored
> > that paramteter. But after thinking again and seeing How
> the long time
> > ago depreacted function are still there I Removed the parameter in
> > order to force apps to make the change.
>
> Again, can we concentrate on first getting this thing to work?
If to make it work for 37 the iommu migration patch will be revert
These set of patches will have to wait until the patches are merged
Again. So the dspbridge would be "fix" first a then the patches would
Be merged.
>
> We can think on breaking things again later.
>
> > You can ignore that argument at API level, so all users of
> the API not
> > need to have change (in that momment). That should be Only few line
> > change.
>
> Yes, that's what I'm proposing.
I meant userspace api or library level:
Example:
Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) {
...
struct proc_map args = {
.map_addr = mpu_addr;
/* ignore req_addr */
.dsp_addr = dsp_addr;
.attr = attr;
}
ret = ioctl(handle, PROCMAP_CMD, args)
...
}
>
> > > > > I think this change should be delayed, preferably
> after we have
> > > > > a working tidspbridge.
> > > >
> > > > The issue you were seeing must be fixed with patch 2/8, and
> > > Having all
> > > > the dependencies tidspbridge has to be working Properly.
> > >
> > > Yes, which might not happen at .37. If we revert the
> iommu stuff, we
> > > might have a chance at having a working bridge on .37.
> >
> > But havi that merged in tidspbridge make more pressure to
> merged The
> > dependencies, if not maybe that wont have event for .38.
>
> Those patches have been there for a while and there's no sign
> of when they'll be merged. Also, remember that the fact that
> the patches have been sent doesn't guarantee they would be on
> this, or even the next cycle.
>
> Besides, the rule followed here is: it's ready when it's
> ready. In the meantime the driver should work, and we can do
> that rather easily.
I don't have much experience in this kind of situacion if it is
believed it is the best solution then ok for me.
>
> > But That is just my opinion if you all are agree revert is the best
> > Option I am ok, but I looks like more work reverting and rebaseing
> > specially because it is a big change.
>
> git makes this easy.
Till we need to fix all the conflics, make sure everything work
Properly and then send it again to review. That can take a while,
But it seems someone else is agree to revert the changes, so maybe
That is the best to do in this situacion.
Thanks and regards,
Fernando.
>
> The hard part is for the people out there that are trying to
> use this driver and have to hunt mailing lists and multiple
> repos just to get this to work. Moving to staging was
> supposed to make this easier, not harder.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-26 20:39 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-26 20:39 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at nokia.com]
> Sent: Tuesday, October 26, 2010 2:38 PM
> To: Guzman Lugo, Fernando; felipe.contreras at gmail.com
> Cc: gregkh at suse.de; hiroshi.doyu at nokia.com;
> linux-kernel at vger.kernel.org; andy.shevchenko at gmail.com;
> linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> fernando.lugo at ti.com wrote:
> > > fernando.lugo at ti.com wrote:
> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> > > > > <x0095840@ti.com> wrote:
> > > > > > The device address is assigned by tidspbridge no need for
> > > > > that parameter anymore.
> > > > > >
> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > > > >
> > > > > This would break the API with user-space, right?
> > > >
> > > > Yes, user-space needs to be changed accordingly.
> > >
> > > Wouldn't it make sense to avoid stuffing so many changes at once
> > > including ABI breakage?
> > >
> > > Does user-space really _needs_ to be changed? Can't you
> just ignore
> > > that argument?
> >
> > Actually, I had a previous version of that patch where I
> only Ignored
> > that paramteter. But after thinking again and seeing How
> the long time
> > ago depreacted function are still there I Removed the parameter in
> > order to force apps to make the change.
>
> Again, can we concentrate on first getting this thing to work?
If to make it work for 37 the iommu migration patch will be revert
These set of patches will have to wait until the patches are merged
Again. So the dspbridge would be "fix" first a then the patches would
Be merged.
>
> We can think on breaking things again later.
>
> > You can ignore that argument at API level, so all users of
> the API not
> > need to have change (in that momment). That should be Only few line
> > change.
>
> Yes, that's what I'm proposing.
I meant userspace api or library level:
Example:
Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) {
...
struct proc_map args = {
.map_addr = mpu_addr;
/* ignore req_addr */
.dsp_addr = dsp_addr;
.attr = attr;
}
ret = ioctl(handle, PROCMAP_CMD, args)
...
}
>
> > > > > I think this change should be delayed, preferably
> after we have
> > > > > a working tidspbridge.
> > > >
> > > > The issue you were seeing must be fixed with patch 2/8, and
> > > Having all
> > > > the dependencies tidspbridge has to be working Properly.
> > >
> > > Yes, which might not happen at .37. If we revert the
> iommu stuff, we
> > > might have a chance at having a working bridge on .37.
> >
> > But havi that merged in tidspbridge make more pressure to
> merged The
> > dependencies, if not maybe that wont have event for .38.
>
> Those patches have been there for a while and there's no sign
> of when they'll be merged. Also, remember that the fact that
> the patches have been sent doesn't guarantee they would be on
> this, or even the next cycle.
>
> Besides, the rule followed here is: it's ready when it's
> ready. In the meantime the driver should work, and we can do
> that rather easily.
I don't have much experience in this kind of situacion if it is
believed it is the best solution then ok for me.
>
> > But That is just my opinion if you all are agree revert is the best
> > Option I am ok, but I looks like more work reverting and rebaseing
> > specially because it is a big change.
>
> git makes this easy.
Till we need to fix all the conflics, make sure everything work
Properly and then send it again to review. That can take a while,
But it seems someone else is agree to revert the changes, so maybe
That is the best to do in this situacion.
Thanks and regards,
Fernando.
>
> The hard part is for the people out there that are trying to
> use this driver and have to hunt mailing lists and multiple
> repos just to get this to work. Moving to staging was
> supposed to make this easier, not harder.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-26 20:39 ` Guzman Lugo, Fernando
(?)
@ 2010-10-27 8:19 ` Felipe Contreras
-1 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-27 8:19 UTC (permalink / raw)
To: Guzman Lugo, Fernando
Cc: Felipe Contreras, gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
>> fernando.lugo@ti.com wrote:
>> > > fernando.lugo@ti.com wrote:
>> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
>> > > > > <x0095840@ti.com> wrote:
>> > > > > > The device address is assigned by tidspbridge no need for
>> > > > > that parameter anymore.
>> > > > > >
>> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>> > > > >
>> > > > > This would break the API with user-space, right?
>> > > >
>> > > > Yes, user-space needs to be changed accordingly.
>> > >
>> > > Wouldn't it make sense to avoid stuffing so many changes at once
>> > > including ABI breakage?
>> > >
>> > > Does user-space really _needs_ to be changed? Can't you
>> just ignore
>> > > that argument?
>> >
>> > Actually, I had a previous version of that patch where I
>> only Ignored
>> > that paramteter. But after thinking again and seeing How
>> the long time
>> > ago depreacted function are still there I Removed the parameter in
>> > order to force apps to make the change.
>>
>> Again, can we concentrate on first getting this thing to work?
>
> If to make it work for 37 the iommu migration patch will be revert
> These set of patches will have to wait until the patches are merged
> Again. So the dspbridge would be "fix" first a then the patches would
> Be merged.
I couldn't parse this correctly, but yes, I think.
>> We can think on breaking things again later.
>>
>> > You can ignore that argument at API level, so all users of
>> the API not
>> > need to have change (in that momment). That should be Only few line
>> > change.
>>
>> Yes, that's what I'm proposing.
>
> I meant userspace api or library level:
> Example:
>
> Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) {
> ...
> struct proc_map args = {
> .map_addr = mpu_addr;
> /* ignore req_addr */
> .dsp_addr = dsp_addr;
> .attr = attr;
> }
>
> ret = ioctl(handle, PROCMAP_CMD, args)
> ...
> }
Yes, but this has to be done on every library/app that is using the dsp.
It's much easier to do it on kernel side.
Besides, at least on gst-dsp we want it to work for _all_ bridge
versions, so it would be:
@@ -829,7 +829,9 @@ struct map_mem {
void *proc_handle;
void *mpu_addr;
unsigned long size;
+#if DSP_API >= 3
void *req_addr;
+#endif
void **ret_map_addr;
unsigned long attr;
};
@@ -838,7 +840,9 @@ bool dsp_map(int handle,
void *proc_handle,
void *mpu_addr,
unsigned long size,
void *req_addr,
void *ret_map_addr,
unsigned long attr)
{
@@ -846,7 +850,9 @@ bool dsp_map(int handle,
.proc_handle = proc_handle,
.mpu_addr = mpu_addr,
.size = size,
+#if DSP_API >= 3
.req_addr = req_addr,
+#endif
.ret_map_addr = ret_map_addr,
.attr = attr,
};
But then if we are breaking the API already, wouldn't it make sense to
use the DMA direction too? (probably on the 'attr' field)
That's why it's a good idea to discuss API breakage, and not do it
willi-nillii. Otherwise we would have #if DSP_API >= 4, and so on.
But it's a bit pointless to be discussing about this at this point
while people are still struggling to get this working and don't know
which branch to use.
Look, you sent patches that broke the bridge (even after manually
applying all the dependency patches), since Omar wasn't there as
gatekeeper and Greg immediately merge them we now have a bridge that
is further broken. And now you want to break it even more arguing that
you want to force people to use the new API, but how do you expect
people to migrate to this new API if they can't even run this?
Besides, if you want to break API you should at least mention that
clearly in the patch.
>> > > > > I think this change should be delayed, preferably
>> after we have
>> > > > > a working tidspbridge.
>> > > >
>> > > > The issue you were seeing must be fixed with patch 2/8, and
>> > > Having all
>> > > > the dependencies tidspbridge has to be working Properly.
>> > >
>> > > Yes, which might not happen at .37. If we revert the
>> iommu stuff, we
>> > > might have a chance at having a working bridge on .37.
>> >
>> > But havi that merged in tidspbridge make more pressure to
>> merged The
>> > dependencies, if not maybe that wont have event for .38.
>>
>> Those patches have been there for a while and there's no sign
>> of when they'll be merged. Also, remember that the fact that
>> the patches have been sent doesn't guarantee they would be on
>> this, or even the next cycle.
>>
>> Besides, the rule followed here is: it's ready when it's
>> ready. In the meantime the driver should work, and we can do
>> that rather easily.
>
> I don't have much experience in this kind of situacion if it is
> believed it is the best solution then ok for me.
>
>> > But That is just my opinion if you all are agree revert is the best
>> > Option I am ok, but I looks like more work reverting and rebaseing
>> > specially because it is a big change.
>>
>> git makes this easy.
>
> Till we need to fix all the conflics, make sure everything work
> Properly and then send it again to review. That can take a while,
> But it seems someone else is agree to revert the changes, so maybe
> That is the best to do in this situation.
Reverting or not reverting is not the question here.
The question is how do we get it working for .37? Are you completely
certain that all the dependent patches will be merged? Has Hiroshi
said it would happen? Has the ARM people? Are you familiar with the
release process enough to say so? I'm not so familiar, but my feeling
is that it's already late, and even if we revert the iommu changes we
might not be able to get this working on .37 either.
Even if we leave iommu out of the picture, there's still the ioremap()
issue that will appear when linux-arm is pulled, and then we need to
deal with memblock changes, which is still not clear what will happen.
And then we need to fix the breakage in the compilation due to the
layer violation with "control.h". I think that already too much
uncertainty.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 70+ messages in thread* Re: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-27 8:19 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-27 8:19 UTC (permalink / raw)
To: Guzman Lugo, Fernando
Cc: Felipe Contreras, gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
>> fernando.lugo@ti.com wrote:
>> > > fernando.lugo@ti.com wrote:
>> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
>> > > > > <x0095840@ti.com> wrote:
>> > > > > > The device address is assigned by tidspbridge no need for
>> > > > > that parameter anymore.
>> > > > > >
>> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>> > > > >
>> > > > > This would break the API with user-space, right?
>> > > >
>> > > > Yes, user-space needs to be changed accordingly.
>> > >
>> > > Wouldn't it make sense to avoid stuffing so many changes at once
>> > > including ABI breakage?
>> > >
>> > > Does user-space really _needs_ to be changed? Can't you
>> just ignore
>> > > that argument?
>> >
>> > Actually, I had a previous version of that patch where I
>> only Ignored
>> > that paramteter. But after thinking again and seeing How
>> the long time
>> > ago depreacted function are still there I Removed the parameter in
>> > order to force apps to make the change.
>>
>> Again, can we concentrate on first getting this thing to work?
>
> If to make it work for 37 the iommu migration patch will be revert
> These set of patches will have to wait until the patches are merged
> Again. So the dspbridge would be "fix" first a then the patches would
> Be merged.
I couldn't parse this correctly, but yes, I think.
>> We can think on breaking things again later.
>>
>> > You can ignore that argument at API level, so all users of
>> the API not
>> > need to have change (in that momment). That should be Only few line
>> > change.
>>
>> Yes, that's what I'm proposing.
>
> I meant userspace api or library level:
> Example:
>
> Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) {
> ...
> struct proc_map args = {
> .map_addr = mpu_addr;
> /* ignore req_addr */
> .dsp_addr = dsp_addr;
> .attr = attr;
> }
>
> ret = ioctl(handle, PROCMAP_CMD, args)
> ...
> }
Yes, but this has to be done on every library/app that is using the dsp.
It's much easier to do it on kernel side.
Besides, at least on gst-dsp we want it to work for _all_ bridge
versions, so it would be:
@@ -829,7 +829,9 @@ struct map_mem {
void *proc_handle;
void *mpu_addr;
unsigned long size;
+#if DSP_API >= 3
void *req_addr;
+#endif
void **ret_map_addr;
unsigned long attr;
};
@@ -838,7 +840,9 @@ bool dsp_map(int handle,
void *proc_handle,
void *mpu_addr,
unsigned long size,
void *req_addr,
void *ret_map_addr,
unsigned long attr)
{
@@ -846,7 +850,9 @@ bool dsp_map(int handle,
.proc_handle = proc_handle,
.mpu_addr = mpu_addr,
.size = size,
+#if DSP_API >= 3
.req_addr = req_addr,
+#endif
.ret_map_addr = ret_map_addr,
.attr = attr,
};
But then if we are breaking the API already, wouldn't it make sense to
use the DMA direction too? (probably on the 'attr' field)
That's why it's a good idea to discuss API breakage, and not do it
willi-nillii. Otherwise we would have #if DSP_API >= 4, and so on.
But it's a bit pointless to be discussing about this at this point
while people are still struggling to get this working and don't know
which branch to use.
Look, you sent patches that broke the bridge (even after manually
applying all the dependency patches), since Omar wasn't there as
gatekeeper and Greg immediately merge them we now have a bridge that
is further broken. And now you want to break it even more arguing that
you want to force people to use the new API, but how do you expect
people to migrate to this new API if they can't even run this?
Besides, if you want to break API you should at least mention that
clearly in the patch.
>> > > > > I think this change should be delayed, preferably
>> after we have
>> > > > > a working tidspbridge.
>> > > >
>> > > > The issue you were seeing must be fixed with patch 2/8, and
>> > > Having all
>> > > > the dependencies tidspbridge has to be working Properly.
>> > >
>> > > Yes, which might not happen at .37. If we revert the
>> iommu stuff, we
>> > > might have a chance at having a working bridge on .37.
>> >
>> > But havi that merged in tidspbridge make more pressure to
>> merged The
>> > dependencies, if not maybe that wont have event for .38.
>>
>> Those patches have been there for a while and there's no sign
>> of when they'll be merged. Also, remember that the fact that
>> the patches have been sent doesn't guarantee they would be on
>> this, or even the next cycle.
>>
>> Besides, the rule followed here is: it's ready when it's
>> ready. In the meantime the driver should work, and we can do
>> that rather easily.
>
> I don't have much experience in this kind of situacion if it is
> believed it is the best solution then ok for me.
>
>> > But That is just my opinion if you all are agree revert is the best
>> > Option I am ok, but I looks like more work reverting and rebaseing
>> > specially because it is a big change.
>>
>> git makes this easy.
>
> Till we need to fix all the conflics, make sure everything work
> Properly and then send it again to review. That can take a while,
> But it seems someone else is agree to revert the changes, so maybe
> That is the best to do in this situation.
Reverting or not reverting is not the question here.
The question is how do we get it working for .37? Are you completely
certain that all the dependent patches will be merged? Has Hiroshi
said it would happen? Has the ARM people? Are you familiar with the
release process enough to say so? I'm not so familiar, but my feeling
is that it's already late, and even if we revert the iommu changes we
might not be able to get this working on .37 either.
Even if we leave iommu out of the picture, there's still the ioremap()
issue that will appear when linux-arm is pulled, and then we need to
deal with memblock changes, which is still not clear what will happen.
And then we need to fix the breakage in the compilation due to the
layer violation with "control.h". I think that already too much
uncertainty.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-27 8:19 ` Felipe Contreras
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Contreras @ 2010-10-27 8:19 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
>> fernando.lugo at ti.com wrote:
>> > > fernando.lugo at ti.com wrote:
>> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
>> > > > > <x0095840@ti.com> wrote:
>> > > > > > The device address is assigned by tidspbridge no need for
>> > > > > that parameter anymore.
>> > > > > >
>> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
>> > > > >
>> > > > > This would break the API with user-space, right?
>> > > >
>> > > > Yes, user-space needs to be changed accordingly.
>> > >
>> > > Wouldn't it make sense to avoid stuffing so many changes at once
>> > > including ABI breakage?
>> > >
>> > > Does user-space really _needs_ to be changed? Can't you
>> just ignore
>> > > that argument?
>> >
>> > Actually, I had a previous version of that patch where I
>> only Ignored
>> > that paramteter. But after thinking again and seeing How
>> the long time
>> > ago depreacted function are still there I Removed the parameter in
>> > order to force apps to make the change.
>>
>> Again, can we concentrate on first getting this thing to work?
>
> If to make it work for 37 the iommu migration patch will be revert
> These set of patches will have to wait until the patches are merged
> Again. So the dspbridge would be "fix" first a then the patches would
> Be merged.
I couldn't parse this correctly, but yes, I think.
>> We can think on breaking things again later.
>>
>> > You can ignore that argument at API level, so all users of
>> the API not
>> > need to have change (in that momment). That should be Only few line
>> > change.
>>
>> Yes, that's what I'm proposing.
>
> I meant userspace api or library level:
> Example:
>
> Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) {
> ...
> ? ? ? ?struct proc_map args = {
> ? ? ? ? ? ? ? ?.map_addr = mpu_addr;
> ? ? ? ? ? ? ? ?/* ignore req_addr */
> ? ? ? ? ? ? ? ?.dsp_addr = dsp_addr;
> ? ? ? ? ? ? ? ?.attr = attr;
> ? ? ? ?}
>
> ? ? ? ?ret = ioctl(handle, PROCMAP_CMD, args)
> ...
> }
Yes, but this has to be done on every library/app that is using the dsp.
It's much easier to do it on kernel side.
Besides, at least on gst-dsp we want it to work for _all_ bridge
versions, so it would be:
@@ -829,7 +829,9 @@ struct map_mem {
void *proc_handle;
void *mpu_addr;
unsigned long size;
+#if DSP_API >= 3
void *req_addr;
+#endif
void **ret_map_addr;
unsigned long attr;
};
@@ -838,7 +840,9 @@ bool dsp_map(int handle,
void *proc_handle,
void *mpu_addr,
unsigned long size,
void *req_addr,
void *ret_map_addr,
unsigned long attr)
{
@@ -846,7 +850,9 @@ bool dsp_map(int handle,
.proc_handle = proc_handle,
.mpu_addr = mpu_addr,
.size = size,
+#if DSP_API >= 3
.req_addr = req_addr,
+#endif
.ret_map_addr = ret_map_addr,
.attr = attr,
};
But then if we are breaking the API already, wouldn't it make sense to
use the DMA direction too? (probably on the 'attr' field)
That's why it's a good idea to discuss API breakage, and not do it
willi-nillii. Otherwise we would have #if DSP_API >= 4, and so on.
But it's a bit pointless to be discussing about this at this point
while people are still struggling to get this working and don't know
which branch to use.
Look, you sent patches that broke the bridge (even after manually
applying all the dependency patches), since Omar wasn't there as
gatekeeper and Greg immediately merge them we now have a bridge that
is further broken. And now you want to break it even more arguing that
you want to force people to use the new API, but how do you expect
people to migrate to this new API if they can't even run this?
Besides, if you want to break API you should at least mention that
clearly in the patch.
>> > > > > I think this change should be delayed, preferably
>> after we have
>> > > > > a working tidspbridge.
>> > > >
>> > > > The issue you were seeing must be fixed with patch 2/8, and
>> > > Having all
>> > > > the dependencies tidspbridge has to be working Properly.
>> > >
>> > > Yes, which might not happen at .37. If we revert the
>> iommu stuff, we
>> > > might have a chance at having a working bridge on .37.
>> >
>> > But havi that merged in tidspbridge make more pressure to
>> merged The
>> > dependencies, if not maybe that wont have event for .38.
>>
>> Those patches have been there for a while and there's no sign
>> of when they'll be merged. Also, remember that the fact that
>> the patches have been sent doesn't guarantee they would be on
>> this, or even the next cycle.
>>
>> Besides, the rule followed here is: it's ready when it's
>> ready. In the meantime the driver should work, and we can do
>> that rather easily.
>
> I don't have much experience in this kind of situacion if it is
> believed it is the best solution then ok for me.
>
>> > But That is just my opinion if you all are agree revert is the best
>> > Option I am ok, but I looks like more work reverting and rebaseing
>> > specially because it is a big change.
>>
>> git makes this easy.
>
> Till we need to fix all the conflics, make sure everything work
> Properly and then send it again to review. That can take a while,
> But it seems someone else is agree to revert the changes, so maybe
> That is the best to do in this situation.
Reverting or not reverting is not the question here.
The question is how do we get it working for .37? Are you completely
certain that all the dependent patches will be merged? Has Hiroshi
said it would happen? Has the ARM people? Are you familiar with the
release process enough to say so? I'm not so familiar, but my feeling
is that it's already late, and even if we revert the iommu changes we
might not be able to get this working on .37 either.
Even if we leave iommu out of the picture, there's still the ioremap()
issue that will appear when linux-arm is pulled, and then we need to
deal with memblock changes, which is still not clear what will happen.
And then we need to fix the breakage in the compilation due to the
layer violation with "control.h". I think that already too much
uncertainty.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 70+ messages in thread* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-27 8:19 ` Felipe Contreras
(?)
@ 2010-10-28 15:38 ` Guzman Lugo, Fernando
-1 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-28 15:38 UTC (permalink / raw)
To: Felipe Contreras
Cc: Felipe Contreras, gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Wednesday, October 27, 2010 3:20 AM
> To: Guzman Lugo, Fernando
> Cc: Felipe Contreras; gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
> >> fernando.lugo@ti.com wrote:
> >> > > fernando.lugo@ti.com wrote:
> >> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> >> > > > > <x0095840@ti.com> wrote:
> >> > > > > > The device address is assigned by tidspbridge no need for
> >> > > > > that parameter anymore.
> >> > > > > >
> >> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> >> > > > >
> >> > > > > This would break the API with user-space, right?
> >> > > >
> >> > > > Yes, user-space needs to be changed accordingly.
> >> > >
> >> > > Wouldn't it make sense to avoid stuffing so many
> changes at once
> >> > > including ABI breakage?
> >> > >
> >> > > Does user-space really _needs_ to be changed? Can't you
> >> just ignore
> >> > > that argument?
> >> >
> >> > Actually, I had a previous version of that patch where I
> >> only Ignored
> >> > that paramteter. But after thinking again and seeing How
> >> the long time
> >> > ago depreacted function are still there I Removed the
> parameter in
> >> > order to force apps to make the change.
> >>
> >> Again, can we concentrate on first getting this thing to work?
> >
> > If to make it work for 37 the iommu migration patch will be revert
> > These set of patches will have to wait until the patches are merged
> > Again. So the dspbridge would be "fix" first a then the
> patches would
> > Be merged.
>
> I couldn't parse this correctly, but yes, I think.
>
> >> We can think on breaking things again later.
> >>
> >> > You can ignore that argument at API level, so all users of
> >> the API not
> >> > need to have change (in that momment). That should be
> Only few line
> >> > change.
> >>
> >> Yes, that's what I'm proposing.
> >
> > I meant userspace api or library level:
> > Example:
> >
> > Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) { ...
> > struct proc_map args = {
> > .map_addr = mpu_addr;
> > /* ignore req_addr */
> > .dsp_addr = dsp_addr;
> > .attr = attr;
> > }
> >
> > ret = ioctl(handle, PROCMAP_CMD, args) ...
> > }
>
> Yes, but this has to be done on every library/app that is
> using the dsp.
> It's much easier to do it on kernel side.
>
> Besides, at least on gst-dsp we want it to work for _all_
> bridge versions, so it would be:
>
> @@ -829,7 +829,9 @@ struct map_mem {
> void *proc_handle;
> void *mpu_addr;
> unsigned long size;
> +#if DSP_API >= 3
> void *req_addr;
> +#endif
> void **ret_map_addr;
> unsigned long attr;
> };
> @@ -838,7 +840,9 @@ bool dsp_map(int handle,
> void *proc_handle,
> void *mpu_addr,
> unsigned long size,
> void *req_addr,
> void *ret_map_addr,
> unsigned long attr)
> {
> @@ -846,7 +850,9 @@ bool dsp_map(int handle,
> .proc_handle = proc_handle,
> .mpu_addr = mpu_addr,
> .size = size,
> +#if DSP_API >= 3
> .req_addr = req_addr,
> +#endif
> .ret_map_addr = ret_map_addr,
> .attr = attr,
> };
>
> But then if we are breaking the API already, wouldn't it make
> sense to use the DMA direction too? (probably on the 'attr' field)
>
> That's why it's a good idea to discuss API breakage, and not
> do it willi-nillii. Otherwise we would have #if DSP_API >= 4,
> and so on.
>
> But it's a bit pointless to be discussing about this at this
> point while people are still struggling to get this working
> and don't know which branch to use.
>
> Look, you sent patches that broke the bridge (even after
> manually applying all the dependency patches), since Omar
> wasn't there as gatekeeper and Greg immediately merge them we
> now have a bridge that is further broken. And now you want to
> break it even more arguing that you want to force people to
> use the new API, but how do you expect people to migrate to
> this new API if they can't even run this?
>
> Besides, if you want to break API you should at least mention
> that clearly in the patch.
Ok lets leave this patch for a moment to send along with all
Depreacted stuff and api changes.
>
> >> > > > > I think this change should be delayed, preferably
> >> after we have
> >> > > > > a working tidspbridge.
> >> > > >
> >> > > > The issue you were seeing must be fixed with patch 2/8, and
> >> > > Having all
> >> > > > the dependencies tidspbridge has to be working Properly.
> >> > >
> >> > > Yes, which might not happen at .37. If we revert the
> >> iommu stuff, we
> >> > > might have a chance at having a working bridge on .37.
> >> >
> >> > But havi that merged in tidspbridge make more pressure to
> >> merged The
> >> > dependencies, if not maybe that wont have event for .38.
> >>
> >> Those patches have been there for a while and there's no
> sign of when
> >> they'll be merged. Also, remember that the fact that the
> patches have
> >> been sent doesn't guarantee they would be on this, or even
> the next
> >> cycle.
> >>
> >> Besides, the rule followed here is: it's ready when it's ready. In
> >> the meantime the driver should work, and we can do that rather
> >> easily.
> >
> > I don't have much experience in this kind of situacion if it is
> > believed it is the best solution then ok for me.
> >
> >> > But That is just my opinion if you all are agree revert
> is the best
> >> > Option I am ok, but I looks like more work reverting and
> rebaseing
> >> > specially because it is a big change.
> >>
> >> git makes this easy.
> >
> > Till we need to fix all the conflics, make sure everything work
> > Properly and then send it again to review. That can take a
> while, But
> > it seems someone else is agree to revert the changes, so
> maybe That is
> > the best to do in this situation.
>
> Reverting or not reverting is not the question here.
>
> The question is how do we get it working for .37? Are you
> completely certain that all the dependent patches will be
> merged? Has Hiroshi said it would happen? Has the ARM people?
> Are you familiar with the release process enough to say so?
> I'm not so familiar, but my feeling is that it's already
> late, and even if we revert the iommu changes we might not be
> able to get this working on .37 either.
I am not familiar with that process to say if that will happen.
I don't know why a lot of discussion on iommu patches, we
Know the depencies and we have the fixes to make it work. Then
Why don't focus on what is missing? Once all other issues have
Been resolved and the only thing missing are the dependencies
For iommu migration, we can revert those patches and make it work,
Otherwise I don't see the point of reverting the patches.
Regards,
Fernando.
>
> Even if we leave iommu out of the picture, there's still the
> ioremap() issue that will appear when linux-arm is pulled,
> and then we need to deal with memblock changes, which is
> still not clear what will happen.
> And then we need to fix the breakage in the compilation due
> to the layer violation with "control.h". I think that already
> too much uncertainty.
>
> Cheers.
>
> --
> Felipe Contreras
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 70+ messages in thread* RE: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-28 15:38 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-28 15:38 UTC (permalink / raw)
To: Felipe Contreras
Cc: Felipe Contreras, gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Wednesday, October 27, 2010 3:20 AM
> To: Guzman Lugo, Fernando
> Cc: Felipe Contreras; gregkh@suse.de; hiroshi.doyu@nokia.com;
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com;
> linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
> >> fernando.lugo@ti.com wrote:
> >> > > fernando.lugo@ti.com wrote:
> >> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> >> > > > > <x0095840@ti.com> wrote:
> >> > > > > > The device address is assigned by tidspbridge no need for
> >> > > > > that parameter anymore.
> >> > > > > >
> >> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> >> > > > >
> >> > > > > This would break the API with user-space, right?
> >> > > >
> >> > > > Yes, user-space needs to be changed accordingly.
> >> > >
> >> > > Wouldn't it make sense to avoid stuffing so many
> changes at once
> >> > > including ABI breakage?
> >> > >
> >> > > Does user-space really _needs_ to be changed? Can't you
> >> just ignore
> >> > > that argument?
> >> >
> >> > Actually, I had a previous version of that patch where I
> >> only Ignored
> >> > that paramteter. But after thinking again and seeing How
> >> the long time
> >> > ago depreacted function are still there I Removed the
> parameter in
> >> > order to force apps to make the change.
> >>
> >> Again, can we concentrate on first getting this thing to work?
> >
> > If to make it work for 37 the iommu migration patch will be revert
> > These set of patches will have to wait until the patches are merged
> > Again. So the dspbridge would be "fix" first a then the
> patches would
> > Be merged.
>
> I couldn't parse this correctly, but yes, I think.
>
> >> We can think on breaking things again later.
> >>
> >> > You can ignore that argument at API level, so all users of
> >> the API not
> >> > need to have change (in that momment). That should be
> Only few line
> >> > change.
> >>
> >> Yes, that's what I'm proposing.
> >
> > I meant userspace api or library level:
> > Example:
> >
> > Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) { ...
> > struct proc_map args = {
> > .map_addr = mpu_addr;
> > /* ignore req_addr */
> > .dsp_addr = dsp_addr;
> > .attr = attr;
> > }
> >
> > ret = ioctl(handle, PROCMAP_CMD, args) ...
> > }
>
> Yes, but this has to be done on every library/app that is
> using the dsp.
> It's much easier to do it on kernel side.
>
> Besides, at least on gst-dsp we want it to work for _all_
> bridge versions, so it would be:
>
> @@ -829,7 +829,9 @@ struct map_mem {
> void *proc_handle;
> void *mpu_addr;
> unsigned long size;
> +#if DSP_API >= 3
> void *req_addr;
> +#endif
> void **ret_map_addr;
> unsigned long attr;
> };
> @@ -838,7 +840,9 @@ bool dsp_map(int handle,
> void *proc_handle,
> void *mpu_addr,
> unsigned long size,
> void *req_addr,
> void *ret_map_addr,
> unsigned long attr)
> {
> @@ -846,7 +850,9 @@ bool dsp_map(int handle,
> .proc_handle = proc_handle,
> .mpu_addr = mpu_addr,
> .size = size,
> +#if DSP_API >= 3
> .req_addr = req_addr,
> +#endif
> .ret_map_addr = ret_map_addr,
> .attr = attr,
> };
>
> But then if we are breaking the API already, wouldn't it make
> sense to use the DMA direction too? (probably on the 'attr' field)
>
> That's why it's a good idea to discuss API breakage, and not
> do it willi-nillii. Otherwise we would have #if DSP_API >= 4,
> and so on.
>
> But it's a bit pointless to be discussing about this at this
> point while people are still struggling to get this working
> and don't know which branch to use.
>
> Look, you sent patches that broke the bridge (even after
> manually applying all the dependency patches), since Omar
> wasn't there as gatekeeper and Greg immediately merge them we
> now have a bridge that is further broken. And now you want to
> break it even more arguing that you want to force people to
> use the new API, but how do you expect people to migrate to
> this new API if they can't even run this?
>
> Besides, if you want to break API you should at least mention
> that clearly in the patch.
Ok lets leave this patch for a moment to send along with all
Depreacted stuff and api changes.
>
> >> > > > > I think this change should be delayed, preferably
> >> after we have
> >> > > > > a working tidspbridge.
> >> > > >
> >> > > > The issue you were seeing must be fixed with patch 2/8, and
> >> > > Having all
> >> > > > the dependencies tidspbridge has to be working Properly.
> >> > >
> >> > > Yes, which might not happen at .37. If we revert the
> >> iommu stuff, we
> >> > > might have a chance at having a working bridge on .37.
> >> >
> >> > But havi that merged in tidspbridge make more pressure to
> >> merged The
> >> > dependencies, if not maybe that wont have event for .38.
> >>
> >> Those patches have been there for a while and there's no
> sign of when
> >> they'll be merged. Also, remember that the fact that the
> patches have
> >> been sent doesn't guarantee they would be on this, or even
> the next
> >> cycle.
> >>
> >> Besides, the rule followed here is: it's ready when it's ready. In
> >> the meantime the driver should work, and we can do that rather
> >> easily.
> >
> > I don't have much experience in this kind of situacion if it is
> > believed it is the best solution then ok for me.
> >
> >> > But That is just my opinion if you all are agree revert
> is the best
> >> > Option I am ok, but I looks like more work reverting and
> rebaseing
> >> > specially because it is a big change.
> >>
> >> git makes this easy.
> >
> > Till we need to fix all the conflics, make sure everything work
> > Properly and then send it again to review. That can take a
> while, But
> > it seems someone else is agree to revert the changes, so
> maybe That is
> > the best to do in this situation.
>
> Reverting or not reverting is not the question here.
>
> The question is how do we get it working for .37? Are you
> completely certain that all the dependent patches will be
> merged? Has Hiroshi said it would happen? Has the ARM people?
> Are you familiar with the release process enough to say so?
> I'm not so familiar, but my feeling is that it's already
> late, and even if we revert the iommu changes we might not be
> able to get this working on .37 either.
I am not familiar with that process to say if that will happen.
I don't know why a lot of discussion on iommu patches, we
Know the depencies and we have the fixes to make it work. Then
Why don't focus on what is missing? Once all other issues have
Been resolved and the only thing missing are the dependencies
For iommu migration, we can revert those patches and make it work,
Otherwise I don't see the point of reverting the patches.
Regards,
Fernando.
>
> Even if we leave iommu out of the picture, there's still the
> ioremap() issue that will appear when linux-arm is pulled,
> and then we need to deal with memblock changes, which is
> still not clear what will happen.
> And then we need to fix the breakage in the compilation due
> to the layer violation with "control.h". I think that already
> too much uncertainty.
>
> Cheers.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-28 15:38 ` Guzman Lugo, Fernando
0 siblings, 0 replies; 70+ messages in thread
From: Guzman Lugo, Fernando @ 2010-10-28 15:38 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras at gmail.com]
> Sent: Wednesday, October 27, 2010 3:20 AM
> To: Guzman Lugo, Fernando
> Cc: Felipe Contreras; gregkh at suse.de; hiroshi.doyu at nokia.com;
> linux-kernel at vger.kernel.org; andy.shevchenko at gmail.com;
> linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/8] staging: tidspbridge - remove
> req_addr from proc_map
>
> On Tue, Oct 26, 2010 at 11:39 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
> >> fernando.lugo at ti.com wrote:
> >> > > fernando.lugo at ti.com wrote:
> >> > > > > On Tue, Oct 26, 2010 at 3:51 AM, Fernando Guzman Lugo
> >> > > > > <x0095840@ti.com> wrote:
> >> > > > > > The device address is assigned by tidspbridge no need for
> >> > > > > that parameter anymore.
> >> > > > > >
> >> > > > > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> >> > > > >
> >> > > > > This would break the API with user-space, right?
> >> > > >
> >> > > > Yes, user-space needs to be changed accordingly.
> >> > >
> >> > > Wouldn't it make sense to avoid stuffing so many
> changes at once
> >> > > including ABI breakage?
> >> > >
> >> > > Does user-space really _needs_ to be changed? Can't you
> >> just ignore
> >> > > that argument?
> >> >
> >> > Actually, I had a previous version of that patch where I
> >> only Ignored
> >> > that paramteter. But after thinking again and seeing How
> >> the long time
> >> > ago depreacted function are still there I Removed the
> parameter in
> >> > order to force apps to make the change.
> >>
> >> Again, can we concentrate on first getting this thing to work?
> >
> > If to make it work for 37 the iommu migration patch will be revert
> > These set of patches will have to wait until the patches are merged
> > Again. So the dspbridge would be "fix" first a then the
> patches would
> > Be merged.
>
> I couldn't parse this correctly, but yes, I think.
>
> >> We can think on breaking things again later.
> >>
> >> > You can ignore that argument at API level, so all users of
> >> the API not
> >> > need to have change (in that momment). That should be
> Only few line
> >> > change.
> >>
> >> Yes, that's what I'm proposing.
> >
> > I meant userspace api or library level:
> > Example:
> >
> > Dsp_proc_map(proc, mpu_addr, req_addr, *dsp_addr, attr) { ...
> > ? ? ? ?struct proc_map args = {
> > ? ? ? ? ? ? ? ?.map_addr = mpu_addr;
> > ? ? ? ? ? ? ? ?/* ignore req_addr */
> > ? ? ? ? ? ? ? ?.dsp_addr = dsp_addr;
> > ? ? ? ? ? ? ? ?.attr = attr;
> > ? ? ? ?}
> >
> > ? ? ? ?ret = ioctl(handle, PROCMAP_CMD, args) ...
> > }
>
> Yes, but this has to be done on every library/app that is
> using the dsp.
> It's much easier to do it on kernel side.
>
> Besides, at least on gst-dsp we want it to work for _all_
> bridge versions, so it would be:
>
> @@ -829,7 +829,9 @@ struct map_mem {
> void *proc_handle;
> void *mpu_addr;
> unsigned long size;
> +#if DSP_API >= 3
> void *req_addr;
> +#endif
> void **ret_map_addr;
> unsigned long attr;
> };
> @@ -838,7 +840,9 @@ bool dsp_map(int handle,
> void *proc_handle,
> void *mpu_addr,
> unsigned long size,
> void *req_addr,
> void *ret_map_addr,
> unsigned long attr)
> {
> @@ -846,7 +850,9 @@ bool dsp_map(int handle,
> .proc_handle = proc_handle,
> .mpu_addr = mpu_addr,
> .size = size,
> +#if DSP_API >= 3
> .req_addr = req_addr,
> +#endif
> .ret_map_addr = ret_map_addr,
> .attr = attr,
> };
>
> But then if we are breaking the API already, wouldn't it make
> sense to use the DMA direction too? (probably on the 'attr' field)
>
> That's why it's a good idea to discuss API breakage, and not
> do it willi-nillii. Otherwise we would have #if DSP_API >= 4,
> and so on.
>
> But it's a bit pointless to be discussing about this at this
> point while people are still struggling to get this working
> and don't know which branch to use.
>
> Look, you sent patches that broke the bridge (even after
> manually applying all the dependency patches), since Omar
> wasn't there as gatekeeper and Greg immediately merge them we
> now have a bridge that is further broken. And now you want to
> break it even more arguing that you want to force people to
> use the new API, but how do you expect people to migrate to
> this new API if they can't even run this?
>
> Besides, if you want to break API you should at least mention
> that clearly in the patch.
Ok lets leave this patch for a moment to send along with all
Depreacted stuff and api changes.
>
> >> > > > > I think this change should be delayed, preferably
> >> after we have
> >> > > > > a working tidspbridge.
> >> > > >
> >> > > > The issue you were seeing must be fixed with patch 2/8, and
> >> > > Having all
> >> > > > the dependencies tidspbridge has to be working Properly.
> >> > >
> >> > > Yes, which might not happen at .37. If we revert the
> >> iommu stuff, we
> >> > > might have a chance at having a working bridge on .37.
> >> >
> >> > But havi that merged in tidspbridge make more pressure to
> >> merged The
> >> > dependencies, if not maybe that wont have event for .38.
> >>
> >> Those patches have been there for a while and there's no
> sign of when
> >> they'll be merged. Also, remember that the fact that the
> patches have
> >> been sent doesn't guarantee they would be on this, or even
> the next
> >> cycle.
> >>
> >> Besides, the rule followed here is: it's ready when it's ready. In
> >> the meantime the driver should work, and we can do that rather
> >> easily.
> >
> > I don't have much experience in this kind of situacion if it is
> > believed it is the best solution then ok for me.
> >
> >> > But That is just my opinion if you all are agree revert
> is the best
> >> > Option I am ok, but I looks like more work reverting and
> rebaseing
> >> > specially because it is a big change.
> >>
> >> git makes this easy.
> >
> > Till we need to fix all the conflics, make sure everything work
> > Properly and then send it again to review. That can take a
> while, But
> > it seems someone else is agree to revert the changes, so
> maybe That is
> > the best to do in this situation.
>
> Reverting or not reverting is not the question here.
>
> The question is how do we get it working for .37? Are you
> completely certain that all the dependent patches will be
> merged? Has Hiroshi said it would happen? Has the ARM people?
> Are you familiar with the release process enough to say so?
> I'm not so familiar, but my feeling is that it's already
> late, and even if we revert the iommu changes we might not be
> able to get this working on .37 either.
I am not familiar with that process to say if that will happen.
I don't know why a lot of discussion on iommu patches, we
Know the depencies and we have the fixes to make it work. Then
Why don't focus on what is missing? Once all other issues have
Been resolved and the only thing missing are the dependencies
For iommu migration, we can revert those patches and make it work,
Otherwise I don't see the point of reverting the patches.
Regards,
Fernando.
>
> Even if we leave iommu out of the picture, there's still the
> ioremap() issue that will appear when linux-arm is pulled,
> and then we need to deal with memblock changes, which is
> still not clear what will happen.
> And then we need to fix the breakage in the compilation due
> to the layer violation with "control.h". I think that already
> too much uncertainty.
>
> Cheers.
>
> --
> Felipe Contreras
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
2010-10-27 8:19 ` Felipe Contreras
@ 2010-10-28 15:56 ` Russell King - ARM Linux
-1 siblings, 0 replies; 70+ messages in thread
From: Russell King - ARM Linux @ 2010-10-28 15:56 UTC (permalink / raw)
To: Felipe Contreras
Cc: Guzman Lugo, Fernando, gregkh@suse.de, hiroshi.doyu@nokia.com,
linux-kernel@vger.kernel.org, Felipe Contreras,
andy.shevchenko@gmail.com, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
On Wed, Oct 27, 2010 at 11:19:47AM +0300, Felipe Contreras wrote:
> Yes, but this has to be done on every library/app that is using the dsp.
> It's much easier to do it on kernel side.
>
> Besides, at least on gst-dsp we want it to work for _all_ bridge
> versions, so it would be:
>
> @@ -829,7 +829,9 @@ struct map_mem {
> void *proc_handle;
> void *mpu_addr;
> unsigned long size;
> +#if DSP_API >= 3
> void *req_addr;
> +#endif
> void **ret_map_addr;
> unsigned long attr;
> };
This thread is getting really stupid. There is a process for changing
the arguments to ioctl.
The first thing is that ioctl numbers are supposed to be defined in a
standard form - using _IO(), _IOR(), _IOW() and _IOWR(). Amongst the
things that these macros take is the structure which is being passed.
What this means is that the ioctl number generated by these macros is
directly related to the size of the structure. This immediately gives
a way to deal with a limited set of changes to the structure.
The steps are as follows:
1. Rename the structure and all uses of it from "struct map_mem" to
"struct old_map_mem"
2. Rename the ioctl definition names and all uses to have an OLD prefix.
So at this stage, the original ioctl number and structure are still
present, and importantly are unchanged except by name.
3. Add the new "struct map_mem".
4. Add the new ioctl name definition using "struct map_mem" to identify
it.
5. Implement the new ioctl number. Optionally, implement the old ioctl
functionality via the new implementation if this is appropriate - but
make sure that the old ioctl still works.
6. arrange for users of the old ioctl to receive a warning that it's
deprecated.
This allows existing userspace to continue working, but with warnings so
that people know that they need to rebuild their libraries against the
new structures.
There's no need for a DSP_API define using this method, provided you
don't end up with these structures shrinking and then re-growing back
to a size that they were before.
^ permalink raw reply [flat|nested] 70+ messages in thread* [PATCH 1/8] staging: tidspbridge - remove req_addr from proc_map
@ 2010-10-28 15:56 ` Russell King - ARM Linux
0 siblings, 0 replies; 70+ messages in thread
From: Russell King - ARM Linux @ 2010-10-28 15:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 27, 2010 at 11:19:47AM +0300, Felipe Contreras wrote:
> Yes, but this has to be done on every library/app that is using the dsp.
> It's much easier to do it on kernel side.
>
> Besides, at least on gst-dsp we want it to work for _all_ bridge
> versions, so it would be:
>
> @@ -829,7 +829,9 @@ struct map_mem {
> void *proc_handle;
> void *mpu_addr;
> unsigned long size;
> +#if DSP_API >= 3
> void *req_addr;
> +#endif
> void **ret_map_addr;
> unsigned long attr;
> };
This thread is getting really stupid. There is a process for changing
the arguments to ioctl.
The first thing is that ioctl numbers are supposed to be defined in a
standard form - using _IO(), _IOR(), _IOW() and _IOWR(). Amongst the
things that these macros take is the structure which is being passed.
What this means is that the ioctl number generated by these macros is
directly related to the size of the structure. This immediately gives
a way to deal with a limited set of changes to the structure.
The steps are as follows:
1. Rename the structure and all uses of it from "struct map_mem" to
"struct old_map_mem"
2. Rename the ioctl definition names and all uses to have an OLD prefix.
So at this stage, the original ioctl number and structure are still
present, and importantly are unchanged except by name.
3. Add the new "struct map_mem".
4. Add the new ioctl name definition using "struct map_mem" to identify
it.
5. Implement the new ioctl number. Optionally, implement the old ioctl
functionality via the new implementation if this is appropriate - but
make sure that the old ioctl still works.
6. arrange for users of the old ioctl to receive a warning that it's
deprecated.
This allows existing userspace to continue working, but with warnings so
that people know that they need to rebuild their libraries against the
new structures.
There's no need for a DSP_API define using this method, provided you
don't end up with these structures shrinking and then re-growing back
to a size that they were before.
^ permalink raw reply [flat|nested] 70+ messages in thread