[PATCH v3 00/10] drm: rcar-du: Rework CRTC and groups for atomic commits

[PATCH v3 00/10] drm: rcar-du: Rework CRTC and groups for atomic commits

[Laurent Pinchart]

Hello everybody,

This patch series refactors atomic commit tail handling in the R-Car DU
driver to simplify the code flow, and open the door to further
optimisations. It takes over Kieran's "[PATCH v2 0/6] drm: rcar-du:
Rework CRTC and groups for atomic commits" and "[RFC PATCH 0/3] VSP1/DU
atomic interface changes" series.

The R-Car DU is a bit of a strange beast, with support for up to four
CRTCs that share resources in groups of two CRTCs. Depending on the
generation, planes can be shared (on Gen 1 and Gen 2), and output
routing configuration is also handled at the group level to some extent.
Furthermore, many configuration parameters, especially those related to
routing or clock handling, require the whole group to be restarted to
take effect, even when the parameter itself affects a single CRTC only.

This hardware architecture is difficult to handle properly on the
software side, and has resulted in group usage being reference-counted
while CRTC usage only tracks the enabled state. Calls are then
unbalanced and difficult to trace, especially for the configuration of
output routing, and implementation of new shared resources is hindered.
This patch series aims at solving this problem.

The series starts with 4 patches that touch the API between the DU and
VSP drivers. It became apparent that we need to split the configuration
of the VSP to allow fine grain control of setting the mode configuration
and enabling/disabling of the pipeline. To support the cross-component
API, the new interface is added in patch 01/10, including an
implementation of vsp1_du_setup_lif() to support the transition. Patch
02/10 prepares for the new call flow that will call the atomic flush
handler before enabling the pipeline. The DRM usage is adapted in patch
03/10, before the call is removed entirely in patch 04/10.

The next two patches convert CRTC clock handling and initial setup,
potentially called from both the CRTC .atomic_begin() and
.atomic_enable() operations, to a simpler code flow controlled by the
commit tail handler. Patch 05/10 takes the CRTCs out of standby and put
them back in standby respectively at the beginning and end of the commit
tail handler, based on the CRTC atomic state instead of state
information stored in the custom rcar_du_crtc structure. Patch 06/10
then performs a similar change for the CRTC mode setting configuration.

Finally, the last four patches introduce a DRM private object for the
CRTC groups, along with an associated state. Patch 07/10 adds a helper
macro to easily iterate over CRTC groups, and patch 08/10 adds the group
private objects and empty states. Patches 09/10 and 10/10 respectively
move the group setup and routing configuration under control of the
commit tail handler, simplifying the configuration and moving state
information from driver structures to state structures.

More refactoring is expected, with plane assignment being moved to group
states, and group restart being optimised to avoid flickering. Better
configuration of pixel clocks could also be implemented on top of this
series.

The whole series has been tested on M3-N and D3 boards with the DU test
suite (http://git.ideasonboard.com/renesas/kms-tests.git). Additional
tests have been developed and bugs in existing tests fixed, with patches
being posted to the linux-renesas-soc@vger.kernel.org mailing list that
will be integrated in the near future. All individual commits have been
tested on M3-N, while only key points (after patch 04/10 and patch
10/10) have been tested on D3. No failure or change in behaviour has
been noticed.

Kieran Bingham (8):
  media: vsp1: drm: Split vsp1_du_setup_lif()
  drm: rcar-du: Convert to the new VSP atomic API
  media: vsp1: drm: Remove vsp1_du_setup_lif()
  drm: rcar-du: Handle CRTC standby from commit tail handler
  drm: rcar-du: Handle CRTC configuration from commit tail handler
  drm: rcar-du: Provide for_each_group helper
  drm: rcar-du: Create a group state object
  drm: rcar-du: Perform group setup from the atomic tail handler

Laurent Pinchart (2):
  media: vsp1: drm: Don't configure hardware when the pipeline is
    disabled
  drm: rcar-du: Centralise routing configuration in commit tail handler

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 168 ++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |   9 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   6 +-
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 377 +++++++++++++++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  44 ++-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  63 ++--
 drivers/gpu/drm/rcar-du/rcar_du_plane.c |  10 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  20 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h   |   2 +
 drivers/media/platform/vsp1/vsp1_drm.c  | 189 ++++++++----
 drivers/media/platform/vsp1/vsp1_drm.h  |   2 +
 include/media/vsp1.h                    |  26 +-
 12 files changed, 637 insertions(+), 279 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 01/10] media: vsp1: drm: Split vsp1_du_setup_lif()

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Break vsp1_du_setup_lif() into components more suited to the DRM Atomic
API. The existing vsp1_du_setup_lif() API call is maintained as it is
still used from the DU.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Minor formatting changes
- Fix NULL pointer dereference in vsp1_du_setup_lif()
---
 drivers/media/platform/vsp1/vsp1_drm.c | 220 ++++++++++++++++++-------
 include/media/vsp1.h                   |  32 +++-
 2 files changed, 189 insertions(+), 63 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index a4a45d68a6ef..7957e1439de0 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -616,10 +616,10 @@ int vsp1_du_init(struct device *dev)
 EXPORT_SYMBOL_GPL(vsp1_du_init);
 
 /**
- * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
+ * vsp1_du_atomic_modeset - Configure the mode as part of an atomic update
  * @dev: the VSP device
  * @pipe_index: the DRM pipeline index
- * @cfg: the LIF configuration
+ * @cfg: the mode configuration
  *
  * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
  * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
@@ -636,14 +636,12 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
  *
  * Return 0 on success or a negative error code on failure.
  */
-int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
-		      const struct vsp1_du_lif_config *cfg)
+int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
+			   const struct vsp1_du_modeset_config *cfg)
 {
 	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
 	struct vsp1_drm_pipeline *drm_pipe;
 	struct vsp1_pipeline *pipe;
-	unsigned long flags;
-	unsigned int i;
 	int ret;
 
 	if (pipe_index >= vsp1->info->lif_count)
@@ -652,60 +650,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 	drm_pipe = &vsp1->drm->pipe[pipe_index];
 	pipe = &drm_pipe->pipe;
 
-	if (!cfg) {
-		struct vsp1_brx *brx;
-
-		mutex_lock(&vsp1->drm->lock);
-
-		brx = to_brx(&pipe->brx->subdev);
-
-		/*
-		 * NULL configuration means the CRTC is being disabled, stop
-		 * the pipeline and turn the light off.
-		 */
-		ret = vsp1_pipeline_stop(pipe);
-		if (ret == -ETIMEDOUT)
-			dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
-
-		for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
-			struct vsp1_rwpf *rpf = pipe->inputs[i];
-
-			if (!rpf)
-				continue;
-
-			/*
-			 * Remove the RPF from the pipe and the list of BRx
-			 * inputs.
-			 */
-			WARN_ON(!rpf->entity.pipe);
-			rpf->entity.pipe = NULL;
-			list_del(&rpf->entity.list_pipe);
-			pipe->inputs[i] = NULL;
-
-			brx->inputs[rpf->brx_input].rpf = NULL;
-		}
-
-		drm_pipe->du_complete = NULL;
-		pipe->num_inputs = 0;
-
-		dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n",
-			__func__, pipe->lif->index,
-			BRX_NAME(pipe->brx));
-
-		list_del(&pipe->brx->list_pipe);
-		pipe->brx->pipe = NULL;
-		pipe->brx = NULL;
-
-		mutex_unlock(&vsp1->drm->lock);
-
-		vsp1_dlm_reset(pipe->output->dlm);
-		vsp1_device_put(vsp1);
-
-		dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
-
-		return 0;
-	}
-
 	drm_pipe->width = cfg->width;
 	drm_pipe->height = cfg->height;
 	pipe->interlaced = cfg->interlaced;
@@ -722,8 +666,43 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 		goto unlock;
 
 	ret = vsp1_du_pipeline_setup_output(vsp1, pipe);
-	if (ret < 0)
-		goto unlock;
+
+unlock:
+	mutex_unlock(&vsp1->drm->lock);
+
+	return ret;
+}
+
+/**
+ * vsp1_du_atomic_enable - Enable and start a DU pipeline
+ * @dev: the VSP device
+ * @pipe_index: the DRM pipeline index
+ * @cfg: the enablement configuration
+ *
+ * The @pipe_index argument selects which DRM pipeline to enable. The number of
+ * available pipelines depend on the VSP instance.
+ *
+ * The configuration passes a callback function to register notification of
+ * frame completion events.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
+			  const struct vsp1_du_enable_config *cfg)
+{
+	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+	struct vsp1_drm_pipeline *drm_pipe;
+	struct vsp1_pipeline *pipe;
+	unsigned long flags;
+	int ret;
+
+	if (pipe_index >= vsp1->info->lif_count)
+		return -EINVAL;
+
+	drm_pipe = &vsp1->drm->pipe[pipe_index];
+	pipe = &drm_pipe->pipe;
+
+	mutex_lock(&vsp1->drm->lock);
 
 	/* Enable the VSP1. */
 	ret = vsp1_device_get(vsp1);
@@ -758,6 +737,123 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 	dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);
 
 	return 0;
+
+}
+EXPORT_SYMBOL_GPL(vsp1_du_atomic_enable);
+
+/**
+ * vsp1_du_atomic_disable - Disable and stop a DU pipeline
+ * @dev: the VSP device
+ * @pipe_index: the DRM pipeline index
+ *
+ * The @pipe_index argument selects which DRM pipeline to disable. The number
+ * of available pipelines depend on the VSP instance.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
+{
+	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+	struct vsp1_drm_pipeline *drm_pipe;
+	struct vsp1_pipeline *pipe;
+	struct vsp1_brx *brx;
+	unsigned int i;
+	int ret;
+
+	if (pipe_index >= vsp1->info->lif_count)
+		return -EINVAL;
+
+	drm_pipe = &vsp1->drm->pipe[pipe_index];
+	pipe = &drm_pipe->pipe;
+
+	mutex_lock(&vsp1->drm->lock);
+
+	brx = to_brx(&pipe->brx->subdev);
+
+	/* Stop the pipeline and turn the light off. */
+	ret = vsp1_pipeline_stop(pipe);
+	if (ret == -ETIMEDOUT)
+		dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
+
+	for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
+		struct vsp1_rwpf *rpf = pipe->inputs[i];
+
+		if (!rpf)
+			continue;
+
+		/* Remove the RPF from the pipe and the list of BRx inputs. */
+		WARN_ON(!rpf->entity.pipe);
+		rpf->entity.pipe = NULL;
+		list_del(&rpf->entity.list_pipe);
+		pipe->inputs[i] = NULL;
+
+		brx->inputs[rpf->brx_input].rpf = NULL;
+	}
+
+	drm_pipe->du_complete = NULL;
+	pipe->num_inputs = 0;
+
+	dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n", __func__,
+		pipe->lif->index, BRX_NAME(pipe->brx));
+
+	list_del(&pipe->brx->list_pipe);
+	pipe->brx->pipe = NULL;
+	pipe->brx = NULL;
+
+	mutex_unlock(&vsp1->drm->lock);
+
+	vsp1_dlm_reset(pipe->output->dlm);
+	vsp1_device_put(vsp1);
+
+	dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vsp1_du_atomic_disable);
+
+/**
+ * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
+ * @dev: the VSP device
+ * @pipe_index: the DRM pipeline index
+ * @cfg: the LIF configuration
+ *
+ * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
+ * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
+ * source pads, and the LIF sink pad.
+ *
+ * The @pipe_index argument selects which DRM pipeline to setup. The number of
+ * available pipelines depend on the VSP instance.
+ *
+ * As the media bus code on the blend unit source pad is conditioned by the
+ * configuration of its sink 0 pad, we also set up the formats on all blend unit
+ * sinks, even if the configuration will be overwritten later by
+ * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
+ * a well defined state.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
+		      const struct vsp1_du_lif_config *cfg)
+{
+	struct vsp1_du_modeset_config modes;
+	struct vsp1_du_enable_config enable;
+	int ret;
+
+	if (!cfg)
+		return vsp1_du_atomic_disable(dev, pipe_index);
+
+	modes.width = cfg->width;
+	modes.height = cfg->height;
+	modes.interlaced = cfg->interlaced;
+
+	ret = vsp1_du_atomic_modeset(dev, pipe_index, &modes);
+	if (ret)
+		return ret;
+
+	enable.callback = cfg->callback;
+	enable.callback_data = cfg->callback_data;
+
+	return vsp1_du_atomic_enable(dev, pipe_index, &enable);
 }
 EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
 
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index cc1b0d42ce95..56643f97d4c9 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -21,7 +21,7 @@ int vsp1_du_init(struct device *dev);
 #define VSP1_DU_STATUS_WRITEBACK	BIT(1)
 
 /**
- * struct vsp1_du_lif_config - VSP LIF configuration
+ * struct vsp1_du_lif_config - VSP LIF configuration - Deprecated
  * @width: output frame width
  * @height: output frame height
  * @interlaced: true for interlaced pipelines
@@ -42,6 +42,30 @@ struct vsp1_du_lif_config {
 int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 		      const struct vsp1_du_lif_config *cfg);
 
+/**
+ * struct vsp1_du_modeset_config - VSP display mode configuration
+ * @width: output frame width
+ * @height: output frame height
+ * @interlaced: true for interlaced pipelines
+ */
+struct vsp1_du_modeset_config {
+	unsigned int width;
+	unsigned int height;
+	bool interlaced;
+};
+
+/**
+ * struct vsp1_du_enable_config - VSP enable configuration
+ * @callback: frame completion callback function (optional). When a callback
+ *	      is provided, the VSP driver guarantees that it will be called once
+ *	      and only once for each vsp1_du_atomic_flush() call.
+ * @callback_data: data to be passed to the frame completion callback
+ */
+struct vsp1_du_enable_config {
+	void (*callback)(void *data, unsigned int status, u32 crc);
+	void *callback_data;
+};
+
 /**
  * struct vsp1_du_atomic_config - VSP atomic configuration parameters
  * @pixelformat: plane pixel format (V4L2 4CC)
@@ -106,6 +130,12 @@ struct vsp1_du_atomic_pipe_config {
 	struct vsp1_du_writeback_config writeback;
 };
 
+
+int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
+		    const struct vsp1_du_modeset_config *cfg);
+int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
+		   const struct vsp1_du_enable_config *cfg);
+int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index);
 void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
 int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
 			  unsigned int rpf,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 01/10] media: vsp1: drm: Split vsp1_du_setup_lif()

[Ulrich Hecht]

Thank you for your patch.

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Break vsp1_du_setup_lif() into components more suited to the DRM Atomic
> API. The existing vsp1_du_setup_lif() API call is maintained as it is
> still used from the DU.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Minor formatting changes
> - Fix NULL pointer dereference in vsp1_du_setup_lif()
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 220 ++++++++++++++++++-------
>  include/media/vsp1.h                   |  32 +++-
>  2 files changed, 189 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index a4a45d68a6ef..7957e1439de0 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -616,10 +616,10 @@ int vsp1_du_init(struct device *dev)
>  EXPORT_SYMBOL_GPL(vsp1_du_init);
>  
>  /**
> - * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> + * vsp1_du_atomic_modeset - Configure the mode as part of an atomic update
>   * @dev: the VSP device
>   * @pipe_index: the DRM pipeline index
> - * @cfg: the LIF configuration
> + * @cfg: the mode configuration
>   *
>   * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
>   * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
> @@ -636,14 +636,12 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
>   *
>   * Return 0 on success or a negative error code on failure.
>   */
> -int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> -		      const struct vsp1_du_lif_config *cfg)
> +int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
> +			   const struct vsp1_du_modeset_config *cfg)
>  {
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  	struct vsp1_drm_pipeline *drm_pipe;
>  	struct vsp1_pipeline *pipe;
> -	unsigned long flags;
> -	unsigned int i;
>  	int ret;
>  
>  	if (pipe_index >= vsp1->info->lif_count)
> @@ -652,60 +650,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
>  	drm_pipe = &vsp1->drm->pipe[pipe_index];
>  	pipe = &drm_pipe->pipe;
>  
> -	if (!cfg) {
> -		struct vsp1_brx *brx;
> -
> -		mutex_lock(&vsp1->drm->lock);
> -
> -		brx = to_brx(&pipe->brx->subdev);
> -
> -		/*
> -		 * NULL configuration means the CRTC is being disabled, stop
> -		 * the pipeline and turn the light off.
> -		 */
> -		ret = vsp1_pipeline_stop(pipe);
> -		if (ret == -ETIMEDOUT)
> -			dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
> -
> -		for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
> -			struct vsp1_rwpf *rpf = pipe->inputs[i];
> -
> -			if (!rpf)
> -				continue;
> -
> -			/*
> -			 * Remove the RPF from the pipe and the list of BRx
> -			 * inputs.
> -			 */
> -			WARN_ON(!rpf->entity.pipe);
> -			rpf->entity.pipe = NULL;
> -			list_del(&rpf->entity.list_pipe);
> -			pipe->inputs[i] = NULL;
> -
> -			brx->inputs[rpf->brx_input].rpf = NULL;
> -		}
> -
> -		drm_pipe->du_complete = NULL;
> -		pipe->num_inputs = 0;
> -
> -		dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n",
> -			__func__, pipe->lif->index,
> -			BRX_NAME(pipe->brx));
> -
> -		list_del(&pipe->brx->list_pipe);
> -		pipe->brx->pipe = NULL;
> -		pipe->brx = NULL;
> -
> -		mutex_unlock(&vsp1->drm->lock);
> -
> -		vsp1_dlm_reset(pipe->output->dlm);
> -		vsp1_device_put(vsp1);
> -
> -		dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
> -
> -		return 0;
> -	}
> -
>  	drm_pipe->width = cfg->width;
>  	drm_pipe->height = cfg->height;
>  	pipe->interlaced = cfg->interlaced;
> @@ -722,8 +666,43 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
>  		goto unlock;
>  
>  	ret = vsp1_du_pipeline_setup_output(vsp1, pipe);
> -	if (ret < 0)
> -		goto unlock;
> +
> +unlock:
> +	mutex_unlock(&vsp1->drm->lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * vsp1_du_atomic_enable - Enable and start a DU pipeline
> + * @dev: the VSP device
> + * @pipe_index: the DRM pipeline index
> + * @cfg: the enablement configuration
> + *
> + * The @pipe_index argument selects which DRM pipeline to enable. The number of
> + * available pipelines depend on the VSP instance.

"depends".

> + *
> + * The configuration passes a callback function to register notification of
> + * frame completion events.
> + *
> + * Return 0 on success or a negative error code on failure.
> + */
> +int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
> +			  const struct vsp1_du_enable_config *cfg)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +	struct vsp1_drm_pipeline *drm_pipe;
> +	struct vsp1_pipeline *pipe;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (pipe_index >= vsp1->info->lif_count)
> +		return -EINVAL;
> +
> +	drm_pipe = &vsp1->drm->pipe[pipe_index];
> +	pipe = &drm_pipe->pipe;
> +
> +	mutex_lock(&vsp1->drm->lock);
>  
>  	/* Enable the VSP1. */
>  	ret = vsp1_device_get(vsp1);
> @@ -758,6 +737,123 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
>  	dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);
>  
>  	return 0;
> +
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_atomic_enable);
> +
> +/**
> + * vsp1_du_atomic_disable - Disable and stop a DU pipeline
> + * @dev: the VSP device
> + * @pipe_index: the DRM pipeline index
> + *
> + * The @pipe_index argument selects which DRM pipeline to disable. The number
> + * of available pipelines depend on the VSP instance.
> + *
> + * Return 0 on success or a negative error code on failure.
> + */
> +int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +	struct vsp1_drm_pipeline *drm_pipe;
> +	struct vsp1_pipeline *pipe;
> +	struct vsp1_brx *brx;
> +	unsigned int i;
> +	int ret;
> +
> +	if (pipe_index >= vsp1->info->lif_count)
> +		return -EINVAL;
> +
> +	drm_pipe = &vsp1->drm->pipe[pipe_index];
> +	pipe = &drm_pipe->pipe;
> +
> +	mutex_lock(&vsp1->drm->lock);
> +
> +	brx = to_brx(&pipe->brx->subdev);
> +
> +	/* Stop the pipeline and turn the light off. */

I may have missed something, but it is not clear to me what "light" refers to here.

> +	ret = vsp1_pipeline_stop(pipe);
> +	if (ret == -ETIMEDOUT)
> +		dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
> +		struct vsp1_rwpf *rpf = pipe->inputs[i];
> +
> +		if (!rpf)
> +			continue;
> +
> +		/* Remove the RPF from the pipe and the list of BRx inputs. */
> +		WARN_ON(!rpf->entity.pipe);
> +		rpf->entity.pipe = NULL;
> +		list_del(&rpf->entity.list_pipe);
> +		pipe->inputs[i] = NULL;
> +
> +		brx->inputs[rpf->brx_input].rpf = NULL;
> +	}
> +
> +	drm_pipe->du_complete = NULL;
> +	pipe->num_inputs = 0;
> +
> +	dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n", __func__,
> +		pipe->lif->index, BRX_NAME(pipe->brx));
> +
> +	list_del(&pipe->brx->list_pipe);
> +	pipe->brx->pipe = NULL;
> +	pipe->brx = NULL;
> +
> +	mutex_unlock(&vsp1->drm->lock);
> +
> +	vsp1_dlm_reset(pipe->output->dlm);
> +	vsp1_device_put(vsp1);
> +
> +	dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_atomic_disable);
> +
> +/**
> + * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> + * @dev: the VSP device
> + * @pipe_index: the DRM pipeline index
> + * @cfg: the LIF configuration
> + *
> + * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
> + * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
> + * source pads, and the LIF sink pad.
> + *
> + * The @pipe_index argument selects which DRM pipeline to setup. The number of
> + * available pipelines depend on the VSP instance.
> + *
> + * As the media bus code on the blend unit source pad is conditioned by the
> + * configuration of its sink 0 pad, we also set up the formats on all blend unit
> + * sinks, even if the configuration will be overwritten later by
> + * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
> + * a well defined state.
> + *
> + * Return 0 on success or a negative error code on failure.
> + */
> +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> +		      const struct vsp1_du_lif_config *cfg)
> +{
> +	struct vsp1_du_modeset_config modes;
> +	struct vsp1_du_enable_config enable;
> +	int ret;
> +
> +	if (!cfg)
> +		return vsp1_du_atomic_disable(dev, pipe_index);
> +
> +	modes.width = cfg->width;
> +	modes.height = cfg->height;
> +	modes.interlaced = cfg->interlaced;
> +
> +	ret = vsp1_du_atomic_modeset(dev, pipe_index, &modes);
> +	if (ret)
> +		return ret;
> +
> +	enable.callback = cfg->callback;
> +	enable.callback_data = cfg->callback_data;
> +
> +	return vsp1_du_atomic_enable(dev, pipe_index, &enable);
>  }
>  EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
>  
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index cc1b0d42ce95..56643f97d4c9 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -21,7 +21,7 @@ int vsp1_du_init(struct device *dev);
>  #define VSP1_DU_STATUS_WRITEBACK	BIT(1)
>  
>  /**
> - * struct vsp1_du_lif_config - VSP LIF configuration
> + * struct vsp1_du_lif_config - VSP LIF configuration - Deprecated
>   * @width: output frame width
>   * @height: output frame height
>   * @interlaced: true for interlaced pipelines
> @@ -42,6 +42,30 @@ struct vsp1_du_lif_config {
>  int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
>  		      const struct vsp1_du_lif_config *cfg);
>  
> +/**
> + * struct vsp1_du_modeset_config - VSP display mode configuration
> + * @width: output frame width
> + * @height: output frame height
> + * @interlaced: true for interlaced pipelines
> + */
> +struct vsp1_du_modeset_config {
> +	unsigned int width;
> +	unsigned int height;
> +	bool interlaced;
> +};
> +
> +/**
> + * struct vsp1_du_enable_config - VSP enable configuration
> + * @callback: frame completion callback function (optional). When a callback
> + *	      is provided, the VSP driver guarantees that it will be called once
> + *	      and only once for each vsp1_du_atomic_flush() call.
> + * @callback_data: data to be passed to the frame completion callback
> + */
> +struct vsp1_du_enable_config {
> +	void (*callback)(void *data, unsigned int status, u32 crc);
> +	void *callback_data;
> +};
> +
>  /**
>   * struct vsp1_du_atomic_config - VSP atomic configuration parameters
>   * @pixelformat: plane pixel format (V4L2 4CC)
> @@ -106,6 +130,12 @@ struct vsp1_du_atomic_pipe_config {
>  	struct vsp1_du_writeback_config writeback;
>  };
>  
> +
> +int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
> +		    const struct vsp1_du_modeset_config *cfg);
> +int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
> +		   const struct vsp1_du_enable_config *cfg);
> +int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index);
>  void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
>  int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
>  			  unsigned int rpf,
> -- 
> Regards,
> 
> Laurent Pinchart
>

With those minor issues fixed:

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH v3 01/10] media: vsp1: drm: Split vsp1_du_setup_lif()

[Laurent Pinchart]

Hi Ulrich,

On Tue, Jun 18, 2019 at 02:32:13PM +0200, Ulrich Hecht wrote:
> > On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> > 
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Break vsp1_du_setup_lif() into components more suited to the DRM Atomic
> > API. The existing vsp1_du_setup_lif() API call is maintained as it is
> > still used from the DU.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > Changes since v2:
> > 
> > - Minor formatting changes
> > - Fix NULL pointer dereference in vsp1_du_setup_lif()
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 220 ++++++++++++++++++-------
> >  include/media/vsp1.h                   |  32 +++-
> >  2 files changed, 189 insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index a4a45d68a6ef..7957e1439de0 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -616,10 +616,10 @@ int vsp1_du_init(struct device *dev)
> >  EXPORT_SYMBOL_GPL(vsp1_du_init);
> >  
> >  /**
> > - * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> > + * vsp1_du_atomic_modeset - Configure the mode as part of an atomic update
> >   * @dev: the VSP device
> >   * @pipe_index: the DRM pipeline index
> > - * @cfg: the LIF configuration
> > + * @cfg: the mode configuration
> >   *
> >   * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
> >   * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
> > @@ -636,14 +636,12 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
> >   *
> >   * Return 0 on success or a negative error code on failure.
> >   */
> > -int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> > -		      const struct vsp1_du_lif_config *cfg)
> > +int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
> > +			   const struct vsp1_du_modeset_config *cfg)
> >  {
> >  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >  	struct vsp1_drm_pipeline *drm_pipe;
> >  	struct vsp1_pipeline *pipe;
> > -	unsigned long flags;
> > -	unsigned int i;
> >  	int ret;
> >  
> >  	if (pipe_index >= vsp1->info->lif_count)
> > @@ -652,60 +650,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> >  	drm_pipe = &vsp1->drm->pipe[pipe_index];
> >  	pipe = &drm_pipe->pipe;
> >  
> > -	if (!cfg) {
> > -		struct vsp1_brx *brx;
> > -
> > -		mutex_lock(&vsp1->drm->lock);
> > -
> > -		brx = to_brx(&pipe->brx->subdev);
> > -
> > -		/*
> > -		 * NULL configuration means the CRTC is being disabled, stop
> > -		 * the pipeline and turn the light off.
> > -		 */
> > -		ret = vsp1_pipeline_stop(pipe);
> > -		if (ret == -ETIMEDOUT)
> > -			dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
> > -
> > -		for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
> > -			struct vsp1_rwpf *rpf = pipe->inputs[i];
> > -
> > -			if (!rpf)
> > -				continue;
> > -
> > -			/*
> > -			 * Remove the RPF from the pipe and the list of BRx
> > -			 * inputs.
> > -			 */
> > -			WARN_ON(!rpf->entity.pipe);
> > -			rpf->entity.pipe = NULL;
> > -			list_del(&rpf->entity.list_pipe);
> > -			pipe->inputs[i] = NULL;
> > -
> > -			brx->inputs[rpf->brx_input].rpf = NULL;
> > -		}
> > -
> > -		drm_pipe->du_complete = NULL;
> > -		pipe->num_inputs = 0;
> > -
> > -		dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n",
> > -			__func__, pipe->lif->index,
> > -			BRX_NAME(pipe->brx));
> > -
> > -		list_del(&pipe->brx->list_pipe);
> > -		pipe->brx->pipe = NULL;
> > -		pipe->brx = NULL;
> > -
> > -		mutex_unlock(&vsp1->drm->lock);
> > -
> > -		vsp1_dlm_reset(pipe->output->dlm);
> > -		vsp1_device_put(vsp1);
> > -
> > -		dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
> > -
> > -		return 0;
> > -	}
> > -
> >  	drm_pipe->width = cfg->width;
> >  	drm_pipe->height = cfg->height;
> >  	pipe->interlaced = cfg->interlaced;
> > @@ -722,8 +666,43 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> >  		goto unlock;
> >  
> >  	ret = vsp1_du_pipeline_setup_output(vsp1, pipe);
> > -	if (ret < 0)
> > -		goto unlock;
> > +
> > +unlock:
> > +	mutex_unlock(&vsp1->drm->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * vsp1_du_atomic_enable - Enable and start a DU pipeline
> > + * @dev: the VSP device
> > + * @pipe_index: the DRM pipeline index
> > + * @cfg: the enablement configuration
> > + *
> > + * The @pipe_index argument selects which DRM pipeline to enable. The number of
> > + * available pipelines depend on the VSP instance.
> 
> "depends".
> 
> > + *
> > + * The configuration passes a callback function to register notification of
> > + * frame completion events.
> > + *
> > + * Return 0 on success or a negative error code on failure.
> > + */
> > +int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
> > +			  const struct vsp1_du_enable_config *cfg)
> > +{
> > +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > +	struct vsp1_drm_pipeline *drm_pipe;
> > +	struct vsp1_pipeline *pipe;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	if (pipe_index >= vsp1->info->lif_count)
> > +		return -EINVAL;
> > +
> > +	drm_pipe = &vsp1->drm->pipe[pipe_index];
> > +	pipe = &drm_pipe->pipe;
> > +
> > +	mutex_lock(&vsp1->drm->lock);
> >  
> >  	/* Enable the VSP1. */
> >  	ret = vsp1_device_get(vsp1);
> > @@ -758,6 +737,123 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> >  	dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);
> >  
> >  	return 0;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(vsp1_du_atomic_enable);
> > +
> > +/**
> > + * vsp1_du_atomic_disable - Disable and stop a DU pipeline
> > + * @dev: the VSP device
> > + * @pipe_index: the DRM pipeline index
> > + *
> > + * The @pipe_index argument selects which DRM pipeline to disable. The number
> > + * of available pipelines depend on the VSP instance.
> > + *
> > + * Return 0 on success or a negative error code on failure.
> > + */
> > +int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
> > +{
> > +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > +	struct vsp1_drm_pipeline *drm_pipe;
> > +	struct vsp1_pipeline *pipe;
> > +	struct vsp1_brx *brx;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (pipe_index >= vsp1->info->lif_count)
> > +		return -EINVAL;
> > +
> > +	drm_pipe = &vsp1->drm->pipe[pipe_index];
> > +	pipe = &drm_pipe->pipe;
> > +
> > +	mutex_lock(&vsp1->drm->lock);
> > +
> > +	brx = to_brx(&pipe->brx->subdev);
> > +
> > +	/* Stop the pipeline and turn the light off. */
> 
> I may have missed something, but it is not clear to me what "light" refers to here.

It's a metaphor, meaning disable everything :-)

> > +	ret = vsp1_pipeline_stop(pipe);
> > +	if (ret == -ETIMEDOUT)
> > +		dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
> > +		struct vsp1_rwpf *rpf = pipe->inputs[i];
> > +
> > +		if (!rpf)
> > +			continue;
> > +
> > +		/* Remove the RPF from the pipe and the list of BRx inputs. */
> > +		WARN_ON(!rpf->entity.pipe);
> > +		rpf->entity.pipe = NULL;
> > +		list_del(&rpf->entity.list_pipe);
> > +		pipe->inputs[i] = NULL;
> > +
> > +		brx->inputs[rpf->brx_input].rpf = NULL;
> > +	}
> > +
> > +	drm_pipe->du_complete = NULL;
> > +	pipe->num_inputs = 0;
> > +
> > +	dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n", __func__,
> > +		pipe->lif->index, BRX_NAME(pipe->brx));
> > +
> > +	list_del(&pipe->brx->list_pipe);
> > +	pipe->brx->pipe = NULL;
> > +	pipe->brx = NULL;
> > +
> > +	mutex_unlock(&vsp1->drm->lock);
> > +
> > +	vsp1_dlm_reset(pipe->output->dlm);
> > +	vsp1_device_put(vsp1);
> > +
> > +	dev_dbg(vsp1->dev, "%s: pipeline disabled\n", __func__);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(vsp1_du_atomic_disable);
> > +
> > +/**
> > + * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> > + * @dev: the VSP device
> > + * @pipe_index: the DRM pipeline index
> > + * @cfg: the LIF configuration
> > + *
> > + * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
> > + * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
> > + * source pads, and the LIF sink pad.
> > + *
> > + * The @pipe_index argument selects which DRM pipeline to setup. The number of
> > + * available pipelines depend on the VSP instance.
> > + *
> > + * As the media bus code on the blend unit source pad is conditioned by the
> > + * configuration of its sink 0 pad, we also set up the formats on all blend unit
> > + * sinks, even if the configuration will be overwritten later by
> > + * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
> > + * a well defined state.
> > + *
> > + * Return 0 on success or a negative error code on failure.
> > + */
> > +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> > +		      const struct vsp1_du_lif_config *cfg)
> > +{
> > +	struct vsp1_du_modeset_config modes;
> > +	struct vsp1_du_enable_config enable;
> > +	int ret;
> > +
> > +	if (!cfg)
> > +		return vsp1_du_atomic_disable(dev, pipe_index);
> > +
> > +	modes.width = cfg->width;
> > +	modes.height = cfg->height;
> > +	modes.interlaced = cfg->interlaced;
> > +
> > +	ret = vsp1_du_atomic_modeset(dev, pipe_index, &modes);
> > +	if (ret)
> > +		return ret;
> > +
> > +	enable.callback = cfg->callback;
> > +	enable.callback_data = cfg->callback_data;
> > +
> > +	return vsp1_du_atomic_enable(dev, pipe_index, &enable);
> >  }
> >  EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
> >  
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index cc1b0d42ce95..56643f97d4c9 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -21,7 +21,7 @@ int vsp1_du_init(struct device *dev);
> >  #define VSP1_DU_STATUS_WRITEBACK	BIT(1)
> >  
> >  /**
> > - * struct vsp1_du_lif_config - VSP LIF configuration
> > + * struct vsp1_du_lif_config - VSP LIF configuration - Deprecated
> >   * @width: output frame width
> >   * @height: output frame height
> >   * @interlaced: true for interlaced pipelines
> > @@ -42,6 +42,30 @@ struct vsp1_du_lif_config {
> >  int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> >  		      const struct vsp1_du_lif_config *cfg);
> >  
> > +/**
> > + * struct vsp1_du_modeset_config - VSP display mode configuration
> > + * @width: output frame width
> > + * @height: output frame height
> > + * @interlaced: true for interlaced pipelines
> > + */
> > +struct vsp1_du_modeset_config {
> > +	unsigned int width;
> > +	unsigned int height;
> > +	bool interlaced;
> > +};
> > +
> > +/**
> > + * struct vsp1_du_enable_config - VSP enable configuration
> > + * @callback: frame completion callback function (optional). When a callback
> > + *	      is provided, the VSP driver guarantees that it will be called once
> > + *	      and only once for each vsp1_du_atomic_flush() call.
> > + * @callback_data: data to be passed to the frame completion callback
> > + */
> > +struct vsp1_du_enable_config {
> > +	void (*callback)(void *data, unsigned int status, u32 crc);
> > +	void *callback_data;
> > +};
> > +
> >  /**
> >   * struct vsp1_du_atomic_config - VSP atomic configuration parameters
> >   * @pixelformat: plane pixel format (V4L2 4CC)
> > @@ -106,6 +130,12 @@ struct vsp1_du_atomic_pipe_config {
> >  	struct vsp1_du_writeback_config writeback;
> >  };
> >  
> > +
> > +int vsp1_du_atomic_modeset(struct device *dev, unsigned int pipe_index,
> > +		    const struct vsp1_du_modeset_config *cfg);
> > +int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
> > +		   const struct vsp1_du_enable_config *cfg);
> > +int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index);
> >  void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
> >  int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> >  			  unsigned int rpf,
> 
> With those minor issues fixed:
> 
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

-- 
Regards,

Laurent Pinchart

[PATCH v3 02/10] media: vsp1: drm: Don't configure hardware when the pipeline is disabled

[Laurent Pinchart]

The vsp1_du_atomic_flush() function calls vsp1_du_pipeline_configure()
to configure the hardware pipeline. The function is currently guaranteed
to be called with the pipeline enabled, but this will change by future
rework of the DU driver. Guard the hardware configuration to skip it
when the pipeline is disabled. The hardware will be configured the next
time the pipeline gets enabled.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 13 ++++++++++++-
 drivers/media/platform/vsp1/vsp1_drm.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 7957e1439de0..900465caf1bf 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -723,6 +723,8 @@ int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
 	/* Configure all entities in the pipeline. */
 	vsp1_du_pipeline_configure(pipe);
 
