Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
Hi Kamil, Thank you for your comments. On 31 January 2012 15:39, Kamil Debski wrote: > Hi Laurent and Sachin, > >> From: Laurent Pinchart [mailto:laurent.pinch...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
Hi Laurent and Sachin, > From: Laurent Pinchart [mailto:laurent.pinch...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
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 wrote: > Hi Laurent and Sachin, > > Thanks for the patch and for your comments. > >> From: Laurent Pinchart [mailto:laurent.pinch...@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 >> > --- >> > 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
RE: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
Hi Laurent and Sachin, Thanks for the patch and for your comments. > From: Laurent Pinchart [mailto:laurent.pinch...@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 > > --- > > 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_
Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][media] s5p-g2d: Add HFLIP and VFLIP support
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 > --- > 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_frameout; > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kern