[PATCH v7 00/13] drm/exynos: atomic modesetting support

[PATCH v7 00/13] 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()

Gustavo Padovan (12):
  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: atomic dpms support
  drm/exynos: remove unnecessary calls to disable_plane()

Joonyoung Shim (1):
  drm/exynos: fix source data argument for plane

 drivers/gpu/drm/bridge/ps8622.c             |   6 +-
 drivers/gpu/drm/bridge/ptn3460.c            |   6 +-
 drivers/gpu/drm/exynos/exynos_dp_core.c     |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c    | 211 ++++++++--------------------
 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     |   4 +-
 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      |  10 ++
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c   |   3 -
 drivers/gpu/drm/exynos/exynos_drm_plane.c   | 124 +++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_plane.h   |  11 --
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |   6 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c        |  10 +-
 15 files changed, 183 insertions(+), 263 deletions(-)

-- 
2.1.0

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

[PATCH v7 01/13] 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

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

[PATCH v7 02/13] 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  | 23 ++++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 40 +++++++++++++++++++++++--------
 drivers/gpu/drm/exynos/exynos_drm_plane.h |  2 +-
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 363b019..27cc450 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,6 +211,7 @@ 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;
+<<<<<<< HEAD
 	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);
@@ -213,6 +223,11 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 		spin_unlock_irq(&dev->event_lock);
 		return ret;
 	}
+=======
+	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
+			    crtc_w, crtc_h, crtc->x, crtc->y,
+			    crtc_w, crtc_h);
+>>>>>>> 2f6d1f4... drm/exynos: atomic phase 1: use drm_plane_helper_update()
 
 	return 0;
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index b1180fb..b218b7a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -144,21 +144,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,
@@ -166,8 +160,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)
@@ -183,11 +175,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 >> 16, state->src_y >> 16,
+			    state->src_w >> 16, state->src_h >> 16);
+}
+
+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)
 {
@@ -223,6 +241,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

Re: [PATCH v7 02/13] drm/exynos: atomic phase 1: use drm_plane_helper_update()

[Joonyoung Shim]

On 05/23/2015 12:40 AM, Gustavo Padovan wrote:
> 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  | 23 ++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_plane.c | 40 +++++++++++++++++++++++--------
>  drivers/gpu/drm/exynos/exynos_drm_plane.h |  2 +-
>  3 files changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 363b019..27cc450 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,6 +211,7 @@ 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;
> +<<<<<<< HEAD
>  	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);
> @@ -213,6 +223,11 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
>  		spin_unlock_irq(&dev->event_lock);
>  		return ret;
>  	}
> +=======
> +	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
> +			    crtc_w, crtc_h, crtc->x, crtc->y,
> +			    crtc_w, crtc_h);
> +>>>>>>> 2f6d1f4... drm/exynos: atomic phase 1: use drm_plane_helper_update()

Maybe unhandled output of merge, could you fix it?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 02/13] drm/exynos: atomic phase 1: use drm_plane_helper_update()

[Gustavo Padovan]

Hi Joonyoung,

2015-05-27 Joonyoung Shim <jy0922.shim@samsung.com>:

> On 05/23/2015 12:40 AM, Gustavo Padovan wrote:
> > 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  | 23 ++++++++++++++----
> >  drivers/gpu/drm/exynos/exynos_drm_plane.c | 40 +++++++++++++++++++++++--------
> >  drivers/gpu/drm/exynos/exynos_drm_plane.h |  2 +-
> >  3 files changed, 50 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index 363b019..27cc450 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,6 +211,7 @@ 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;
> > +<<<<<<< HEAD
> >  	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);
> > @@ -213,6 +223,11 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
> >  		spin_unlock_irq(&dev->event_lock);
> >  		return ret;
> >  	}
> > +=======
> > +	exynos_update_plane(crtc->primary, crtc, fb, 0, 0,
> > +			    crtc_w, crtc_h, crtc->x, crtc->y,
> > +			    crtc_w, crtc_h);
> > +>>>>>>> 2f6d1f4... drm/exynos: atomic phase 1: use drm_plane_helper_update()
> 
> Maybe unhandled output of merge, could you fix it?

Sure. Fixed.

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

[PATCH v7 03/13] 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 b218b7a..a0fdfe2 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);
@@ -162,21 +165,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,
 };
 
@@ -201,9 +192,24 @@ static void exynos_plane_atomic_update(struct drm_plane *plane,
 			    state->src_w >> 16, state->src_h >> 16);
 }
 
+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

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

[PATCH v7 04/13] 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.

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 | 82 +++++---------------------------
 1 file changed, 11 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 27cc450..c524f0c 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,16 @@ 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);
+	if (WARN_ON(!crtc->state))
+		return;
 
-	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 +112,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,
 };
 
@@ -211,23 +165,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;
-<<<<<<< HEAD
-	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, crtc->y,
-			    crtc_w, crtc_h);
->>>>>>> 2f6d1f4... drm/exynos: atomic phase 1: use drm_plane_helper_update()
+			    crtc_w, crtc_h, crtc->x << 16, crtc->y << 16,
+			    crtc_w << 16, crtc_h << 16);
 
 	return 0;
 
-- 
2.1.0

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

[PATCH v7 05/13] 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 c524f0c..5bfee9b 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"
@@ -191,6 +193,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 a0fdfe2..43ada86 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"
@@ -169,6 +170,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

[PATCH v7 06/13] 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 5bfee9b..fe77516 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -171,6 +171,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 v7 07/13] 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 43ada86..e052c72 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -167,8 +167,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

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

[PATCH v7 08/13] 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.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index fe77516..9e14ba3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -193,7 +193,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,
-- 
2.1.0

[PATCH v7 09/13] 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.

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 +-------------------------------
 drivers/gpu/drm/exynos/exynos_drm_fb.c   |  9 ++++-
 2 files changed, 9 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 9e14ba3..fd5ef2c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -120,67 +120,6 @@ static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
 	.disable	= exynos_drm_crtc_disable,
 };
 
-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);
@@ -194,7 +133,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 v7 10/13] 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 e052c72..54a83ee 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);
 	unsigned int actual_w;
@@ -148,24 +123,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,
@@ -178,22 +135,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 >> 16, state->src_y >> 16,
-			    state->src_w >> 16, state->src_h >> 16);
+	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

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

[PATCH v7 11/13] 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

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

[PATCH v7 12/13] 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

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    | 95 ++++++++++++++++-------------
 drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
 drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
 10 files changed, 69 insertions(+), 75 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 fd5ef2c..ca501a9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -22,48 +22,54 @@
 #include "exynos_drm_encoder.h"
 #include "exynos_drm_plane.h"
 
-static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
+static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
 
-	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
-
-	if (exynos_crtc->dpms == mode) {
-		DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
+	if (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_prepare(struct drm_crtc *crtc)
-{
-	/* drm framework doesn't check NULL. */
+	if (exynos_crtc->ops->win_commit)
+		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
 }
 
-static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
+static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
+	struct drm_plane *plane;
+	int ret;
 
-	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
+	if (!exynos_crtc->enabled)
+		return;
 
-	if (exynos_crtc->ops->win_commit)
-		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
+	/* 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)
+			continue;
+
+		ret = plane->funcs->disable_plane(plane);
+		if (ret)
+			DRM_ERROR("Failed to disable plane %d\n", ret);
+	}
 }
 
 static bool
@@ -92,32 +98,36 @@ exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
 		exynos_crtc->ops->commit(exynos_crtc);
 }
 
-static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
+static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
+				     struct drm_crtc_state *state)
 {
-	struct drm_plane *plane;
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
 	int ret;
 
-	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+	if (exynos_crtc->event)
+		return -EBUSY;
 
-	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
-		if (plane->crtc != crtc)
-			continue;
+	if (state->event) {
+		ret = drm_vblank_get(crtc->dev, exynos_crtc->pipe);
+		if (ret) {
+			DRM_ERROR("failed to acquire vblank counter\n");
+			return ret;
+		}
 
-		ret = plane->funcs->disable_plane(plane);
-		if (ret)
-			DRM_ERROR("Failed to disable plane %d\n", ret);
+		exynos_crtc->event = state->event;
 	}
+
+	return 0;
 }
 
 static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
-	.dpms		= exynos_drm_crtc_dpms,
-	.prepare	= exynos_drm_crtc_prepare,
-	.commit		= exynos_drm_crtc_commit,
+	.enable		= exynos_drm_crtc_enable,
+	.disable	= exynos_drm_crtc_disable,
 	.mode_fixup	= exynos_drm_crtc_mode_fixup,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
-	.disable	= exynos_drm_crtc_disable,
+	.atomic_check	= exynos_crtc_atomic_check,
 };
 
 static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
@@ -158,7 +168,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;
@@ -189,7 +198,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)
@@ -204,7 +213,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 57de0bd..0648ba4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -32,17 +32,6 @@ struct exynos_drm_encoder {
 	struct exynos_drm_display	*display;
 };
 
-static void exynos_drm_encoder_dpms(struct drm_encoder *encoder, int mode)
-{
-	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
-	struct exynos_drm_display *display = exynos_encoder->display;
-
-	DRM_DEBUG_KMS("encoder dpms: %d\n", mode);
-
-	if (display->ops->dpms)
-		display->ops->dpms(display, mode);
-}
-
 static bool
 exynos_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 			       const struct drm_display_mode *mode,
@@ -76,12 +65,7 @@ static void exynos_drm_encoder_mode_set(struct drm_encoder *encoder,
 		display->ops->mode_set(display, adjusted_mode);
 }
 
-static void exynos_drm_encoder_prepare(struct drm_encoder *encoder)
-{
-	/* drm framework doesn't check NULL. */
-}
-
-static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
+static void exynos_drm_encoder_enable(struct drm_encoder *encoder)
 {
 	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
 	struct exynos_drm_display *display = exynos_encoder->display;
@@ -95,10 +79,13 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
 
 static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
 {
+	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
+	struct exynos_drm_display *display = exynos_encoder->display;
 	struct drm_plane *plane;
 	struct drm_device *dev = encoder->dev;
 
-	exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+	if (display->ops->dpms)
+		display->ops->dpms(display, DRM_MODE_DPMS_OFF);
 
 	/* all planes connected to this encoder should be also disabled. */
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
@@ -108,11 +95,9 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
 }
 
 static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
-	.dpms		= exynos_drm_encoder_dpms,
 	.mode_fixup	= exynos_drm_encoder_mode_fixup,
 	.mode_set	= exynos_drm_encoder_mode_set,
-	.prepare	= exynos_drm_encoder_prepare,
-	.commit		= exynos_drm_encoder_commit,
+	.enable		= exynos_drm_encoder_enable,
 	.disable	= exynos_drm_encoder_disable,
 };
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Joonyoung Shim]

Hi Gustavo,

Sorry, i was missing some review points.

On 05/23/2015 12:40 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Run dpms operations through the atomic intefaces. This basically removes
> the .dpms() callback from econders and crtcs and use .disable() and
> .enable() to turn the crtc on and off.
> 
> 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
> 
> 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    | 95 ++++++++++++++++-------------
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>  10 files changed, 69 insertions(+), 75 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 fd5ef2c..ca501a9 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -22,48 +22,54 @@
>  #include "exynos_drm_encoder.h"
>  #include "exynos_drm_plane.h"
>  
> -static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
> +static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
>  
> -	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
> -
> -	if (exynos_crtc->dpms == mode) {
> -		DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
> +	if (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_prepare(struct drm_crtc *crtc)
> -{
> -	/* drm framework doesn't check NULL. */
> +	if (exynos_crtc->ops->win_commit)
> +		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);

Unnecessary also? because be called by exynos_plane_atomic_update
already.

>  }
>  
> -static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
> +static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> -	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
> +	struct drm_plane *plane;
> +	int ret;
>  
> -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> +	if (!exynos_crtc->enabled)
> +		return;
>  
> -	if (exynos_crtc->ops->win_commit)
> -		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> +	/* 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)
> +			continue;
> +
> +		ret = plane->funcs->disable_plane(plane);
> +		if (ret)
> +			DRM_ERROR("Failed to disable plane %d\n", ret);
> +	}
>  }
>  
>  static bool
> @@ -92,32 +98,36 @@ exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  		exynos_crtc->ops->commit(exynos_crtc);
>  }
>  
> -static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> +static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *state)
>  {
> -	struct drm_plane *plane;
> +	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>  	int ret;
>  
> -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
> +	if (exynos_crtc->event)
> +		return -EBUSY;
>  
> -	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
> -		if (plane->crtc != crtc)
> -			continue;
> +	if (state->event) {
> +		ret = drm_vblank_get(crtc->dev, exynos_crtc->pipe);
> +		if (ret) {
> +			DRM_ERROR("failed to acquire vblank counter\n");
> +			return ret;
> +		}
>  
> -		ret = plane->funcs->disable_plane(plane);
> -		if (ret)
> -			DRM_ERROR("Failed to disable plane %d\n", ret);
> +		exynos_crtc->event = state->event;
>  	}
> +
> +	return 0;

Should this go to .atomic_begin? Then you are missing drm_vblank_get
from a patch "drm/exynos: atomic phase 3: convert page flips"?

>  }
>  
>  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,

This .prepare and .prepare of encoder_helper_funcs are not used by
drm_atomic_helper_set_config so you can remove them from "drm/exynos:
atomic phase 3: use atomic .set_config helper"?

> +	.enable		= exynos_drm_crtc_enable,
> +	.disable	= exynos_drm_crtc_disable,
>  	.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,

Could you make a split patch to move exynos_drm_crtc_disable function?
If not, we are difficult to understand code changes via diff.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Gustavo Padovan]

Hi Joonyoung,

2015-05-27 Joonyoung Shim <jy0922.shim@samsung.com>:

> Hi Gustavo,
> 
> Sorry, i was missing some review points.
> 
> On 05/23/2015 12:40 AM, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Run dpms operations through the atomic intefaces. This basically removes
> > the .dpms() callback from econders and crtcs and use .disable() and
> > .enable() to turn the crtc on and off.
> > 
> > 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
> > 
> > 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    | 95 ++++++++++++++++-------------
> >  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
> >  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
> >  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
> >  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
> >  10 files changed, 69 insertions(+), 75 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 fd5ef2c..ca501a9 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -22,48 +22,54 @@
> >  #include "exynos_drm_encoder.h"
> >  #include "exynos_drm_plane.h"
> >  
> > -static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
> > +static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
> >  {
> >  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> > +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
> >  
> > -	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
> > -
> > -	if (exynos_crtc->dpms == mode) {
> > -		DRM_DEBUG_KMS("desired dpms mode is same as previous one.\n");
> > +	if (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_prepare(struct drm_crtc *crtc)
> > -{
> > -	/* drm framework doesn't check NULL. */
> > +	if (exynos_crtc->ops->win_commit)
> > +		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> 
> Unnecessary also? because be called by exynos_plane_atomic_update
> already.

It is, I already had a patch queued here that was removing this call,
but let's remove it now in this commit.

> 
> >  }
> >  
> > -static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
> > +static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> >  {
> >  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> > -	struct exynos_drm_plane *exynos_plane = to_exynos_plane(crtc->primary);
> > +	struct drm_plane *plane;
> > +	int ret;
> >  
> > -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
> > +	if (!exynos_crtc->enabled)
> > +		return;
> >  
> > -	if (exynos_crtc->ops->win_commit)
> > -		exynos_crtc->ops->win_commit(exynos_crtc, exynos_plane->zpos);
> > +	/* 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)
> > +			continue;
> > +
> > +		ret = plane->funcs->disable_plane(plane);
> > +		if (ret)
> > +			DRM_ERROR("Failed to disable plane %d\n", ret);
> > +	}
> >  }
> >  
> >  static bool
> > @@ -92,32 +98,36 @@ exynos_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >  		exynos_crtc->ops->commit(exynos_crtc);
> >  }
> >  
> > -static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> > +static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> > +				     struct drm_crtc_state *state)
> >  {
> > -	struct drm_plane *plane;
> > +	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> >  	int ret;
> >  
> > -	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
> > +	if (exynos_crtc->event)
> > +		return -EBUSY;
> >  
> > -	drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) {
> > -		if (plane->crtc != crtc)
> > -			continue;
> > +	if (state->event) {
> > +		ret = drm_vblank_get(crtc->dev, exynos_crtc->pipe);
> > +		if (ret) {
> > +			DRM_ERROR("failed to acquire vblank counter\n");
> > +			return ret;
> > +		}
> >  
> > -		ret = plane->funcs->disable_plane(plane);
> > -		if (ret)
> > -			DRM_ERROR("Failed to disable plane %d\n", ret);
> > +		exynos_crtc->event = state->event;
> >  	}
> > +
> > +	return 0;
> 
> Should this go to .atomic_begin? Then you are missing drm_vblank_get
> from a patch "drm/exynos: atomic phase 3: convert page flips"?

Yeah, makes more sense. I moved it to that commit and changed it to
atomic_begin.

> 
> >  }
> >  
> >  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,
> 
> This .prepare and .prepare of encoder_helper_funcs are not used by
> drm_atomic_helper_set_config so you can remove them from "drm/exynos:
> atomic phase 3: use atomic .set_config helper"?

Done.

> 
> > +	.enable		= exynos_drm_crtc_enable,
> > +	.disable	= exynos_drm_crtc_disable,
> >  	.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,
> 
> Could you make a split patch to move exynos_drm_crtc_disable function?
> If not, we are difficult to understand code changes via diff.

Done.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Inki Dae]

Hi Gustavo,

On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Run dpms operations through the atomic intefaces. This basically removes
> the .dpms() callback from econders and crtcs and use .disable() and
> .enable() to turn the crtc on and off.
> 
> 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
> 
> 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    | 95 ++++++++++++++++-------------
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>  10 files changed, 69 insertions(+), 75 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)
>  }
>  

[--snip--]

>  
>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> -	.dpms		= exynos_drm_crtc_dpms,
> -	.prepare	= exynos_drm_crtc_prepare,
> -	.commit		= exynos_drm_crtc_commit,
> +	.enable		= exynos_drm_crtc_enable,
> +	.disable	= exynos_drm_crtc_disable,
>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>  	.mode_set	= drm_helper_crtc_mode_set,
>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,

I think it'd be better to use atomic_flush callback to enable global dma
like commit callback did. Is there any reason that you don't use
atomic_begin and atomic_flush callbacks?

atomic relevant codes I looked into do as follows,

atomic_begin();

atomic_update();  /* this will call win_commit callback to set a overlay
relevant registers and enable its dma channel. */

atomic_flush();

So atomic overlay updating between atomic_begin() ~ atomic_flush() will
be guaranteed.

Thanks,
Inki Dae

>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
> -	.disable	= exynos_drm_crtc_disable,
> +	.atomic_check	= exynos_crtc_atomic_check,
>  };
>  