+	drm_pipe->enabled = true;
+
 unlock:
 	mutex_unlock(&vsp1->drm->lock);
 
@@ -800,6 +802,8 @@ int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
 	pipe->brx->pipe = NULL;
 	pipe->brx = NULL;
 
+	drm_pipe->enabled = false;
+
 	mutex_unlock(&vsp1->drm->lock);
 
 	vsp1_dlm_reset(pipe->output->dlm);
@@ -992,7 +996,14 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
 	}
 
 	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
-	vsp1_du_pipeline_configure(pipe);
+
+	/*
+	 * We may get called before the pipeline gets enabled, postpone
+	 * configuration in that case. vsp1_du_pipeline_configure() will be
+	 * called from vsp1_du_atomic_enable().
+	 */
+	if (drm_pipe->enabled)
+		vsp1_du_pipeline_configure(pipe);
 
 done:
 	mutex_unlock(&vsp1->drm->lock);
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
index e85ad4366fbb..d780dafc1324 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -20,6 +20,7 @@
 /**
  * vsp1_drm_pipeline - State for the API exposed to the DRM driver
  * @pipe: the VSP1 pipeline used for display
+ * @enabled: true if the pipeline is enabled
  * @width: output display width
  * @height: output display height
  * @force_brx_release: when set, release the BRx during the next reconfiguration
@@ -31,6 +32,7 @@
  */
 struct vsp1_drm_pipeline {
 	struct vsp1_pipeline pipe;
+	bool enabled;
 
 	unsigned int width;
 	unsigned int height;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/10] media: vsp1: drm: Don't configure hardware when the pipeline is disabled

[Kieran Bingham]

Hi Laurent,

On 17/06/2019 22:09, Laurent Pinchart wrote:
> The vsp1_du_atomic_flush() function calls vsp1_du_pipeline_configure()
> to configure the hardware pipeline. The function is currently guaranteed
> to be called with the pipeline enabled, but this will change by future
> rework of the DU driver. Guard the hardware configuration to skip it
> when the pipeline is disabled. The hardware will be configured the next
> time the pipeline gets enabled.

Aha, Yes, I think this now makes sense to why I was still getting hangs!

Thanks for the fix!

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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


> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 13 ++++++++++++-
>  drivers/media/platform/vsp1/vsp1_drm.h |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 7957e1439de0..900465caf1bf 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -723,6 +723,8 @@ int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
>  	/* Configure all entities in the pipeline. */
>  	vsp1_du_pipeline_configure(pipe);
>  
> +	drm_pipe->enabled = true;
> +
>  unlock:
>  	mutex_unlock(&vsp1->drm->lock);
>  
> @@ -800,6 +802,8 @@ int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
>  	pipe->brx->pipe = NULL;
>  	pipe->brx = NULL;
>  
> +	drm_pipe->enabled = false;
> +
>  	mutex_unlock(&vsp1->drm->lock);
>  
>  	vsp1_dlm_reset(pipe->output->dlm);
> @@ -992,7 +996,14 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
>  	}
>  
>  	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> -	vsp1_du_pipeline_configure(pipe);
> +
> +	/*
> +	 * We may get called before the pipeline gets enabled, postpone
> +	 * configuration in that case. vsp1_du_pipeline_configure() will be
> +	 * called from vsp1_du_atomic_enable().
> +	 */
> +	if (drm_pipe->enabled)
> +		vsp1_du_pipeline_configure(pipe);
>  
>  done:
>  	mutex_unlock(&vsp1->drm->lock);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
> index e85ad4366fbb..d780dafc1324 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -20,6 +20,7 @@
>  /**
>   * vsp1_drm_pipeline - State for the API exposed to the DRM driver
>   * @pipe: the VSP1 pipeline used for display
> + * @enabled: true if the pipeline is enabled
>   * @width: output display width
>   * @height: output display height
>   * @force_brx_release: when set, release the BRx during the next reconfiguration
> @@ -31,6 +32,7 @@
>   */
>  struct vsp1_drm_pipeline {
>  	struct vsp1_pipeline pipe;
> +	bool enabled;
>  
>  	unsigned int width;
>  	unsigned int height;
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 02/10] media: vsp1: drm: Don't configure hardware when the pipeline is disabled

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> The vsp1_du_atomic_flush() function calls vsp1_du_pipeline_configure()
> to configure the hardware pipeline. The function is currently guaranteed
> to be called with the pipeline enabled, but this will change by future
> rework of the DU driver. Guard the hardware configuration to skip it
> when the pipeline is disabled. The hardware will be configured the next
> time the pipeline gets enabled.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 13 ++++++++++++-
>  drivers/media/platform/vsp1/vsp1_drm.h |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 7957e1439de0..900465caf1bf 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -723,6 +723,8 @@ int vsp1_du_atomic_enable(struct device *dev, unsigned int pipe_index,
>  	/* Configure all entities in the pipeline. */
>  	vsp1_du_pipeline_configure(pipe);
>  
> +	drm_pipe->enabled = true;
> +
>  unlock:
>  	mutex_unlock(&vsp1->drm->lock);
>  
> @@ -800,6 +802,8 @@ int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
>  	pipe->brx->pipe = NULL;
>  	pipe->brx = NULL;
>  
> +	drm_pipe->enabled = false;
> +
>  	mutex_unlock(&vsp1->drm->lock);
>  
>  	vsp1_dlm_reset(pipe->output->dlm);
> @@ -992,7 +996,14 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
>  	}
>  
>  	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> -	vsp1_du_pipeline_configure(pipe);
> +
> +	/*
> +	 * We may get called before the pipeline gets enabled, postpone
> +	 * configuration in that case. vsp1_du_pipeline_configure() will be
> +	 * called from vsp1_du_atomic_enable().
> +	 */
> +	if (drm_pipe->enabled)
> +		vsp1_du_pipeline_configure(pipe);
>  
>  done:
>  	mutex_unlock(&vsp1->drm->lock);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
> index e85ad4366fbb..d780dafc1324 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -20,6 +20,7 @@
>  /**
>   * vsp1_drm_pipeline - State for the API exposed to the DRM driver
>   * @pipe: the VSP1 pipeline used for display
> + * @enabled: true if the pipeline is enabled
>   * @width: output display width
>   * @height: output display height
>   * @force_brx_release: when set, release the BRx during the next reconfiguration
> @@ -31,6 +32,7 @@
>   */
>  struct vsp1_drm_pipeline {
>  	struct vsp1_pipeline pipe;
> +	bool enabled;
>  
>  	unsigned int width;
>  	unsigned int height;
> -- 
> Regards,
> 
> Laurent Pinchart
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 03/10] drm: rcar-du: Convert to the new VSP atomic API

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The configuration API between the VSP and the DU has been updated to
provide finer grain control over modesetting, and enablement.

Split rcar_du_vsp_enable() into rcar_du_vsp_modeset() and
rcar_du_vsp_enable() accordingly, and update each function to use the
new VSP API.

There are no further users of the deprecated vsp1_du_setup_lif() which
can now be removed.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Minor formatting changes
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  4 +++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 20 ++++++++++++++------
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h  |  2 ++
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 2da46e3dc4ae..cccd6fe85749 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -492,8 +492,10 @@ 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->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+		rcar_du_vsp_modeset(rcrtc);
 		rcar_du_vsp_enable(rcrtc);
+	}
 
 	/* Turn vertical blanking interrupt reporting on. */
 	drm_crtc_vblank_on(&rcrtc->crtc);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 5e4faf258c31..3d007d38d7c1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -44,16 +44,14 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
 	drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
 }
 
