[PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Sachin Kamat]

This patch adds support for flipping the image horizontally and vertically.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/video/s5p-g2d/g2d-hw.c |    5 +++
 drivers/media/video/s5p-g2d/g2d.c    |   47 +++++++++++++++++++++++++++------
 drivers/media/video/s5p-g2d/g2d.h    |    3 ++
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/s5p-g2d/g2d-hw.c b/drivers/media/video/s5p-g2d/g2d-hw.c
index 39937cf..5b86cbe 100644
--- a/drivers/media/video/s5p-g2d/g2d-hw.c
+++ b/drivers/media/video/s5p-g2d/g2d-hw.c
@@ -77,6 +77,11 @@ void g2d_set_rop4(struct g2d_dev *d, u32 r)
 	w(r, ROP4_REG);
 }
 
+void g2d_set_flip(struct g2d_dev *d, u32 r)
+{
+	w(r, SRC_MSK_DIRECT_REG);
+}
+
 u32 g2d_cmd_stretch(u32 e)
 {
 	e &= 1;
diff --git a/drivers/media/video/s5p-g2d/g2d.c b/drivers/media/video/s5p-g2d/g2d.c
index febaa67..dea9701 100644
--- a/drivers/media/video/s5p-g2d/g2d.c
+++ b/drivers/media/video/s5p-g2d/g2d.c
@@ -178,6 +178,7 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct g2d_ctx *ctx = container_of(ctrl->handler, struct g2d_ctx,
 								ctrl_handler);
+
 	switch (ctrl->id) {
 	case V4L2_CID_COLORFX:
 		if (ctrl->val == V4L2_COLORFX_NEGATIVE)
@@ -185,6 +186,21 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
 		else
 			ctx->rop = ROP4_COPY;
 		break;
+
+	case V4L2_CID_HFLIP:
+		if (ctrl->val == 1)
+			ctx->hflip = 1;
+		else
+			ctx->hflip = 0;
+		break;
+
+	case V4L2_CID_VFLIP:
+		if (ctrl->val == 1)
+			ctx->vflip = (1 << 1);
+		else
+			ctx->vflip = 0;
+		break;
+
 	default:
 		v4l2_err(&ctx->dev->v4l2_dev, "unknown control\n");
 		return -EINVAL;
@@ -200,11 +216,9 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
 {
 	struct g2d_dev *dev = ctx->dev;
 
-	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 1);
-	if (ctx->ctrl_handler.error) {
-		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
-		return ctx->ctrl_handler.error;
-	}
+	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
+	if (ctx->ctrl_handler.error)
+		goto error;
 
 	v4l2_ctrl_new_std_menu(
 		&ctx->ctrl_handler,
@@ -214,12 +228,25 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
 		~((1 << V4L2_COLORFX_NONE) | (1 << V4L2_COLORFX_NEGATIVE)),
 		V4L2_COLORFX_NONE);
 
-	if (ctx->ctrl_handler.error) {
-		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
-		return ctx->ctrl_handler.error;
-	}
+	if (ctx->ctrl_handler.error)
+		goto error;
+
+	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
+						V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (ctx->ctrl_handler.error)
+		goto error;
+
+	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
+						V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (ctx->ctrl_handler.error)
+		goto error;
 
 	return 0;
+
+error:
+	v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
+	return ctx->ctrl_handler.error;
+
 }
 
 static int g2d_open(struct file *file)
@@ -564,6 +591,8 @@ static void device_run(void *prv)
 	g2d_set_dst_addr(dev, vb2_dma_contig_plane_dma_addr(dst, 0));
 
 	g2d_set_rop4(dev, ctx->rop);
+	g2d_set_flip(dev, ctx->hflip | ctx->vflip);
+
 	if (ctx->in.c_width != ctx->out.c_width ||
 		ctx->in.c_height != ctx->out.c_height)
 		cmd |= g2d_cmd_stretch(1);
diff --git a/drivers/media/video/s5p-g2d/g2d.h b/drivers/media/video/s5p-g2d/g2d.h
index 5eae901..b3be3c8 100644
--- a/drivers/media/video/s5p-g2d/g2d.h
+++ b/drivers/media/video/s5p-g2d/g2d.h
@@ -59,6 +59,8 @@ struct g2d_ctx {
 	struct g2d_frame	out;
 	struct v4l2_ctrl_handler ctrl_handler;
 	u32 rop;
+	u32 hflip;
+	u32 vflip;
 };
 
 struct g2d_fmt {
@@ -77,6 +79,7 @@ void g2d_set_dst_addr(struct g2d_dev *d, dma_addr_t a);
 void g2d_start(struct g2d_dev *d);
 void g2d_clear_int(struct g2d_dev *d);
 void g2d_set_rop4(struct g2d_dev *d, u32 r);
+void g2d_set_flip(struct g2d_dev *d, u32 r);
 u32 g2d_cmd_stretch(u32 e);
 void g2d_set_cmd(struct g2d_dev *d, u32 c);
 
-- 
1.7.4.1

Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Laurent Pinchart]

Hi Sashin,

Thanks for the patch.

On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
> This patch adds support for flipping the image horizontally and vertically.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/media/video/s5p-g2d/g2d-hw.c |    5 +++
>  drivers/media/video/s5p-g2d/g2d.c    |   47 +++++++++++++++++++++++++------
>  drivers/media/video/s5p-g2d/g2d.h    |    3 ++
>  3 files changed, 46 insertions(+), 9 deletions(-)

[snip]

> diff --git a/drivers/media/video/s5p-g2d/g2d.c
> b/drivers/media/video/s5p-g2d/g2d.c index febaa67..dea9701 100644
> --- a/drivers/media/video/s5p-g2d/g2d.c
> +++ b/drivers/media/video/s5p-g2d/g2d.c
> @@ -178,6 +178,7 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct g2d_ctx *ctx = container_of(ctrl->handler, struct g2d_ctx,
>  								ctrl_handler);
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_COLORFX:
>  		if (ctrl->val == V4L2_COLORFX_NEGATIVE)
> @@ -185,6 +186,21 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
>  		else
>  			ctx->rop = ROP4_COPY;
>  		break;
> +
> +	case V4L2_CID_HFLIP:
> +		if (ctrl->val == 1)
> +			ctx->hflip = 1;
> +		else
> +			ctx->hflip = 0;
> +		break;
> +
> +	case V4L2_CID_VFLIP:
> +		if (ctrl->val == 1)
> +			ctx->vflip = (1 << 1);
> +		else
> +			ctx->vflip = 0;
> +		break;
> +
>  	default:
>  		v4l2_err(&ctx->dev->v4l2_dev, "unknown control\n");
>  		return -EINVAL;
> @@ -200,11 +216,9 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
>  {
>  	struct g2d_dev *dev = ctx->dev;
> 
> -	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 1);
> -	if (ctx->ctrl_handler.error) {
> -		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> -		return ctx->ctrl_handler.error;
> -	}
> +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
> +	if (ctx->ctrl_handler.error)
> +		goto error;

There's not need to verify ctx->ctrl_handler.error after every call to 
v4l2_ctrl_handler_init() or v4l2_ctrl_new_*(). You can verify it once only 
after initialization all controls.

> 
>  	v4l2_ctrl_new_std_menu(
>  		&ctx->ctrl_handler,
> @@ -214,12 +228,25 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
>  		~((1 << V4L2_COLORFX_NONE) | (1 << V4L2_COLORFX_NEGATIVE)),
>  		V4L2_COLORFX_NONE);
> 
> -	if (ctx->ctrl_handler.error) {
> -		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> -		return ctx->ctrl_handler.error;
> -	}
> +	if (ctx->ctrl_handler.error)
> +		goto error;
> +
> +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> +						V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (ctx->ctrl_handler.error)
> +		goto error;
> +
> +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> +						V4L2_CID_VFLIP, 0, 1, 1, 0);

