> -----Original Message-----
> From: djkurtz at google.com [mailto:djkurtz at google.com] On Behalf Of Daniel
> Kurtz
> Sent: Wednesday, April 30, 2014 3:21 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 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.
>
Again,
Good idea but it doesn't consider drm_device object that it needs to check if
iommu is supported or not - if iommu is not supported,
drm_iommu_attach_device() shouldn't be called. At probe(), we couldn't get
drm_device object. Anyway, there may be another better way.
Thanks,
Inki Dae
> >
> > 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
> >
> >