-void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
+void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc)
 {
 	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
 	struct rcar_du_device *rcdu = crtc->dev;
-	struct vsp1_du_lif_config cfg = {
+	struct vsp1_du_modeset_config cfg = {
 		.width = mode->hdisplay,
 		.height = mode->vdisplay,
 		.interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE,
-		.callback = rcar_du_vsp_complete,
-		.callback_data = crtc,
 	};
 	struct rcar_du_plane_state state = {
 		.state = {
@@ -90,12 +88,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 	 */
 	crtc->group->need_restart = true;
 
-	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
+	vsp1_du_atomic_modeset(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
+}
+
+void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
+{
+	struct vsp1_du_enable_config cfg = {
+		.callback = rcar_du_vsp_complete,
+		.callback_data = crtc,
+	};
+
+	vsp1_du_atomic_enable(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
 }
 
 void rcar_du_vsp_disable(struct rcar_du_crtc *crtc)
 {
-	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, NULL);
+	vsp1_du_atomic_disable(crtc->vsp->vsp, crtc->vsp_pipe);
 }
 
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
index 9b4724159378..a6f6bb4690f2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
@@ -58,6 +58,7 @@ to_rcar_vsp_plane_state(struct drm_plane_state *state)
 #ifdef CONFIG_DRM_RCAR_VSP
 int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 		     unsigned int crtcs);
+void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_disable(struct rcar_du_crtc *crtc);
 void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
@@ -73,6 +74,7 @@ static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,
 {
 	return -ENXIO;
 }
+static inlinc void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
 static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 03/10] drm: rcar-du: Convert to the new VSP atomic API

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The configuration API between the VSP and the DU has been updated to
> provide finer grain control over modesetting, and enablement.
> 
> Split rcar_du_vsp_enable() into rcar_du_vsp_modeset() and
> rcar_du_vsp_enable() accordingly, and update each function to use the
> new VSP API.
> 
> There are no further users of the deprecated vsp1_du_setup_lif() which
> can now be removed.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Minor formatting changes
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  4 +++-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 20 ++++++++++++++------
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h  |  2 ++
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2da46e3dc4ae..cccd6fe85749 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -492,8 +492,10 @@ 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->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> +		rcar_du_vsp_modeset(rcrtc);
>  		rcar_du_vsp_enable(rcrtc);
> +	}
>  
>  	/* Turn vertical blanking interrupt reporting on. */
>  	drm_crtc_vblank_on(&rcrtc->crtc);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 5e4faf258c31..3d007d38d7c1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -44,16 +44,14 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
>  	drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
>  }
>  
> -void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
> +void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc)
>  {
>  	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
>  	struct rcar_du_device *rcdu = crtc->dev;
> -	struct vsp1_du_lif_config cfg = {
> +	struct vsp1_du_modeset_config cfg = {
>  		.width = mode->hdisplay,
>  		.height = mode->vdisplay,
>  		.interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE,
> -		.callback = rcar_du_vsp_complete,
> -		.callback_data = crtc,
>  	};
>  	struct rcar_du_plane_state state = {
>  		.state = {
> @@ -90,12 +88,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  	 */
>  	crtc->group->need_restart = true;
>  
> -	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> +	vsp1_du_atomic_modeset(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> +}
> +
> +void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
> +{
> +	struct vsp1_du_enable_config cfg = {
> +		.callback = rcar_du_vsp_complete,
> +		.callback_data = crtc,
> +	};
> +
> +	vsp1_du_atomic_enable(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
>  }
>  
>  void rcar_du_vsp_disable(struct rcar_du_crtc *crtc)
>  {
> -	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, NULL);
> +	vsp1_du_atomic_disable(crtc->vsp->vsp, crtc->vsp_pipe);
>  }
>  
>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> index 9b4724159378..a6f6bb4690f2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> @@ -58,6 +58,7 @@ to_rcar_vsp_plane_state(struct drm_plane_state *state)
>  #ifdef CONFIG_DRM_RCAR_VSP
>  int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
>  		     unsigned int crtcs);
> +void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_disable(struct rcar_du_crtc *crtc);
>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
> @@ -73,6 +74,7 @@ static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,
>  {
>  	return -ENXIO;
>  }
> +static inlinc void rcar_du_vsp_modeset(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
>  static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
> -- 
> Regards,
> 
> Laurent Pinchart
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 04/10] media: vsp1: drm: Remove vsp1_du_setup_lif()

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The vsp1_du_setup_lif() function is deprecated, and the users have been
removed. Remove the implementation and the associated configuration
structure.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 46 --------------------------
 include/media/vsp1.h                   | 22 ------------
 2 files changed, 68 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 900465caf1bf..3452d7a6dd89 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -815,52 +815,6 @@ int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
 }
 EXPORT_SYMBOL_GPL(vsp1_du_atomic_disable);
 
-/**
- * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
- * @dev: the VSP device
- * @pipe_index: the DRM pipeline index
- * @cfg: the LIF configuration
- *
- * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
- * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
- * source pads, and the LIF sink pad.
- *
- * The @pipe_index argument selects which DRM pipeline to setup. The number of
- * available pipelines depend on the VSP instance.
- *
- * As the media bus code on the blend unit source pad is conditioned by the
- * configuration of its sink 0 pad, we also set up the formats on all blend unit
- * sinks, even if the configuration will be overwritten later by
- * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
- * a well defined state.
- *
- * Return 0 on success or a negative error code on failure.
- */
-int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
-		      const struct vsp1_du_lif_config *cfg)
-{
-	struct vsp1_du_modeset_config modes;
-	struct vsp1_du_enable_config enable;
-	int ret;
-
-	if (!cfg)
-		return vsp1_du_atomic_disable(dev, pipe_index);
-
-	modes.width = cfg->width;
-	modes.height = cfg->height;
-	modes.interlaced = cfg->interlaced;
-
-	ret = vsp1_du_atomic_modeset(dev, pipe_index, &modes);
-	if (ret)
-		return ret;
-
-	enable.callback = cfg->callback;
-	enable.callback_data = cfg->callback_data;
-
-	return vsp1_du_atomic_enable(dev, pipe_index, &enable);
-}
-EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
-
 /**
  * vsp1_du_atomic_begin - Prepare for an atomic update
  * @dev: the VSP device
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 56643f97d4c9..b8eadd62fd15 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -20,28 +20,6 @@ int vsp1_du_init(struct device *dev);
 #define VSP1_DU_STATUS_COMPLETE		BIT(0)
 #define VSP1_DU_STATUS_WRITEBACK	BIT(1)
 
-/**
- * struct vsp1_du_lif_config - VSP LIF configuration - Deprecated
- * @width: output frame width
- * @height: output frame height
- * @interlaced: true for interlaced pipelines
- * @callback: frame completion callback function (optional). When a callback
- *	      is provided, the VSP driver guarantees that it will be called once
- *	      and only once for each vsp1_du_atomic_flush() call.
- * @callback_data: data to be passed to the frame completion callback
- */
-struct vsp1_du_lif_config {
-	unsigned int width;
-	unsigned int height;
-	bool interlaced;
-
-	void (*callback)(void *data, unsigned int status, u32 crc);
-	void *callback_data;
-};
-
-int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
-		      const struct vsp1_du_lif_config *cfg);
-
 /**
  * struct vsp1_du_modeset_config - VSP display mode configuration
  * @width: output frame width
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/10] media: vsp1: drm: Remove vsp1_du_setup_lif()

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The vsp1_du_setup_lif() function is deprecated, and the users have been
> removed. Remove the implementation and the associated configuration
> structure.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 46 --------------------------
>  include/media/vsp1.h                   | 22 ------------
>  2 files changed, 68 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 900465caf1bf..3452d7a6dd89 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -815,52 +815,6 @@ int vsp1_du_atomic_disable(struct device *dev, unsigned int pipe_index)
>  }
>  EXPORT_SYMBOL_GPL(vsp1_du_atomic_disable);
>  
> -/**
> - * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> - * @dev: the VSP device
> - * @pipe_index: the DRM pipeline index
> - * @cfg: the LIF configuration
> - *
> - * Configure the output part of VSP DRM pipeline for the given frame @cfg.width
> - * and @cfg.height. This sets up formats on the BRx source pad, the WPF sink and
> - * source pads, and the LIF sink pad.
> - *
> - * The @pipe_index argument selects which DRM pipeline to setup. The number of
> - * available pipelines depend on the VSP instance.
> - *
> - * As the media bus code on the blend unit source pad is conditioned by the
> - * configuration of its sink 0 pad, we also set up the formats on all blend unit
> - * sinks, even if the configuration will be overwritten later by
> - * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
> - * a well defined state.
> - *
> - * Return 0 on success or a negative error code on failure.
> - */
> -int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> -		      const struct vsp1_du_lif_config *cfg)
> -{
> -	struct vsp1_du_modeset_config modes;
> -	struct vsp1_du_enable_config enable;
> -	int ret;
> -
> -	if (!cfg)
> -		return vsp1_du_atomic_disable(dev, pipe_index);
> -
> -	modes.width = cfg->width;
> -	modes.height = cfg->height;
> -	modes.interlaced = cfg->interlaced;
> -
> -	ret = vsp1_du_atomic_modeset(dev, pipe_index, &modes);
> -	if (ret)
> -		return ret;
> -
> -	enable.callback = cfg->callback;
> -	enable.callback_data = cfg->callback_data;
> -
> -	return vsp1_du_atomic_enable(dev, pipe_index, &enable);
> -}
> -EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
> -
>  /**
>   * vsp1_du_atomic_begin - Prepare for an atomic update
>   * @dev: the VSP device
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 56643f97d4c9..b8eadd62fd15 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -20,28 +20,6 @@ int vsp1_du_init(struct device *dev);
>  #define VSP1_DU_STATUS_COMPLETE		BIT(0)
>  #define VSP1_DU_STATUS_WRITEBACK	BIT(1)
>  
> -/**
> - * struct vsp1_du_lif_config - VSP LIF configuration - Deprecated
> - * @width: output frame width
> - * @height: output frame height
> - * @interlaced: true for interlaced pipelines
> - * @callback: frame completion callback function (optional). When a callback
> - *	      is provided, the VSP driver guarantees that it will be called once
> - *	      and only once for each vsp1_du_atomic_flush() call.
> - * @callback_data: data to be passed to the frame completion callback
> - */
> -struct vsp1_du_lif_config {
> -	unsigned int width;
> -	unsigned int height;
> -	bool interlaced;
> -
> -	void (*callback)(void *data, unsigned int status, u32 crc);
> -	void *callback_data;
> -};
> -
> -int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
> -		      const struct vsp1_du_lif_config *cfg);
> -
>  /**
>   * struct vsp1_du_modeset_config - VSP display mode configuration
>   * @width: output frame width
> -- 
> Regards,
> 
> Laurent Pinchart
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 05/10] drm: rcar-du: Handle CRTC standby from commit tail handler

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Manage the power state, and initial configuration of the CRTC from the
commit tail handler. CRTCs which need to be activated are taken out of
standby, and any deactivated CRTCs are put into standby.

This aims at removing CRTC state tracking from the rcar_du_crtc
structure. The initial configuration of the CRTC background colours and
disabling of all planes is taken out of rcar_du_crtc_setup() and moved
inline into rcar_du_crtc_enable(). rcar_du_crtc_get() and
rcar_du_crtc_put() are kept as they are needed to configure the VSP at
the correct time, this will be addressed in a separate change.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Add more documentation
- Keep rcar_du_crtc_get() and rcar_du_crtc_put()
- Renamed rcar_du_crtc_enable() to rcar_du_crtc_exit_standby() and
  rcar_du_crtc_disable() to rcar_du_crtc_enter_standby()
- Reword commit message

Changes since v1:

- 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 | 90 ++++++++++++++++++++------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  5 ++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  4 ++
 3 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index cccd6fe85749..23f4bdef0e3a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -480,17 +480,10 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
 
 static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 {
-	/* 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));
-
 	/* Configure display timings and output routing */
 	rcar_du_crtc_set_display_timing(rcrtc);
 	rcar_du_group_set_routing(rcrtc->group);
 
-	/* 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->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
 		rcar_du_vsp_modeset(rcrtc);
@@ -501,17 +494,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_exit_standby(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;
@@ -524,8 +510,12 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_group;
 
-	rcar_du_crtc_setup(rcrtc);
-	rcrtc->initialized = true;
+	/* 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));
+
+	/* Start with all planes disabled. */
+	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
 	return 0;
 
@@ -536,13 +526,29 @@ 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_enter_standby(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_get(struct rcar_du_crtc *rcrtc)
+{
+	/*
+	 * Guard against double-get, as the function is called from both the
+	 * .atomic_enable() and .atomic_begin() handlers.
+	 */
+	if (rcrtc->initialized)
+		return;
+
+	rcar_du_crtc_setup(rcrtc);
+	rcrtc->initialized = true;
+}
+
+static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
+{
 	rcrtc->initialized = false;
 }
 
@@ -662,6 +668,54 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
+/*
+ * 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_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned int i;
+	int ret;
+
+	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) {
+			ret = rcar_du_crtc_exit_standby(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_state *crtc_state;
+	struct drm_crtc *crtc;
+	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_enter_standby(rcrtc);
+	}
+
+	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 3b7fc668996f..3ce7610793b2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -107,6 +107,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 2dc9caee8767..59680de271cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -409,11 +409,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

Re: [PATCH v3 05/10] drm: rcar-du: Handle CRTC standby from commit tail handler

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Manage the power state, and initial configuration of the CRTC from the
> commit tail handler. CRTCs which need to be activated are taken out of
> standby, and any deactivated CRTCs are put into standby.
> 
> This aims at removing CRTC state tracking from the rcar_du_crtc
> structure. The initial configuration of the CRTC background colours and
> disabling of all planes is taken out of rcar_du_crtc_setup() and moved
> inline into rcar_du_crtc_enable(). rcar_du_crtc_get() and
> rcar_du_crtc_put() are kept as they are needed to configure the VSP at
> the correct time, this will be addressed in a separate change.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Add more documentation
> - Keep rcar_du_crtc_get() and rcar_du_crtc_put()
> - Renamed rcar_du_crtc_enable() to rcar_du_crtc_exit_standby() and
>   rcar_du_crtc_disable() to rcar_du_crtc_enter_standby()
> - Reword commit message
> 
> Changes since v1:
> 
> - 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 | 90 ++++++++++++++++++++------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  5 ++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  4 ++
>  3 files changed, 81 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index cccd6fe85749..23f4bdef0e3a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -480,17 +480,10 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>  
>  static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  {
> -	/* 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));
> -
>  	/* Configure display timings and output routing */
>  	rcar_du_crtc_set_display_timing(rcrtc);
>  	rcar_du_group_set_routing(rcrtc->group);
>  
> -	/* 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->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
>  		rcar_du_vsp_modeset(rcrtc);
> @@ -501,17 +494,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_exit_standby(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;
> @@ -524,8 +510,12 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  	if (ret < 0)
>  		goto error_group;
>  
> -	rcar_du_crtc_setup(rcrtc);
> -	rcrtc->initialized = true;
> +	/* 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));
> +
> +	/* Start with all planes disabled. */
> +	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>  	return 0;
>  
> @@ -536,13 +526,29 @@ 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_enter_standby(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_get(struct rcar_du_crtc *rcrtc)
> +{
> +	/*
> +	 * Guard against double-get, as the function is called from both the
> +	 * .atomic_enable() and .atomic_begin() handlers.
> +	 */
> +	if (rcrtc->initialized)
> +		return;
> +
> +	rcar_du_crtc_setup(rcrtc);
> +	rcrtc->initialized = true;
> +}
> +
> +static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
> +{
>  	rcrtc->initialized = false;
>  }
>  
> @@ -662,6 +668,54 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> +/*
> + * 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_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +	int ret;
> +
> +	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) {
> +			ret = rcar_du_crtc_exit_standby(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_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	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_enter_standby(rcrtc);
> +	}
> +
> +	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 3b7fc668996f..3ce7610793b2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -107,6 +107,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 2dc9caee8767..59680de271cc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -409,11 +409,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
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 06/10] drm: rcar-du: Handle CRTC configuration from commit tail handler

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The CRTC mode setting and routing configuration are performed at the
earliest of atomic enable and atomic begin, to ensure that a valid
configuration is applied to the hardware before the CRTC gets enabled
and before planes are setup (the latter being required in particular by
the VSP). This requires the rcar_du_crtc structure to track the CRTC
enabled state.

Simplify the code and remove the manual state tracking by moving CRTC
setup to a new rcar_du_crtc_atomic_modeset() function, called directly
from the commit tail handler. The function iterates over all CRTCs in
the state transaction and performs CRTC mode setting, routing
configuration and VSP configuration.

drm_crtc_vblank_on() has to be called from the atomic begin handler, to
ensure that vertical blanking reporting is enabled before the call to
drm_crtc_vblank_get() in the atomic flush handler. As the
drm_crtc_vblank_on() and drm_crtc_vblank_off() calls don't need to be
balanced this is not an issue. A balanced alternative would have been to
call drm_crtc_vblank_on() and drm_crtc_vblank_off() from the CRTC exit
and enter standby handlers respectively, but
drm_atomic_helper_commit_modeset_disables() complains if
drm_crtc_vblank_off() hasn't been called by the atomic disable handler.

As a result, the vsp1_du_atomic_flush() operation can be called with the
DRM pipeline disabled. Correct operation has been ensured by "media:
vsp1: drm: Don't configure hardware when the pipeline is disabled".

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Deconstruct rcar_du_crtc_setup() completely
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 +++++++++++---------------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  1 +
 3 files changed, 42 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 23f4bdef0e3a..d11a474f6f72 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -478,22 +478,6 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
  * Start/Stop and Suspend/Resume
  */
 
-static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
-{
-	/* Configure display timings and output routing */
-	rcar_du_crtc_set_display_timing(rcrtc);
-	rcar_du_group_set_routing(rcrtc->group);
-
-	/* Enable the VSP compositor. */
-	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
-		rcar_du_vsp_modeset(rcrtc);
-		rcar_du_vsp_enable(rcrtc);
-	}
-
-	/* Turn vertical blanking interrupt reporting on. */
-	drm_crtc_vblank_on(&rcrtc->crtc);
-}
-
 static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
 {
 	int ret;
@@ -534,24 +518,6 @@ static void rcar_du_crtc_enter_standby(struct rcar_du_crtc *rcrtc)
 	clk_disable_unprepare(rcrtc->clock);
 }
 
-static void rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
-{
-	/*
-	 * Guard against double-get, as the function is called from both the
-	 * .atomic_enable() and .atomic_begin() handlers.
-	 */
-	if (rcrtc->initialized)
-		return;
-
-	rcar_du_crtc_setup(rcrtc);
-	rcrtc->initialized = true;
-}
-
-static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
-{
-	rcrtc->initialized = false;
-}
-
 static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
 {
 	bool interlaced;
@@ -716,6 +682,40 @@ int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
 	return 0;
 }
 
+/*
+ * Configure the mode for all CRTCs that require it. For now we only handle mode
+ * set on the VSP, CRTC configuration will be handled later.
+ */
+int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
+				struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned int i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+
+		/*
+		 * Skip commits that don't change the mode. We manually skip
+		 * inactive CRTCs as disabling a CRTC is considered as a mode
+		 * set by drm_atomic_crtc_needs_modeset().
+		 */
+		if (!drm_atomic_crtc_needs_modeset(crtc_state) ||
+		    !crtc_state->active)
+			continue;
+
+		/* Configure display timings and output routing. */
+		rcar_du_crtc_set_display_timing(rcrtc);
+		rcar_du_group_set_routing(rcrtc->group);
+
+		if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+			rcar_du_vsp_modeset(rcrtc);
+	}
+
+	return 0;
+}
+
 static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 				       struct drm_crtc_state *old_state)
 {
@@ -723,8 +723,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
@@ -741,6 +739,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 				     mode->clock * 1000);
 	}
 
+	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+		rcar_du_vsp_enable(rcrtc);
+
 	rcar_du_crtc_start(rcrtc);
 }
 
@@ -752,7 +753,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)) {
@@ -781,19 +781,8 @@ 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);
+	/* Turn vertical blanking interrupt reporting on. */
+	drm_crtc_vblank_on(&rcrtc->crtc);
 
 	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 3ce7610793b2..61d79aa7c8e8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -30,7 +30,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 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
