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

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

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