As a single register controls hflip and vflip, you should group the two 
controls in a cluster.

> +	if (ctx->ctrl_handler.error)
> +		goto error;
> 
>  	return 0;
> +
> +error:
> +	v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> +	return ctx->ctrl_handler.error;
> +
>  }
> 
>  static int g2d_open(struct file *file)
> @@ -564,6 +591,8 @@ static void device_run(void *prv)
>  	g2d_set_dst_addr(dev, vb2_dma_contig_plane_dma_addr(dst, 0));
> 
>  	g2d_set_rop4(dev, ctx->rop);
> +	g2d_set_flip(dev, ctx->hflip | ctx->vflip);
> +

Is this called for every frame, or once at stream start only ? In the later 
case, this means that hflip and vflip won't be changeable during streaming. Is 
that on purpose ?

>  	if (ctx->in.c_width != ctx->out.c_width ||
>  		ctx->in.c_height != ctx->out.c_height)
>  		cmd |= g2d_cmd_stretch(1);
> diff --git a/drivers/media/video/s5p-g2d/g2d.h
> b/drivers/media/video/s5p-g2d/g2d.h index 5eae901..b3be3c8 100644
> --- a/drivers/media/video/s5p-g2d/g2d.h
> +++ b/drivers/media/video/s5p-g2d/g2d.h
> @@ -59,6 +59,8 @@ struct g2d_ctx {
>  	struct g2d_frame	out;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	u32 rop;
> +	u32 hflip;
> +	u32 vflip;
>  };
> 
>  struct g2d_fmt {
> @@ -77,6 +79,7 @@ void g2d_set_dst_addr(struct g2d_dev *d, dma_addr_t a);
>  void g2d_start(struct g2d_dev *d);
>  void g2d_clear_int(struct g2d_dev *d);
>  void g2d_set_rop4(struct g2d_dev *d, u32 r);
> +void g2d_set_flip(struct g2d_dev *d, u32 r);
>  u32 g2d_cmd_stretch(u32 e);
>  void g2d_set_cmd(struct g2d_dev *d, u32 c);

-- 
Regards,

Laurent Pinchart

Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Sylwester Nawrocki]

