diff mbox

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

Message ID 1327917523-29836-1-git-send-email-sachin.kamat@linaro.org
State Superseded
Headers show

Commit Message

Sachin Kamat Jan. 30, 2012, 9:58 a.m. UTC
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(-)

Comments

Laurent Pinchart Jan. 30, 2012, 12:11 p.m. UTC | #1
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);
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,
Kamil Debski Jan. 30, 2012, 1:39 p.m. UTC | #3
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
Sachin Kamat Jan. 31, 2012, 5:07 a.m. UTC | #4
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
>
diff mbox

Patch

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);