From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Harvey Subject: Re: [PATCH v6 4/6] media: i2c: Add TDA1997x HDMI receiver driver Date: Thu, 25 Jan 2018 08:15:32 -0800 Message-ID: References: <1514491789-8697-1-git-send-email-tharvey@gateworks.com> <1514491789-8697-5-git-send-email-tharvey@gateworks.com> <1e65ee61-f282-4b53-dd03-68a89a91da8e@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1e65ee61-f282-4b53-dd03-68a89a91da8e@xs4all.nl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Hans Verkuil Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Hans Verkuil , Mauro Carvalho Chehab , Philipp Zabel , Steve Longerbeam , Shawn Guo , linux-media List-Id: devicetree@vger.kernel.org On Mon, Jan 15, 2018 at 4:56 AM, Hans Verkuil wrote: > On 12/28/2017 09:09 PM, Tim Harvey wrote: >> Add support for the TDA1997x HDMI receivers. >> > > This looks good. > > But there is one corner case that isn't handled in this driver: what if there > is no AVI InfoFrame (e.g. you receive a DVI signal)? > > The CTA-861 spec says that in the absence of an AVI InfoFrame you should use > limited range for CE formats and full range for IT formats. However, without > an AVI InfoFrame it is hard to detect a CE format since there is no VIC code. > In addition, a real DVI source will always transmit full range. > > I would recommend that in the absence of an AVI InfoFrame you fall back to > sRGB with full range quantization. Not quite according to the spec but more > likely to work in practice. > Hans, Sounds like we are just talking about the default settings which would be used in the case of no infoframes. I'll default the struct v4l2_hdmi_colorimetry I store in the state struct as: + /* + * default to SRGB full range quantization + * (in case we don't get an infoframe such as DVI signal + */ + state->colorimetry.colorspace = V4L2_COLORSPACE_SRGB; + state->colorimetry.quantization = V4L2_QUANTIZATION_FULL_RANGE; + >> >> +static int tda1997x_enum_mbus_code(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_mbus_code_enum *code) >> +{ >> + struct tda1997x_state *state = to_state(sd); >> + >> + if (code->index > 0) >> + return -EINVAL; >> + >> + code->code = state->code; > > Hmm. This receiver supports multiple output formats, but you advertise only one. > That looks wrong. If nothing else, you should be able to switch between RGB and > YUV 4:4:4 since they use the same port config. > > It's a common use-case that you want to switch between RGB and YUV depending on > the source material (i.e. if you receive a desktop/graphics then RGB is best, if > you receive video then YUV 4:2:2 or 4:2:0 is best). > > Hardcoding just one format won't do. > I've been thinking about this a bit. I had hard-coded a single format for now because I haven't had any good ideas on how to deal with the fact that the port mappings would need to differ if you change from the RGB888/YUV444 (I think these are referred to as 'planar' formats?) to YUV422 (semi-planar) and BT656 formats. It is true though that the 36bit (TDA19973) RGB888/YUV444 and 24bit (TDA19971/2) formats can both be supported with the same port mappings / pinout. For example the GW5400 has a TDA19971 mapped to IMX6 CSI_DATA[19:4] (16bit) for YUV422. However if you want to use BT656 you have to shift the TDA19971 port mappings to get the YCbCr pins mapped to CSI_DATA[19:x] and those pin groups are at the bottom of the bus for the RGB888/YUV444 format. I suppose however that perhaps for the example above if I have a 16bit width required to support YUV422 there would never be a useful case for supporting 8-bit/10-bit/12-bit BT656 on the same board? Maybe I can define an array of mbus_codes in the state structure then fill in the possible states per bit width during init like this: switch (state->info->type) { case TDA19973: switch (pdata->vidout_bus_width) { case 36: /* 36bit YUV */ state->mbus_codes[0] = MEDIA_BUS_FMT_YUV12_1X36; /* 36bit RGB */ state->mbus_codes[1] = MEDIA_BUS_FMT_RGB121212_1X36; break; case 24: /* 24bit BT656 (YUV422 semi-planar: 1-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY12_1X24; /* 24bit YUV */ state->mbus_codes[1] = MEDIA_BUS_FMT_YUV8_1X24; break; case 12: /* 12bit BT656 (2-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY12_2X12; break; } break; case TDA19971: switch (pdata->vidout_bus_width) { case 24: /* 24bit YUV */ state->mbus_codes[0] = MEDIA_BUS_FMT_YUV8_1X24; /* 24bit RGB */ state->mbus_codes[1] = MEDIA_BUS_FMT_RGB888_1X24; break; case 20: /* 20bit YUV422 */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY10_1X20; break; case 16: /* 16bit BT656 (YUV422 semi-planar: 1-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY8_1X16; break; case 12: /* 12bit BT656 (2-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY12_2X12; break; case 10: /* 10bit BT656 (2-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY10_2X10; break; case 8: /* 8bit BT656 (2-cycle) */ state->mbus_codes[0] = MEDIA_BUS_FMT_UYVY8_2X8; break; } } /* default format */ state->mbus_code = state->mbus_codes[0]; This doesn't support the fact for example that a 20bit bus can also support the 8/10/12bit BT656 formats. >> + >> + return 0; >> +} >> + >> +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; >> + >> + memset(format, 0, sizeof(*format)); >> + bt = &state->detected_timings->bt; >> + format->width = bt->width; >> + format->height = bt->height; >> + format->colorspace = state->colorimetry.colorspace; >> + >> + return 0; >> +} >> + >> +static int tda1997x_get_pad_format(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_format *format) >> +{ >> + struct tda1997x_state *state = to_state(sd); >> + >> + v4l_dbg(1, debug, state->client, "%s\n", __func__); >> + if (format->pad != TDA1997X_PAD_SOURCE) >> + return -EINVAL; >> + >> + tda1997x_fill_format(state, &format->format); >> + >> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { >> + struct v4l2_mbus_framefmt *fmt; >> + >> + fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad); >> + format->format.code = format->format.code; >> + } else >> + format->format.code = state->code; > > No need to mess around with TRY vs ACTIVE. The behavior is the same for > both. only if I have a single bus format though right? > >> + >> + return 0; >> +} >> + >> +static int tda1997x_set_pad_format(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_format *format) >> +{ >> + struct v4l2_mbus_framefmt *fmt; >> + >> + if (format->pad != TDA1997X_PAD_SOURCE) >> + return -EINVAL; >> + >> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) >> + return tda1997x_get_pad_format(sd, cfg, format); > > Same here, just call return tda1997x_get_pad_format(sd, cfg, format); > > But note that it makes no sense to support only one possible mbus code > when it is clearly possible to switch between multiple output formats. > > I'm not sure why I didn't see that earlier... > But once I add in the multiple mbus formats then I need the try vs active correct? Regards, Tim