Re: [PATCH 10/10] drm/exynos: atomic dpms support

[Joonyoung Shim <jy0922.shim@samsung.com>]

Hi,

On 03/31/2015 04:11 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Run dpms operations through the atomic intefaces. This basically removes
> the .dpms() callback from econders and crtcs and use .disable() and
> .enable() to turn the crtc on and off.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 99 ++++++++++++++++-------------
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c   |  3 -
>  drivers/gpu/drm/exynos/exynos_drm_plane.c   |  1 -
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  2 +-
>  10 files changed, 67 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 6704d5c..e030d16 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -949,7 +949,7 @@ static void exynos_dp_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs exynos_dp_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.detect = exynos_dp_detect,
>  	.destroy = exynos_dp_connector_destroy,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 0db7b91..b493993 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -22,51 +22,57 @@
>  #include "exynos_drm_encoder.h"
>  #include "exynos_drm_plane.h"
>  
> -static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
> +static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
>  
> -	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
> -
> -	if (exynos_crtc->dpms == mode) {
> -		DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
> +	if (WARN_ON(exynos_crtc->enabled))

I'm not sure this is really important condition to report warning.

>  		return;
> -	}
> -
> -	if (mode > DRM_MODE_DPMS_ON) {
> -		/* wait for the completion of page flip. */
> -		if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
> -				(exynos_crtc->event == NULL), HZ/20))
> -			exynos_crtc->event = NULL;
> -		drm_crtc_vblank_off(crtc);
> -	}
>  
>  	if (exynos_crtc->ops->dpms)
> -		exynos_crtc->ops->dpms(exynos_crtc, mode);
> +		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_ON);
>  
> -	exynos_crtc->dpms = mode;
> +	exynos_crtc->enabled = true;
>  
> -	if (mode == DRM_MODE_DPMS_ON)
> -		drm_crtc_vblank_on(crtc);
> -}
> +	drm_crtc_vblank_on(crtc);
>  
> -static void exynos_drm_crtc_prepare(struct drm_crtc *crtc)
> -{
> -	/* drm framework doesn't check NULL. */
> +	if (exynos_crtc->ops->win_commit)
> +		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> +
> +	if (exynos_crtc->ops->commit)
> +		exynos_crtc->ops->commit(exynos_crtc);
>  }
>  
> -static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
> +static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> -	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
> +	struct drm_plane *plane;
> +	int ret;
> +
> +	if (WARN_ON(!exynos_crtc->enabled))
> +		return;
>  
> -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> +	/* wait for the completion of page flip. */
> +	if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
> +				(exynos_crtc->event == NULL), HZ/20))
> +		exynos_crtc->event = NULL;
>  
> -	if (exynos_crtc->ops->win_commit)
> -		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> +	drm_crtc_vblank_off(crtc);
>  
> -	if (exynos_crtc->ops->commit)
> -		exynos_crtc->ops->commit(exynos_crtc);
> +	if (exynos_crtc->ops->dpms)
> +		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_OFF);
> +
> +	exynos_crtc->enabled = false;
> +
> +	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
> +		if (plane->crtc != crtc)
> +			continue;
> +
> +		ret = plane->funcs->disable_plane(plane);
> +		if (ret)
> +			DRM_ERROR("Failed to disable plane %d\n", ret);
> +	}
>  }
>  
>  static bool
> @@ -95,32 +101,36 @@ exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  		exynos_crtc->ops->commit(exynos_crtc);
>  }
>  
> -static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> +static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *state)
>  {
> -	struct drm_plane *plane;
> +	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>  	int ret;
>  
> -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
> +	if (exynos_crtc->event)
> +		return -EBUSY;
>  
> -	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
> -		if (plane->crtc != crtc)
> -			continue;
> +	if (state->event) {
> +		ret = drm_vblank_get(crtc->dev, exynos_crtc->pipe);
> +		if (ret) {
> +			DRM_ERROR("failed to acquire vblank counter\n");
> +			return ret;
> +		}
>  
> -		ret = plane->funcs->disable_plane(plane);
> -		if (ret)
> -			DRM_ERROR("Failed to disable plane %d\n", ret);
> +		exynos_crtc->event = state->event;
>  	}
> +
> +	return 0;
>  }
>  
>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> -	.dpms		= exynos_drm_crtc_dpms,
> -	.prepare	= exynos_drm_crtc_prepare,
> -	.commit		= exynos_drm_crtc_commit,
> +	.enable		= exynos_drm_crtc_enable,
> +	.disable	= exynos_drm_crtc_disable,

By this change, funcs->dpms should be changed to funcs->disable on
hdmi_dpms function of drivers/gpu/drm/exynos/exynos_hdmi.c or any idea?

	case DRM_MODE_DPMS_OFF:
		/*
		 * The SFRs of VP and Mixer are updated by Vertical Sync of
		 * Timing generator which is a part of HDMI so the sequence
		 * to disable TV Subsystem should be as following,
		 *	VP -> Mixer -> HDMI
		 *
		 * Below codes will try to disable Mixer and VP(if used)
		 * prior to disabling HDMI.
		 */
		if (crtc)
			funcs = crtc->helper_private;
		if (funcs && funcs->dpms)
			(*funcs->dpms)(crtc, mode);

		hdmi_poweroff(hdata);
		break;

>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>  	.mode_set	= drm_helper_crtc_mode_set,
>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
> -	.disable	= exynos_drm_crtc_disable,
> +	.atomic_check	= exynos_crtc_atomic_check,
>  };
>  
>  static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
> @@ -162,7 +172,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
>  
>  	init_waitqueue_head(&exynos_crtc->pending_flip_queue);
>  
> -	exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
>  	exynos_crtc->pipe = pipe;
>  	exynos_crtc->type = type;
>  	exynos_crtc->ops = ops;
> @@ -193,7 +202,7 @@ int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>  	struct exynos_drm_crtc *exynos_crtc =
>  		to_exynos_crtc(private->crtc[pipe]);
>  
> -	if (exynos_crtc->dpms != DRM_MODE_DPMS_ON)
> +	if (!exynos_crtc->enabled)
>  		return -EPERM;
>  
>  	if (exynos_crtc->ops->enable_vblank)
> @@ -208,7 +217,7 @@ void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe)
>  	struct exynos_drm_crtc *exynos_crtc =
>  		to_exynos_crtc(private->crtc[pipe]);
>  
> -	if (exynos_crtc->dpms != DRM_MODE_DPMS_ON)
> +	if (!exynos_crtc->enabled)
>  		return;
>  
>  	if (exynos_crtc->ops->disable_vblank)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> index ced5c23..6dc328e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> @@ -60,7 +60,7 @@ static void exynos_dpi_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs exynos_dpi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = exynos_dpi_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = exynos_dpi_connector_destroy,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index a1013aa..a7b708e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -205,7 +205,7 @@ struct exynos_drm_crtc_ops {
>   *	drm framework doesn't support multiple irq yet.
>   *	we can refer to the crtc to current hardware interrupt occurred through
>   *	this pipe value.
> - * @dpms: store the crtc dpms value
> + * @enabled: if the crtc is enabled or not
>   * @event: vblank event that is currently queued for flip
>   * @ops: pointer to callbacks for exynos drm specific functionality
>   * @ctx: A pointer to the crtc's implementation specific context
> @@ -214,7 +214,7 @@ struct exynos_drm_crtc {
>  	struct drm_crtc			base;
>  	enum exynos_drm_output_type	type;
>  	unsigned int			pipe;
> -	unsigned int			dpms;
> +	bool				enabled;
>  	wait_queue_head_t		pending_flip_queue;
>  	struct drm_pending_vblank_event	*event;
>  	struct exynos_drm_crtc_ops	*ops;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 6e9c2c3..28233a5 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1458,7 +1458,7 @@ static void exynos_dsi_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs exynos_dsi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = exynos_dsi_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = exynos_dsi_connector_destroy,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> index 57de0bd..0648ba4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> @@ -32,17 +32,6 @@ struct exynos_drm_encoder {
>  	struct exynos_drm_display	*display;
>  };
>  
> -static void exynos_drm_encoder_dpms(struct drm_encoder *encoder, int mode)
> -{
> -	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
> -	struct exynos_drm_display *display = exynos_encoder->display;
> -
> -	DRM_DEBUG_KMS("encoder dpms: %d\n", mode);
> -
> -	if (display->ops->dpms)
> -		display->ops->dpms(display, mode);
> -}
> -
>  static bool
>  exynos_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>  			       const struct drm_display_mode *mode,
> @@ -76,12 +65,7 @@ static void exynos_drm_encoder_mode_set(struct drm_encoder *encoder,
>  		display->ops->mode_set(display, adjusted_mode);
>  }
>  
> -static void exynos_drm_encoder_prepare(struct drm_encoder *encoder)
> -{
> -	/* drm framework doesn't check NULL. */
> -}

This function is related with below flow on disable_outputs fuction of
drivers/gpu/drm/drm_atomic_helper.c

		/* Right function depends upon target state. */
		if (connector->state->crtc && funcs->prepare)
			funcs->prepare(encoder);
		else if (funcs->disable)
			funcs->disable(encoder);
		else
			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);

I'm not sure it's right to call funcs->disable instead of
funcs->prepare.

> -
> -static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
> +static void exynos_drm_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
>  	struct exynos_drm_display *display = exynos_encoder->display;
> @@ -95,10 +79,13 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
>  
>  static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
>  {
> +	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
> +	struct exynos_drm_display *display = exynos_encoder->display;
>  	struct drm_plane *plane;
>  	struct drm_device *dev = encoder->dev;
>  
> -	exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
> +	if (display->ops->dpms)
> +		display->ops->dpms(display, DRM_MODE_DPMS_OFF);
>  
>  	/* all planes connected to this encoder should be also disabled. */
>  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> @@ -108,11 +95,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
>  }
>  
>  static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
> -	.dpms		= exynos_drm_encoder_dpms,
>  	.mode_fixup	= exynos_drm_encoder_mode_fixup,
>  	.mode_set	= exynos_drm_encoder_mode_set,
> -	.prepare	= exynos_drm_encoder_prepare,
> -	.commit		= exynos_drm_encoder_commit,
> +	.enable		= exynos_drm_encoder_enable,
>  	.disable	= exynos_drm_encoder_disable,
>  };
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index e71e331..e0b085b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -275,9 +275,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>  
>  	}
>  
> -	/* disable all the possible outputs/crtcs before entering KMS mode */
> -	drm_helper_disable_unused_functions(dev);
> -

I think this is not related with atomic dpms support, if this needs,
could you make another patch?

>  	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
>  	if (ret < 0) {
>  		DRM_ERROR("failed to set up hw configuration.\n");
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 7f8f962..5d3066d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -131,7 +131,6 @@ static int exynos_plane_atomic_check(struct drm_plane *plane,
>  				     struct drm_plane_state *state)
>  {
>  	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> -	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(plane->crtc);
>  	int nr;
>  	int i;
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 6eddfc0..f6cad75 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -385,7 +385,7 @@ static void vidi_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs vidi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.detect = vidi_detect,
>  	.destroy = vidi_connector_destroy,
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 5b597bc..d395da0 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1051,7 +1051,7 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static struct drm_connector_funcs hdmi_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.detect = hdmi_detect,
>  	.destroy = hdmi_connector_destroy,
> 

Thanks.