Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

2017-07-12 Thread Laurent Pinchart 
Hi Kieran,

On Wednesday 12 Jul 2017 11:30:19 Kieran Bingham wrote:
> On 11/07/17 23:29, Laurent Pinchart wrote:
> > The DU can compose the output of a VSP with other planes on Gen2
> > hardware, and of two VSPs on Gen3 hardware. Neither of these features
> > are supported by the driver, and the current implementation always
> > assigns planes to CRTCs the same way.
> > 
> > Simplify the implementation by configuring plane assignment when setting
> > up DU groups, instead of recomputing it for every atomic plane update.
> > This allows skipping the wait for vertical blanking when stopping a
> > CRTC, as there's no need to reconfigure plane assignment at that point.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ++-
> >  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +---
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 -
> >  5 files changed, 46 insertions(+), 44 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_group.c index
> > 00d5f470d377..d26b647207b8 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group
> > *rgrp)> 
> > if (rcdu->info->gen >= 3)
> > rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | 
DEFR10_DEFE10);
> > 
> > +   if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > +   /*
> > +* The CRTCs can compose the output of a VSP with other planes
> > +* on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> > +* of these features are supported by the driver, so we 
hardcode
> > +* plane assignment to CRTCs when setting the group up to 
avoid
> > +* the need to restart then group when setting planes up.
> 
> Minor nits in comment:
> 
>   /restart then group/restart the group/
> 
> I would also possibly swap the final 'planes up' as 'up planes' if you
> update here anyway:
> 
> * so we hardcode plane assignment to CRTCs when setting the group up to
> avoid
> * the need to restart the group when setting up planes.
> 
> Up to you of course :)

Thanks, I've fixed both, and also replaced "setting the group up" with 
"setting up the group".

> > +*/
> > +   rcar_du_group_write(rgrp, DS1PR, 1);
> > +   rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 
2);
> 
> whew ... that DS2PR indexing change from g2 to g3 looks annoying ... I had
> to write out the logic tables on paper to verify the change here from the
> previous code.

That's also how I wrote the code :-)

> > +   }
> > +
> > 
> > /*
> > 
> >  * Use DS1PR and DS2PR to configure planes priorities and connects the
> >  * superposition 0 to DU0 pins. DU1 pins will be configured 
dynamically.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

2017-07-12 Thread Kieran Bingham 
Hi Laurent,

Thanks for the patch

Only a minor nit on one comment, but aside from that,

On 11/07/17 23:29, Laurent Pinchart wrote:
> The DU can compose the output of a VSP with other planes on Gen2
> hardware, and of two VSPs on Gen3 hardware. Neither of these features
> are supported by the driver, and the current implementation always
> assigns planes to CRTCs the same way.
> 
> Simplify the implementation by configuring plane assignment when setting
> up DU groups, instead of recomputing it for every atomic plane update.
> This allows skipping the wait for vertical blanking when stopping a
> CRTC, as there's no need to reconfigure plane assignment at that point.
> 
> Signed-off-by: Laurent Pinchart 

Reviewed-by: Kieran Bingham 

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 -
>  5 files changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 17fd1cd5212c..413ab032afed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -315,6 +315,10 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   unsigned int i;
>   u32 dspr = 0;
>  
> + /* Plane assignment is fixed when using the VSP. */
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
> + return;
> +
>   for (i = 0; i < rcrtc->group->num_planes; ++i) {
>   struct rcar_du_plane *plane = >group->planes[i];
>   unsigned int j;
> @@ -351,17 +355,6 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   }
>   }
>  
> - /* If VSP+DU integration is enabled the plane assignment is fixed. */
> - if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> - if (rcdu->info->gen < 3) {
> - dspr = (rcrtc->index % 2) + 1;
> - hwplanes = 1 << (rcrtc->index % 2);
> - } else {
> - dspr = (rcrtc->index % 2) ? 3 : 1;
> - hwplanes = 1 << ((rcrtc->index % 2) ? 2 : 0);
> - }
> - }
> -
>   /*
>* Update the planes to display timing and dot clock generator
>* associations.
> @@ -462,8 +455,13 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc 
> *rcrtc)
>   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);
> + /*
> +  * Start with all planes disabled, except when using the VSP in which
> +  * case the fixed plane assignment must not be modified.
> +  */
> + if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> + rcar_du_group_write(rcrtc->group,
> + rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>   /* Enable the VSP compositor. */
>   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> @@ -505,8 +503,11 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>* are stopped in one operation as we now wait for one vblank per CRTC.
>* Whether this can be improved needs to be researched.
>*/
> - rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> - drm_crtc_wait_one_vblank(crtc);
> + if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> + rcar_du_group_write(rcrtc->group,
> + rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> + drm_crtc_wait_one_vblank(crtc);
> + }
>  
>   /*
>* Disable vertical blanking interrupt reporting. We first need to wait
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 00d5f470d377..d26b647207b8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group 
> *rgrp)
>   if (rcdu->info->gen >= 3)
>   rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
>  
> + if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> + /*
> +  * The CRTCs can compose the output of a VSP with other planes
> +  * on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> +  * of these features are supported by the driver, so we hardcode
> +  * plane assignment to CRTCs when setting the group up to avoid
> +  * the need to

[PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

2017-07-11 Thread Laurent Pinchart 
The DU can compose the output of a VSP with other planes on Gen2
hardware, and of two VSPs on Gen3 hardware. Neither of these features
are supported by the driver, and the current implementation always
assigns planes to CRTCs the same way.

Simplify the implementation by configuring plane assignment when setting
up DU groups, instead of recomputing it for every atomic plane update.
This allows skipping the wait for vertical blanking when stopping a
CRTC, as there's no need to reconfigure plane assignment at that point.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 -
 5 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 17fd1cd5212c..413ab032afed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -315,6 +315,10 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc 
*rcrtc)
unsigned int i;
u32 dspr = 0;
 
+   /* Plane assignment is fixed when using the VSP. */
+   if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
+   return;
+
for (i = 0; i < rcrtc->group->num_planes; ++i) {
struct rcar_du_plane *plane = >group->planes[i];
unsigned int j;
@@ -351,17 +355,6 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc 
*rcrtc)
}
}
 
-   /* If VSP+DU integration is enabled the plane assignment is fixed. */
-   if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
-   if (rcdu->info->gen < 3) {
-   dspr = (rcrtc->index % 2) + 1;
-   hwplanes = 1 << (rcrtc->index % 2);
-   } else {
-   dspr = (rcrtc->index % 2) ? 3 : 1;
-   hwplanes = 1 << ((rcrtc->index % 2) ? 2 : 0);
-   }
-   }
-
/*
 * Update the planes to display timing and dot clock generator
 * associations.
@@ -462,8 +455,13 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
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);
+   /*
+* Start with all planes disabled, except when using the VSP in which
+* case the fixed plane assignment must not be modified.
+*/
+   if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+   rcar_du_group_write(rcrtc->group,
+   rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
/* Enable the VSP compositor. */
if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
@@ -505,8 +503,11 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 * are stopped in one operation as we now wait for one vblank per CRTC.
 * Whether this can be improved needs to be researched.
 */
-   rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
-   drm_crtc_wait_one_vblank(crtc);
+   if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+   rcar_du_group_write(rcrtc->group,
+   rcrtc->index % 2 ? DS2PR : DS1PR, 0);
+   drm_crtc_wait_one_vblank(crtc);
+   }
 
/*
 * Disable vertical blanking interrupt reporting. We first need to wait
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c 
b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 00d5f470d377..d26b647207b8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
if (rcdu->info->gen >= 3)
rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
 
+   if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+   /*
+* The CRTCs can compose the output of a VSP with other planes
+* on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
+* of these features are supported by the driver, so we hardcode
+* plane assignment to CRTCs when setting the group up to avoid
+* the need to restart then group when setting planes up.
+*/
+   rcar_du_group_write(rgrp, DS1PR, 1);
+   rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 2);
+   }
+
/*
 * Use DS1PR and DS2PR to configure planes priorities and connects the