Hi Laurent and Sachin, Thanks for the patch and for your comments. > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > 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@xxxxxxxxxx> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
