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
