Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

2017-06-19 Thread Marek Szyprowski 

Hi All,

I'm sorry for the late reply, I just got back from holidays.

On 2017-06-02 23:43, Jacek Anaszewski wrote:

Cc Marek Szyprowski.

Marek, could you share your opinion about this patch?

On 06/02/2017 06:02 PM, Thierry Escande wrote:

From: Tony K Nadackal 

This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
and ARM DMA IOMMU configurations are supported. The address space is
created with size limited to 256M and base address set to 0x2000.


Could you clarify WHY this patch is needed? IOMMU core configures per-device
IO address space by default and AFAIR JPEG module doesn't have any specific
requirements for the IO address space layout (base or size), so it should
work fine (and works in my tests!) without this patch.

Please drop this patch for now.


Signed-off-by: Tony K Nadackal 
Signed-off-by: Thierry Escande 
---
  drivers/media/platform/s5p-jpeg/jpeg-core.c | 77 +
  1 file changed, 77 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 770a709..5569b99 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -28,6 +28,14 @@
  #include 
  #include 
  #include 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#endif
  
  #include "jpeg-core.h"

  #include "jpeg-hw-s5p.h"
@@ -35,6 +43,10 @@
  #include "jpeg-hw-exynos3250.h"
  #include "jpeg-regs.h"
  
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)

+static struct dma_iommu_mapping *mapping;
+#endif
+
  static struct s5p_jpeg_fmt sjpeg_formats[] = {
{
.name   = "JPEG JFIF",
@@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx 
*ctx)
}
  }
  
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)

+static int jpeg_iommu_init(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+   int err;
+
+   mapping = arm_iommu_create_mapping(_bus_type, 0x2000,
+  SZ_512M);
+   if (IS_ERR(mapping)) {
+   dev_err(dev, "IOMMU mapping failed\n");
+   return PTR_ERR(mapping);
+   }
+
+   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
+   if (!dev->dma_parms) {
+   err = -ENOMEM;
+   goto error_alloc;
+   }
+
+   err = dma_set_max_seg_size(dev, 0xu);
+   if (err)
+   goto error;
+
+   err = arm_iommu_attach_device(dev, mapping);
+   if (err)
+   goto error;
+
+   return 0;
+
+error:
+   devm_kfree(dev, dev->dma_parms);
+   dev->dma_parms = NULL;
+
+error_alloc:
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+
+   return err;
+}
+
+static void jpeg_iommu_deinit(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+
+   if (mapping) {
+   arm_iommu_detach_device(dev);
+   devm_kfree(dev, dev->dma_parms);
+   dev->dma_parms = NULL;
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+   }
+}
+#endif
+
  /*
   * 

   * Device file operations
@@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
spin_lock_init(>slock);
jpeg->dev = >dev;
  
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)

+   ret = jpeg_iommu_init(pdev);
+   if (ret) {
+   dev_err(>dev, "IOMMU Initialization failed\n");
+   return ret;
+   }
+#endif
/* memory-mapped registers */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  
@@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)

clk_disable_unprepare(jpeg->clocks[i]);
}
  
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)

+   jpeg_iommu_deinit(pdev);
+#endif
+
return 0;
  }
  



Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

2017-06-05 Thread Sylwester Nawrocki 

On 06/02/2017 06:02 PM, Thierry Escande wrote:

From: Tony K Nadackal 

This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
and ARM DMA IOMMU configurations are supported. The address space is
created with size limited to 256M and base address set to 0x2000.


I don't think this patch is needed now, a few things changed in mainline
since v3.8. The mapping is being created automatically now for this single
JPEG CODEC device by the driver core/dma-mapping code AFAICS.
See dma_configure() in drivers/base/dd.c.
I doubt we need a specific CPU address range, but even if we would shouldn't
it be specified through the dma-ranges DT property?


Signed-off-by: Tony K Nadackal 
Signed-off-by: Thierry Escande 
---
  drivers/media/platform/s5p-jpeg/jpeg-core.c | 77 +
  1 file changed, 77 insertions(+)



+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+static int jpeg_iommu_init(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+   int err;
+
+   mapping = arm_iommu_create_mapping(_bus_type, 0x2000,
+  SZ_512M);
+   if (IS_ERR(mapping)) {
+   dev_err(dev, "IOMMU mapping failed\n");
+   return PTR_ERR(mapping);
+   }
+
+   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);


dev->dma_parms seems to be unused.


+   if (!dev->dma_parms) {
+   err = -ENOMEM;
+   goto error_alloc;
+   }
+
+   err = dma_set_max_seg_size(dev, 0xu);
+   if (err)
+   goto error;
+
+   err = arm_iommu_attach_device(dev, mapping);
+   if (err)
+   goto error;
+
+   return 0;
+
+error:
+   devm_kfree(dev, dev->dma_parms);


There is no need for this devm_kfree() call.