[--snip--]

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Gustavo Padovan]

Hi Inki,

2015-05-27 Inki Dae <inki.dae@samsung.com>:

> Hi Gustavo,
> 
> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Run dpms operations through the atomic intefaces. This basically removes
> > the .dpms() callback from econders and crtcs and use .disable() and
> > .enable() to turn the crtc on and off.
> > 
> > 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
> > 
> > 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    | 95 ++++++++++++++++-------------
> >  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
> >  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
> >  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
> >  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
> >  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
> >  10 files changed, 69 insertions(+), 75 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)
> >  }
> >  
> 
> [--snip--]
> 
> >  
> >  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> > -	.dpms		= exynos_drm_crtc_dpms,
> > -	.prepare	= exynos_drm_crtc_prepare,
> > -	.commit		= exynos_drm_crtc_commit,
> > +	.enable		= exynos_drm_crtc_enable,
> > +	.disable	= exynos_drm_crtc_disable,
> >  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
> >  	.mode_set	= drm_helper_crtc_mode_set,
> >  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
> 
> I think it'd be better to use atomic_flush callback to enable global dma
> like commit callback did. Is there any reason that you don't use
> atomic_begin and atomic_flush callbacks?
> 
> atomic relevant codes I looked into do as follows,
> 
> atomic_begin();
> 
> atomic_update();  /* this will call win_commit callback to set a overlay
> relevant registers and enable its dma channel. */
> 
> atomic_flush();
> 
> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
> be guaranteed.

I think we can go down that road, but I'd suggest we push the atomic
patches v8 (with the lastest comments from Joonyoung fixed) and then 
work on the change you are proposing as a follow-up together with the 
other improvements for atomic I already have queued here. This way
we don't take the risk of missing one more merge window.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Inki Dae]

Hi Gustavo,

On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
> Hi Inki,
> 
> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
> 
>> Hi Gustavo,
>>
>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> Run dpms operations through the atomic intefaces. This basically removes
>>> the .dpms() callback from econders and crtcs and use .disable() and
>>> .enable() to turn the crtc on and off.
>>>
>>> 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
>>>
>>> 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    | 95 ++++++++++++++++-------------
>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>>>  10 files changed, 69 insertions(+), 75 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)
>>>  }
>>>  
>>
>> [--snip--]
>>
>>>  
>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>>> -	.dpms		= exynos_drm_crtc_dpms,
>>> -	.prepare	= exynos_drm_crtc_prepare,
>>> -	.commit		= exynos_drm_crtc_commit,
>>> +	.enable		= exynos_drm_crtc_enable,
>>> +	.disable	= exynos_drm_crtc_disable,
>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>>
>> I think it'd be better to use atomic_flush callback to enable global dma
>> like commit callback did. Is there any reason that you don't use
>> atomic_begin and atomic_flush callbacks?
>>
>> atomic relevant codes I looked into do as follows,
>>
>> atomic_begin();
>>
>> atomic_update();  /* this will call win_commit callback to set a overlay
>> relevant registers and enable its dma channel. */
>>
>> atomic_flush();
>>
>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
>> be guaranteed.
> 
> I think we can go down that road, but I'd suggest we push the atomic
> patches v8 (with the lastest comments from Joonyoung fixed) and then 
> work on the change you are proposing as a follow-up together with the 
> other improvements for atomic I already have queued here. This way
> we don't take the risk of missing one more merge window.

We(I and Joonyoung) will have discussion about this patch series. For
this, we have already started to analyze entire atomic features
including your patch set so I'd merge it at end of next week according
to the discussion. I'm not kind of sure yet but I will merge it as long
as there is no big problem.

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
> 

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Joonyoung Shim]

