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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: djkurtz@xxxxxxxxxx [mailto:djkurtz@xxxxxxxxxx] 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@xxxxxxxxxxx> wrote:
> > Hi Daniel,
> >
> > Please use only text format. :)
> >
> >
> >>From: djkurtz@xxxxxxxxxx [mailto:djkurtz@xxxxxxxxxx] 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@xxxxxxxxxxx> wrote:
> >>From: Akshu Agrawal <akshu.a@xxxxxxxxxxx>
> >>
> >>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@xxxxxxxxxxx>
> >>Signed-off-by: Prathyush K <prathyush.k@xxxxxxxxxxx>
> >>Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> >>---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> >>http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux