[PATCH v10 00/17] drm/exynos: atomic modesetting support

[PATCH v10 00/17] drm/exynos: atomic modesetting support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

Here goes the full support for atomic modesetting on exynos. I've
split the patches in the various phases of atomic support.

v2: fixes comments by Joonyoung
        - remove unused var in patch 09
        - use ->disable instead of outdated ->dpms in hdmi code
        - remove WARN_ON from crtc enable/disable

v3: fixes comment by Joonyoung
        - move the removal of drm_helper_disable_unused_functions() to
        separated patch

v4: add patches that remove unnecessary calls to disable_plane()

v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)

v6: rebase on latest exynos_drm_next

v7: fix comments by Joonyoung
        - fix two checkpatch errors
        - remove extra crtc->commit() call
        - check for null fb on exynos_check_plane()

v8: fix comments by Joonyoung
        - fix merge error
        - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
        - remove .prepare() in the apropiated patch
        - add a new patch to move exynos_drm_crtc_disable()

v9:  * fix comments by Joonyoung
        - also remove encoder .prepare()
        - do not shift exynos_update_plane() parameters
        - remove unused .mode_set() and .mode_set_base()
     * add specific exynos .atomic_commit()
     * add split of exynos_crtc->ops->dpms() into enable() and disable()
     * add other atomic clean ups

v10: * fix comments by Joonyoung
	- add more comments on exynos_atomic_commit()
	- make exynos_crtc's .enable and .disable void


	Gustavo

---
Gustavo Padovan (15):
  drm/exynos: atomic phase 1: use drm_plane_helper_update()
  drm/exynos: atomic phase 1: use drm_plane_helper_disable()
  drm/exynos: atomic phase 1: add .mode_set_nofb() callback
  drm/exynos: atomic phase 2: wire up state reset(), duplicate() and
    destroy()
  drm/exynos: atomic phase 2: keep track of framebuffer pointer
  drm/exynos: atomic phase 3: atomic updates of planes
  drm/exynos: atomic phase 3: use atomic .set_config helper
  drm/exynos: atomic phase 3: convert page flips
  drm/exynos: remove exported functions from exynos_drm_plane
  drm/exynos: don't disable unused functions at init
  drm/exynos: move exynos_drm_crtc_disable()
  drm/exynos: add exynos specific .atomic_commit()
  drm/exynos: atomic dpms support
  drm/exynos: remove unnecessary calls to disable_plane()
  drm/exynos: split exynos_crtc->dpms in enable() and disable()

Joonyoung Shim (2):
  drm/exynos: fix source data argument for plane
  drm/exynos: use adjusted_mode of crtc_state instead of mode

 drivers/gpu/drm/bridge/ps8622.c             |   6 +-
 drivers/gpu/drm/bridge/ptn3460.c            |   6 +-
 drivers/gpu/drm/exynos/exynos7_drm_decon.c  |  91 +++----------
 drivers/gpu/drm/exynos/exynos_dp_core.c     |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 201 ++++++----------------------
 drivers/gpu/drm/exynos/exynos_drm_dpi.c     |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c     |   2 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |  10 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c     |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_encoder.c |  35 +----
 drivers/gpu/drm/exynos/exynos_drm_fb.c      |  41 ++++++
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c   |   3 -
 drivers/gpu/drm/exynos/exynos_drm_fimd.c    |  71 +++-------
 drivers/gpu/drm/exynos/exynos_drm_plane.c   | 137 ++++++++++---------
 drivers/gpu/drm/exynos/exynos_drm_plane.h   |  11 --
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  59 ++++----
 drivers/gpu/drm/exynos/exynos_hdmi.c        |  10 +-
 drivers/gpu/drm/exynos/exynos_mixer.c       |  26 +---
 18 files changed, 269 insertions(+), 458 deletions(-)

-- 
2.1.0

[PATCH v10 01/17] drm/exynos: fix source data argument for plane

[Gustavo Padovan]

From: Joonyoung Shim <jy0922.shim@samsung.com>

The exynos_update_plane function needs 16.16 fixed point source data.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 9006b94..363b019 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -127,7 +127,8 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	crtc_h = fb->height - y;
 
 	return exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-				   crtc_w, crtc_h, x, y, crtc_w, crtc_h);
+				   crtc_w, crtc_h, x << 16, y << 16,
+				   crtc_w << 16, crtc_h << 16);
 }
 
 static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
@@ -202,8 +203,8 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 	crtc_w = fb->width - crtc->x;
 	crtc_h = fb->height - crtc->y;
 	ret = exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-				  crtc_w, crtc_h, crtc->x, crtc->y,
-				  crtc_w, crtc_h);
+				  crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
+				  crtc_w << 16, crtc_h << 16);
 	if (ret) {
 		crtc->primary->fb = old_fb;
 		spin_lock_irq(&dev->event_lock);
-- 
2.1.0

[PATCH v10 02/17] drm/exynos: use adjusted_mode of crtc_state instead of mode

[Gustavo Padovan]

From: Joonyoung Shim <jy0922.shim@samsung.com>

Handle changes by removing copy from adjusted_mode to mode as using
adjusted_mode of crtc_state.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c |  4 ++--
 drivers/gpu/drm/exynos/exynos_drm_fimd.c   |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_plane.c  | 13 +++++++------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 6714e5b..f29e4be 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -175,7 +175,7 @@ static bool decon_mode_fixup(struct exynos_drm_crtc *crtc,
 static void decon_commit(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
-	struct drm_display_mode *mode = &crtc->base.mode;
+	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
 	u32 val, clkdiv;
 
 	if (ctx->suspended)
@@ -395,7 +395,7 @@ static void decon_shadow_protect_win(struct decon_context *ctx,
 static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
 {
 	struct decon_context *ctx = crtc->ctx;
-	struct drm_display_mode *mode = &crtc->base.mode;
+	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
 	struct exynos_drm_plane *plane;
 	int padding;
 	unsigned long val, alpha;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a0edab8..b326b31 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -337,7 +337,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc *crtc,
 static void fimd_commit(struct exynos_drm_crtc *crtc)
 {
 	struct fimd_context *ctx = crtc->ctx;
-	struct drm_display_mode *mode = &crtc->base.mode;
+	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
 	struct fimd_driver_data *driver_data = ctx->driver_data;
 	void *timing_base = ctx->regs + driver_data->timing_base;
 	u32 val, clkdiv;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index b1180fb..01d2918 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -92,11 +92,12 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 			  uint32_t src_w, uint32_t src_h)
 {
 	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 	unsigned int actual_w;
 	unsigned int actual_h;
 
-	actual_w = exynos_plane_get_size(crtc_x, crtc_w, crtc->mode.hdisplay);
-	actual_h = exynos_plane_get_size(crtc_y, crtc_h, crtc->mode.vdisplay);
+	actual_w = exynos_plane_get_size(crtc_x, crtc_w, mode->hdisplay);
+	actual_h = exynos_plane_get_size(crtc_y, crtc_h, mode->vdisplay);
 
 	if (crtc_x < 0) {
 		if (actual_w)
@@ -132,10 +133,10 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 	exynos_plane->crtc_height = actual_h;
 
 	/* set drm mode data. */
-	exynos_plane->mode_width = crtc->mode.hdisplay;
-	exynos_plane->mode_height = crtc->mode.vdisplay;
-	exynos_plane->refresh = crtc->mode.vrefresh;
-	exynos_plane->scan_flag = crtc->mode.flags;
+	exynos_plane->mode_width = mode->hdisplay;
+	exynos_plane->mode_height = mode->vdisplay;
+	exynos_plane->refresh = mode->vrefresh;
+	exynos_plane->scan_flag = mode->flags;
 
 	DRM_DEBUG_KMS("plane : offset_x/y(%d,%d), width/height(%d,%d)",
 			exynos_plane->crtc_x, exynos_plane->crtc_y,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 02/17] drm/exynos: use adjusted_mode of crtc_state instead of mode

[Tobias Jakobi]

Hello,

as pointed out in [1] before, this gives me an kernel oops during boot.

With best wishes,
Tobias

[1] http://www.spinics.net/lists/linux-samsung-soc/msg44736.html


On 2015-06-01 17:04, Gustavo Padovan wrote:
> From: Joonyoung Shim <jy0922.shim@samsung.com>
> 
> Handle changes by removing copy from adjusted_mode to mode as using
> adjusted_mode of crtc_state.
> 
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 13 +++++++------
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index 6714e5b..f29e4be 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -175,7 +175,7 @@ static bool decon_mode_fixup(struct exynos_drm_crtc 
> *crtc,
>  static void decon_commit(struct exynos_drm_crtc *crtc)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> -	struct drm_display_mode *mode = &crtc->base.mode;
> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>  	u32 val, clkdiv;
> 
>  	if (ctx->suspended)
> @@ -395,7 +395,7 @@ static void decon_shadow_protect_win(struct
> decon_context *ctx,
>  static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned 
> int win)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> -	struct drm_display_mode *mode = &crtc->base.mode;
> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>  	struct exynos_drm_plane *plane;
>  	int padding;
>  	unsigned long val, alpha;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index a0edab8..b326b31 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -337,7 +337,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc 
> *crtc,
>  static void fimd_commit(struct exynos_drm_crtc *crtc)
>  {
>  	struct fimd_context *ctx = crtc->ctx;
> -	struct drm_display_mode *mode = &crtc->base.mode;
> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>  	struct fimd_driver_data *driver_data = ctx->driver_data;
>  	void *timing_base = ctx->regs + driver_data->timing_base;
>  	u32 val, clkdiv;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index b1180fb..01d2918 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -92,11 +92,12 @@ void exynos_plane_mode_set(struct drm_plane
> *plane, struct drm_crtc *crtc,
>  			  uint32_t src_w, uint32_t src_h)
>  {
>  	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>  	unsigned int actual_w;
>  	unsigned int actual_h;
> 
> -	actual_w = exynos_plane_get_size(crtc_x, crtc_w, 
> crtc->mode.hdisplay);
> -	actual_h = exynos_plane_get_size(crtc_y, crtc_h, 
> crtc->mode.vdisplay);
> +	actual_w = exynos_plane_get_size(crtc_x, crtc_w, mode->hdisplay);
> +	actual_h = exynos_plane_get_size(crtc_y, crtc_h, mode->vdisplay);
> 
>  	if (crtc_x < 0) {
>  		if (actual_w)
> @@ -132,10 +133,10 @@ void exynos_plane_mode_set(struct drm_plane
> *plane, struct drm_crtc *crtc,
>  	exynos_plane->crtc_height = actual_h;
> 
>  	/* set drm mode data. */
> -	exynos_plane->mode_width = crtc->mode.hdisplay;
> -	exynos_plane->mode_height = crtc->mode.vdisplay;
> -	exynos_plane->refresh = crtc->mode.vrefresh;
> -	exynos_plane->scan_flag = crtc->mode.flags;
> +	exynos_plane->mode_width = mode->hdisplay;
> +	exynos_plane->mode_height = mode->vdisplay;
> +	exynos_plane->refresh = mode->vrefresh;
> +	exynos_plane->scan_flag = mode->flags;
> 
>  	DRM_DEBUG_KMS("plane : offset_x/y(%d,%d), width/height(%d,%d)",
>  			exynos_plane->crtc_x, exynos_plane->crtc_y,

Re: [PATCH v10 02/17] drm/exynos: use adjusted_mode of crtc_state instead of mode

[Joonyoung Shim]

On 06/02/2015 12:09 AM, Tobias Jakobi wrote:
> Hello,
> 
> as pointed out in [1] before, this gives me an kernel oops during boot.
> 
> With best wishes,
> Tobias
> 
> [1] http://www.spinics.net/lists/linux-samsung-soc/msg44736.html
> 

Yeah, this patch should go after switched by drm_helper_crtc_mode_set,
e.g. after the patch "drm/exynos: atomic phase 1: add .mode_set_nofb()
callback". Then crtc->base.state cannot be NULL.

Gustavo, sorry for that, i meant rebase based on the patch "drm/exynos:
atomic phase 1: add .mode_set_nofb() callback"

Inki, i think it's time to merge this patchset for switch to atomic
modeset functions if no any objection, just need to rebase of this patch
based on the patch "drm/exynos: atomic phase 1 : add .mode_set_nofb()
callback" and except a patch 17/17 from this merge, i think clean up
patches like it need to more review after merge atomic patchset.

Thanks.

> 
> On 2015-06-01 17:04, Gustavo Padovan wrote:
>> From: Joonyoung Shim <jy0922.shim@samsung.com>
>>
>> Handle changes by removing copy from adjusted_mode to mode as using
>> adjusted_mode of crtc_state.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c |  4 ++--
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   |  2 +-
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 13 +++++++------
>>  3 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> index 6714e5b..f29e4be 100644
>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> @@ -175,7 +175,7 @@ static bool decon_mode_fixup(struct exynos_drm_crtc *crtc,
>>  static void decon_commit(struct exynos_drm_crtc *crtc)
>>  {
>>      struct decon_context *ctx = crtc->ctx;
>> -    struct drm_display_mode *mode = &crtc->base.mode;
>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>      u32 val, clkdiv;
>>
>>      if (ctx->suspended)
>> @@ -395,7 +395,7 @@ static void decon_shadow_protect_win(struct
>> decon_context *ctx,
>>  static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>>  {
>>      struct decon_context *ctx = crtc->ctx;
>> -    struct drm_display_mode *mode = &crtc->base.mode;
>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>      struct exynos_drm_plane *plane;
>>      int padding;
>>      unsigned long val, alpha;
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index a0edab8..b326b31 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -337,7 +337,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc *crtc,
>>  static void fimd_commit(struct exynos_drm_crtc *crtc)
>>  {
>>      struct fimd_context *ctx = crtc->ctx;
>> -    struct drm_display_mode *mode = &crtc->base.mode;
>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>      struct fimd_driver_data *driver_data = ctx->driver_data;
>>      void *timing_base = ctx->regs + driver_data->timing_base;
>>      u32 val, clkdiv;
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index b1180fb..01d2918 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -92,11 +92,12 @@ void exynos_plane_mode_set(struct drm_plane
>> *plane, struct drm_crtc *crtc,
>>                uint32_t src_w, uint32_t src_h)
>>  {
>>      struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>> +    struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>>      unsigned int actual_w;
>>      unsigned int actual_h;
>>
>> -    actual_w = exynos_plane_get_size(crtc_x, crtc_w, crtc->mode.hdisplay);
>> -    actual_h = exynos_plane_get_size(crtc_y, crtc_h, crtc->mode.vdisplay);
>> +    actual_w = exynos_plane_get_size(crtc_x, crtc_w, mode->hdisplay);
>> +    actual_h = exynos_plane_get_size(crtc_y, crtc_h, mode->vdisplay);
>>
>>      if (crtc_x < 0) {
>>          if (actual_w)
>> @@ -132,10 +133,10 @@ void exynos_plane_mode_set(struct drm_plane
>> *plane, struct drm_crtc *crtc,
>>      exynos_plane->crtc_height = actual_h;
>>
>>      /* set drm mode data. */
>> -    exynos_plane->mode_width = crtc->mode.hdisplay;
>> -    exynos_plane->mode_height = crtc->mode.vdisplay;
>> -    exynos_plane->refresh = crtc->mode.vrefresh;
>> -    exynos_plane->scan_flag = crtc->mode.flags;
>> +    exynos_plane->mode_width = mode->hdisplay;
>> +    exynos_plane->mode_height = mode->vdisplay;
>> +    exynos_plane->refresh = mode->vrefresh;
>> +    exynos_plane->scan_flag = mode->flags;
>>
>>      DRM_DEBUG_KMS("plane : offset_x/y(%d,%d), width/height(%d,%d)",
>>              exynos_plane->crtc_x, exynos_plane->crtc_y,
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v10 02/17] drm/exynos: use adjusted_mode of crtc_state instead of mode

[Inki Dae]

On 2015년 06월 02일 09:03, Joonyoung Shim wrote:
> On 06/02/2015 12:09 AM, Tobias Jakobi wrote:
>> Hello,
>>
>> as pointed out in [1] before, this gives me an kernel oops during boot.
>>
>> With best wishes,
>> Tobias
>>
>> [1] http://www.spinics.net/lists/linux-samsung-soc/msg44736.html
>>
> 
> Yeah, this patch should go after switched by drm_helper_crtc_mode_set,
> e.g. after the patch "drm/exynos: atomic phase 1: add .mode_set_nofb()
> callback". Then crtc->base.state cannot be NULL.
> 
> Gustavo, sorry for that, i meant rebase based on the patch "drm/exynos:
> atomic phase 1: add .mode_set_nofb() callback"
> 
> Inki, i think it's time to merge this patchset for switch to atomic
> modeset functions if no any objection, just need to rebase of this patch
> based on the patch "drm/exynos: atomic phase 1 : add .mode_set_nofb()
> callback" and except a patch 17/17 from this merge, i think clean up
> patches like it need to more review after merge atomic patchset.

Thanks Joonyoung. Got it. I will merge this patch series to my local
repository for test and final review. If there is no any big problem,
will merge it.

Thanks,
Inki Dae

> 
> Thanks.
> 
>>
>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>> From: Joonyoung Shim <jy0922.shim@samsung.com>
>>>
>>> Handle changes by removing copy from adjusted_mode to mode as using
>>> adjusted_mode of crtc_state.
>>>
>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c |  4 ++--
>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   |  2 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 13 +++++++------
>>>  3 files changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> index 6714e5b..f29e4be 100644
>>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>> @@ -175,7 +175,7 @@ static bool decon_mode_fixup(struct exynos_drm_crtc *crtc,
>>>  static void decon_commit(struct exynos_drm_crtc *crtc)
>>>  {
>>>      struct decon_context *ctx = crtc->ctx;
>>> -    struct drm_display_mode *mode = &crtc->base.mode;
>>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>>      u32 val, clkdiv;
>>>
>>>      if (ctx->suspended)
>>> @@ -395,7 +395,7 @@ static void decon_shadow_protect_win(struct
>>> decon_context *ctx,
>>>  static void decon_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>>>  {
>>>      struct decon_context *ctx = crtc->ctx;
>>> -    struct drm_display_mode *mode = &crtc->base.mode;
>>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>>      struct exynos_drm_plane *plane;
>>>      int padding;
>>>      unsigned long val, alpha;
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index a0edab8..b326b31 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -337,7 +337,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc *crtc,
>>>  static void fimd_commit(struct exynos_drm_crtc *crtc)
>>>  {
>>>      struct fimd_context *ctx = crtc->ctx;
>>> -    struct drm_display_mode *mode = &crtc->base.mode;
>>> +    struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
>>>      struct fimd_driver_data *driver_data = ctx->driver_data;
>>>      void *timing_base = ctx->regs + driver_data->timing_base;
>>>      u32 val, clkdiv;
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> index b1180fb..01d2918 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> @@ -92,11 +92,12 @@ void exynos_plane_mode_set(struct drm_plane
>>> *plane, struct drm_crtc *crtc,
>>>                uint32_t src_w, uint32_t src_h)
>>>  {
>>>      struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>>> +    struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>>>      unsigned int actual_w;
>>>      unsigned int actual_h;
>>>
>>> -    actual_w = exynos_plane_get_size(crtc_x, crtc_w, crtc->mode.hdisplay);
>>> -    actual_h = exynos_plane_get_size(crtc_y, crtc_h, crtc->mode.vdisplay);
>>> +    actual_w = exynos_plane_get_size(crtc_x, crtc_w, mode->hdisplay);
>>> +    actual_h = exynos_plane_get_size(crtc_y, crtc_h, mode->vdisplay);
>>>
>>>      if (crtc_x < 0) {
>>>          if (actual_w)
>>> @@ -132,10 +133,10 @@ void exynos_plane_mode_set(struct drm_plane
>>> *plane, struct drm_crtc *crtc,
>>>      exynos_plane->crtc_height = actual_h;
>>>
>>>      /* set drm mode data. */
>>> -    exynos_plane->mode_width = crtc->mode.hdisplay;
>>> -    exynos_plane->mode_height = crtc->mode.vdisplay;
>>> -    exynos_plane->refresh = crtc->mode.vrefresh;
>>> -    exynos_plane->scan_flag = crtc->mode.flags;
>>> +    exynos_plane->mode_width = mode->hdisplay;
>>> +    exynos_plane->mode_height = mode->vdisplay;
>>> +    exynos_plane->refresh = mode->vrefresh;
>>> +    exynos_plane->scan_flag = mode->flags;
>>>
>>>      DRM_DEBUG_KMS("plane : offset_x/y(%d,%d), width/height(%d,%d)",
>>>              exynos_plane->crtc_x, exynos_plane->crtc_y,
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 02/17] drm/exynos: use adjusted_mode of crtc_state instead of mode

[Gustavo Padovan]

Hi Inki,

2015-06-02 Inki Dae <inki.dae@samsung.com>:

> On 2015년 06월 02일 09:03, Joonyoung Shim wrote:
> > On 06/02/2015 12:09 AM, Tobias Jakobi wrote:
> >> Hello,
> >>
> >> as pointed out in [1] before, this gives me an kernel oops during boot.
> >>
> >> With best wishes,
> >> Tobias
> >>
> >> [1] http://www.spinics.net/lists/linux-samsung-soc/msg44736.html
> >>
> >
> > Yeah, this patch should go after switched by drm_helper_crtc_mode_set,
> > e.g. after the patch "drm/exynos: atomic phase 1: add .mode_set_nofb()
> > callback". Then crtc->base.state cannot be NULL.
> >
> > Gustavo, sorry for that, i meant rebase based on the patch "drm/exynos:
> > atomic phase 1: add .mode_set_nofb() callback"
> >
> > Inki, i think it's time to merge this patchset for switch to atomic
> > modeset functions if no any objection, just need to rebase of this patch
> > based on the patch "drm/exynos: atomic phase 1 : add .mode_set_nofb()
> > callback" and except a patch 17/17 from this merge, i think clean up
> > patches like it need to more review after merge atomic patchset.
> 
> Thanks Joonyoung. Got it. I will merge this patch series to my local
> repository for test and final review. If there is no any big problem,
> will merge it.

Thanks for that! As I will keep working on exynos I will be around to
send any follow up patch to send any follow up patch to fix potential
issues of the atomic modesetting patches.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 03/17] drm/exynos: atomic phase 1: use drm_plane_helper_update()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Rip out the check from exynos_update_plane() and create
exynos_check_plane() for the check phase enabling use to use
the atomic helpers to call our check and update phases when updating
planes.

Update all users of exynos_update_plane() accordingly to call
exynos_check_plane() before.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>y
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 31 ++++++++++++------------
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 40 +++++++++++++++++++++++--------
 drivers/gpu/drm/exynos/exynos_drm_plane.h |  2 +-
 3 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 363b019..ba44c9b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -116,6 +116,7 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_framebuffer *fb = crtc->primary->fb;
 	unsigned int crtc_w;
 	unsigned int crtc_h;
+	int ret;
 
 	/* when framebuffer changing is requested, crtc's dpms should be on */
 	if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
@@ -123,12 +124,17 @@ static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EPERM;
 	}
 
+	ret = exynos_check_plane(crtc->primary, fb);
+	if (ret)
+		return ret;
+
 	crtc_w = fb->width - x;
 	crtc_h = fb->height - y;
+	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
+			    crtc_w, crtc_h, x << 16, y << 16,
+			    crtc_w << 16, crtc_h << 16);
 
-	return exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-				   crtc_w, crtc_h, x << 16, y << 16,
-				   crtc_w << 16, crtc_h << 16);
+	return 0;
 }
 
 static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
@@ -165,7 +171,6 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	unsigned int crtc_w, crtc_h;
 	int ret;
 
@@ -184,6 +189,10 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 		goto out;
 	}
 
+	ret = exynos_check_plane(crtc->primary, fb);
+	if (ret)
+		goto out;
+
 	ret = drm_vblank_get(dev, exynos_crtc->pipe);
 	if (ret) {
 		DRM_DEBUG("failed to acquire vblank counter\n");
@@ -202,17 +211,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 	crtc->primary->fb = fb;
 	crtc_w = fb->width - crtc->x;
 	crtc_h = fb->height - crtc->y;
-	ret = exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-				  crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
-				  crtc_w << 16, crtc_h << 16);
-	if (ret) {
-		crtc->primary->fb = old_fb;
-		spin_lock_irq(&dev->event_lock);
-		exynos_crtc->event = NULL;
-		drm_vblank_put(dev, exynos_crtc->pipe);
-		spin_unlock_irq(&dev->event_lock);
-		return ret;
-	}
+	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
+			    crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
+			    crtc_w << 16, crtc_h << 16);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 01d2918..6b10373 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -145,21 +145,15 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 	plane->crtc = crtc;
 }
 
-int
+void
 exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 		     unsigned int crtc_w, unsigned int crtc_h,
 		     uint32_t src_x, uint32_t src_y,
 		     uint32_t src_w, uint32_t src_h)
 {
-
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
 	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-	int ret;
-
-	ret = exynos_check_plane(plane, fb);
-	if (ret < 0)
-		return ret;
 
 	exynos_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y,
 			      crtc_w, crtc_h, src_x >> 16, src_y >> 16,
@@ -167,8 +161,6 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	if (exynos_crtc->ops->win_commit)
 		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
-
-	return 0;
 }
 
 static int exynos_disable_plane(struct drm_plane *plane)
@@ -184,11 +176,37 @@ static int exynos_disable_plane(struct drm_plane *plane)
 }
 
 static struct drm_plane_funcs exynos_plane_funcs = {
-	.update_plane	= exynos_update_plane,
+	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= exynos_disable_plane,
 	.destroy	= drm_plane_cleanup,
 };
 
+static int exynos_plane_atomic_check(struct drm_plane *plane,
+				     struct drm_plane_state *state)
+{
+	return exynos_check_plane(plane, state->fb);
+}
+
+static void exynos_plane_atomic_update(struct drm_plane *plane,
+				       struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = plane->state;
+
+	if (!state->crtc)
+		return;
+
+	exynos_update_plane(plane, state->crtc, state->fb,
+			    state->crtc_x, state->crtc_y,
+			    state->crtc_w, state->crtc_h,
+			    state->src_x, state->src_y,
+			    state->src_w, state->src_h);
+}
+
+static const struct drm_plane_helper_funcs plane_helper_funcs = {
+	.atomic_check = exynos_plane_atomic_check,
+	.atomic_update = exynos_plane_atomic_update,
+};
+
 static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
 					      unsigned int zpos)
 {
@@ -224,6 +242,8 @@ int exynos_plane_init(struct drm_device *dev,
 		return err;
 	}
 
+	drm_plane_helper_add(&exynos_plane->base, &plane_helper_funcs);
+
 	exynos_plane->zpos = zpos;
 
 	if (type == DRM_PLANE_TYPE_OVERLAY)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index f360590..560ca71 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -15,7 +15,7 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 			   unsigned int crtc_w, unsigned int crtc_h,
 			   uint32_t src_x, uint32_t src_y,
 			   uint32_t src_w, uint32_t src_h);
-int exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+void exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 			unsigned int crtc_w, unsigned int crtc_h,
 			uint32_t src_x, uint32_t src_y,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 04/17] drm/exynos: atomic phase 1: use drm_plane_helper_disable()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The atomic helper to disable planes also uses the optional
.atomic_disable() helper. The unique operation it does is calling
.win_disable()

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 32 ++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 6b10373..f0067f7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -67,6 +67,9 @@ int exynos_check_plane(struct drm_plane *plane, struct drm_framebuffer *fb)
 	int nr;
 	int i;
 
+	if (!fb)
+		return 0;
+
 	nr = exynos_drm_fb_get_buf_cnt(fb);
 	for (i = 0; i < nr; i++) {
 		struct exynos_drm_gem_buf *buffer = exynos_drm_fb_buffer(fb, i);
@@ -163,21 +166,9 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
 }
 
-static int exynos_disable_plane(struct drm_plane *plane)
-{
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(plane->crtc);
-
-	if (exynos_crtc && exynos_crtc->ops->win_disable)
-		exynos_crtc->ops->win_disable(exynos_crtc,
-					      exynos_plane->zpos);
-
-	return 0;
-}
-
 static struct drm_plane_funcs exynos_plane_funcs = {
 	.update_plane	= drm_plane_helper_update,
-	.disable_plane	= exynos_disable_plane,
+	.disable_plane	= drm_plane_helper_disable,
 	.destroy	= drm_plane_cleanup,
 };
 
@@ -202,9 +193,24 @@ static void exynos_plane_atomic_update(struct drm_plane *plane,
 			    state->src_w, state->src_h);
 }
 
+static void exynos_plane_atomic_disable(struct drm_plane *plane,
+					struct drm_plane_state *old_state)
+{
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(old_state->crtc);
+
+	if (!old_state->crtc)
+		return;
+
+	if (exynos_crtc->ops->win_disable)
+		exynos_crtc->ops->win_disable(exynos_crtc,
+					      exynos_plane->zpos);
+}
+
 static const struct drm_plane_helper_funcs plane_helper_funcs = {
 	.atomic_check = exynos_plane_atomic_check,
 	.atomic_update = exynos_plane_atomic_update,
+	.atomic_disable = exynos_plane_atomic_disable,
 };
 
 static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
-- 
2.1.0

[PATCH v10 05/17] drm/exynos: atomic phase 1: add .mode_set_nofb() callback

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The new atomic infrastructure needs the .mode_set_nofb() callback to
update CRTC timings before setting any plane.

v2: remove WARN_ON(!crtc->state) from mode_set_nofb

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 63 ++++----------------------------
 1 file changed, 7 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index ba44c9b..75eb61b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -62,9 +62,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
 
 	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 bool
@@ -81,60 +78,13 @@ exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static int
-exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
-			  struct drm_display_mode *adjusted_mode, int x, int y,
-			  struct drm_framebuffer *old_fb)
-{
-	struct drm_framebuffer *fb = crtc->primary->fb;
-	unsigned int crtc_w;
-	unsigned int crtc_h;
-	int ret;
-
-	/*
-	 * copy the mode data adjusted by mode_fixup() into crtc->mode
-	 * so that hardware can be seet to proper mode.
-	 */
-	memcpy(&crtc->mode, adjusted_mode, sizeof(*adjusted_mode));
-
-	ret = exynos_check_plane(crtc->primary, fb);
-	if (ret < 0)
-		return ret;
-
-	crtc_w = fb->width - x;
-	crtc_h = fb->height - y;
-	exynos_plane_mode_set(crtc->primary, crtc, fb, 0, 0,
-			      crtc_w, crtc_h, x, y, crtc_w, crtc_h);
-
-	return 0;
-}
-
-static int exynos_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-					  struct drm_framebuffer *old_fb)
+static void
+exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct drm_framebuffer *fb = crtc->primary->fb;
-	unsigned int crtc_w;
-	unsigned int crtc_h;
-	int ret;
-
-	/* when framebuffer changing is requested, crtc's dpms should be on */
-	if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
-		DRM_ERROR("failed framebuffer changing request.\n");
-		return -EPERM;
-	}
 
-	ret = exynos_check_plane(crtc->primary, fb);
-	if (ret)
-		return ret;
-
-	crtc_w = fb->width - x;
-	crtc_h = fb->height - y;
-	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-			    crtc_w, crtc_h, x << 16, y << 16,
-			    crtc_w << 16, crtc_h << 16);
-
-	return 0;
+	if (exynos_crtc->ops->commit)
+		exynos_crtc->ops->commit(exynos_crtc);
 }
 
 static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
@@ -159,8 +109,9 @@ static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
 	.prepare	= exynos_drm_crtc_prepare,
 	.commit		= exynos_drm_crtc_commit,
 	.mode_fixup	= exynos_drm_crtc_mode_fixup,
-	.mode_set	= exynos_drm_crtc_mode_set,
-	.mode_set_base	= exynos_drm_crtc_mode_set_base,
+	.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,
 };
 
-- 
2.1.0

[PATCH v10 06/17] drm/exynos: atomic phase 2: wire up state reset(), duplicate() and destroy()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Set CRTC, planes and connectors to use the default implementations from
the atomic helper library. The helpers will work to keep track of state
for each DRM object.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/bridge/ps8622.c           | 4 ++++
 drivers/gpu/drm/bridge/ptn3460.c          | 4 ++++
 drivers/gpu/drm/exynos/exynos_dp_core.c   | 4 ++++
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 5 +++++
 drivers/gpu/drm/exynos/exynos_drm_dpi.c   | 4 ++++
 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 2 ++
 drivers/gpu/drm/exynos/exynos_drm_dsi.c   | 4 ++++
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 ++++
 drivers/gpu/drm/exynos/exynos_drm_vidi.c  | 4 ++++
 drivers/gpu/drm/exynos/exynos_hdmi.c      | 4 ++++
 10 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c
index e895aa7..b604326 100644
--- a/drivers/gpu/drm/bridge/ps8622.c
+++ b/drivers/gpu/drm/bridge/ps8622.c
@@ -31,6 +31,7 @@
 #include "drmP.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_atomic_helper.h"
 
 /* Brightness scale on the Parade chip */
 #define PS8622_MAX_BRIGHTNESS 0xff
@@ -502,6 +503,9 @@ static const struct drm_connector_funcs ps8622_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ps8622_detect,
 	.destroy = ps8622_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int ps8622_attach(struct drm_bridge *bridge)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index 9d2f053..8ed3617 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -27,6 +27,7 @@
 
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_atomic_helper.h"
 #include "drm_edid.h"
 #include "drmP.h"
 
@@ -263,6 +264,9 @@ static struct drm_connector_funcs ptn3460_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ptn3460_detect,
 	.destroy = ptn3460_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int ptn3460_bridge_attach(struct drm_bridge *bridge)
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 30feb7d..195fe60 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -28,6 +28,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_panel.h>
 #include <drm/bridge/ptn3460.h>
 
@@ -957,6 +958,9 @@ static struct drm_connector_funcs exynos_dp_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = exynos_dp_detect,
 	.destroy = exynos_dp_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int exynos_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 75eb61b..73ccfa7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -14,6 +14,8 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "exynos_drm_crtc.h"
 #include "exynos_drm_drv.h"
@@ -188,6 +190,9 @@ static struct drm_crtc_funcs exynos_crtc_funcs = {
 	.set_config	= drm_crtc_helper_set_config,
 	.page_flip	= exynos_drm_crtc_page_flip,
 	.destroy	= exynos_drm_crtc_destroy,
+	.reset = drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
 struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 37678cf..ced5c23 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -13,6 +13,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
+#include <drm/drm_atomic_helper.h>
 
 #include <linux/regulator/consumer.h>
 
@@ -63,6 +64,9 @@ static struct drm_connector_funcs exynos_dpi_connector_funcs = {
 	.detect = exynos_dpi_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = exynos_dpi_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int exynos_dpi_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 8ac4652..08b9a8c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -98,6 +98,8 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto err_cleanup_vblank;
 
+	drm_mode_config_reset(dev);
+
 	/*
 	 * enable drm irq mode.
 	 * - with irq_enabled = true, we can use the vblank feature.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 0492715..e4e7f74 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -14,6 +14,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_panel.h>
+#include <drm/drm_atomic_helper.h>
 
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
@@ -1461,6 +1462,9 @@ static struct drm_connector_funcs exynos_dsi_connector_funcs = {
 	.detect = exynos_dsi_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = exynos_dsi_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int exynos_dsi_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index f0067f7..42fcaca 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -13,6 +13,7 @@
 
 #include <drm/exynos_drm.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include "exynos_drm_drv.h"
 #include "exynos_drm_crtc.h"
 #include "exynos_drm_fb.h"
@@ -170,6 +171,9 @@ static struct drm_plane_funcs exynos_plane_funcs = {
 	.update_plane	= drm_plane_helper_update,
 	.disable_plane	= drm_plane_helper_disable,
 	.destroy	= drm_plane_cleanup,
+	.reset = drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 };
 
 static int exynos_plane_atomic_check(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 1b3479a..fc3a14b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -20,6 +20,7 @@
 
 #include <drm/drm_edid.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_crtc.h"
@@ -388,6 +389,9 @@ static struct drm_connector_funcs vidi_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = vidi_detect,
 	.destroy = vidi_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int vidi_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 5eba971..471e486 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -17,6 +17,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "regs-hdmi.h"
 
@@ -1054,6 +1055,9 @@ static struct drm_connector_funcs hdmi_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = hdmi_detect,
 	.destroy = hdmi_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int hdmi_get_modes(struct drm_connector *connector)
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 07/17] drm/exynos: atomic phase 2: keep track of framebuffer pointer

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Use drm_atomic_set_fb_for_plane() in the legacy page_flip path to keep
track of the framebuffer pointer and reference.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 73ccfa7..f782d1f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -168,6 +168,9 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 			    crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
 			    crtc_w << 16, crtc_h << 16);
 
+	if (crtc->primary->state)
+		drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
+
 	return 0;
 
 out:
-- 
2.1.0

[PATCH v10 08/17] drm/exynos: atomic phase 3: atomic updates of planes

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now that phase 1 and 2 are complete we can switch the update/disable_plane
callbacks to their atomic version.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c    | 3 +++
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 142eb4e..19c0642 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -16,6 +16,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <uapi/drm/exynos_drm.h>
 
 #include "exynos_drm_drv.h"
@@ -268,6 +269,8 @@ static void exynos_drm_output_poll_changed(struct drm_device *dev)
 static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
 	.fb_create = exynos_user_fb_create,
 	.output_poll_changed = exynos_drm_output_poll_changed,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 void exynos_drm_mode_config_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 42fcaca..fc332b4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -168,8 +168,8 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 static struct drm_plane_funcs exynos_plane_funcs = {
-	.update_plane	= drm_plane_helper_update,
-	.disable_plane	= drm_plane_helper_disable,
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= drm_plane_cleanup,
 	.reset = drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
-- 
2.1.0

[PATCH v10 09/17] drm/exynos: atomic phase 3: use atomic .set_config helper

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now that phase 1 and 2 are complete switch .set_config helper to
use the atomic one.

v2: also remove .prepare() callback

v3: remove .mode_set() and .mode_set_base() and encoder's
.prepare() callbacks

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 10 +---------
 drivers/gpu/drm/exynos/exynos_drm_encoder.c |  6 ------
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index f782d1f..dd1ee7f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -50,11 +50,6 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
 		drm_crtc_vblank_on(crtc);
 }
 
-static void exynos_drm_crtc_prepare(struct drm_crtc *crtc)
-{
-	/* drm framework doesn't check NULL. */
-}
-
 static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
@@ -108,12 +103,9 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 
 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,
 	.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,
 };
 
@@ -190,7 +182,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
 }
 
 static struct drm_crtc_funcs exynos_crtc_funcs = {
-	.set_config	= drm_crtc_helper_set_config,
+	.set_config	= drm_atomic_helper_set_config,
 	.page_flip	= exynos_drm_crtc_page_flip,
 	.destroy	= exynos_drm_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index 57de0bd..915de13 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -76,11 +76,6 @@ 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. */
-}
-
 static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
 {
 	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
@@ -111,7 +106,6 @@ 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,
 	.disable	= exynos_drm_encoder_disable,
 };
-- 
2.1.0

[PATCH v10 10/17] drm/exynos: atomic phase 3: convert page flips

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

PageFlips now use the atomic helper to work through the atomic modesetting
API. Async page flips are not supported yet.

v2: Add .atomic_begin() step to handle the vblank part we removed from
exynos page_flip code.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 79 +++++++-------------------------
 drivers/gpu/drm/exynos/exynos_drm_fb.c   |  9 +++-
 2 files changed, 25 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index dd1ee7f..a137799 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -101,75 +101,30 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 	}
 }
 
+static void exynos_crtc_atomic_begin(struct drm_crtc *crtc)
+{
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+
+	if (crtc->state->event) {
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+		exynos_crtc->event = crtc->state->event;
+	}
+}
+
+static void exynos_crtc_atomic_flush(struct drm_crtc *crtc)
+{
+}
+
 static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
 	.dpms		= exynos_drm_crtc_dpms,
 	.commit		= exynos_drm_crtc_commit,
 	.mode_fixup	= exynos_drm_crtc_mode_fixup,
 	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
 	.disable	= exynos_drm_crtc_disable,
+	.atomic_begin	= exynos_crtc_atomic_begin,
+	.atomic_flush	= exynos_crtc_atomic_flush,
 };
 
-static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
-				     struct drm_framebuffer *fb,
-				     struct drm_pending_vblank_event *event,
-				     uint32_t page_flip_flags)
-{
-	struct drm_device *dev = crtc->dev;
-	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	unsigned int crtc_w, crtc_h;
-	int ret;
-
-	/* when the page flip is requested, crtc's dpms should be on */
-	if (exynos_crtc->dpms > DRM_MODE_DPMS_ON) {
-		DRM_ERROR("failed page flip request.\n");
-		return -EINVAL;
-	}
-
-	if (!event)
-		return -EINVAL;
-
-	spin_lock_irq(&dev->event_lock);
-	if (exynos_crtc->event) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	ret = exynos_check_plane(crtc->primary, fb);
-	if (ret)
-		goto out;
-
-	ret = drm_vblank_get(dev, exynos_crtc->pipe);
-	if (ret) {
-		DRM_DEBUG("failed to acquire vblank counter\n");
-		goto out;
-	}
-
-	exynos_crtc->event = event;
-	spin_unlock_irq(&dev->event_lock);
-
-	/*
-	 * the pipe from user always is 0 so we can set pipe number
-	 * of current owner to event.
-	 */
-	event->pipe = exynos_crtc->pipe;
-
-	crtc->primary->fb = fb;
-	crtc_w = fb->width - crtc->x;
-	crtc_h = fb->height - crtc->y;
-	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
-			    crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
-			    crtc_w << 16, crtc_h << 16);
-
-	if (crtc->primary->state)
-		drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
-
-	return 0;
-
-out:
-	spin_unlock_irq(&dev->event_lock);
-	return ret;
-}
-
 static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
@@ -183,7 +138,7 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
 
 static struct drm_crtc_funcs exynos_crtc_funcs = {
 	.set_config	= drm_atomic_helper_set_config,
-	.page_flip	= exynos_drm_crtc_page_flip,
+	.page_flip	= drm_atomic_helper_page_flip,
 	.destroy	= exynos_drm_crtc_destroy,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 19c0642..05d229c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -266,11 +266,18 @@ static void exynos_drm_output_poll_changed(struct drm_device *dev)
 		exynos_drm_fbdev_init(dev);
 }
 
+static int exynos_atomic_commit(struct drm_device *dev,
+				struct drm_atomic_state *state,
+				bool async)
+{
+	return drm_atomic_helper_commit(dev, state, false);
+}
+
 static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
 	.fb_create = exynos_user_fb_create,
 	.output_poll_changed = exynos_drm_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = drm_atomic_helper_commit,
+	.atomic_commit = exynos_atomic_commit,
 };
 
 void exynos_drm_mode_config_init(struct drm_device *dev)
-- 
2.1.0

[PATCH v10 11/17] drm/exynos: remove exported functions from exynos_drm_plane

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Now that no one is using the functions exported by exynos_drm_plane due
to the atomic conversion we can make remove some of the them or make them
static.

v2: remove unused exynos_drm_crtc

v3: fix checkpatch error (reported by Joonyoung)

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 98 +++++++++++++------------------
 drivers/gpu/drm/exynos/exynos_drm_plane.h | 11 ----
 2 files changed, 42 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index fc332b4..a729980 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -62,38 +62,13 @@ static int exynos_plane_get_size(int start, unsigned length, unsigned last)
 	return size;
 }
 
-int exynos_check_plane(struct drm_plane *plane, struct drm_framebuffer *fb)
-{
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-	int nr;
-	int i;
-
-	if (!fb)
-		return 0;
-
-	nr = exynos_drm_fb_get_buf_cnt(fb);
-	for (i = 0; i < nr; i++) {
-		struct exynos_drm_gem_buf *buffer = exynos_drm_fb_buffer(fb, i);
-
-		if (!buffer) {
-			DRM_DEBUG_KMS("buffer is null\n");
-			return -EFAULT;
-		}
-
-		exynos_plane->dma_addr[i] = buffer->dma_addr + fb->offsets[i];
-
-		DRM_DEBUG_KMS("buffer: %d, dma_addr = 0x%lx\n",
-				i, (unsigned long)exynos_plane->dma_addr[i]);
-	}
-
-	return 0;
-}
-
-void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
-			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			  unsigned int crtc_w, unsigned int crtc_h,
-			  uint32_t src_x, uint32_t src_y,
-			  uint32_t src_w, uint32_t src_h)
+static void exynos_plane_mode_set(struct drm_plane *plane,
+				  struct drm_crtc *crtc,
+				  struct drm_framebuffer *fb,
+				  int crtc_x, int crtc_y,
+				  unsigned int crtc_w, unsigned int crtc_h,
+				  uint32_t src_x, uint32_t src_y,
+				  uint32_t src_w, uint32_t src_h)
 {
 	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
@@ -149,24 +124,6 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 	plane->crtc = crtc;
 }
 
-void
-exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-		     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-		     unsigned int crtc_w, unsigned int crtc_h,
-		     uint32_t src_x, uint32_t src_y,
-		     uint32_t src_w, uint32_t src_h)
-{
-	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-
-	exynos_plane_mode_set(plane, crtc, fb, crtc_x, crtc_y,
-			      crtc_w, crtc_h, src_x >> 16, src_y >> 16,
-			      src_w >> 16, src_h >> 16);
-
-	if (exynos_crtc->ops->win_commit)
-		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
-}
-
 static struct drm_plane_funcs exynos_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -179,22 +136,51 @@ static struct drm_plane_funcs exynos_plane_funcs = {
 static int exynos_plane_atomic_check(struct drm_plane *plane,
 				     struct drm_plane_state *state)
 {
-	return exynos_check_plane(plane, state->fb);
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+	int nr;
+	int i;
+
+	if (!state->fb)
+		return 0;
+
+	nr = exynos_drm_fb_get_buf_cnt(state->fb);
+	for (i = 0; i < nr; i++) {
+		struct exynos_drm_gem_buf *buffer =
+					exynos_drm_fb_buffer(state->fb, i);
+
+		if (!buffer) {
+			DRM_DEBUG_KMS("buffer is null\n");
+			return -EFAULT;
+		}
+
+		exynos_plane->dma_addr[i] = buffer->dma_addr +
+					    state->fb->offsets[i];
+
+		DRM_DEBUG_KMS("buffer: %d, dma_addr = 0x%lx\n",
+				i, (unsigned long)exynos_plane->dma_addr[i]);
+	}
+
+	return 0;
 }
 
 static void exynos_plane_atomic_update(struct drm_plane *plane,
 				       struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = plane->state;
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(state->crtc);
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
 
 	if (!state->crtc)
 		return;
 
-	exynos_update_plane(plane, state->crtc, state->fb,
-			    state->crtc_x, state->crtc_y,
-			    state->crtc_w, state->crtc_h,
-			    state->src_x, state->src_y,
-			    state->src_w, state->src_h);
+	exynos_plane_mode_set(plane, state->crtc, state->fb,
+			      state->crtc_x, state->crtc_y,
+			      state->crtc_w, state->crtc_h,
+			      state->src_x >> 16, state->src_y >> 16,
+			      state->src_w >> 16, state->src_h >> 16);
+
+	if (exynos_crtc->ops->win_commit)
+		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
 }
 
 static void exynos_plane_atomic_disable(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index 560ca71..8c88ae9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -9,17 +9,6 @@
  *
  */
 
-int exynos_check_plane(struct drm_plane *plane, struct drm_framebuffer *fb);
-void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
-			   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			   unsigned int crtc_w, unsigned int crtc_h,
-			   uint32_t src_x, uint32_t src_y,
-			   uint32_t src_w, uint32_t src_h);
-void exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-			struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			unsigned int crtc_w, unsigned int crtc_h,
-			uint32_t src_x, uint32_t src_y,
-			uint32_t src_w, uint32_t src_h);
 int exynos_plane_init(struct drm_device *dev,
 		      struct exynos_drm_plane *exynos_plane,
 		      unsigned long possible_crtcs, enum drm_plane_type type,
-- 
2.1.0

[PATCH v10 12/17] drm/exynos: don't disable unused functions at init

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Everything starts disabled so we don't really need to disable anything.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
 1 file changed, 3 deletions(-)

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);
-
 	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
 	if (ret < 0) {
 		DRM_ERROR("failed to set up hw configuration.\n");
-- 
2.1.0

[PATCH v10 13/17] drm/exynos: move exynos_drm_crtc_disable()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This is a preparation commit to move exynos_drm_crtc_disable() together
with the future exynos_drm_crtc_enable() that will come from the split of
exynos_drm_crtc_dpms() callback.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 36 ++++++++++++++++----------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index a137799..7125fbe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -50,6 +50,23 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
 		drm_crtc_vblank_on(crtc);
 }
 
+static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	int ret;
+
+	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+
+	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 void exynos_drm_crtc_commit(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
@@ -84,23 +101,6 @@ 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)
-{
-	struct drm_plane *plane;
-	int ret;
-
-	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
-
-	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 void exynos_crtc_atomic_begin(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
@@ -117,10 +117,10 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc)
 
 static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
 	.dpms		= exynos_drm_crtc_dpms,
+	.disable	= exynos_drm_crtc_disable,
 	.commit		= exynos_drm_crtc_commit,
 	.mode_fixup	= exynos_drm_crtc_mode_fixup,
 	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
-	.disable	= exynos_drm_crtc_disable,
 	.atomic_begin	= exynos_crtc_atomic_begin,
 	.atomic_flush	= exynos_crtc_atomic_flush,
 };
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 14/17] drm/exynos: add exynos specific .atomic_commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

exynos needs to update planes with the crtc enabled (mainly for the FIMD
case) so this specific atomic commit changes the order of
drm_atomic_helper_commit_modeset_enables() and
drm_atomic_helper_commit_planes() to commit planes after we enable crtc
and encoders.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 05d229c..789db6f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -16,6 +16,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <uapi/drm/exynos_drm.h>
 
@@ -270,7 +271,37 @@ static int exynos_atomic_commit(struct drm_device *dev,
 				struct drm_atomic_state *state,
 				bool async)
 {
-	return drm_atomic_helper_commit(dev, state, false);
+	int ret;
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (ret)
+		return ret;
+
+	/* This is the point of no return */
+
+	drm_atomic_helper_swap_state(dev, 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.
+	 */
+
+	drm_atomic_helper_commit_planes(dev, state);
+
+	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	drm_atomic_helper_cleanup_planes(dev, state);
+
+	drm_atomic_state_free(state);
+
+	return 0;
 }
 
 static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
-- 
2.1.0

[PATCH v10 15/17] drm/exynos: atomic dpms support

[Gustavo Padovan]

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.

v2: Address comments by Joonyoung:
	- make hdmi code call ->disable() instead of ->dpms()
	- do not use WARN_ON on crtc enable/disable

v3: - Fix build failure after the hdmi change in v2
    - Change dpms helper of ptn3460 bridge

v4: - remove win_commit() call from .enable()

v5: - move .atomic_check() to the atomic PageFlip patch, and transform it
in .atomic_begin()

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/bridge/ps8622.c             |  2 +-
 drivers/gpu/drm/bridge/ptn3460.c            |  2 +-
 drivers/gpu/drm/exynos/exynos_dp_core.c     |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 58 ++++++++++++-----------------
 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 | 21 +++--------
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +--
 10 files changed, 40 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c
index b604326..d686235 100644
--- a/drivers/gpu/drm/bridge/ps8622.c
+++ b/drivers/gpu/drm/bridge/ps8622.c
@@ -499,7 +499,7 @@ static void ps8622_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs ps8622_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ps8622_detect,
 	.destroy = ps8622_connector_destroy,
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index 8ed3617..260bc9f 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -260,7 +260,7 @@ static void ptn3460_connector_destroy(struct drm_connector *connector)
 }
 
 static struct drm_connector_funcs ptn3460_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ptn3460_detect,
 	.destroy = ptn3460_connector_destroy,
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 195fe60..c9995b1 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -954,7 +954,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 7125fbe..dd8e454 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -22,40 +22,41 @@
 #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);
 
-	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 (exynos_crtc->enabled)
 		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_disable(struct drm_crtc *crtc)
 {
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
 	struct drm_plane *plane;
 	int ret;
 
-	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+	if (!exynos_crtc->enabled)
+		return;
+
+	/* 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, DRM_MODE_DPMS_OFF);
+
+	exynos_crtc->enabled = false;
 
 	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
 		if (plane->crtc != crtc)
@@ -67,17 +68,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 	}
 }
 
-static void exynos_drm_crtc_commit(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);
-
-	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
-
-	if (exynos_crtc->ops->win_commit)
-		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
-}
-
 static bool
 exynos_drm_crtc_mode_fixup(struct drm_crtc *crtc,
 			    const struct drm_display_mode *mode,
@@ -116,9 +106,8 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc)
 }
 
 static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
-	.dpms		= exynos_drm_crtc_dpms,
+	.enable		= exynos_drm_crtc_enable,
 	.disable	= exynos_drm_crtc_disable,
-	.commit		= exynos_drm_crtc_commit,
 	.mode_fixup	= exynos_drm_crtc_mode_fixup,
 	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
 	.atomic_begin	= exynos_crtc_atomic_begin,
@@ -163,7 +152,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;
@@ -194,7 +182,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)
@@ -209,7 +197,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 29e3fb7..86d6894 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -201,7 +201,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
@@ -210,7 +210,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;
 	const 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 e4e7f74..190f3b3 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 915de13..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,7 +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_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;
@@ -90,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) {
@@ -103,10 +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,
-	.commit		= exynos_drm_encoder_commit,
+	.enable		= exynos_drm_encoder_enable,
 	.disable	= exynos_drm_encoder_disable,
 };
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index fc3a14b..63c1536 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 471e486..8c3c27b 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,
@@ -2127,8 +2127,8 @@ static void hdmi_dpms(struct exynos_drm_display *display, int mode)
 		 */
 		if (crtc)
 			funcs = crtc->helper_private;
-		if (funcs && funcs->dpms)
-			(*funcs->dpms)(crtc, mode);
+		if (funcs && funcs->disable)
+			(*funcs->disable)(crtc);
 
 		hdmi_poweroff(hdata);
 		break;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 16/17] drm/exynos: remove unnecessary calls to disable_plane()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The planes are already disabled by the drm_atomic_helper_commit() code
so we don't need to disable the in these two places.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Tested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 11 -----------
 drivers/gpu/drm/exynos/exynos_drm_encoder.c |  8 --------
 2 files changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index dd8e454..b7c6d51 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -40,8 +40,6 @@ static void exynos_drm_crtc_enable(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 drm_plane *plane;
-	int ret;
 
 	if (!exynos_crtc->enabled)
 		return;
@@ -57,15 +55,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 		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
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index 0648ba4..7b89fd5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -81,17 +81,9 @@ 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;
 
 	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) {
-		if (plane->crtc && (plane->crtc == encoder->crtc))
-			plane->funcs->disable_plane(plane);
-	}
 }
 
 static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

To follow more closely the new atomic API we split the dpms()
helper into the enable() and disable() helper to get exactly the
same semantics.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c | 87 ++++++------------------------
 drivers/gpu/drm/exynos/exynos_drm_crtc.c   |  8 +--
 drivers/gpu/drm/exynos/exynos_drm_drv.h    |  6 ++-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 69 +++++-------------------
 drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 53 +++++++-----------
 drivers/gpu/drm/exynos/exynos_mixer.c      | 26 +++------
 6 files changed, 62 insertions(+), 187 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index f29e4be..d659ba2 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -603,75 +603,39 @@ static void decon_init(struct decon_context *ctx)
 		writel(VIDCON1_VCLK_HOLD, ctx->regs + VIDCON1(0));
 }
 
-static int decon_poweron(struct decon_context *ctx)
+static void decon_enable(struct exynos_drm_crtc *crtc)
 {
-	int ret;
+	struct decon_context *ctx = crtc->ctx;
 
 	if (!ctx->suspended)
-		return 0;
+		return;
 
 	ctx->suspended = false;
 
 	pm_runtime_get_sync(ctx->dev);
 
-	ret = clk_prepare_enable(ctx->pclk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
-		goto pclk_err;
-	}
-
-	ret = clk_prepare_enable(ctx->aclk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
-		goto aclk_err;
-	}
-
-	ret = clk_prepare_enable(ctx->eclk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
-		goto eclk_err;
-	}
-
-	ret = clk_prepare_enable(ctx->vclk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
-		goto vclk_err;
-	}
+	clk_prepare_enable(ctx->pclk);
+	clk_prepare_enable(ctx->aclk);
+	clk_prepare_enable(ctx->eclk);
+	clk_prepare_enable(ctx->vclk);
 
 	decon_init(ctx);
 
 	/* if vblank was enabled status, enable it again. */
-	if (test_and_clear_bit(0, &ctx->irq_flags)) {
-		ret = decon_enable_vblank(ctx->crtc);
-		if (ret) {
-			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
-			goto err;
-		}
-	}
+	if (test_and_clear_bit(0, &ctx->irq_flags))
+		decon_enable_vblank(ctx->crtc);
 
 	decon_window_resume(ctx);
 
 	decon_apply(ctx);
-
-	return 0;
-
-err:
-	clk_disable_unprepare(ctx->vclk);
-vclk_err:
-	clk_disable_unprepare(ctx->eclk);
-eclk_err:
-	clk_disable_unprepare(ctx->aclk);
-aclk_err:
-	clk_disable_unprepare(ctx->pclk);
-pclk_err:
-	ctx->suspended = true;
-	return ret;
 }
 
-static int decon_poweroff(struct decon_context *ctx)
+static void decon_disable(struct exynos_drm_crtc *crtc)
 {
+	struct decon_context *ctx = crtc->ctx;
+
 	if (ctx->suspended)
-		return 0;
+		return;
 
 	/*
 	 * We need to make sure that all windows are disabled before we
@@ -688,30 +652,11 @@ static int decon_poweroff(struct decon_context *ctx)
 	pm_runtime_put_sync(ctx->dev);
 
 	ctx->suspended = true;
-	return 0;
-}
-
-static void decon_dpms(struct exynos_drm_crtc *crtc, int mode)
-{
-	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
-
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		decon_poweron(crtc->ctx);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		decon_poweroff(crtc->ctx);
-		break;
-	default:
-		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
-		break;
-	}
 }
 
 static const struct exynos_drm_crtc_ops decon_crtc_ops = {
-	.dpms = decon_dpms,
+	.enable = decon_enable,
+	.disable = decon_disable,
 	.mode_fixup = decon_mode_fixup,
 	.commit = decon_commit,
 	.enable_vblank = decon_enable_vblank,
@@ -796,7 +741,7 @@ static void decon_unbind(struct device *dev, struct device *master,
 {
 	struct decon_context *ctx = dev_get_drvdata(dev);
 
-	decon_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
+	decon_disable(ctx->crtc);
 
 	if (ctx->display)
 		exynos_dpi_remove(ctx->display);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b7c6d51..644b4b7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -29,8 +29,8 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
 	if (exynos_crtc->enabled)
 		return;
 
-	if (exynos_crtc->ops->dpms)
-		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_ON);
+	if (exynos_crtc->ops->enable)
+		exynos_crtc->ops->enable(exynos_crtc);
 
 	exynos_crtc->enabled = true;
 
@@ -51,8 +51,8 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 
 	drm_crtc_vblank_off(crtc);
 
-	if (exynos_crtc->ops->dpms)
-		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_OFF);
+	if (exynos_crtc->ops->disable)
+		exynos_crtc->ops->disable(exynos_crtc);
 
 	exynos_crtc->enabled = false;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 86d6894..1c66f65 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -157,7 +157,8 @@ struct exynos_drm_display {
 /*
  * Exynos drm crtc ops
  *
- * @dpms: control device power.
+ * @enable: enable the device
+ * @disable: disable the device
  * @mode_fixup: fix mode data before applying it
  * @commit: set current hw specific display mode to hw.
  * @enable_vblank: specific driver callback for enabling vblank interrupt.
@@ -175,7 +176,8 @@ struct exynos_drm_display {
  */
 struct exynos_drm_crtc;
 struct exynos_drm_crtc_ops {
-	void (*dpms)(struct exynos_drm_crtc *crtc, int mode);
+	void (*enable)(struct exynos_drm_crtc *crtc);
+	void (*disable)(struct exynos_drm_crtc *crtc);
 	bool (*mode_fixup)(struct exynos_drm_crtc *crtc,
 				const struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index b326b31..9661853 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -805,57 +805,35 @@ static void fimd_apply(struct fimd_context *ctx)
 	fimd_commit(ctx->crtc);
 }
 
-static int fimd_poweron(struct fimd_context *ctx)
+static void fimd_enable(struct exynos_drm_crtc *crtc)
 {
-	int ret;
+	struct fimd_context *ctx = crtc->ctx;
 
 	if (!ctx->suspended)
-		return 0;
+		return;
 
 	ctx->suspended = false;
 
 	pm_runtime_get_sync(ctx->dev);
 
-	ret = clk_prepare_enable(ctx->bus_clk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
-		goto bus_clk_err;
-	}
-
-	ret = clk_prepare_enable(ctx->lcd_clk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n", ret);
-		goto lcd_clk_err;
-	}
+	clk_prepare_enable(ctx->bus_clk);
+	clk_prepare_enable(ctx->lcd_clk);
 
 	/* if vblank was enabled status, enable it again. */
-	if (test_and_clear_bit(0, &ctx->irq_flags)) {
-		ret = fimd_enable_vblank(ctx->crtc);
-		if (ret) {
-			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
-			goto enable_vblank_err;
-		}
-	}
+	if (test_and_clear_bit(0, &ctx->irq_flags))
+		fimd_enable_vblank(ctx->crtc);
 
 	fimd_window_resume(ctx);
 
 	fimd_apply(ctx);
-
-	return 0;
-
-enable_vblank_err:
-	clk_disable_unprepare(ctx->lcd_clk);
-lcd_clk_err:
-	clk_disable_unprepare(ctx->bus_clk);
-bus_clk_err:
-	ctx->suspended = true;
-	return ret;
 }
 
-static int fimd_poweroff(struct fimd_context *ctx)
+static void fimd_disable(struct exynos_drm_crtc *crtc)
 {
+	struct fimd_context *ctx = crtc->ctx;
+
 	if (ctx->suspended)
-		return 0;
+		return;
 
 	/*
 	 * We need to make sure that all windows are disabled before we
@@ -870,26 +848,6 @@ static int fimd_poweroff(struct fimd_context *ctx)
 	pm_runtime_put_sync(ctx->dev);
 
 	ctx->suspended = true;
-	return 0;
-}
-
-static void fimd_dpms(struct exynos_drm_crtc *crtc, int mode)
-{
-	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
-
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		fimd_poweron(crtc->ctx);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		fimd_poweroff(crtc->ctx);
-		break;
-	default:
-		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
-		break;
-	}
 }
 
 static void fimd_trigger(struct device *dev)
@@ -964,7 +922,8 @@ static void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable)
 }
 
 static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
-	.dpms = fimd_dpms,
+	.enable = fimd_enable,
+	.disable = fimd_disable,
 	.mode_fixup = fimd_mode_fixup,
 	.commit = fimd_commit,
 	.enable_vblank = fimd_enable_vblank,
@@ -1051,7 +1010,7 @@ static void fimd_unbind(struct device *dev, struct device *master,
 {
 	struct fimd_context *ctx = dev_get_drvdata(dev);
 
-	fimd_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
+	fimd_disable(ctx->crtc);
 
 	fimd_iommu_detach_devices(ctx);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 63c1536..abe4ee0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -153,56 +153,38 @@ static void vidi_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
 	/* TODO. */
 }
 
-static int vidi_power_on(struct vidi_context *ctx, bool enable)
+static void vidi_enable(struct exynos_drm_crtc *crtc)
 {
+	struct vidi_context *ctx = crtc->ctx;
 	struct exynos_drm_plane *plane;
 	int i;
 
-	DRM_DEBUG_KMS("%s\n", __FILE__);
-
-	if (enable != false && enable != true)
-		return -EINVAL;
+	mutex_lock(&ctx->lock);
 
-	if (enable) {
-		ctx->suspended = false;
+	ctx->suspended = false;
 
-		/* if vblank was enabled status, enable it again. */
-		if (test_and_clear_bit(0, &ctx->irq_flags))
-			vidi_enable_vblank(ctx->crtc);
+	/* if vblank was enabled status, enable it again. */
+	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);
-		}
-	} else {
-		ctx->suspended = true;
+	for (i = 0; i < WINDOWS_NR; i++) {
+		plane = &ctx->planes[i];
+		if (plane->enabled)
+			vidi_win_commit(ctx->crtc, i);
 	}
 
-	return 0;
+	mutex_unlock(&ctx->lock);
 }
 
-static void vidi_dpms(struct exynos_drm_crtc *crtc, int mode)
+static void vidi_disable(struct exynos_drm_crtc *crtc)
 {
 	struct vidi_context *ctx = crtc->ctx;
-
-	DRM_DEBUG_KMS("%d\n", mode);
+	struct exynos_drm_plane *plane;
+	int i;
 
 	mutex_lock(&ctx->lock);
 
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		vidi_power_on(ctx, true);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		vidi_power_on(ctx, false);
-		break;
-	default:
-		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
-		break;
-	}
+	ctx->suspended = true;
 
 	mutex_unlock(&ctx->lock);
 }
@@ -219,7 +201,8 @@ static int vidi_ctx_initialize(struct vidi_context *ctx,
 }
 
 static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
-	.dpms = vidi_dpms,
+	.enable = vidi_enable,
+	.disable = vidi_disable,
 	.enable_vblank = vidi_enable_vblank,
 	.disable_vblank = vidi_disable_vblank,
 	.win_commit = vidi_win_commit,
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 8874c1f..6bab717 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1027,8 +1027,9 @@ static void mixer_window_resume(struct mixer_context *ctx)
 	}
 }
 
-static void mixer_poweron(struct mixer_context *ctx)
+static void mixer_enable(struct exynos_drm_crtc *crtc)
 {
+	struct mixer_context *ctx = crtc->ctx;
 	struct mixer_resources *res = &ctx->mixer_res;
 
 	mutex_lock(&ctx->mixer_mutex);
@@ -1061,8 +1062,9 @@ static void mixer_poweron(struct mixer_context *ctx)
 	mixer_window_resume(ctx);
 }
 
-static void mixer_poweroff(struct mixer_context *ctx)
+static void mixer_disable(struct exynos_drm_crtc *crtc)
 {
+	struct mixer_context *ctx = crtc->ctx;
 	struct mixer_resources *res = &ctx->mixer_res;
 
 	mutex_lock(&ctx->mixer_mutex);
@@ -1093,23 +1095,6 @@ static void mixer_poweroff(struct mixer_context *ctx)
 	pm_runtime_put_sync(ctx->dev);
 }
 
-static void mixer_dpms(struct exynos_drm_crtc *crtc, int mode)
-{
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
-		mixer_poweron(crtc->ctx);
-		break;
-	case DRM_MODE_DPMS_STANDBY:
-	case DRM_MODE_DPMS_SUSPEND:
-	case DRM_MODE_DPMS_OFF:
-		mixer_poweroff(crtc->ctx);
-		break;
-	default:
-		DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
-		break;
-	}
-}
-
 /* Only valid for Mixer version 16.0.33.0 */
 int mixer_check_mode(struct drm_display_mode *mode)
 {
@@ -1131,7 +1116,8 @@ int mixer_check_mode(struct drm_display_mode *mode)
 }
 
 static const struct exynos_drm_crtc_ops mixer_crtc_ops = {
-	.dpms			= mixer_dpms,
+	.enable			= mixer_enable,
+	.disable		= mixer_disable,
 	.enable_vblank		= mixer_enable_vblank,
 	.disable_vblank		= mixer_disable_vblank,
 	.wait_for_vblank	= mixer_wait_for_vblank,
-- 
2.1.0

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Inki Dae]

Hi,

On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> To follow more closely the new atomic API we split the dpms()
> helper into the enable() and disable() helper to get exactly the
> same semantics.

Below is the result from checkpatch.pl. Please fix all errors and check
your patch with checkpatch.pl before posting it.

Thanks,
Inki Dae

total: 62 errors, 0 warnings, 410 lines checked


> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 87 ++++++------------------------
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c   |  8 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h    |  6 ++-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 69 +++++-------------------
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 53 +++++++-----------
>  drivers/gpu/drm/exynos/exynos_mixer.c      | 26 +++------
>  6 files changed, 62 insertions(+), 187 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index f29e4be..d659ba2 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -603,75 +603,39 @@ static void decon_init(struct decon_context *ctx)
>  		writel(VIDCON1_VCLK_HOLD, ctx->regs + VIDCON1(0));
>  }
>  
> -static int decon_poweron(struct decon_context *ctx)
> +static void decon_enable(struct exynos_drm_crtc *crtc)
>  {
> -	int ret;
> +	struct decon_context *ctx = crtc->ctx;
>  
>  	if (!ctx->suspended)
> -		return 0;
> +		return;
>  
>  	ctx->suspended = false;
>  
>  	pm_runtime_get_sync(ctx->dev);
>  
> -	ret = clk_prepare_enable(ctx->pclk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
> -		goto pclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->aclk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
> -		goto aclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->eclk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
> -		goto eclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->vclk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
> -		goto vclk_err;
> -	}
> +	clk_prepare_enable(ctx->pclk);
> +	clk_prepare_enable(ctx->aclk);
> +	clk_prepare_enable(ctx->eclk);
> +	clk_prepare_enable(ctx->vclk);
>  
>  	decon_init(ctx);
>  
>  	/* if vblank was enabled status, enable it again. */
> -	if (test_and_clear_bit(0, &ctx->irq_flags)) {
> -		ret = decon_enable_vblank(ctx->crtc);
> -		if (ret) {
> -			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
> -			goto err;
> -		}
> -	}
> +	if (test_and_clear_bit(0, &ctx->irq_flags))
> +		decon_enable_vblank(ctx->crtc);
>  
>  	decon_window_resume(ctx);
>  
>  	decon_apply(ctx);
> -
> -	return 0;
> -
> -err:
> -	clk_disable_unprepare(ctx->vclk);
> -vclk_err:
> -	clk_disable_unprepare(ctx->eclk);
> -eclk_err:
> -	clk_disable_unprepare(ctx->aclk);
> -aclk_err:
> -	clk_disable_unprepare(ctx->pclk);
> -pclk_err:
> -	ctx->suspended = true;
> -	return ret;
>  }
>  
> -static int decon_poweroff(struct decon_context *ctx)
> +static void decon_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct decon_context *ctx = crtc->ctx;
> +
>  	if (ctx->suspended)
> -		return 0;
> +		return;
>  
>  	/*
>  	 * We need to make sure that all windows are disabled before we
> @@ -688,30 +652,11 @@ static int decon_poweroff(struct decon_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  
>  	ctx->suspended = true;
> -	return 0;
> -}
> -
> -static void decon_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		decon_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		decon_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
>  }
>  
>  static const struct exynos_drm_crtc_ops decon_crtc_ops = {
> -	.dpms = decon_dpms,
> +	.enable = decon_enable,
> +	.disable = decon_disable,
>  	.mode_fixup = decon_mode_fixup,
>  	.commit = decon_commit,
>  	.enable_vblank = decon_enable_vblank,
> @@ -796,7 +741,7 @@ static void decon_unbind(struct device *dev, struct device *master,
>  {
>  	struct decon_context *ctx = dev_get_drvdata(dev);
>  
> -	decon_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
> +	decon_disable(ctx->crtc);
>  
>  	if (ctx->display)
>  		exynos_dpi_remove(ctx->display);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b7c6d51..644b4b7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -29,8 +29,8 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>  	if (exynos_crtc->enabled)
>  		return;
>  
> -	if (exynos_crtc->ops->dpms)
> -		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_ON);
> +	if (exynos_crtc->ops->enable)
> +		exynos_crtc->ops->enable(exynos_crtc);
>  
>  	exynos_crtc->enabled = true;
>  
> @@ -51,8 +51,8 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  
>  	drm_crtc_vblank_off(crtc);
>  
> -	if (exynos_crtc->ops->dpms)
> -		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_OFF);
> +	if (exynos_crtc->ops->disable)
> +		exynos_crtc->ops->disable(exynos_crtc);
>  
>  	exynos_crtc->enabled = false;
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 86d6894..1c66f65 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -157,7 +157,8 @@ struct exynos_drm_display {
>  /*
>   * Exynos drm crtc ops
>   *
> - * @dpms: control device power.
> + * @enable: enable the device
> + * @disable: disable the device
>   * @mode_fixup: fix mode data before applying it
>   * @commit: set current hw specific display mode to hw.
>   * @enable_vblank: specific driver callback for enabling vblank interrupt.
> @@ -175,7 +176,8 @@ struct exynos_drm_display {
>   */
>  struct exynos_drm_crtc;
>  struct exynos_drm_crtc_ops {
> -	void (*dpms)(struct exynos_drm_crtc *crtc, int mode);
> +	void (*enable)(struct exynos_drm_crtc *crtc);
> +	void (*disable)(struct exynos_drm_crtc *crtc);
>  	bool (*mode_fixup)(struct exynos_drm_crtc *crtc,
>  				const struct drm_display_mode *mode,
>  				struct drm_display_mode *adjusted_mode);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index b326b31..9661853 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -805,57 +805,35 @@ static void fimd_apply(struct fimd_context *ctx)
>  	fimd_commit(ctx->crtc);
>  }
>  
> -static int fimd_poweron(struct fimd_context *ctx)
> +static void fimd_enable(struct exynos_drm_crtc *crtc)
>  {
> -	int ret;
> +	struct fimd_context *ctx = crtc->ctx;
>  
>  	if (!ctx->suspended)
> -		return 0;
> +		return;
>  
>  	ctx->suspended = false;
>  
>  	pm_runtime_get_sync(ctx->dev);
>  
> -	ret = clk_prepare_enable(ctx->bus_clk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
> -		goto bus_clk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->lcd_clk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n", ret);
> -		goto lcd_clk_err;
> -	}
> +	clk_prepare_enable(ctx->bus_clk);
> +	clk_prepare_enable(ctx->lcd_clk);
>  
>  	/* if vblank was enabled status, enable it again. */
> -	if (test_and_clear_bit(0, &ctx->irq_flags)) {
> -		ret = fimd_enable_vblank(ctx->crtc);
> -		if (ret) {
> -			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
> -			goto enable_vblank_err;
> -		}
> -	}
> +	if (test_and_clear_bit(0, &ctx->irq_flags))
> +		fimd_enable_vblank(ctx->crtc);
>  
>  	fimd_window_resume(ctx);
>  
>  	fimd_apply(ctx);
> -
> -	return 0;
> -
> -enable_vblank_err:
> -	clk_disable_unprepare(ctx->lcd_clk);
> -lcd_clk_err:
> -	clk_disable_unprepare(ctx->bus_clk);
> -bus_clk_err:
> -	ctx->suspended = true;
> -	return ret;
>  }
>  
> -static int fimd_poweroff(struct fimd_context *ctx)
> +static void fimd_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct fimd_context *ctx = crtc->ctx;
> +
>  	if (ctx->suspended)
> -		return 0;
> +		return;
>  
>  	/*
>  	 * We need to make sure that all windows are disabled before we
> @@ -870,26 +848,6 @@ static int fimd_poweroff(struct fimd_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  
>  	ctx->suspended = true;
> -	return 0;
> -}
> -
> -static void fimd_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		fimd_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		fimd_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
>  }
>  
>  static void fimd_trigger(struct device *dev)
> @@ -964,7 +922,8 @@ static void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable)
>  }
>  
>  static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
> -	.dpms = fimd_dpms,
> +	.enable = fimd_enable,
> +	.disable = fimd_disable,
>  	.mode_fixup = fimd_mode_fixup,
>  	.commit = fimd_commit,
>  	.enable_vblank = fimd_enable_vblank,
> @@ -1051,7 +1010,7 @@ static void fimd_unbind(struct device *dev, struct device *master,
>  {
>  	struct fimd_context *ctx = dev_get_drvdata(dev);
>  
> -	fimd_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
> +	fimd_disable(ctx->crtc);
>  
>  	fimd_iommu_detach_devices(ctx);
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 63c1536..abe4ee0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -153,56 +153,38 @@ static void vidi_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>  	/* TODO. */
>  }
>  
> -static int vidi_power_on(struct vidi_context *ctx, bool enable)
> +static void vidi_enable(struct exynos_drm_crtc *crtc)
>  {
> +	struct vidi_context *ctx = crtc->ctx;
>  	struct exynos_drm_plane *plane;
>  	int i;
>  
> -	DRM_DEBUG_KMS("%s\n", __FILE__);
> -
> -	if (enable != false && enable != true)
> -		return -EINVAL;
> +	mutex_lock(&ctx->lock);
>  
> -	if (enable) {
> -		ctx->suspended = false;
> +	ctx->suspended = false;
>  
> -		/* if vblank was enabled status, enable it again. */
> -		if (test_and_clear_bit(0, &ctx->irq_flags))
> -			vidi_enable_vblank(ctx->crtc);
> +	/* if vblank was enabled status, enable it again. */
> +	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);
> -		}
> -	} else {
> -		ctx->suspended = true;
> +	for (i = 0; i < WINDOWS_NR; i++) {
> +		plane = &ctx->planes[i];
> +		if (plane->enabled)
> +			vidi_win_commit(ctx->crtc, i);
>  	}
>  
> -	return 0;
> +	mutex_unlock(&ctx->lock);
>  }
>  
> -static void vidi_dpms(struct exynos_drm_crtc *crtc, int mode)
> +static void vidi_disable(struct exynos_drm_crtc *crtc)
>  {
>  	struct vidi_context *ctx = crtc->ctx;
> -
> -	DRM_DEBUG_KMS("%d\n", mode);
> +	struct exynos_drm_plane *plane;
> +	int i;
>  
>  	mutex_lock(&ctx->lock);
>  
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		vidi_power_on(ctx, true);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		vidi_power_on(ctx, false);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
> +	ctx->suspended = true;
>  
>  	mutex_unlock(&ctx->lock);
>  }
> @@ -219,7 +201,8 @@ static int vidi_ctx_initialize(struct vidi_context *ctx,
>  }
>  
>  static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
> -	.dpms = vidi_dpms,
> +	.enable = vidi_enable,
> +	.disable = vidi_disable,
>  	.enable_vblank = vidi_enable_vblank,
>  	.disable_vblank = vidi_disable_vblank,
>  	.win_commit = vidi_win_commit,
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 8874c1f..6bab717 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -1027,8 +1027,9 @@ static void mixer_window_resume(struct mixer_context *ctx)
>  	}
>  }
>  
> -static void mixer_poweron(struct mixer_context *ctx)
> +static void mixer_enable(struct exynos_drm_crtc *crtc)
>  {
> +	struct mixer_context *ctx = crtc->ctx;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  
>  	mutex_lock(&ctx->mixer_mutex);
> @@ -1061,8 +1062,9 @@ static void mixer_poweron(struct mixer_context *ctx)
>  	mixer_window_resume(ctx);
>  }
>  
> -static void mixer_poweroff(struct mixer_context *ctx)
> +static void mixer_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct mixer_context *ctx = crtc->ctx;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  
>  	mutex_lock(&ctx->mixer_mutex);
> @@ -1093,23 +1095,6 @@ static void mixer_poweroff(struct mixer_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  }
>  
> -static void mixer_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		mixer_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		mixer_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
> -		break;
> -	}
> -}
> -
>  /* Only valid for Mixer version 16.0.33.0 */
>  int mixer_check_mode(struct drm_display_mode *mode)
>  {
> @@ -1131,7 +1116,8 @@ int mixer_check_mode(struct drm_display_mode *mode)
>  }
>  
>  static const struct exynos_drm_crtc_ops mixer_crtc_ops = {
> -	.dpms			= mixer_dpms,
> +	.enable			= mixer_enable,
> +	.disable		= mixer_disable,
>  	.enable_vblank		= mixer_enable_vblank,
>  	.disable_vblank		= mixer_disable_vblank,
>  	.wait_for_vblank	= mixer_wait_for_vblank,
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Gustavo Padovan]