On 05/28/2015 02:39 PM, Inki Dae wrote:
> Hi Gustavo,
> 
> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
>> Hi Inki,
>>
>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
>>
>>> Hi Gustavo,
>>>
>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> Run dpms operations through the atomic intefaces. This basically removes
>>>> the .dpms() callback from econders and crtcs and use .disable() and
>>>> .enable() to turn the crtc on and off.
>>>>
>>>> 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
>>>>
>>>> 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    | 95 ++++++++++++++++-------------
>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>>>>  10 files changed, 69 insertions(+), 75 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)
>>>>  }
>>>>  
>>>
>>> [--snip--]
>>>
>>>>  
>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>>>> -	.dpms		= exynos_drm_crtc_dpms,
>>>> -	.prepare	= exynos_drm_crtc_prepare,
>>>> -	.commit		= exynos_drm_crtc_commit,
>>>> +	.enable		= exynos_drm_crtc_enable,
>>>> +	.disable	= exynos_drm_crtc_disable,
>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>>>
>>> I think it'd be better to use atomic_flush callback to enable global dma
>>> like commit callback did. Is there any reason that you don't use
>>> atomic_begin and atomic_flush callbacks?
>>>
>>> atomic relevant codes I looked into do as follows,
>>>
>>> atomic_begin();
>>>
>>> atomic_update();  /* this will call win_commit callback to set a overlay
>>> relevant registers and enable its dma channel. */
>>>
>>> atomic_flush();
>>>
>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
>>> be guaranteed.
>>
>> I think we can go down that road, but I'd suggest we push the atomic
>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
>> work on the change you are proposing as a follow-up together with the 
>> other improvements for atomic I already have queued here. This way
>> we don't take the risk of missing one more merge window.
> 
> We(I and Joonyoung) will have discussion about this patch series. For
> this, we have already started to analyze entire atomic features
> including your patch set so I'd merge it at end of next week according
> to the discussion. I'm not kind of sure yet but I will merge it as long
> as there is no big problem.
> 

Actually i agree to opinion of Gustavo and will repost the patchset of
Gustavo with some patches fixed by me.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Joonyoung Shim]

On 05/28/2015 05:24 PM, Joonyoung Shim wrote:
> On 05/28/2015 02:39 PM, Inki Dae wrote:
>> Hi Gustavo,
>>
>> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
>>> Hi Inki,
>>>
>>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
>>>
>>>> Hi Gustavo,
>>>>
>>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>
>>>>> Run dpms operations through the atomic intefaces. This basically removes
>>>>> the .dpms() callback from econders and crtcs and use .disable() and
>>>>> .enable() to turn the crtc on and off.
>>>>>
>>>>> 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
>>>>>
>>>>> 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    | 95 ++++++++++++++++-------------
>>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>>>>>  10 files changed, 69 insertions(+), 75 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)
>>>>>  }
>>>>>  
>>>>
>>>> [--snip--]
>>>>
>>>>>  
>>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>>>>> -	.dpms		= exynos_drm_crtc_dpms,
>>>>> -	.prepare	= exynos_drm_crtc_prepare,
>>>>> -	.commit		= exynos_drm_crtc_commit,
>>>>> +	.enable		= exynos_drm_crtc_enable,
>>>>> +	.disable	= exynos_drm_crtc_disable,
>>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>>>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>>>>
>>>> I think it'd be better to use atomic_flush callback to enable global dma
>>>> like commit callback did. Is there any reason that you don't use
>>>> atomic_begin and atomic_flush callbacks?
>>>>
>>>> atomic relevant codes I looked into do as follows,
>>>>
>>>> atomic_begin();
>>>>
>>>> atomic_update();  /* this will call win_commit callback to set a overlay
>>>> relevant registers and enable its dma channel. */
>>>>
>>>> atomic_flush();
>>>>
>>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
>>>> be guaranteed.
>>>
>>> I think we can go down that road, but I'd suggest we push the atomic
>>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
>>> work on the change you are proposing as a follow-up together with the 
>>> other improvements for atomic I already have queued here. This way
>>> we don't take the risk of missing one more merge window.
>>
>> We(I and Joonyoung) will have discussion about this patch series. For
>> this, we have already started to analyze entire atomic features
>> including your patch set so I'd merge it at end of next week according
>> to the discussion. I'm not kind of sure yet but I will merge it as long
>> as there is no big problem.
>>
> 
> Actually i agree to opinion of Gustavo and will repost the patchset of
> Gustavo with some patches fixed by me.
> 

Hmm, i meet problem of operations order. It's called .atomic_update
before enable crtc and called .atomic_disable after disable crtc. It
means .win_commit and .win_disable just return 0 without any operations
because e.g. mixer_ctx->powered is 0 in mixer driver, so we cannot
enable or disable overlay normally.

To reproduce the problem, use modetest -s and after terminate it, i
can't see framebuffer output of fbdev because primary overlay isn't
enabled yet.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Gustavo Padovan]

2015-05-28 Joonyoung Shim <jy0922.shim@samsung.com>:

