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