diff mbox

[v4,09/21] modetest: Allow specifying plane position

Message ID 1363704962-14077-10-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart March 19, 2013, 2:55 p.m. UTC
Extend the -P option to allow specifying the plane x and y offsets. The
position is optional, if not specified the plane will be positioned at
the center of the screen as before.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/modetest.c | 72 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 15 deletions(-)

Comments

archit taneja March 25, 2013, 6:14 a.m. UTC | #1
Hi Laurent,

On Tuesday 19 March 2013 08:25 PM, Laurent Pinchart wrote:
> Extend the -P option to allow specifying the plane x and y offsets. The
> position is optional, if not specified the plane will be positioned at
> the center of the screen as before.

Thanks for this series. I tested the patches with a Panda ES board.

I was facing issues with the plane position though, when I execute this 
on the command line:

./modetest -s 12:1440x900 -P 6:(0,0)/300x200

I get a syntax error by bash saying it doesn't expect "(". I guess there 
are ways around to get over this, but I was wondering if we could get 
rid of the braces all together to keep it simple? The "/" character 
could be used to figure out whether the user has also mentioned position 
or not.

Archit

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   tests/modetest/modetest.c | 72 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index 7153a40..f95efe6 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c
> @@ -645,6 +645,7 @@ struct connector_arg {
>
>   struct plane_arg {
>   	uint32_t con_id;  /* the id of connector to bind to */
> +	uint32_t x, y;
>   	uint32_t w, h;
>   	unsigned int fb_id;
>   	char format_str[5]; /* need to leave room for terminating \0 */
> @@ -855,11 +856,16 @@ set_plane(struct kms_driver *kms, struct connector_arg *c, struct plane_arg *p)
>   		return -1;
>   	}
>
> -	/* ok, boring.. but for now put in middle of screen: */
> -	crtc_x = c->mode->hdisplay / 3;
> -	crtc_y = c->mode->vdisplay / 3;
> -	crtc_w = crtc_x;
> -	crtc_h = crtc_y;
> +	if (p->x == (uint32_t)-1 || p->y == (uint32_t)-1) {
> +		/* Default to the middle of the screen */
> +		crtc_x = (c->mode->hdisplay - p->w) / 2;
> +		crtc_y = (c->mode->vdisplay - p->h) / 2;
> +	} else {
> +		crtc_x = p->x;
> +		crtc_y = p->y;
> +	}
> +	crtc_w = p->w;
> +	crtc_h = p->h;
>
>   	/* note src coords (last 4 args) are in Q16 format */
>   	if (drmModeSetPlane(fd, plane_id, c->crtc, p->fb_id,
> @@ -870,6 +876,8 @@ set_plane(struct kms_driver *kms, struct connector_arg *c, struct plane_arg *p)
>   		return -1;
>   	}
>
> +	ovr->crtc_id = c->crtc;
> +
>   	return 0;
>   }
>
> @@ -1063,18 +1071,52 @@ static int parse_connector(struct connector_arg *c, const char *arg)
>   	return 0;
>   }
>
> -static int parse_plane(struct plane_arg *p, const char *arg)
> +static int parse_plane(struct plane_arg *plane, const char *p)
>   {
> -	strcpy(p->format_str, "XR24");
> +	char *end;
> +
> +	plane->con_id = strtoul(p, &end, 10);
> +	if (*end != ':')
> +		return -EINVAL;
> +
> +	p = end + 1;
> +	if (*p == '(') {
> +		plane->x = strtoul(p + 1, &end, 10);
> +		if (*end != ',')
> +			return -EINVAL;
> +		p = end + 1;
> +		plane->y = strtoul(p, &end, 10);
> +		if (*end++ != ')')
> +			return -EINVAL;
> +		if (*end++ != '/')
> +			return -EINVAL;
> +		p = end;
> +	} else {
> +		plane->x = (uint32_t)-1;
> +		plane->y = (uint32_t)-1;
> +	}
>
> -	if (sscanf(arg, "%d:%dx%d@%4s", &p->con_id, &p->w, &p->h, p->format_str) != 4 &&
> -	    sscanf(arg, "%d:%dx%d", &p->con_id, &p->w, &p->h) != 3)
> -		return -1;
> +	plane->w = strtoul(p, &end, 10);
> +	if (*end != 'x')
> +		return -EINVAL;
>
> -	p->fourcc = format_fourcc(p->format_str);
> -	if (p->fourcc == 0) {
> -		fprintf(stderr, "unknown format %s\n", p->format_str);
> -		return -1;
> +	p = end + 1;
> +	plane->h = strtoul(p, &end, 10);
> +
> +	if (*end == '@') {
> +		p = end + 1;
> +		if (strlen(p) != 4)
> +			return -EINVAL;
> +
> +		strcpy(plane->format_str, p);
> +	} else {
> +		strcpy(plane->format_str, "XR24");
> +	}
> +
> +	plane->fourcc = format_fourcc(plane->format_str);
> +	if (plane->fourcc == 0) {
> +		fprintf(stderr, "unknown format %s\n", plane->format_str);
> +		return -EINVAL;
>   	}
>
>   	return 0;
> @@ -1103,7 +1145,7 @@ static void usage(char *name)
>   	fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
>
>   	fprintf(stderr, "\n Test options:\n\n");
> -	fprintf(stderr, "\t-P <connector_id>:<w>x<h>[@<format>]\tset a plane\n");
> +	fprintf(stderr, "\t-P <connector_id>:[(x,y)/]<w>x<h>[@<format>]\tset a plane\n");
>   	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
>   	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
>   	fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n");
>
Laurent Pinchart March 25, 2013, 10:41 p.m. UTC | #2
Hi Archit,

On Monday 25 March 2013 11:44:35 Archit Taneja wrote:
> Hi Laurent,
> 
> On Tuesday 19 March 2013 08:25 PM, Laurent Pinchart wrote:
> > Extend the -P option to allow specifying the plane x and y offsets. The
> > position is optional, if not specified the plane will be positioned at
> > the center of the screen as before.
> 
> Thanks for this series. I tested the patches with a Panda ES board.
> 
> I was facing issues with the plane position though, when I execute this
> on the command line:
> 
> ./modetest -s 12:1440x900 -P 6:(0,0)/300x200
> 
> I get a syntax error by bash saying it doesn't expect "(". I guess there
> are ways around to get over this,

I use

./modetest -s 12:1440x900 -P '6:(0,0)/300x200'

> but I was wondering if we could get rid of the braces all together to keep
> it simple? The "/" character could be used to figure out whether the user
> has also mentioned position or not.

It makes parsing the option a bit more complex, but I can do that if you think 
it's better.
archit taneja March 26, 2013, 8:44 a.m. UTC | #3
On Tuesday 26 March 2013 04:11 AM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Monday 25 March 2013 11:44:35 Archit Taneja wrote:
>> Hi Laurent,
>>
>> On Tuesday 19 March 2013 08:25 PM, Laurent Pinchart wrote:
>>> Extend the -P option to allow specifying the plane x and y offsets. The
>>> position is optional, if not specified the plane will be positioned at
>>> the center of the screen as before.
>>
>> Thanks for this series. I tested the patches with a Panda ES board.
>>
>> I was facing issues with the plane position though, when I execute this
>> on the command line:
>>
>> ./modetest -s 12:1440x900 -P 6:(0,0)/300x200
>>
>> I get a syntax error by bash saying it doesn't expect "(". I guess there
>> are ways around to get over this,
>
> I use
>
> ./modetest -s 12:1440x900 -P '6:(0,0)/300x200'
>
>> but I was wondering if we could get rid of the braces all together to keep
>> it simple? The "/" character could be used to figure out whether the user
>> has also mentioned position or not.
>
> It makes parsing the option a bit more complex, but I can do that if you think
> it's better.

I think it's fine. After googling a bit on the syntax error issue, I 
thought that putting the command in a script file was the only option, 
but your method above is convenient enough.

Thanks,
Archit
Ville Syrjälä March 27, 2013, 3:57 p.m. UTC | #4
On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> Extend the -P option to allow specifying the plane x and y offsets. The
> position is optional, if not specified the plane will be positioned at
> the center of the screen as before.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  tests/modetest/modetest.c | 72 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index 7153a40..f95efe6 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c
> @@ -645,6 +645,7 @@ struct connector_arg {
>  
>  struct plane_arg {
>  	uint32_t con_id;  /* the id of connector to bind to */
> +	uint32_t x, y;

I'd like the coordinates to allow negative values too. I just posted my
latest version of the plane clipping patches. The main feature there is
to allow coordinates to be partially/fully offscreen and the driver will
clip then the plane to the screen dimensions. Hence negative coordinates
become somewhat useful.

And if you're looking into plane stuff in general, maybe you can review
my patches? The drm_rect utility functions should be useful for all
drivers that have to deal with movable or scalable planes.

Now I'll see about trying this stuff out with my latest patches.

<snip>
Ville Syrjälä March 27, 2013, 5:15 p.m. UTC | #5
On Wed, Mar 27, 2013 at 05:57:20PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > Extend the -P option to allow specifying the plane x and y offsets. The
> > position is optional, if not specified the plane will be positioned at
> > the center of the screen as before.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  tests/modetest/modetest.c | 72 +++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 57 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > index 7153a40..f95efe6 100644
> > --- a/tests/modetest/modetest.c
> > +++ b/tests/modetest/modetest.c
> > @@ -645,6 +645,7 @@ struct connector_arg {
> >  
> >  struct plane_arg {
> >  	uint32_t con_id;  /* the id of connector to bind to */
> > +	uint32_t x, y;
> 
> I'd like the coordinates to allow negative values too.

Tested it and it actually works w/ negative values thanks to the way
strtoul() works :) The only real obstacle is the magic '-1' handling.
I guess you should just give up on magic values and add some flag to
indicate whether the user provided the coords or not.

Also I must say that I don't like the syntax you used for specifying the
coords. '(' and ')' need to be escaped or the shell eats them. I've been
using the x11 -geometry syntax whenever I have to deal with the x/y/w/h
combination. It's a reasonably well known syntax and doesn't have these
shell issues. Maybe you could use it here as well.
Laurent Pinchart March 28, 2013, 12:16 a.m. UTC | #6
Hi Ville,

On Wednesday 27 March 2013 19:15:31 Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 05:57:20PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > > Extend the -P option to allow specifying the plane x and y offsets. The
> > > position is optional, if not specified the plane will be positioned at
> > > the center of the screen as before.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  tests/modetest/modetest.c | 72 ++++++++++++++++++++++++++++++----------
> > >  1 file changed, 57 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > index 7153a40..f95efe6 100644
> > > --- a/tests/modetest/modetest.c
> > > +++ b/tests/modetest/modetest.c
> > > @@ -645,6 +645,7 @@ struct connector_arg {
> > > 
> > >  struct plane_arg {
> > >  
> > >  	uint32_t con_id;  /* the id of connector to bind to */
> > > 
> > > +	uint32_t x, y;
> > 
> > I'd like the coordinates to allow negative values too.
> 
> Tested it and it actually works w/ negative values thanks to the way
> strtoul() works :) The only real obstacle is the magic '-1' handling.
> I guess you should just give up on magic values and add some flag to
> indicate whether the user provided the coords or not.
> 
> Also I must say that I don't like the syntax you used for specifying the
> coords. '(' and ')' need to be escaped or the shell eats them.

You're not the first one to complain, I don't mind changing the syntax 
(although escaping is not mandatory, you can just enclose the whole argument 
in quotes).

> I've been using the x11 -geometry syntax whenever I have to deal with the
> x/y/w/h combination. It's a reasonably well known syntax and doesn't have
> these shell issues. Maybe you could use it here as well.

The issue with the geometry syntax is that you can't put the top-left corner 
at negative coordinates, as -XOFF places the right edge of the plane XOFF 
pixels from the right edge of the screen, and similarly for -YOFF. Should we 
deviate from that spec and consider -XOFF to mean XOFF pixels on the left side 
of the left edge (outside of the screen) ?
Laurent Pinchart March 28, 2013, 12:17 a.m. UTC | #7
Hi Ville,

On Wednesday 27 March 2013 17:57:20 Ville Syrjälä wrote:
> On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > Extend the -P option to allow specifying the plane x and y offsets. The
> > position is optional, if not specified the plane will be positioned at
> > the center of the screen as before.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  tests/modetest/modetest.c | 72 ++++++++++++++++++++++++++++++++----------
> >  1 file changed, 57 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > index 7153a40..f95efe6 100644
> > --- a/tests/modetest/modetest.c
> > +++ b/tests/modetest/modetest.c
> > @@ -645,6 +645,7 @@ struct connector_arg {
> > 
> >  struct plane_arg {
> >  	uint32_t con_id;  /* the id of connector to bind to */
> > +	uint32_t x, y;
> 
> I'd like the coordinates to allow negative values too. I just posted my
> latest version of the plane clipping patches. The main feature there is
> to allow coordinates to be partially/fully offscreen and the driver will
> clip then the plane to the screen dimensions. Hence negative coordinates
> become somewhat useful.
> 
> And if you're looking into plane stuff in general, maybe you can review
> my patches? The drm_rect utility functions should be useful for all
> drivers that have to deal with movable or scalable planes.

I'll try to, but I'm very busy nowadays with pinctrl patches :-/

> Now I'll see about trying this stuff out with my latest patches.
Ville Syrjälä March 28, 2013, 10:44 a.m. UTC | #8
On Thu, Mar 28, 2013 at 01:16:09AM +0100, Laurent Pinchart wrote:
> Hi Ville,
> 
> On Wednesday 27 March 2013 19:15:31 Ville Syrjälä wrote:
> > On Wed, Mar 27, 2013 at 05:57:20PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > > > Extend the -P option to allow specifying the plane x and y offsets. The
> > > > position is optional, if not specified the plane will be positioned at
> > > > the center of the screen as before.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  tests/modetest/modetest.c | 72 ++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 57 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > > index 7153a40..f95efe6 100644
> > > > --- a/tests/modetest/modetest.c
> > > > +++ b/tests/modetest/modetest.c
> > > > @@ -645,6 +645,7 @@ struct connector_arg {
> > > > 
> > > >  struct plane_arg {
> > > >  
> > > >  	uint32_t con_id;  /* the id of connector to bind to */
> > > > 
> > > > +	uint32_t x, y;
> > > 
> > > I'd like the coordinates to allow negative values too.
> > 
> > Tested it and it actually works w/ negative values thanks to the way
> > strtoul() works :) The only real obstacle is the magic '-1' handling.
> > I guess you should just give up on magic values and add some flag to
> > indicate whether the user provided the coords or not.
> > 
> > Also I must say that I don't like the syntax you used for specifying the
> > coords. '(' and ')' need to be escaped or the shell eats them.
> 
> You're not the first one to complain, I don't mind changing the syntax 
> (although escaping is not mandatory, you can just enclose the whole argument 
> in quotes).
> 
> > I've been using the x11 -geometry syntax whenever I have to deal with the
> > x/y/w/h combination. It's a reasonably well known syntax and doesn't have
> > these shell issues. Maybe you could use it here as well.
> 
> The issue with the geometry syntax is that you can't put the top-left corner 
> at negative coordinates, as -XOFF places the right edge of the plane XOFF 
> pixels from the right edge of the screen, and similarly for -YOFF. Should we 
> deviate from that spec and consider -XOFF to mean XOFF pixels on the left side 
> of the left edge (outside of the screen) ?

I forgot that there's this kind of magic change of origin in the
specification. I guess it's been too long since I've used negative
geometry coordinates under X.

I've just been using it so that the origin is always the top left
corner. That's how I chose to interpret things in my
drm_rect_debug_print() function for example. But since it's a bit
contrary to the X geometry spec, maybe it's not the best syntax
either. If anyone has a better idea in mind, I'm open to suggestions.
Laurent Pinchart April 4, 2013, 4:24 p.m. UTC | #9
Hi Ville,

On Wednesday 27 March 2013 19:15:31 Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 05:57:20PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > > Extend the -P option to allow specifying the plane x and y offsets. The
> > > position is optional, if not specified the plane will be positioned at
> > > the center of the screen as before.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  tests/modetest/modetest.c | 72 ++++++++++++++++++++++++++++++----------
> > >  1 file changed, 57 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > index 7153a40..f95efe6 100644
> > > --- a/tests/modetest/modetest.c
> > > +++ b/tests/modetest/modetest.c
> > > @@ -645,6 +645,7 @@ struct connector_arg {
> > > 
> > >  struct plane_arg {
> > >  
> > >  	uint32_t con_id;  /* the id of connector to bind to */
> > > 
> > > +	uint32_t x, y;
> > 
> > I'd like the coordinates to allow negative values too.
> 
> Tested it and it actually works w/ negative values thanks to the way
> strtoul() works :) The only real obstacle is the magic '-1' handling.
> I guess you should just give up on magic values and add some flag to
> indicate whether the user provided the coords or not.

Done :-) I'll post a new version.

> Also I must say that I don't like the syntax you used for specifying the
> coords. '(' and ')' need to be escaped or the shell eats them. I've been
> using the x11 -geometry syntax whenever I have to deal with the x/y/w/h
> combination. It's a reasonably well known syntax and doesn't have these
> shell issues. Maybe you could use it here as well.

Given that negative coordinates in the X geometry syntax don't match we what 
need, should I keep the current syntax, abuse the X geometry syntax, or use 
something else ?
Ville Syrjälä April 5, 2013, 3:15 p.m. UTC | #10
On Thu, Apr 04, 2013 at 06:24:24PM +0200, Laurent Pinchart wrote:
> Hi Ville,
> 
> On Wednesday 27 March 2013 19:15:31 Ville Syrjälä wrote:
> > On Wed, Mar 27, 2013 at 05:57:20PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 19, 2013 at 03:55:50PM +0100, Laurent Pinchart wrote:
> > > > Extend the -P option to allow specifying the plane x and y offsets. The
> > > > position is optional, if not specified the plane will be positioned at
> > > > the center of the screen as before.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  tests/modetest/modetest.c | 72 ++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 57 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > > index 7153a40..f95efe6 100644
> > > > --- a/tests/modetest/modetest.c
> > > > +++ b/tests/modetest/modetest.c
> > > > @@ -645,6 +645,7 @@ struct connector_arg {
> > > > 
> > > >  struct plane_arg {
> > > >  
> > > >  	uint32_t con_id;  /* the id of connector to bind to */
> > > > 
> > > > +	uint32_t x, y;
> > > 
> > > I'd like the coordinates to allow negative values too.
> > 
> > Tested it and it actually works w/ negative values thanks to the way
> > strtoul() works :) The only real obstacle is the magic '-1' handling.
> > I guess you should just give up on magic values and add some flag to
> > indicate whether the user provided the coords or not.
> 
> Done :-) I'll post a new version.
> 
> > Also I must say that I don't like the syntax you used for specifying the
> > coords. '(' and ')' need to be escaped or the shell eats them. I've been
> > using the x11 -geometry syntax whenever I have to deal with the x/y/w/h
> > combination. It's a reasonably well known syntax and doesn't have these
> > shell issues. Maybe you could use it here as well.
> 
> Given that negative coordinates in the X geometry syntax don't match we what 
> need, should I keep the current syntax, abuse the X geometry syntax, or use 
> something else ?

Since no-one suggeste anything better, I vote for the abuse. I've
already gotten used to it :)
diff mbox

Patch

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 7153a40..f95efe6 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -645,6 +645,7 @@  struct connector_arg {
 
 struct plane_arg {
 	uint32_t con_id;  /* the id of connector to bind to */
+	uint32_t x, y;
 	uint32_t w, h;
 	unsigned int fb_id;
 	char format_str[5]; /* need to leave room for terminating \0 */
@@ -855,11 +856,16 @@  set_plane(struct kms_driver *kms, struct connector_arg *c, struct plane_arg *p)
 		return -1;
 	}
 
-	/* ok, boring.. but for now put in middle of screen: */
-	crtc_x = c->mode->hdisplay / 3;
-	crtc_y = c->mode->vdisplay / 3;
-	crtc_w = crtc_x;
-	crtc_h = crtc_y;
+	if (p->x == (uint32_t)-1 || p->y == (uint32_t)-1) {
+		/* Default to the middle of the screen */
+		crtc_x = (c->mode->hdisplay - p->w) / 2;
+		crtc_y = (c->mode->vdisplay - p->h) / 2;
+	} else {
+		crtc_x = p->x;
+		crtc_y = p->y;
+	}
+	crtc_w = p->w;
+	crtc_h = p->h;
 
 	/* note src coords (last 4 args) are in Q16 format */
 	if (drmModeSetPlane(fd, plane_id, c->crtc, p->fb_id,
@@ -870,6 +876,8 @@  set_plane(struct kms_driver *kms, struct connector_arg *c, struct plane_arg *p)
 		return -1;
 	}
 
+	ovr->crtc_id = c->crtc;
+
 	return 0;
 }
 
@@ -1063,18 +1071,52 @@  static int parse_connector(struct connector_arg *c, const char *arg)
 	return 0;
 }
 
-static int parse_plane(struct plane_arg *p, const char *arg)
+static int parse_plane(struct plane_arg *plane, const char *p)
 {
-	strcpy(p->format_str, "XR24");
+	char *end;
+
+	plane->con_id = strtoul(p, &end, 10);
+	if (*end != ':')
+		return -EINVAL;
+
+	p = end + 1;
+	if (*p == '(') {
+		plane->x = strtoul(p + 1, &end, 10);
+		if (*end != ',')
+			return -EINVAL;
+		p = end + 1;
+		plane->y = strtoul(p, &end, 10);
+		if (*end++ != ')')
+			return -EINVAL;
+		if (*end++ != '/')
+			return -EINVAL;
+		p = end;
+	} else {
+		plane->x = (uint32_t)-1;
+		plane->y = (uint32_t)-1;
+	}
 
-	if (sscanf(arg, "%d:%dx%d@%4s", &p->con_id, &p->w, &p->h, p->format_str) != 4 &&
-	    sscanf(arg, "%d:%dx%d", &p->con_id, &p->w, &p->h) != 3)
-		return -1;
+	plane->w = strtoul(p, &end, 10);
+	if (*end != 'x')
+		return -EINVAL;
 
-	p->fourcc = format_fourcc(p->format_str);
-	if (p->fourcc == 0) {
-		fprintf(stderr, "unknown format %s\n", p->format_str);
-		return -1;
+	p = end + 1;
+	plane->h = strtoul(p, &end, 10);
+
+	if (*end == '@') {
+		p = end + 1;
+		if (strlen(p) != 4)
+			return -EINVAL;
+
+		strcpy(plane->format_str, p);
+	} else {
+		strcpy(plane->format_str, "XR24");
+	}
+
+	plane->fourcc = format_fourcc(plane->format_str);
+	if (plane->fourcc == 0) {
+		fprintf(stderr, "unknown format %s\n", plane->format_str);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -1103,7 +1145,7 @@  static void usage(char *name)
 	fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
 
 	fprintf(stderr, "\n Test options:\n\n");
-	fprintf(stderr, "\t-P <connector_id>:<w>x<h>[@<format>]\tset a plane\n");
+	fprintf(stderr, "\t-P <connector_id>:[(x,y)/]<w>x<h>[@<format>]\tset a plane\n");
 	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
 	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
 	fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n");