> On 05/28/2015 05:24 PM, Joonyoung Shim wrote:
> > On 05/28/2015 02:39 PM, Inki Dae wrote:
> >> Hi Gustavo,
> >>
> >> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
> >>> Hi Inki,
> >>>
> >>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
> >>>
> >>>> Hi Gustavo,
> >>>>
> >>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
> >>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>>>
> >>>>> Run dpms operations through the atomic intefaces. This basically removes
> >>>>> the .dpms() callback from econders and crtcs and use .disable() and
> >>>>> .enable() to turn the crtc on and off.
> >>>>>
> >>>>> 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
> >>>>>
> >>>>> 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    | 95 ++++++++++++++++-------------
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
> >>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
> >>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
> >>>>>  10 files changed, 69 insertions(+), 75 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)
> >>>>>  }
> >>>>>  
> >>>>
> >>>> [--snip--]
> >>>>
> >>>>>  
> >>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> >>>>> -	.dpms		= exynos_drm_crtc_dpms,
> >>>>> -	.prepare	= exynos_drm_crtc_prepare,
> >>>>> -	.commit		= exynos_drm_crtc_commit,
> >>>>> +	.enable		= exynos_drm_crtc_enable,
> >>>>> +	.disable	= exynos_drm_crtc_disable,
> >>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
> >>>>>  	.mode_set	= drm_helper_crtc_mode_set,
> >>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
> >>>>
> >>>> I think it'd be better to use atomic_flush callback to enable global dma
> >>>> like commit callback did. Is there any reason that you don't use
> >>>> atomic_begin and atomic_flush callbacks?
> >>>>
> >>>> atomic relevant codes I looked into do as follows,
> >>>>
> >>>> atomic_begin();
> >>>>
> >>>> atomic_update();  /* this will call win_commit callback to set a overlay
> >>>> relevant registers and enable its dma channel. */
> >>>>
> >>>> atomic_flush();
> >>>>
> >>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
> >>>> be guaranteed.
> >>>
> >>> I think we can go down that road, but I'd suggest we push the atomic
> >>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
> >>> work on the change you are proposing as a follow-up together with the 
> >>> other improvements for atomic I already have queued here. This way
> >>> we don't take the risk of missing one more merge window.
> >>
> >> We(I and Joonyoung) will have discussion about this patch series. For
> >> this, we have already started to analyze entire atomic features
> >> including your patch set so I'd merge it at end of next week according
> >> to the discussion. I'm not kind of sure yet but I will merge it as long
> >> as there is no big problem.
> >>
> > 
> > Actually i agree to opinion of Gustavo and will repost the patchset of
> > Gustavo with some patches fixed by me.
> > 
> 
> Hmm, i meet problem of operations order. It's called .atomic_update
> before enable crtc and called .atomic_disable after disable crtc. It
> means .win_commit and .win_disable just return 0 without any operations
> because e.g. mixer_ctx->powered is 0 in mixer driver, so we cannot
> enable or disable overlay normally.

The removal of the extra win_commit() from exynos_drm_crtc_enable() that
you pointed out in the last review round led to this issue. The
win_commit() call was hiding the issue since we were calling it a second
time with the FIMD device already enabled.

I think we can solve this by creating a specific exynos atomic_commit()
callback that does call

drm_atomic_helper_commit_modeset_enables(dev, state);                   

before

drm_atomic_helper_commit_planes(dev, state);


This is the opposite order of what drm atomic default implementation
would do, but we won't hit the issue of having the FIMD clks disabled
when trying to setup the plane. This similar to rcar-du solution to the 
same problem.

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

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Joonyoung Shim]

On 05/29/2015 06:36 AM, Gustavo Padovan wrote:
> 2015-05-28 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> On 05/28/2015 05:24 PM, Joonyoung Shim wrote:
>>> On 05/28/2015 02:39 PM, Inki Dae wrote:
>>>> Hi Gustavo,
>>>>
>>>> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
>>>>> Hi Inki,
>>>>>
>>>>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
>>>>>
>>>>>> Hi Gustavo,
>>>>>>
>>>>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
>>>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>>>
>>>>>>> Run dpms operations through the atomic intefaces. This basically removes
>>>>>>> the .dpms() callback from econders and crtcs and use .disable() and
>>>>>>> .enable() to turn the crtc on and off.
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>> 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    | 95 ++++++++++++++++-------------
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>>>>>>>  10 files changed, 69 insertions(+), 75 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)
>>>>>>>  }
>>>>>>>  
>>>>>>
>>>>>> [--snip--]
>>>>>>
>>>>>>>  
>>>>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>>>>>>> -	.dpms		= exynos_drm_crtc_dpms,
>>>>>>> -	.prepare	= exynos_drm_crtc_prepare,
>>>>>>> -	.commit		= exynos_drm_crtc_commit,
>>>>>>> +	.enable		= exynos_drm_crtc_enable,
>>>>>>> +	.disable	= exynos_drm_crtc_disable,
>>>>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>>>>>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>>>>>>
>>>>>> I think it'd be better to use atomic_flush callback to enable global dma
>>>>>> like commit callback did. Is there any reason that you don't use
>>>>>> atomic_begin and atomic_flush callbacks?
>>>>>>
>>>>>> atomic relevant codes I looked into do as follows,
>>>>>>
>>>>>> atomic_begin();
>>>>>>
>>>>>> atomic_update();  /* this will call win_commit callback to set a overlay
>>>>>> relevant registers and enable its dma channel. */
>>>>>>
>>>>>> atomic_flush();
>>>>>>
>>>>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
>>>>>> be guaranteed.
>>>>>
>>>>> I think we can go down that road, but I'd suggest we push the atomic
>>>>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
>>>>> work on the change you are proposing as a follow-up together with the 
>>>>> other improvements for atomic I already have queued here. This way
>>>>> we don't take the risk of missing one more merge window.
>>>>
>>>> We(I and Joonyoung) will have discussion about this patch series. For
>>>> this, we have already started to analyze entire atomic features
>>>> including your patch set so I'd merge it at end of next week according
>>>> to the discussion. I'm not kind of sure yet but I will merge it as long
>>>> as there is no big problem.
>>>>
>>>
>>> Actually i agree to opinion of Gustavo and will repost the patchset of
>>> Gustavo with some patches fixed by me.
>>>
>>
>> Hmm, i meet problem of operations order. It's called .atomic_update
>> before enable crtc and called .atomic_disable after disable crtc. It
>> means .win_commit and .win_disable just return 0 without any operations
>> because e.g. mixer_ctx->powered is 0 in mixer driver, so we cannot
>> enable or disable overlay normally.
> 
> The removal of the extra win_commit() from exynos_drm_crtc_enable() that
> you pointed out in the last review round led to this issue. The
> win_commit() call was hiding the issue since we were calling it a second
> time with the FIMD device already enabled.
> 
> I think we can solve this by creating a specific exynos atomic_commit()
> callback that does call
> 
> drm_atomic_helper_commit_modeset_enables(dev, state);                   
> 
> before
> 
> drm_atomic_helper_commit_planes(dev, state);
> 
> 
> This is the opposite order of what drm atomic default implementation
> would do, but we won't hit the issue of having the FIMD clks disabled
> when trying to setup the plane. This similar to rcar-du solution to the 
> same problem.
> 