@@ -51,7 +50,6 @@ struct rcar_du_crtc {
 	struct clk *extclock;
 	unsigned int mmio_offset;
 	unsigned int index;
-	bool initialized;
 
 	u32 dsysr;
 
@@ -111,6 +109,8 @@ 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);
+int rcar_du_crtc_atomic_modeset(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);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 59680de271cc..144c0c1b1591 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -412,6 +412,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	rcar_du_crtc_atomic_exit_standby(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
+	rcar_du_crtc_atomic_modeset(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);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 06/10] drm: rcar-du: Handle CRTC configuration from commit tail handler

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The CRTC mode setting and routing configuration are performed at the
> earliest of atomic enable and atomic begin, to ensure that a valid
> configuration is applied to the hardware before the CRTC gets enabled
> and before planes are setup (the latter being required in particular by
> the VSP). This requires the rcar_du_crtc structure to track the CRTC
> enabled state.
> 
> Simplify the code and remove the manual state tracking by moving CRTC
> setup to a new rcar_du_crtc_atomic_modeset() function, called directly
> from the commit tail handler. The function iterates over all CRTCs in
> the state transaction and performs CRTC mode setting, routing
> configuration and VSP configuration.
> 
> drm_crtc_vblank_on() has to be called from the atomic begin handler, to
> ensure that vertical blanking reporting is enabled before the call to
> drm_crtc_vblank_get() in the atomic flush handler. As the
> drm_crtc_vblank_on() and drm_crtc_vblank_off() calls don't need to be
> balanced this is not an issue. A balanced alternative would have been to
> call drm_crtc_vblank_on() and drm_crtc_vblank_off() from the CRTC exit
> and enter standby handlers respectively, but
> drm_atomic_helper_commit_modeset_disables() complains if
> drm_crtc_vblank_off() hasn't been called by the atomic disable handler.
> 
> As a result, the vsp1_du_atomic_flush() operation can be called with the
> DRM pipeline disabled. Correct operation has been ensured by "media:
> vsp1: drm: Don't configure hardware when the pipeline is disabled".
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Deconstruct rcar_du_crtc_setup() completely
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 89 +++++++++++---------------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  1 +
>  3 files changed, 42 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 23f4bdef0e3a..d11a474f6f72 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -478,22 +478,6 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>   * Start/Stop and Suspend/Resume
>   */
>  
> -static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
> -{
> -	/* Configure display timings and output routing */
> -	rcar_du_crtc_set_display_timing(rcrtc);
> -	rcar_du_group_set_routing(rcrtc->group);
> -
> -	/* Enable the VSP compositor. */
> -	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> -		rcar_du_vsp_modeset(rcrtc);
> -		rcar_du_vsp_enable(rcrtc);
> -	}
> -
> -	/* Turn vertical blanking interrupt reporting on. */
> -	drm_crtc_vblank_on(&rcrtc->crtc);
> -}
> -
>  static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
>  {
>  	int ret;
> @@ -534,24 +518,6 @@ static void rcar_du_crtc_enter_standby(struct rcar_du_crtc *rcrtc)
>  	clk_disable_unprepare(rcrtc->clock);
>  }
>  
> -static void rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
> -{
> -	/*
> -	 * Guard against double-get, as the function is called from both the
> -	 * .atomic_enable() and .atomic_begin() handlers.
> -	 */
> -	if (rcrtc->initialized)
> -		return;
> -
> -	rcar_du_crtc_setup(rcrtc);
> -	rcrtc->initialized = true;
> -}
> -
> -static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
> -{
> -	rcrtc->initialized = false;
> -}
> -
>  static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>  {
>  	bool interlaced;
> @@ -716,6 +682,40 @@ int rcar_du_crtc_atomic_enter_standby(struct drm_device *dev,
>  	return 0;
>  }
>  
> +/*
> + * Configure the mode for all CRTCs that require it. For now we only handle mode
> + * set on the VSP, CRTC configuration will be handled later.
> + */
> +int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
> +				struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +		/*
> +		 * Skip commits that don't change the mode. We manually skip
> +		 * inactive CRTCs as disabling a CRTC is considered as a mode
> +		 * set by drm_atomic_crtc_needs_modeset().
> +		 */
> +		if (!drm_atomic_crtc_needs_modeset(crtc_state) ||
> +		    !crtc_state->active)
> +			continue;
> +
> +		/* Configure display timings and output routing. */
> +		rcar_du_crtc_set_display_timing(rcrtc);
> +		rcar_du_group_set_routing(rcrtc->group);
> +
> +		if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +			rcar_du_vsp_modeset(rcrtc);
> +	}
> +
> +	return 0;
> +}
> +
>  static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  				       struct drm_crtc_state *old_state)
>  {
> @@ -723,8 +723,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
> @@ -741,6 +739,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  				     mode->clock * 1000);
>  	}
>  
> +	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +		rcar_du_vsp_enable(rcrtc);
> +
>  	rcar_du_crtc_start(rcrtc);
>  }
>  
> @@ -752,7 +753,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)) {
> @@ -781,19 +781,8 @@ 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);
> +	/* Turn vertical blanking interrupt reporting on. */
> +	drm_crtc_vblank_on(&rcrtc->crtc);
>  
>  	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 3ce7610793b2..61d79aa7c8e8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -30,7 +30,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 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
> @@ -51,7 +50,6 @@ struct rcar_du_crtc {
>  	struct clk *extclock;
>  	unsigned int mmio_offset;
>  	unsigned int index;
> -	bool initialized;
>  
>  	u32 dsysr;
>  
> @@ -111,6 +109,8 @@ 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);
> +int rcar_du_crtc_atomic_modeset(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);
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 59680de271cc..144c0c1b1591 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -412,6 +412,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	rcar_du_crtc_atomic_exit_standby(dev, old_state);
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +	rcar_du_crtc_atomic_modeset(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);
> -- 
> Regards,
> 
> Laurent Pinchart
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 07/10] drm: rcar-du: Provide for_each_group helper

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Don't assign __group in the condition part of the for statement of the
  for_each_rcdu_group() macro
---
 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..0cc0984bf2ea 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[0]; \
+	     (__i) < DIV_ROUND_UP((__rcdu)->num_crtcs, 2); \
+	     __i++, __group++)
+
 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 144c0c1b1591..c04136674e58 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -627,9 +627,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;
@@ -670,11 +670,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;
@@ -711,8 +707,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

Re: [PATCH v3 07/10] drm: rcar-du: Provide for_each_group helper

[Kieran Bingham]

Hi Laurent,

On 17/06/2019 22:09, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Don't assign __group in the condition part of the for statement of the
>   for_each_rcdu_group() macro

Nice!.

> ---
>  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..0cc0984bf2ea 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[0]; \
> +	     (__i) < DIV_ROUND_UP((__rcdu)->num_crtcs, 2); \
> +	     __i++, __group++)


Aha - studying the difference here, I do like this better.
--
KB.


> +
>  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 144c0c1b1591..c04136674e58 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -627,9 +627,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;
> @@ -670,11 +670,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;
> @@ -711,8 +707,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
--
Kieran

Re: [PATCH v3 07/10] drm: rcar-du: Provide for_each_group helper

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Don't assign __group in the condition part of the for statement of the
>   for_each_rcdu_group() macro
> ---
>  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..0cc0984bf2ea 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[0]; \
> +	     (__i) < DIV_ROUND_UP((__rcdu)->num_crtcs, 2); \
> +	     __i++, __group++)
> +
>  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 144c0c1b1591..c04136674e58 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -627,9 +627,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;
> @@ -670,11 +670,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;
> @@ -711,8 +707,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
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 08/10] drm: rcar-du: Create a group state object

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Call mutex_destroy() when cleaning up the group
- Include mutex.h and slab.h
- Squash "drm: rcar-du: Add rcar_du_get_{old,new}_group_state()"
---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 144 ++++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  29 +++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  27 +----
 3 files changed, 177 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..8e12bd42890e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -25,6 +25,11 @@
 
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/slab.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"
@@ -351,3 +356,142 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
 
 	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
 }
+
+/* -----------------------------------------------------------------------------
+ * Group State Handling
+ */
+
+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_get_old_group_state - get old group state, if it exists
+ * @state: global atomic state object
+ * @rgrp: group to grab
+ *
+ * This function returns the old group state for the given group, or
+ * NULL if the group is not part of the global atomic state.
+ */
+struct rcar_du_group_state *
+rcar_du_get_old_group_state(struct drm_atomic_state *state,
+			    struct rcar_du_group *rgrp)
+{
+	struct drm_private_obj *obj = &rgrp->private;
+	struct drm_private_state *pstate;
+	unsigned int i;
+
+	for (i = 0; i < state->num_private_objs; i++) {
+		if (obj == state->private_objs[i].ptr) {
+			pstate = state->private_objs[i].old_state;
+			return to_rcar_group_state(pstate);
+		}
+	}
+
+	return NULL;
+}
+
+/**
+ * rcar_du_get_new_group_state - get new group state, if it exists
+ * @state: global atomic state object
+ * @rgrp: group to grab
+ *
+ * This function returns the new group state for the given group, or
+ * NULL if the group is not part of the global atomic state.
+ */
+struct rcar_du_group_state *
+rcar_du_get_new_group_state(struct drm_atomic_state *state,
+			    struct rcar_du_group *rgrp)
+{
+	struct drm_private_obj *obj = &rgrp->private;
+	struct drm_private_state *pstate;
+	unsigned int i;
+
+	for (i = 0; i < state->num_private_objs; i++) {
+		if (obj == state->private_objs[i].ptr) {
+			pstate = state->private_objs[i].new_state;
+			return to_rcar_group_state(pstate);
+		}
+	}
+
+	return NULL;
+}
+
+/* -----------------------------------------------------------------------------
+ * Init and Cleanup
+ */
+
+/*
+ * 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)
+{
+	mutex_destroy(&rgrp->lock);
+
+	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..f9961f89fd97 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,15 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
 
 int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
 
+struct rcar_du_group_state *
+rcar_du_get_old_group_state(struct drm_atomic_state *state,
+			    struct rcar_du_group *rgrp);
+struct rcar_du_group_state *
+rcar_du_get_new_group_state(struct drm_atomic_state *state,
+			    struct rcar_du_group *rgrp);
+
+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 c04136674e58..f57a035a94ee 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"
@@ -621,10 +622,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;
@@ -671,25 +668,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

Re: [PATCH v3 08/10] drm: rcar-du: Create a group state object

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Call mutex_destroy() when cleaning up the group
> - Include mutex.h and slab.h
> - Squash "drm: rcar-du: Add rcar_du_get_{old,new}_group_state()"
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 144 ++++++++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  29 +++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  27 +----
>  3 files changed, 177 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..8e12bd42890e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -25,6 +25,11 @@
>  
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/slab.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"
> @@ -351,3 +356,142 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  
>  	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
>  }
> +
> +/* -----------------------------------------------------------------------------
> + * Group State Handling
> + */
> +
> +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_get_old_group_state - get old group state, if it exists
> + * @state: global atomic state object
> + * @rgrp: group to grab
> + *
> + * This function returns the old group state for the given group, or
> + * NULL if the group is not part of the global atomic state.
> + */
> +struct rcar_du_group_state *
> +rcar_du_get_old_group_state(struct drm_atomic_state *state,
> +			    struct rcar_du_group *rgrp)
> +{
> +	struct drm_private_obj *obj = &rgrp->private;
> +	struct drm_private_state *pstate;
> +	unsigned int i;
> +
> +	for (i = 0; i < state->num_private_objs; i++) {
> +		if (obj == state->private_objs[i].ptr) {
> +			pstate = state->private_objs[i].old_state;
> +			return to_rcar_group_state(pstate);
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * rcar_du_get_new_group_state - get new group state, if it exists
> + * @state: global atomic state object
> + * @rgrp: group to grab
> + *
> + * This function returns the new group state for the given group, or
> + * NULL if the group is not part of the global atomic state.
> + */
> +struct rcar_du_group_state *
> +rcar_du_get_new_group_state(struct drm_atomic_state *state,
> +			    struct rcar_du_group *rgrp)
> +{
> +	struct drm_private_obj *obj = &rgrp->private;
> +	struct drm_private_state *pstate;
> +	unsigned int i;
> +
> +	for (i = 0; i < state->num_private_objs; i++) {
> +		if (obj == state->private_objs[i].ptr) {
> +			pstate = state->private_objs[i].new_state;
> +			return to_rcar_group_state(pstate);
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Init and Cleanup
> + */
> +
> +/*
> + * 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)
> +{
> +	mutex_destroy(&rgrp->lock);
> +
> +	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..f9961f89fd97 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,15 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
>  
>  int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
>  
> +struct rcar_du_group_state *
> +rcar_du_get_old_group_state(struct drm_atomic_state *state,
> +			    struct rcar_du_group *rgrp);
> +struct rcar_du_group_state *
> +rcar_du_get_new_group_state(struct drm_atomic_state *state,
> +			    struct rcar_du_group *rgrp);
> +
> +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 c04136674e58..f57a035a94ee 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"
> @@ -621,10 +622,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;
> @@ -671,25 +668,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
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 09/10] drm: rcar-du: Perform group setup from the atomic tail handler

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Create rcar_du_group_atomic_check() and rcar_du_group_atomic_setup()
functions to track and apply group state through the DRM atomic state.
The use_count field is moved from the rcar_du_group structure to an
enabled field in the rcar_du_group_state structure.

This allows separating group setup from the configuration of the CRTCs
that are part of the group, simplifying the CRTC code and improving
overall readability. The existing rcar_du_group_{get,put}() functions
are now redundant and removed.

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Simplify error handling in rcar_du_crtc_enable()
- Rename rcar_du_group_atomic_pre_commit() to
  rcar_du_group_atomic_setup() and turn it into a void function
- Remove rcar_du_group_atomic_post_commit()
- Replace group state use_count field by enabled
- Rename group state variable from rstate to gstate

Changes since v1:

- 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  | 18 ++---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 91 ++++++++++++++++---------
 drivers/gpu/drm/rcar-du/rcar_du_group.h | 12 ++--
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 ++
 4 files changed, 76 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index d11a474f6f72..ab5c288f9d09 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -487,12 +487,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
 		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;
+	if (ret < 0) {
+		clk_disable_unprepare(rcrtc->clock);
+		return ret;
+	}
 
 	/* Set display off and background to black. */
 	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
@@ -502,18 +500,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
 	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
 	return 0;
-
-error_group:
-	clk_disable_unprepare(rcrtc->extclock);
-error_clock:
-	clk_disable_unprepare(rcrtc->clock);
-	return ret;
 }
 
 static void rcar_du_crtc_enter_standby(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 8e12bd42890e..7c9145778567 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 <linux/slab.h>
 
@@ -173,38 +174,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;
@@ -389,6 +358,23 @@ 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);
+}
+
 /**
  * rcar_du_get_old_group_state - get old group state, if it exists
  * @state: global atomic state object
@@ -441,6 +427,47 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
 	return NULL;
 }
 
+int rcar_du_group_atomic_check(struct drm_device *dev,
+			       struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	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 *gstate;
+
+		gstate = rcar_du_get_group_state(state, rcrtc->group);
+		if (IS_ERR(gstate))
+			return PTR_ERR(gstate);
+
+		if (crtc_state->active)
+			gstate->enabled = true;
+	}
+
+	return 0;
+}
+
+void rcar_du_group_atomic_setup(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->enabled && new_state->enabled)
+			rcar_du_group_setup(rgrp);
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * Init and Cleanup
  */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index f9961f89fd97..20efd2251ec4 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
+ * @enabled: true if at least one CRTC in the group is enabled
  */
 struct rcar_du_group_state {
 	struct drm_private_state state;
+
+	bool enabled;
 };
 
 #define to_rcar_group_state(s) \
@@ -70,8 +71,6 @@ 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);
@@ -85,6 +84,11 @@ struct rcar_du_group_state *
 rcar_du_get_new_group_state(struct drm_atomic_state *state,
 			    struct rcar_du_group *rgrp);
 
+int rcar_du_group_atomic_check(struct drm_device *dev,
+			       struct drm_atomic_state *state);
+void rcar_du_group_atomic_setup(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 f57a035a94ee..65396134fba1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -377,6 +377,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;
 
@@ -411,6 +415,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_setup(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 	rcar_du_crtc_atomic_modeset(dev, old_state);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 09/10] drm: rcar-du: Perform group setup from the atomic tail handler

[Ulrich Hecht]

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Create rcar_du_group_atomic_check() and rcar_du_group_atomic_setup()
> functions to track and apply group state through the DRM atomic state.
> The use_count field is moved from the rcar_du_group structure to an
> enabled field in the rcar_du_group_state structure.
> 
> This allows separating group setup from the configuration of the CRTCs
> that are part of the group, simplifying the CRTC code and improving
> overall readability. The existing rcar_du_group_{get,put}() functions
> are now redundant and removed.
> 
> 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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Simplify error handling in rcar_du_crtc_enable()
> - Rename rcar_du_group_atomic_pre_commit() to
>   rcar_du_group_atomic_setup() and turn it into a void function
> - Remove rcar_du_group_atomic_post_commit()
> - Replace group state use_count field by enabled
> - Rename group state variable from rstate to gstate
> 
> Changes since v1:
> 
> - 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  | 18 ++---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 91 ++++++++++++++++---------
>  drivers/gpu/drm/rcar-du/rcar_du_group.h | 12 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 ++
>  4 files changed, 76 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index d11a474f6f72..ab5c288f9d09 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -487,12 +487,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
>  		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;
> +	if (ret < 0) {
> +		clk_disable_unprepare(rcrtc->clock);
> +		return ret;
> +	}
>  
>  	/* Set display off and background to black. */
>  	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
> @@ -502,18 +500,10 @@ static int rcar_du_crtc_exit_standby(struct rcar_du_crtc *rcrtc)
>  	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>  	return 0;
> -
> -error_group:
> -	clk_disable_unprepare(rcrtc->extclock);
> -error_clock:
> -	clk_disable_unprepare(rcrtc->clock);
> -	return ret;
>  }
>  
>  static void rcar_du_crtc_enter_standby(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 8e12bd42890e..7c9145778567 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 <linux/slab.h>
>  
> @@ -173,38 +174,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;
> @@ -389,6 +358,23 @@ 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);
> +}
> +
>  /**
>   * rcar_du_get_old_group_state - get old group state, if it exists
>   * @state: global atomic state object
> @@ -441,6 +427,47 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
>  	return NULL;
>  }
>  
> +int rcar_du_group_atomic_check(struct drm_device *dev,
> +			       struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	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 *gstate;
> +
> +		gstate = rcar_du_get_group_state(state, rcrtc->group);
> +		if (IS_ERR(gstate))
> +			return PTR_ERR(gstate);
> +
> +		if (crtc_state->active)
> +			gstate->enabled = true;
> +	}
> +
> +	return 0;
> +}
> +
> +void rcar_du_group_atomic_setup(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->enabled && new_state->enabled)
> +			rcar_du_group_setup(rgrp);
> +	}
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Init and Cleanup
>   */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index f9961f89fd97..20efd2251ec4 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
> + * @enabled: true if at least one CRTC in the group is enabled
>   */
>  struct rcar_du_group_state {
>  	struct drm_private_state state;
> +
> +	bool enabled;
>  };
>  
>  #define to_rcar_group_state(s) \
> @@ -70,8 +71,6 @@ 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);
> @@ -85,6 +84,11 @@ struct rcar_du_group_state *
>  rcar_du_get_new_group_state(struct drm_atomic_state *state,
>  			    struct rcar_du_group *rgrp);
>  
> +int rcar_du_group_atomic_check(struct drm_device *dev,
> +			       struct drm_atomic_state *state);
> +void rcar_du_group_atomic_setup(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 f57a035a94ee..65396134fba1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -377,6 +377,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;
>  
> @@ -411,6 +415,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_setup(dev, old_state);
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  	rcar_du_crtc_atomic_modeset(dev, old_state);
> -- 
> Regards,
> 
> Laurent Pinchart
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

[PATCH v3 10/10] drm: rcar-du: Centralise routing configuration in commit tail handler

[Laurent Pinchart]

Routing configuration for the DU is complex. Depending on the SoC
generation various routing options are available:

- The VSP to DU routing is not available on Gen1, is configurable on
  Gen2 and is fixed on Gen3. When configurable, the routing affects both
  CRTC groups but is set in a register of the first CRTC group.
- The DU channel to DPAD output routing is explicitly configurable on
  some SoCs of the Gen2 and Gen3 family. When configurable, the DPAD
  outputs never offer routing options to CRTCs belonging to different
  groups.
- On all SoCs the routing of DU channels to DU pin controllers (internal
  output of the DU channels) can be swapped within a group. This feature
  is only used on Gen1 to control routing of the DPAD1 output.

Routing is thus handled at the group level, but for Gen2 hardware
requires configuration of the DPAD1 and VSPD1 routing in the first group
even when only the second group is enabled.

Routing at the group level is currently configured when applying CRTC
configuration. Global routing is configured at the same time, and is
additionally configured by the plane setup code to set VSPD1 routing.
This results in code paths that are difficult to follow.

Simplify the routing configuration by performing it all directly, based
on CRTC and CRTC group state. Group-level routing is moved to group
setup as it only depends on the group state and the state of the CRTCs
it contains. Global routing is moved to the commit tail handler, and
based on global DU state.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |   3 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   1 -
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 154 ++++++++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_group.h |   3 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  16 +--
 drivers/gpu/drm/rcar-du/rcar_du_plane.c |  10 +-
 6 files changed, 115 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index ab5c288f9d09..f6ea19674a31 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -695,9 +695,8 @@ int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
 		    !crtc_state->active)
 			continue;
 
-		/* Configure display timings and output routing. */
+		/* Configure display timings. */
 		rcar_du_crtc_set_display_timing(rcrtc);
-		rcar_du_group_set_routing(rcrtc->group);
 
 		if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 			rcar_du_vsp_modeset(rcrtc);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 0cc0984bf2ea..4e3a12496098 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -92,7 +92,6 @@ struct rcar_du_device {
 	} props;
 
 	unsigned int dpad0_source;
-	unsigned int dpad1_source;
 	unsigned int vspd1_sink;
 };
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 7c9145778567..261baaf7c1f5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -46,6 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
 	rcar_du_write(rgrp->dev, rgrp->mmio_offset + reg, data);
 }
 
+/* -----------------------------------------------------------------------------
+ * Static Group Setup
+ */
+
 static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
 {
 	u32 defr6 = DEFR6_CODE;
@@ -59,37 +63,6 @@ static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
 	rcar_du_group_write(rgrp, DEFR6, defr6);
 }
 
-static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
-{
-	struct rcar_du_device *rcdu = rgrp->dev;
-	u32 defr8 = DEFR8_CODE;
-
-	if (rcdu->info->gen < 3) {
-		defr8 |= DEFR8_DEFE8;
-
-		/*
-		 * On Gen2 the DEFR8 register for the first group also controls
-		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
-		 * DU instances that support it.
-		 */
-		if (rgrp->index == 0) {
-			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
-			if (rgrp->dev->vspd1_sink == 2)
-				defr8 |= DEFR8_VSCS;
-		}
-	} else {
-		/*
-		 * On Gen3 VSPD routing can't be configured, and DPAD routing
-		 * is set in the group corresponding to the DPAD output (no Gen3
-		 * SoC has multiple DPAD sources belonging to separate groups).
-		 */
-		if (rgrp->index == rcdu->dpad0_source / 2)
-			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
-	}
-
-	rcar_du_group_write(rgrp, DEFR8, defr8);
-}
-
 static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
@@ -153,10 +126,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 
 	rcar_du_group_setup_pins(rgrp);
 
-	if (rcdu->info->gen >= 2) {
-		rcar_du_group_setup_defr8(rgrp);
+	if (rcdu->info->gen >= 2)
 		rcar_du_group_setup_didsr(rgrp);
-	}
 
 	if (rcdu->info->gen >= 3)
 		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
@@ -174,6 +145,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 	mutex_unlock(&rgrp->lock);
 }
 
+/* -----------------------------------------------------------------------------
+ * Start & Stop
+ */
+
 static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
@@ -229,26 +204,63 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp)
 	__rcar_du_group_start_stop(rgrp, true);
 }
 
+/* -----------------------------------------------------------------------------
+ * Input and Output Routing
+ */
+
+static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
+{
+	struct rcar_du_device *rcdu = rgrp->dev;
+	u32 defr8 = DEFR8_CODE;
+
+	if (rcdu->info->gen < 3) {
+		defr8 |= DEFR8_DEFE8;
+
+		/*
+		 * On Gen2 the DEFR8 register for the first group also controls
+		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
+		 * DU instances that support it.
+		 */
+		if (rgrp->index == 0) {
+			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
+			if (rgrp->dev->vspd1_sink == 2)
+				defr8 |= DEFR8_VSCS;
+		}
+	} else {
+		/*
+		 * On Gen3 VSPD routing can't be configured, and DPAD routing
+		 * is set in the group corresponding to the DPAD output (no Gen3
+		 * SoC has multiple DPAD sources belonging to separate groups).
+		 */
+		if (rgrp->index == rcdu->dpad0_source / 2)
+			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
+	}
+
+	rcar_du_group_write(rgrp, DEFR8, defr8);
+}
+
 int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)
 {
 	struct rcar_du_group *rgrp;
 	struct rcar_du_crtc *crtc;
-	unsigned int index;
 	int ret;
 
-	if (rcdu->info->gen < 2)
+	/*
+	 * Only Gen2 hardware has global routing not handled in the group that
+	 * holds the corresponding CRTCs.
+	 */
+	if (rcdu->info->gen != 2)
 		return 0;
 
 	/*
 	 * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are
-	 * configured in the DEFR8 register of the first group on Gen2 and the
-	 * last group on Gen3. As this function can be called with the DU
-	 * channels of the corresponding CRTCs disabled, we need to enable the
-	 * group clock before accessing the register.
+	 * configured in the DEFR8 register of the first group on Gen2. As this
+	 * function can be called with the DU channels of the corresponding
+	 * CRTCs disabled, we need to enable the group clock before accessing
+	 * the register.
 	 */
-	index = rcdu->info->gen < 3 ? 0 : DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1;
-	rgrp = &rcdu->groups[index];
-	crtc = &rcdu->crtcs[index * 2];
+	rgrp = &rcdu->groups[0];
+	crtc = &rcdu->crtcs[0];
 
 	ret = clk_prepare_enable(crtc->clock);
 	if (ret < 0)
@@ -302,19 +314,33 @@ static void rcar_du_group_set_dpad_levels(struct rcar_du_group *rgrp)
 	rcar_du_group_write(rgrp, DOFLR, doflr);
 }
 
-int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
+static void rcar_du_group_set_routing(struct rcar_du_group *rgrp)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
 	u32 dorcr = rcar_du_group_read(rgrp, DORCR);
+	bool sp1_to_pin2 = false;
 
 	dorcr &= ~(DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_MASK);
 
 	/*
-	 * Set the DPAD1 pins sources. Select CRTC 0 if explicitly requested and
-	 * CRTC 1 in all other cases to avoid cloning CRTC 0 to DPAD0 and DPAD1
-	 * by default.
+	 * Configure the superposition processor to pin controller routing.
+	 * Hardcode the assignment, except on Gen1 where we use it to route the
+	 * DU channels to DPAD1. There we route CRTC 0 to DPAD1 if explicitly
+	 * requested, and CRTC 1 in all other cases to avoid cloning CRTC 0 to
+	 * DPAD0 and DPAD1 by default.
 	 */
-	if (rcdu->dpad1_source == rgrp->index * 2)
+	if (rcdu->info->gen == 1 && rgrp->index == 0) {
+		struct rcar_du_crtc_state *rstate;
+		struct rcar_du_crtc *rcrtc;
+
+		rcrtc = &rcdu->crtcs[0];
+		rstate = to_rcar_crtc_state(rcrtc->crtc.state);
+
+		if (rstate->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
+			sp1_to_pin2 = true;
+	}
+
+	if (sp1_to_pin2)
 		dorcr |= DORCR_PG2D_DS1;
 	else
 		dorcr |= DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_DS2;
@@ -323,7 +349,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
 
 	rcar_du_group_set_dpad_levels(rgrp);
 
-	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
+	rcar_du_group_setup_defr8(rgrp);
 }
 
 /* -----------------------------------------------------------------------------
@@ -430,20 +456,36 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
 int rcar_du_group_atomic_check(struct drm_device *dev,
 			       struct drm_atomic_state *state)
 {
-	struct drm_crtc_state *crtc_state;
+	static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
+				   | BIT(RCAR_DU_OUTPUT_DPAD0);
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+		struct rcar_du_crtc_state *old_rcrtc_state;
+		struct rcar_du_crtc_state *new_rcrtc_state;
 		struct rcar_du_group_state *gstate;
 
 		gstate = rcar_du_get_group_state(state, rcrtc->group);
 		if (IS_ERR(gstate))
 			return PTR_ERR(gstate);
 
-		if (crtc_state->active)
+		if (new_crtc_state->active)
 			gstate->enabled = true;
+
+		if (!new_crtc_state->active_changed &&
+		    !new_crtc_state->connectors_changed)
+			continue;
+
+		old_rcrtc_state = to_rcar_crtc_state(old_crtc_state);
+		new_rcrtc_state = to_rcar_crtc_state(new_crtc_state);
+
+		if ((old_rcrtc_state->outputs & dpad_mask) !=
+		    (new_rcrtc_state->outputs & dpad_mask))
+			gstate->dpad_routing_changed = true;
 	}
 
 	return 0;
@@ -463,8 +505,14 @@ void rcar_du_group_atomic_setup(struct drm_device *dev,
 		old_state = to_rcar_group_state(old_pstate);
 		new_state = to_rcar_group_state(new_pstate);
 
-		if (!old_state->enabled && new_state->enabled)
+		if (!new_state->enabled)
+			continue;
+
+		if (!old_state->enabled)
 			rcar_du_group_setup(rgrp);
+
+		if (!old_state->enabled || new_state->dpad_routing_changed)
+			rcar_du_group_set_routing(rgrp);
 	}
 }
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 20efd2251ec4..b31b3bf8cb94 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -58,11 +58,13 @@ struct rcar_du_group {
  * struct rcar_du_group_state - Driver-specific group state
  * @state: base DRM private state
  * @enabled: true if at least one CRTC in the group is enabled
+ * @dpad_routing_changed: set if CRTC to DPAD output routing has changed
  */
 struct rcar_du_group_state {
 	struct drm_private_state state;
 
 	bool enabled;
+	bool dpad_routing_changed;
 };
 
 #define to_rcar_group_state(s) \
@@ -73,7 +75,6 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
 
 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);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 65396134fba1..778060bfd383 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -393,14 +393,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	struct rcar_du_device *rcdu = dev->dev_private;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
+	unsigned int vspd1_sink = rcdu->vspd1_sink;
+	unsigned int dpad0_source = rcdu->dpad0_source;
 	unsigned int i;
 
 	/*
-	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
-	 * when starting the CRTCs.
+	 * Store RGB routing to DPAD0, the hardware will be configured when
+	 * setting up the groups.
 	 */
-	rcdu->dpad1_source = -1;
-
 	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
 		struct rcar_du_crtc_state *rcrtc_state =
 			to_rcar_crtc_state(crtc_state);
@@ -408,9 +408,6 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD0))
 			rcdu->dpad0_source = rcrtc->index;
-
-		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
-			rcdu->dpad1_source = rcrtc->index;
 	}
 
 	/* Apply the atomic update. */
@@ -421,6 +418,11 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	rcar_du_crtc_atomic_modeset(dev, old_state);
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
+
+	if (rcdu->vspd1_sink != vspd1_sink ||
+	    rcdu->dpad0_source != dpad0_source)
+		rcar_du_set_dpad0_vsp1_routing(rcdu);
+
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	rcar_du_crtc_atomic_enter_standby(dev, old_state);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index c6430027169f..9bf32585dab3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -552,14 +552,8 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
 	if (rcdu->info->gen < 3)
 		rcar_du_plane_setup_scanout(rgrp, state);
 
-	if (state->source == RCAR_DU_PLANE_VSPD1) {
-		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
-
-		if (rcdu->vspd1_sink != vspd1_sink) {
-			rcdu->vspd1_sink = vspd1_sink;
-			rcar_du_set_dpad0_vsp1_routing(rcdu);
-		}
-	}
+	if (state->source == RCAR_DU_PLANE_VSPD1)
+		rcdu->vspd1_sink = rgrp->index ? 2 : 0;
 }
 
 int __rcar_du_plane_atomic_check(struct drm_plane *plane,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 10/10] drm: rcar-du: Centralise routing configuration in commit tail handler

[Ulrich Hecht]