On 01/30/2012 01:11 PM, Laurent Pinchart wrote:
>>  static int g2d_open(struct file *file)
>> @@ -564,6 +591,8 @@ static void device_run(void *prv)
>>  	g2d_set_dst_addr(dev, vb2_dma_contig_plane_dma_addr(dst, 0));
>>
>>  	g2d_set_rop4(dev, ctx->rop);
>> +	g2d_set_flip(dev, ctx->hflip | ctx->vflip);
>> +
> 
> Is this called for every frame, or once at stream start only ? In the later 
> case, this means that hflip and vflip won't be changeable during streaming. Is 
> that on purpose ?

The device_run() callback is called per each frame, i.e. per each single
buffer pair. Hence it should be possible to reconfigure flipping when
streaming is on.

Thanks,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

RE: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Kamil Debski]

Hi Laurent and Sachin,

Thanks for the patch and for your comments.

> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: 30 January 2012 13:12
> 
> Hi Sashin,
> 
> Thanks for the patch.
> 
> On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
> > This patch adds support for flipping the image horizontally and vertically.
> >
> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > ---
> >  drivers/media/video/s5p-g2d/g2d-hw.c |    5 +++
> >  drivers/media/video/s5p-g2d/g2d.c    |   47 +++++++++++++++++++++++++----
> --
> >  drivers/media/video/s5p-g2d/g2d.h    |    3 ++
> >  3 files changed, 46 insertions(+), 9 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/video/s5p-g2d/g2d.c
> > b/drivers/media/video/s5p-g2d/g2d.c index febaa67..dea9701 100644
> > --- a/drivers/media/video/s5p-g2d/g2d.c
> > +++ b/drivers/media/video/s5p-g2d/g2d.c
> > @@ -178,6 +178,7 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >  	struct g2d_ctx *ctx = container_of(ctrl->handler, struct g2d_ctx,
> >  								ctrl_handler);
> > +
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_COLORFX:
> >  		if (ctrl->val == V4L2_COLORFX_NEGATIVE)
> > @@ -185,6 +186,21 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		else
> >  			ctx->rop = ROP4_COPY;
> >  		break;
> > +
> > +	case V4L2_CID_HFLIP:
> > +		if (ctrl->val == 1)
> > +			ctx->hflip = 1;
> > +		else
> > +			ctx->hflip = 0;
> > +		break;
> > +
> > +	case V4L2_CID_VFLIP:
> > +		if (ctrl->val == 1)
> > +			ctx->vflip = (1 << 1);
> > +		else
> > +			ctx->vflip = 0;
> > +		break;

I think that 

case V4L2_CID_HFLIP:
	ctx->hflip = ctrl->val;
	break;

case V4L2_CID_VFLIP:
	ctx->vflip = (ctrl->val << 1);
	break;

will be sufficient, as flip controls have min=0 and max=1 which makes
them safe to use.

> > +
> >  	default:
> >  		v4l2_err(&ctx->dev->v4l2_dev, "unknown control\n");
> >  		return -EINVAL;
> > @@ -200,11 +216,9 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
> >  {
> >  	struct g2d_dev *dev = ctx->dev;
> >
> > -	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 1);
> > -	if (ctx->ctrl_handler.error) {
> > -		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> > -		return ctx->ctrl_handler.error;
> > -	}
> > +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
> > +	if (ctx->ctrl_handler.error)
> > +		goto error;
> 
> There's not need to verify ctx->ctrl_handler.error after every call to
> v4l2_ctrl_handler_init() or v4l2_ctrl_new_*(). You can verify it once only
> after initialization all controls.

I agree.
 
> >
> >  	v4l2_ctrl_new_std_menu(
> >  		&ctx->ctrl_handler,
> > @@ -214,12 +228,25 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
> >  		~((1 << V4L2_COLORFX_NONE) | (1 << V4L2_COLORFX_NEGATIVE)),
> >  		V4L2_COLORFX_NONE);
> >
> > -	if (ctx->ctrl_handler.error) {
> > -		v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> > -		return ctx->ctrl_handler.error;
> > -	}
> > +	if (ctx->ctrl_handler.error)
> > +		goto error;
> > +
> > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > +						V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +	if (ctx->ctrl_handler.error)
> > +		goto error;
> > +
> > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > +						V4L2_CID_VFLIP, 0, 1, 1, 0);
> 
> As a single register controls hflip and vflip, you should group the two
> controls in a cluster.

I think it doesn't matter in this use case. As register are not written
in the g2d_s_ctrl. Because the driver uses multiple context it modifies
the appropriate values in its context structure and registers are written
when the transaction is run.

Also there is no logical connection between horizontal and vertical flip.
I think this is the case when using clusters. Here one is independent from
another.

> 
> > +	if (ctx->ctrl_handler.error)
> > +		goto error;
> >
> >  	return 0;
> > +
> > +error:
> > +	v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
> > +	return ctx->ctrl_handler.error;
> > +
> >  }
> >
> >  static int g2d_open(struct file *file)
> > @@ -564,6 +591,8 @@ static void device_run(void *prv)
> >  	g2d_set_dst_addr(dev, vb2_dma_contig_plane_dma_addr(dst, 0));
> >
> >  	g2d_set_rop4(dev, ctx->rop);
> > +	g2d_set_flip(dev, ctx->hflip | ctx->vflip);
> > +
> 
> Is this called for every frame, or once at stream start only ? In the later
> case, this means that hflip and vflip won't be changeable during streaming.
> Is
> that on purpose ?
> 
> >  	if (ctx->in.c_width != ctx->out.c_width ||
> >  		ctx->in.c_height != ctx->out.c_height)
> >  		cmd |= g2d_cmd_stretch(1);
> > diff --git a/drivers/media/video/s5p-g2d/g2d.h
> > b/drivers/media/video/s5p-g2d/g2d.h index 5eae901..b3be3c8 100644
> > --- a/drivers/media/video/s5p-g2d/g2d.h
> > +++ b/drivers/media/video/s5p-g2d/g2d.h
> > @@ -59,6 +59,8 @@ struct g2d_ctx {
> >  	struct g2d_frame	out;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	u32 rop;
> > +	u32 hflip;
> > +	u32 vflip;
> >  };
> >
> >  struct g2d_fmt {
> > @@ -77,6 +79,7 @@ void g2d_set_dst_addr(struct g2d_dev *d, dma_addr_t a);
> >  void g2d_start(struct g2d_dev *d);
> >  void g2d_clear_int(struct g2d_dev *d);
> >  void g2d_set_rop4(struct g2d_dev *d, u32 r);
> > +void g2d_set_flip(struct g2d_dev *d, u32 r);
> >  u32 g2d_cmd_stretch(u32 e);
> >  void g2d_set_cmd(struct g2d_dev *d, u32 c);
> 

Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Sachin Kamat]

Hi Laurent, Sylwester and Kamil,

Thank you for your comments and suggestions.
I will re-send the patch with the following 2 changes:

1. Error checking done only once at the end of the init call.
2. Modification in switch case as suggested by Kamil.

Regards
Sachin

On 30 January 2012 19:09, Kamil Debski <k.debski@samsung.com> wrote:
> Hi Laurent and Sachin,
>
> Thanks for the patch and for your comments.
>
>> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
>> Sent: 30 January 2012 13:12
>>
>> Hi Sashin,
>>
>> Thanks for the patch.
>>
>> On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
>> > This patch adds support for flipping the image horizontally and vertically.
>> >
>> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> > ---
>> >  drivers/media/video/s5p-g2d/g2d-hw.c |    5 +++
>> >  drivers/media/video/s5p-g2d/g2d.c    |   47 +++++++++++++++++++++++++----
>> --
>> >  drivers/media/video/s5p-g2d/g2d.h    |    3 ++
>> >  3 files changed, 46 insertions(+), 9 deletions(-)
>>
>> [snip]
>>
>> > diff --git a/drivers/media/video/s5p-g2d/g2d.c
>> > b/drivers/media/video/s5p-g2d/g2d.c index febaa67..dea9701 100644
>> > --- a/drivers/media/video/s5p-g2d/g2d.c
>> > +++ b/drivers/media/video/s5p-g2d/g2d.c
>> > @@ -178,6 +178,7 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
>> >  {
>> >     struct g2d_ctx *ctx = container_of(ctrl->handler, struct g2d_ctx,
>> >                                                             ctrl_handler);
>> > +
>> >     switch (ctrl->id) {
>> >     case V4L2_CID_COLORFX:
>> >             if (ctrl->val == V4L2_COLORFX_NEGATIVE)
>> > @@ -185,6 +186,21 @@ static int g2d_s_ctrl(struct v4l2_ctrl *ctrl)
>> >             else
>> >                     ctx->rop = ROP4_COPY;
>> >             break;
>> > +
>> > +   case V4L2_CID_HFLIP:
>> > +           if (ctrl->val == 1)
>> > +                   ctx->hflip = 1;
>> > +           else
>> > +                   ctx->hflip = 0;
>> > +           break;
>> > +
>> > +   case V4L2_CID_VFLIP:
>> > +           if (ctrl->val == 1)
>> > +                   ctx->vflip = (1 << 1);
>> > +           else
>> > +                   ctx->vflip = 0;
>> > +           break;
>
> I think that
>
> case V4L2_CID_HFLIP:
>        ctx->hflip = ctrl->val;
>        break;
>
> case V4L2_CID_VFLIP:
>        ctx->vflip = (ctrl->val << 1);
>        break;
>
> will be sufficient, as flip controls have min=0 and max=1 which makes
> them safe to use.
>
>> > +
>> >     default:
>> >             v4l2_err(&ctx->dev->v4l2_dev, "unknown control\n");
>> >             return -EINVAL;
>> > @@ -200,11 +216,9 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
>> >  {
>> >     struct g2d_dev *dev = ctx->dev;
>> >
>> > -   v4l2_ctrl_handler_init(&ctx->ctrl_handler, 1);
>> > -   if (ctx->ctrl_handler.error) {
>> > -           v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
>> > -           return ctx->ctrl_handler.error;
>> > -   }
>> > +   v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
>> > +   if (ctx->ctrl_handler.error)
>> > +           goto error;
>>
>> There's not need to verify ctx->ctrl_handler.error after every call to
>> v4l2_ctrl_handler_init() or v4l2_ctrl_new_*(). You can verify it once only
>> after initialization all controls.
>
> I agree.
>
>> >
>> >     v4l2_ctrl_new_std_menu(
>> >             &ctx->ctrl_handler,
>> > @@ -214,12 +228,25 @@ int g2d_setup_ctrls(struct g2d_ctx *ctx)
>> >             ~((1 << V4L2_COLORFX_NONE) | (1 << V4L2_COLORFX_NEGATIVE)),
>> >             V4L2_COLORFX_NONE);
>> >
>> > -   if (ctx->ctrl_handler.error) {
>> > -           v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
>> > -           return ctx->ctrl_handler.error;
>> > -   }
>> > +   if (ctx->ctrl_handler.error)
>> > +           goto error;
>> > +
>> > +   v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
>> > +                                           V4L2_CID_HFLIP, 0, 1, 1, 0);
>> > +   if (ctx->ctrl_handler.error)
>> > +           goto error;
>> > +
>> > +   v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
>> > +                                           V4L2_CID_VFLIP, 0, 1, 1, 0);
>>
>> As a single register controls hflip and vflip, you should group the two
>> controls in a cluster.
>
> I think it doesn't matter in this use case. As register are not written
> in the g2d_s_ctrl. Because the driver uses multiple context it modifies
> the appropriate values in its context structure and registers are written
> when the transaction is run.
>
> Also there is no logical connection between horizontal and vertical flip.
> I think this is the case when using clusters. Here one is independent from
> another.
>
>>
>> > +   if (ctx->ctrl_handler.error)
>> > +           goto error;
>> >
>> >     return 0;
>> > +
>> > +error:
>> > +   v4l2_err(&dev->v4l2_dev, "v4l2_ctrl_handler_init failed\n");
>> > +   return ctx->ctrl_handler.error;
>> > +
>> >  }
>> >
>> >  static int g2d_open(struct file *file)
>> > @@ -564,6 +591,8 @@ static void device_run(void *prv)
>> >     g2d_set_dst_addr(dev, vb2_dma_contig_plane_dma_addr(dst, 0));
>> >
>> >     g2d_set_rop4(dev, ctx->rop);
>> > +   g2d_set_flip(dev, ctx->hflip | ctx->vflip);
>> > +
>>
>> Is this called for every frame, or once at stream start only ? In the later
>> case, this means that hflip and vflip won't be changeable during streaming.
>> Is
>> that on purpose ?
>>
>> >     if (ctx->in.c_width != ctx->out.c_width ||
>> >             ctx->in.c_height != ctx->out.c_height)
>> >             cmd |= g2d_cmd_stretch(1);
>> > diff --git a/drivers/media/video/s5p-g2d/g2d.h
>> > b/drivers/media/video/s5p-g2d/g2d.h index 5eae901..b3be3c8 100644
>> > --- a/drivers/media/video/s5p-g2d/g2d.h
>> > +++ b/drivers/media/video/s5p-g2d/g2d.h
>> > @@ -59,6 +59,8 @@ struct g2d_ctx {
>> >     struct g2d_frame        out;
>> >     struct v4l2_ctrl_handler ctrl_handler;
>> >     u32 rop;
>> > +   u32 hflip;
>> > +   u32 vflip;
>> >  };
>> >
>> >  struct g2d_fmt {
>> > @@ -77,6 +79,7 @@ void g2d_set_dst_addr(struct g2d_dev *d, dma_addr_t a);
>> >  void g2d_start(struct g2d_dev *d);
>> >  void g2d_clear_int(struct g2d_dev *d);
>> >  void g2d_set_rop4(struct g2d_dev *d, u32 r);
>> > +void g2d_set_flip(struct g2d_dev *d, u32 r);
>> >  u32 g2d_cmd_stretch(u32 e);
>> >  void g2d_set_cmd(struct g2d_dev *d, u32 c);
>>
>
> Best wishes,
> --
> Kamil Debski
> Linux Platform Group
> Samsung Poland R&D Center
>



-- 
With warm regards,
Sachin

Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Laurent Pinchart]

Hi Kamil,

On Monday 30 January 2012 14:39:22 Kamil Debski wrote:
> On 30 January 2012 13:12 Laurent Pinchart wrote:
> > On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
> > > This patch adds support for flipping the image horizontally and
> > > vertically.

[snip]

> > > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > > +						V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > +	if (ctx->ctrl_handler.error)
> > > +		goto error;
> > > +
> > > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > > +						V4L2_CID_VFLIP, 0, 1, 1, 0);
> > 
> > As a single register controls hflip and vflip, you should group the two
> > controls in a cluster.
> 
> I think it doesn't matter in this use case. As register are not written
> in the g2d_s_ctrl. Because the driver uses multiple context it modifies
> the appropriate values in its context structure and registers are written
> when the transaction is run.
> 
> Also there is no logical connection between horizontal and vertical flip.
> I think this is the case when using clusters. Here one is independent from
> another.

As the value is only written to hardware registers later, not in the s_ctrl() 
handler, a cluster is (probably) not mandatory if the driver uses proper 
locking. Otherwise there will be no guarantee that setting both hflip and 
vflip in a single VIDIOC_S_EXT_CTRLS call will not result in one frame with 
only hflip or vflip applied.

-- 
Regards,

Laurent Pinchart

RE: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Kamil Debski]

Hi Laurent and Sachin,

> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: 31 January 2012 10:30
> 
> Hi Kamil,
> 
> On Monday 30 January 2012 14:39:22 Kamil Debski wrote:
> > On 30 January 2012 13:12 Laurent Pinchart wrote:
> > > On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
> > > > This patch adds support for flipping the image horizontally and
> > > > vertically.
> 
> [snip]
> 
> > > > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > > > +						V4L2_CID_HFLIP, 0, 1, 1,
0);
> > > > +	if (ctx->ctrl_handler.error)
> > > > +		goto error;
> > > > +
> > > > +	v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
> > > > +						V4L2_CID_VFLIP, 0, 1, 1,
0);
> > >
> > > As a single register controls hflip and vflip, you should group the two
> > > controls in a cluster.
> >
> > I think it doesn't matter in this use case. As register are not written
> > in the g2d_s_ctrl. Because the driver uses multiple context it modifies
> > the appropriate values in its context structure and registers are written
> > when the transaction is run.
> >
> > Also there is no logical connection between horizontal and vertical flip.
> > I think this is the case when using clusters. Here one is independent from
> > another.
> 
> As the value is only written to hardware registers later, not in the s_ctrl()
> handler, a cluster is (probably) not mandatory if the driver uses proper
> locking. Otherwise there will be no guarantee that setting both hflip and
> vflip in a single VIDIOC_S_EXT_CTRLS call will not result in one frame with
> only hflip or vflip applied.

I see your point - this could happen. So Sachin - I think you need to add the
cluster.
You can find documentation about this in
Documentation/video4linux/v4l2-controls.txt

Also I have talked with Sylwester about locking. It turns out that a spinlock in
device_run and s_ctrl is necessary. I'll add it after you send your patch,
Sachin.

Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support

[Sachin Kamat]

Hi Kamil,

Thank you for your comments.

On 31 January 2012 15:39, Kamil Debski <k.debski@samsung.com> wrote:
> Hi Laurent and Sachin,
>
>> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
>> Sent: 31 January 2012 10:30
>>
>> Hi Kamil,
>>
>> On Monday 30 January 2012 14:39:22 Kamil Debski wrote:
>> > On 30 January 2012 13:12 Laurent Pinchart wrote:
>> > > On Monday 30 January 2012 10:58:43 Sachin Kamat wrote:
>> > > > This patch adds support for flipping the image horizontally and
>> > > > vertically.
>>
>> [snip]
>>
>> > > > +       v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
>> > > > +                                               V4L2_CID_HFLIP, 0, 1, 1,
> 0);
>> > > > +       if (ctx->ctrl_handler.error)
>> > > > +               goto error;
>> > > > +
>> > > > +       v4l2_ctrl_new_std(&ctx->ctrl_handler, &g2d_ctrl_ops,
>> > > > +                                               V4L2_CID_VFLIP, 0, 1, 1,
> 0);
>> > >
>> > > As a single register controls hflip and vflip, you should group the two
>> > > controls in a cluster.
>> >
>> > I think it doesn't matter in this use case. As register are not written
>> > in the g2d_s_ctrl. Because the driver uses multiple context it modifies
>> > the appropriate values in its context structure and registers are written
>> > when the transaction is run.
>> >
>> > Also there is no logical connection between horizontal and vertical flip.
>> > I think this is the case when using clusters. Here one is independent from
>> > another.
>>
>> As the value is only written to hardware registers later, not in the s_ctrl()
>> handler, a cluster is (probably) not mandatory if the driver uses proper
>> locking. Otherwise there will be no guarantee that setting both hflip and
>> vflip in a single VIDIOC_S_EXT_CTRLS call will not result in one frame with
>> only hflip or vflip applied.
>
> I see your point - this could happen. So Sachin - I think you need to add the
> cluster.
> You can find documentation about this in
> Documentation/video4linux/v4l2-controls.txt

OK. I will add this and submit the patch again.

>
> Also I have talked with Sylwester about locking. It turns out that a spinlock in
> device_run and s_ctrl is necessary. I'll add it after you send your patch,
> Sachin.
>
> Best wishes,
> --
> Kamil Debski
> Linux Platform Group
> Samsung Poland R&D Center
>



-- 
With warm regards,
Sachin