[PATCH] drm/exynos: remove chained calls to enable
Marek Szyprowski
m.szyprowski at samsung.com
Thu Jun 11 00:52:06 PDT 2015
Hello,
On 2015-06-11 09:07, Joonyoung Shim wrote:
> With atomic modesetting all the control for CRTC, Planes, Encoders and
> Connectors should come from DRM core, so the driver is not allowed to
> enable or disable planes from inside the crtc_enable()/disable() call.
>
> But it needs to disable planes with crtc_disable in exynos hw driver
> internally. Because crtc is disabled before plane is disabled, it means
> plane_disable just returns without any register changes, then we cannot
> be sure setting register to disable plane when crtc is disable.
>
> This patch removes this chained calls to enable plane from exynos hw
> drivers code letting only DRM core touch planes except to disable plane.
> Also it leads eliminable enabled and resume of struct exynos_drm_plane.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Signed-off-by: Joonyoung Shim <jy0922.shim at samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski at samsung.com>
> ---
> This patch was modified from original patch of Gustavo Padovan -
> http://www.spinics.net/lists/linux-samsung-soc/msg45351.html
>
> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 63 +++---------------------------
> drivers/gpu/drm/exynos/exynos_drm_drv.h | 5 ---
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 63 +++---------------------------
> drivers/gpu/drm/exynos/exynos_drm_vidi.c | 27 -------------
> drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++----------------
> 5 files changed, 18 insertions(+), 181 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index d659ba2..f73ceae 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -410,11 +410,8 @@ static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>
> plane = &ctx->planes[win];
>
> - /* If suspended, enable this on resume */
> - if (ctx->suspended) {
> - plane->resume = true;
> + if (ctx->suspended)
> return;
> - }
>
> /*
> * SHADOWCON/PRTCON register is used for enabling timing.
> @@ -506,8 +503,6 @@ static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
> val = readl(ctx->regs + DECON_UPDATE);
> val |= DECON_UPDATE_STANDALONE_F;
> writel(val, ctx->regs + DECON_UPDATE);
> -
> - plane->enabled = true;
> }
>
> static void decon_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
> @@ -521,11 +516,8 @@ static void decon_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>
> plane = &ctx->planes[win];
>
> - if (ctx->suspended) {
> - /* do not resume this window*/
> - plane->resume = false;
> + if (ctx->suspended)
> return;
> - }
>
> /* protect windows */
> decon_shadow_protect_win(ctx, win, true);
> @@ -541,49 +533,6 @@ static void decon_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
> val = readl(ctx->regs + DECON_UPDATE);
> val |= DECON_UPDATE_STANDALONE_F;
> writel(val, ctx->regs + DECON_UPDATE);
> -
> - plane->enabled = false;
> -}
> -
> -static void decon_window_suspend(struct decon_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->resume = plane->enabled;
> - if (plane->enabled)
> - decon_win_disable(ctx->crtc, i);
> - }
> -}
> -
> -static void decon_window_resume(struct decon_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->enabled = plane->resume;
> - plane->resume = false;
> - }
> -}
> -
> -static void decon_apply(struct decon_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - if (plane->enabled)
> - decon_win_commit(ctx->crtc, i);
> - else
> - decon_win_disable(ctx->crtc, i);
> - }
> -
> - decon_commit(ctx->crtc);
> }
>
> static void decon_init(struct decon_context *ctx)
> @@ -625,14 +574,13 @@ static void decon_enable(struct exynos_drm_crtc *crtc)
> if (test_and_clear_bit(0, &ctx->irq_flags))
> decon_enable_vblank(ctx->crtc);
>
> - decon_window_resume(ctx);
> -
> - decon_apply(ctx);
> + decon_commit(ctx->crtc);
> }
>
> static void decon_disable(struct exynos_drm_crtc *crtc)
> {
> struct decon_context *ctx = crtc->ctx;
> + int i;
>
> if (ctx->suspended)
> return;
> @@ -642,7 +590,8 @@ static void decon_disable(struct exynos_drm_crtc *crtc)
> * suspend that connector. Otherwise we might try to scan from
> * a destroyed buffer later.
> */
> - decon_window_suspend(ctx);
> + for (i = 0; i < WINDOWS_NR; i++)
> + decon_win_disable(crtc, i);
>
> clk_disable_unprepare(ctx->vclk);
> clk_disable_unprepare(ctx->eclk);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 1c66f65..291f130 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -71,8 +71,6 @@ enum exynos_drm_output_type {
> * @dma_addr: array of bus(accessed by dma) address to the memory region
> * allocated for a overlay.
> * @zpos: order of overlay layer(z position).
> - * @enabled: enabled or not.
> - * @resume: to resume or not.
> *
> * this structure is common to exynos SoC and its contents would be copied
> * to hardware specific overlay info.
> @@ -101,9 +99,6 @@ struct exynos_drm_plane {
> uint32_t pixel_format;
> dma_addr_t dma_addr[MAX_FB_BUFFER];
> unsigned int zpos;
> -
> - bool enabled:1;
> - bool resume:1;
> };
>
> /*
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9661853..b45e09b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -634,11 +634,8 @@ static void fimd_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>
> plane = &ctx->planes[win];
>
> - /* If suspended, enable this on resume */
> - if (ctx->suspended) {
> - plane->resume = true;
> + if (ctx->suspended)
> return;
> - }
>
> /*
> * SHADOWCON/PRTCON register is used for enabling timing.
> @@ -728,8 +725,6 @@ static void fimd_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
> /* Enable DMA channel and unprotect windows */
> fimd_shadow_protect_win(ctx, win, false);
>
> - plane->enabled = true;
> -
> if (ctx->i80_if)
> atomic_set(&ctx->win_updated, 1);
> }
> @@ -744,11 +739,8 @@ static void fimd_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>
> plane = &ctx->planes[win];
>
> - if (ctx->suspended) {
> - /* do not resume this window*/
> - plane->resume = false;
> + if (ctx->suspended)
> return;
> - }
>
> /* protect windows */
> fimd_shadow_protect_win(ctx, win, true);
> @@ -760,49 +752,6 @@ static void fimd_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>
> /* unprotect windows */
> fimd_shadow_protect_win(ctx, win, false);
> -
> - plane->enabled = false;
> -}
> -
> -static void fimd_window_suspend(struct fimd_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->resume = plane->enabled;
> - if (plane->enabled)
> - fimd_win_disable(ctx->crtc, i);
> - }
> -}
> -
> -static void fimd_window_resume(struct fimd_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->enabled = plane->resume;
> - plane->resume = false;
> - }
> -}
> -
> -static void fimd_apply(struct fimd_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - if (plane->enabled)
> - fimd_win_commit(ctx->crtc, i);
> - else
> - fimd_win_disable(ctx->crtc, i);
> - }
> -
> - fimd_commit(ctx->crtc);
> }
>
> static void fimd_enable(struct exynos_drm_crtc *crtc)
> @@ -823,14 +772,13 @@ static void fimd_enable(struct exynos_drm_crtc *crtc)
> if (test_and_clear_bit(0, &ctx->irq_flags))
> fimd_enable_vblank(ctx->crtc);
>
> - fimd_window_resume(ctx);
> -
> - fimd_apply(ctx);
> + fimd_commit(ctx->crtc);
> }
>
> static void fimd_disable(struct exynos_drm_crtc *crtc)
> {
> struct fimd_context *ctx = crtc->ctx;
> + int i;
>
> if (ctx->suspended)
> return;
> @@ -840,7 +788,8 @@ static void fimd_disable(struct exynos_drm_crtc *crtc)
> * suspend that connector. Otherwise we might try to scan from
> * a destroyed buffer later.
> */
> - fimd_window_suspend(ctx);
> + for (i = 0; i < WINDOWS_NR; i++)
> + fimd_win_disable(crtc, i);
>
> clk_disable_unprepare(ctx->lcd_clk);
> clk_disable_unprepare(ctx->bus_clk);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index abe4ee0..e55174f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -131,33 +131,15 @@ static void vidi_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>
> plane = &ctx->planes[win];
>
> - plane->enabled = true;
> -
> DRM_DEBUG_KMS("dma_addr = %pad\n", plane->dma_addr);
>
> if (ctx->vblank_on)
> schedule_work(&ctx->work);
> }
>
> -static void vidi_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
> -{
> - struct vidi_context *ctx = crtc->ctx;
> - struct exynos_drm_plane *plane;
> -
> - if (win < 0 || win >= WINDOWS_NR)
> - return;
> -
> - plane = &ctx->planes[win];
> - plane->enabled = false;
> -
> - /* TODO. */
> -}
> -
> static void vidi_enable(struct exynos_drm_crtc *crtc)
> {
> struct vidi_context *ctx = crtc->ctx;
> - struct exynos_drm_plane *plane;
> - int i;
>
> mutex_lock(&ctx->lock);
>
> @@ -167,20 +149,12 @@ static void vidi_enable(struct exynos_drm_crtc *crtc)
> if (test_and_clear_bit(0, &ctx->irq_flags))
> vidi_enable_vblank(ctx->crtc);
>
> - for (i = 0; i < WINDOWS_NR; i++) {
> - plane = &ctx->planes[i];
> - if (plane->enabled)
> - vidi_win_commit(ctx->crtc, i);
> - }
> -
> mutex_unlock(&ctx->lock);
> }
>
> static void vidi_disable(struct exynos_drm_crtc *crtc)
> {
> struct vidi_context *ctx = crtc->ctx;
> - struct exynos_drm_plane *plane;
> - int i;
>
> mutex_lock(&ctx->lock);
>
> @@ -206,7 +180,6 @@ static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
> .enable_vblank = vidi_enable_vblank,
> .disable_vblank = vidi_disable_vblank,
> .win_commit = vidi_win_commit,
> - .win_disable = vidi_win_disable,
> };
>
> static void vidi_fake_vblank_handler(struct work_struct *work)
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 6bab717..fd88ae1 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -937,8 +937,6 @@ static void mixer_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
> vp_video_buffer(mixer_ctx, win);
> else
> mixer_graph_buffer(mixer_ctx, win);
> -
> - mixer_ctx->planes[win].enabled = true;
> }
>
> static void mixer_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
> @@ -952,7 +950,6 @@ static void mixer_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
> mutex_lock(&mixer_ctx->mixer_mutex);
> if (!mixer_ctx->powered) {
> mutex_unlock(&mixer_ctx->mixer_mutex);
> - mixer_ctx->planes[win].resume = false;
> return;
> }
> mutex_unlock(&mixer_ctx->mixer_mutex);
> @@ -964,8 +961,6 @@ static void mixer_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>
> mixer_vsync_set_update(mixer_ctx, true);
> spin_unlock_irqrestore(&res->reg_slock, flags);
> -
> - mixer_ctx->planes[win].enabled = false;
> }
>
> static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
> @@ -1000,33 +995,6 @@ static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
> drm_vblank_put(mixer_ctx->drm_dev, mixer_ctx->pipe);
> }
>
> -static void mixer_window_suspend(struct mixer_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < MIXER_WIN_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->resume = plane->enabled;
> - mixer_win_disable(ctx->crtc, i);
> - }
> - mixer_wait_for_vblank(ctx->crtc);
> -}
> -
> -static void mixer_window_resume(struct mixer_context *ctx)
> -{
> - struct exynos_drm_plane *plane;
> - int i;
> -
> - for (i = 0; i < MIXER_WIN_NR; i++) {
> - plane = &ctx->planes[i];
> - plane->enabled = plane->resume;
> - plane->resume = false;
> - if (plane->enabled)
> - mixer_win_commit(ctx->crtc, i);
> - }
> -}
> -
> static void mixer_enable(struct exynos_drm_crtc *crtc)
> {
> struct mixer_context *ctx = crtc->ctx;
> @@ -1058,14 +1026,13 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
>
> mixer_reg_write(res, MXR_INT_EN, ctx->int_en);
> mixer_win_reset(ctx);
> -
> - mixer_window_resume(ctx);
> }
>
> static void mixer_disable(struct exynos_drm_crtc *crtc)
> {
> struct mixer_context *ctx = crtc->ctx;
> struct mixer_resources *res = &ctx->mixer_res;
> + int i;
>
> mutex_lock(&ctx->mixer_mutex);
> if (!ctx->powered) {
> @@ -1076,7 +1043,11 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
>
> mixer_stop(ctx);
> mixer_regs_dump(ctx);
> - mixer_window_suspend(ctx);
> +
> + for (i = 0; i < MIXER_WIN_NR; i++)
> + mixer_win_disable(crtc, i);
> +
> + mixer_wait_for_vblank(ctx->crtc);
>
> ctx->int_en = mixer_reg_read(res, MXR_INT_EN);
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
More information about the dri-devel
mailing list