[PATCH v2 0/6] drm: rcar-du: Rework CRTC and groups for atomic commits

[PATCH v2 0/6] drm: rcar-du: Rework CRTC and groups for atomic commits

[Kieran Bingham]

The DU processes the CRTC and group objects, with various operations storing
state of those objects globally, along with performing configuration operations
in the atomic_begin handlers.

This series aims to refactor the code to establish a private object for the
group state, and move all hardware operations to occur only in the commit_tail
code paths.

The sequence of register writes to the DU is confirmed to be unchanged by this
series, however follow on patches after this may begin to modify the sequences.
Because of this, these patches are kept as their own series.

Note that the rcar_du_crtc_get() in rcar_du_crtc_atomic_begin() is safe to
remove, as the call to rcar_du_vsp_atomic_begin() now maps to a no-op, but is
kept for API integrity, though it could now be removed.

Removal of rcar_du_vsp_atomic_begin() and indeed rcar_du_crtc_atomic_begin()
could occur in a later patch, but needs to be synchronised with VSP update.

These patches are based on top of my previous cleanup series, which is on top
of drm-next.

Kieran Bingham (6):
  drm: rcar-du: Link CRTCs to the DU device
  drm: rcar-du: Add CRTC standby helpers
  drm: rcar-du: Add pre/post commit CRTC helpers
  drm: rcar-du: Provide for_each_group helper
  drm: rcar-du: Create a group state object
  drm: rcar-du: Add group hooks for atomic-commit

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 150 +++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  14 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   5 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 182 +++++++++++++++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  36 ++++-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  49 +++----
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |   2 +-
 7 files changed, 305 insertions(+), 133 deletions(-)

-- 
2.19.1

[PATCH v2 1/6] drm: rcar-du: Link CRTCs to the DU device

[Kieran Bingham]

The rcar_du_crtc functions have a heavy reliance on the rcar_du_group
structure, in many cases just to access the DU device context.

To better separate the groups out of the CRTC handling code, give the
rcar_du_crtc its own pointer to the device and remove the indirection
through the group pointers.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 48 +++++++++++++-------------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 57fd5c00494b..471ce464654a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -32,21 +32,21 @@
 
 static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	return rcar_du_read(rcdu, rcrtc->mmio_offset + reg);
 }
 
 static void rcar_du_crtc_write(struct rcar_du_crtc *rcrtc, u32 reg, u32 data)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, data);
 }
 
 static void rcar_du_crtc_clr(struct rcar_du_crtc *rcrtc, u32 reg, u32 clr)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_write(rcdu, rcrtc->mmio_offset + reg,
 		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) & ~clr);
@@ -54,7 +54,7 @@ static void rcar_du_crtc_clr(struct rcar_du_crtc *rcrtc, u32 reg, u32 clr)
 
 static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_write(rcdu, rcrtc->mmio_offset + reg,
 		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) | set);
@@ -62,7 +62,7 @@ static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
 
 void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcrtc->dsysr = (rcrtc->dsysr & ~clr) | set;
 	rcar_du_write(rcdu, rcrtc->mmio_offset + DSYSR, rcrtc->dsysr);
@@ -157,10 +157,9 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 	}
 
 done:
-	dev_dbg(rcrtc->group->dev->dev,
+	dev_dbg(rcrtc->dev->dev,
 		"output:%u, fdpll:%u, n:%u, m:%u, diff:%lu\n",
-		 dpll->output, dpll->fdpll, dpll->n, dpll->m,
-		 best_diff);
+		 dpll->output, dpll->fdpll, dpll->n, dpll->m, best_diff);
 }
 
 struct du_clk_params {
@@ -212,7 +211,7 @@ static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
 static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 {
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	unsigned long mode_clock = mode->clock * 1000;
 	u32 dsmr;
 	u32 escr;
@@ -277,7 +276,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 			rcar_du_escr_divider(rcrtc->extclock, mode_clock,
 					     ESCR_DCLKSEL_DCLKIN, &params);
 
-		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
+		dev_dbg(rcrtc->dev->dev, "mode clock %lu %s rate %lu\n",
 			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
 			params.rate);
 
@@ -285,7 +284,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		escr = params.escr;
 	}
 
-	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
+	dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
 
 	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
 	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
@@ -333,7 +332,7 @@ plane_format(struct rcar_du_plane *plane)
 static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
 {
 	struct rcar_du_plane *planes[RCAR_DU_NUM_HW_PLANES];
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	unsigned int num_planes = 0;
 	unsigned int dptsr_planes;
 	unsigned int hwplanes = 0;
@@ -463,7 +462,7 @@ static bool rcar_du_crtc_page_flip_pending(struct rcar_du_crtc *rcrtc)
 
 static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	if (wait_event_timeout(rcrtc->flip_wait,
 			       !rcar_du_crtc_page_flip_pending(rcrtc),
@@ -493,7 +492,7 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
 	/* Enable the VSP compositor. */
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_enable(rcrtc);
 
 	/* Turn vertical blanking interrupt reporting on. */
@@ -564,7 +563,7 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 
 static void rcar_du_crtc_disable_planes(struct rcar_du_crtc *rcrtc)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	struct drm_crtc *crtc = &rcrtc->crtc;
 	u32 status;
 
@@ -617,7 +616,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	drm_crtc_vblank_off(crtc);
 
 	/* Disable the VSP compositor. */
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_disable(rcrtc);
 
 	/*
@@ -627,7 +626,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	 * TODO: Find another way to stop the display for DUs that don't support
 	 * TVM sync.
 	 */
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_TVM_SYNC))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_TVM_SYNC))
 		rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK,
 					   DSYSR_TVM_SWITCH);
 
@@ -661,7 +660,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_crtc_get(rcrtc);
 
@@ -689,7 +688,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(old_state);
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_crtc_stop(rcrtc);
 	rcar_du_crtc_put(rcrtc);
@@ -735,7 +734,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 	 */
 	rcar_du_crtc_get(rcrtc);
 
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_begin(rcrtc);
 }
 
@@ -757,7 +756,7 @@ static void rcar_du_crtc_atomic_flush(struct drm_crtc *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_flush(rcrtc);
 }
 
@@ -766,7 +765,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
 			const struct drm_display_mode *mode)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
 	unsigned int vbp;
 
