Re: [PATCH] drm/exynos: use atomic helper commit

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@samsung.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel