linux-kernel - Re: [PATCH v5 4/6] media: i2c: Add TDA1997x HDMI receiver driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ+vNU34fBjBos4g9vxv7UkcJOm1YJux7NM2=iUKBjOrxZeMBA@mail.gmail.com>
Date:   Tue, 19 Dec 2017 09:01:12 -0800
From:   Tim Harvey <tharvey@...eworks.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     linux-media <linux-media@...r.kernel.org>,
        alsa-devel@...a-project.org,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Hans Verkuil <hansverk@...co.com>,
        Mauro Carvalho Chehab <mchehab@...pensource.com>
Subject: Re: [PATCH v5 4/6] media: i2c: Add TDA1997x HDMI receiver driver

On Tue, Dec 19, 2017 at 3:12 AM, Hans Verkuil <hverkuil@...all.nl> wrote:
> On 16/12/17 19:00, Tim Harvey wrote:
>> +
>> +static int tda1997x_fill_format(struct tda1997x_state *state,
>> +                             struct v4l2_mbus_framefmt *format)
>> +{
>> +     const struct v4l2_bt_timings *bt;
>> +     struct v4l2_hdmi_colorimetry c;
>> +
>> +     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));
>> +     c = v4l2_hdmi_rx_colorimetry(&state->avi_infoframe, NULL, bt->height);
>> +     format->width = bt->width;
>> +     format->height = bt->height;
>> +     format->field = (bt->interlaced) ?
>> +             V4L2_FIELD_ALTERNATE : V4L2_FIELD_NONE;
>> +     format->colorspace = c.colorspace;
>> +     format->ycbcr_enc = c.ycbcr_enc;
>> +     format->quantization = c.quantization;
>> +     format->xfer_func = c.xfer_func;
>
> This is wrong. v4l2_hdmi_rx_colorimetry returns what arrives on the HDMI link,
> that's not the same as is output towards the SoC. You need to take limited/full
> range conversions and 601/709 conversions into account since that's what ends
> up in memory.
>
> Also note: you are still parsing the colorimetry information from avi_infoframe
> in the infoframe parse function. There is no need to do that, just call
> v4l2_hdmi_rx_colorimetry and let that function parse and interpret all this.
>
> Otherwise we still have two places that try to interpret that information.

Hans,

Ok so v4l2_hdmi_rx_colorimetry() handles parsing the source avi
infoframe and deals with enforcing the detailed rules and returns
'v4l2' enums:

tda1997x_parse_infoframe(...)
...
        case HDMI_INFOFRAME_TYPE_AVI:
                state->avi_infoframe = frame.avi; /* hold on to avi
infoframe for later use in logging etc */
                /* parse avi infoframe colorimetry data for v4l2
colorspace/ycbcr_encoding/quantization/xfer_func */
                state->hdmi_colorimetry = v4l2_hdmi_rx_colorimetry(&frame.avi,
                                                NULL,
                                                state->timings.bt.height);

Also here I still need to override the quant range passed from the
source avi infoframe per the user control (if not auto) and set per
vic if default:

                /* 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;
                }


Then tda1997x_fill_format() then needs to fill in details of what's on
the bus so I should be filling in only width/height/field/colorspace
and use colorspace based on my csc conversion chosen output
(V4L2_COLORSPACE_SRGB|V4L2_COLORSPACE_SMPTE170M|V4L2_COLORSPACE_REC709)
and I don't need to set ycbcr_enc/quantization/xfer_func.

does this sound right?

Thanks,

Tim