+   dev->dma_parms = NULL;
+
+error_alloc:
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+
+   return err;
+}
+
+static void jpeg_iommu_deinit(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+
+   if (mapping) {
+   arm_iommu_detach_device(dev);
+   devm_kfree(dev, dev->dma_parms);


Ditto.


+   dev->dma_parms = NULL;
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+   }
+}



  /*
   * 

   * Device file operations
@@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)



+   ret = jpeg_iommu_init(pdev);



@@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)



+   jpeg_iommu_deinit(pdev);



return 0;
  }


--
Thanks,
Sylwester

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

2017-06-02 Thread Shuah Khan 
On Fri, Jun 2, 2017 at 10:02 AM, Thierry Escande
 wrote:
> From: Tony K Nadackal 
>
> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
> and ARM DMA IOMMU configurations are supported. The address space is
> created with size limited to 256M and base address set to 0x2000.
>
> Signed-off-by: Tony K Nadackal 
> Signed-off-by: Thierry Escande 
> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 77 
> +
>  1 file changed, 77 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
> b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 770a709..5569b99 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -28,6 +28,14 @@
>  #include 
>  #include 
>  #include 
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
>
>  #include "jpeg-core.h"
>  #include "jpeg-hw-s5p.h"
> @@ -35,6 +43,10 @@
>  #include "jpeg-hw-exynos3250.h"
>  #include "jpeg-regs.h"
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static struct dma_iommu_mapping *mapping;
> +#endif
> +
>  static struct s5p_jpeg_fmt sjpeg_formats[] = {
> {
> .name   = "JPEG JFIF",
> @@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx 
> *ctx)
> }
>  }
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static int jpeg_iommu_init(struct platform_device *pdev)
> +{
> +   struct device *dev = >dev;
> +   int err;
> +
> +   mapping = arm_iommu_create_mapping(_bus_type, 0x2000,
> +  SZ_512M);

Change log says 256M??

What happens when another driver uses the same start point?
exynos drm uses the same  looks like

EXYNOS_DEV_ADDR_START   0x2000

> +   if (IS_ERR(mapping)) {
> +   dev_err(dev, "IOMMU mapping failed\n");
> +   return PTR_ERR(mapping);
> +   }
> +
> +   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), 
> GFP_KERNEL);
> +   if (!dev->dma_parms) {
> +   err = -ENOMEM;
> +   goto error_alloc;
> +   }
> +
> +   err = dma_set_max_seg_size(dev, 0xu);

You could use DMA_BIT_MASK(32) instead of 0xu

> +   if (err)
> +   goto error;
> +
> +   err = arm_iommu_attach_device(dev, mapping);
> +   if (err)
> +   goto error;
> +
> +   return 0;
> +
> +error:
> +   devm_kfree(dev, dev->dma_parms);
> +   dev->dma_parms = NULL;
> +
> +error_alloc:
> +   arm_iommu_release_mapping(mapping);
> +   mapping = NULL;
> +
> +   return err;
> +}
> +
> +static void jpeg_iommu_deinit(struct platform_device *pdev)
> +{
> +   struct device *dev = >dev;
> +
> +   if (mapping) {
> +   arm_iommu_detach_device(dev);
> +   devm_kfree(dev, dev->dma_parms);
> +   dev->dma_parms = NULL;
> +   arm_iommu_release_mapping(mapping);
> +   mapping = NULL;
> +   }
> +}
> +#endif
> +
>  /*
>   * 
> 
>   * Device file operations
> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
> spin_lock_init(>slock);
> jpeg->dev = >dev;
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +   ret = jpeg_iommu_init(pdev);
> +   if (ret) {
> +   dev_err(>dev, "IOMMU Initialization failed\n");
> +   return ret;
> +   }
> +#endif

You might be able to avoid use of ifdefs if you define stubs for !defines case.

> /* memory-mapped registers */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device 
> *pdev)
> clk_disable_unprepare(jpeg->clocks[i]);
> }
>
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +   jpeg_iommu_deinit(pdev);
> +#endif
> +
> return 0;
>  }
>
> --
> 2.7.4
>