@@ -798,7 +797,7 @@ static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
 
 static void rcar_du_crtc_crc_init(struct rcar_du_crtc *rcrtc)
 {
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	const char **sources;
 	unsigned int count;
 	int i = -1;
@@ -1080,7 +1079,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
 static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 {
 	struct rcar_du_crtc *rcrtc = arg;
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
+	struct rcar_du_device *rcdu = rcrtc->dev;
 	irqreturn_t ret = IRQ_NONE;
 	u32 status;
 
@@ -1172,6 +1171,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 	init_waitqueue_head(&rcrtc->vblank_wait);
 	spin_lock_init(&rcrtc->vblank_lock);
 
+	rcrtc->dev = rcdu;
 	rcrtc->group = rgrp;
 	rcrtc->mmio_offset = mmio_offsets[hwindex];
 	rcrtc->index = hwindex;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 3f339a7e8d14..11814eafef77 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -24,6 +24,7 @@ struct rcar_du_vsp;
 /**
  * struct rcar_du_crtc - the CRTC, representing a DU superposition processor
  * @crtc: base DRM CRTC
+ * @dev: the DU device
  * @clock: the CRTC functional clock
  * @extclock: external pixel dot clock (optional)
  * @mmio_offset: offset of the CRTC registers in the DU MMIO block
@@ -43,6 +44,7 @@ struct rcar_du_vsp;
 struct rcar_du_crtc {
 	struct drm_crtc crtc;
 
+	struct rcar_du_device *dev;
 	struct clk *clock;
 	struct clk *extclock;
 	unsigned int mmio_offset;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 0878accbd134..f6deea90dcce 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -43,7 +43,7 @@ static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 {
 	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
-	struct rcar_du_device *rcdu = crtc->group->dev;
+	struct rcar_du_device *rcdu = crtc->dev;
 	struct vsp1_du_lif_config cfg = {
 		.width = mode->hdisplay,
 		.height = mode->vdisplay,
-- 
2.19.1

Re: [PATCH v2 1/6] drm: rcar-du: Link CRTCs to the DU device

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 15, 2019 at 05:01:05PM +0000, Kieran Bingham wrote:
> The rcar_du_crtc functions have a heavy reliance on the rcar_du_group
> structure, in many cases just to access the DU device context.
> 
> To better separate the groups out of the CRTC handling code, give the
> rcar_du_crtc its own pointer to the device and remove the indirection
> through the group pointers.

I've thought about doing this so many times, every time I had to access
the device from the crtc.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

And taken in my tree.

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 48 +++++++++++++-------------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
>  3 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 57fd5c00494b..471ce464654a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -32,21 +32,21 @@
>  
>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	return rcar_du_read(rcdu, rcrtc->mmio_offset + reg);
>  }
>  
>  static void rcar_du_crtc_write(struct rcar_du_crtc *rcrtc, u32 reg, u32 data)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, data);
>  }
>  
>  static void rcar_du_crtc_clr(struct rcar_du_crtc *rcrtc, u32 reg, u32 clr)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_write(rcdu, rcrtc->mmio_offset + reg,
>  		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) & ~clr);
> @@ -54,7 +54,7 @@ static void rcar_du_crtc_clr(struct rcar_du_crtc *rcrtc, u32 reg, u32 clr)
>  
>  static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_write(rcdu, rcrtc->mmio_offset + reg,
>  		      rcar_du_read(rcdu, rcrtc->mmio_offset + reg) | set);
> @@ -62,7 +62,7 @@ static void rcar_du_crtc_set(struct rcar_du_crtc *rcrtc, u32 reg, u32 set)
>  
>  void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcrtc->dsysr = (rcrtc->dsysr & ~clr) | set;
>  	rcar_du_write(rcdu, rcrtc->mmio_offset + DSYSR, rcrtc->dsysr);
> @@ -157,10 +157,9 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
>  	}
>  
>  done:
> -	dev_dbg(rcrtc->group->dev->dev,
> +	dev_dbg(rcrtc->dev->dev,
>  		"output:%u, fdpll:%u, n:%u, m:%u, diff:%lu\n",
> -		 dpll->output, dpll->fdpll, dpll->n, dpll->m,
> -		 best_diff);
> +		 dpll->output, dpll->fdpll, dpll->n, dpll->m, best_diff);
>  }
>  
>  struct du_clk_params {
> @@ -212,7 +211,7 @@ static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
>  static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  {
>  	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	unsigned long mode_clock = mode->clock * 1000;
>  	u32 dsmr;
>  	u32 escr;
> @@ -277,7 +276,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  			rcar_du_escr_divider(rcrtc->extclock, mode_clock,
>  					     ESCR_DCLKSEL_DCLKIN, &params);
>  
> -		dev_dbg(rcrtc->group->dev->dev,	"mode clock %lu %s rate %lu\n",
> +		dev_dbg(rcrtc->dev->dev, "mode clock %lu %s rate %lu\n",
>  			mode_clock, params.clk == rcrtc->clock ? "cpg" : "ext",
>  			params.rate);
>  
> @@ -285,7 +284,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  		escr = params.escr;
>  	}
>  
> -	dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +	dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
>  
>  	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
>  	rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
> @@ -333,7 +332,7 @@ plane_format(struct rcar_du_plane *plane)
>  static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
>  {
>  	struct rcar_du_plane *planes[RCAR_DU_NUM_HW_PLANES];
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	unsigned int num_planes = 0;
>  	unsigned int dptsr_planes;
>  	unsigned int hwplanes = 0;
> @@ -463,7 +462,7 @@ static bool rcar_du_crtc_page_flip_pending(struct rcar_du_crtc *rcrtc)
>  
>  static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	if (wait_event_timeout(rcrtc->flip_wait,
>  			       !rcar_du_crtc_page_flip_pending(rcrtc),
> @@ -493,7 +492,7 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>  	/* Enable the VSP compositor. */
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_enable(rcrtc);
>  
>  	/* Turn vertical blanking interrupt reporting on. */
> @@ -564,7 +563,7 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  
>  static void rcar_du_crtc_disable_planes(struct rcar_du_crtc *rcrtc)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	struct drm_crtc *crtc = &rcrtc->crtc;
>  	u32 status;
>  
> @@ -617,7 +616,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	drm_crtc_vblank_off(crtc);
>  
>  	/* Disable the VSP compositor. */
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_disable(rcrtc);
>  
>  	/*
> @@ -627,7 +626,7 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	 * TODO: Find another way to stop the display for DUs that don't support
>  	 * TVM sync.
>  	 */
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_TVM_SYNC))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_TVM_SYNC))
>  		rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_TVM_MASK,
>  					   DSYSR_TVM_SWITCH);
>  
> @@ -661,7 +660,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_crtc_get(rcrtc);
>  
> @@ -689,7 +688,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(old_state);
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_crtc_stop(rcrtc);
>  	rcar_du_crtc_put(rcrtc);
> @@ -735,7 +734,7 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>  	 */
>  	rcar_du_crtc_get(rcrtc);
>  
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_atomic_begin(rcrtc);
>  }
>  
> @@ -757,7 +756,7 @@ static void rcar_du_crtc_atomic_flush(struct drm_crtc *crtc,
>  		spin_unlock_irqrestore(&dev->event_lock, flags);
>  	}
>  
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_atomic_flush(rcrtc);
>  }
>  
> @@ -766,7 +765,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
>  			const struct drm_display_mode *mode)
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
>  	unsigned int vbp;
>  
> @@ -798,7 +797,7 @@ static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
>  
>  static void rcar_du_crtc_crc_init(struct rcar_du_crtc *rcrtc)
>  {
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	const char **sources;
>  	unsigned int count;
>  	int i = -1;
> @@ -1080,7 +1079,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  {
>  	struct rcar_du_crtc *rcrtc = arg;
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct rcar_du_device *rcdu = rcrtc->dev;
>  	irqreturn_t ret = IRQ_NONE;
>  	u32 status;
>  
> @@ -1172,6 +1171,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	init_waitqueue_head(&rcrtc->vblank_wait);
>  	spin_lock_init(&rcrtc->vblank_lock);
>  
> +	rcrtc->dev = rcdu;
>  	rcrtc->group = rgrp;
>  	rcrtc->mmio_offset = mmio_offsets[hwindex];
>  	rcrtc->index = hwindex;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 3f339a7e8d14..11814eafef77 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -24,6 +24,7 @@ struct rcar_du_vsp;
>  /**
>   * struct rcar_du_crtc - the CRTC, representing a DU superposition processor
>   * @crtc: base DRM CRTC
> + * @dev: the DU device
>   * @clock: the CRTC functional clock
>   * @extclock: external pixel dot clock (optional)
>   * @mmio_offset: offset of the CRTC registers in the DU MMIO block
> @@ -43,6 +44,7 @@ struct rcar_du_vsp;
>  struct rcar_du_crtc {
>  	struct drm_crtc crtc;
>  
> +	struct rcar_du_device *dev;
>  	struct clk *clock;
>  	struct clk *extclock;
>  	unsigned int mmio_offset;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 0878accbd134..f6deea90dcce 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -43,7 +43,7 @@ static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>  	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
> -	struct rcar_du_device *rcdu = crtc->group->dev;
> +	struct rcar_du_device *rcdu = crtc->dev;
>  	struct vsp1_du_lif_config cfg = {
>  		.width = mode->hdisplay,
>  		.height = mode->vdisplay,

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/6] drm: rcar-du: Add CRTC standby helpers

[Kieran Bingham]

Provide helpers to manage the power state, and initial configuration of
the CRTC.

rcar_du_crtc_get() and rcar_du_crtc_get() are no longer used, and are
removed, simplifying the implementation and removing the initialized
flag which was needed to track the state of the CRTC.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
v2:
 - Registers sequence confirmed unchanged
 - re-ordered in the series to handle before groups.
 - Do not merge rcar_du_crtc_setup(). (now handled by _crtc_pre_commit)

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 69 +++++++++++++++-----------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  7 ++-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  4 ++
 3 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 471ce464654a..6109a97b0bb9 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -499,17 +499,10 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 	drm_crtc_vblank_on(&rcrtc->crtc);
 }
 
-static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
+static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
 {
 	int ret;
 
-	/*
-	 * Guard against double-get, as the function is called from both the
-	 * .atomic_enable() and .atomic_begin() handlers.
-	 */
-	if (rcrtc->initialized)
-		return 0;
-
 	ret = clk_prepare_enable(rcrtc->clock);
 	if (ret < 0)
 		return ret;
@@ -523,7 +516,6 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 		goto error_group;
 
 	rcar_du_crtc_setup(rcrtc);
-	rcrtc->initialized = true;
 
 	return 0;
 
@@ -534,14 +526,12 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 	return ret;
 }
 
-static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
+static void rcar_du_crtc_disable(struct rcar_du_crtc *rcrtc)
 {
 	rcar_du_group_put(rcrtc->group);
 
 	clk_disable_unprepare(rcrtc->extclock);
 	clk_disable_unprepare(rcrtc->clock);
-
-	rcrtc->initialized = false;
 }
 
 static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
@@ -655,6 +645,44 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
+int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
+				     struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+
+		if (crtc_state->active_changed && crtc_state->active) {
+			int ret = rcar_du_crtc_enable(rcrtc);
+
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
+				      struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+
+		if (crtc_state->active_changed && !crtc_state->active)
+			rcar_du_crtc_disable(rcrtc);
+	}
+
+	return 0;
+}
+
 static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 				       struct drm_crtc_state *old_state)
 {
@@ -662,8 +690,6 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
 	struct rcar_du_device *rcdu = rcrtc->dev;
 
-	rcar_du_crtc_get(rcrtc);
-
 	/*
 	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
 	 * the DU channel. We need to enable its clock output explicitly if
@@ -691,7 +717,6 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
 	struct rcar_du_device *rcdu = rcrtc->dev;
 
 	rcar_du_crtc_stop(rcrtc);
-	rcar_du_crtc_put(rcrtc);
 
 	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
 	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
@@ -720,20 +745,6 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 
 	WARN_ON(!crtc->state->enable);
 
-	/*
-	 * If a mode set is in progress we can be called with the CRTC disabled.
-	 * We thus need to first get and setup the CRTC in order to configure
-	 * planes. We must *not* put the CRTC in .atomic_flush(), as it must be
-	 * kept awake until the .atomic_enable() call that will follow. The get
-	 * operation in .atomic_enable() will in that case be a no-op, and the
-	 * CRTC will be put later in .atomic_disable().
-	 *
-	 * If a mode set is not in progress the CRTC is enabled, and the
-	 * following get call will be a no-op. There is thus no need to balance
-	 * it in .atomic_flush() either.
-	 */
-	rcar_du_crtc_get(rcrtc);
-
 	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_begin(rcrtc);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 11814eafef77..d12d4a788e9f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -29,7 +29,6 @@ struct rcar_du_vsp;
  * @extclock: external pixel dot clock (optional)
  * @mmio_offset: offset of the CRTC registers in the DU MMIO block
  * @index: CRTC software and hardware index
- * @initialized: whether the CRTC has been initialized and clocks enabled
  * @dsysr: cached value of the DSYSR register
  * @vblank_enable: whether vblank events are enabled on this CRTC
  * @event: event to post when the pending page flip completes
@@ -49,7 +48,6 @@ struct rcar_du_crtc {
 	struct clk *extclock;
 	unsigned int mmio_offset;
 	unsigned int index;
-	bool initialized;
 
 	u32 dsysr;
 
@@ -102,6 +100,11 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 
 void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
 
+int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
+				     struct drm_atomic_state *state);
+int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
+				      struct drm_atomic_state *state);
+
 void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
 
 #endif /* __RCAR_DU_CRTC_H__ */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 3b7d50a8fb9b..b8da4dfc79d2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -303,11 +303,15 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	}
 
 	/* Apply the atomic update. */
