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

[Kamil Debski <k.debski@samsung.com>]

Hi Laurent and Sachin,

> From: Laurent Pinchart [mailto:laurent.pinchart@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