Thank you for your patch.

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote:
> 
> 
> Routing configuration for the DU is complex. Depending on the SoC
> generation various routing options are available:
> 
> - The VSP to DU routing is not available on Gen1, is configurable on
>   Gen2 and is fixed on Gen3. When configurable, the routing affects both
>   CRTC groups but is set in a register of the first CRTC group.
> - The DU channel to DPAD output routing is explicitly configurable on
>   some SoCs of the Gen2 and Gen3 family. When configurable, the DPAD
>   outputs never offer routing options to CRTCs belonging to different
>   groups.
> - On all SoCs the routing of DU channels to DU pin controllers (internal
>   output of the DU channels) can be swapped within a group. This feature
>   is only used on Gen1 to control routing of the DPAD1 output.
> 
> Routing is thus handled at the group level, but for Gen2 hardware
> requires configuration of the DPAD1 and VSPD1 routing in the first group
> even when only the second group is enabled.

That's "DPAD0 and VSPD1", isn't it?

> 
> Routing at the group level is currently configured when applying CRTC
> configuration. Global routing is configured at the same time, and is
> additionally configured by the plane setup code to set VSPD1 routing.
> This results in code paths that are difficult to follow.
> 
> Simplify the routing configuration by performing it all directly, based
> on CRTC and CRTC group state. Group-level routing is moved to group
> setup as it only depends on the group state and the state of the CRTCs
> it contains. Global routing is moved to the commit tail handler, and
> based on global DU state.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |   3 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   1 -
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 154 ++++++++++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |   3 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  16 +--
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c |  10 +-
>  6 files changed, 115 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index ab5c288f9d09..f6ea19674a31 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -695,9 +695,8 @@ int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
>  		    !crtc_state->active)
>  			continue;
>  
> -		/* Configure display timings and output routing. */
> +		/* Configure display timings. */
>  		rcar_du_crtc_set_display_timing(rcrtc);
> -		rcar_du_group_set_routing(rcrtc->group);
>  
>  		if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  			rcar_du_vsp_modeset(rcrtc);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 0cc0984bf2ea..4e3a12496098 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -92,7 +92,6 @@ struct rcar_du_device {
>  	} props;
>  
>  	unsigned int dpad0_source;
> -	unsigned int dpad1_source;
>  	unsigned int vspd1_sink;
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 7c9145778567..261baaf7c1f5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -46,6 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
>  	rcar_du_write(rgrp->dev, rgrp->mmio_offset + reg, data);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Static Group Setup
> + */
> +
>  static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>  {
>  	u32 defr6 = DEFR6_CODE;
> @@ -59,37 +63,6 @@ static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>  	rcar_du_group_write(rgrp, DEFR6, defr6);
>  }
>  
> -static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> -{
> -	struct rcar_du_device *rcdu = rgrp->dev;
> -	u32 defr8 = DEFR8_CODE;
> -
> -	if (rcdu->info->gen < 3) {
> -		defr8 |= DEFR8_DEFE8;
> -
> -		/*
> -		 * On Gen2 the DEFR8 register for the first group also controls
> -		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> -		 * DU instances that support it.
> -		 */
> -		if (rgrp->index == 0) {
> -			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> -			if (rgrp->dev->vspd1_sink == 2)
> -				defr8 |= DEFR8_VSCS;
> -		}
> -	} else {
> -		/*
> -		 * On Gen3 VSPD routing can't be configured, and DPAD routing
> -		 * is set in the group corresponding to the DPAD output (no Gen3
> -		 * SoC has multiple DPAD sources belonging to separate groups).
> -		 */
> -		if (rgrp->index == rcdu->dpad0_source / 2)
> -			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> -	}
> -
> -	rcar_du_group_write(rgrp, DEFR8, defr8);
> -}
> -
>  static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
> @@ -153,10 +126,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  
>  	rcar_du_group_setup_pins(rgrp);
>  
> -	if (rcdu->info->gen >= 2) {
> -		rcar_du_group_setup_defr8(rgrp);
> +	if (rcdu->info->gen >= 2)
>  		rcar_du_group_setup_didsr(rgrp);
> -	}
>  
>  	if (rcdu->info->gen >= 3)
>  		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
> @@ -174,6 +145,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  	mutex_unlock(&rgrp->lock);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Start & Stop
> + */
> +
>  static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
> @@ -229,26 +204,63 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp)
>  	__rcar_du_group_start_stop(rgrp, true);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Input and Output Routing
> + */
> +
> +static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> +{
> +	struct rcar_du_device *rcdu = rgrp->dev;
> +	u32 defr8 = DEFR8_CODE;
> +
> +	if (rcdu->info->gen < 3) {
> +		defr8 |= DEFR8_DEFE8;
> +
> +		/*
> +		 * On Gen2 the DEFR8 register for the first group also controls
> +		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> +		 * DU instances that support it.
> +		 */
> +		if (rgrp->index == 0) {
> +			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> +			if (rgrp->dev->vspd1_sink == 2)
> +				defr8 |= DEFR8_VSCS;
> +		}
> +	} else {
> +		/*
> +		 * On Gen3 VSPD routing can't be configured, and DPAD routing
> +		 * is set in the group corresponding to the DPAD output (no Gen3
> +		 * SoC has multiple DPAD sources belonging to separate groups).
> +		 */
> +		if (rgrp->index == rcdu->dpad0_source / 2)
> +			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> +	}
> +
> +	rcar_du_group_write(rgrp, DEFR8, defr8);
> +}
> +
>  int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)
>  {
>  	struct rcar_du_group *rgrp;
>  	struct rcar_du_crtc *crtc;
> -	unsigned int index;
>  	int ret;
>  
> -	if (rcdu->info->gen < 2)
> +	/*
> +	 * Only Gen2 hardware has global routing not handled in the group that
> +	 * holds the corresponding CRTCs.
> +	 */
> +	if (rcdu->info->gen != 2)
>  		return 0;
>  
>  	/*
>  	 * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are

"VSPD1"?

> -	 * configured in the DEFR8 register of the first group on Gen2 and the
> -	 * last group on Gen3. As this function can be called with the DU
> -	 * channels of the corresponding CRTCs disabled, we need to enable the
> -	 * group clock before accessing the register.
> +	 * configured in the DEFR8 register of the first group on Gen2. As this
> +	 * function can be called with the DU channels of the corresponding
> +	 * CRTCs disabled, we need to enable the group clock before accessing
> +	 * the register.
>  	 */
> -	index = rcdu->info->gen < 3 ? 0 : DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1;
> -	rgrp = &rcdu->groups[index];
> -	crtc = &rcdu->crtcs[index * 2];
> +	rgrp = &rcdu->groups[0];
> +	crtc = &rcdu->crtcs[0];
>  
>  	ret = clk_prepare_enable(crtc->clock);
>  	if (ret < 0)
> @@ -302,19 +314,33 @@ static void rcar_du_group_set_dpad_levels(struct rcar_du_group *rgrp)
>  	rcar_du_group_write(rgrp, DOFLR, doflr);
>  }
>  
> -int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> +static void rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
>  	u32 dorcr = rcar_du_group_read(rgrp, DORCR);
> +	bool sp1_to_pin2 = false;
>  
>  	dorcr &= ~(DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_MASK);
>  
>  	/*
> -	 * Set the DPAD1 pins sources. Select CRTC 0 if explicitly requested and
> -	 * CRTC 1 in all other cases to avoid cloning CRTC 0 to DPAD0 and DPAD1
> -	 * by default.
> +	 * Configure the superposition processor to pin controller routing.
> +	 * Hardcode the assignment, except on Gen1 where we use it to route the
> +	 * DU channels to DPAD1. There we route CRTC 0 to DPAD1 if explicitly
> +	 * requested, and CRTC 1 in all other cases to avoid cloning CRTC 0 to
> +	 * DPAD0 and DPAD1 by default.
>  	 */
> -	if (rcdu->dpad1_source == rgrp->index * 2)
> +	if (rcdu->info->gen == 1 && rgrp->index == 0) {
> +		struct rcar_du_crtc_state *rstate;
> +		struct rcar_du_crtc *rcrtc;
> +
> +		rcrtc = &rcdu->crtcs[0];
> +		rstate = to_rcar_crtc_state(rcrtc->crtc.state);
> +
> +		if (rstate->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> +			sp1_to_pin2 = true;
> +	}
> +
> +	if (sp1_to_pin2)
>  		dorcr |= DORCR_PG2D_DS1;
>  	else
>  		dorcr |= DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_DS2;
> @@ -323,7 +349,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  
>  	rcar_du_group_set_dpad_levels(rgrp);
>  
> -	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
> +	rcar_du_group_setup_defr8(rgrp);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -430,20 +456,36 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
>  int rcar_du_group_atomic_check(struct drm_device *dev,
>  			       struct drm_atomic_state *state)
>  {
> -	struct drm_crtc_state *crtc_state;
> +	static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
> +				   | BIT(RCAR_DU_OUTPUT_DPAD0);
> +	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	unsigned int i;
>  
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +		struct rcar_du_crtc_state *old_rcrtc_state;
> +		struct rcar_du_crtc_state *new_rcrtc_state;
>  		struct rcar_du_group_state *gstate;
>  
>  		gstate = rcar_du_get_group_state(state, rcrtc->group);
>  		if (IS_ERR(gstate))
>  			return PTR_ERR(gstate);
>  
> -		if (crtc_state->active)
> +		if (new_crtc_state->active)
>  			gstate->enabled = true;
> +
> +		if (!new_crtc_state->active_changed &&
> +		    !new_crtc_state->connectors_changed)
> +			continue;
> +
> +		old_rcrtc_state = to_rcar_crtc_state(old_crtc_state);
> +		new_rcrtc_state = to_rcar_crtc_state(new_crtc_state);
> +
> +		if ((old_rcrtc_state->outputs & dpad_mask) !=
> +		    (new_rcrtc_state->outputs & dpad_mask))
> +			gstate->dpad_routing_changed = true;
>  	}
>  
>  	return 0;
> @@ -463,8 +505,14 @@ void rcar_du_group_atomic_setup(struct drm_device *dev,
>  		old_state = to_rcar_group_state(old_pstate);
>  		new_state = to_rcar_group_state(new_pstate);
>  
> -		if (!old_state->enabled && new_state->enabled)
> +		if (!new_state->enabled)
> +			continue;
> +
> +		if (!old_state->enabled)
>  			rcar_du_group_setup(rgrp);
> +
> +		if (!old_state->enabled || new_state->dpad_routing_changed)
> +			rcar_du_group_set_routing(rgrp);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 20efd2251ec4..b31b3bf8cb94 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -58,11 +58,13 @@ struct rcar_du_group {
>   * struct rcar_du_group_state - Driver-specific group state
>   * @state: base DRM private state
>   * @enabled: true if at least one CRTC in the group is enabled
> + * @dpad_routing_changed: set if CRTC to DPAD output routing has changed
>   */
>  struct rcar_du_group_state {
>  	struct drm_private_state state;
>  
>  	bool enabled;
> +	bool dpad_routing_changed;
>  };
>  
>  #define to_rcar_group_state(s) \
> @@ -73,7 +75,6 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
>  
>  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);
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 65396134fba1..778060bfd383 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -393,14 +393,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	struct rcar_du_device *rcdu = dev->dev_private;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> +	unsigned int vspd1_sink = rcdu->vspd1_sink;
> +	unsigned int dpad0_source = rcdu->dpad0_source;
>  	unsigned int i;
>  
>  	/*
> -	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> -	 * when starting the CRTCs.
> +	 * Store RGB routing to DPAD0, the hardware will be configured when
> +	 * setting up the groups.
>  	 */
> -	rcdu->dpad1_source = -1;
> -
>  	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
>  		struct rcar_du_crtc_state *rcrtc_state =
>  			to_rcar_crtc_state(crtc_state);
> @@ -408,9 +408,6 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  
>  		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD0))
>  			rcdu->dpad0_source = rcrtc->index;
> -
> -		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> -			rcdu->dpad1_source = rcrtc->index;
>  	}
>  
>  	/* Apply the atomic update. */
> @@ -421,6 +418,11 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	rcar_du_crtc_atomic_modeset(dev, old_state);
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +
> +	if (rcdu->vspd1_sink != vspd1_sink ||
> +	    rcdu->dpad0_source != dpad0_source)
> +		rcar_du_set_dpad0_vsp1_routing(rcdu);
> +
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
>  	rcar_du_crtc_atomic_enter_standby(dev, old_state);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index c6430027169f..9bf32585dab3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -552,14 +552,8 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
>  	if (rcdu->info->gen < 3)
>  		rcar_du_plane_setup_scanout(rgrp, state);
>  
> -	if (state->source == RCAR_DU_PLANE_VSPD1) {
> -		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
> -
> -		if (rcdu->vspd1_sink != vspd1_sink) {
> -			rcdu->vspd1_sink = vspd1_sink;
> -			rcar_du_set_dpad0_vsp1_routing(rcdu);
> -		}
> -	}
> +	if (state->source == RCAR_DU_PLANE_VSPD1)
> +		rcdu->vspd1_sink = rgrp->index ? 2 : 0;
>  }
>  
>  int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> -- 
> Regards,
> 
> Laurent Pinchart
>

CU
Uli

Re: [PATCH v3 10/10] drm: rcar-du: Centralise routing configuration in commit tail handler

[Laurent Pinchart]

Hi Ulrich,

On Tue, Jun 18, 2019 at 04:12:01PM +0200, Ulrich Hecht wrote:
> > On June 17, 2019 at 11:09 PM Laurent Pinchart wrote:
> > 
> > Routing configuration for the DU is complex. Depending on the SoC
> > generation various routing options are available:
> > 
> > - The VSP to DU routing is not available on Gen1, is configurable on
> >   Gen2 and is fixed on Gen3. When configurable, the routing affects both
> >   CRTC groups but is set in a register of the first CRTC group.
> > - The DU channel to DPAD output routing is explicitly configurable on
> >   some SoCs of the Gen2 and Gen3 family. When configurable, the DPAD
> >   outputs never offer routing options to CRTCs belonging to different
> >   groups.
> > - On all SoCs the routing of DU channels to DU pin controllers (internal
> >   output of the DU channels) can be swapped within a group. This feature
> >   is only used on Gen1 to control routing of the DPAD1 output.
> > 
> > Routing is thus handled at the group level, but for Gen2 hardware
> > requires configuration of the DPAD1 and VSPD1 routing in the first group
> > even when only the second group is enabled.
> 
> That's "DPAD0 and VSPD1", isn't it?

Yes, my bad. Will fix.