+	rcar_du_crtc_atomic_exit_standby(dev, old_state);
+
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
+	rcar_du_crtc_atomic_enter_standby(dev, old_state);
+
 	drm_atomic_helper_commit_hw_done(old_state);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
 
-- 
2.19.1

Re: [PATCH v2 2/6] drm: rcar-du: Add CRTC standby helpers

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 15, 2019 at 05:01:06PM +0000, Kieran Bingham wrote:
> Provide helpers to manage the power state, and initial configuration of
> the CRTC.

I would add a sentence here to mention that these helpers operate from
the atomic commit tail handler, and respectively take out of standby all
CRTCs that need to be activated, and put in standby all the CRTCs that
have been deactivated.

> rcar_du_crtc_get() and rcar_du_crtc_get() are no longer used, and are
> removed, simplifying the implementation and removing the initialized
> flag which was needed to track the state of the CRTC.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v2:
>  - Registers sequence confirmed unchanged
>  - re-ordered in the series to handle before groups.
>  - Do not merge rcar_du_crtc_setup(). (now handled by _crtc_pre_commit)
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 69 +++++++++++++++-----------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  7 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  4 ++
>  3 files changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 471ce464654a..6109a97b0bb9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -499,17 +499,10 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  	drm_crtc_vblank_on(&rcrtc->crtc);
>  }
>  
> -static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
> +static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
>  {
>  	int ret;
>  
> -	/*
> -	 * Guard against double-get, as the function is called from both the
> -	 * .atomic_enable() and .atomic_begin() handlers.
> -	 */
> -	if (rcrtc->initialized)
> -		return 0;
> -
>  	ret = clk_prepare_enable(rcrtc->clock);
>  	if (ret < 0)
>  		return ret;
> @@ -523,7 +516,6 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  		goto error_group;
>  
>  	rcar_du_crtc_setup(rcrtc);
> -	rcrtc->initialized = true;
>  
>  	return 0;
>  
> @@ -534,14 +526,12 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  	return ret;
>  }
>  
> -static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
> +static void rcar_du_crtc_disable(struct rcar_du_crtc *rcrtc)
>  {
>  	rcar_du_group_put(rcrtc->group);
>  
>  	clk_disable_unprepare(rcrtc->extclock);
>  	clk_disable_unprepare(rcrtc->clock);
> -
> -	rcrtc->initialized = false;
>  }
>  
>  static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> @@ -655,6 +645,44 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  

Similarly a comment above each function would be useful in my opinion.
Something along the lines of

/*
 * Take all CRTCs that are made active in this commit out of standby.
 * CRTCs that are deactivated by the commit are untouched and will be
 * put in standby by rcar_du_crtc_atomic_enter_standby().
 /

> +int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
> +				     struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned int i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +		if (crtc_state->active_changed && crtc_state->active) {
> +			int ret = rcar_du_crtc_enable(rcrtc);
> +
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

/*
 * Put all CRTCs that have been deactivated by this commit in standby.
 * This shall be called at the end of the commit tail handler as the
 * last operation that touches the CRTC hardware.
 /

> +int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
> +				      struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned int i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +		if (crtc_state->active_changed && !crtc_state->active)
> +			rcar_du_crtc_disable(rcrtc);
> +	}
> +
> +	return 0;
> +}
> +
>  static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  				       struct drm_crtc_state *old_state)
>  {
> @@ -662,8 +690,6 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
>  	struct rcar_du_device *rcdu = rcrtc->dev;
>  
> -	rcar_du_crtc_get(rcrtc);
> -
>  	/*
>  	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
>  	 * the DU channel. We need to enable its clock output explicitly if
> @@ -691,7 +717,6 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  	struct rcar_du_device *rcdu = rcrtc->dev;
>  
>  	rcar_du_crtc_stop(rcrtc);
> -	rcar_du_crtc_put(rcrtc);
>  
>  	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
>  	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
> @@ -720,20 +745,6 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	/*
> -	 * If a mode set is in progress we can be called with the CRTC disabled.
> -	 * We thus need to first get and setup the CRTC in order to configure
> -	 * planes. We must *not* put the CRTC in .atomic_flush(), as it must be
> -	 * kept awake until the .atomic_enable() call that will follow. The get
> -	 * operation in .atomic_enable() will in that case be a no-op, and the
> -	 * CRTC will be put later in .atomic_disable().
> -	 *
> -	 * If a mode set is not in progress the CRTC is enabled, and the
> -	 * following get call will be a no-op. There is thus no need to balance
> -	 * it in .atomic_flush() either.
> -	 */
> -	rcar_du_crtc_get(rcrtc);
> -