Yeah, but it can solve only enable operation order, disable operation
order still is wrong. It's not big problem yet because drm drivers
internally disable planes when crtc is disabled so try to disable again
already disabled plane.

I'm not sure this is workaround but it seems be simple solution at
present.

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Gustavo Padovan]

2015-05-29 Joonyoung Shim <jy0922.shim@samsung.com>:

> On 05/29/2015 06:36 AM, Gustavo Padovan wrote:
> > 2015-05-28 Joonyoung Shim <jy0922.shim@samsung.com>:
> > 
> >> On 05/28/2015 05:24 PM, Joonyoung Shim wrote:
> >>> On 05/28/2015 02:39 PM, Inki Dae wrote:
> >>>> Hi Gustavo,
> >>>>
> >>>> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
> >>>>> Hi Inki,
> >>>>>
> >>>>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
> >>>>>
> >>>>>> Hi Gustavo,
> >>>>>>
> >>>>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
> >>>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>>>>>
> >>>>>>> Run dpms operations through the atomic intefaces. This basically removes
> >>>>>>> the .dpms() callback from econders and crtcs and use .disable() and
> >>>>>>> .enable() to turn the crtc on and off.
> >>>>>>>
> >>>>>>> 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
> >>>>>>>
> >>>>>>> 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    | 95 ++++++++++++++++-------------
> >>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
> >>>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
> >>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
> >>>>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
> >>>>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
> >>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
> >>>>>>>  10 files changed, 69 insertions(+), 75 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)
> >>>>>>>  }
> >>>>>>>  
> >>>>>>
> >>>>>> [--snip--]
> >>>>>>
> >>>>>>>  
> >>>>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> >>>>>>> -	.dpms		= exynos_drm_crtc_dpms,
> >>>>>>> -	.prepare	= exynos_drm_crtc_prepare,
> >>>>>>> -	.commit		= exynos_drm_crtc_commit,
> >>>>>>> +	.enable		= exynos_drm_crtc_enable,
> >>>>>>> +	.disable	= exynos_drm_crtc_disable,
> >>>>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
> >>>>>>>  	.mode_set	= drm_helper_crtc_mode_set,
> >>>>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
> >>>>>>
> >>>>>> I think it'd be better to use atomic_flush callback to enable global dma
> >>>>>> like commit callback did. Is there any reason that you don't use
> >>>>>> atomic_begin and atomic_flush callbacks?
> >>>>>>
> >>>>>> atomic relevant codes I looked into do as follows,
> >>>>>>
> >>>>>> atomic_begin();
> >>>>>>
> >>>>>> atomic_update();  /* this will call win_commit callback to set a overlay
> >>>>>> relevant registers and enable its dma channel. */
> >>>>>>
> >>>>>> atomic_flush();
> >>>>>>
> >>>>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
> >>>>>> be guaranteed.
> >>>>>
> >>>>> I think we can go down that road, but I'd suggest we push the atomic
> >>>>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
> >>>>> work on the change you are proposing as a follow-up together with the 
> >>>>> other improvements for atomic I already have queued here. This way
> >>>>> we don't take the risk of missing one more merge window.
> >>>>
> >>>> We(I and Joonyoung) will have discussion about this patch series. For
> >>>> this, we have already started to analyze entire atomic features
> >>>> including your patch set so I'd merge it at end of next week according
> >>>> to the discussion. I'm not kind of sure yet but I will merge it as long
> >>>> as there is no big problem.
> >>>>
> >>>
> >>> Actually i agree to opinion of Gustavo and will repost the patchset of
> >>> Gustavo with some patches fixed by me.
> >>>
> >>
> >> Hmm, i meet problem of operations order. It's called .atomic_update
> >> before enable crtc and called .atomic_disable after disable crtc. It
> >> means .win_commit and .win_disable just return 0 without any operations
> >> because e.g. mixer_ctx->powered is 0 in mixer driver, so we cannot
> >> enable or disable overlay normally.
> > 
> > The removal of the extra win_commit() from exynos_drm_crtc_enable() that
> > you pointed out in the last review round led to this issue. The
> > win_commit() call was hiding the issue since we were calling it a second
> > time with the FIMD device already enabled.
> > 
> > I think we can solve this by creating a specific exynos atomic_commit()
> > callback that does call
> > 
> > drm_atomic_helper_commit_modeset_enables(dev, state);                   
> > 
> > before
> > 
> > drm_atomic_helper_commit_planes(dev, state);
> > 
> > 
> > This is the opposite order of what drm atomic default implementation
> > would do, but we won't hit the issue of having the FIMD clks disabled
> > when trying to setup the plane. This similar to rcar-du solution to the 
> > same problem.
> > 
> 
> Yeah, but it can solve only enable operation order, disable operation
> order still is wrong. It's not big problem yet because drm drivers
> internally disable planes when crtc is disabled so try to disable again
> already disabled plane.
> 
> I'm not sure this is workaround but it seems be simple solution at
> present.

This is not workaround, is a valid change according to the atomic doc:

 * For compatability with legacy crtc helpers this should be called before      
 * drm_atomic_helper_commit_planes(), which is what the default commit function 
 * does. But drivers with different needs can group the modeset commits together
 * and do the plane commits at the end. This is useful for drivers doing runtime
 * PM since planes updates then only happen when the CRTC is actually enabled.

So it is completely fine to go ahead with this change as we are now fully ported
to the atomic helpers.

	Gustavo

Re: [PATCH v7 12/13] drm/exynos: atomic dpms support

[Joonyoung Shim]

