Re: [PATCH v2 12/14] drm: rcar-du: Support multiple sources from the same VSP

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:

On R-Car H3 ES2.0, DU channels 0 and 3 are served by two separate pipelines from the same VSP. Support this in the DU driver.

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

This looks good to me.

Minor nit / comment can be safely ignored. Mostly just me thinking outload.

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

---

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 3 ++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 91 ++++++++++++++++++++++++++++++---- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 37 +++++++------- drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 10 +++- 5 files changed, 110 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 345eff72f581..8f942ebdd0c6 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -721,7 +721,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index) rcrtc->index = index;

if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))

• primary = &rcrtc->vsp->planes[0].plane;
  

• primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
  

  else primary = &rgrp->planes[index % 2].plane;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index b199ed5adf36..0b6d26ecfc38 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -35,6 +35,8 @@ struct rcar_du_vsp;

• @flip_wait: wait queue used to signal page flip completion
• @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
• @group: CRTC group this CRTC belongs to

• • @vsp: VSP feeding video to this CRTC
• • @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
  */

struct rcar_du_crtc { struct drm_crtc crtc; @@ -52,6 +54,7 @@ struct rcar_du_crtc {

struct rcar_du_group *group; struct rcar_du_vsp *vsp;

• unsigned int vsp_pipe;

};

#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index f4125c8ca902..82b978a5dae6 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -432,6 +432,83 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu) return 0; }

+static int rcar_du_vsps_init(struct rcar_du_device *rcdu) +{

• const struct device_node *np = rcdu->dev->of_node;
• struct of_phandle_args args;
• struct {

• struct device_node *np;
  

• unsigned int crtcs_mask;
  

• } vsps[RCAR_DU_MAX_VSPS] = { { 0, }, };
• unsigned int vsps_count = 0;
• unsigned int cells;
• unsigned int i;
• int ret;
• /*

• * First parse the DT vsps property to populate the list of VSPs. Each
  

• * entry contains a pointer to the VSP DT node and a bitmask of the
  

• * connected DU CRTCs.
  

• */
  

• cells = of_property_count_u32_elems(np, "vsps") / rcdu->num_crtcs - 1;
• if (cells > 1)

• return -EINVAL;
  

• for (i = 0; i < rcdu->num_crtcs; ++i) {

• unsigned int j;
  

• ret = of_parse_phandle_with_fixed_args(np, "vsps", cells, i,
  

• 				       &args);
  

• if (ret < 0)
  

• 	goto error;
  

• /*
  

•  * Add the VSP to the list or update the corresponding existing
  

•  * entry if the VSP has already been added.
  

•  */
  

• for (j = 0; j < vsps_count; ++j) {
  

• 	if (vsps[j].np == args.np)
  

• 		break;
  

• }
  

• if (j < vsps_count)
  

• 	of_node_put(args.np);
  

• else
  

• 	vsps[vsps_count++].np = args.np;
  

• vsps[j].crtcs_mask |= 1 << i;
  

I do love the BIT(x) macro personally - but it's not important :)

• /* Store the VSP pointer and pipe index in the CRTC. */
  

• rcdu->crtcs[i].vsp = &rcdu->vsps[j];
  

• rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0;
  

• }
• /*

• * Then initialize all the VSPs from the node pointers and CRTCs bitmask
  

• * computed previously.
  

• */
  

• for (i = 0; i < vsps_count; ++i) {

• struct rcar_du_vsp *vsp = &rcdu->vsps[i];
  

• vsp->index = i;
  

• vsp->dev = rcdu;
  

• ret = rcar_du_vsp_init(vsp, vsps[i].np, vsps[i].crtcs_mask);
  

• if (ret < 0)
  

• 	goto error;
  

• }
• return 0;

+error:

• for (i = 0; i < ARRAY_SIZE(vsps); ++i) {

• if (vsps[i].np)
  

Minor nit: of_node_put already has NULL protection so we don't need this 'if' but it probably does make it clearer that we are only putting back nodes that we collected.

• 	of_node_put(vsps[i].np);
  

• }
• return ret;

+}

int rcar_du_modeset_init(struct rcar_du_device *rcdu) { static const unsigned int mmio_offsets[] = { @@ -499,17 +576,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)

/* Initialize the compositors. */ if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {

• for (i = 0; i < rcdu->num_crtcs; ++i) {
  

• 	struct rcar_du_vsp *vsp = &rcdu->vsps[i];
  

• 	vsp->index = i;
  

• 	vsp->dev = rcdu;
  

• 	rcdu->crtcs[i].vsp = vsp;
  

• 	ret = rcar_du_vsp_init(vsp);
  

• 	if (ret < 0)
  

• 		return ret;
  

• }
  

• ret = rcar_du_vsps_init(rcdu);
  

• if (ret < 0)
  

• 	return ret;
  

  }

  /* Create the CRTCs. */

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index d46dce054442..3e9098dac0d2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -19,6 +19,7 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_plane_helper.h>

+#include <linux/bitops.h> #include <linux/dma-mapping.h> #include <linux/of_platform.h> #include <linux/scatterlist.h> @@ -81,22 +82,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) */ crtc->group->need_restart = true;

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, &cfg);

• vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);

}