I love how this hack is now gone.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_atomic_begin(rcrtc);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 11814eafef77..d12d4a788e9f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -29,7 +29,6 @@ struct rcar_du_vsp;
>   * @extclock: external pixel dot clock (optional)
>   * @mmio_offset: offset of the CRTC registers in the DU MMIO block
>   * @index: CRTC software and hardware index
> - * @initialized: whether the CRTC has been initialized and clocks enabled
>   * @dsysr: cached value of the DSYSR register
>   * @vblank_enable: whether vblank events are enabled on this CRTC
>   * @event: event to post when the pending page flip completes
> @@ -49,7 +48,6 @@ struct rcar_du_crtc {
>  	struct clk *extclock;
>  	unsigned int mmio_offset;
>  	unsigned int index;
> -	bool initialized;
>  
>  	u32 dsysr;
>  
> @@ -102,6 +100,11 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  
>  void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
> +int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
> +				     struct drm_atomic_state *state);
> +int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
> +				      struct drm_atomic_state *state);
> +
>  void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
>  
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 3b7d50a8fb9b..b8da4dfc79d2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -303,11 +303,15 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	}
>  
>  	/* Apply the atomic update. */
> +	rcar_du_crtc_atomic_exit_standby(dev, old_state);
> +
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> +	rcar_du_crtc_atomic_enter_standby(dev, old_state);
> +
>  	drm_atomic_helper_commit_hw_done(old_state);
>  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/6] drm: rcar-du: Add pre/post commit CRTC helpers

[Kieran Bingham]

Provide helpers to allow CRTC configuration to be separated from the power
state handling. rcar_du_crtc_atomic_post_commit() is a no-op, but maintained
for API symmetry.

---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  5 +++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 ++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 6109a97b0bb9..2606de788688 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -515,8 +515,6 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_group;
 
-	rcar_du_crtc_setup(rcrtc);
-
 	return 0;
 
 error_group:
@@ -683,6 +681,29 @@ int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
 	return 0;
 }
 
+int rcar_du_crtc_atomic_pre_commit(struct drm_device *dev,
+				   struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+
+		if (crtc_state->active_changed && crtc_state->active)
+			rcar_du_crtc_setup(rcrtc);
+	}
+
+	return 0;
+}
+
+int rcar_du_crtc_atomic_post_commit(struct drm_device *dev,
+				    struct drm_atomic_state *state)
+{
+	return 0;
+}
+
 static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 				       struct drm_crtc_state *old_state)
 {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index d12d4a788e9f..0b60a6e0b753 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -105,6 +105,11 @@ int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
 int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
 				      struct drm_atomic_state *state);
 
+int rcar_du_crtc_atomic_pre_commit(struct drm_device *dev,
+				   struct drm_atomic_state *state);
+int rcar_du_crtc_atomic_post_commit(struct drm_device *dev,
+				    struct drm_atomic_state *state);
+
 void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
 
 #endif /* __RCAR_DU_CRTC_H__ */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index b8da4dfc79d2..e4befb1937f8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -304,12 +304,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 	/* Apply the atomic update. */
 	rcar_du_crtc_atomic_exit_standby(dev, old_state);
+	rcar_du_crtc_atomic_pre_commit(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
+	rcar_du_crtc_atomic_post_commit(dev, old_state);
 	rcar_du_crtc_atomic_enter_standby(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
-- 
2.19.1

Re: [PATCH v2 3/6] drm: rcar-du: Add pre/post commit CRTC helpers

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 15, 2019 at 05:01:07PM +0000, Kieran Bingham wrote:
> Provide helpers to allow CRTC configuration to be separated from the power
> state handling. rcar_du_crtc_atomic_post_commit() is a no-op, but maintained
> for API symmetry.

Do you think we will need to fill rcar_du_crtc_atomic_post_commit()
later ? If not I wouldn't add it, and I may even rename
rcar_du_crtc_atomic_pre_commit() to rcar_du_crtc_atomic_setup() to make
its purpose clearer.

> 

Missing SoB line ?

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 25 +++++++++++++++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  5 +++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 ++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 6109a97b0bb9..2606de788688 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -515,8 +515,6 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
>  	if (ret < 0)
>  		goto error_group;
>  
> -	rcar_du_crtc_setup(rcrtc);
> -
>  	return 0;
>  
>  error_group:
> @@ -683,6 +681,29 @@ int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
>  	return 0;
>  }
>  
> +int rcar_du_crtc_atomic_pre_commit(struct drm_device *dev,
> +				   struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned int i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +		if (crtc_state->active_changed && crtc_state->active)
> +			rcar_du_crtc_setup(rcrtc);

I wondered why you didn't merge this with the existing
rcar_du_crtc_atomic_exit_standby() function, and saw in a later patch
that you have to introduce another operation in-between. I would explain
this in the commit message.

> +	}
> +
> +	return 0;

As this function and the next one are called in a context that can never
fail, and as the functions never return a failure, I would make them
void.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
> +int rcar_du_crtc_atomic_post_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state)
> +{
> +	return 0;
> +}
> +
>  static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  				       struct drm_crtc_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index d12d4a788e9f..0b60a6e0b753 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -105,6 +105,11 @@ int rcar_du_crtc_atomic_exit_standby(struct drm_device *dev,
>  int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
>  				      struct drm_atomic_state *state);
>  
> +int rcar_du_crtc_atomic_pre_commit(struct drm_device *dev,
> +				   struct drm_atomic_state *state);
> +int rcar_du_crtc_atomic_post_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state);
> +
>  void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
>  
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index b8da4dfc79d2..e4befb1937f8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -304,12 +304,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  
>  	/* Apply the atomic update. */
>  	rcar_du_crtc_atomic_exit_standby(dev, old_state);
> +	rcar_du_crtc_atomic_pre_commit(dev, old_state);
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> +	rcar_du_crtc_atomic_post_commit(dev, old_state);
>  	rcar_du_crtc_atomic_enter_standby(dev, old_state);
>  
>  	drm_atomic_helper_commit_hw_done(old_state);

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] drm: rcar-du: Provide for_each_group helper

[Kieran Bingham]

Refactoring of the group control code will soon require more iteration
over the available groups. Simplify this process by introducing a group
iteration helper.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v2:
 - no change

 drivers/gpu/drm/rcar-du/rcar_du_drv.h |  5 +++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 10 ++--------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 1327cd0df90a..1e9dd494e8ac 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -96,6 +96,11 @@ struct rcar_du_device {
 	unsigned int vspd1_sink;
 };
 
