On Mon, 2017-02-13 at 11:05 +0100, Hans Verkuil wrote:
[...]
> > @@ -1469,38 +1477,88 @@ static int tc358743_s_stream(struct v4l2_subdev *sd, int enable)
> >
> > /* --------------- PAD OPS --------------- */
> >
> > -static int tc358743_get_fmt(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > - struct v4l2_subdev_format *format)
> > +static void tc358743_get_csi_color_space(struct v4l2_subdev *sd,
> > + struct v4l2_mbus_framefmt *format)
> > {
> > - struct tc358743_state *state = to_state(sd);
> > + u8 vi_status3 = i2c_rd8(sd, VI_STATUS3);
> > u8 vi_rep = i2c_rd8(sd, VI_REP);
> >
> > - if (format->pad != 0)
> > - return -EINVAL;
> > + switch (vi_status3 & MASK_S_V_COLOR) {
> > + default:
> > + case MASK_S_V_COLOR_RGB:
> > + case MASK_S_V_COLOR_SYCC601:
> > + format->colorspace = V4L2_COLORSPACE_SRGB;
> > + format->xfer_func = V4L2_XFER_FUNC_SRGB;
> > + break;
> > + case MASK_S_V_COLOR_YCBCR601:
> > + case MASK_S_V_COLOR_XVYCC601:
> > + format->colorspace = V4L2_COLORSPACE_SMPTE170M;
>
> Not correct. XVYCC601 uses the REC709 colorspace. The XVYCC formats
> are only defined for the REC709 colorspace and only differ in the
> YCbCr encoding that they use.
I'll move the XVYCC601 case down to join YCBCR709 and XVYCC709.
> > + format->xfer_func = V4L2_XFER_FUNC_709;
> > + break;
> > + case MASK_S_V_COLOR_YCBCR709:
> > + case MASK_S_V_COLOR_XVYCC709:
> > + format->colorspace = V4L2_COLORSPACE_REC709;
> > + format->xfer_func = V4L2_XFER_FUNC_709;
> > + break;
> > + case MASK_S_V_COLOR_ADOBERGB:
> > + case MASK_S_V_COLOR_ADOBEYCC601:
> > + format->colorspace = V4L2_COLORSPACE_ADOBERGB;
> > + format->xfer_func = V4L2_XFER_FUNC_ADOBERGB;
> > + break;
> > + }
> >
> > - format->format.code = state->mbus_fmt_code;
> > - format->format.width = state->timings.bt.width;
> > - format->format.height = state->timings.bt.height;
> > - format->format.field = V4L2_FIELD_NONE;
> > + format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> >
> > switch (vi_rep & MASK_VOUT_COLOR_SEL) {
> > + default:
> > case MASK_VOUT_COLOR_RGB_FULL:
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > + break;
> > case MASK_VOUT_COLOR_RGB_LIMITED:
> > - format->format.colorspace = V4L2_COLORSPACE_SRGB;
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > + format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > break;
> > case MASK_VOUT_COLOR_601_YCBCR_LIMITED:
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > + format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > + break;
> > case MASK_VOUT_COLOR_601_YCBCR_FULL:
> > - format->format.colorspace = V4L2_COLORSPACE_SMPTE170M;
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_XV601;
>
> This isn't right. Only use V4L2_YCBCR_ENC_XV601 (or XV709) if this is
> signaled in the InfoFrame.
>
> Full Range V4L2_YCBCR_ENC_601 != Full Range V4L2_YCBCR_ENC_XV601.
>
> XV601 is similar to Limited Range 601, except that it also utilizes
> values 0-15 and 241-255, thus allowing for a wider gamut of colors.
Thanks. With that explanation, I think I should just change this to
V4L2_YCBCR_ENC_601 and limit the VOUT_COLOR_RGB/601/709 selections to
RGB input.
> > + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > break;
> > case MASK_VOUT_COLOR_709_YCBCR_FULL:
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_XV709;
> > + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > + break;
> > case MASK_VOUT_COLOR_709_YCBCR_LIMITED:
> > - format->format.colorspace = V4L2_COLORSPACE_REC709;
> > + format->ycbcr_enc = V4L2_YCBCR_ENC_709;
> > + format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > break;
> > - default:
> > - format->format.colorspace = 0;
> > + case MASK_VOUT_COLOR_FULL_TO_LIMITED:
> > + format->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> > + break;
> > + case MASK_VOUT_COLOR_LIMITED_TO_FULL:
> > + format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > break;
Then I'll use FULL_TO_LIMITED / LIMITED_TO_FULL to for YUV inputs and
correctly set ycbcr_enc and quantization depending on this and input
colorspace.
[...]
> Except for the noted issues this looks good to me.
Thank you for the review.
regards
Philipp
