On Mon, Dec 4, 2017 at 4:50 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Hi Tim,
>
> Found a few more small issues. After that's fixed and you have the Ack for the
> bindings this can be merged I think.
Hans,
Thanks. Can you weigh in on the bindings? Rob was hoping for some
discussion on making some generic bus format types for video and I'm
not familiar with the other video encoders/decoders enough to know if
there is enough commonality.
>
> On 11/29/2017 10:19 PM, Tim Harvey wrote:
<snip>
>> +
>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = &state->sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(&frame, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + hdmi_infoframe_log(KERN_INFO, &state->client->dev, &frame);
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> + /* reset the audio FIFO */
>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> + }
>> + break;
>> +
>> + /* Auxiliary Video information (AVI) InfoFrame: see HDMI spec 8.2.1 */
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> + state->colorspace = frame.avi.colorspace;
>> + state->colorimetry = frame.avi.colorimetry;
>> + state->content = frame.avi.content_type;
>> + /* Quantization Range */
>> + switch (state->rgb_quantization_range) {
>> + case V4L2_DV_RGB_RANGE_AUTO:
>> + state->range = frame.avi.quantization_range;
>> + break;
>> + case V4L2_DV_RGB_RANGE_LIMITED:
>> + state->range = HDMI_QUANTIZATION_RANGE_LIMITED;
>> + break;
>> + case V4L2_DV_RGB_RANGE_FULL:
>> + state->range = HDMI_QUANTIZATION_RANGE_FULL;
>> + break;
>> + }
>> + if (state->range == HDMI_QUANTIZATION_RANGE_DEFAULT) {
>> + if (frame.avi.video_code <= 1)
>> + state->range = HDMI_QUANTIZATION_RANGE_FULL;
>> + else
>> + state->range = HDMI_QUANTIZATION_RANGE_LIMITED;
>> + }
>> + /*
>> + * If colorimetry not specified, conversion depends on res type:
>> + * - SDTV: ITU601 for SD (480/576/240/288 line resolution)
>> + * - HDTV: ITU709 for HD (720/1080 line resolution)
>> + * - PC: sRGB
>> + * see HDMI specification section 6.7
>> + */
>> + if ((state->colorspace == HDMI_COLORSPACE_YUV422 ||
>> + state->colorspace == HDMI_COLORSPACE_YUV444) &&
>> + (state->colorimetry == HDMI_COLORIMETRY_EXTENDED ||
>> + state->colorimetry == HDMI_COLORIMETRY_NONE)) {
>> + if (is_sd(state->timings.bt.height))
>> + state->colorimetry = HDMI_COLORIMETRY_ITU_601;
>> + else if (is_hd(state->timings.bt.height))
>> + state->colorimetry = HDMI_COLORIMETRY_ITU_709;
>> + else
>> + state->colorimetry = HDMI_COLORIMETRY_NONE;
>> + }
>> + v4l_dbg(1, debug, state->client,
>> + "colorspace=%d colorimetry=%d range=%d content=%d\n",
>> + state->colorspace, state->colorimetry, state->range,
>> + state->content);
>> +
>> + /* configure upsampler: 0=bypass 1=repeatchroma 2=interpolate */
>> + reg = io_read(sd, REG_PIX_REPEAT);
>> + reg &= ~PIX_REPEAT_MASK_UP_SEL;
>> + if (state->colorspace == HDMI_COLORSPACE_YUV422)
>> + reg |= (PIX_REPEAT_CHROMA << PIX_REPEAT_SHIFT);
>> + io_write(sd, REG_PIX_REPEAT, reg);
>> +
>> + /* ConfigurePixelRepeater: repeat n-times each pixel */
>> + reg = io_read(sd, REG_PIX_REPEAT);
>> + reg &= ~PIX_REPEAT_MASK_REP;
>> + reg |= frame.avi.pixel_repeat;
>> + io_write(sd, REG_PIX_REPEAT, reg);
>> +
>> + /* configure the receiver with the new colorspace */
>> + tda1997x_configure_csc(sd);
>> + break;
>> + default:
>> + break;
>> + }
>> + return 0;
>> +}
>> +
<snip>
>> +
>> +static int tda1997x_fill_format(struct tda1997x_state *state,
>> + struct v4l2_mbus_framefmt *format)
>> +{
>> + const struct v4l2_bt_timings *bt;
>> +
>> + v4l_dbg(1, debug, state->client, "%s\n", __func__);
>> +
>> + if (!state->detected_timings)
>> + return -EINVAL;
>> + bt = &state->detected_timings->bt;
>> + memset(format, 0, sizeof(*format));
>> +
>> + format->width = bt->width;
>> + format->height = bt->height;
>> + format->field = V4L2_FIELD_NONE;
>> + format->colorspace = V4L2_COLORSPACE_SRGB;
>> + if (bt->flags & V4L2_DV_FL_IS_CE_VIDEO)
>> + format->colorspace = (bt->height <= 576) ?
>> + V4L2_COLORSPACE_SMPTE170M : V4L2_COLORSPACE_REC709;
>
> Close. What is missing is a check of the AVI InfoFrame: if it has an explicit
> colorimetry then use that. E.g. check for HDMI_COLORIMETRY_ITU_601 or ITU_709
> and set the colorspace accordingly. Otherwise fall back to what you have here.
>
This function currently matches adv7604/adv7842 where they don't look
at colorimetry (but I do see a TODO in adv748x_hdmi_fill_format to
look at this) so I don't have an example and may not understand.
Do you mean:
format->colorspace = V4L2_COLORSPACE_SRGB;
if (bt->flags & V4L2_DV_FL_IS_CE_VIDEO) {
if ((state->colorimetry == HDMI_COLORIMETRY_ITU_601) ||
(state->colorimetry == HDMI_COLORIMETRY_ITU_709))
format->colorspace = state->colorspace;
else
format->colorspace = is_sd(bt->height) ?
V4L2_COLORSPACE_SMPTE170M :
V4L2_COLORSPACE_REC709;
}
Also during more testing I've found that I'm not capturing interlaced
properly and know I at least need:
- format->field = V4L2_FIELD_NONE;
+ format->field = (bt->interlaced) ?
+ V4L2_FIELD_ALTERNATE : V4L2_FIELD_NONE;
I'm still not quite capturing interlaced yet but I think its an issue
of setting up the media pipeline improperly.
> Make a note that we ignore HDMI_COLORIMETRY_EXTENDED as we don't properly
> support that anyway. For the record: I know that the colorimetry field in the
> AVI InfoFrame can change dynamically, so this is just a snapshot in time. We
> don't have good support for dynamic colorimetry changes. Or to be precise: we
> never got around to implement it.
>
>> +
>> + return 0;
>> +}
>> +
<snip>
>> +
>> +static int tda1997x_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct v4l2_subdev *sd = to_sd(ctrl);
>> + struct tda1997x_state *state = to_state(sd);
>> +
>> + switch (ctrl->id) {
>> + /* allow overriding the default RGB quantization range */
>> + case V4L2_CID_DV_RX_RGB_RANGE:
>> + state->range = ctrl->val;
>
> This should be assigned to state->rgb_quantization_range!
oops... thanks!
>
>> + tda1997x_configure_csc(sd);
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +};
>> +
Thanks,
Tim