+#define for_each_rcdu_group(__rcdu, __group, __i) \
+	for ((__i) = 0; (__group = &(rcdu)->groups[__i]), \
+	     (__i) < DIV_ROUND_UP((rcdu)->num_crtcs, 2); \
+	     __i++)
+
 static inline bool rcar_du_has(struct rcar_du_device *rcdu,
 			       unsigned int feature)
 {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index e4befb1937f8..ece92cff2137 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -522,9 +522,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 
 	struct drm_device *dev = rcdu->ddev;
 	struct drm_encoder *encoder;
+	struct rcar_du_group *rgrp;
 	unsigned int dpad0_sources;
 	unsigned int num_encoders;
-	unsigned int num_groups;
 	unsigned int swindex;
 	unsigned int hwindex;
 	unsigned int i;
@@ -565,11 +565,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 		return ret;
 
 	/* Initialize the groups. */
-	num_groups = DIV_ROUND_UP(rcdu->num_crtcs, 2);
-
-	for (i = 0; i < num_groups; ++i) {
-		struct rcar_du_group *rgrp = &rcdu->groups[i];
-
+	for_each_rcdu_group(rcdu, rgrp, i) {
 		mutex_init(&rgrp->lock);
 
 		rgrp->dev = rcdu;
@@ -606,8 +602,6 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 
 	/* Create the CRTCs. */
 	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
-		struct rcar_du_group *rgrp;
-
 		/* Skip unpopulated DU channels. */
 		if (!(rcdu->info->channels_mask & BIT(hwindex)))
 			continue;
-- 
2.19.1

Re: [PATCH v2 4/6] drm: rcar-du: Provide for_each_group helper

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch?

On Fri, Mar 15, 2019 at 05:01:08PM +0000, Kieran Bingham wrote:
> Refactoring of the group control code will soon require more iteration
> over the available groups. Simplify this process by introducing a group
> iteration helper.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> v2:
>  - no change
> 
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h |  5 +++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 10 ++--------
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 1327cd0df90a..1e9dd494e8ac 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -96,6 +96,11 @@ struct rcar_du_device {
>  	unsigned int vspd1_sink;
>  };
>  
> +#define for_each_rcdu_group(__rcdu, __group, __i) \
> +	for ((__i) = 0; (__group = &(rcdu)->groups[__i]), \
> +	     (__i) < DIV_ROUND_UP((rcdu)->num_crtcs, 2); \
> +	     __i++)

s/(rcdu)/(__rcdu)/

Assigning __group in the condition part of the for statement seems
weird, even if it should work. How about writing it as

#define for_each_rcdu_group(__rcdu, __group, __i) \
	for ((__i) = 0, (__group) = &(__rcdu)->groups[0]; \
	     (__i) < DIV_ROUND_UP((__rcdu)->num_crtcs, 2); \
	     __i++, __group++)

Apart from this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  static inline bool rcar_du_has(struct rcar_du_device *rcdu,
>  			       unsigned int feature)
>  {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index e4befb1937f8..ece92cff2137 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -522,9 +522,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  
>  	struct drm_device *dev = rcdu->ddev;
>  	struct drm_encoder *encoder;
> +	struct rcar_du_group *rgrp;
>  	unsigned int dpad0_sources;
>  	unsigned int num_encoders;
> -	unsigned int num_groups;
>  	unsigned int swindex;
>  	unsigned int hwindex;
>  	unsigned int i;
> @@ -565,11 +565,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  		return ret;
>  
>  	/* Initialize the groups. */
> -	num_groups = DIV_ROUND_UP(rcdu->num_crtcs, 2);
> -
> -	for (i = 0; i < num_groups; ++i) {
> -		struct rcar_du_group *rgrp = &rcdu->groups[i];
> -
> +	for_each_rcdu_group(rcdu, rgrp, i) {
>  		mutex_init(&rgrp->lock);
>  
>  		rgrp->dev = rcdu;
> @@ -606,8 +602,6 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  
>  	/* Create the CRTCs. */
>  	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
> -		struct rcar_du_group *rgrp;
> -
>  		/* Skip unpopulated DU channels. */
>  		if (!(rcdu->info->channels_mask & BIT(hwindex)))
>  			continue;

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/6] drm: rcar-du: Create a group state object

[Kieran Bingham]

Create a new private state object for the DU groups, and move the
initialisation of a group object to a new function rcar_du_group_init().

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v2:
 - No change

 drivers/gpu/drm/rcar-du/rcar_du_group.c | 81 +++++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_group.h | 22 +++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 27 ++-------
 3 files changed, 107 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 9eee47969e77..9c82d666f170 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -26,6 +26,10 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_device.h>
+
 #include "rcar_du_drv.h"
 #include "rcar_du_group.h"
 #include "rcar_du_regs.h"
@@ -351,3 +355,80 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
 
 	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
 }
+
+static struct drm_private_state *
+rcar_du_group_atomic_duplicate_state(struct drm_private_obj *obj)
+{
+	struct rcar_du_group_state *state;
+
+	if (WARN_ON(!obj->state))
+		return NULL;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (state == NULL)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->state);
+
+	return &state->state;
+}
+
+static void rcar_du_group_atomic_destroy_state(struct drm_private_obj *obj,
+					       struct drm_private_state *state)
+{
+	kfree(to_rcar_group_state(state));
+}
+
+static const struct drm_private_state_funcs rcar_du_group_state_funcs = {
+	.atomic_duplicate_state = rcar_du_group_atomic_duplicate_state,
+	.atomic_destroy_state = rcar_du_group_atomic_destroy_state,
+};
+
+/*
+ * rcar_du_group_init - Initialise and reset a group object
+ *
+ * Return 0 in case of success or a negative error code otherwise.
+ */
+int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
+		       unsigned int index)
+{
+	static const unsigned int mmio_offsets[] = {
+		DU0_REG_OFFSET, DU2_REG_OFFSET
+	};
+
+	struct rcar_du_group_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(rcdu->ddev, &rgrp->private, &state->state,
+				    &rcar_du_group_state_funcs);
+
+	mutex_init(&rgrp->lock);
+
+	rgrp->dev = rcdu;
+	rgrp->mmio_offset = mmio_offsets[index];
+	rgrp->index = index;
+	/* Extract the channel mask for this group only. */
+	rgrp->channels_mask = (rcdu->info->channels_mask >> (2 * index))
+			    & GENMASK(1, 0);
+	rgrp->num_crtcs = hweight8(rgrp->channels_mask);
+
+	/*
+	 * If we have more than one CRTC in this group pre-associate
+	 * the low-order planes with CRTC 0 and the high-order planes
+	 * with CRTC 1 to minimize flicker occurring when the
+	 * association is changed.
+	 */
+	rgrp->dptsr_planes = rgrp->num_crtcs > 1
+			   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
+			   : 0;
+
+	return 0;
+}
+
+void rcar_du_group_cleanup(struct rcar_du_group *rgrp)
+{
+	drm_atomic_private_obj_fini(&rgrp->private);
+}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 87950c1f6a52..4b812e167987 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -12,12 +12,15 @@
 
 #include <linux/mutex.h>
 
+#include <drm/drm_atomic.h>
+
 #include "rcar_du_plane.h"
 
 struct rcar_du_device;
 
 /*
  * struct rcar_du_group - CRTCs and planes group
+ * @private: The base drm private object
  * @dev: the DU device
  * @mmio_offset: registers offset in the device memory map
  * @index: group index
@@ -32,6 +35,8 @@ struct rcar_du_device;
  * @need_restart: the group needs to be restarted due to a configuration change
  */
 struct rcar_du_group {
+	struct drm_private_obj private;
+
 	struct rcar_du_device *dev;
 	unsigned int mmio_offset;
 	unsigned int index;
@@ -49,6 +54,19 @@ struct rcar_du_group {
 	bool need_restart;
 };
 
+#define to_rcar_group(s) container_of(s, struct rcar_du_group, private)
+
+/**
+ * struct rcar_du_group_state - Driver-specific group state
+ * @state: base DRM private state
+ */
+struct rcar_du_group_state {
+	struct drm_private_state state;
+};
+
+#define to_rcar_group_state(s) \
+	container_of(s, struct rcar_du_group_state, state)
+
 u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg);
 void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
 
@@ -60,4 +78,8 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
 
 int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
 
+int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
+		       unsigned int index);
+void rcar_du_group_cleanup(struct rcar_du_group *rgrp);
+
 #endif /* __RCAR_DU_GROUP_H__ */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index ece92cff2137..eb01bea1ab83 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -23,6 +23,7 @@
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
+#include "rcar_du_group.h"
 #include "rcar_du_kms.h"
 #include "rcar_du_regs.h"
 #include "rcar_du_vsp.h"
@@ -516,10 +517,6 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
 
 int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 {
-	static const unsigned int mmio_offsets[] = {
-		DU0_REG_OFFSET, DU2_REG_OFFSET
-	};
-
 	struct drm_device *dev = rcdu->ddev;
 	struct drm_encoder *encoder;
 	struct rcar_du_group *rgrp;
@@ -566,25 +563,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 
 	/* Initialize the groups. */
 	for_each_rcdu_group(rcdu, rgrp, i) {
-		mutex_init(&rgrp->lock);
-
-		rgrp->dev = rcdu;
-		rgrp->mmio_offset = mmio_offsets[i];
-		rgrp->index = i;
-		/* Extract the channel mask for this group only. */
-		rgrp->channels_mask = (rcdu->info->channels_mask >> (2 * i))
-				   & GENMASK(1, 0);
-		rgrp->num_crtcs = hweight8(rgrp->channels_mask);
-
-		/*
-		 * If we have more than one CRTCs in this group pre-associate
-		 * the low-order planes with CRTC 0 and the high-order planes
-		 * with CRTC 1 to minimize flicker occurring when the
-		 * association is changed.
-		 */
-		rgrp->dptsr_planes = rgrp->num_crtcs > 1
-				   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
-				   : 0;
+		ret = rcar_du_group_init(rcdu, rgrp, i);
+		if (ret < 0)
+			return ret;
 
 		if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
 			ret = rcar_du_planes_init(rgrp);
-- 
2.19.1

Re: [PATCH v2 5/6] drm: rcar-du: Create a group state object

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 15, 2019 at 05:01:09PM +0000, Kieran Bingham wrote:
> Create a new private state object for the DU groups, and move the
> initialisation of a group object to a new function rcar_du_group_init().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> v2:
>  - No change
> 
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 81 +++++++++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_group.h | 22 +++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 27 ++-------
>  3 files changed, 107 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9eee47969e77..9c82d666f170 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -26,6 +26,10 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>

Please include <linux/slab.h> for kzalloc() and kfree().

>  
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_device.h>
> +
>  #include "rcar_du_drv.h"
>  #include "rcar_du_group.h"
>  #include "rcar_du_regs.h"
> @@ -351,3 +355,80 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  
>  	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
>  }
> +
> +static struct drm_private_state *
> +rcar_du_group_atomic_duplicate_state(struct drm_private_obj *obj)
> +{
> +	struct rcar_du_group_state *state;
> +
> +	if (WARN_ON(!obj->state))
> +		return NULL;
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (state == NULL)
> +		return NULL;
> +
> +	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->state);
> +
> +	return &state->state;
> +}
> +
> +static void rcar_du_group_atomic_destroy_state(struct drm_private_obj *obj,
> +					       struct drm_private_state *state)
> +{
> +	kfree(to_rcar_group_state(state));
> +}
> +
> +static const struct drm_private_state_funcs rcar_du_group_state_funcs = {
> +	.atomic_duplicate_state = rcar_du_group_atomic_duplicate_state,
> +	.atomic_destroy_state = rcar_du_group_atomic_destroy_state,
> +};
> +
> +/*
> + * rcar_du_group_init - Initialise and reset a group object
> + *
> + * Return 0 in case of success or a negative error code otherwise.
> + */
> +int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
> +		       unsigned int index)
> +{
> +	static const unsigned int mmio_offsets[] = {
> +		DU0_REG_OFFSET, DU2_REG_OFFSET
> +	};
> +
> +	struct rcar_du_group_state *state;
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_atomic_private_obj_init(rcdu->ddev, &rgrp->private, &state->state,
> +				    &rcar_du_group_state_funcs);
> +
> +	mutex_init(&rgrp->lock);
> +
> +	rgrp->dev = rcdu;
> +	rgrp->mmio_offset = mmio_offsets[index];
> +	rgrp->index = index;
> +	/* Extract the channel mask for this group only. */
> +	rgrp->channels_mask = (rcdu->info->channels_mask >> (2 * index))
> +			    & GENMASK(1, 0);
> +	rgrp->num_crtcs = hweight8(rgrp->channels_mask);
> +
> +	/*
> +	 * If we have more than one CRTC in this group pre-associate
> +	 * the low-order planes with CRTC 0 and the high-order planes
> +	 * with CRTC 1 to minimize flicker occurring when the
> +	 * association is changed.
> +	 */
> +	rgrp->dptsr_planes = rgrp->num_crtcs > 1
> +			   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
> +			   : 0;
> +
> +	return 0;
> +}
> +
> +void rcar_du_group_cleanup(struct rcar_du_group *rgrp)
> +{

I would add a mutex_cleanup(&rgrp->lock) here. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	drm_atomic_private_obj_fini(&rgrp->private);
> +}
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..4b812e167987 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -12,12 +12,15 @@
>  
>  #include <linux/mutex.h>
>  
> +#include <drm/drm_atomic.h>
> +
>  #include "rcar_du_plane.h"
>  
>  struct rcar_du_device;
>  
>  /*
>   * struct rcar_du_group - CRTCs and planes group
> + * @private: The base drm private object
>   * @dev: the DU device
>   * @mmio_offset: registers offset in the device memory map
>   * @index: group index
> @@ -32,6 +35,8 @@ struct rcar_du_device;
>   * @need_restart: the group needs to be restarted due to a configuration change
>   */
>  struct rcar_du_group {
> +	struct drm_private_obj private;
> +
>  	struct rcar_du_device *dev;
>  	unsigned int mmio_offset;
>  	unsigned int index;
> @@ -49,6 +54,19 @@ struct rcar_du_group {
>  	bool need_restart;
>  };
>  
> +#define to_rcar_group(s) container_of(s, struct rcar_du_group, private)
> +
> +/**
> + * struct rcar_du_group_state - Driver-specific group state
> + * @state: base DRM private state
> + */
> +struct rcar_du_group_state {
> +	struct drm_private_state state;
> +};
> +
> +#define to_rcar_group_state(s) \
> +	container_of(s, struct rcar_du_group_state, state)
> +
>  u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg);
>  void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
>  
> @@ -60,4 +78,8 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
>  
>  int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
>  
> +int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
> +		       unsigned int index);
> +void rcar_du_group_cleanup(struct rcar_du_group *rgrp);
> +
>  #endif /* __RCAR_DU_GROUP_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index ece92cff2137..eb01bea1ab83 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -23,6 +23,7 @@
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
> +#include "rcar_du_group.h"
>  #include "rcar_du_kms.h"
>  #include "rcar_du_regs.h"
>  #include "rcar_du_vsp.h"
> @@ -516,10 +517,6 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>  
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  {
> -	static const unsigned int mmio_offsets[] = {
> -		DU0_REG_OFFSET, DU2_REG_OFFSET
> -	};
> -
>  	struct drm_device *dev = rcdu->ddev;
>  	struct drm_encoder *encoder;
>  	struct rcar_du_group *rgrp;
> @@ -566,25 +563,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  
>  	/* Initialize the groups. */
>  	for_each_rcdu_group(rcdu, rgrp, i) {
> -		mutex_init(&rgrp->lock);
> -
> -		rgrp->dev = rcdu;
> -		rgrp->mmio_offset = mmio_offsets[i];
> -		rgrp->index = i;
> -		/* Extract the channel mask for this group only. */
> -		rgrp->channels_mask = (rcdu->info->channels_mask >> (2 * i))
> -				   & GENMASK(1, 0);
> -		rgrp->num_crtcs = hweight8(rgrp->channels_mask);
> -
> -		/*
> -		 * If we have more than one CRTCs in this group pre-associate
> -		 * the low-order planes with CRTC 0 and the high-order planes
> -		 * with CRTC 1 to minimize flicker occurring when the
> -		 * association is changed.
> -		 */
> -		rgrp->dptsr_planes = rgrp->num_crtcs > 1
> -				   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
> -				   : 0;
> +		ret = rcar_du_group_init(rcdu, rgrp, i);
> +		if (ret < 0)
> +			return ret;
>  
>  		if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>  			ret = rcar_du_planes_init(rgrp);

-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] drm: rcar-du: Add group hooks for atomic-commit

[Kieran Bingham]

The group can now be handled independently from the CRTC tracking its
own state.

Introduce an rcar_du_group_atomic_check() call which will iterate the
CRTCs to determine the per-state use-count of the group.

This use count then allows us to determine if the group should be
configured or disabled in our commit tail through the introduction of
two new calls rcar_du_group_atomic_{pre,post}_commit().

The existing rcar_du_group_{get,put}() functions are now redundant and
removed along with their interactions in the CRTC get/put calls.

The groups share clocking with the CRTCs within the group, and so are
accessible only when one of its CRTCs has been powered through
rcar_du_crtc_atomic_exit_standby().

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
v2:
 - All register sequences now maintained.
 - Clock management is no longer handled by the group
   (_crtc_{exit,enter}_standby handles this for us)



 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |   8 --
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 101 ++++++++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  14 +++-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |   6 ++
 4 files changed, 85 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 2606de788688..fce8bd1d9d25 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -511,14 +511,8 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_clock;
 
-	ret = rcar_du_group_get(rcrtc->group);
-	if (ret < 0)
-		goto error_group;
-
 	return 0;
 
-error_group:
-	clk_disable_unprepare(rcrtc->extclock);
 error_clock:
 	clk_disable_unprepare(rcrtc->clock);
 	return ret;
@@ -526,8 +520,6 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
 
 static void rcar_du_crtc_disable(struct rcar_du_crtc *rcrtc)
 {
-	rcar_du_group_put(rcrtc->group);
-
 	clk_disable_unprepare(rcrtc->extclock);
 	clk_disable_unprepare(rcrtc->clock);
 }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 9c82d666f170..1f9504bca0f3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -24,6 +24,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/err.h>
 #include <linux/io.h>
 
 #include <drm/drm_atomic.h>
@@ -172,38 +173,6 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 	mutex_unlock(&rgrp->lock);
 }
 