Hi Inki,

2015-06-02 Inki Dae <inki.dae@samsung.com>:

> Hi,
> 
> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > To follow more closely the new atomic API we split the dpms()
> > helper into the enable() and disable() helper to get exactly the
> > same semantics.
> 
> Below is the result from checkpatch.pl. Please fix all errors and check
> your patch with checkpatch.pl before posting it.
> 
> Thanks,
> Inki Dae
> 
> total: 62 errors, 0 warnings, 410 lines checked

I think you did something wrong when checking, for me it looks like
this:

0016-drm-exynos-split-exynos_crtc-dpms-in-enable-and-disa.patch has no
obvious style problems and is ready for submission.
WARNING: Do not use whitespace before Signed-off-by:
#12: 
    Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

total: 0 errors, 1 warnings, 594 lines checked

	Gustavo

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Javier Martinez Canillas]

Hello Gustavo,

On Tue, Jun 2, 2015 at 4:06 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi Inki,
>
> 2015-06-02 Inki Dae <inki.dae@samsung.com>:
>
>> Hi,
>>
>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >
>> > To follow more closely the new atomic API we split the dpms()
>> > helper into the enable() and disable() helper to get exactly the
>> > same semantics.
>>
>> Below is the result from checkpatch.pl. Please fix all errors and check
>> your patch with checkpatch.pl before posting it.
>>
>> Thanks,
>> Inki Dae
>>
>> total: 62 errors, 0 warnings, 410 lines checked
>
> I think you did something wrong when checking, for me it looks like
> this:
>
> 0016-drm-exynos-split-exynos_crtc-dpms-in-enable-and-disa.patch has no
> obvious style problems and is ready for submission.
> WARNING: Do not use whitespace before Signed-off-by:
> #12:
>     Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> total: 0 errors, 1 warnings, 594 lines checked
>

I also don't get any checkpatch.pl error or warnings when testing your patch:

$ pwclient get 6523251
Saved patch to v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch.0

$ ./scripts/checkpatch.pl
v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch
total: 0 errors, 0 warnings, 410 lines checked

v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch
has no obvious style problems and is ready for submission.

>         Gustavo

Best regards,
Javier

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Inki Dae]

On 2015년 06월 02일 23:19, Javier Martinez Canillas wrote:
> Hello Gustavo,
> 
> On Tue, Jun 2, 2015 at 4:06 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
>> Hi Inki,
>>
>> 2015-06-02 Inki Dae <inki.dae@samsung.com>:
>>
>>> Hi,
>>>
>>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> To follow more closely the new atomic API we split the dpms()
>>>> helper into the enable() and disable() helper to get exactly the
>>>> same semantics.
>>>
>>> Below is the result from checkpatch.pl. Please fix all errors and check
>>> your patch with checkpatch.pl before posting it.
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>> total: 62 errors, 0 warnings, 410 lines checked
>>
>> I think you did something wrong when checking, for me it looks like
>> this:
>>
>> 0016-drm-exynos-split-exynos_crtc-dpms-in-enable-and-disa.patch has no
>> obvious style problems and is ready for submission.
>> WARNING: Do not use whitespace before Signed-off-by:
>> #12:
>>     Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> total: 0 errors, 1 warnings, 594 lines checked
>>
> 
> I also don't get any checkpatch.pl error or warnings when testing your patch:
> 
> $ pwclient get 6523251
> Saved patch to v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch.0
> 
> $ ./scripts/checkpatch.pl
> v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch
> total: 0 errors, 0 warnings, 410 lines checked
> 
> v10-17-17-drm-exynos-split-exynos_crtc--dpms-in-enable-and-disable.patch
> has no obvious style problems and is ready for submission.

Yes, there was something wrong. While coping this patch series to my PC,
it seems that the text format of this patch file was mutated.

Thanks for checking,
Inki Dae

> 
>>         Gustavo
> 
> Best regards,
> Javier
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Inki Dae]

Hi,

On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> To follow more closely the new atomic API we split the dpms()
> helper into the enable() and disable() helper to get exactly the
> same semantics.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 87 ++++++------------------------
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c   |  8 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h    |  6 ++-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 69 +++++-------------------
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 53 +++++++-----------
>  drivers/gpu/drm/exynos/exynos_mixer.c      | 26 +++------
>  6 files changed, 62 insertions(+), 187 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index f29e4be..d659ba2 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -603,75 +603,39 @@ static void decon_init(struct decon_context *ctx)
>  		writel(VIDCON1_VCLK_HOLD, ctx->regs + VIDCON1(0));
>  }
>  
> -static int decon_poweron(struct decon_context *ctx)
> +static void decon_enable(struct exynos_drm_crtc *crtc)
>  {
> -	int ret;
> +	struct decon_context *ctx = crtc->ctx;
>  
>  	if (!ctx->suspended)
> -		return 0;
> +		return;
>  
>  	ctx->suspended = false;
>  
>  	pm_runtime_get_sync(ctx->dev);
>  
> -	ret = clk_prepare_enable(ctx->pclk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
> -		goto pclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->aclk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
> -		goto aclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->eclk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
> -		goto eclk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->vclk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
> -		goto vclk_err;
> -	}
> +	clk_prepare_enable(ctx->pclk);
> +	clk_prepare_enable(ctx->aclk);
> +	clk_prepare_enable(ctx->eclk);
> +	clk_prepare_enable(ctx->vclk);

Merged this patch series to exynos-drm-next. However, this patch
especially above codes is required for more clean-up. Even though
decon_enable function never return error number, I think its internal
codes should be considered for some exception cases to check where an
error occurred at. So could you post the clean-up patch?

Thanks,
Inki Dae

>  
>  	decon_init(ctx);
>  
>  	/* if vblank was enabled status, enable it again. */
> -	if (test_and_clear_bit(0, &ctx->irq_flags)) {
> -		ret = decon_enable_vblank(ctx->crtc);
> -		if (ret) {
> -			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
> -			goto err;
> -		}
> -	}
> +	if (test_and_clear_bit(0, &ctx->irq_flags))
> +		decon_enable_vblank(ctx->crtc);
>  
>  	decon_window_resume(ctx);
>  
>  	decon_apply(ctx);
> -
> -	return 0;
> -
> -err:
> -	clk_disable_unprepare(ctx->vclk);
> -vclk_err:
> -	clk_disable_unprepare(ctx->eclk);
> -eclk_err:
> -	clk_disable_unprepare(ctx->aclk);
> -aclk_err:
> -	clk_disable_unprepare(ctx->pclk);
> -pclk_err:
> -	ctx->suspended = true;
> -	return ret;
>  }
>  
> -static int decon_poweroff(struct decon_context *ctx)
> +static void decon_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct decon_context *ctx = crtc->ctx;
> +
>  	if (ctx->suspended)
> -		return 0;
> +		return;
>  
>  	/*
>  	 * We need to make sure that all windows are disabled before we
> @@ -688,30 +652,11 @@ static int decon_poweroff(struct decon_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  
>  	ctx->suspended = true;
> -	return 0;
> -}
> -
> -static void decon_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		decon_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		decon_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
>  }
>  
>  static const struct exynos_drm_crtc_ops decon_crtc_ops = {
> -	.dpms = decon_dpms,
> +	.enable = decon_enable,
> +	.disable = decon_disable,
>  	.mode_fixup = decon_mode_fixup,
>  	.commit = decon_commit,
>  	.enable_vblank = decon_enable_vblank,
> @@ -796,7 +741,7 @@ static void decon_unbind(struct device *dev, struct device *master,
>  {
>  	struct decon_context *ctx = dev_get_drvdata(dev);
>  
> -	decon_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
> +	decon_disable(ctx->crtc);
>  
>  	if (ctx->display)
>  		exynos_dpi_remove(ctx->display);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b7c6d51..644b4b7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -29,8 +29,8 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>  	if (exynos_crtc->enabled)
>  		return;
>  
> -	if (exynos_crtc->ops->dpms)
> -		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_ON);
> +	if (exynos_crtc->ops->enable)
> +		exynos_crtc->ops->enable(exynos_crtc);
>  
>  	exynos_crtc->enabled = true;
>  
> @@ -51,8 +51,8 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  
>  	drm_crtc_vblank_off(crtc);
>  
> -	if (exynos_crtc->ops->dpms)
> -		exynos_crtc->ops->dpms(exynos_crtc, DRM_MODE_DPMS_OFF);
> +	if (exynos_crtc->ops->disable)
> +		exynos_crtc->ops->disable(exynos_crtc);
>  
>  	exynos_crtc->enabled = false;
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 86d6894..1c66f65 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -157,7 +157,8 @@ struct exynos_drm_display {
>  /*
>   * Exynos drm crtc ops
>   *
> - * @dpms: control device power.
> + * @enable: enable the device
> + * @disable: disable the device
>   * @mode_fixup: fix mode data before applying it
>   * @commit: set current hw specific display mode to hw.
>   * @enable_vblank: specific driver callback for enabling vblank interrupt.
> @@ -175,7 +176,8 @@ struct exynos_drm_display {
>   */
>  struct exynos_drm_crtc;
>  struct exynos_drm_crtc_ops {
> -	void (*dpms)(struct exynos_drm_crtc *crtc, int mode);
> +	void (*enable)(struct exynos_drm_crtc *crtc);
> +	void (*disable)(struct exynos_drm_crtc *crtc);
>  	bool (*mode_fixup)(struct exynos_drm_crtc *crtc,
>  				const struct drm_display_mode *mode,
>  				struct drm_display_mode *adjusted_mode);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index b326b31..9661853 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -805,57 +805,35 @@ static void fimd_apply(struct fimd_context *ctx)
>  	fimd_commit(ctx->crtc);
>  }
>  
> -static int fimd_poweron(struct fimd_context *ctx)
> +static void fimd_enable(struct exynos_drm_crtc *crtc)
>  {
> -	int ret;
> +	struct fimd_context *ctx = crtc->ctx;
>  
>  	if (!ctx->suspended)
> -		return 0;
> +		return;
>  
>  	ctx->suspended = false;
>  
>  	pm_runtime_get_sync(ctx->dev);
>  
> -	ret = clk_prepare_enable(ctx->bus_clk);
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
> -		goto bus_clk_err;
> -	}
> -
> -	ret = clk_prepare_enable(ctx->lcd_clk);
> -	if  (ret < 0) {
> -		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n", ret);
> -		goto lcd_clk_err;
> -	}
> +	clk_prepare_enable(ctx->bus_clk);
> +	clk_prepare_enable(ctx->lcd_clk);
>  
>  	/* if vblank was enabled status, enable it again. */
> -	if (test_and_clear_bit(0, &ctx->irq_flags)) {
> -		ret = fimd_enable_vblank(ctx->crtc);
> -		if (ret) {
> -			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
> -			goto enable_vblank_err;
> -		}
> -	}
> +	if (test_and_clear_bit(0, &ctx->irq_flags))
> +		fimd_enable_vblank(ctx->crtc);
>  
>  	fimd_window_resume(ctx);
>  
>  	fimd_apply(ctx);
> -
> -	return 0;
> -
> -enable_vblank_err:
> -	clk_disable_unprepare(ctx->lcd_clk);
> -lcd_clk_err:
> -	clk_disable_unprepare(ctx->bus_clk);
> -bus_clk_err:
> -	ctx->suspended = true;
> -	return ret;
>  }
>  
> -static int fimd_poweroff(struct fimd_context *ctx)
> +static void fimd_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct fimd_context *ctx = crtc->ctx;
> +
>  	if (ctx->suspended)
> -		return 0;
> +		return;
>  
>  	/*
>  	 * We need to make sure that all windows are disabled before we
> @@ -870,26 +848,6 @@ static int fimd_poweroff(struct fimd_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  
>  	ctx->suspended = true;
> -	return 0;
> -}
> -
> -static void fimd_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		fimd_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		fimd_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
>  }
>  
>  static void fimd_trigger(struct device *dev)
> @@ -964,7 +922,8 @@ static void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable)
>  }
>  
>  static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
> -	.dpms = fimd_dpms,
> +	.enable = fimd_enable,
> +	.disable = fimd_disable,
>  	.mode_fixup = fimd_mode_fixup,
>  	.commit = fimd_commit,
>  	.enable_vblank = fimd_enable_vblank,
> @@ -1051,7 +1010,7 @@ static void fimd_unbind(struct device *dev, struct device *master,
>  {
>  	struct fimd_context *ctx = dev_get_drvdata(dev);
>  
> -	fimd_dpms(ctx->crtc, DRM_MODE_DPMS_OFF);
> +	fimd_disable(ctx->crtc);
>  
>  	fimd_iommu_detach_devices(ctx);
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 63c1536..abe4ee0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -153,56 +153,38 @@ static void vidi_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
>  	/* TODO. */
>  }
>  
> -static int vidi_power_on(struct vidi_context *ctx, bool enable)
> +static void vidi_enable(struct exynos_drm_crtc *crtc)
>  {
> +	struct vidi_context *ctx = crtc->ctx;
>  	struct exynos_drm_plane *plane;
>  	int i;
>  
> -	DRM_DEBUG_KMS("%s\n", __FILE__);
> -
> -	if (enable != false && enable != true)
> -		return -EINVAL;
> +	mutex_lock(&ctx->lock);
>  
> -	if (enable) {
> -		ctx->suspended = false;
> +	ctx->suspended = false;
>  
> -		/* if vblank was enabled status, enable it again. */
> -		if (test_and_clear_bit(0, &ctx->irq_flags))
> -			vidi_enable_vblank(ctx->crtc);
> +	/* if vblank was enabled status, enable it again. */
> +	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);
> -		}
> -	} else {
> -		ctx->suspended = true;
> +	for (i = 0; i < WINDOWS_NR; i++) {
> +		plane = &ctx->planes[i];
> +		if (plane->enabled)
> +			vidi_win_commit(ctx->crtc, i);
>  	}
>  
> -	return 0;
> +	mutex_unlock(&ctx->lock);
>  }
>  
> -static void vidi_dpms(struct exynos_drm_crtc *crtc, int mode)
> +static void vidi_disable(struct exynos_drm_crtc *crtc)
>  {
>  	struct vidi_context *ctx = crtc->ctx;
> -
> -	DRM_DEBUG_KMS("%d\n", mode);
> +	struct exynos_drm_plane *plane;
> +	int i;
>  
>  	mutex_lock(&ctx->lock);
>  
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		vidi_power_on(ctx, true);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		vidi_power_on(ctx, false);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
> +	ctx->suspended = true;
>  
>  	mutex_unlock(&ctx->lock);
>  }
> @@ -219,7 +201,8 @@ static int vidi_ctx_initialize(struct vidi_context *ctx,
>  }
>  
>  static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
> -	.dpms = vidi_dpms,
> +	.enable = vidi_enable,
> +	.disable = vidi_disable,
>  	.enable_vblank = vidi_enable_vblank,
>  	.disable_vblank = vidi_disable_vblank,
>  	.win_commit = vidi_win_commit,
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 8874c1f..6bab717 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -1027,8 +1027,9 @@ static void mixer_window_resume(struct mixer_context *ctx)
>  	}
>  }
>  
> -static void mixer_poweron(struct mixer_context *ctx)
> +static void mixer_enable(struct exynos_drm_crtc *crtc)
>  {
> +	struct mixer_context *ctx = crtc->ctx;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  
>  	mutex_lock(&ctx->mixer_mutex);
> @@ -1061,8 +1062,9 @@ static void mixer_poweron(struct mixer_context *ctx)
>  	mixer_window_resume(ctx);
>  }
>  
> -static void mixer_poweroff(struct mixer_context *ctx)
> +static void mixer_disable(struct exynos_drm_crtc *crtc)
>  {
> +	struct mixer_context *ctx = crtc->ctx;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  
>  	mutex_lock(&ctx->mixer_mutex);
> @@ -1093,23 +1095,6 @@ static void mixer_poweroff(struct mixer_context *ctx)
>  	pm_runtime_put_sync(ctx->dev);
>  }
>  
> -static void mixer_dpms(struct exynos_drm_crtc *crtc, int mode)
> -{
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		mixer_poweron(crtc->ctx);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		mixer_poweroff(crtc->ctx);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unknown dpms mode: %d\n", mode);
> -		break;
> -	}
> -}
> -
>  /* Only valid for Mixer version 16.0.33.0 */
>  int mixer_check_mode(struct drm_display_mode *mode)
>  {
> @@ -1131,7 +1116,8 @@ int mixer_check_mode(struct drm_display_mode *mode)
>  }
>  
>  static const struct exynos_drm_crtc_ops mixer_crtc_ops = {
> -	.dpms			= mixer_dpms,
> +	.enable			= mixer_enable,
> +	.disable		= mixer_disable,
>  	.enable_vblank		= mixer_enable_vblank,
>  	.disable_vblank		= mixer_disable_vblank,
>  	.wait_for_vblank	= mixer_wait_for_vblank,
> 

Re: [PATCH v10 17/17] drm/exynos: split exynos_crtc->dpms in enable() and disable()

[Gustavo Padovan]

Hi Inki,

2015-06-03 Inki Dae <inki.dae@samsung.com>:

> Hi,
> 
> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > To follow more closely the new atomic API we split the dpms()
> > helper into the enable() and disable() helper to get exactly the
> > same semantics.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 87 ++++++------------------------
> >  drivers/gpu/drm/exynos/exynos_drm_crtc.c   |  8 +--
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h    |  6 ++-
> >  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 69 +++++-------------------
> >  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 53 +++++++-----------
> >  drivers/gpu/drm/exynos/exynos_mixer.c      | 26 +++------
> >  6 files changed, 62 insertions(+), 187 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> > index f29e4be..d659ba2 100644
> > --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> > +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> > @@ -603,75 +603,39 @@ static void decon_init(struct decon_context *ctx)
> >  		writel(VIDCON1_VCLK_HOLD, ctx->regs + VIDCON1(0));
> >  }
> >  
> > -static int decon_poweron(struct decon_context *ctx)
> > +static void decon_enable(struct exynos_drm_crtc *crtc)
> >  {
> > -	int ret;
> > +	struct decon_context *ctx = crtc->ctx;
> >  
> >  	if (!ctx->suspended)
> > -		return 0;
> > +		return;
> >  
> >  	ctx->suspended = false;
> >  
> >  	pm_runtime_get_sync(ctx->dev);
> >  
> > -	ret = clk_prepare_enable(ctx->pclk);
> > -	if (ret < 0) {
> > -		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
> > -		goto pclk_err;
> > -	}
> > -
> > -	ret = clk_prepare_enable(ctx->aclk);
> > -	if (ret < 0) {
> > -		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
> > -		goto aclk_err;
> > -	}
> > -
> > -	ret = clk_prepare_enable(ctx->eclk);
> > -	if  (ret < 0) {
> > -		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
> > -		goto eclk_err;
> > -	}
> > -
> > -	ret = clk_prepare_enable(ctx->vclk);
> > -	if  (ret < 0) {
> > -		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
> > -		goto vclk_err;
> > -	}
> > +	clk_prepare_enable(ctx->pclk);
> > +	clk_prepare_enable(ctx->aclk);
> > +	clk_prepare_enable(ctx->eclk);
> > +	clk_prepare_enable(ctx->vclk);
> 
> Merged this patch series to exynos-drm-next. However, this patch
> especially above codes is required for more clean-up. Even though
> decon_enable function never return error number, I think its internal
> codes should be considered for some exception cases to check where an
> error occurred at. So could you post the clean-up patch?

Thanks for merging the patches! I will send follow-up patches shortly to
add checks and errors messages back.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Marek Szyprowski]