void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) {

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, NULL);

• vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, NULL);

}

void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_begin(crtc->vsp->vsp, 0);

• vsp1_du_atomic_begin(crtc->vsp->vsp, crtc->vsp_pipe);

}

void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_flush(crtc->vsp->vsp, 0);

• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);

}

/* Keep the two tables in sync. */ @@ -162,6 +163,7 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) { struct rcar_du_vsp_plane_state *state = to_rcar_vsp_plane_state(plane->plane.state);

• struct rcar_du_crtc *crtc = to_rcar_crtc(state->state.crtc); struct drm_framebuffer *fb = plane->plane.state->fb; struct vsp1_du_atomic_config cfg = { .pixelformat = 0,

@@ -192,7 +194,8 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) } }

• vsp1_du_atomic_update(plane->vsp->vsp, 0, plane->index, &cfg);

• vsp1_du_atomic_update(plane->vsp->vsp, crtc->vsp_pipe,

• 	      plane->index, &cfg);
  

}

static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane, @@ -288,11 +291,13 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { struct rcar_du_vsp_plane *rplane = to_rcar_vsp_plane(plane);

• struct rcar_du_crtc *crtc = to_rcar_crtc(old_state->crtc);

  if (plane->state->crtc) rcar_du_vsp_plane_setup(rplane); else

• vsp1_du_atomic_update(rplane->vsp->vsp, 0, rplane->index, NULL);
  

• vsp1_du_atomic_update(rplane->vsp->vsp, crtc->vsp_pipe,
  

• 		      rplane->index, NULL);
  

}

static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = { @@ -391,23 +396,17 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .atomic_get_property = rcar_du_vsp_plane_atomic_get_property, };

-int rcar_du_vsp_init(struct rcar_du_vsp *vsp) +int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

•      unsigned int crtcs)
  

{ struct rcar_du_device *rcdu = vsp->dev; struct platform_device *pdev;

• struct device_node *np;

• unsigned int num_crtcs = hweight32(crtcs);

Ohhh a new one to me - I didn't know we had hweight in the kernel. I think I'd done bit-counts maually elsewhere. :S

/me updates mind wiki.

unsigned int i; int ret;

/* Find the VSP device and initialize it. */

• np = of_parse_phandle(rcdu->dev->of_node, "vsps", vsp->index);
• if (!np) {

• dev_err(rcdu->dev, "vsps node not found\n");
  

• return -ENXIO;
  

• }
• pdev = of_find_device_by_node(np);
• of_node_put(np); if (!pdev) return -ENXIO;

@@ -428,15 +427,15 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp) return -ENOMEM;

for (i = 0; i < vsp->num_planes; ++i) {

• enum drm_plane_type type = i ? DRM_PLANE_TYPE_OVERLAY
  

• 			 : DRM_PLANE_TYPE_PRIMARY;
  

• enum drm_plane_type type = i < num_crtcs
  

• 			 ? DRM_PLANE_TYPE_PRIMARY
  

• 			 : DRM_PLANE_TYPE_OVERLAY;
  

  struct rcar_du_vsp_plane *plane = &vsp->planes[i];

  plane->vsp = vsp; plane->index = i;

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane,
  

• 			       1 << vsp->index,
  

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
  		       &rcar_du_vsp_plane_funcs,
  		       formats_kms,
  		       ARRAY_SIZE(formats_kms), type,
  

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h index 8861661590ff..f876c512163c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h @@ -64,13 +64,19 @@ 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); +int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

•      unsigned int crtcs);
  

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); void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc); #else -static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; }; +static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,

• 		   struct device_node *np,
  

• 		   unsigned int crtcs)
  

+{

• return -ENXIO;

+} 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) { };