Re: [PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

2017-06-02 Thread Jacek Anaszewski 
Cc Marek Szyprowski.

Marek, could you share your opinion about this patch?

On 06/02/2017 06:02 PM, Thierry Escande wrote:
> From: Tony K Nadackal 
> 
> This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
> and ARM DMA IOMMU configurations are supported. The address space is
> created with size limited to 256M and base address set to 0x2000.
> 
> Signed-off-by: Tony K Nadackal 
> Signed-off-by: Thierry Escande 
> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 77 
> +
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
> b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 770a709..5569b99 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -28,6 +28,14 @@
>  #include 
>  #include 
>  #include 
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
>  
>  #include "jpeg-core.h"
>  #include "jpeg-hw-s5p.h"
> @@ -35,6 +43,10 @@
>  #include "jpeg-hw-exynos3250.h"
>  #include "jpeg-regs.h"
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static struct dma_iommu_mapping *mapping;
> +#endif
> +
>  static struct s5p_jpeg_fmt sjpeg_formats[] = {
>   {
>   .name   = "JPEG JFIF",
> @@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx 
> *ctx)
>   }
>  }
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> +static int jpeg_iommu_init(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + int err;
> +
> + mapping = arm_iommu_create_mapping(_bus_type, 0x2000,
> +SZ_512M);
> + if (IS_ERR(mapping)) {
> + dev_err(dev, "IOMMU mapping failed\n");
> + return PTR_ERR(mapping);
> + }
> +
> + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
> + if (!dev->dma_parms) {
> + err = -ENOMEM;
> + goto error_alloc;
> + }
> +
> + err = dma_set_max_seg_size(dev, 0xu);
> + if (err)
> + goto error;
> +
> + err = arm_iommu_attach_device(dev, mapping);
> + if (err)
> + goto error;
> +
> + return 0;
> +
> +error:
> + devm_kfree(dev, dev->dma_parms);
> + dev->dma_parms = NULL;
> +
> +error_alloc:
> + arm_iommu_release_mapping(mapping);
> + mapping = NULL;
> +
> + return err;
> +}
> +
> +static void jpeg_iommu_deinit(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> +
> + if (mapping) {
> + arm_iommu_detach_device(dev);
> + devm_kfree(dev, dev->dma_parms);
> + dev->dma_parms = NULL;
> + arm_iommu_release_mapping(mapping);
> + mapping = NULL;
> + }
> +}
> +#endif
> +
>  /*
>   * 
> 
>   * Device file operations
> @@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>   spin_lock_init(>slock);
>   jpeg->dev = >dev;
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> + ret = jpeg_iommu_init(pdev);
> + if (ret) {
> + dev_err(>dev, "IOMMU Initialization failed\n");
> + return ret;
> + }
> +#endif
>   /* memory-mapped registers */
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> @@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device 
> *pdev)
>   clk_disable_unprepare(jpeg->clocks[i]);
>   }
>  
> +#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
> + jpeg_iommu_deinit(pdev);
> +#endif
> +
>   return 0;
>  }
>  
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH 5/9] [media] s5p-jpeg: Add IOMMU support

2017-06-02 Thread Thierry Escande 
From: Tony K Nadackal 

This patch adds support for IOMMU s5p-jpeg driver if the Exynos IOMMU
and ARM DMA IOMMU configurations are supported. The address space is
created with size limited to 256M and base address set to 0x2000.

Signed-off-by: Tony K Nadackal 
Signed-off-by: Thierry Escande 
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 77 +
 1 file changed, 77 insertions(+)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 770a709..5569b99 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -28,6 +28,14 @@
 #include 
 #include 
 #include 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#endif
 
 #include "jpeg-core.h"
 #include "jpeg-hw-s5p.h"
@@ -35,6 +43,10 @@
 #include "jpeg-hw-exynos3250.h"
 #include "jpeg-regs.h"
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+static struct dma_iommu_mapping *mapping;
+#endif
+
 static struct s5p_jpeg_fmt sjpeg_formats[] = {
{
.name   = "JPEG JFIF",
@@ -956,6 +968,60 @@ static void exynos4_jpeg_parse_q_tbl(struct s5p_jpeg_ctx 
*ctx)
}
 }
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+static int jpeg_iommu_init(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+   int err;
+
+   mapping = arm_iommu_create_mapping(_bus_type, 0x2000,
+  SZ_512M);
+   if (IS_ERR(mapping)) {
+   dev_err(dev, "IOMMU mapping failed\n");
+   return PTR_ERR(mapping);
+   }
+
+   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
+   if (!dev->dma_parms) {
+   err = -ENOMEM;
+   goto error_alloc;
+   }
+
+   err = dma_set_max_seg_size(dev, 0xu);
+   if (err)
+   goto error;
+
+   err = arm_iommu_attach_device(dev, mapping);
+   if (err)
+   goto error;
+
+   return 0;
+
+error:
+   devm_kfree(dev, dev->dma_parms);
+   dev->dma_parms = NULL;
+
+error_alloc:
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+
+   return err;
+}
+
+static void jpeg_iommu_deinit(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+
+   if (mapping) {
+   arm_iommu_detach_device(dev);
+   devm_kfree(dev, dev->dma_parms);
+   dev->dma_parms = NULL;
+   arm_iommu_release_mapping(mapping);
+   mapping = NULL;
+   }
+}
+#endif
+
 /*
  * 
  * Device file operations
@@ -2816,6 +2882,13 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
spin_lock_init(>slock);
jpeg->dev = >dev;
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+   ret = jpeg_iommu_init(pdev);
+   if (ret) {
+   dev_err(>dev, "IOMMU Initialization failed\n");
+   return ret;
+   }
+#endif
/* memory-mapped registers */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
@@ -2962,6 +3035,10 @@ static int s5p_jpeg_remove(struct platform_device *pdev)
clk_disable_unprepare(jpeg->clocks[i]);
}
 
+#if defined(CONFIG_EXYNOS_IOMMU) && defined(CONFIG_ARM_DMA_USE_IOMMU)
+   jpeg_iommu_deinit(pdev);
+#endif
+
return 0;
 }
 
-- 
2.7.4