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

[Laurent Pinchart]

Hi Kieran,

On Tuesday 01 Aug 2017 19:10:20 Kieran Bingham wrote:
> 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
> > 
> 
> This looks good to me.
> 
> Minor nit / comment can be safely ignored. Mostly just me thinking outload.
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  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(-)

[snip]

> > 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,
> > +  );
> > +   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 :)

I wonder why I don't like the BIT macro. There's probably no good reason.

> > +
> > +   /* Store the VSP pointer and pipe index in the CRTC. */
> > +   rcdu->crtcs[i].vsp = >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 = >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.

It's a good point, I'll remove the check.

> > +   of_node_put(vsps[i].np);
> > +   }
> > +
> > +   return ret;
> > +}

[snip]

-- 
Regards,

Laurent Pinchart

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

[Kieran Bingham]

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 

This looks good to me.

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

Reviewed-by: Kieran Bingham 

> ---
>  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 = >vsp->planes[0].plane;
> + primary = >vsp->planes[rcrtc->vsp_pipe].plane;
>   else
>   primary = >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,
> +);
> + 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 = >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 = >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;
> +
>