[PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
start to CRTC resume") changed the order of the plane commit and CRTC
enable operations to accommodate the runtime PM requirements. However,
this introduced corruption in the first displayed frame, as the CRTC is
now enabled without any plane configured. On Gen2 hardware the first
frame will be black and likely unnoticed, but on Gen3 hardware we end up
starting the display before the VSP compositor, which is more
noticeable.

To fix this, revert the order of the commit operations back, and handle
runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
helper operation handlers.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++--------------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
 3 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 6b5219ef0ad2..76cdb88b2b8e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
  * Start/Stop and Suspend/Resume
  */
 
-static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
+static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 {
-	struct drm_crtc *crtc = &rcrtc->crtc;
-	bool interlaced;
-
-	if (rcrtc->started)
-		return;
-
 	/* Set display off and background to black */
 	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
 	rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
@@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 	/* Start with all planes disabled. */
 	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))
+		rcar_du_vsp_enable(rcrtc);
+
+	/* Turn vertical blanking interrupt reporting on. */
+	drm_crtc_vblank_on(&rcrtc->crtc);
+}
+
+static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
+{
+	bool interlaced;
+
 	/* Select master sync mode. This enables display operation in master
 	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
 	 * actively driven).
@@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 			     DSYSR_TVM_MASTER);
 
 	rcar_du_group_start_stop(rcrtc->group, true);
-
-	/* Enable the VSP compositor. */
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
-		rcar_du_vsp_enable(rcrtc);
-
-	/* Turn vertical blanking interrupt reporting back on. */
-	drm_crtc_vblank_on(crtc);
-
-	rcrtc->started = true;
 }
 
 static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 {
 	struct drm_crtc *crtc = &rcrtc->crtc;
 
-	if (!rcrtc->started)
-		return;
-
 	/* Disable all planes and wait for the change to take effect. This is
 	 * required as the DSnPR registers are updated on vblank, and no vblank
 	 * will occur once the CRTC is stopped. Disabling planes when starting
@@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
 
 	rcar_du_group_start_stop(rcrtc->group, false);
-
-	rcrtc->started = false;
 }
 
 void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
@@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
 		return;
 
 	rcar_du_crtc_get(rcrtc);
-	rcar_du_crtc_start(rcrtc);
+	rcar_du_crtc_setup(rcrtc);
 
 	/* Commit the planes state. */
-	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
-		rcar_du_vsp_enable(rcrtc);
-	} else {
+	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
 		for (i = 0; i < rcrtc->group->num_planes; ++i) {
 			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
 
@@ -563,6 +553,7 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
 	}
 
 	rcar_du_crtc_update_planes(rcrtc);
+	rcar_du_crtc_start(rcrtc);
 }
 
 /* -----------------------------------------------------------------------------
@@ -574,7 +565,16 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
-	rcar_du_crtc_get(rcrtc);
+	/*
+	 * If the CRTC has already been setup by the .atomic_begin() handler we
+	 * can skip the setup stage.
+	 */
+	if (!rcrtc->initialized) {
+		rcar_du_crtc_get(rcrtc);
+		rcar_du_crtc_setup(rcrtc);
+		rcrtc->initialized = true;
+	}
+
 	rcar_du_crtc_start(rcrtc);
 }
 
@@ -593,6 +593,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
 	}
 	spin_unlock_irq(&crtc->dev->event_lock);
 
+	rcrtc->initialized = false;
 	rcrtc->outputs = 0;
 }
 
@@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
+	WARN_ON(!crtc->state->enable);
+
+	/*
+	 * If a mode set is in progress we can be called with the CRTC disabled.
+	 * We then need to first setup the CRTC in order to configure planes.
+	 * The .atomic_enable() handler will notice and skip the CRTC setup.
+	 */
+	if (!rcrtc->initialized) {
+		rcar_du_crtc_get(rcrtc);
+		rcar_du_crtc_setup(rcrtc);
+		rcrtc->initialized = true;
+	}
+
 	if (rcar_du_has(rcrtc->group->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 0b6d26ecfc38..3cc376826592 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -30,7 +30,7 @@ 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
- * @started: whether the CRTC has been started and is running
+ * @initialized: whether the CRTC has been initialized and clocks enabled
  * @event: event to post when the pending page flip completes
  * @flip_wait: wait queue used to signal page flip completion
  * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
@@ -45,7 +45,7 @@ struct rcar_du_crtc {
 	struct clk *extclock;
 	unsigned int mmio_offset;
 	unsigned int index;
-	bool started;
+	bool initialized;
 
 	struct drm_pending_vblank_event *event;
 	wait_queue_head_t flip_wait;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 82b978a5dae6..c2f382feca07 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 	/* Apply the atomic update. */
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_modeset_enables(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);
 
 	drm_atomic_helper_commit_hw_done(old_state);
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Geert Uytterhoeven]

On Wed, Jun 28, 2017 at 8:50 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> start to CRTC resume") changed the order of the plane commit and CRTC
> enable operations to accommodate the runtime PM requirements. However,
> this introduced corruption in the first displayed frame, as the CRTC is
> now enabled without any plane configured. On Gen2 hardware the first
> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> starting the display before the VSP compositor, which is more
> noticeable.
>
> To fix this, revert the order of the commit operations back, and handle
> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> helper operation handlers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Fixes: ...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Geert,

On Wednesday 28 Jun 2017 20:52:53 Geert Uytterhoeven wrote:
> On Wed, Jun 28, 2017 at 8:50 PM, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Fixes: ...

I thought about that, but this patch fixes a problem caused by a combination 
of commits. The one mentioned above is probably the easiest to point at, but 
the problem only became, well, problematic, with the introduction of Gen3 
support in v4.6.

Furthermore, while it's probably possible to backport this patch to v4.6, I 
don't think it needs to be included in -stable. For all those reasons, a Fixes 
tag might not be useful. Of course please feel free to disagree :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Geert,

On Wednesday 28 Jun 2017 20:52:53 Geert Uytterhoeven wrote:
> On Wed, Jun 28, 2017 at 8:50 PM, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Fixes: ...

I thought about that, but this patch fixes a problem caused by a combination 
of commits. The one mentioned above is probably the easiest to point at, but 
the problem only became, well, problematic, with the introduction of Gen3 
support in v4.6.

Furthermore, while it's probably possible to backport this patch to v4.6, I 
don't think it needs to be included in -stable. For all those reasons, a Fixes 
tag might not be useful. Of course please feel free to disagree :-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Laurent,

On 28/06/17 19:50, Laurent Pinchart wrote:
> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> start to CRTC resume") changed the order of the plane commit and CRTC
> enable operations to accommodate the runtime PM requirements. However,
> this introduced corruption in the first displayed frame, as the CRTC is
> now enabled without any plane configured. On Gen2 hardware the first
> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> starting the display before the VSP compositor, which is more
> noticeable.
> 
> To fix this, revert the order of the commit operations back, and handle
> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> helper operation handlers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I only have code reduction or comment suggestions below - so either with or
without those changes, feel free to add my:

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

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++--------------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  3 files changed, 43 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 6b5219ef0ad2..76cdb88b2b8e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>   * Start/Stop and Suspend/Resume
>   */
>  
> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  {
> -	struct drm_crtc *crtc = &rcrtc->crtc;
> -	bool interlaced;
> -
> -	if (rcrtc->started)
> -		return;
> -
>  	/* Set display off and background to black */
>  	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>  	rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  	/* Start with all planes disabled. */
>  	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))
> +		rcar_du_vsp_enable(rcrtc);
> +
> +	/* Turn vertical blanking interrupt reporting on. */
> +	drm_crtc_vblank_on(&rcrtc->crtc);
> +}
> +
> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> +{
> +	bool interlaced;
> +
>  	/* Select master sync mode. This enables display operation in master

Are we close enough here to fix this multiline comment style ?
(Not worth doing unless the patch is respun for other reasons ...)

Actually - there are a lot in this file, so it would be better to do them all in
one hit/patch at a point of least conflicts ...


>  	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
>  	 * actively driven).
> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  			     DSYSR_TVM_MASTER);
>  
>  	rcar_du_group_start_stop(rcrtc->group, true);
> -
> -	/* Enable the VSP compositor. */
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> -		rcar_du_vsp_enable(rcrtc);
> -
> -	/* Turn vertical blanking interrupt reporting back on. */
> -	drm_crtc_vblank_on(crtc);
> -
> -	rcrtc->started = true;
>  }
>  
>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  {
>  	struct drm_crtc *crtc = &rcrtc->crtc;
>  
> -	if (!rcrtc->started)
> -		return;
> -
>  	/* Disable all planes and wait for the change to take effect. This is
>  	 * required as the DSnPR registers are updated on vblank, and no vblank
>  	 * will occur once the CRTC is stopped. Disabling planes when starting
> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>  
>  	rcar_du_group_start_stop(rcrtc->group, false);
> -
> -	rcrtc->started = false;
>  }
>  
>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>  		return;
>  
>  	rcar_du_crtc_get(rcrtc);
> -	rcar_du_crtc_start(rcrtc);
> +	rcar_du_crtc_setup(rcrtc);

Every call to _setup is immediately prefixed by a call to _get()

Could the _get() be done in the _setup() for code reduction?

I'm entirely open to that not happening here as it might be preferred to keep
the _get() and _start() separate for style purposes.

>  
>  	/* Commit the planes state. */
> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> -		rcar_du_vsp_enable(rcrtc);
> -	} else {
> +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>  		for (i = 0; i < rcrtc->group->num_planes; ++i) {
>  			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
>  
> @@ -563,6 +553,7 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>  	}
>  
>  	rcar_du_crtc_update_planes(rcrtc);
> +	rcar_du_crtc_start(rcrtc);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -574,7 +565,16 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  
> -	rcar_du_crtc_get(rcrtc);
> +	/*
> +	 * If the CRTC has already been setup by the .atomic_begin() handler we
> +	 * can skip the setup stage.
> +	 */
> +	if (!rcrtc->initialized) {
> +		rcar_du_crtc_get(rcrtc);
> +		rcar_du_crtc_setup(rcrtc);
> +		rcrtc->initialized = true;
> +	}
> +
>  	rcar_du_crtc_start(rcrtc);
>  }
>  
> @@ -593,6 +593,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  
> +	rcrtc->initialized = false;
>  	rcrtc->outputs = 0;
>  }
>  
> @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  
> +	WARN_ON(!crtc->state->enable);

Is this necessary if it's handled by the rcrtc->initialized flag? or is it so
that we find out if it happens ?

(Or is this due to the re-ordering of the _commit_tail() function below?)


> +
> +	/*
> +	 * If a mode set is in progress we can be called with the CRTC disabled.
> +	 * We then need to first setup the CRTC in order to configure planes.
> +	 * The .atomic_enable() handler will notice and skip the CRTC setup.
> +	 */

I'm assuming this comment is the reason for the WARN_ON above ...


> +	if (!rcrtc->initialized) {
> +		rcar_du_crtc_get(rcrtc);
> +		rcar_du_crtc_setup(rcrtc);
> +		rcrtc->initialized = true;
> +	}


If the _get() was moved into the _setup(), and _setup() was protected by the
rcrtc->initialized flag, then _atomic_begin() _enable() and _resume() could all
just simply call _setup(). The _resume() should only ever be called with
rcrtc->initialized = false anyway, as that is set in _suspend()

> +
>  	if (rcar_du_has(rcrtc->group->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 0b6d26ecfc38..3cc376826592 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -30,7 +30,7 @@ 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
> - * @started: whether the CRTC has been started and is running
> + * @initialized: whether the CRTC has been initialized and clocks enabled
>   * @event: event to post when the pending page flip completes
>   * @flip_wait: wait queue used to signal page flip completion
>   * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
> @@ -45,7 +45,7 @@ struct rcar_du_crtc {
>  	struct clk *extclock;
>  	unsigned int mmio_offset;
>  	unsigned int index;
> -	bool started;
> +	bool initialized;
>  
>  	struct drm_pending_vblank_event *event;
>  	wait_queue_head_t flip_wait;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 82b978a5dae6..c2f382feca07 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  
>  	/* Apply the atomic update. */
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);

Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much like
the default drm_atomic_helper_commit_tail() code.

Reading around other uses /variants of commit_tail() style functions in other
drivers has left me confused as to how the ordering affects things here.

Could be worth adding a comment at least to describe why we can't use the
default helper...


> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  >  	drm_atomic_helper_commit_hw_done(old_state);
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> 

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Laurent,

I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
access bug" and it relates directly to a comment I had in this patch:

On 12/07/17 17:35, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 28/06/17 19:50, Laurent Pinchart wrote:
>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>> start to CRTC resume") changed the order of the plane commit and CRTC
>> enable operations to accommodate the runtime PM requirements. However,
>> this introduced corruption in the first displayed frame, as the CRTC is
>> now enabled without any plane configured. On Gen2 hardware the first
>> frame will be black and likely unnoticed, but on Gen3 hardware we end up
>> starting the display before the VSP compositor, which is more
>> noticeable.
>>
>> To fix this, revert the order of the commit operations back, and handle
>> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
>> helper operation handlers.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> I only have code reduction or comment suggestions below - so either with or
> without those changes, feel free to add my:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++--------------
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>  3 files changed, 43 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index 6b5219ef0ad2..76cdb88b2b8e 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>>   * Start/Stop and Suspend/Resume
>>   */
>>  
>> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>>  {
>> -	struct drm_crtc *crtc = &rcrtc->crtc;
>> -	bool interlaced;
>> -
>> -	if (rcrtc->started)
>> -		return;
>> -
>>  	/* Set display off and background to black */
>>  	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>>  	rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
>> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>  	/* Start with all planes disabled. */
>>  	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))
>> +		rcar_du_vsp_enable(rcrtc);
>> +
>> +	/* Turn vertical blanking interrupt reporting on. */
>> +	drm_crtc_vblank_on(&rcrtc->crtc);
>> +}
>> +
>> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>> +{
>> +	bool interlaced;
>> +
>>  	/* Select master sync mode. This enables display operation in master
> 
> Are we close enough here to fix this multiline comment style ?
> (Not worth doing unless the patch is respun for other reasons ...)
> 
> Actually - there are a lot in this file, so it would be better to do them all in
> one hit/patch at a point of least conflicts ...
> 
> 
>>  	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
>>  	 * actively driven).
>> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>  			     DSYSR_TVM_MASTER);
>>  
>>  	rcar_du_group_start_stop(rcrtc->group, true);
>> -
>> -	/* Enable the VSP compositor. */
>> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>> -		rcar_du_vsp_enable(rcrtc);
>> -
>> -	/* Turn vertical blanking interrupt reporting back on. */
>> -	drm_crtc_vblank_on(crtc);
>> -
>> -	rcrtc->started = true;
>>  }
>>  
>>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>  {
>>  	struct drm_crtc *crtc = &rcrtc->crtc;
>>  
>> -	if (!rcrtc->started)
>> -		return;
>> -
>>  	/* Disable all planes and wait for the change to take effect. This is
>>  	 * required as the DSnPR registers are updated on vblank, and no vblank
>>  	 * will occur once the CRTC is stopped. Disabling planes when starting
>> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>  	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>>  
>>  	rcar_du_group_start_stop(rcrtc->group, false);
>> -
>> -	rcrtc->started = false;
>>  }
>>  
>>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
>> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>  		return;
>>  
>>  	rcar_du_crtc_get(rcrtc);
>> -	rcar_du_crtc_start(rcrtc);
>> +	rcar_du_crtc_setup(rcrtc);
> 
> Every call to _setup is immediately prefixed by a call to _get()
> 
> Could the _get() be done in the _setup() for code reduction?
> 
> I'm entirely open to that not happening here as it might be preferred to keep
> the _get() and _start() separate for style purposes.
> 
>>  
>>  	/* Commit the planes state. */
>> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>> -		rcar_du_vsp_enable(rcrtc);
>> -	} else {
>> +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>>  		for (i = 0; i < rcrtc->group->num_planes; ++i) {
>>  			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
>>  
>> @@ -563,6 +553,7 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>  	}
>>  
>>  	rcar_du_crtc_update_planes(rcrtc);
>> +	rcar_du_crtc_start(rcrtc);
>>  }
>>  
>>  /* -----------------------------------------------------------------------------
>> @@ -574,7 +565,16 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>>  {
>>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>>  
>> -	rcar_du_crtc_get(rcrtc);
>> +	/*
>> +	 * If the CRTC has already been setup by the .atomic_begin() handler we
>> +	 * can skip the setup stage.
>> +	 */
>> +	if (!rcrtc->initialized) {
>> +		rcar_du_crtc_get(rcrtc);
>> +		rcar_du_crtc_setup(rcrtc);
>> +		rcrtc->initialized = true;
>> +	}
>> +
>>  	rcar_du_crtc_start(rcrtc);
>>  }
>>  
>> @@ -593,6 +593,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>>  	}
>>  	spin_unlock_irq(&crtc->dev->event_lock);
>>  
>> +	rcrtc->initialized = false;
>>  	rcrtc->outputs = 0;
>>  }
>>  
>> @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>>  {
>>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>>  
>> +	WARN_ON(!crtc->state->enable);
> 
> Is this necessary if it's handled by the rcrtc->initialized flag? or is it so
> that we find out if it happens ?
> 
> (Or is this due to the re-ordering of the _commit_tail() function below?)
> 
> 
>> +
>> +	/*
>> +	 * If a mode set is in progress we can be called with the CRTC disabled.
>> +	 * We then need to first setup the CRTC in order to configure planes.
>> +	 * The .atomic_enable() handler will notice and skip the CRTC setup.
>> +	 */
> 
> I'm assuming this comment is the reason for the WARN_ON above ...
> 
> 
>> +	if (!rcrtc->initialized) {
>> +		rcar_du_crtc_get(rcrtc);
>> +		rcar_du_crtc_setup(rcrtc);
>> +		rcrtc->initialized = true;
>> +	}
> 
> 
> If the _get() was moved into the _setup(), and _setup() was protected by the
> rcrtc->initialized flag, then _atomic_begin() _enable() and _resume() could all
> just simply call _setup(). The _resume() should only ever be called with
> rcrtc->initialized = false anyway, as that is set in _suspend()
> 
>> +
>>  	if (rcar_du_has(rcrtc->group->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 0b6d26ecfc38..3cc376826592 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> @@ -30,7 +30,7 @@ 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
>> - * @started: whether the CRTC has been started and is running
>> + * @initialized: whether the CRTC has been initialized and clocks enabled
>>   * @event: event to post when the pending page flip completes
>>   * @flip_wait: wait queue used to signal page flip completion
>>   * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
>> @@ -45,7 +45,7 @@ struct rcar_du_crtc {
>>  	struct clk *extclock;
>>  	unsigned int mmio_offset;
>>  	unsigned int index;
>> -	bool started;
>> +	bool initialized;
>>  
>>  	struct drm_pending_vblank_event *event;
>>  	wait_queue_head_t flip_wait;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> index 82b978a5dae6..c2f382feca07 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>>  
>>  	/* Apply the atomic update. */
>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>  	drm_atomic_helper_commit_planes(dev, old_state,
>>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much like
> the default drm_atomic_helper_commit_tail() code.
> 
> Reading around other uses /variants of commit_tail() style functions in other
> drivers has left me confused as to how the ordering affects things here.
> 
> Could be worth adding a comment at least to describe why we can't use the
> default helper...

Or better still ... Use Maxime's new :

[PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users




> 
> 
>> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>  >  	drm_atomic_helper_commit_hw_done(old_state);
>>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>>

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

On 13/07/17 16:51, Kieran Bingham wrote:
> Hi Laurent,
> 
> I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
> access bug" and it relates directly to a comment I had in this patch:
> 
> On 12/07/17 17:35, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> On 28/06/17 19:50, Laurent Pinchart wrote:
>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>>> start to CRTC resume") changed the order of the plane commit and CRTC
>>> enable operations to accommodate the runtime PM requirements. However,
>>> this introduced corruption in the first displayed frame, as the CRTC is
>>> now enabled without any plane configured. On Gen2 hardware the first
>>> frame will be black and likely unnoticed, but on Gen3 hardware we end up
>>> starting the display before the VSP compositor, which is more
>>> noticeable.
>>>
>>> To fix this, revert the order of the commit operations back, and handle
>>> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
>>> helper operation handlers.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> I only have code reduction or comment suggestions below - so either with or
>> without those changes, feel free to add my:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++--------------
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>>  3 files changed, 43 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> index 6b5219ef0ad2..76cdb88b2b8e 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>>>   * Start/Stop and Suspend/Resume
>>>   */
>>>  
>>> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>>>  {
>>> -	struct drm_crtc *crtc = &rcrtc->crtc;
>>> -	bool interlaced;
>>> -
>>> -	if (rcrtc->started)
>>> -		return;
>>> -
>>>  	/* Set display off and background to black */
>>>  	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>>>  	rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
>>> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>>  	/* Start with all planes disabled. */
>>>  	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))
>>> +		rcar_du_vsp_enable(rcrtc);
>>> +
>>> +	/* Turn vertical blanking interrupt reporting on. */
>>> +	drm_crtc_vblank_on(&rcrtc->crtc);
>>> +}
>>> +
>>> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>> +{
>>> +	bool interlaced;
>>> +
>>>  	/* Select master sync mode. This enables display operation in master
>>
>> Are we close enough here to fix this multiline comment style ?
>> (Not worth doing unless the patch is respun for other reasons ...)
>>
>> Actually - there are a lot in this file, so it would be better to do them all in
>> one hit/patch at a point of least conflicts ...
>>
>>
>>>  	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
>>>  	 * actively driven).
>>> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>>  			     DSYSR_TVM_MASTER);
>>>  
>>>  	rcar_du_group_start_stop(rcrtc->group, true);
>>> -
>>> -	/* Enable the VSP compositor. */
>>> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>>> -		rcar_du_vsp_enable(rcrtc);
>>> -
>>> -	/* Turn vertical blanking interrupt reporting back on. */
>>> -	drm_crtc_vblank_on(crtc);
>>> -
>>> -	rcrtc->started = true;
>>>  }
>>>  
>>>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>>  {
>>>  	struct drm_crtc *crtc = &rcrtc->crtc;
>>>  
>>> -	if (!rcrtc->started)
>>> -		return;
>>> -
>>>  	/* Disable all planes and wait for the change to take effect. This is
>>>  	 * required as the DSnPR registers are updated on vblank, and no vblank
>>>  	 * will occur once the CRTC is stopped. Disabling planes when starting
>>> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>>  	rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>>>  
>>>  	rcar_du_group_start_stop(rcrtc->group, false);
>>> -
>>> -	rcrtc->started = false;
>>>  }
>>>  
>>>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
>>> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>>  		return;
>>>  
>>>  	rcar_du_crtc_get(rcrtc);
>>> -	rcar_du_crtc_start(rcrtc);
>>> +	rcar_du_crtc_setup(rcrtc);
>>
>> Every call to _setup is immediately prefixed by a call to _get()
>>
>> Could the _get() be done in the _setup() for code reduction?
>>
>> I'm entirely open to that not happening here as it might be preferred to keep
>> the _get() and _start() separate for style purposes.
>>
>>>  
>>>  	/* Commit the planes state. */
>>> -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>>> -		rcar_du_vsp_enable(rcrtc);
>>> -	} else {
>>> +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>>>  		for (i = 0; i < rcrtc->group->num_planes; ++i) {
>>>  			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
>>>  
>>> @@ -563,6 +553,7 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>>  	}
>>>  
>>>  	rcar_du_crtc_update_planes(rcrtc);
>>> +	rcar_du_crtc_start(rcrtc);
>>>  }
>>>  
>>>  /* -----------------------------------------------------------------------------
>>> @@ -574,7 +565,16 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>>>  {
>>>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>>>  
>>> -	rcar_du_crtc_get(rcrtc);
>>> +	/*
>>> +	 * If the CRTC has already been setup by the .atomic_begin() handler we
>>> +	 * can skip the setup stage.
>>> +	 */
>>> +	if (!rcrtc->initialized) {
>>> +		rcar_du_crtc_get(rcrtc);
>>> +		rcar_du_crtc_setup(rcrtc);
>>> +		rcrtc->initialized = true;
>>> +	}
>>> +
>>>  	rcar_du_crtc_start(rcrtc);
>>>  }
>>>  
>>> @@ -593,6 +593,7 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>>>  	}
>>>  	spin_unlock_irq(&crtc->dev->event_lock);
>>>  
>>> +	rcrtc->initialized = false;
>>>  	rcrtc->outputs = 0;
>>>  }
>>>  
>>> @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
>>>  {
>>>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>>>  
>>> +	WARN_ON(!crtc->state->enable);
>>
>> Is this necessary if it's handled by the rcrtc->initialized flag? or is it so
>> that we find out if it happens ?
>>
>> (Or is this due to the re-ordering of the _commit_tail() function below?)
>>
>>
>>> +
>>> +	/*
>>> +	 * If a mode set is in progress we can be called with the CRTC disabled.
>>> +	 * We then need to first setup the CRTC in order to configure planes.
>>> +	 * The .atomic_enable() handler will notice and skip the CRTC setup.
>>> +	 */
>>
>> I'm assuming this comment is the reason for the WARN_ON above ...
>>
>>
>>> +	if (!rcrtc->initialized) {
>>> +		rcar_du_crtc_get(rcrtc);
>>> +		rcar_du_crtc_setup(rcrtc);
>>> +		rcrtc->initialized = true;
>>> +	}
>>
>>
>> If the _get() was moved into the _setup(), and _setup() was protected by the
>> rcrtc->initialized flag, then _atomic_begin() _enable() and _resume() could all
>> just simply call _setup(). The _resume() should only ever be called with
>> rcrtc->initialized = false anyway, as that is set in _suspend()
>>
>>> +
>>>  	if (rcar_du_has(rcrtc->group->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 0b6d26ecfc38..3cc376826592 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>>> @@ -30,7 +30,7 @@ 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
>>> - * @started: whether the CRTC has been started and is running
>>> + * @initialized: whether the CRTC has been initialized and clocks enabled
>>>   * @event: event to post when the pending page flip completes
>>>   * @flip_wait: wait queue used to signal page flip completion
>>>   * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
>>> @@ -45,7 +45,7 @@ struct rcar_du_crtc {
>>>  	struct clk *extclock;
>>>  	unsigned int mmio_offset;
>>>  	unsigned int index;
>>> -	bool started;
>>> +	bool initialized;
>>>  
>>>  	struct drm_pending_vblank_event *event;
>>>  	wait_queue_head_t flip_wait;
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> index 82b978a5dae6..c2f382feca07 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>>>  
>>>  	/* Apply the atomic update. */
>>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>>> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>>  	drm_atomic_helper_commit_planes(dev, old_state,
>>>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much like
>> the default drm_atomic_helper_commit_tail() code.
>>
>> Reading around other uses /variants of commit_tail() style functions in other
>> drivers has left me confused as to how the ordering affects things here.
>>
>> Could be worth adding a comment at least to describe why we can't use the
>> default helper...
> 
> Or better still ... Use Maxime's new :
> 
> [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users


Never mind - I've just looked again, and seen that this new helper function is
the ordering previous to *this* patch, and therefore isn't the same.

However - it's worth noting that Maxime's patch converts this function to the
new helper - which contradicts this patch's motivations.


>>
>>
>>> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>>  >  	drm_atomic_helper_commit_hw_done(old_state);
>>>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
>>>

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 16:51:18 Kieran Bingham wrote:
> Hi Laurent,
> 
> I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
> access bug" and it relates directly to a comment I had in this patch:
> On 12/07/17 17:35, Kieran Bingham wrote:
>
> > On 28/06/17 19:50, Laurent Pinchart wrote:
> >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> >> start to CRTC resume") changed the order of the plane commit and CRTC
> >> enable operations to accommodate the runtime PM requirements. However,
> >> this introduced corruption in the first displayed frame, as the CRTC is
> >> now enabled without any plane configured. On Gen2 hardware the first
> >> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> >> starting the display before the VSP compositor, which is more
> >> noticeable.
> >> 
> >> To fix this, revert the order of the commit operations back, and handle
> >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> >> helper operation handlers.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > I only have code reduction or comment suggestions below - so either with
> > or without those changes, feel free to add my:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++-----------
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
> >>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >>  3 files changed, 43 insertions(+), 29 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 82b978a5dae6..c2f382feca07
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct
> >> drm_atomic_state *old_state)>> 
> >>  	/* Apply the atomic update. */
> >>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >>  	drm_atomic_helper_commit_planes(dev, old_state,
> >>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > 
> > Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much
> > like the default drm_atomic_helper_commit_tail() code.
> > 
> > Reading around other uses /variants of commit_tail() style functions in
> > other drivers has left me confused as to how the ordering affects things
> > here.
> > 
> > Could be worth adding a comment at least to describe why we can't use the
> > default helper...
> 
> Or better still ... Use Maxime's new :
> 
> [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for
> runtime_pm users

Note that Maxime's patch implements the commit tail as

	drm_atomic_helper_commit_modeset_disables(dev, old_state);
	drm_atomic_helper_commit_modeset_enables(dev, old_state);
	drm_atomic_helper_commit_planes(dev, old_state,
					DRM_PLANE_COMMIT_ACTIVE_ONLY);

while this patches moves the drm_atomic_helper_commit_planes() back between 
drm_atomic_helper_commit_modeset_disables() and 
drm_atomic_helper_commit_modeset_enables().

> >> +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >> 
> >>  	drm_atomic_helper_commit_hw_done(old_state);
> >>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Kieran,

On Wednesday 12 Jul 2017 17:35:53 Kieran Bingham wrote:
> On 28/06/17 19:50, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> I only have code reduction or comment suggestions below - so either with or
> without those changes, feel free to add my:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++------------
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >  3 files changed, 43 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6b5219ef0ad2..76cdb88b2b8e
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c

[snip]

> > @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc
> > *rcrtc)
> >  	/* Start with all planes disabled. */
> >  	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))
> > +		rcar_du_vsp_enable(rcrtc);
> > +
> > +	/* Turn vertical blanking interrupt reporting on. */
> > +	drm_crtc_vblank_on(&rcrtc->crtc);
> > +}
> > +
> > +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> > +{
> > +	bool interlaced;
> > +
> >  	/* Select master sync mode. This enables display operation in master
> 
> Are we close enough here to fix this multiline comment style ?
> (Not worth doing unless the patch is respun for other reasons ...)
> 
> Actually - there are a lot in this file, so it would be better to do them
> all in one hit/patch at a point of least conflicts ...

Done :-) I actually had such a patch in my tree before receiving your comment.

> >  	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
> >  	 * actively driven).

[snip]

> > @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
> >  		return;
> >  	
> >  	rcar_du_crtc_get(rcrtc);
> > -	rcar_du_crtc_start(rcrtc);
> > +	rcar_du_crtc_setup(rcrtc);
> 
> Every call to _setup is immediately prefixed by a call to _get()
> 
> Could the _get() be done in the _setup() for code reduction?
> 
> I'm entirely open to that not happening here as it might be preferred to
> keep the _get() and _start() separate for style purposes.

Please see below.

> >  	/* Commit the planes state. */
> > -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > -		rcar_du_vsp_enable(rcrtc);
> > -	} else {
> > +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> >  		for (i = 0; i < rcrtc->group->num_planes; ++i) {
> >  			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
> > 

[snip]

> > @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >  {
> >  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > 
> > +	WARN_ON(!crtc->state->enable);
> 
> Is this necessary if it's handled by the rcrtc->initialized flag? or is it
> so that we find out if it happens ?
> 
> (Or is this due to the re-ordering of the _commit_tail() function below?)

It's to find out whether it happens. Before this patch the plane update
occurred after enabling the CRTC (through
drm_atomic_helper_commit_modeset_enables()). With this patch the CRTC can be
disabled at the hardware level, but only if it will be enabled right after
plane update. The state->enable flag should thus always be true, and I added
a WARN_ON to ensure that. I'd like to keep it a bit, we can remove it after
running more tests.

> > +
> > +	/*
> > +	 * If a mode set is in progress we can be called with the CRTC disabled.
> > +	 * We then need to first setup the CRTC in order to configure planes.
> > +	 * The .atomic_enable() handler will notice and skip the CRTC setup.
> > +	 */
> 
> I'm assuming this comment is the reason for the WARN_ON above ...
> 
> > +	if (!rcrtc->initialized) {
> > +		rcar_du_crtc_get(rcrtc);
> > +		rcar_du_crtc_setup(rcrtc);
> > +		rcrtc->initialized = true;
> > +	}
> 
> If the _get() was moved into the _setup(), and _setup() was protected by the
> rcrtc->initialized flag, then _atomic_begin() _enable() and _resume() could
> all just simply call _setup(). The _resume() should only ever be called
> with rcrtc->initialized = false anyway, as that is set in _suspend()

I think that makes sense, but I wonder whether it would make sense to address
that when fixing the currently broken suspend/resume code, as I expect the
get/setup/start sequence to be reworked then.

To help you decide, here's what we would merge now on top of this patch if we
don't wait.

-------------------------------------- 8< ------------------------------------commit 4e43b3a5400e40e8ef7ecc50640a2ca77fb8effa
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date:   Fri Jul 14 03:26:17 2017 +0300

    drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()
    
    The rcar_du_crtc_get() function is always immediately followed by a call
    to rcar_du_crtc_setup(). Call the later from the former to simplify the
    code, and add a comment to explain how the get and put calls are
    balanced.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 98cf446391dc..e29d8d494720 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -70,39 +70,6 @@ static void rcar_du_crtc_clr_set(struct rcar_du_crtc *rcrtc, u32 reg,
 	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
 }
 
-static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
-{
-	int ret;
-
-	ret = clk_prepare_enable(rcrtc->clock);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare_enable(rcrtc->extclock);
-	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;
-}
-
-static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
-{
-	rcar_du_group_put(rcrtc->group);
-
-	clk_disable_unprepare(rcrtc->extclock);
-	clk_disable_unprepare(rcrtc->clock);
-}
-
 /* -----------------------------------------------------------------------------
  * Hardware Setup
  */
@@ -473,6 +440,49 @@ 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)
+{
+	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;
+
+	ret = clk_prepare_enable(rcrtc->extclock);
+	if (ret < 0)
+		goto error_clock;
+
+	ret = rcar_du_group_get(rcrtc->group);
+	if (ret < 0)
+		goto error_group;
+
+	rcar_du_crtc_setup(rcrtc);
+	rcrtc->initialized = true;
+
+	return 0;
+
+error_group:
+	clk_disable_unprepare(rcrtc->extclock);
+error_clock:
+	clk_disable_unprepare(rcrtc->clock);
+	return ret;
+}
+
+static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
+{
+	rcar_du_group_put(rcrtc->group);
+
+	clk_disable_unprepare(rcrtc->extclock);
+	clk_disable_unprepare(rcrtc->clock);
+}
+
 static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 {
 	bool interlaced;
@@ -546,7 +556,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
 		return;
 
 	rcar_du_crtc_get(rcrtc);
-	rcar_du_crtc_setup(rcrtc);
 
 	/* Commit the planes state. */
 	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
@@ -573,16 +582,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
-	/*
-	 * If the CRTC has already been setup by the .atomic_begin() handler we
-	 * can skip the setup stage.
-	 */
-	if (!rcrtc->initialized) {
-		rcar_du_crtc_get(rcrtc);
-		rcar_du_crtc_setup(rcrtc);
-		rcrtc->initialized = true;
-	}
-
+	rcar_du_crtc_get(rcrtc);
 	rcar_du_crtc_start(rcrtc);
 }
 
@@ -614,14 +614,17 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 
 	/*
 	 * If a mode set is in progress we can be called with the CRTC disabled.
-	 * We then need to first setup the CRTC in order to configure planes.
-	 * The .atomic_enable() handler will notice and skip the CRTC setup.
+	 * 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 belance
+	 * it in .atomic_flush() either.
 	 */
-	if (!rcrtc->initialized) {
-		rcar_du_crtc_get(rcrtc);
-		rcar_du_crtc_setup(rcrtc);
-		rcrtc->initialized = true;
-	}
+	rcar_du_crtc_get(rcrtc);
 
 	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_begin(rcrtc);
-------------------------------------- 8< ------------------------------------

> > +
> >  	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> >  		rcar_du_vsp_atomic_begin(rcrtc);
> >  }

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 82b978a5dae6..c2f382feca07
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct
> > drm_atomic_state *old_state)
> > 
> >  	/* Apply the atomic update. */
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much
> like the default drm_atomic_helper_commit_tail() code.
> 
> Reading around other uses /variants of commit_tail() style functions in
> other drivers has left me confused as to how the ordering affects things
> here.
> 
> Could be worth adding a comment at least to describe why we can't use the
> default helper...

The first reason, as you mentioned, is DRM_PLANE_COMMIT_ACTIVE_ONLY, as we
don't need to receive plane updates for disabled CRTCs.

The second reason is patch "[PATCH] drm: rcar-du: Wait for flip completion
instead of vblank in  commit tail" that I have just submitted, which replaces
the drm_atomic_helper_wait_for_vblanks() call with
drm_atomic_helper_wait_flip_done(). I can add a comment to that patch if you
think that would be helpful.

> > +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > 
> >   	drm_atomic_helper_commit_hw_done(old_state);
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Kieran,

On Wednesday 12 Jul 2017 17:35:53 Kieran Bingham wrote:
> On 28/06/17 19:50, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> I only have code reduction or comment suggestions below - so either with or
> without those changes, feel free to add my:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++------------
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >  3 files changed, 43 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6b5219ef0ad2..76cdb88b2b8e
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c

[snip]

> > @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc
> > *rcrtc)
> >  	/* Start with all planes disabled. */
> >  	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))
> > +		rcar_du_vsp_enable(rcrtc);
> > +
> > +	/* Turn vertical blanking interrupt reporting on. */
> > +	drm_crtc_vblank_on(&rcrtc->crtc);
> > +}
> > +
> > +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> > +{
> > +	bool interlaced;
> > +
> >  	/* Select master sync mode. This enables display operation in master
> 
> Are we close enough here to fix this multiline comment style ?
> (Not worth doing unless the patch is respun for other reasons ...)
> 
> Actually - there are a lot in this file, so it would be better to do them
> all in one hit/patch at a point of least conflicts ...

Done :-) I actually had such a patch in my tree before receiving your comment.

> >  	 * sync mode (with the HSYNC and VSYNC signals configured as outputs and
> >  	 * actively driven).

[snip]

> > @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
> >  		return;
> >  	
> >  	rcar_du_crtc_get(rcrtc);
> > -	rcar_du_crtc_start(rcrtc);
> > +	rcar_du_crtc_setup(rcrtc);
> 
> Every call to _setup is immediately prefixed by a call to _get()
> 
> Could the _get() be done in the _setup() for code reduction?
> 
> I'm entirely open to that not happening here as it might be preferred to
> keep the _get() and _start() separate for style purposes.

Please see below.

> >  	/* Commit the planes state. */
> > -	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > -		rcar_du_vsp_enable(rcrtc);
> > -	} else {
> > +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> >  		for (i = 0; i < rcrtc->group->num_planes; ++i) {
> >  			struct rcar_du_plane *plane = &rcrtc->group->planes[i];
> > 

[snip]

> > @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >  {
> >  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > 
> > +	WARN_ON(!crtc->state->enable);
> 
> Is this necessary if it's handled by the rcrtc->initialized flag? or is it
> so that we find out if it happens ?
> 
> (Or is this due to the re-ordering of the _commit_tail() function below?)

It's to find out whether it happens. Before this patch the plane update
occurred after enabling the CRTC (through
drm_atomic_helper_commit_modeset_enables()). With this patch the CRTC can be
disabled at the hardware level, but only if it will be enabled right after
plane update. The state->enable flag should thus always be true, and I added
a WARN_ON to ensure that. I'd like to keep it a bit, we can remove it after
running more tests.

> > +
> > +	/*
> > +	 * If a mode set is in progress we can be called with the CRTC disabled.
> > +	 * We then need to first setup the CRTC in order to configure planes.
> > +	 * The .atomic_enable() handler will notice and skip the CRTC setup.
> > +	 */
> 
> I'm assuming this comment is the reason for the WARN_ON above ...
> 
> > +	if (!rcrtc->initialized) {
> > +		rcar_du_crtc_get(rcrtc);
> > +		rcar_du_crtc_setup(rcrtc);
> > +		rcrtc->initialized = true;
> > +	}
> 
> If the _get() was moved into the _setup(), and _setup() was protected by the
> rcrtc->initialized flag, then _atomic_begin() _enable() and _resume() could
> all just simply call _setup(). The _resume() should only ever be called
> with rcrtc->initialized = false anyway, as that is set in _suspend()

I think that makes sense, but I wonder whether it would make sense to address
that when fixing the currently broken suspend/resume code, as I expect the
get/setup/start sequence to be reworked then.

To help you decide, here's what we would merge now on top of this patch if we
don't wait.

-------------------------------------- 8< ------------------------------------commit 4e43b3a5400e40e8ef7ecc50640a2ca77fb8effa
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date:   Fri Jul 14 03:26:17 2017 +0300

    drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()
    
    The rcar_du_crtc_get() function is always immediately followed by a call
    to rcar_du_crtc_setup(). Call the later from the former to simplify the
    code, and add a comment to explain how the get and put calls are
    balanced.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 98cf446391dc..e29d8d494720 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -70,39 +70,6 @@ static void rcar_du_crtc_clr_set(struct rcar_du_crtc *rcrtc, u32 reg,
 	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
 }
 
-static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
-{
-	int ret;
-
-	ret = clk_prepare_enable(rcrtc->clock);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare_enable(rcrtc->extclock);
-	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;
-}
-
-static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
-{
-	rcar_du_group_put(rcrtc->group);
-
-	clk_disable_unprepare(rcrtc->extclock);
-	clk_disable_unprepare(rcrtc->clock);
-}
-
 /* -----------------------------------------------------------------------------
  * Hardware Setup
  */
@@ -473,6 +440,49 @@ 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)
+{
+	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;
+
+	ret = clk_prepare_enable(rcrtc->extclock);
+	if (ret < 0)
+		goto error_clock;
+
+	ret = rcar_du_group_get(rcrtc->group);
+	if (ret < 0)
+		goto error_group;
+
+	rcar_du_crtc_setup(rcrtc);
+	rcrtc->initialized = true;
+
+	return 0;
+
+error_group:
+	clk_disable_unprepare(rcrtc->extclock);
+error_clock:
+	clk_disable_unprepare(rcrtc->clock);
+	return ret;
+}
+
+static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
+{
+	rcar_du_group_put(rcrtc->group);
+
+	clk_disable_unprepare(rcrtc->extclock);
+	clk_disable_unprepare(rcrtc->clock);
+}
+
 static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 {
 	bool interlaced;
@@ -546,7 +556,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
 		return;
 
 	rcar_du_crtc_get(rcrtc);
-	rcar_du_crtc_setup(rcrtc);
 
 	/* Commit the planes state. */
 	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
@@ -573,16 +582,7 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
-	/*
-	 * If the CRTC has already been setup by the .atomic_begin() handler we
-	 * can skip the setup stage.
-	 */
-	if (!rcrtc->initialized) {
-		rcar_du_crtc_get(rcrtc);
-		rcar_du_crtc_setup(rcrtc);
-		rcrtc->initialized = true;
-	}
-
+	rcar_du_crtc_get(rcrtc);
 	rcar_du_crtc_start(rcrtc);
 }
 
@@ -614,14 +614,17 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
 
 	/*
 	 * If a mode set is in progress we can be called with the CRTC disabled.
-	 * We then need to first setup the CRTC in order to configure planes.
-	 * The .atomic_enable() handler will notice and skip the CRTC setup.
+	 * 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 belance
+	 * it in .atomic_flush() either.
 	 */
-	if (!rcrtc->initialized) {
-		rcar_du_crtc_get(rcrtc);
-		rcar_du_crtc_setup(rcrtc);
-		rcrtc->initialized = true;
-	}
+	rcar_du_crtc_get(rcrtc);
 
 	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_atomic_begin(rcrtc);
-------------------------------------- 8< ------------------------------------

> > +
> >  	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> >  		rcar_du_vsp_atomic_begin(rcrtc);
> >  }

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 82b978a5dae6..c2f382feca07
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct
> > drm_atomic_state *old_state)
> > 
> >  	/* Apply the atomic update. */
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> 
> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much
> like the default drm_atomic_helper_commit_tail() code.
> 
> Reading around other uses /variants of commit_tail() style functions in
> other drivers has left me confused as to how the ordering affects things
> here.
> 
> Could be worth adding a comment at least to describe why we can't use the
> default helper...