On 05/30/2015 06:33 AM, Gustavo Padovan wrote:
> 2015-05-29 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> On 05/29/2015 06:36 AM, Gustavo Padovan wrote:
>>> 2015-05-28 Joonyoung Shim <jy0922.shim@samsung.com>:
>>>
>>>> On 05/28/2015 05:24 PM, Joonyoung Shim wrote:
>>>>> On 05/28/2015 02:39 PM, Inki Dae wrote:
>>>>>> Hi Gustavo,
>>>>>>
>>>>>> On 2015년 05월 28일 05:27, Gustavo Padovan wrote:
>>>>>>> Hi Inki,
>>>>>>>
>>>>>>> 2015-05-27 Inki Dae <inki.dae@samsung.com>:
>>>>>>>
>>>>>>>> Hi Gustavo,
>>>>>>>>
>>>>>>>> On 2015년 05월 23일 00:40, Gustavo Padovan wrote:
>>>>>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>>>>>>
>>>>>>>>> Run dpms operations through the atomic intefaces. This basically removes
>>>>>>>>> the .dpms() callback from econders and crtcs and use .disable() and
>>>>>>>>> .enable() to turn the crtc on and off.
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>> 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    | 95 ++++++++++++++++-------------
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c     |  2 +-
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  4 +-
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c     |  2 +-
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 27 ++------
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  2 +-
>>>>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c        |  6 +-
>>>>>>>>>  10 files changed, 69 insertions(+), 75 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)
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>
>>>>>>>> [--snip--]
>>>>>>>>
>>>>>>>>>  
>>>>>>>>>  static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
>>>>>>>>> -	.dpms		= exynos_drm_crtc_dpms,
>>>>>>>>> -	.prepare	= exynos_drm_crtc_prepare,
>>>>>>>>> -	.commit		= exynos_drm_crtc_commit,
>>>>>>>>> +	.enable		= exynos_drm_crtc_enable,
>>>>>>>>> +	.disable	= exynos_drm_crtc_disable,
>>>>>>>>>  	.mode_fixup	= exynos_drm_crtc_mode_fixup,
>>>>>>>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>>>>>>>  	.mode_set_nofb	= exynos_drm_crtc_mode_set_nofb,
>>>>>>>>
>>>>>>>> I think it'd be better to use atomic_flush callback to enable global dma
>>>>>>>> like commit callback did. Is there any reason that you don't use
>>>>>>>> atomic_begin and atomic_flush callbacks?
>>>>>>>>
>>>>>>>> atomic relevant codes I looked into do as follows,
>>>>>>>>
>>>>>>>> atomic_begin();
>>>>>>>>
>>>>>>>> atomic_update();  /* this will call win_commit callback to set a overlay
>>>>>>>> relevant registers and enable its dma channel. */
>>>>>>>>
>>>>>>>> atomic_flush();
>>>>>>>>
>>>>>>>> So atomic overlay updating between atomic_begin() ~ atomic_flush() will
>>>>>>>> be guaranteed.
>>>>>>>
>>>>>>> I think we can go down that road, but I'd suggest we push the atomic
>>>>>>> patches v8 (with the lastest comments from Joonyoung fixed) and then 
>>>>>>> work on the change you are proposing as a follow-up together with the 
>>>>>>> other improvements for atomic I already have queued here. This way
>>>>>>> we don't take the risk of missing one more merge window.
>>>>>>
>>>>>> We(I and Joonyoung) will have discussion about this patch series. For
>>>>>> this, we have already started to analyze entire atomic features
>>>>>> including your patch set so I'd merge it at end of next week according
>>>>>> to the discussion. I'm not kind of sure yet but I will merge it as long
>>>>>> as there is no big problem.
>>>>>>
>>>>>
>>>>> Actually i agree to opinion of Gustavo and will repost the patchset of
>>>>> Gustavo with some patches fixed by me.
>>>>>
>>>>
>>>> Hmm, i meet problem of operations order. It's called .atomic_update
>>>> before enable crtc and called .atomic_disable after disable crtc. It
>>>> means .win_commit and .win_disable just return 0 without any operations
>>>> because e.g. mixer_ctx->powered is 0 in mixer driver, so we cannot
>>>> enable or disable overlay normally.
>>>
>>> The removal of the extra win_commit() from exynos_drm_crtc_enable() that
>>> you pointed out in the last review round led to this issue. The
>>> win_commit() call was hiding the issue since we were calling it a second
>>> time with the FIMD device already enabled.
>>>
>>> I think we can solve this by creating a specific exynos atomic_commit()
>>> callback that does call
>>>
>>> drm_atomic_helper_commit_modeset_enables(dev, state);                   
>>>
>>> before
>>>
>>> drm_atomic_helper_commit_planes(dev, state);
>>>
>>>
>>> This is the opposite order of what drm atomic default implementation
>>> would do, but we won't hit the issue of having the FIMD clks disabled
>>> when trying to setup the plane. This similar to rcar-du solution to the 
>>> same problem.
>>>
>>
>> Yeah, but it can solve only enable operation order, disable operation
>> order still is wrong. It's not big problem yet because drm drivers
>> internally disable planes when crtc is disabled so try to disable again
>> already disabled plane.
>>
>> I'm not sure this is workaround but it seems be simple solution at
>> present.
> 
> This is not workaround, is a valid change according to the atomic doc:
> 
>  * For compatability with legacy crtc helpers this should be called before      
>  * drm_atomic_helper_commit_planes(), which is what the default commit function 
>  * does. But drivers with different needs can group the modeset commits together
>  * and do the plane commits at the end. This is useful for drivers doing runtime
>  * PM since planes updates then only happen when the CRTC is actually enabled.
> 
> So it is completely fine to go ahead with this change as we are now fully ported
> to the atomic helpers.
> 

Right, we can go ahead. Just as i said, disable operation order is
unnatural a bit.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 13/13] 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 ca501a9..3fbc543 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -44,8 +44,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;
@@ -61,15 +59,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