[PATCH 0/3] Exynos SYSMMU (IOMMU) updates for Exynos DRM

[PATCH 0/3] Exynos SYSMMU (IOMMU) updates for Exynos DRM

[Marek Szyprowski]

Hello,

Main changes for Exynos SYSMMU (IOMMU) driver has been finally scheduled
for merging - see
https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/commit/?h=next

To let Exynos IOMMU driver work correctly, some fixes are also needed in
Exynos DRM driver. To ease merge process I've decided to rebased Exynos
DRM changes onto latest exynos-drm-next tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
exynos-drm-next

I've also added one more patch fixing issue, which can be observed when
upcoming IOMMU probe deferal patches has been applied. To hide
dma-mapping internals from Exynos DRM driver, a direct check for NULL
has been replaced by more error-proof comparison of dma_get_ops()
values.

Inki: could you queue those patches to exynos-drm-next and get them
merged to v4.2 to have it functional for the release?

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Marek Szyprowski (3):
  drm/exynos: fimd: ensure proper hw state in fimd_clear_channel()
  drm/exynos: iommu: detach from default dma-mapping domain on init
  drm/exynos: iommu: improve a check for non-iommu dma_ops

 drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 26 +++++++++++++++++---------
 drivers/gpu/drm/exynos/exynos_drm_iommu.c |  7 +++++--
 2 files changed, 22 insertions(+), 11 deletions(-)

-- 
1.9.2

[PATCH 1/3] drm/exynos: fimd: ensure proper hw state in fimd_clear_channel()

[Marek Szyprowski]

One should not do any assumptions on the stare of the fimd hardware
during driver initialization, so to properly reset fimd before enabling
IOMMU, one should ensure that all power domains and clocks are really
enabled. This patch adds calls to power on/off in the
fimd_clear_channel() function to ensure that any access to fimd
registers will be performed with clocks and power domains enabled.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a0edab833148..d10ad3920e78 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -242,12 +242,21 @@ static void fimd_enable_shadow_channel_path(struct fimd_context *ctx,
 	writel(val, ctx->regs + SHADOWCON);
 }
 
-static void fimd_clear_channel(struct fimd_context *ctx)
+static int fimd_poweron(struct fimd_context *ctx);
+static int fimd_poweroff(struct fimd_context *ctx);
+
+static int fimd_clear_channel(struct fimd_context *ctx)
 {
 	unsigned int win, ch_enabled = 0;
+	int ret;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
+	/* Hardware is in unknown state, so ensure it get enabled properly */
+	ret = fimd_poweron(ctx);
+	if (ret)
+		return ret;
+
 	/* Check if any channel is enabled. */
 	for (win = 0; win < WINDOWS_NR; win++) {
 		u32 val = readl(ctx->regs + WINCON(win));
@@ -258,19 +267,15 @@ static void fimd_clear_channel(struct fimd_context *ctx)
 			if (ctx->driver_data->has_shadowcon)
 				fimd_enable_shadow_channel_path(ctx, win,
 								false);
-
 			ch_enabled = 1;
 		}
 	}
 
 	/* Wait for vsync, as disable channel takes effect at next vsync */
