Re: [PATCH v3 1/3] media: imx: add capture compose rectangle

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

Hi Hans,

thank you for the review.

On Wed, 2019-01-16 at 16:29 +0100, Hans Verkuil wrote:
[...]
> @@ -290,6 +294,34 @@ static int capture_s_std(struct file *file, void *fh, v4l2_std_id std)
> >  	return v4l2_subdev_call(priv->src_sd, video, s_std, std);
> >  }
> >  
> > +static int capture_g_selection(struct file *file, void *fh,
> > +			       struct v4l2_selection *s)
> > +{
> > +	struct capture_priv *priv = video_drvdata(file);
> > +
> > +	switch (s->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> 
> The crop rectangle is equal to the compose rectangle? That can't be right.
> Does the hardware support cropping at all? Probably not, and in that case
> you shouldn't support the crop target at all.

Indeed the hardware does not support cropping at the DMA controller,
that has to be done in the CSI subdev already. I'll drop the CROP
targets.

> > +	case V4L2_SEL_TGT_COMPOSE:
> > +	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > +	case V4L2_SEL_TGT_COMPOSE_PADDED:
> > +		s->r = priv->vdev.compose;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int capture_s_selection(struct file *file, void *fh,
> > +			       struct v4l2_selection *s)
> > +{
> > +	return capture_g_selection(file, fh, s);
> > +}
> 
> Don't implement s_selection unless you can actually change the selection
> rectangle(s).

Ok, I'll only support g_selection for now. The main purpose of this
series is to allow capturing non-burstsize aligned widths at the CSI,
which are written padded to burst size alignement, and to report the
valid pixels to userspace via COMPOSE_DEFAULT rectangle.

regards
Philipp