> > Routing at the group level is currently configured when applying CRTC
> > configuration. Global routing is configured at the same time, and is
> > additionally configured by the plane setup code to set VSPD1 routing.
> > This results in code paths that are difficult to follow.
> > 
> > Simplify the routing configuration by performing it all directly, based
> > on CRTC and CRTC group state. Group-level routing is moved to group
> > setup as it only depends on the group state and the state of the CRTCs
> > it contains. Global routing is moved to the commit tail handler, and
> > based on global DU state.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |   3 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   1 -
> >  drivers/gpu/drm/rcar-du/rcar_du_group.c | 154 ++++++++++++++++--------
> >  drivers/gpu/drm/rcar-du/rcar_du_group.h |   3 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  16 +--
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c |  10 +-
> >  6 files changed, 115 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index ab5c288f9d09..f6ea19674a31 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -695,9 +695,8 @@ int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
> >  		    !crtc_state->active)
> >  			continue;
> >  
> > -		/* Configure display timings and output routing. */
> > +		/* Configure display timings. */
> >  		rcar_du_crtc_set_display_timing(rcrtc);
> > -		rcar_du_group_set_routing(rcrtc->group);
> >  
> >  		if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> >  			rcar_du_vsp_modeset(rcrtc);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > index 0cc0984bf2ea..4e3a12496098 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > @@ -92,7 +92,6 @@ struct rcar_du_device {
> >  	} props;
> >  
> >  	unsigned int dpad0_source;
> > -	unsigned int dpad1_source;
> >  	unsigned int vspd1_sink;
> >  };
> >  
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > index 7c9145778567..261baaf7c1f5 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -46,6 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
> >  	rcar_du_write(rgrp->dev, rgrp->mmio_offset + reg, data);
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Static Group Setup
> > + */
> > +
> >  static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
> >  {
> >  	u32 defr6 = DEFR6_CODE;
> > @@ -59,37 +63,6 @@ static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
> >  	rcar_du_group_write(rgrp, DEFR6, defr6);
> >  }
> >  
> > -static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> > -{
> > -	struct rcar_du_device *rcdu = rgrp->dev;
> > -	u32 defr8 = DEFR8_CODE;
> > -
> > -	if (rcdu->info->gen < 3) {
> > -		defr8 |= DEFR8_DEFE8;
> > -
> > -		/*
> > -		 * On Gen2 the DEFR8 register for the first group also controls
> > -		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> > -		 * DU instances that support it.
> > -		 */
> > -		if (rgrp->index == 0) {
> > -			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > -			if (rgrp->dev->vspd1_sink == 2)
> > -				defr8 |= DEFR8_VSCS;
> > -		}
> > -	} else {
> > -		/*
> > -		 * On Gen3 VSPD routing can't be configured, and DPAD routing
> > -		 * is set in the group corresponding to the DPAD output (no Gen3
> > -		 * SoC has multiple DPAD sources belonging to separate groups).
> > -		 */
> > -		if (rgrp->index == rcdu->dpad0_source / 2)
> > -			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > -	}
> > -
> > -	rcar_du_group_write(rgrp, DEFR8, defr8);
> > -}
> > -
> >  static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
> >  {
> >  	struct rcar_du_device *rcdu = rgrp->dev;
> > @@ -153,10 +126,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> >  
> >  	rcar_du_group_setup_pins(rgrp);
> >  
> > -	if (rcdu->info->gen >= 2) {
> > -		rcar_du_group_setup_defr8(rgrp);
> > +	if (rcdu->info->gen >= 2)
> >  		rcar_du_group_setup_didsr(rgrp);
> > -	}
> >  
> >  	if (rcdu->info->gen >= 3)
> >  		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
> > @@ -174,6 +145,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> >  	mutex_unlock(&rgrp->lock);
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Start & Stop
> > + */
> > +
> >  static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
> >  {
> >  	struct rcar_du_device *rcdu = rgrp->dev;
> > @@ -229,26 +204,63 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp)
> >  	__rcar_du_group_start_stop(rgrp, true);
> >  }
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Input and Output Routing
> > + */
> > +
> > +static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> > +{
> > +	struct rcar_du_device *rcdu = rgrp->dev;
> > +	u32 defr8 = DEFR8_CODE;
> > +
> > +	if (rcdu->info->gen < 3) {
> > +		defr8 |= DEFR8_DEFE8;
> > +
> > +		/*
> > +		 * On Gen2 the DEFR8 register for the first group also controls
> > +		 * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> > +		 * DU instances that support it.
> > +		 */
> > +		if (rgrp->index == 0) {
> > +			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > +			if (rgrp->dev->vspd1_sink == 2)
> > +				defr8 |= DEFR8_VSCS;
> > +		}
> > +	} else {
> > +		/*
> > +		 * On Gen3 VSPD routing can't be configured, and DPAD routing
> > +		 * is set in the group corresponding to the DPAD output (no Gen3
> > +		 * SoC has multiple DPAD sources belonging to separate groups).
> > +		 */
> > +		if (rgrp->index == rcdu->dpad0_source / 2)
> > +			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > +	}
> > +
> > +	rcar_du_group_write(rgrp, DEFR8, defr8);
> > +}
> > +
> >  int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)
> >  {
> >  	struct rcar_du_group *rgrp;
> >  	struct rcar_du_crtc *crtc;
> > -	unsigned int index;
> >  	int ret;
> >  
> > -	if (rcdu->info->gen < 2)
> > +	/*
> > +	 * Only Gen2 hardware has global routing not handled in the group that
> > +	 * holds the corresponding CRTCs.
> > +	 */
> > +	if (rcdu->info->gen != 2)
> >  		return 0;
> >  
> >  	/*
> >  	 * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are
> 
> "VSPD1"?

Yes, will fix this too.

> > -	 * configured in the DEFR8 register of the first group on Gen2 and the
> > -	 * last group on Gen3. As this function can be called with the DU
> > -	 * channels of the corresponding CRTCs disabled, we need to enable the
> > -	 * group clock before accessing the register.
> > +	 * configured in the DEFR8 register of the first group on Gen2. As this
> > +	 * function can be called with the DU channels of the corresponding
> > +	 * CRTCs disabled, we need to enable the group clock before accessing
> > +	 * the register.
> >  	 */
> > -	index = rcdu->info->gen < 3 ? 0 : DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1;
> > -	rgrp = &rcdu->groups[index];
> > -	crtc = &rcdu->crtcs[index * 2];
> > +	rgrp = &rcdu->groups[0];
> > +	crtc = &rcdu->crtcs[0];
> >  
> >  	ret = clk_prepare_enable(crtc->clock);
> >  	if (ret < 0)
> > @@ -302,19 +314,33 @@ static void rcar_du_group_set_dpad_levels(struct rcar_du_group *rgrp)
> >  	rcar_du_group_write(rgrp, DOFLR, doflr);
> >  }
> >  
> > -int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> > +static void rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> >  {
> >  	struct rcar_du_device *rcdu = rgrp->dev;
> >  	u32 dorcr = rcar_du_group_read(rgrp, DORCR);
> > +	bool sp1_to_pin2 = false;
> >  
> >  	dorcr &= ~(DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_MASK);
> >  
> >  	/*
> > -	 * Set the DPAD1 pins sources. Select CRTC 0 if explicitly requested and
> > -	 * CRTC 1 in all other cases to avoid cloning CRTC 0 to DPAD0 and DPAD1
> > -	 * by default.
> > +	 * Configure the superposition processor to pin controller routing.
> > +	 * Hardcode the assignment, except on Gen1 where we use it to route the
> > +	 * DU channels to DPAD1. There we route CRTC 0 to DPAD1 if explicitly
> > +	 * requested, and CRTC 1 in all other cases to avoid cloning CRTC 0 to
> > +	 * DPAD0 and DPAD1 by default.
> >  	 */
> > -	if (rcdu->dpad1_source == rgrp->index * 2)
> > +	if (rcdu->info->gen == 1 && rgrp->index == 0) {
> > +		struct rcar_du_crtc_state *rstate;
> > +		struct rcar_du_crtc *rcrtc;
> > +
> > +		rcrtc = &rcdu->crtcs[0];
> > +		rstate = to_rcar_crtc_state(rcrtc->crtc.state);
> > +
> > +		if (rstate->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> > +			sp1_to_pin2 = true;
> > +	}
> > +
> > +	if (sp1_to_pin2)
> >  		dorcr |= DORCR_PG2D_DS1;
> >  	else
> >  		dorcr |= DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_DS2;
> > @@ -323,7 +349,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> >  
> >  	rcar_du_group_set_dpad_levels(rgrp);
> >  
> > -	return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
> > +	rcar_du_group_setup_defr8(rgrp);
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > @@ -430,20 +456,36 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
> >  int rcar_du_group_atomic_check(struct drm_device *dev,
> >  			       struct drm_atomic_state *state)
> >  {
> > -	struct drm_crtc_state *crtc_state;
> > +	static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
> > +				   | BIT(RCAR_DU_OUTPUT_DPAD0);
> > +	struct drm_crtc_state *old_crtc_state;
> > +	struct drm_crtc_state *new_crtc_state;
> >  	struct drm_crtc *crtc;
> >  	unsigned int i;
> >  
> > -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >  		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > +		struct rcar_du_crtc_state *old_rcrtc_state;
> > +		struct rcar_du_crtc_state *new_rcrtc_state;
> >  		struct rcar_du_group_state *gstate;
> >  
> >  		gstate = rcar_du_get_group_state(state, rcrtc->group);
> >  		if (IS_ERR(gstate))
> >  			return PTR_ERR(gstate);
> >  
> > -		if (crtc_state->active)
> > +		if (new_crtc_state->active)
> >  			gstate->enabled = true;
> > +
> > +		if (!new_crtc_state->active_changed &&
> > +		    !new_crtc_state->connectors_changed)
> > +			continue;
> > +
> > +		old_rcrtc_state = to_rcar_crtc_state(old_crtc_state);
> > +		new_rcrtc_state = to_rcar_crtc_state(new_crtc_state);
> > +
> > +		if ((old_rcrtc_state->outputs & dpad_mask) !=
> > +		    (new_rcrtc_state->outputs & dpad_mask))
> > +			gstate->dpad_routing_changed = true;
> >  	}
> >  
> >  	return 0;
> > @@ -463,8 +505,14 @@ void rcar_du_group_atomic_setup(struct drm_device *dev,
> >  		old_state = to_rcar_group_state(old_pstate);
> >  		new_state = to_rcar_group_state(new_pstate);
> >  
> > -		if (!old_state->enabled && new_state->enabled)
> > +		if (!new_state->enabled)
> > +			continue;
> > +
> > +		if (!old_state->enabled)
> >  			rcar_du_group_setup(rgrp);
> > +
> > +		if (!old_state->enabled || new_state->dpad_routing_changed)
> > +			rcar_du_group_set_routing(rgrp);
> >  	}
> >  }
> >  
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > index 20efd2251ec4..b31b3bf8cb94 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > @@ -58,11 +58,13 @@ struct rcar_du_group {
> >   * struct rcar_du_group_state - Driver-specific group state
> >   * @state: base DRM private state
> >   * @enabled: true if at least one CRTC in the group is enabled
> > + * @dpad_routing_changed: set if CRTC to DPAD output routing has changed
> >   */
> >  struct rcar_du_group_state {
> >  	struct drm_private_state state;
> >  
> >  	bool enabled;
> > +	bool dpad_routing_changed;
> >  };
> >  
> >  #define to_rcar_group_state(s) \
> > @@ -73,7 +75,6 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
> >  
> >  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);
> >  
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 65396134fba1..778060bfd383 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -393,14 +393,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  	struct rcar_du_device *rcdu = dev->dev_private;
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_crtc *crtc;
> > +	unsigned int vspd1_sink = rcdu->vspd1_sink;
> > +	unsigned int dpad0_source = rcdu->dpad0_source;
> >  	unsigned int i;
> >  
> >  	/*
> > -	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> > -	 * when starting the CRTCs.
> > +	 * Store RGB routing to DPAD0, the hardware will be configured when
> > +	 * setting up the groups.
> >  	 */
> > -	rcdu->dpad1_source = -1;
> > -
> >  	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
> >  		struct rcar_du_crtc_state *rcrtc_state =
> >  			to_rcar_crtc_state(crtc_state);
> > @@ -408,9 +408,6 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  
> >  		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD0))
> >  			rcdu->dpad0_source = rcrtc->index;
> > -
> > -		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> > -			rcdu->dpad1_source = rcrtc->index;
> >  	}
> >  
> >  	/* Apply the atomic update. */
> > @@ -421,6 +418,11 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  	rcar_du_crtc_atomic_modeset(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > +
> > +	if (rcdu->vspd1_sink != vspd1_sink ||
> > +	    rcdu->dpad0_source != dpad0_source)
> > +		rcar_du_set_dpad0_vsp1_routing(rcdu);
> > +
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  
> >  	rcar_du_crtc_atomic_enter_standby(dev, old_state);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > index c6430027169f..9bf32585dab3 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > @@ -552,14 +552,8 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
> >  	if (rcdu->info->gen < 3)
> >  		rcar_du_plane_setup_scanout(rgrp, state);
> >  
> > -	if (state->source == RCAR_DU_PLANE_VSPD1) {
> > -		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
> > -
> > -		if (rcdu->vspd1_sink != vspd1_sink) {
> > -			rcdu->vspd1_sink = vspd1_sink;
> > -			rcar_du_set_dpad0_vsp1_routing(rcdu);
> > -		}
> > -	}
> > +	if (state->source == RCAR_DU_PLANE_VSPD1)
> > +		rcdu->vspd1_sink = rgrp->index ? 2 : 0;
> >  }
> >  
> >  int __rcar_du_plane_atomic_check(struct drm_plane *plane,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 00/10] drm: rcar-du: Rework CRTC and groups for atomic commits

[Kieran Bingham]

Hi Laurent,

On 17/06/2019 22:09, Laurent Pinchart wrote:
> Hello everybody,
> 
> This patch series refactors atomic commit tail handling in the R-Car DU
> driver to simplify the code flow, and open the door to further
> optimisations. It takes over Kieran's "[PATCH v2 0/6] drm: rcar-du:
> Rework CRTC and groups for atomic commits" and "[RFC PATCH 0/3] VSP1/DU
> atomic interface changes" series.

Thanks for getting this series ready for integration.

For the changes made to patches originally authored by me:
  Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

For your new patches, see those patches directly.
 (One is reviewed, and one is not fully reviewed yet).


For the whole series:
  Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Including testing specifically against a previously reported modetest
failure from the test teams which is now functioning correctly.
Interestingly my connector id's seem to have incremented. I'm not sure
why yet...

As discussed, it could be due to the group changes.

--
Kieran


> The R-Car DU is a bit of a strange beast, with support for up to four
> CRTCs that share resources in groups of two CRTCs. Depending on the
> generation, planes can be shared (on Gen 1 and Gen 2), and output
> routing configuration is also handled at the group level to some extent.
> Furthermore, many configuration parameters, especially those related to
> routing or clock handling, require the whole group to be restarted to
> take effect, even when the parameter itself affects a single CRTC only.
> 
> This hardware architecture is difficult to handle properly on the
> software side, and has resulted in group usage being reference-counted
> while CRTC usage only tracks the enabled state. Calls are then
> unbalanced and difficult to trace, especially for the configuration of
> output routing, and implementation of new shared resources is hindered.
> This patch series aims at solving this problem.
> 
> The series starts with 4 patches that touch the API between the DU and
> VSP drivers. It became apparent that we need to split the configuration
> of the VSP to allow fine grain control of setting the mode configuration
> and enabling/disabling of the pipeline. To support the cross-component
> API, the new interface is added in patch 01/10, including an
> implementation of vsp1_du_setup_lif() to support the transition. Patch
> 02/10 prepares for the new call flow that will call the atomic flush
> handler before enabling the pipeline. The DRM usage is adapted in patch
> 03/10, before the call is removed entirely in patch 04/10.
> 
> The next two patches convert CRTC clock handling and initial setup,
> potentially called from both the CRTC .atomic_begin() and
> .atomic_enable() operations, to a simpler code flow controlled by the
> commit tail handler. Patch 05/10 takes the CRTCs out of standby and put
> them back in standby respectively at the beginning and end of the commit
> tail handler, based on the CRTC atomic state instead of state
> information stored in the custom rcar_du_crtc structure. Patch 06/10
> then performs a similar change for the CRTC mode setting configuration.
> 
> Finally, the last four patches introduce a DRM private object for the
> CRTC groups, along with an associated state. Patch 07/10 adds a helper
> macro to easily iterate over CRTC groups, and patch 08/10 adds the group
> private objects and empty states. Patches 09/10 and 10/10 respectively
> move the group setup and routing configuration under control of the
> commit tail handler, simplifying the configuration and moving state
> information from driver structures to state structures.
> 
> More refactoring is expected, with plane assignment being moved to group
> states, and group restart being optimised to avoid flickering. Better
> configuration of pixel clocks could also be implemented on top of this
> series.
> 
> The whole series has been tested on M3-N and D3 boards with the DU test
> suite (http://git.ideasonboard.com/renesas/kms-tests.git). Additional
> tests have been developed and bugs in existing tests fixed, with patches
> being posted to the linux-renesas-soc@vger.kernel.org mailing list that
> will be integrated in the near future. All individual commits have been
> tested on M3-N, while only key points (after patch 04/10 and patch
> 10/10) have been tested on D3. No failure or change in behaviour has
> been noticed.
> 
> Kieran Bingham (8):
>   media: vsp1: drm: Split vsp1_du_setup_lif()
>   drm: rcar-du: Convert to the new VSP atomic API
>   media: vsp1: drm: Remove vsp1_du_setup_lif()
>   drm: rcar-du: Handle CRTC standby from commit tail handler
>   drm: rcar-du: Handle CRTC configuration from commit tail handler
>   drm: rcar-du: Provide for_each_group helper
>   drm: rcar-du: Create a group state object
>   drm: rcar-du: Perform group setup from the atomic tail handler
> 
> Laurent Pinchart (2):
>   media: vsp1: drm: Don't configure hardware when the pipeline is
>     disabled
>   drm: rcar-du: Centralise routing configuration in commit tail handler
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 168 ++++++-----
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |   9 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |   6 +-
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 377 +++++++++++++++++++-----
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  44 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  63 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c |  10 +-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  20 +-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h   |   2 +
>  drivers/media/platform/vsp1/vsp1_drm.c  | 189 ++++++++----
>  drivers/media/platform/vsp1/vsp1_drm.h  |   2 +
>  include/media/vsp1.h                    |  26 +-
>  12 files changed, 637 insertions(+), 279 deletions(-)
> 

-- 
Regards
--
Kieran