Hello,

On 2015-06-01 17:04, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> Hi,
>
> Here goes the full support for atomic modesetting on exynos. I've
> split the patches in the various phases of atomic support.

Thanks for this patchses, however I've noticed a problem after applying 
them.
The issue gets revealed when support for IOMMU is enabled. I've did my tests
with Exynos HDMI driver on Odroid U3 board.

To demonstrated the issue I've added following additional debug in the 
exynos
mixer driver in mixer_graph_buffer() function:
pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n", 
&plane->dma_addr[0], plane->src_width, plane->src_height);

Before applying patches setting 640x480 mode and getting back to 1920x1080
console generates following log:

# modetest -M exynos -s 23:640x480
setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
[ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
^C
[ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
[ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080

After applying atomic modesetting patchset:
# modetest -M exynos -s 24:640x480
[  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
[  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
[  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
[  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
^C
[  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080

As you can see from the above log, mixer_graph_buffer function is called
several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
mixer_graph_buffer() is first called with the new DMA address of the
framebuffer, but with the old mode parameters (1920x1080 size) and then
in the next call it updates the plane parameters to the correct values
(size changed to 640x480). When IOMMU is not used, this can be easily
missed, but after enabling IOMMU support, any DMA access to unallocated
address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
address of the buffer without changing the size.

A quick workaround to resolve this multiple calls to mixer_graph_buffer()
with partially updated mode values is to remove calls to
mixer_window_suspend/mixer_window_resume from mixer_disable and
mixer_disable functions, but I expect that this is not the right
approach.

Probably the same problem can be observed with Exynos FIMD driver.

Gustavo: could you check if mixer_enable functions should really
call mixer_window_resume function, which in turn calls mixer_win_commit,
which calls mixer_graph_buffer with partially updated display buffer
data?

 > (...)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

Hi Marek,

On 2015년 06월 10일 19:03, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-06-01 17:04, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> Hi,
>>
>> Here goes the full support for atomic modesetting on exynos. I've
>> split the patches in the various phases of atomic support.
> 
> Thanks for this patchses, however I've noticed a problem after applying
> them.
> The issue gets revealed when support for IOMMU is enabled. I've did my
> tests
> with Exynos HDMI driver on Odroid U3 board.
> 
> To demonstrated the issue I've added following additional debug in the
> exynos
> mixer driver in mixer_graph_buffer() function:
> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
> &plane->dma_addr[0], plane->src_width, plane->src_height);
> 
> Before applying patches setting 640x480 mode and getting back to 1920x1080
> console generates following log:
> 
> # modetest -M exynos -s 23:640x480
> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> ^C
> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> 
> After applying atomic modesetting patchset:
> # modetest -M exynos -s 24:640x480
> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> ^C
> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> 
> As you can see from the above log, mixer_graph_buffer function is called
> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
> mixer_graph_buffer() is first called with the new DMA address of the
> framebuffer, but with the old mode parameters (1920x1080 size) and then
> in the next call it updates the plane parameters to the correct values
> (size changed to 640x480). When IOMMU is not used, this can be easily
> missed, but after enabling IOMMU support, any DMA access to unallocated
> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
> address of the buffer without changing the size.
> 
> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
> with partially updated mode values is to remove calls to
> mixer_window_suspend/mixer_window_resume from mixer_disable and
> mixer_disable functions, but I expect that this is not the right
> approach.
> 
> Probably the same problem can be observed with Exynos FIMD driver.
> 
> Gustavo: could you check if mixer_enable functions should really
> call mixer_window_resume function, which in turn calls mixer_win_commit,
> which calls mixer_graph_buffer with partially updated display buffer
> data?

Marek, can you share how other people can test the atomic feature with
iommu?

I should have merged below several patches and added device tree
relevant codes to test iommu.

1. Merged iommu support patches for Exynos SoC below iommu exynos tree,

https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/exynos

2. Merged below patch,
    [PATCH v7 24/25] ARM: DMA-mapping: add
support for creating reserved mappings in iova space

3. Added device node relevant codes - I tested Exynos drm on trats2
board based on Exynos4412 SoC - like below,
in exynos4.dtsi file:
fimd: fimd@11c00000 {
                 ...
               iommus = <&sysmmu_fimd0>;
                 ...

sysmmu_fimd0: sysmmu@11E20000 {
        compatible = "samsung,exynos-sysmmu";
        reg = <0x11E20000 0x1000>;
        interrupt-parent = <&combiner>;
        interrupts = <5 2>;
        clock-names = "sysmmu", "master";
        clocks = <&clock CLK_SMMU_FIMD0>, <&clock CLK_FIMD0>;
         power-domains = <&pd_lcd0>;
        #iommu-cells = <0>;
};

in exynos4412-trats2.dts file:
fimd@11c00000 {
                status = "okay";
                iommu-reserved-mapping = <0x40000000 0x40000000 0x40000000>;
};

Is that all? You would need to share exact guide about iommu enabling to
other people so that they can test atomic feature with iommu.

Thanks,
Inki Dae

> 
>> (...)
> 
> Best regards

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Marek Szyprowski]

Hello,

On 2015-06-10 12:59, Inki Dae wrote:
> Hi Marek,
>
> On 2015년 06월 10일 19:03, Marek Szyprowski wrote:
>> Hello,
>>
>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Hi,
>>>
>>> Here goes the full support for atomic modesetting on exynos. I've
>>> split the patches in the various phases of atomic support.
>> Thanks for this patchses, however I've noticed a problem after applying
>> them.
>> The issue gets revealed when support for IOMMU is enabled. I've did my
>> tests
>> with Exynos HDMI driver on Odroid U3 board.
>>
>> To demonstrated the issue I've added following additional debug in the
>> exynos
>> mixer driver in mixer_graph_buffer() function:
>> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
>> &plane->dma_addr[0], plane->src_width, plane->src_height);
>>
>> Before applying patches setting 640x480 mode and getting back to 1920x1080
>> console generates following log:
>>
>> # modetest -M exynos -s 23:640x480
>> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
>> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>> ^C
>> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>
>> After applying atomic modesetting patchset:
>> # modetest -M exynos -s 24:640x480
>> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
>> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
>> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>> ^C
>> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>
>> As you can see from the above log, mixer_graph_buffer function is called
>> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
>> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
>> mixer_graph_buffer() is first called with the new DMA address of the
>> framebuffer, but with the old mode parameters (1920x1080 size) and then
>> in the next call it updates the plane parameters to the correct values
>> (size changed to 640x480). When IOMMU is not used, this can be easily
>> missed, but after enabling IOMMU support, any DMA access to unallocated
>> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
>> address of the buffer without changing the size.
>>
>> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
>> with partially updated mode values is to remove calls to
>> mixer_window_suspend/mixer_window_resume from mixer_disable and
>> mixer_disable functions, but I expect that this is not the right
>> approach.
>>
>> Probably the same problem can be observed with Exynos FIMD driver.
>>
>> Gustavo: could you check if mixer_enable functions should really
>> call mixer_window_resume function, which in turn calls mixer_win_commit,
>> which calls mixer_graph_buffer with partially updated display buffer
>> data?
> Marek, can you share how other people can test the atomic feature with
> iommu?
>
> I should have merged below several patches and added device tree
> relevant codes to test iommu.
>
> 1. Merged iommu support patches for Exynos SoC below iommu exynos tree,
>
> https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/exynos
>
> 2. Merged below patch,
>      [PATCH v7 24/25] ARM: DMA-mapping: add
> support for creating reserved mappings in iova space
>
> 3. Added device node relevant codes - I tested Exynos drm on trats2
> board based on Exynos4412 SoC - like below,
> in exynos4.dtsi file:
> fimd: fimd@11c00000 {
>                   ...
>                 iommus = <&sysmmu_fimd0>;
>                   ...
>
> sysmmu_fimd0: sysmmu@11E20000 {
>          compatible = "samsung,exynos-sysmmu";
>          reg = <0x11E20000 0x1000>;
>          interrupt-parent = <&combiner>;
>          interrupts = <5 2>;
>          clock-names = "sysmmu", "master";
>          clocks = <&clock CLK_SMMU_FIMD0>, <&clock CLK_FIMD0>;
>           power-domains = <&pd_lcd0>;
>          #iommu-cells = <0>;
> };
>
> in exynos4412-trats2.dts file:
> fimd@11c00000 {
>                  status = "okay";
>                  iommu-reserved-mapping = <0x40000000 0x40000000 0x40000000>;
> };
>
> Is that all? You would need to share exact guide about iommu enabling to
> other people so that they can test atomic feature with iommu.

Right, the above should be enough. For convenience I've prepared a 
branch with all needed patches:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git 
v4.1-exynos-drm-iommu

I've included following branches:
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/log/?h=exynos-drm/for-next
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=v4.2-next/dt-samsung-4th
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=v4.2-next/mach-samsung
https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/exynos

Then I've applied my Exynos DRM patches 
(http://www.spinics.net/lists/linux-samsung-soc/msg45114.html)
as well as rebased patch 24/25 and 25/25 from my initial v7 IOMMU series
(http://www.spinics.net/lists/linux-samsung-soc/msg44652.html).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

On 2015년 06월 10일 20:38, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-06-10 12:59, Inki Dae wrote:
>> Hi Marek,
>>
>> On 2015년 06월 10일 19:03, Marek Szyprowski wrote:
>>> Hello,
>>>
>>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> Hi,
>>>>
>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>> split the patches in the various phases of atomic support.
>>> Thanks for this patchses, however I've noticed a problem after applying
>>> them.
>>> The issue gets revealed when support for IOMMU is enabled. I've did my
>>> tests
>>> with Exynos HDMI driver on Odroid U3 board.
>>>
>>> To demonstrated the issue I've added following additional debug in the
>>> exynos
>>> mixer driver in mixer_graph_buffer() function:
>>> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
>>> &plane->dma_addr[0], plane->src_width, plane->src_height);
>>>
>>> Before applying patches setting 640x480 mode and getting back to
>>> 1920x1080
>>> console generates following log:
>>>
>>> # modetest -M exynos -s 23:640x480
>>> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
>>> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>> ^C
>>> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height
>>> 1080
>>> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height
>>> 1080
>>>
>>> After applying atomic modesetting patchset:
>>> # modetest -M exynos -s 24:640x480
>>> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height
>>> 1080
>>> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height
>>> 1080
>>> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
>>> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height
>>> 1080
>>> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>> ^C
>>> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height
>>> 1080
>>>
>>> As you can see from the above log, mixer_graph_buffer function is called
>>> several times. 0xbbd00000 is the DMA address of the 1920x1080
>>> framebuffer
>>> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
>>> mixer_graph_buffer() is first called with the new DMA address of the
>>> framebuffer, but with the old mode parameters (1920x1080 size) and then
>>> in the next call it updates the plane parameters to the correct values
>>> (size changed to 640x480). When IOMMU is not used, this can be easily
>>> missed, but after enabling IOMMU support, any DMA access to unallocated
>>> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
>>> address of the buffer without changing the size.
>>>
>>> A quick workaround to resolve this multiple calls to
>>> mixer_graph_buffer()
>>> with partially updated mode values is to remove calls to
>>> mixer_window_suspend/mixer_window_resume from mixer_disable and
>>> mixer_disable functions, but I expect that this is not the right
>>> approach.
>>>
>>> Probably the same problem can be observed with Exynos FIMD driver.
>>>
>>> Gustavo: could you check if mixer_enable functions should really
>>> call mixer_window_resume function, which in turn calls mixer_win_commit,
>>> which calls mixer_graph_buffer with partially updated display buffer
>>> data?
>> Marek, can you share how other people can test the atomic feature with
>> iommu?
>>
>> I should have merged below several patches and added device tree
>> relevant codes to test iommu.
>>
>> 1. Merged iommu support patches for Exynos SoC below iommu exynos tree,
>>
>> https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/exynos
>>
>>
>> 2. Merged below patch,
>>      [PATCH v7 24/25] ARM: DMA-mapping: add
>> support for creating reserved mappings in iova space
>>
>> 3. Added device node relevant codes - I tested Exynos drm on trats2
>> board based on Exynos4412 SoC - like below,
>> in exynos4.dtsi file:
>> fimd: fimd@11c00000 {
>>                   ...
>>                 iommus = <&sysmmu_fimd0>;
>>                   ...
>>
>> sysmmu_fimd0: sysmmu@11E20000 {
>>          compatible = "samsung,exynos-sysmmu";
>>          reg = <0x11E20000 0x1000>;
>>          interrupt-parent = <&combiner>;
>>          interrupts = <5 2>;
>>          clock-names = "sysmmu", "master";
>>          clocks = <&clock CLK_SMMU_FIMD0>, <&clock CLK_FIMD0>;
>>           power-domains = <&pd_lcd0>;
>>          #iommu-cells = <0>;
>> };
>>
>> in exynos4412-trats2.dts file:
>> fimd@11c00000 {
>>                  status = "okay";
>>                  iommu-reserved-mapping = <0x40000000 0x40000000
>> 0x40000000>;
>> };
>>
>> Is that all? You would need to share exact guide about iommu enabling to
>> other people so that they can test atomic feature with iommu.
> 
> Right, the above should be enough. For convenience I've prepared a
> branch with all needed patches:
> https://git.linaro.org/people/marek.szyprowski/linux-srpol.git
> v4.1-exynos-drm-iommu
> 
> I've included following branches:
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/log/?h=exynos-drm/for-next
> 
> https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=v4.2-next/dt-samsung-4th
> 
> https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/log/?h=v4.2-next/mach-samsung
> 
> https://git.kernel.org/cgit/linux/kernel/git/joro/iommu.git/log/?h=arm/exynos
> 
> 
> Then I've applied my Exynos DRM patches
> (http://www.spinics.net/lists/linux-samsung-soc/msg45114.html)
> as well as rebased patch 24/25 and 25/25 from my initial v7 IOMMU series
> (http://www.spinics.net/lists/linux-samsung-soc/msg44652.html).
> 

Thanks, Marek.

Gustavo and Joonyoung, let's resolve the page fault issue with iommu
this time. I'd like to merge most patch sets - atomic feature, iommu,
Exynos5433 IP support, and driver initialization consolidating. I should
have pull-request in the near feature. So we need to do this in a hurry.

Thanks,
Inki Dae

> Best regards

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Gustavo Padovan]

Hi Marek,

2015-06-10 Marek Szyprowski <m.szyprowski@samsung.com>:

> Hello,
> 
> On 2015-06-01 17:04, Gustavo Padovan wrote:
> >From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> >Hi,
> >
> >Here goes the full support for atomic modesetting on exynos. I've
> >split the patches in the various phases of atomic support.
> 
> Thanks for this patchses, however I've noticed a problem after applying
> them.
> The issue gets revealed when support for IOMMU is enabled. I've did my tests
> with Exynos HDMI driver on Odroid U3 board.
> 
> To demonstrated the issue I've added following additional debug in the
> exynos
> mixer driver in mixer_graph_buffer() function:
> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
> &plane->dma_addr[0], plane->src_width, plane->src_height);
> 
> Before applying patches setting 640x480 mode and getting back to 1920x1080
> console generates following log:
> 
> # modetest -M exynos -s 23:640x480
> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> ^C
> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> 
> After applying atomic modesetting patchset:
> # modetest -M exynos -s 24:640x480
> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> ^C
> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> 
> As you can see from the above log, mixer_graph_buffer function is called
> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
> mixer_graph_buffer() is first called with the new DMA address of the
> framebuffer, but with the old mode parameters (1920x1080 size) and then
> in the next call it updates the plane parameters to the correct values
> (size changed to 640x480). When IOMMU is not used, this can be easily
> missed, but after enabling IOMMU support, any DMA access to unallocated
> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
> address of the buffer without changing the size.
> 
> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
> with partially updated mode values is to remove calls to
> mixer_window_suspend/mixer_window_resume from mixer_disable and
> mixer_disable functions, but I expect that this is not the right
> approach.
> 
> Probably the same problem can be observed with Exynos FIMD driver.
> 
> Gustavo: could you check if mixer_enable functions should really
> call mixer_window_resume function, which in turn calls mixer_win_commit,
> which calls mixer_graph_buffer with partially updated display buffer
> data?

It should not, you are right. This is actually the correct fix. Atomic
modesetting should not do chained calls, e.g., crtc_disable calling
plane_disable. This change was already in my plan actually, but as I had
IOMMU disabled I didn't see it here, and I didn't create this patch yet.

	Gustavo

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Joonyoung Shim]

On 06/10/2015 10:36 PM, Gustavo Padovan wrote:
> Hi Marek,
> 
> 2015-06-10 Marek Szyprowski <m.szyprowski@samsung.com>:
> 
>> Hello,
>>
>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Hi,
>>>
>>> Here goes the full support for atomic modesetting on exynos. I've
>>> split the patches in the various phases of atomic support.
>>
>> Thanks for this patchses, however I've noticed a problem after applying
>> them.
>> The issue gets revealed when support for IOMMU is enabled. I've did my tests
>> with Exynos HDMI driver on Odroid U3 board.
>>
>> To demonstrated the issue I've added following additional debug in the
>> exynos
>> mixer driver in mixer_graph_buffer() function:
>> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
>> &plane->dma_addr[0], plane->src_width, plane->src_height);
>>
>> Before applying patches setting 640x480 mode and getting back to 1920x1080
>> console generates following log:
>>
>> # modetest -M exynos -s 23:640x480
>> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
>> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>> ^C
>> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>
>> After applying atomic modesetting patchset:
>> # modetest -M exynos -s 24:640x480
>> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
>> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
>> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>> ^C
>> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>
>> As you can see from the above log, mixer_graph_buffer function is called
>> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
>> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
>> mixer_graph_buffer() is first called with the new DMA address of the
>> framebuffer, but with the old mode parameters (1920x1080 size) and then
>> in the next call it updates the plane parameters to the correct values
>> (size changed to 640x480). When IOMMU is not used, this can be easily
>> missed, but after enabling IOMMU support, any DMA access to unallocated
>> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
>> address of the buffer without changing the size.
>>
>> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
>> with partially updated mode values is to remove calls to
>> mixer_window_suspend/mixer_window_resume from mixer_disable and
>> mixer_disable functions, but I expect that this is not the right
>> approach.
>>
>> Probably the same problem can be observed with Exynos FIMD driver.
>>
>> Gustavo: could you check if mixer_enable functions should really
>> call mixer_window_resume function, which in turn calls mixer_win_commit,
>> which calls mixer_graph_buffer with partially updated display buffer
>> data?
> 
> It should not, you are right. This is actually the correct fix. Atomic
> modesetting should not do chained calls, e.g., crtc_disable calling
> plane_disable. This change was already in my plan actually, but as I had
> IOMMU disabled I didn't see it here, and I didn't create this patch yet.
> 

Right, but it needs that crtc_disable calls plane_disable in exynos
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.

I think a solution is enough only to eliminate calling xxx_resume
function in exynos driver function when enable plane, we can remove it
because it's called to enable plane from drm atomic modeset framework.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Gustavo Padovan]

Hi Joonyoung,

2015-06-11 Joonyoung Shim <jy0922.shim@samsung.com>:

> On 06/10/2015 10:36 PM, Gustavo Padovan wrote:
> > Hi Marek,
> > 
> > 2015-06-10 Marek Szyprowski <m.szyprowski@samsung.com>:
> > 
> >> Hello,
> >>
> >> On 2015-06-01 17:04, Gustavo Padovan wrote:
> >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>
> >>> Hi,
> >>>
> >>> Here goes the full support for atomic modesetting on exynos. I've
> >>> split the patches in the various phases of atomic support.
> >>
> >> Thanks for this patchses, however I've noticed a problem after applying
> >> them.
> >> The issue gets revealed when support for IOMMU is enabled. I've did my tests
> >> with Exynos HDMI driver on Odroid U3 board.
> >>
> >> To demonstrated the issue I've added following additional debug in the
> >> exynos
> >> mixer driver in mixer_graph_buffer() function:
> >> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
> >> &plane->dma_addr[0], plane->src_width, plane->src_height);
> >>
> >> Before applying patches setting 640x480 mode and getting back to 1920x1080
> >> console generates following log:
> >>
> >> # modetest -M exynos -s 23:640x480
> >> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
> >> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> >> ^C
> >> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> >> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> >>
> >> After applying atomic modesetting patchset:
> >> # modetest -M exynos -s 24:640x480
> >> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> >> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> >> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
> >> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
> >> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
> >> ^C
> >> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
> >>
> >> As you can see from the above log, mixer_graph_buffer function is called
> >> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
> >> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
> >> mixer_graph_buffer() is first called with the new DMA address of the
> >> framebuffer, but with the old mode parameters (1920x1080 size) and then
> >> in the next call it updates the plane parameters to the correct values
> >> (size changed to 640x480). When IOMMU is not used, this can be easily
> >> missed, but after enabling IOMMU support, any DMA access to unallocated
> >> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
> >> address of the buffer without changing the size.
> >>
> >> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
> >> with partially updated mode values is to remove calls to
> >> mixer_window_suspend/mixer_window_resume from mixer_disable and
> >> mixer_disable functions, but I expect that this is not the right
> >> approach.
> >>
> >> Probably the same problem can be observed with Exynos FIMD driver.
> >>
> >> Gustavo: could you check if mixer_enable functions should really
> >> call mixer_window_resume function, which in turn calls mixer_win_commit,
> >> which calls mixer_graph_buffer with partially updated display buffer
> >> data?
> > 
> > It should not, you are right. This is actually the correct fix. Atomic
> > modesetting should not do chained calls, e.g., crtc_disable calling
> > plane_disable. This change was already in my plan actually, but as I had
> > IOMMU disabled I didn't see it here, and I didn't create this patch yet.
> > 
> 
> Right, but it needs that crtc_disable calls plane_disable in exynos
> 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.
> 
> I think a solution is enough only to eliminate calling xxx_resume
> function in exynos driver function when enable plane, we can remove it
> because it's called to enable plane from drm atomic modeset framework.

I think this should work. We really need to disable the planes before
the crtc is disabled, for FIMD for example, the overlay planes are
removed from the screen if we remove the _suspend call.

I'll sent a new patch to remove only the resume part from all our crtc
drivers.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Joonyoung Shim]

On 06/11/2015 11:01 PM, Gustavo Padovan wrote:
> Hi Joonyoung,
> 
> 2015-06-11 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> On 06/10/2015 10:36 PM, Gustavo Padovan wrote:
>>> Hi Marek,
>>>
>>> 2015-06-10 Marek Szyprowski <m.szyprowski@samsung.com>:
>>>
>>>> Hello,
>>>>
>>>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>>> split the patches in the various phases of atomic support.
>>>>
>>>> Thanks for this patchses, however I've noticed a problem after applying
>>>> them.
>>>> The issue gets revealed when support for IOMMU is enabled. I've did my tests
>>>> with Exynos HDMI driver on Odroid U3 board.
>>>>
>>>> To demonstrated the issue I've added following additional debug in the
>>>> exynos
>>>> mixer driver in mixer_graph_buffer() function:
>>>> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
>>>> &plane->dma_addr[0], plane->src_width, plane->src_height);
>>>>
>>>> Before applying patches setting 640x480 mode and getting back to 1920x1080
>>>> console generates following log:
>>>>
>>>> # modetest -M exynos -s 23:640x480
>>>> setting mode 640x480-60Hz@XR24 on connectors 23, crtc 21
>>>> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>>> ^C
>>>> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>>
>>>> After applying atomic modesetting patchset:
>>>> # modetest -M exynos -s 24:640x480
>>>> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> setting mode 640x480-60Hz@XR24 on connectors 24, crtc 22
>>>> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
>>>> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>>> ^C
>>>> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>>
>>>> As you can see from the above log, mixer_graph_buffer function is called
>>>> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
>>>> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
>>>> mixer_graph_buffer() is first called with the new DMA address of the
>>>> framebuffer, but with the old mode parameters (1920x1080 size) and then
>>>> in the next call it updates the plane parameters to the correct values
>>>> (size changed to 640x480). When IOMMU is not used, this can be easily
>>>> missed, but after enabling IOMMU support, any DMA access to unallocated
>>>> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
>>>> address of the buffer without changing the size.
>>>>
>>>> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
>>>> with partially updated mode values is to remove calls to
>>>> mixer_window_suspend/mixer_window_resume from mixer_disable and
>>>> mixer_disable functions, but I expect that this is not the right
>>>> approach.
>>>>
>>>> Probably the same problem can be observed with Exynos FIMD driver.
>>>>
>>>> Gustavo: could you check if mixer_enable functions should really
>>>> call mixer_window_resume function, which in turn calls mixer_win_commit,
>>>> which calls mixer_graph_buffer with partially updated display buffer
>>>> data?
>>>
>>> It should not, you are right. This is actually the correct fix. Atomic
>>> modesetting should not do chained calls, e.g., crtc_disable calling
>>> plane_disable. This change was already in my plan actually, but as I had
>>> IOMMU disabled I didn't see it here, and I didn't create this patch yet.
>>>
>>
>> Right, but it needs that crtc_disable calls plane_disable in exynos
>> 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.
>>
>> I think a solution is enough only to eliminate calling xxx_resume
>> function in exynos driver function when enable plane, we can remove it
>> because it's called to enable plane from drm atomic modeset framework.
> 
> I think this should work. We really need to disable the planes before
> the crtc is disabled, for FIMD for example, the overlay planes are
> removed from the screen if we remove the _suspend call.
> 
> I'll sent a new patch to remove only the resume part from all our crtc
> drivers.
> 

I sent patchset related, is it same with which you try?

Thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

Hi Gustavo,

On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> Here goes the full support for atomic modesetting on exynos. I've
> split the patches in the various phases of atomic support.
> 
> v2: fixes comments by Joonyoung
>         - remove unused var in patch 09
>         - use ->disable instead of outdated ->dpms in hdmi code
>         - remove WARN_ON from crtc enable/disable
> 
> v3: fixes comment by Joonyoung
>         - move the removal of drm_helper_disable_unused_functions() to
>         separated patch
> 
> v4: add patches that remove unnecessary calls to disable_plane()
> 
> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
> 
> v6: rebase on latest exynos_drm_next
> 
> v7: fix comments by Joonyoung
>         - fix two checkpatch errors
>         - remove extra crtc->commit() call
>         - check for null fb on exynos_check_plane()
> 
> v8: fix comments by Joonyoung
>         - fix merge error
>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
>         - remove .prepare() in the apropiated patch
>         - add a new patch to move exynos_drm_crtc_disable()
> 
> v9:  * fix comments by Joonyoung
>         - also remove encoder .prepare()
>         - do not shift exynos_update_plane() parameters
>         - remove unused .mode_set() and .mode_set_base()
>      * add specific exynos .atomic_commit()
>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
>      * add other atomic clean ups
> 
> v10: * fix comments by Joonyoung
> 	- add more comments on exynos_atomic_commit()
> 	- make exynos_crtc's .enable and .disable void

I found out one issue that refresh rate gets low with display extension
mode test.

I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
is how to test it,

#echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
# modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
freq: 20.00Hz
freq: 20.00Hz

As you can see, refresh rate is 20.

Below is the result without atomic patch series,
# modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
freq: 60.18Hz
freq: 49.09Hz

I can see 60Hz for FIMD and 49Hz for vidi.

Thanks,
Inki Dae

> 
> 
> 	Gustavo
> 
> ---
> Gustavo Padovan (15):
>   drm/exynos: atomic phase 1: use drm_plane_helper_update()
>   drm/exynos: atomic phase 1: use drm_plane_helper_disable()
>   drm/exynos: atomic phase 1: add .mode_set_nofb() callback
>   drm/exynos: atomic phase 2: wire up state reset(), duplicate() and
>     destroy()
>   drm/exynos: atomic phase 2: keep track of framebuffer pointer
>   drm/exynos: atomic phase 3: atomic updates of planes
>   drm/exynos: atomic phase 3: use atomic .set_config helper
>   drm/exynos: atomic phase 3: convert page flips
>   drm/exynos: remove exported functions from exynos_drm_plane
>   drm/exynos: don't disable unused functions at init
>   drm/exynos: move exynos_drm_crtc_disable()
>   drm/exynos: add exynos specific .atomic_commit()
>   drm/exynos: atomic dpms support
>   drm/exynos: remove unnecessary calls to disable_plane()
>   drm/exynos: split exynos_crtc->dpms in enable() and disable()
> 
> Joonyoung Shim (2):
>   drm/exynos: fix source data argument for plane
>   drm/exynos: use adjusted_mode of crtc_state instead of mode
> 
>  drivers/gpu/drm/bridge/ps8622.c             |   6 +-
>  drivers/gpu/drm/bridge/ptn3460.c            |   6 +-
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c  |  91 +++----------
>  drivers/gpu/drm/exynos/exynos_dp_core.c     |   6 +-
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 201 ++++++----------------------
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |   6 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c     |   2 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  10 +-
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |   6 +-
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c |  35 +----
>  drivers/gpu/drm/exynos/exynos_drm_fb.c      |  41 ++++++
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c   |   3 -
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c    |  71 +++-------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c   | 137 ++++++++++---------
>  drivers/gpu/drm/exynos/exynos_drm_plane.h   |  11 --
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  59 ++++----
>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  10 +-
>  drivers/gpu/drm/exynos/exynos_mixer.c       |  26 +---
>  18 files changed, 269 insertions(+), 458 deletions(-)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Gustavo Padovan]

HI Inki,

2015-06-15 Inki Dae <inki.dae@samsung.com>:

> Hi Gustavo,
> 
> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Hi,
> > 
> > Here goes the full support for atomic modesetting on exynos. I've
> > split the patches in the various phases of atomic support.
> > 
> > v2: fixes comments by Joonyoung
> >         - remove unused var in patch 09
> >         - use ->disable instead of outdated ->dpms in hdmi code
> >         - remove WARN_ON from crtc enable/disable
> > 
> > v3: fixes comment by Joonyoung
> >         - move the removal of drm_helper_disable_unused_functions() to
> >         separated patch
> > 
> > v4: add patches that remove unnecessary calls to disable_plane()
> > 
> > v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
> > 
> > v6: rebase on latest exynos_drm_next
> > 
> > v7: fix comments by Joonyoung
> >         - fix two checkpatch errors
> >         - remove extra crtc->commit() call
> >         - check for null fb on exynos_check_plane()
> > 
> > v8: fix comments by Joonyoung
> >         - fix merge error
> >         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
> >         - remove .prepare() in the apropiated patch
> >         - add a new patch to move exynos_drm_crtc_disable()
> > 
> > v9:  * fix comments by Joonyoung
> >         - also remove encoder .prepare()
> >         - do not shift exynos_update_plane() parameters
> >         - remove unused .mode_set() and .mode_set_base()
> >      * add specific exynos .atomic_commit()
> >      * add split of exynos_crtc->ops->dpms() into enable() and disable()
> >      * add other atomic clean ups
> > 
> > v10: * fix comments by Joonyoung
> > 	- add more comments on exynos_atomic_commit()
> > 	- make exynos_crtc's .enable and .disable void
> 
> I found out one issue that refresh rate gets low with display extension
> mode test.
> 
> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
> is how to test it,
> 
> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
> freq: 20.00Hz
> freq: 20.00Hz
> 
> As you can see, refresh rate is 20.
> 
> Below is the result without atomic patch series,
> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
> freq: 60.18Hz
> freq: 49.09Hz
> 
> I can see 60Hz for FIMD and 49Hz for vidi.

I gave this a try and figured out that this might be a vidi specific
problem. If I try VIDI and FIMD I get the same results as you but with
Mixer and FIMD(the setup I actually use to test) everything works fine.
So somehow my patches caused a regression on vidi that I'm still
ivestigating.

	Gustavo

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

Hi Gustavo,

On 2015년 06월 17일 05:35, Gustavo Padovan wrote:
> HI Inki,
> 
> 2015-06-15 Inki Dae <inki.dae@samsung.com>:
> 
>> Hi Gustavo,
>>
>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Hi,
>>>
>>> Here goes the full support for atomic modesetting on exynos. I've
>>> split the patches in the various phases of atomic support.
>>>
>>> v2: fixes comments by Joonyoung
>>>         - remove unused var in patch 09
>>>         - use ->disable instead of outdated ->dpms in hdmi code
>>>         - remove WARN_ON from crtc enable/disable
>>>
>>> v3: fixes comment by Joonyoung
>>>         - move the removal of drm_helper_disable_unused_functions() to
>>>         separated patch
>>>
>>> v4: add patches that remove unnecessary calls to disable_plane()
>>>
>>> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
>>>
>>> v6: rebase on latest exynos_drm_next
>>>
>>> v7: fix comments by Joonyoung
>>>         - fix two checkpatch errors
>>>         - remove extra crtc->commit() call
>>>         - check for null fb on exynos_check_plane()
>>>
>>> v8: fix comments by Joonyoung
>>>         - fix merge error
>>>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
>>>         - remove .prepare() in the apropiated patch
>>>         - add a new patch to move exynos_drm_crtc_disable()
>>>
>>> v9:  * fix comments by Joonyoung
>>>         - also remove encoder .prepare()
>>>         - do not shift exynos_update_plane() parameters
>>>         - remove unused .mode_set() and .mode_set_base()
>>>      * add specific exynos .atomic_commit()
>>>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
>>>      * add other atomic clean ups
>>>
>>> v10: * fix comments by Joonyoung
>>> 	- add more comments on exynos_atomic_commit()
>>> 	- make exynos_crtc's .enable and .disable void
>>
>> I found out one issue that refresh rate gets low with display extension
>> mode test.
>>
>> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
>> is how to test it,
>>
>> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>> freq: 20.00Hz
>> freq: 20.00Hz
>>
>> As you can see, refresh rate is 20.
>>
>> Below is the result without atomic patch series,
>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>> freq: 60.18Hz
>> freq: 49.09Hz
>>
>> I can see 60Hz for FIMD and 49Hz for vidi.
> 
> I gave this a try and figured out that this might be a vidi specific
> problem. If I try VIDI and FIMD I get the same results as you but with
> Mixer and FIMD(the setup I actually use to test) everything works fine.

Hm... Did Mixer and FIMD combination really work correctly with
extension mode?

> So somehow my patches caused a regression on vidi that I'm still
> ivestigating.

I think this issue is because page flip operations are performed in
atomic: if I see correctly, when page flip is requested by modetest, the
call flow is as follows,

drm_atomic_helper_page_flip
	drm_atomic_async_commit
		exynos_atomic_commit
			...
			drm_atomic_helper_wait_for_vblanks
			...

However, the function, drm_atomic_helper_wait_for_vblanks called by
exynos_atomic_commit, waits for the vblank completion of each crtc
driver . See wait_event_timeout function call in
drm_atomic_helper_wait_for_vblanks function.

This means that a page flip request of a crtc driver cannot be performed
until the vblank completion of another crtc driver. I think you should
have implemented exynos_atomic_commit function asynchronously, not
synchronously like you did. So it seems that this function should be
re-implemented.

If my analysis is correct and you cannot post the change set within this
week, I'm not sure whether the atomic patch series should go to
mainline. Anyway, I will decide it and have a pull-request at the end of
this week at least.

Thanks,
Inki Dae

> 
> 	Gustavo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Gustavo Padovan]

Hi Inki,

2015-06-17 Inki Dae <inki.dae@samsung.com>:

> Hi Gustavo,
> 
> On 2015년 06월 17일 05:35, Gustavo Padovan wrote:
> > HI Inki,
> > 
> > 2015-06-15 Inki Dae <inki.dae@samsung.com>:
> > 
> >> Hi Gustavo,
> >>
> >> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
> >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>
> >>> Hi,
> >>>
> >>> Here goes the full support for atomic modesetting on exynos. I've
> >>> split the patches in the various phases of atomic support.
> >>>
> >>> v2: fixes comments by Joonyoung
> >>>         - remove unused var in patch 09
> >>>         - use ->disable instead of outdated ->dpms in hdmi code
> >>>         - remove WARN_ON from crtc enable/disable
> >>>
> >>> v3: fixes comment by Joonyoung
> >>>         - move the removal of drm_helper_disable_unused_functions() to
> >>>         separated patch
> >>>
> >>> v4: add patches that remove unnecessary calls to disable_plane()
> >>>
> >>> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
> >>>
> >>> v6: rebase on latest exynos_drm_next
> >>>
> >>> v7: fix comments by Joonyoung
> >>>         - fix two checkpatch errors
> >>>         - remove extra crtc->commit() call
> >>>         - check for null fb on exynos_check_plane()
> >>>
> >>> v8: fix comments by Joonyoung
> >>>         - fix merge error
> >>>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
> >>>         - remove .prepare() in the apropiated patch
> >>>         - add a new patch to move exynos_drm_crtc_disable()
> >>>
> >>> v9:  * fix comments by Joonyoung
> >>>         - also remove encoder .prepare()
> >>>         - do not shift exynos_update_plane() parameters
> >>>         - remove unused .mode_set() and .mode_set_base()
> >>>      * add specific exynos .atomic_commit()
> >>>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
> >>>      * add other atomic clean ups
> >>>
> >>> v10: * fix comments by Joonyoung
> >>> 	- add more comments on exynos_atomic_commit()
> >>> 	- make exynos_crtc's .enable and .disable void
> >>
> >> I found out one issue that refresh rate gets low with display extension
> >> mode test.
> >>
> >> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
> >> is how to test it,
> >>
> >> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
> >> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
> >> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
> >> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
> >> freq: 20.00Hz
> >> freq: 20.00Hz
> >>
> >> As you can see, refresh rate is 20.
> >>
> >> Below is the result without atomic patch series,
> >> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
> >> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
> >> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
> >> freq: 60.18Hz
> >> freq: 49.09Hz
> >>
> >> I can see 60Hz for FIMD and 49Hz for vidi.
> > 
> > I gave this a try and figured out that this might be a vidi specific
> > problem. If I try VIDI and FIMD I get the same results as you but with
> > Mixer and FIMD(the setup I actually use to test) everything works fine.
> 
> Hm... Did Mixer and FIMD combination really work correctly with
> extension mode?

Yes.

collabora@singularity:~$ modetest -M exynos -s 31:1920x1080 -v -s
25:1366x768
setting mode 1920x1080-60Hz@XR24 on connectors 31, crtc 29
setting mode 1366x768-60Hz@XR24 on connectors 25, crtc 23
freq: 55.82Hz
freq: 55.38Hz
freq: 55.30Hz
freq: 55.38Hz
freq: 55.30Hz
freq: 55.38Hz
freq: 55.30Hz
freq: 55.38Hz
freq: 55.30Hz
freq: 55.38Hz

> 
> > So somehow my patches caused a regression on vidi that I'm still
> > ivestigating.
> 
> I think this issue is because page flip operations are performed in
> atomic: if I see correctly, when page flip is requested by modetest, the
> call flow is as follows,
> 
> drm_atomic_helper_page_flip
> 	drm_atomic_async_commit
> 		exynos_atomic_commit
> 			...
> 			drm_atomic_helper_wait_for_vblanks
> 			...
> 
> However, the function, drm_atomic_helper_wait_for_vblanks called by
> exynos_atomic_commit, waits for the vblank completion of each crtc
> driver . See wait_event_timeout function call in
> drm_atomic_helper_wait_for_vblanks function.
> 
> This means that a page flip request of a crtc driver cannot be performed
> until the vblank completion of another crtc driver. I think you should
> have implemented exynos_atomic_commit function asynchronously, not
> synchronously like you did. So it seems that this function should be
> re-implemented.

I have a patch for it already. I'll resend a v2 of my last series. 

> 
> If my analysis is correct and you cannot post the change set within this
> week, I'm not sure whether the atomic patch series should go to
> mainline. Anyway, I will decide it and have a pull-request at the end of
> this week at least.

We have about 3 months to fix this, until v4.2 is out, not one week. And
it is working for FIMD and Mixer.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

On 2015년 06월 17일 23:33, Gustavo Padovan wrote:
> Hi Inki,
> 
> 2015-06-17 Inki Dae <inki.dae@samsung.com>:
> 
>> Hi Gustavo,
>>
>> On 2015년 06월 17일 05:35, Gustavo Padovan wrote:
>>> HI Inki,
>>>
>>> 2015-06-15 Inki Dae <inki.dae@samsung.com>:
>>>
>>>> Hi Gustavo,
>>>>
>>>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>>> split the patches in the various phases of atomic support.
>>>>>
>>>>> v2: fixes comments by Joonyoung
>>>>>         - remove unused var in patch 09
>>>>>         - use ->disable instead of outdated ->dpms in hdmi code
>>>>>         - remove WARN_ON from crtc enable/disable
>>>>>
>>>>> v3: fixes comment by Joonyoung
>>>>>         - move the removal of drm_helper_disable_unused_functions() to
>>>>>         separated patch
>>>>>
>>>>> v4: add patches that remove unnecessary calls to disable_plane()
>>>>>
>>>>> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
>>>>>
>>>>> v6: rebase on latest exynos_drm_next
>>>>>
>>>>> v7: fix comments by Joonyoung
>>>>>         - fix two checkpatch errors
>>>>>         - remove extra crtc->commit() call
>>>>>         - check for null fb on exynos_check_plane()
>>>>>
>>>>> v8: fix comments by Joonyoung
>>>>>         - fix merge error
>>>>>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
>>>>>         - remove .prepare() in the apropiated patch
>>>>>         - add a new patch to move exynos_drm_crtc_disable()
>>>>>
>>>>> v9:  * fix comments by Joonyoung
>>>>>         - also remove encoder .prepare()
>>>>>         - do not shift exynos_update_plane() parameters
>>>>>         - remove unused .mode_set() and .mode_set_base()
>>>>>      * add specific exynos .atomic_commit()
>>>>>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
>>>>>      * add other atomic clean ups
>>>>>
>>>>> v10: * fix comments by Joonyoung
>>>>> 	- add more comments on exynos_atomic_commit()
>>>>> 	- make exynos_crtc's .enable and .disable void
>>>>
>>>> I found out one issue that refresh rate gets low with display extension
>>>> mode test.
>>>>
>>>> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
>>>> is how to test it,
>>>>
>>>> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
>>>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>>>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>>>> freq: 20.00Hz
>>>> freq: 20.00Hz
>>>>
>>>> As you can see, refresh rate is 20.
>>>>
>>>> Below is the result without atomic patch series,
>>>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>>>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>>>> freq: 60.18Hz
>>>> freq: 49.09Hz
>>>>
>>>> I can see 60Hz for FIMD and 49Hz for vidi.
>>>
>>> I gave this a try and figured out that this might be a vidi specific
>>> problem. If I try VIDI and FIMD I get the same results as you but with
>>> Mixer and FIMD(the setup I actually use to test) everything works fine.
>>
>> Hm... Did Mixer and FIMD combination really work correctly with
>> extension mode?
> 
> Yes.
> 
> collabora@singularity:~$ modetest -M exynos -s 31:1920x1080 -v -s
> 25:1366x768
> setting mode 1920x1080-60Hz@XR24 on connectors 31, crtc 29
> setting mode 1366x768-60Hz@XR24 on connectors 25, crtc 23
> freq: 55.82Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> 
>>
>>> So somehow my patches caused a regression on vidi that I'm still
>>> ivestigating.
>>
>> I think this issue is because page flip operations are performed in
>> atomic: if I see correctly, when page flip is requested by modetest, the
>> call flow is as follows,
>>
>> drm_atomic_helper_page_flip
>> 	drm_atomic_async_commit
>> 		exynos_atomic_commit
>> 			...
>> 			drm_atomic_helper_wait_for_vblanks
>> 			...
>>
>> However, the function, drm_atomic_helper_wait_for_vblanks called by
>> exynos_atomic_commit, waits for the vblank completion of each crtc
>> driver . See wait_event_timeout function call in
>> drm_atomic_helper_wait_for_vblanks function.
>>
>> This means that a page flip request of a crtc driver cannot be performed
>> until the vblank completion of another crtc driver. I think you should
>> have implemented exynos_atomic_commit function asynchronously, not
>> synchronously like you did. So it seems that this function should be
>> re-implemented.
> 
> I have a patch for it already. I'll resend a v2 of my last series. 

Can you post it now?

Thanks,
Inki Dae

> 
>>
>> If my analysis is correct and you cannot post the change set within this
>> week, I'm not sure whether the atomic patch series should go to
>> mainline. Anyway, I will decide it and have a pull-request at the end of
>> this week at least.
> 
> We have about 3 months to fix this, until v4.2 is out, not one week. And
> it is working for FIMD and Mixer.
> 
> 	Gustavo
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v10 00/17] drm/exynos: atomic modesetting support

[Inki Dae]

On 2015년 06월 17일 23:33, Gustavo Padovan wrote:
> Hi Inki,
> 
> 2015-06-17 Inki Dae <inki.dae@samsung.com>:
> 
>> Hi Gustavo,
>>
>> On 2015년 06월 17일 05:35, Gustavo Padovan wrote:
>>> HI Inki,
>>>
>>> 2015-06-15 Inki Dae <inki.dae@samsung.com>:
>>>
>>>> Hi Gustavo,
>>>>
>>>> On 2015년 06월 02일 00:04, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>>> split the patches in the various phases of atomic support.
>>>>>
>>>>> v2: fixes comments by Joonyoung
>>>>>         - remove unused var in patch 09
>>>>>         - use ->disable instead of outdated ->dpms in hdmi code
>>>>>         - remove WARN_ON from crtc enable/disable
>>>>>
>>>>> v3: fixes comment by Joonyoung
>>>>>         - move the removal of drm_helper_disable_unused_functions() to
>>>>>         separated patch
>>>>>
>>>>> v4: add patches that remove unnecessary calls to disable_plane()
>>>>>
>>>>> v5: fixes NULL CRTC crash on planes updates (reported by Inki and Tobias)
>>>>>
>>>>> v6: rebase on latest exynos_drm_next
>>>>>
>>>>> v7: fix comments by Joonyoung
>>>>>         - fix two checkpatch errors
>>>>>         - remove extra crtc->commit() call
>>>>>         - check for null fb on exynos_check_plane()
>>>>>
>>>>> v8: fix comments by Joonyoung
>>>>>         - fix merge error
>>>>>         - move drm_crtc_vblank_get to the commit that introduces atomic pageflip
>>>>>         - remove .prepare() in the apropiated patch
>>>>>         - add a new patch to move exynos_drm_crtc_disable()
>>>>>
>>>>> v9:  * fix comments by Joonyoung
>>>>>         - also remove encoder .prepare()
>>>>>         - do not shift exynos_update_plane() parameters
>>>>>         - remove unused .mode_set() and .mode_set_base()
>>>>>      * add specific exynos .atomic_commit()
>>>>>      * add split of exynos_crtc->ops->dpms() into enable() and disable()
>>>>>      * add other atomic clean ups
>>>>>
>>>>> v10: * fix comments by Joonyoung
>>>>> 	- add more comments on exynos_atomic_commit()
>>>>> 	- make exynos_crtc's .enable and .disable void
>>>>
>>>> I found out one issue that refresh rate gets low with display extension
>>>> mode test.
>>>>
>>>> I tested it with two crtc drivers - vidi and fimd on Trats2 board. Here
>>>> is how to test it,
>>>>
>>>> #echo 1 > /sys/devices/platform/exynos-drm-vidi/connection
>>>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>>>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>>>> freq: 20.00Hz
>>>> freq: 20.00Hz
>>>>
>>>> As you can see, refresh rate is 20.
>>>>
>>>> Below is the result without atomic patch series,
>>>> # modetest -M exynos -v -s 31@29:720x1280 -s 23@21:640x480
>>>> setting mode 720x1280-60Hz@XR24 on connectors 31, crtc 29
>>>> setting mode 640x480-75Hz@XR24 on connectors 23, crtc 21
>>>> freq: 60.18Hz
>>>> freq: 49.09Hz
>>>>
>>>> I can see 60Hz for FIMD and 49Hz for vidi.
>>>
>>> I gave this a try and figured out that this might be a vidi specific
>>> problem. If I try VIDI and FIMD I get the same results as you but with
>>> Mixer and FIMD(the setup I actually use to test) everything works fine.
>>
>> Hm... Did Mixer and FIMD combination really work correctly with
>> extension mode?
> 
> Yes.
> 
> collabora@singularity:~$ modetest -M exynos -s 31:1920x1080 -v -s
> 25:1366x768
> setting mode 1920x1080-60Hz@XR24 on connectors 31, crtc 29
> setting mode 1366x768-60Hz@XR24 on connectors 25, crtc 23
> freq: 55.82Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz
> freq: 55.30Hz
> freq: 55.38Hz

Your patch series had one bug that the vblank operation was duplicated.
As you may know, the page flip operation already contains vblank
functionality but your patch made atomic_commit callback wait for the
completion of vblank again.

See the below patch I posted,
[PATCH] drm/exynos: do not wait for vblank at atomic operation

That was why I asked for that mixer and fimd combination worked
correctly with extension mode. Don't you think above frequency is
strange? The frequency should be 60Hz, not 55Hz.

We would need to enhance the atomic feature for Exynos drm later.
Current atomic feature doesn't really do atomic operation, and is
required for more cleanups. Anyway, I have already merged it and will
have a pull-request soon.

Thanks for your contributions,
Inki Dae

> 
>>
>>> So somehow my patches caused a regression on vidi that I'm still
>>> ivestigating.
>>
>> I think this issue is because page flip operations are performed in
>> atomic: if I see correctly, when page flip is requested by modetest, the
>> call flow is as follows,
>>
>> drm_atomic_helper_page_flip
>> 	drm_atomic_async_commit
>> 		exynos_atomic_commit
>> 			...
>> 			drm_atomic_helper_wait_for_vblanks
>> 			...
>>
>> However, the function, drm_atomic_helper_wait_for_vblanks called by
>> exynos_atomic_commit, waits for the vblank completion of each crtc
>> driver . See wait_event_timeout function call in
>> drm_atomic_helper_wait_for_vblanks function.
>>
>> This means that a page flip request of a crtc driver cannot be performed
>> until the vblank completion of another crtc driver. I think you should
>> have implemented exynos_atomic_commit function asynchronously, not
>> synchronously like you did. So it seems that this function should be
>> re-implemented.
> 
> I have a patch for it already. I'll resend a v2 of my last series. 
> 
>>
>> If my analysis is correct and you cannot post the change set within this
>> week, I'm not sure whether the atomic patch series should go to
>> mainline. Anyway, I will decide it and have a pull-request at the end of
>> this week at least.
> 
> We have about 3 months to fix this, until v4.2 is out, not one week. And
> it is working for FIMD and Mixer.
> 
> 	Gustavo
>