| Message ID | 1398735488-19475-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 29, 2014 at 9:38 AM, Inki Dae <inki.dae@samsung.com> wrote: > From: Akshu Agrawal <akshu.a@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@samsung.com> > Signed-off-by: Prathyush K <prathyush.k@samsung.com> > Signed-off-by: Inki Dae <inki.dae@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: 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? 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Daniel, Please use only text format. :) >From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: >From: Akshu Agrawal <akshu.a@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@samsung.com> >Signed-off-by: Prathyush K <prathyush.k@samsung.com> >Signed-off-by: Inki Dae <inki.dae@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@lists.freedesktop.org >http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Apr 30, 2014 at 1:44 PM, Inki Dae <inki.dae@samsung.com> wrote: > Hi Daniel, > > Please use only text format. :) > > >>From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: >>From: Akshu Agrawal <akshu.a@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@samsung.com> >>Signed-off-by: Prathyush K <prathyush.k@samsung.com> >>Signed-off-by: Inki Dae <inki.dae@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@lists.freedesktop.org >>http://lists.freedesktop.org/mailman/listinfo/dri-devel > >
> -----Original Message----- > From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: > > Hi Daniel, > > > > Please use only text format. :) > > > > > >>From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: > >>From: Akshu Agrawal <akshu.a@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@samsung.com> > >>Signed-off-by: Prathyush K <prathyush.k@samsung.com> > >>Signed-off-by: Inki Dae <inki.dae@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. > It seems more clear. I tried to take care of patch already posted so I didn't consider a 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@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > >
> -----Original Message----- > From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: > > Hi Daniel, > > > > Please use only text format. :) > > > > > >>From: djkurtz@google.com [mailto:djkurtz@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@samsung.com> wrote: > >>From: Akshu Agrawal <akshu.a@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@samsung.com> > >>Signed-off-by: Prathyush K <prathyush.k@samsung.com> > >>Signed-off-by: Inki Dae <inki.dae@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@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > >
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; + + 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_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); 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) {