[PATCH 0/3] R-Car DU: Clip planes to screen boundaries

[PATCH 0/3] R-Car DU: Clip planes to screen boundaries

[Laurent Pinchart]

Hello,

This patch series fixes support for planes that cross the screen boundaries.
The KMS API supports such a configuration, but the DU and VSP hardware
doesn't. This leads to different kind of dispay artifacts or hangs.

The series starts with a bit of refactoring to share existing code and make it
easier to share new code (1/3). Patch 2/3 then reject planes that are fully
off-screen as they don't work properly and we have currently no use case for
them. Finally, patch 3/3 clips planes to screen boundaries to fix the main
issue.

The patches are based on top of Dave's latest master branch.

Laurent Pinchart (3):
  drm: rcar-du: Share plane atomic check code between Gen2 and Gen3
  drm: rcar-du: Reject planes located fully off-screen
  drm: rcar-du: Clip planes to screen boundaries

 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 109 ++++++++++++++++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_plane.h |   8 +++
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  43 ++++---------
 3 files changed, 105 insertions(+), 55 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] drm: rcar-du: Share plane atomic check code between Gen2 and Gen3

[Laurent Pinchart]

The plane atomic check implementation is identical on Gen2 (DU planes)
and Gen3 (VSP planes), but two separate functions exist as they operate
on different data structures. Refactor the code to share the
implementation.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 27 +++++++++++++++++----------
 drivers/gpu/drm/rcar-du/rcar_du_plane.h |  4 ++++
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 22 +---------------------
 3 files changed, 22 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 61833cc1c699..4f076c364f25 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -565,27 +565,26 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
 	}
 }
 
-static int rcar_du_plane_atomic_check(struct drm_plane *plane,
-				      struct drm_plane_state *state)
+int __rcar_du_plane_atomic_check(struct drm_plane *plane,
+				 struct drm_plane_state *state,
+				 const struct rcar_du_format_info **format)
 {
-	struct rcar_du_plane_state *rstate = to_rcar_plane_state(state);
-	struct rcar_du_plane *rplane = to_rcar_plane(plane);
-	struct rcar_du_device *rcdu = rplane->group->dev;
+	struct drm_device *dev = plane->dev;
 
 	if (!state->fb || !state->crtc) {
-		rstate->format = NULL;
+		*format = NULL;
 		return 0;
 	}
 
 	if (state->src_w >> 16 != state->crtc_w ||
 	    state->src_h >> 16 != state->crtc_h) {
-		dev_dbg(rcdu->dev, "%s: scaling not supported\n", __func__);
+		dev_dbg(dev->dev, "%s: scaling not supported\n", __func__);
 		return -EINVAL;
 	}
 
-	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
-		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
+	*format = rcar_du_format_info(state->fb->format->format);
+	if (*format == NULL) {
+		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
 			state->fb->format->format);
 		return -EINVAL;
 	}
@@ -593,6 +592,14 @@ static int rcar_du_plane_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
+static int rcar_du_plane_atomic_check(struct drm_plane *plane,
+				      struct drm_plane_state *state)
+{
+	struct rcar_du_plane_state *rstate = to_rcar_plane_state(state);
+
+	return __rcar_du_plane_atomic_check(plane, state, &rstate->format);
+}
+
 static void rcar_du_plane_atomic_update(struct drm_plane *plane,
 					struct drm_plane_state *old_state)
 {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
index f62e09f195de..890321b4665d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
@@ -73,6 +73,10 @@ to_rcar_plane_state(struct drm_plane_state *state)
 int rcar_du_atomic_check_planes(struct drm_device *dev,
 				struct drm_atomic_state *state);
 
+int __rcar_du_plane_atomic_check(struct drm_plane *plane,
+				 struct drm_plane_state *state,
+				 const struct rcar_du_format_info **format);
+
 int rcar_du_planes_init(struct rcar_du_group *rgrp);
 
 void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 2c96147bc444..dd66dcb8da23 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -268,28 +268,8 @@ static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
 					  struct drm_plane_state *state)
 {
 	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
-	struct rcar_du_vsp_plane *rplane = to_rcar_vsp_plane(plane);
-	struct rcar_du_device *rcdu = rplane->vsp->dev;
-
-	if (!state->fb || !state->crtc) {
-		rstate->format = NULL;
-		return 0;
-	}
 
-	if (state->src_w >> 16 != state->crtc_w ||
-	    state->src_h >> 16 != state->crtc_h) {
-		dev_dbg(rcdu->dev, "%s: scaling not supported\n", __func__);
-		return -EINVAL;
-	}
-
-	rstate->format = rcar_du_format_info(state->fb->format->format);
-	if (rstate->format == NULL) {
-		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
-			state->fb->format->format);
-		return -EINVAL;
-	}
-
-	return 0;
+	return __rcar_du_plane_atomic_check(plane, state, &rstate->format);
 }
 
 static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane,
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm: rcar-du: Share plane atomic check code between Gen2 and Gen3

[Kieran Bingham]

Hi Laurent,

On 16/08/17 00:03, Laurent Pinchart wrote:
> The plane atomic check implementation is identical on Gen2 (DU planes)
> and Gen3 (VSP planes), but two separate functions exist as they operate
> on different data structures. Refactor the code to share the
> implementation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

This looks good to me.

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

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 27 +++++++++++++++++----------
>  drivers/gpu/drm/rcar-du/rcar_du_plane.h |  4 ++++
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 22 +---------------------
>  3 files changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index 61833cc1c699..4f076c364f25 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -565,27 +565,26 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
>  	}
>  }
>  
> -static int rcar_du_plane_atomic_check(struct drm_plane *plane,
> -				      struct drm_plane_state *state)
> +int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> +				 struct drm_plane_state *state,
> +				 const struct rcar_du_format_info **format)
>  {
> -	struct rcar_du_plane_state *rstate = to_rcar_plane_state(state);
> -	struct rcar_du_plane *rplane = to_rcar_plane(plane);
> -	struct rcar_du_device *rcdu = rplane->group->dev;
> +	struct drm_device *dev = plane->dev;
>  
>  	if (!state->fb || !state->crtc) {
> -		rstate->format = NULL;
> +		*format = NULL;
>  		return 0;
>  	}
>  
>  	if (state->src_w >> 16 != state->crtc_w ||
>  	    state->src_h >> 16 != state->crtc_h) {
> -		dev_dbg(rcdu->dev, "%s: scaling not supported\n", __func__);
> +		dev_dbg(dev->dev, "%s: scaling not supported\n", __func__);
>  		return -EINVAL;
>  	}
>  
> -	rstate->format = rcar_du_format_info(state->fb->format->format);
> -	if (rstate->format == NULL) {
> -		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
> +	*format = rcar_du_format_info(state->fb->format->format);
> +	if (*format == NULL) {
> +		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
>  			state->fb->format->format);
>  		return -EINVAL;
>  	}
> @@ -593,6 +592,14 @@ static int rcar_du_plane_atomic_check(struct drm_plane *plane,
>  	return 0;
>  }
>  
> +static int rcar_du_plane_atomic_check(struct drm_plane *plane,
> +				      struct drm_plane_state *state)
> +{
> +	struct rcar_du_plane_state *rstate = to_rcar_plane_state(state);
> +
> +	return __rcar_du_plane_atomic_check(plane, state, &rstate->format);
> +}
> +
>  static void rcar_du_plane_atomic_update(struct drm_plane *plane,
>  					struct drm_plane_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> index f62e09f195de..890321b4665d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> @@ -73,6 +73,10 @@ to_rcar_plane_state(struct drm_plane_state *state)
>  int rcar_du_atomic_check_planes(struct drm_device *dev,
>  				struct drm_atomic_state *state);
>  
> +int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> +				 struct drm_plane_state *state,
> +				 const struct rcar_du_format_info **format);
> +
>  int rcar_du_planes_init(struct rcar_du_group *rgrp);
>  
>  void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 2c96147bc444..dd66dcb8da23 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -268,28 +268,8 @@ static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
>  					  struct drm_plane_state *state)
>  {
>  	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> -	struct rcar_du_vsp_plane *rplane = to_rcar_vsp_plane(plane);
> -	struct rcar_du_device *rcdu = rplane->vsp->dev;
> -
> -	if (!state->fb || !state->crtc) {
> -		rstate->format = NULL;
> -		return 0;
> -	}
>  
> -	if (state->src_w >> 16 != state->crtc_w ||
> -	    state->src_h >> 16 != state->crtc_h) {
> -		dev_dbg(rcdu->dev, "%s: scaling not supported\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	rstate->format = rcar_du_format_info(state->fb->format->format);
> -	if (rstate->format == NULL) {
> -		dev_dbg(rcdu->dev, "%s: unsupported format %08x\n", __func__,
> -			state->fb->format->format);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> +	return __rcar_du_plane_atomic_check(plane, state, &rstate->format);
>  }
>  
>  static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane,
> 

[PATCH 2/3] drm: rcar-du: Reject planes located fully off-screen

[Laurent Pinchart]

There is no point in accepting fully off-screen planes as they won't be
displayed. Reject them in the atomic check.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 4f076c364f25..714e02960635 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -569,6 +569,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
 				 struct drm_plane_state *state,
 				 const struct rcar_du_format_info **format)
 {
+	const struct drm_display_mode *mode;
+	struct drm_crtc_state *crtc_state;
 	struct drm_device *dev = plane->dev;
 
 	if (!state->fb || !state->crtc) {
@@ -582,6 +584,20 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	mode = &crtc_state->adjusted_mode;
+	if (state->crtc_x >= mode->hdisplay ||
+	    state->crtc_y >= mode->vdisplay ||
+	    state->crtc_x + (int)state->crtc_w <= 0 ||
+	    state->crtc_y + (int)state->crtc_h <= 0) {
+		dev_dbg(dev->dev, "%s: plane can't be fully off-screen\n",
+			__func__);
+		return -EINVAL;
+	}
+
 	*format = rcar_du_format_info(state->fb->format->format);
 	if (*format == NULL) {
 		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm: rcar-du: Reject planes located fully off-screen

[Kieran Bingham]

Hi Laurent,

Thankyou for the patch,

This looks good, and passes the tests.

On 16/08/17 00:03, Laurent Pinchart wrote:
> There is no point in accepting fully off-screen planes as they won't be
> displayed. Reject them in the atomic check.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index 4f076c364f25..714e02960635 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -569,6 +569,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
>  				 struct drm_plane_state *state,
>  				 const struct rcar_du_format_info **format)
>  {
> +	const struct drm_display_mode *mode;
> +	struct drm_crtc_state *crtc_state;
>  	struct drm_device *dev = plane->dev;
>  
>  	if (!state->fb || !state->crtc) {
> @@ -582,6 +584,20 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	mode = &crtc_state->adjusted_mode;
> +	if (state->crtc_x >= mode->hdisplay ||
> +	    state->crtc_y >= mode->vdisplay ||
> +	    state->crtc_x + (int)state->crtc_w <= 0 ||
> +	    state->crtc_y + (int)state->crtc_h <= 0) {
> +		dev_dbg(dev->dev, "%s: plane can't be fully off-screen\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
>  	*format = rcar_du_format_info(state->fb->format->format);
>  	if (*format == NULL) {
>  		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
> 

Re: [PATCH 2/3] drm: rcar-du: Reject planes located fully off-screen

[Daniel Vetter]

On Wed, Sep 27, 2017 at 07:58:42PM +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> Thankyou for the patch,
> 
> This looks good, and passes the tests.
> 
> On 16/08/17 00:03, Laurent Pinchart wrote:
> > There is no point in accepting fully off-screen planes as they won't be
> > displayed. Reject them in the atomic check.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Just a note, this is different from what i915 does. Not sure you want to
intentionally be incompatible with i915 and hence defacto make this part
of atomic undefined (or break some apps, depending upon how this pans
out). I think X loves to put the cursor offscreen at least.

I think we even have igts to test this properly.
-Daniel
> 
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > index 4f076c364f25..714e02960635 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > @@ -569,6 +569,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> >  				 struct drm_plane_state *state,
> >  				 const struct rcar_du_format_info **format)
> >  {
> > +	const struct drm_display_mode *mode;
> > +	struct drm_crtc_state *crtc_state;
> >  	struct drm_device *dev = plane->dev;
> >  
> >  	if (!state->fb || !state->crtc) {
> > @@ -582,6 +584,20 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
> >  		return -EINVAL;
> >  	}
> >  
> > +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	mode = &crtc_state->adjusted_mode;
> > +	if (state->crtc_x >= mode->hdisplay ||
> > +	    state->crtc_y >= mode->vdisplay ||
> > +	    state->crtc_x + (int)state->crtc_w <= 0 ||
> > +	    state->crtc_y + (int)state->crtc_h <= 0) {
> > +		dev_dbg(dev->dev, "%s: plane can't be fully off-screen\n",
> > +			__func__);
> > +		return -EINVAL;
> > +	}
> > +
> >  	*format = rcar_du_format_info(state->fb->format->format);
> >  	if (*format == NULL) {
> >  		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: rcar-du: Reject planes located fully off-screen

[Laurent Pinchart]

Hi Daniel,

On Thursday, 28 September 2017 10:10:34 EEST Daniel Vetter wrote:
> On Wed, Sep 27, 2017 at 07:58:42PM +0100, Kieran Bingham wrote:
> > Hi Laurent,
> > 
> > Thankyou for the patch,
> > 
> > This looks good, and passes the tests.
> > 
> > On 16/08/17 00:03, Laurent Pinchart wrote:
> > > There is no point in accepting fully off-screen planes as they won't be
> > > displayed. Reject them in the atomic check.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Just a note, this is different from what i915 does. Not sure you want to
> intentionally be incompatible with i915 and hence defacto make this part
> of atomic undefined (or break some apps, depending upon how this pans
> out). I think X loves to put the cursor offscreen at least.
> 
> I think we even have igts to test this properly.

If you update the documentation I'll submit a v2 that allows fully off-screen 
planes :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm: rcar-du: Reject planes located fully off-screen

[Daniel Vetter]

On Thu, Sep 28, 2017 at 11:16 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> On Thursday, 28 September 2017 10:10:34 EEST Daniel Vetter wrote:
>> On Wed, Sep 27, 2017 at 07:58:42PM +0100, Kieran Bingham wrote:
>> > Hi Laurent,
>> >
>> > Thankyou for the patch,
>> >
>> > This looks good, and passes the tests.
>> >
>> > On 16/08/17 00:03, Laurent Pinchart wrote:
>> > > There is no point in accepting fully off-screen planes as they won't be
>> > > displayed. Reject them in the atomic check.
>> > >
>> > > Signed-off-by: Laurent Pinchart
>> > > <laurent.pinchart+renesas@ideasonboard.com>
>> >
>> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> Just a note, this is different from what i915 does. Not sure you want to
>> intentionally be incompatible with i915 and hence defacto make this part
>> of atomic undefined (or break some apps, depending upon how this pans
>> out). I think X loves to put the cursor offscreen at least.
>>
>> I think we even have igts to test this properly.
>
> If you update the documentation I'll submit a v2 that allows fully off-screen
> planes :-)

Quoting them:

"The destination rectangle can lie outside of the visible area of the
current mode of the CRTC."

This shit is better than I thought!

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Now can I raise you one and challenge you to run the kms tests in igt
and see how non-compliant your driver is :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: rcar-du: Clip planes to screen boundaries

[Laurent Pinchart]

Unlike the KMS API, the hardware doesn't support planes exceeding the
screen boundaries. Clip plane coordinates to support the use case.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 66 +++++++++++++++++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_plane.h |  4 ++
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 21 +++++++----
 3 files changed, 67 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 714e02960635..7944790bac25 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -19,6 +19,7 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 
 #include "rcar_du_drv.h"
 #include "rcar_du_group.h"
@@ -329,11 +330,39 @@ static void rcar_du_plane_write(struct rcar_du_group *rgrp,
 		      data);
 }
 
+void rcar_du_plane_clip(const struct drm_plane_state *state,
+			struct drm_rect *src, struct drm_rect *dst)
+{
+	const struct drm_display_mode *mode;
+
+	/*
+	 * The hardware requires all planes to be fully contained in the output
+	 * rectangle. Crop the destination rectangle to fit in the CRTC.
+	 */
+	mode = &state->crtc->state->adjusted_mode;
+
+	dst->x1 = max(0, state->crtc_x);
+	dst->y1 = max(0, state->crtc_y);
+	dst->x2 = min_t(unsigned int, mode->hdisplay,
+			state->crtc_x + state->crtc_w);
+	dst->y2 = min_t(unsigned int, mode->vdisplay,
+			state->crtc_y + state->crtc_h);
+
+	/*
+	 * Offset the left and top source coordinates by the same displacement
+	 * we have applied to the destination rectangle. Copy the width and
+	 * height as the hardware can't scale.
+	 */
+	src->x1 = (state->src_x >> 16) + (dst->x1 - state->crtc_x);
+	src->y1 = (state->src_y >> 16) + (dst->y1 - state->crtc_y);
+	src->x2 = src->x1 + (dst->x2 - dst->x1);
+	src->y2 = src->y1 + (dst->y2 - dst->y1);
+}
+
 static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
-					const struct rcar_du_plane_state *state)
+					const struct rcar_du_plane_state *state,
+					const struct drm_rect *src)
 {
-	unsigned int src_x = state->state.src_x >> 16;
-	unsigned int src_y = state->state.src_y >> 16;
 	unsigned int index = state->hwindex;
 	unsigned int pitch;
 	bool interlaced;
@@ -357,7 +386,7 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
 			dma[i] = gem->paddr + fb->offsets[i];
 		}
 	} else {
-		pitch = state->state.src_w >> 16;
+		pitch = drm_rect_width(&state->state.src);
 		dma[0] = 0;
 		dma[1] = 0;
 	}
@@ -383,8 +412,8 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
 	 * require a halved Y position value, in both progressive and interlaced
 	 * modes.
 	 */
-	rcar_du_plane_write(rgrp, index, PnSPXR, src_x);
-	rcar_du_plane_write(rgrp, index, PnSPYR, src_y *
+	rcar_du_plane_write(rgrp, index, PnSPXR, src->x1);
+	rcar_du_plane_write(rgrp, index, PnSPYR, src->y1 *
 			    (!interlaced && state->format->bpp == 32 ? 2 : 1));
 
 	rcar_du_plane_write(rgrp, index, PnDSA0R, dma[0]);
@@ -394,8 +423,8 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
 
 		rcar_du_plane_write(rgrp, index, PnMWR, pitch);
 
-		rcar_du_plane_write(rgrp, index, PnSPXR, src_x);
-		rcar_du_plane_write(rgrp, index, PnSPYR, src_y *
+		rcar_du_plane_write(rgrp, index, PnSPXR, src->x1);
+		rcar_du_plane_write(rgrp, index, PnSPYR, src->y1 *
 				    (state->format->bpp == 16 ? 2 : 1) / 2);
 
 		rcar_du_plane_write(rgrp, index, PnDSA0R, dma[1]);
@@ -518,7 +547,8 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
 
 static void rcar_du_plane_setup_format(struct rcar_du_group *rgrp,
 				       unsigned int index,
-				       const struct rcar_du_plane_state *state)
+				       const struct rcar_du_plane_state *state,
+				       const struct drm_rect *dst)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
 
@@ -528,10 +558,10 @@ static void rcar_du_plane_setup_format(struct rcar_du_group *rgrp,
 		rcar_du_plane_setup_format_gen3(rgrp, index, state);
 
 	/* Destination position and size */
-	rcar_du_plane_write(rgrp, index, PnDSXR, state->state.crtc_w);
-	rcar_du_plane_write(rgrp, index, PnDSYR, state->state.crtc_h);
-	rcar_du_plane_write(rgrp, index, PnDPXR, state->state.crtc_x);
-	rcar_du_plane_write(rgrp, index, PnDPYR, state->state.crtc_y);
+	rcar_du_plane_write(rgrp, index, PnDSXR, drm_rect_width(dst));
+	rcar_du_plane_write(rgrp, index, PnDSYR, drm_rect_height(dst));
+	rcar_du_plane_write(rgrp, index, PnDPXR, dst->x1);
+	rcar_du_plane_write(rgrp, index, PnDPYR, dst->y1);
 
 	if (rcdu->info->gen < 3) {
 		/* Wrap-around and blinking, disabled */
@@ -546,14 +576,18 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
 			   const struct rcar_du_plane_state *state)
 {
 	struct rcar_du_device *rcdu = rgrp->dev;
+	struct drm_rect src;
+	struct drm_rect dst;
+
+	rcar_du_plane_clip(&state->state, &src, &dst);
 
-	rcar_du_plane_setup_format(rgrp, state->hwindex, state);
+	rcar_du_plane_setup_format(rgrp, state->hwindex, state, &dst);
 	if (state->format->planes == 2)
 		rcar_du_plane_setup_format(rgrp, (state->hwindex + 1) % 8,
-					   state);
+					   state, &dst);
 
 	if (rcdu->info->gen < 3)
-		rcar_du_plane_setup_scanout(rgrp, state);
+		rcar_du_plane_setup_scanout(rgrp, state, &src);
 
 	if (state->source == RCAR_DU_PLANE_VSPD1) {
 		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
index 890321b4665d..ec52a2af7cd4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
@@ -17,6 +17,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 
+struct drm_rect;
 struct rcar_du_format_info;
 struct rcar_du_group;
 
@@ -79,6 +80,9 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
 
 int rcar_du_planes_init(struct rcar_du_group *rgrp);
 
+void rcar_du_plane_clip(const struct drm_plane_state *state,
+			struct drm_rect *src, struct drm_rect *dst);
+
 void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
 			   const struct rcar_du_plane_state *state);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index dd66dcb8da23..e62a8d7ee0c5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -18,6 +18,7 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 
 #include <linux/bitops.h>
 #include <linux/dma-mapping.h>
@@ -176,17 +177,21 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
 		.alpha = state->alpha,
 		.zpos = state->state.zpos,
 	};
+	struct drm_rect src;
+	struct drm_rect dst;
 	unsigned int i;
 
-	cfg.src.left = state->state.src_x >> 16;
-	cfg.src.top = state->state.src_y >> 16;
-	cfg.src.width = state->state.src_w >> 16;
-	cfg.src.height = state->state.src_h >> 16;
+	rcar_du_plane_clip(&state->state, &src, &dst);
 
-	cfg.dst.left = state->state.crtc_x;
-	cfg.dst.top = state->state.crtc_y;
-	cfg.dst.width = state->state.crtc_w;
-	cfg.dst.height = state->state.crtc_h;
+	cfg.src.left = src.x1;
+	cfg.src.top = src.y1;
+	cfg.src.width = drm_rect_width(&src);
+	cfg.src.height = drm_rect_height(&src);
+
+	cfg.dst.left = dst.x1;
+	cfg.dst.top = dst.y1;
+	cfg.dst.width = drm_rect_width(&dst);
+	cfg.dst.height = drm_rect_height(&dst);
 
 	for (i = 0; i < state->format->planes; ++i)
 		cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: rcar-du: Clip planes to screen boundaries

[Kieran Bingham]

Hi Laurent,

Thanks for the patch,

On 16/08/17 00:03, Laurent Pinchart wrote:
> Unlike the KMS API, the hardware doesn't support planes exceeding the
> screen boundaries. Clip plane coordinates to support the use case.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 66 +++++++++++++++++++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_plane.h |  4 ++
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 21 +++++++----
>  3 files changed, 67 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index 714e02960635..7944790bac25 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -19,6 +19,7 @@
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  
>  #include "rcar_du_drv.h"
>  #include "rcar_du_group.h"
> @@ -329,11 +330,39 @@ static void rcar_du_plane_write(struct rcar_du_group *rgrp,
>  		      data);
>  }
>  
> +void rcar_du_plane_clip(const struct drm_plane_state *state,
> +			struct drm_rect *src, struct drm_rect *dst)
> +{
> +	const struct drm_display_mode *mode;
> +
> +	/*
> +	 * The hardware requires all planes to be fully contained in the output
> +	 * rectangle. Crop the destination rectangle to fit in the CRTC.
> +	 */
> +	mode = &state->crtc->state->adjusted_mode;
> +
> +	dst->x1 = max(0, state->crtc_x);
> +	dst->y1 = max(0, state->crtc_y);
> +	dst->x2 = min_t(unsigned int, mode->hdisplay,
> +			state->crtc_x + state->crtc_w);
> +	dst->y2 = min_t(unsigned int, mode->vdisplay,
> +			state->crtc_y + state->crtc_h);
> +
> +	/*
> +	 * Offset the left and top source coordinates by the same displacement
> +	 * we have applied to the destination rectangle. Copy the width and
> +	 * height as the hardware can't scale.
> +	 */
> +	src->x1 = (state->src_x >> 16) + (dst->x1 - state->crtc_x);
> +	src->y1 = (state->src_y >> 16) + (dst->y1 - state->crtc_y);

I had to look up why we do a >> 16 here. I guess this is for sub-pixel
positioning. That might be obvious to DRM developers, but was new to me.

Probably not worth a comment though - as I know what it is now :)
Do we have any macros to make this 16.16 -> int conversion more explicit?
(I think this comes under the same category as using BIT() to which we differ on
opinion anyway though)


> +	src->x2 = src->x1 + (dst->x2 - dst->x1);
> +	src->y2 = src->y1 + (dst->y2 - dst->y1);
> +}
> +
>  static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
> -					const struct rcar_du_plane_state *state)
> +					const struct rcar_du_plane_state *state,
> +					const struct drm_rect *src)
>  {
> -	unsigned int src_x = state->state.src_x >> 16;
> -	unsigned int src_y = state->state.src_y >> 16;
>  	unsigned int index = state->hwindex;
>  	unsigned int pitch;
>  	bool interlaced;
> @@ -357,7 +386,7 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
>  			dma[i] = gem->paddr + fb->offsets[i];
>  		}
>  	} else {
> -		pitch = state->state.src_w >> 16;
> +		pitch = drm_rect_width(&state->state.src);
>  		dma[0] = 0;
>  		dma[1] = 0;
>  	}
> @@ -383,8 +412,8 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
>  	 * require a halved Y position value, in both progressive and interlaced
>  	 * modes.
>  	 */
> -	rcar_du_plane_write(rgrp, index, PnSPXR, src_x);
> -	rcar_du_plane_write(rgrp, index, PnSPYR, src_y *
> +	rcar_du_plane_write(rgrp, index, PnSPXR, src->x1);
> +	rcar_du_plane_write(rgrp, index, PnSPYR, src->y1 *
>  			    (!interlaced && state->format->bpp == 32 ? 2 : 1));
>  
>  	rcar_du_plane_write(rgrp, index, PnDSA0R, dma[0]);
> @@ -394,8 +423,8 @@ static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
>  
>  		rcar_du_plane_write(rgrp, index, PnMWR, pitch);
>  
> -		rcar_du_plane_write(rgrp, index, PnSPXR, src_x);
> -		rcar_du_plane_write(rgrp, index, PnSPYR, src_y *
> +		rcar_du_plane_write(rgrp, index, PnSPXR, src->x1);
> +		rcar_du_plane_write(rgrp, index, PnSPYR, src->y1 *
>  				    (state->format->bpp == 16 ? 2 : 1) / 2);
>  
>  		rcar_du_plane_write(rgrp, index, PnDSA0R, dma[1]);
> @@ -518,7 +547,8 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
>  
>  static void rcar_du_plane_setup_format(struct rcar_du_group *rgrp,
>  				       unsigned int index,
> -				       const struct rcar_du_plane_state *state)
> +				       const struct rcar_du_plane_state *state,
> +				       const struct drm_rect *dst)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
>  
> @@ -528,10 +558,10 @@ static void rcar_du_plane_setup_format(struct rcar_du_group *rgrp,
>  		rcar_du_plane_setup_format_gen3(rgrp, index, state);
>  
>  	/* Destination position and size */
> -	rcar_du_plane_write(rgrp, index, PnDSXR, state->state.crtc_w);
> -	rcar_du_plane_write(rgrp, index, PnDSYR, state->state.crtc_h);
> -	rcar_du_plane_write(rgrp, index, PnDPXR, state->state.crtc_x);
> -	rcar_du_plane_write(rgrp, index, PnDPYR, state->state.crtc_y);
> +	rcar_du_plane_write(rgrp, index, PnDSXR, drm_rect_width(dst));
> +	rcar_du_plane_write(rgrp, index, PnDSYR, drm_rect_height(dst));
> +	rcar_du_plane_write(rgrp, index, PnDPXR, dst->x1);
> +	rcar_du_plane_write(rgrp, index, PnDPYR, dst->y1);
>  
>  	if (rcdu->info->gen < 3) {
>  		/* Wrap-around and blinking, disabled */
> @@ -546,14 +576,18 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
>  			   const struct rcar_du_plane_state *state)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
> +	struct drm_rect src;
> +	struct drm_rect dst;
> +
> +	rcar_du_plane_clip(&state->state, &src, &dst);
>  
> -	rcar_du_plane_setup_format(rgrp, state->hwindex, state);
> +	rcar_du_plane_setup_format(rgrp, state->hwindex, state, &dst);
>  	if (state->format->planes == 2)
>  		rcar_du_plane_setup_format(rgrp, (state->hwindex + 1) % 8,
> -					   state);
> +					   state, &dst);
>  
>  	if (rcdu->info->gen < 3)
> -		rcar_du_plane_setup_scanout(rgrp, state);
> +		rcar_du_plane_setup_scanout(rgrp, state, &src);
>  
>  	if (state->source == RCAR_DU_PLANE_VSPD1) {
>  		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> index 890321b4665d..ec52a2af7cd4 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
> @@ -17,6 +17,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  
> +struct drm_rect;
>  struct rcar_du_format_info;
>  struct rcar_du_group;
>  
> @@ -79,6 +80,9 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
>  
>  int rcar_du_planes_init(struct rcar_du_group *rgrp);
>  
> +void rcar_du_plane_clip(const struct drm_plane_state *state,
> +			struct drm_rect *src, struct drm_rect *dst);
> +
>  void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
>  			   const struct rcar_du_plane_state *state);
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index dd66dcb8da23..e62a8d7ee0c5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -18,6 +18,7 @@
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  
>  #include <linux/bitops.h>
>  #include <linux/dma-mapping.h>
> @@ -176,17 +177,21 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
>  		.alpha = state->alpha,
>  		.zpos = state->state.zpos,
>  	};
> +	struct drm_rect src;
> +	struct drm_rect dst;
>  	unsigned int i;
>  
> -	cfg.src.left = state->state.src_x >> 16;
> -	cfg.src.top = state->state.src_y >> 16;
> -	cfg.src.width = state->state.src_w >> 16;
> -	cfg.src.height = state->state.src_h >> 16;
> +	rcar_du_plane_clip(&state->state, &src, &dst);
>  
> -	cfg.dst.left = state->state.crtc_x;
> -	cfg.dst.top = state->state.crtc_y;
> -	cfg.dst.width = state->state.crtc_w;
> -	cfg.dst.height = state->state.crtc_h;
> +	cfg.src.left = src.x1;
> +	cfg.src.top = src.y1;
> +	cfg.src.width = drm_rect_width(&src);
> +	cfg.src.height = drm_rect_height(&src);
> +
> +	cfg.dst.left = dst.x1;
> +	cfg.dst.top = dst.y1;
> +	cfg.dst.width = drm_rect_width(&dst);
> +	cfg.dst.height = drm_rect_height(&dst);
>  
>  	for (i = 0; i < state->format->planes; ++i)
>  		cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] R-Car DU: Clip planes to screen boundaries

