Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751166AbdLSR1R (ORCPT ); Tue, 19 Dec 2017 12:27:17 -0500 Received: from lb3-smtp-cloud7.xs4all.net ([194.109.24.31]:60246 "EHLO lb3-smtp-cloud7.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750796AbdLSR1P (ORCPT ); Tue, 19 Dec 2017 12:27:15 -0500 Subject: Re: [PATCH v5 4/6] media: i2c: Add TDA1997x HDMI receiver driver To: Tim Harvey References: <1513447230-30948-1-git-send-email-tharvey@gateworks.com> <1513447230-30948-5-git-send-email-tharvey@gateworks.com> <3ae0ee4d-e754-1e97-33ed-d1fedf442dfb@xs4all.nl> Cc: linux-media , alsa-devel@alsa-project.org, "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Shawn Guo , Steve Longerbeam , Philipp Zabel , Hans Verkuil , Mauro Carvalho Chehab From: Hans Verkuil Message-ID: Date: Tue, 19 Dec 2017 18:27:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfFunTK5EK+bx88uhcYfcN4nya1R2fzGt7t2SGA6kD2MOnp4vHITEnRwm0ushxAOcYrGDlFRZ5rLOMFsK2vzX+zVEJV9L4nyv+Yc/iQXPJXFib6yc/B7+ YODPKpSSiY5lJ9liB8200Sj9cochQnGuEC+TS7T0xKlcrKojUJDhehzXvz6osR+a1F9T1Ta8n1ctoKYikma+++/6S/KaUOJ0uY9iUCtq6GV2krQudx4y4Qxr uMhHbFGJS5NF/gvCfNa6s3/Tmnz7/86bK0CiZxyMMLfHso4ZfKXjah4sXjLp82rHEia0fO8J3rifsthlk1/FiV8p/VaaBxw+K+zglR/V7VI/dFqyoQy55T7k kVexqDsA+suAWEoD9wxKpsuKtGUP+59ljR05gRPknKB2FVmrlIo73oPeL9E+k64whu14dn5D0RqbXyb5LmKZ0swgwcnNZ14N4y8HEFSo0c6tj5CDR2MFibMz 4Y/LvXAmdJ7Ua9pi Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4854 Lines: 123 On 19/12/17 18:01, Tim Harvey wrote: > On Tue, Dec 19, 2017 at 3:12 AM, Hans Verkuil 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; > } No, the vic check is already done in v4l2_hdmi_rx_colorimetry. Call v4l2_hdmi_rx_colorimetry first, then: /* If ycbcr_enc is V4L2_YCBCR_ENC_DEFAULT, then we receive RGB */ if (c.ycbcr_enc == V4L2_YCBCR_ENC_DEFAULT) switch (state->rgb_quantization_range) { case V4L2_DV_RGB_RANGE_LIMITED: c.quantization = V4L2_QUANTIZATION_FULL_RANGE; break; case V4L2_DV_RGB_RANGE_FULL: c.quantization = V4L2_QUANTIZATION_LIM_RANGE; break; } (c is of type struct v4l2_hdmi_colorimetry) > > > 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. You don't touch the colorspace and xfer_func fields. The simple matrix csc can only change quantization range and/or ycbcr encoding. It doesn't change the underlying colorspace ('chromaticities') or the used transfer function. In practice if the output is RGB then ycbcr_enc should be set to V4L2_YCBCR_ENC_DEFAULT and quantization to FULL_RANGE. For YUV output you set ycbcr_enc to V4L2_YCBCR_ENC_601 or 709 and quantization to LIM_RANGE. Regards, Hans > > does this sound right? > > Thanks, > > Tim >