-	if (ch_enabled) {
-		unsigned int state = ctx->suspended;
-
-		ctx->suspended = 0;
+	if (ch_enabled)
 		fimd_wait_for_vblank(ctx->crtc);
-		ctx->suspended = state;
-	}
+
+	return fimd_poweroff(ctx);
 }
 
 static int fimd_iommu_attach_devices(struct fimd_context *ctx,
@@ -285,7 +290,10 @@ static int fimd_iommu_attach_devices(struct fimd_context *ctx,
 		 * If any channel is already active, iommu will throw
 		 * a PAGE FAULT when enabled. So clear any channel if enabled.
 		 */
-		fimd_clear_channel(ctx);
+		ret = fimd_clear_channel(ctx);
+		if (ret)
+			return ret;
+
 		ret = drm_iommu_attach_device(ctx->drm_dev, ctx->dev);
 		if (ret) {
 			DRM_ERROR("drm_iommu_attach failed.\n");
-- 
1.9.2

[PATCH 2/3] drm/exynos: iommu: detach from default dma-mapping domain on init

[Marek Szyprowski]

This patch adds code, which detach sub-device nodes from default iommu
domain if such has been configured. This lets Exynos DRM driver to properly
attach sub-devices to its own, common for all sub-devices domain.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Tested-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
 drivers/gpu/drm/exynos/exynos_drm_iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
index b32b291f88ff..323601a52a25 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
@@ -100,6 +100,9 @@ int drm_iommu_attach_device(struct drm_device *drm_dev,
 
 	dma_set_max_seg_size(subdrv_dev, 0xffffffffu);
 
+	if (subdrv_dev->archdata.mapping)
+		arm_iommu_detach_device(subdrv_dev);
+
 	ret = arm_iommu_attach_device(subdrv_dev, dev->archdata.mapping);
 	if (ret < 0) {
 		DRM_DEBUG_KMS("failed iommu attach.\n");
-- 
1.9.2

[PATCH 3/3] drm/exynos: iommu: improve a check for non-iommu dma_ops

[Marek Szyprowski]

DRM Exynos driver is relying on dma-mapping internal structures when used
with IOMMU enabled. This patch partially hides dma-mapping internal things
by using proper get_dma_ops/set_dma_ops calls.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
index 323601a52a25..34596da7be33 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
@@ -117,8 +117,8 @@ int drm_iommu_attach_device(struct drm_device *drm_dev,
 	 * If iommu attach succeeded, the sub driver would have dma_ops
 	 * for iommu and also all sub drivers have same dma_ops.
 	 */
-	if (!dev->archdata.dma_ops)
-		dev->archdata.dma_ops = subdrv_dev->archdata.dma_ops;
+	if (get_dma_ops(dev) == get_dma_ops(NULL))
+		set_dma_ops(dev, get_dma_ops(subdrv_dev));
 
 	return 0;
 }
-- 
1.9.2

Re: [PATCH 1/3] drm/exynos: fimd: ensure proper hw state in fimd_clear_channel()

[Inki Dae]

Hi Marek,

I have merged atomic patch series. Can you re-base your patch series on
top of exynos-drm-next?

Thanks,
Inki Dae

On 2015년 06월 01일 20:15, Marek Szyprowski wrote:
> One should not do any assumptions on the stare of the fimd hardware
> during driver initialization, so to properly reset fimd before enabling
> IOMMU, one should ensure that all power domains and clocks are really
> enabled. This patch adds calls to power on/off in the
> fimd_clear_channel() function to ensure that any access to fimd
> registers will be performed with clocks and power domains enabled.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index a0edab833148..d10ad3920e78 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -242,12 +242,21 @@ static void fimd_enable_shadow_channel_path(struct fimd_context *ctx,
>  	writel(val, ctx->regs + SHADOWCON);
>  }
>  
> -static void fimd_clear_channel(struct fimd_context *ctx)
> +static int fimd_poweron(struct fimd_context *ctx);
> +static int fimd_poweroff(struct fimd_context *ctx);
> +
> +static int fimd_clear_channel(struct fimd_context *ctx)
>  {
>  	unsigned int win, ch_enabled = 0;
> +	int ret;
>  
>  	DRM_DEBUG_KMS("%s\n", __FILE__);
>  
> +	/* Hardware is in unknown state, so ensure it get enabled properly */
> +	ret = fimd_poweron(ctx);
> +	if (ret)
> +		return ret;
> +
>  	/* Check if any channel is enabled. */
>  	for (win = 0; win < WINDOWS_NR; win++) {
>  		u32 val = readl(ctx->regs + WINCON(win));
> @@ -258,19 +267,15 @@ static void fimd_clear_channel(struct fimd_context *ctx)
>  			if (ctx->driver_data->has_shadowcon)
>  				fimd_enable_shadow_channel_path(ctx, win,
>  								false);
> -
>  			ch_enabled = 1;
>  		}
>  	}
>  
>  	/* Wait for vsync, as disable channel takes effect at next vsync */
> -	if (ch_enabled) {
> -		unsigned int state = ctx->suspended;
> -
> -		ctx->suspended = 0;
> +	if (ch_enabled)
>  		fimd_wait_for_vblank(ctx->crtc);
> -		ctx->suspended = state;
> -	}
> +
> +	return fimd_poweroff(ctx);
>  }
>  
>  static int fimd_iommu_attach_devices(struct fimd_context *ctx,
> @@ -285,7 +290,10 @@ static int fimd_iommu_attach_devices(struct fimd_context *ctx,
>  		 * If any channel is already active, iommu will throw
>  		 * a PAGE FAULT when enabled. So clear any channel if enabled.
>  		 */
> -		fimd_clear_channel(ctx);
> +		ret = fimd_clear_channel(ctx);
> +		if (ret)
> +			return ret;
> +
>  		ret = drm_iommu_attach_device(ctx->drm_dev, ctx->dev);
>  		if (ret) {
>  			DRM_ERROR("drm_iommu_attach failed.\n");
>