-/*
- * rcar_du_group_get - Acquire a reference to the DU channels group
- *
- * Acquiring the first reference setups core registers. A reference must be held
- * before accessing any hardware registers.
- *
- * This function must be called with the DRM mode_config lock held.
- *
- * Return 0 in case of success or a negative error code otherwise.
- */
-int rcar_du_group_get(struct rcar_du_group *rgrp)
-{
-	if (rgrp->use_count)
-		goto done;
-
-	rcar_du_group_setup(rgrp);
-
-done:
-	rgrp->use_count++;
-	return 0;
-}
-
-/*
- * rcar_du_group_put - Release a reference to the DU
- *
- * This function must be called with the DRM mode_config lock held.
- */
-void rcar_du_group_put(struct rcar_du_group *rgrp)
-{
-	--rgrp->use_count;
-}
-
 static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
@@ -384,6 +353,74 @@ static const struct drm_private_state_funcs rcar_du_group_state_funcs = {
 	.atomic_destroy_state = rcar_du_group_atomic_destroy_state,
 };
 
+#define for_each_oldnew_group_in_state(__state, __obj, __old_state, __new_state, __i) \
+	for_each_oldnew_private_obj_in_state((__state), (__obj), (__old_state), (__new_state), (__i)) \
+		for_each_if((__obj)->funcs == &rcar_du_group_state_funcs)
+
+static struct rcar_du_group_state *
+rcar_du_get_group_state(struct drm_atomic_state *state,
+			struct rcar_du_group *rgrp)
+{
+	struct drm_private_state *pstate;
+
+	pstate = drm_atomic_get_private_obj_state(state, &rgrp->private);
+	if (IS_ERR(pstate))
+		return ERR_CAST(pstate);
+
+	return to_rcar_group_state(pstate);
+}
+
+int rcar_du_group_atomic_check(struct drm_device *dev,
+			       struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+		struct rcar_du_group_state *rstate;
+
+		if (crtc_state->active_changed || crtc_state->mode_changed) {
+			rstate = rcar_du_get_group_state(state, rcrtc->group);
+			if (IS_ERR(rstate))
+				return PTR_ERR(rstate);
+
+			if (crtc_state->active)
+				rstate->use_count++;
+		}
+	}
+
+	return 0;
+}
+
+int rcar_du_group_atomic_pre_commit(struct drm_device *dev,
+				    struct drm_atomic_state *state)
+{
+	struct drm_private_state *old_pstate, *new_pstate;
+	struct drm_private_obj *obj;
+	unsigned int i;
+
+	for_each_oldnew_group_in_state(state, obj, old_pstate, new_pstate, i) {
+		struct rcar_du_group *rgrp = to_rcar_group(obj);
+		struct rcar_du_group_state *old_state, *new_state;
+
+		old_state = to_rcar_group_state(old_pstate);
+		new_state = to_rcar_group_state(new_pstate);
+
+		if (!old_state->use_count && new_state->use_count)
+			rcar_du_group_setup(rgrp);
+	}
+
+	return 0;
+}
+
+int rcar_du_group_atomic_post_commit(struct drm_device *dev,
+				     struct drm_atomic_state *state)
+{
+	return 0;
+}
+
 /*
  * rcar_du_group_init - Initialise and reset a group object
  *
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 4b812e167987..288c09a6d7d0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -26,7 +26,6 @@ struct rcar_du_device;
  * @index: group index
  * @channels_mask: bitmask of populated DU channels in this group
  * @num_crtcs: number of CRTCs in this group (1 or 2)
- * @use_count: number of users of the group (rcar_du_group_(get|put))
  * @used_crtcs: number of CRTCs currently in use
  * @lock: protects the dptsr_planes field and the DPTSR register
  * @dptsr_planes: bitmask of planes driven by dot-clock and timing generator 1
@@ -43,7 +42,6 @@ struct rcar_du_group {
 
 	unsigned int channels_mask;
 	unsigned int num_crtcs;
-	unsigned int use_count;
 	unsigned int used_crtcs;
 
 	struct mutex lock;
@@ -59,9 +57,12 @@ struct rcar_du_group {
 /**
  * struct rcar_du_group_state - Driver-specific group state
  * @state: base DRM private state
+ * @use_count: number of users of the group
  */
 struct rcar_du_group_state {
 	struct drm_private_state state;
+
+	unsigned int use_count;
 };
 
 #define to_rcar_group_state(s) \
