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

[Sachin Kamat <sachin.kamat@xxxxxxxxxx>]

Hi Kamil,

Thank you for your comments.

On 31 January 2012 15:39, Kamil Debski <k.debski@xxxxxxxxxxx> wrote:
> Hi Laurent and Sachin,
>
>> From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx]
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html