[PATCH v2] drm/exynos: fimd: clear channel before enabling iommu

[Inki Dae]

Hi Daniel,

Please use only text format. :)


>From: djkurtz at google.com [mailto:djkurtz at google.com] On Behalf Of Daniel Kurtz
>Sent: Wednesday, April 30, 2014 1:57 PM
>To: Inki Dae
>Cc: David Airlie; dri-devel; Akshu Agrawal; Prathyush K
>Subject: Re: [PATCH v2] drm/exynos: fimd: clear channel before enabling iommu
>
>
>
>On Tue, Apr 29, 2014 at 9:38 AM, Inki Dae <inki.dae at samsung.com> wrote:
>From: Akshu Agrawal <akshu.a at samsung.com>
>
>If any fimd channel was already active, initializing iommu will result
>in a PAGE FAULT (e.e. u-boot could have turned on the display and
>not disabled it before the kernel starts). This patch checks if any
>channel is active before initializing iommu and disables it.
>
>Changelog v2:
>- consider SoC without SHADOWCON register
>
>Signed-off-by: Akshu Agrawal <akshu.a at samsung.com>
>Signed-off-by: Prathyush K <prathyush.k at samsung.com>
>Signed-off-by: Inki Dae <inki.dae at samsung.com>
>---
> drivers/gpu/drm/exynos/exynos_drm_fimd.c |   70 +++++++++++++++++++++---------
> 1 file changed, 50 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>index 40fd6cc..ef21ce4 100644
>--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>@@ -143,6 +143,48 @@ static inline struct fimd_driver_data *drm_fimd_get_driver_data(
>        return (struct fimd_driver_data *)of_id->data;
> }
>
>+static void fimd_wait_for_vblank(struct exynos_drm_manager *mgr)
>+{
>+       struct fimd_context *ctx = mgr->ctx;
>+
>+       if (ctx->suspended)
>+               return;
>+
>
>Hi Inki,
>
>I think you need this to guarantee that the irq is actually enabled:
>

Right, it needs to make sure that the irq is enabled.


>	drm_vblank_get(ctx->drm_dev, ctx->pipe);
>
>+       atomic_set(&ctx->wait_vsync_event, 1);
>+
>+       /*
>+        * wait for FIMD to signal VSYNC interrupt or return after
>+        * timeout which is set to 50ms (refresh rate of 20).
>+        */
>+       if (!wait_event_timeout(ctx->wait_vsync_queue,
>+                               !atomic_read(&ctx->wait_vsync_event),
>+                               HZ/20))
>+               DRM_DEBUG_KMS("vblank wait timed out.\n");
>
>	drm_vblank_put(ctx->drm_dev, ctx->pipe);
> 
>+}
>+
>+
>+static void fimd_clear_channel(struct exynos_drm_manager *mgr)
>+{
>+       struct fimd_context *ctx = mgr->ctx;
>+       int win, ch_enabled = 0;
>+
>+       DRM_DEBUG_KMS("%s\n", __FILE__);
>+
>+       /* Check if any channel is enabled. */
>+       for (win = 0; win < WINDOWS_NR; win++) {
>+               u32 val = readl(ctx->regs + SHADOWCON);
>+               if (val & SHADOWCON_CHx_ENABLE(win)) {
>+                       val &= ~SHADOWCON_CHx_ENABLE(win);
>+                       writel(val, ctx->regs + SHADOWCON);
>+                       ch_enabled = 1;
>+               }
>+       }
>+
>+       /* Wait for vsync, as disable channel takes effect at next vsync */
>+       if (ch_enabled)
>+               fimd_wait_for_vblank(mgr);
>+}
>+
> static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
>                        struct drm_device *drm_dev, int pipe)
> {
>@@ -169,8 +211,15 @@ static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
>        drm_dev->vblank_disable_allowed = true;
>
>        /* attach this sub driver to iommu mapping if supported. */
>-       if (is_drm_iommu_supported(ctx->drm_dev))
>+       if (is_drm_iommu_supported(ctx->drm_dev)) {
>+               /*
>+                * If any channel is already active, iommu will throw
>+                * a PAGE FAULT when enabled. So clear any channel if enabled.
>+                */
>+               if (ctx->driver_data->has_shadowcon)
>+                       fimd_clear_channel(mgr);
>
>We already disable all overlays at the end of fimd_probe() by calling fimd_clear_win().
>Perhaps all that was missing was just waiting until the next vblank to ensure that these clears have all completed?
>

No, fimd_mgr_initialize() will be called at load() so the iommu unit for fimd ip will be tried to be enabled prior to fimd_clear_channel() call - before all dma channels are disabled. In this case, page fault could be incurred if fimd ip was already on by bootloader. 

Thanks,
Inki Dae

>Best Regards,
>-Daniel
>
>                drm_iommu_attach_device(ctx->drm_dev, ctx->dev);
>+       }
>
>        return 0;
> }
>@@ -324,25 +373,6 @@ static void fimd_disable_vblank(struct exynos_drm_manager *mgr)
>        }
> }
>
>-static void fimd_wait_for_vblank(struct exynos_drm_manager *mgr)
>-{
>-       struct fimd_context *ctx = mgr->ctx;
>-
>-       if (ctx->suspended)
>-               return;
>-
>-       atomic_set(&ctx->wait_vsync_event, 1);
>-
>-       /*
>-        * wait for FIMD to signal VSYNC interrupt or return after
>-        * timeout which is set to 50ms (refresh rate of 20).
>-        */
>-       if (!wait_event_timeout(ctx->wait_vsync_queue,
>-                               !atomic_read(&ctx->wait_vsync_event),
>-                               HZ/20))
>-               DRM_DEBUG_KMS("vblank wait timed out.\n");
>-}
>-
> static void fimd_win_mode_set(struct exynos_drm_manager *mgr,
>                        struct exynos_drm_overlay *overlay)
> {
>--
>1.7.9.5
>
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/dri-devel