@@ -70,14 +71,19 @@ struct rcar_du_group_state {
 u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg);
 void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
 
-int rcar_du_group_get(struct rcar_du_group *rgrp);
-void rcar_du_group_put(struct rcar_du_group *rgrp);
 void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start);
 void rcar_du_group_restart(struct rcar_du_group *rgrp);
 int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
 
 int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
 
+int rcar_du_group_atomic_check(struct drm_device *dev,
+			       struct drm_atomic_state *state);
+int rcar_du_group_atomic_pre_commit(struct drm_device *dev,
+				    struct drm_atomic_state *state);
+int rcar_du_group_atomic_post_commit(struct drm_device *dev,
+				     struct drm_atomic_state *state);
+
 int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
 		       unsigned int index);
 void rcar_du_group_cleanup(struct rcar_du_group *rgrp);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index eb01bea1ab83..77f0ff38298b 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -271,6 +271,10 @@ static int rcar_du_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	ret = rcar_du_group_atomic_check(dev, state);
+	if (ret)
+		return ret;
+
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
 		return 0;
 
@@ -305,6 +309,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 	/* Apply the atomic update. */
 	rcar_du_crtc_atomic_exit_standby(dev, old_state);
+	rcar_du_group_atomic_pre_commit(dev, old_state);
 	rcar_du_crtc_atomic_pre_commit(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
@@ -313,6 +318,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	rcar_du_crtc_atomic_post_commit(dev, old_state);
+	rcar_du_group_atomic_post_commit(dev, old_state);
 	rcar_du_crtc_atomic_enter_standby(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
-- 
2.19.1

Re: [PATCH v2 6/6] drm: rcar-du: Add group hooks for atomic-commit

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 15, 2019 at 05:01:10PM +0000, Kieran Bingham wrote:
> The group can now be handled independently from the CRTC tracking its
> own state.
> 
> Introduce an rcar_du_group_atomic_check() call which will iterate the
> CRTCs to determine the per-state use-count of the group.
> 
> This use count then allows us to determine if the group should be
> configured or disabled in our commit tail through the introduction of
> two new calls rcar_du_group_atomic_{pre,post}_commit().
> 
> The existing rcar_du_group_{get,put}() functions are now redundant and
> removed along with their interactions in the CRTC get/put calls.
> 
> The groups share clocking with the CRTCs within the group, and so are
> accessible only when one of its CRTCs has been powered through
> rcar_du_crtc_atomic_exit_standby().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v2:
>  - All register sequences now maintained.
>  - Clock management is no longer handled by the group
>    (_crtc_{exit,enter}_standby handles this for us)
> 
> 
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |   8 --
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 101 ++++++++++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  14 +++-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |   6 ++
>  4 files changed, 85 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2606de788688..fce8bd1d9d25 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -511,14 +511,8 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
>  	if (ret < 0)
>  		goto error_clock;
>  
> -	ret = rcar_du_group_get(rcrtc->group);
> -	if (ret < 0)
> -		goto error_group;
> -
>  	return 0;
>  
> -error_group:
> -	clk_disable_unprepare(rcrtc->extclock);
>  error_clock:
>  	clk_disable_unprepare(rcrtc->clock);
>  	return ret;

You can simplify this function further by inlining the
clk_disable_unprepare() in the single error path and removing the
error_clock label.

> @@ -526,8 +520,6 @@ static int rcar_du_crtc_enable(struct rcar_du_crtc *rcrtc)
>  
>  static void rcar_du_crtc_disable(struct rcar_du_crtc *rcrtc)
>  {
> -	rcar_du_group_put(rcrtc->group);
> -
>  	clk_disable_unprepare(rcrtc->extclock);
>  	clk_disable_unprepare(rcrtc->clock);
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9c82d666f170..1f9504bca0f3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -24,6 +24,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/err.h>
>  #include <linux/io.h>
>  
>  #include <drm/drm_atomic.h>
> @@ -172,38 +173,6 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  	mutex_unlock(&rgrp->lock);
>  }
>  
> -/*
> - * rcar_du_group_get - Acquire a reference to the DU channels group
> - *
> - * Acquiring the first reference setups core registers. A reference must be held
> - * before accessing any hardware registers.
> - *
> - * This function must be called with the DRM mode_config lock held.
> - *
> - * Return 0 in case of success or a negative error code otherwise.
> - */
> -int rcar_du_group_get(struct rcar_du_group *rgrp)
> -{
> -	if (rgrp->use_count)
> -		goto done;
> -
> -	rcar_du_group_setup(rgrp);
> -
> -done:
> -	rgrp->use_count++;
> -	return 0;
> -}
> -
> -/*
> - * rcar_du_group_put - Release a reference to the DU
> - *
> - * This function must be called with the DRM mode_config lock held.
> - */
> -void rcar_du_group_put(struct rcar_du_group *rgrp)
> -{
> -	--rgrp->use_count;
> -}
> -
>  static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
> @@ -384,6 +353,74 @@ static const struct drm_private_state_funcs rcar_du_group_state_funcs = {
>  	.atomic_destroy_state = rcar_du_group_atomic_destroy_state,
>  };
>  
> +#define for_each_oldnew_group_in_state(__state, __obj, __old_state, __new_state, __i) \
> +	for_each_oldnew_private_obj_in_state((__state), (__obj), (__old_state), (__new_state), (__i)) \
> +		for_each_if((__obj)->funcs == &rcar_du_group_state_funcs)
> +
> +static struct rcar_du_group_state *
> +rcar_du_get_group_state(struct drm_atomic_state *state,
> +			struct rcar_du_group *rgrp)
> +{
> +	struct drm_private_state *pstate;
> +
> +	pstate = drm_atomic_get_private_obj_state(state, &rgrp->private);
> +	if (IS_ERR(pstate))
> +		return ERR_CAST(pstate);
> +
> +	return to_rcar_group_state(pstate);
> +}
> +
> +int rcar_du_group_atomic_check(struct drm_device *dev,
> +			       struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned int i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +		struct rcar_du_group_state *rstate;
> +
> +		if (crtc_state->active_changed || crtc_state->mode_changed) {
> +			rstate = rcar_du_get_group_state(state, rcrtc->group);
> +			if (IS_ERR(rstate))
> +				return PTR_ERR(rstate);
> +
> +			if (crtc_state->active)
> +				rstate->use_count++;

This doesn't seem right to me. A commit with just a page flip will have
neither active_changed not mode_changed set. The group use count will
thus be 0. If the next commit enables the second CRTC in the group, then
you will call rcar_du_group_setup() again in
rcar_du_group_atomic_pre_commit().

I think you should increment use_count when crtc_state->active
regardless of active_changed and mode_changed.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int rcar_du_group_atomic_pre_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state)
> +{
> +	struct drm_private_state *old_pstate, *new_pstate;
> +	struct drm_private_obj *obj;
> +	unsigned int i;
> +
> +	for_each_oldnew_group_in_state(state, obj, old_pstate, new_pstate, i) {
> +		struct rcar_du_group *rgrp = to_rcar_group(obj);
> +		struct rcar_du_group_state *old_state, *new_state;
> +
> +		old_state = to_rcar_group_state(old_pstate);
> +		new_state = to_rcar_group_state(new_pstate);
> +
> +		if (!old_state->use_count && new_state->use_count)
> +			rcar_du_group_setup(rgrp);
> +	}
> +
> +	return 0;
> +}
> +
> +int rcar_du_group_atomic_post_commit(struct drm_device *dev,
> +				     struct drm_atomic_state *state)
> +{
> +	return 0;
> +}

As for the similar CRTC patch, I think you can drop
rcar_du_group_atomic_post_commit(), turn
rcar_du_group_atomic_pre_commit() into a void function, and rename it to
rcar_du_group_atomic_setup().

The rest looks good to me, so with this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  /*
>   * rcar_du_group_init - Initialise and reset a group object
>   *
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 4b812e167987..288c09a6d7d0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -26,7 +26,6 @@ struct rcar_du_device;
>   * @index: group index
>   * @channels_mask: bitmask of populated DU channels in this group
>   * @num_crtcs: number of CRTCs in this group (1 or 2)
> - * @use_count: number of users of the group (rcar_du_group_(get|put))
>   * @used_crtcs: number of CRTCs currently in use
>   * @lock: protects the dptsr_planes field and the DPTSR register
>   * @dptsr_planes: bitmask of planes driven by dot-clock and timing generator 1
> @@ -43,7 +42,6 @@ struct rcar_du_group {
>  
>  	unsigned int channels_mask;
>  	unsigned int num_crtcs;
> -	unsigned int use_count;
>  	unsigned int used_crtcs;
>  
>  	struct mutex lock;
> @@ -59,9 +57,12 @@ struct rcar_du_group {
>  /**
>   * struct rcar_du_group_state - Driver-specific group state
>   * @state: base DRM private state
> + * @use_count: number of users of the group
>   */
>  struct rcar_du_group_state {
>  	struct drm_private_state state;
> +
> +	unsigned int use_count;
>  };
>  
>  #define to_rcar_group_state(s) \
> @@ -70,14 +71,19 @@ struct rcar_du_group_state {
>  u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg);
>  void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
>  
> -int rcar_du_group_get(struct rcar_du_group *rgrp);
> -void rcar_du_group_put(struct rcar_du_group *rgrp);
>  void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start);
>  void rcar_du_group_restart(struct rcar_du_group *rgrp);
>  int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
>  
>  int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
>  
> +int rcar_du_group_atomic_check(struct drm_device *dev,
> +			       struct drm_atomic_state *state);
> +int rcar_du_group_atomic_pre_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state);
> +int rcar_du_group_atomic_post_commit(struct drm_device *dev,
> +				     struct drm_atomic_state *state);
> +
>  int rcar_du_group_init(struct rcar_du_device *rcdu, struct rcar_du_group *rgrp,
>  		       unsigned int index);
>  void rcar_du_group_cleanup(struct rcar_du_group *rgrp);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index eb01bea1ab83..77f0ff38298b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -271,6 +271,10 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	ret = rcar_du_group_atomic_check(dev, state);
> +	if (ret)
> +		return ret;
> +
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		return 0;
>  
> @@ -305,6 +309,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  
>  	/* Apply the atomic update. */
>  	rcar_du_crtc_atomic_exit_standby(dev, old_state);
> +	rcar_du_group_atomic_pre_commit(dev, old_state);
>  	rcar_du_crtc_atomic_pre_commit(dev, old_state);
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> @@ -313,6 +318,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
>  	rcar_du_crtc_atomic_post_commit(dev, old_state);
> +	rcar_du_group_atomic_post_commit(dev, old_state);
>  	rcar_du_crtc_atomic_enter_standby(dev, old_state);
>  
>  	drm_atomic_helper_commit_hw_done(old_state);

-- 
Regards,

Laurent Pinchart