On Thu, Nov 23, 2017 at 12:08 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 11/23/2017 05:27 AM, Tim Harvey wrote:
>> On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> Hi Tim,
>>>
>>> Some more review comments:
>>>
>>> On 11/09/2017 07:45 PM, Tim Harvey wrote:
>>>> Add support for the TDA1997x HDMI receivers.
>> <snip>
>>>> + */
>>>> +struct color_matrix_coefs {
>>>> + const char *name;
>>>> + /* Input offsets */
>>>> + s16 offint1;
>>>> + s16 offint2;
>>>> + s16 offint3;
>>>> + /* Coeficients */
>>>> + s16 p11coef;
>>>> + s16 p12coef;
>>>> + s16 p13coef;
>>>> + s16 p21coef;
>>>> + s16 p22coef;
>>>> + s16 p23coef;
>>>> + s16 p31coef;
>>>> + s16 p32coef;
>>>> + s16 p33coef;
>>>> + /* Output offsets */
>>>> + s16 offout1;
>>>> + s16 offout2;
>>>> + s16 offout3;
>>>> +};
>>>> +
>>>> +enum {
>>>> + ITU709_RGBLIMITED,
>>>> + ITU709_RGBFULL,
>>>> + ITU601_RGBLIMITED,
>>>> + ITU601_RGBFULL,
>>>> + RGBLIMITED_RGBFULL,
>>>> + RGBLIMITED_ITU601,
>>>> + RGBFULL_ITU601,
>>>
>>> This can't be right.
>>> ITU709_RGBLIMITED
>>> You have these conversions:
>>>
>>> ITU709_RGBFULL
>>> ITU601_RGBFULL
>>> RGBLIMITED_RGBFULL
>>> RGBLIMITED_ITU601
>>> RGBFULL_ITU601
>>> RGBLIMITED_ITU709
>>> RGBFULL_ITU709
>>>
>>> I.e. on the HDMI receiver side you can receive RGB full/limited or ITU601/709.
>>> On the output side you have RGB full or ITU601/709.
>>>
>>> So something like ITU709_RGBLIMITED makes no sense.
>>>
>>
>> I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
>> you to configure the output range. If output to the SoC is only ever
>> full quant range for RGB then I can drop the
>> ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.
>
> Output for RGB is always full range. The reason is simply that the V4L2 API
> has no way of selecting the quantization range it wants to receive. I made
> a patch for that a few years back, but there really is no demand for it (yet).
> Userspace expects full range RGB and limited range YUV.
>
>>
>> However, If the output is YUV how do I know if I need to convert to
>> ITU709 or ITU601 and what are my conversion matrices for
>> RGBLIMITED_ITU709/RGBFULL_ITU709?
>
> You can choose yourself whether you convert to YUV 601 or 709. I would
> recommend to use 601 for SDTV resolutions (i.e. width/height <= 720x576)
> and 709 for HDTV.
>
> I made a little program that calculates the values for RGB lim/full to
> YUV 601/709:
>
> -------------------------------------
> #include <stdlib.h>
> #include <stdio.h>
>
> #define COEFF(v, r) ((v) * (r) * 16.0)
>
> static const double bt601[3][3] = {
> { COEFF(0.299, 219), COEFF(0.587, 219), COEFF(0.114, 219) },
> { COEFF(-0.1687, 224), COEFF(-0.3313, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4187, 224), COEFF(-0.0813, 224) },
> };
> static const double rec709[3][3] = {
> { COEFF(0.2126, 219), COEFF(0.7152, 219), COEFF(0.0722, 219) },
> { COEFF(-0.1146, 224), COEFF(-0.3854, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4542, 224), COEFF(-0.0458, 224) },
> };
>
> int main(int argc, char **argv)
> {
> int i, j;
> int mapi[] = { 0, 2, 1 };
> int mapj[] = { 1, 0, 2 };
>
> printf("rgb full -> 601\n");
> printf(" 0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ", (int)(0.5 + bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf(" 256, 2048, 2048,\n\n");
>
> printf("rgb lim -> 601\n");
> printf(" -256, -256, -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ", (int)(0.5 + 255.0 / 219.0 * bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf(" 256, 2048, 2048,\n\n");
>
> printf("rgb full -> 709\n");
> printf(" 0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ", (int)(0.5 + rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf(" 256, 2048, 2048,\n\n");
>
> printf("rgb lim -> 709\n");
> printf(" -256, -256, -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ", (int)(0.5 + 255.0 / 219.0 * rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf(" 256, 2048, 2048,\n\n");
> return 0;
> }
> -------------------------------------
>
> This should give you the needed matrices. It's up to you whether to keep the
> existing matrices for 601 or replace them with these. Probably best to keep
> them.
>
Hans,
Thanks for the code - this gives me what I need.
>>
>> Sorry for all the questions, the colorspace/colorimetry options
>> confuse the heck out of me.
>>
>>>> +};
>>>> +
>> <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->range = frame.avi.quantization_range;
>>>
>>> This should be ignored if it is overridden by the RGB Quantization Range
>>> control, or am I missing something?
>>>
>>
>> Ok. Sounds like I should only use the range from the infoframe if
>> range == V4L2_DV_RGB_RANGE_AUTO:
>>
>> /* Quantization Range */
>> if (state->range == V4L2_DV_RGB_RANGE_AUTO)
>> state->range = frame.avi.quantization_range;
>
> Huh? You're mixing V4L2_DV_RGB_* defines with HDMI_QUANTIZATION_RANGE_*
> defines.
>
> You probably mean to check the control value here.
>
Yes, I think I've got it now and will post a v4 today.
Tim
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
