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

[Daniel Kurtz]

On Wed, Apr 30, 2014 at 1:44 PM, Inki Dae <inki.dae at samsung.com> wrote:
> 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.

Right.  So, I suggest moving drm_iommu_attach_device() from
mgr_initialize to fimd_probe(), after clearing the windows and waiting
for vblank.

>
> 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
>
>