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 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