The first reason, as you mentioned, is DRM_PLANE_COMMIT_ACTIVE_ONLY, as we
don't need to receive plane updates for disabled CRTCs.

The second reason is patch "[PATCH] drm: rcar-du: Wait for flip completion
instead of vblank in  commit tail" that I have just submitted, which replaces
the drm_atomic_helper_wait_for_vblanks() call with
drm_atomic_helper_wait_flip_done(). I can add a comment to that patch if you
think that would be helpful.

> > +	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > 
> >   	drm_atomic_helper_commit_hw_done(old_state);
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On Thu, Jul 13, 2017 at 05:25:08PM +0100, Kieran Bingham wrote:
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> index 82b978a5dae6..c2f382feca07 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >>>  
> >>>  	/* Apply the atomic update. */
> >>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >>> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >>>  	drm_atomic_helper_commit_planes(dev, old_state,
> >>>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >>
> >> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much like
> >> the default drm_atomic_helper_commit_tail() code.
> >>
> >> Reading around other uses /variants of commit_tail() style functions in other
> >> drivers has left me confused as to how the ordering affects things here.
> >>
> >> Could be worth adding a comment at least to describe why we can't use the
> >> default helper...
> > 
> > Or better still ... Use Maxime's new :
> > 
> > [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users
> 
> Never mind - I've just looked again, and seen that this new helper function is
> the ordering previous to *this* patch, and therefore isn't the same.
> 
> However - it's worth noting that Maxime's patch converts this function to the
> new helper - which contradicts this patch's motivations.

So I guess I should drop the renesas modifications in my serie then?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Maxime,

On 17/07/17 07:32, Maxime Ripard wrote:
> On Thu, Jul 13, 2017 at 05:25:08PM +0100, Kieran Bingham wrote:
>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>>>> index 82b978a5dae6..c2f382feca07 100644
>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>>>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>>>>>  
>>>>>  	/* Apply the atomic update. */
>>>>>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>>>>> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>>>>  	drm_atomic_helper_commit_planes(dev, old_state,
>>>>>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>>>
>>>> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much like
>>>> the default drm_atomic_helper_commit_tail() code.
>>>>
>>>> Reading around other uses /variants of commit_tail() style functions in other
>>>> drivers has left me confused as to how the ordering affects things here.
>>>>
>>>> Could be worth adding a comment at least to describe why we can't use the
>>>> default helper...
>>>
>>> Or better still ... Use Maxime's new :
>>>
>>> [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users
>>
>> Never mind - I've just looked again, and seen that this new helper function is
>> the ordering previous to *this* patch, and therefore isn't the same.
>>
>> However - it's worth noting that Maxime's patch converts this function to the
>> new helper - which contradicts this patch's motivations.
> 
> So I guess I should drop the renesas modifications in my serie then?

Yes, Please.

I think we have a few extra modifications in this function as well which will
take it further away from a default implementation.

Regards

Kieran

> 
> Maxime
>