[Kieran Bingham]

Hi Laurent,

Thankyou for the patches,

On 16/08/17 00:03, Laurent Pinchart wrote:
> Hello,
> 
> This patch series fixes support for planes that cross the screen boundaries.
> The KMS API supports such a configuration, but the DU and VSP hardware
> doesn't. This leads to different kind of dispay artifacts or hangs.
> 
> The series starts with a bit of refactoring to share existing code and make it
> easier to share new code (1/3). Patch 2/3 then reject planes that are fully
> off-screen as they don't work properly and we have currently no use case for
> them. Finally, patch 3/3 clips planes to screen boundaries to fix the main
> issue.
> 
> The patches are based on top of Dave's latest master branch.

Series tested and verified on Salvator-X ES1.0 using the KMS Test suite,
including suspend/resume, in combination with the "v4l: vsp1: Start and stop DRM
pipeline independently of planes" patch.

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

> Laurent Pinchart (3):
>   drm: rcar-du: Share plane atomic check code between Gen2 and Gen3
>   drm: rcar-du: Reject planes located fully off-screen
>   drm: rcar-du: Clip planes to screen boundaries
> 
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 109 ++++++++++++++++++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_plane.h |   8 +++
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  43 ++++---------
>  3 files changed, 105 insertions(+), 55 deletions(-)
>