Re: [PATCH v6] media: imx: add mem2mem device

[Philipp Zabel <p.zabel@pengutronix.de>]

On Wed, 2019-01-16 at 17:19 +0100, Hans Verkuil wrote:
> Hi Philipp,
> 
> A quick review (just a few small points):
> 
> On 1/8/19 4:38 PM, Philipp Zabel wrote:
[...]
> > +/*
> > + * Video ioctls
> > + */
> > +static int ipu_csc_scaler_querycap(struct file *file, void *priv,
> > +				   struct v4l2_capability *cap)
> > +{
> > +	strscpy(cap->driver, "imx-media-mem2mem", sizeof(cap->driver));
> > +	strscpy(cap->card, "imx-media-mem2mem", sizeof(cap->card));
> > +	strscpy(cap->bus_info, "platform:imx-media-mem2mem",
> 
> Please update the names to imx-media-csc-scaler.

Ok, will do.

> > +static int ipu_csc_scaler_g_selection(struct file *file, void *priv,
> > +				      struct v4l2_selection *s)
> > +{
> > +	struct ipu_csc_scaler_ctx *ctx = fh_to_ctx(priv);
> > +	struct ipu_csc_scaler_q_data *q_data;
> > +
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +		if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > +			return -EINVAL;
> > +		q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > +		break;
> > +	case V4L2_SEL_TGT_COMPOSE:
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> 
> I don't think you need to implement COMPOSE_PADDED, unless there
> is actual padding going on.

I'll remove the COMPOSE_PADDED target for now.

Support could be added to scale to non-burstsize-aligned width for some
orientations, but that would have rather complicated interactions with
the tiling and flip/rotation support.

[...]
> > +static int ipu_csc_scaler_init_controls(struct ipu_csc_scaler_ctx *ctx)
> > +{
> > +	struct v4l2_ctrl_handler *hdlr = &ctx->ctrl_hdlr;
> > +	int ret;
> > +
> > +	v4l2_ctrl_handler_init(hdlr, 3);
> > +
> > +	v4l2_ctrl_new_std(hdlr, &ipu_csc_scaler_ctrl_ops, V4L2_CID_HFLIP,
> > +			  0, 1, 1, 0);
> > +	v4l2_ctrl_new_std(hdlr, &ipu_csc_scaler_ctrl_ops, V4L2_CID_VFLIP,
> > +			  0, 1, 1, 0);
> > +	v4l2_ctrl_new_std(hdlr, &ipu_csc_scaler_ctrl_ops, V4L2_CID_ROTATE,
> > +			  0, 270, 90, 0);
> > +
> > +	if (hdlr->error) {
> > +		ret = hdlr->error;
> > +		goto out_free;
> > +	}
> > +
> > +	v4l2_ctrl_handler_setup(hdlr);
> > +	return 0;
> > +
> > +out_free:
> > +	v4l2_ctrl_handler_free(hdlr);
> > +	return ret;
> 
> You don't really need a goto here, just replace the 'goto' with these last two lines.

Ok.

thanks
Philipp