[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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 <k.debski@xxxxxxxxxxx> wrote:
> 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
>



-- 
With warm regards,
Sachin
--
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


[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux