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
> > <laurent.pinchart+rene...@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+rene...@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(-)
[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,
> > + &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 :)
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 = &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.
It's a good point, I'll remove the check.
> > + of_node_put(vsps[i].np);
> > + }
> > +
> > + return ret;
> > +}
[snip]
--
Regards,
Laurent Pinchart
