Hi Inki,
Thank you for the patch.
On Monday 16 Jan 2017 18:13:22 Inki Dae wrote:
> This patch relpaces specific atomic commit function
> with atomic helper commit one, which also includes
> atomic_commit_tail callback for Exynos SoC becasue
> crtc devices on Exynos SoC uses power domain device
> so drm_atomic_helper_commit_planes should be called
> after drm_atomic_helper_commit_modeset_enables.
>
> Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 9 ++-
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 110 +---------------------------
> drivers/gpu/drm/exynos/exynos_drm_fb.c | 25 ++++++-
> 3 files changed, 33 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index 2530bf5..47da612 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -39,6 +39,14 @@ static void exynos_drm_crtc_disable(struct drm_crtc
> *crtc)
>
> if (exynos_crtc->ops->disable)
> exynos_crtc->ops->disable(exynos_crtc);
> +
> + if (crtc->state->event && !crtc->state->active) {
> + spin_lock_irq(&crtc->dev->event_lock);
> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
> + spin_unlock_irq(&crtc->dev->event_lock);
> +
> + crtc->state->event = NULL;
> + }
You also need to handle events for exynos_drm_crtc_enable(). I'm not too
familiar with the exynos drm driver, is that taken care of by event handling
in exynos_crtc_atomic_flush() ?
You could also split this change into a separate patch with a clear
explanation of why it's needed in the commit message.
> }
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 3ec0535..9d0df00 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
[snip]
> -static void exynos_atomic_commit_complete(struct exynos_atomic_commit
> *commit)
> -{
> - struct drm_device *dev = commit->dev;
> - struct exynos_drm_private *priv = dev->dev_private;
> - struct drm_atomic_state *state = commit->state;
> -
> - drm_atomic_helper_commit_modeset_disables(dev, state);
> -
> - drm_atomic_helper_commit_modeset_enables(dev, state);
> -
> - /*
> - * Exynos can't update planes with CRTCs and encoders disabled,
> - * its updates routines, specially for FIMD, requires the clocks
> - * to be enabled. So it is necessary to handle the modeset operations
> - * *before* the commit_planes() step, this way it will always
> - * have the relevant clocks enabled to perform the update.
> - */
There's a value in this comment, I would copy it to
exynos_drm_atomic_commit_tail()
> - drm_atomic_helper_commit_planes(dev, state, 0);
> -
> - drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> - drm_atomic_helper_cleanup_planes(dev, state);
> -
> - drm_atomic_state_put(state);
> -
> - spin_lock(&priv->lock);
> - priv->pending &= ~commit->crtcs;
> - spin_unlock(&priv->lock);
> -
> - wake_up_all(&priv->wait);
> -
> - kfree(commit);
> -}
[snip]
> @@ -313,6 +204,7 @@ static void exynos_drm_preclose(struct drm_device *dev,
>
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> exynos_drm_crtc_cancel_page_flip(crtc, file);
> +
This isn't needed.
> }
>
> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file
> *file) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 68d4142..1e10b73 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,11 +187,33 @@ dma_addr_t exynos_drm_fb_dma_addr(struct
> drm_framebuffer *fb, int index) return exynos_fb->dma_addr[index];
> }
>
> +static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
> +{
> + struct drm_device *dev = state->dev;
> +
> + drm_atomic_helper_commit_modeset_disables(dev, state);
> +
> + drm_atomic_helper_commit_modeset_enables(dev, state);
> +
> + drm_atomic_helper_commit_planes(dev, state,
> + DRM_PLANE_COMMIT_ACTIVE_ONLY);
The DRM_PLANE_COMMIT_ACTIVE_ONLY flag wasn't set before, I think this change
should go in a separate patch (or at least be documented in the commit
message).
> + drm_atomic_helper_commit_hw_done(state);
> +
> + drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> + drm_atomic_helper_cleanup_planes(dev, state);
> +}
[snip]
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
