Hi Tim,
I was so hoping I could make a pull request for this, but I still found problems with g/s/query_dv_timings.
I strongly suspect that v4l2-compliance would fail if you boot up the system *without* a source connected.
And I discovered that I was missing additional checks in the timings tests for v4l2-compliance that would have found the same issue if run with a source connected. I've fixed and committed those tests now. I'll also try to test that a S_DV_TIMINGS calls updates the format.
Details below:
On 02/07/18 23:42, Tim Harvey wrote:
+struct tda1997x_state {
...
- struct v4l2_dv_timings timings;
- const struct v4l2_dv_timings *detected_timings;
...
+/* Configure frame detection window and VHREF timing generator */ +static int +tda1997x_configure_vhref(struct v4l2_subdev *sd) +{
- struct tda1997x_state *state = to_state(sd);
- const struct v4l2_bt_timings *bt;
- int width, lines;
- u16 href_start, href_end;
- u16 vref_f1_start, vref_f2_start;
- u8 vref_f1_width, vref_f2_width;
- u8 field_polarity;
- u16 fieldref_f1_start, fieldref_f2_start;
- u8 reg;
- if (!state->detected_timings)
return -EINVAL;
Why this test? Who cares if there are no detected timings? It's certainly not a failure. S_DV_TIMINGS should succeed regardless of whether there is a signal or not and regardless of the current detected timings.
- bt = &state->detected_timings->bt;
Ouch. The timings passed in with S_DV_TIMINGS should be used.
Just use state->timings here, not detected_timings.
- href_start = bt->hbackporch + bt->hsync + 1;
- href_end = href_start + bt->width;
- vref_f1_start = bt->height + bt->vbackporch + bt->vsync +
bt->il_vbackporch + bt->il_vsync +
bt->il_vfrontporch;
- vref_f1_width = bt->vbackporch + bt->vsync + bt->vfrontporch;
- vref_f2_start = 0;
- vref_f2_width = 0;
- fieldref_f1_start = 0;
- fieldref_f2_start = 0;
- if (bt->interlaced) {
vref_f2_start = (bt->height / 2) +
(bt->il_vbackporch + bt->il_vsync - 1);
vref_f2_width = bt->il_vbackporch + bt->il_vsync +
bt->il_vfrontporch;
fieldref_f2_start = vref_f2_start + bt->il_vfrontporch +
fieldref_f1_start;
- }
- field_polarity = 0;
- width = V4L2_DV_BT_FRAME_WIDTH(bt);
- lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
- /*
* Configure Frame Detection Window:
* horiz area where the VHREF module consider a VSYNC a new frame
*/
- io_write16(sd, REG_FDW_S, 0x2ef); /* start position */
- io_write16(sd, REG_FDW_E, 0x141); /* end position */
- /* Set Pixel And Line Counters */
- if (state->chip_revision == 0)
io_write16(sd, REG_PXCNT_PR, 4);
- else
io_write16(sd, REG_PXCNT_PR, 1);
- io_write16(sd, REG_PXCNT_NPIX, width & MASK_VHREF);
- io_write16(sd, REG_LCNT_PR, 1);
- io_write16(sd, REG_LCNT_NLIN, lines & MASK_VHREF);
- /*
* Configure the VHRef timing generator responsible for rebuilding all
* horiz and vert synch and ref signals from its input allowing auto
* detection algorithms and forcing predefined modes (480i & 576i)
*/
- reg = VHREF_STD_DET_OFF << VHREF_STD_DET_SHIFT;
- io_write(sd, REG_VHREF_CTRL, reg);
- /*
* Configure the VHRef timing values. In case the VHREF generator has
* been configured in manual mode, this will allow to manually set all
* horiz and vert ref values (non-active pixel areas) of the generator
* and allows setting the frame reference params.
*/
- /* horizontal reference start/end */
- io_write16(sd, REG_HREF_S, href_start & MASK_VHREF);
- io_write16(sd, REG_HREF_E, href_end & MASK_VHREF);
- /* vertical reference f1 start/end */
- io_write16(sd, REG_VREF_F1_S, vref_f1_start & MASK_VHREF);
- io_write(sd, REG_VREF_F1_WIDTH, vref_f1_width);
- /* vertical reference f2 start/end */
- io_write16(sd, REG_VREF_F2_S, vref_f2_start & MASK_VHREF);
- io_write(sd, REG_VREF_F2_WIDTH, vref_f2_width);
- /* F1/F2 FREF, field polarity */
- reg = fieldref_f1_start & MASK_VHREF;
- reg |= field_polarity << 8;
- io_write16(sd, REG_FREF_F1_S, reg);
- reg = fieldref_f2_start & MASK_VHREF;
- io_write16(sd, REG_FREF_F2_S, reg);
- return 0;
+}
...
+static int +tda1997x_detect_std(struct tda1997x_state *state) +{
- struct v4l2_subdev *sd = &state->sd;
- u32 vper;
- u16 hper;
- u16 hsper;
- int i;
- /*
* Read the FMT registers
* REG_V_PER: Period of a frame (or two fields) in MCLK(27MHz) cycles
* REG_H_PER: Period of a line in MCLK(27MHz) cycles
* REG_HS_WIDTH: Period of horiz sync pulse in MCLK(27MHz) cycles
*/
- vper = io_read24(sd, REG_V_PER) & MASK_VPER;
- hper = io_read16(sd, REG_H_PER) & MASK_HPER;
- hsper = io_read16(sd, REG_HS_WIDTH) & MASK_HSWIDTH;
- if (!vper || !hper || !hsper)
return -ENOLINK;
- v4l2_dbg(1, debug, sd, "Signal Timings: %u/%u/%u\n", vper, hper, hsper);
- for (i = 0; v4l2_dv_timings_presets[i].bt.width; i++) {
const struct v4l2_bt_timings *bt;
u32 lines, width, _hper, _hsper;
u32 vmin, vmax, hmin, hmax, hsmin, hsmax;
bool vmatch, hmatch, hsmatch;
bt = &v4l2_dv_timings_presets[i].bt;
width = V4L2_DV_BT_FRAME_WIDTH(bt);
lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
_hper = (u32)bt->pixelclock / width;
if (bt->interlaced)
lines /= 2;
/* vper +/- 0.7% */
vmin = ((27000000 / 1000) * 993) / _hper * lines;
vmax = ((27000000 / 1000) * 1007) / _hper * lines;
/* hper +/- 1.0% */
hmin = ((27000000 / 100) * 99) / _hper;
hmax = ((27000000 / 100) * 101) / _hper;
/* hsper +/- 2 (take care to avoid 32bit overflow) */
_hsper = 27000 * bt->hsync / ((u32)bt->pixelclock/1000);
hsmin = _hsper - 2;
hsmax = _hsper + 2;
/* vmatch matches the framerate */
vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
/* hmatch matches the width */
hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
/* hsmatch matches the hswidth */
hsmatch = ((hsper <= hsmax) && (hsper >= hsmin)) ? 1 : 0;
if (hmatch && vmatch && hsmatch) {
state->detected_timings = &v4l2_dv_timings_presets[i];
v4l2_print_dv_timings(sd->name, "Detected format: ",
state->detected_timings, false);
return 0;
}
- }
- v4l_err(state->client, "no resolution match for timings: %d/%d/%d\n",
vper, hper, hsper);
- return -EINVAL;
+}
...
+static void tda1997x_irq_sus(struct tda1997x_state *state, u8 *flags) +{
- struct v4l2_subdev *sd = &state->sd;
- u8 reg, source;
- source = io_read(sd, REG_INT_FLG_CLR_SUS);
- io_write(sd, REG_INT_FLG_CLR_SUS, source);
- if (source & MASK_MPT) {
/* reset MTP in use flag if set */
if (state->mptrw_in_progress)
state->mptrw_in_progress = 0;
- }
- if (source & MASK_SUS_END) {
/* reset audio FIFO */
reg = io_read(sd, REG_HDMI_INFO_RST);
reg |= MASK_SR_FIFO_FIFO_CTRL;
io_write(sd, REG_HDMI_INFO_RST, reg);
reg &= ~MASK_SR_FIFO_FIFO_CTRL;
io_write(sd, REG_HDMI_INFO_RST, reg);
/* reset HDMI flags */
state->hdmi_status = 0;
- }
- /* filter FMT interrupt based on SUS state */
- reg = io_read(sd, REG_SUS_STATUS);
- if (((reg & MASK_SUS_STATUS) != LAST_STATE_REACHED)
|| (source & MASK_MPT)) {
source &= ~MASK_FMT;
- }
- if (source & (MASK_FMT | MASK_SUS_END)) {
reg = io_read(sd, REG_SUS_STATUS);
if ((reg & MASK_SUS_STATUS) != LAST_STATE_REACHED) {
v4l_err(state->client, "BAD SUS STATUS\n");
return;
}
/* There is new activity, the status for HDCP repeater state */
state->state_c5_reached = 0;
/* Detect the new resolution */
if (!tda1997x_detect_std(state))
v4l2_subdev_notify_event(&state->sd, &tda1997x_ev_fmt);
Why detect a new resolution here? That only need to be done when the user calls query_dv_timings. Just send the event unconditionally to tell userspace that the resolution changed.
- }
+}
...
+/* -----------------------------------------------------------------------------
- v4l2_subdev_video_ops
- */
+static int +tda1997x_g_input_status(struct v4l2_subdev *sd, u32 *status) +{
- struct tda1997x_state *state = to_state(sd);
- mutex_lock(&state->lock);
- if (state->detected_timings)
What this actually tests if the driver was able to detect a valid signal and lock to it.
In practice you have three possible outcomes:
- There is no HDMI signal at all: return V4L2_IN_ST_NO_SIGNAL - There is a signal, but the receiver could not lock to it: return V4L2_IN_ST_NO_SYNC - There is a signal, and the receiver could lock: return 0.
Just because this returns 0, doesn't mean that QUERY_DV_TIMINGS will succeed. There may be other constraints (e.g. the driver doesn't support certain formats such as interlaced) that can cause QUERY_DV_TIMINGS to return an error, even though the receiver could sync.
Usually the hardware has some bits that tell whether there is a signal (usually the TMDS clock) and whether it could sync or not (H and V syncs).
That's really all you need to test here.
*status = 0;
- else
*status |= V4L2_IN_ST_NO_SIGNAL;
- mutex_unlock(&state->lock);
- return 0;
+};
+static int tda1997x_s_dv_timings(struct v4l2_subdev *sd,
struct v4l2_dv_timings *timings)
+{
- struct tda1997x_state *state = to_state(sd);
- int ret;
- v4l_dbg(1, debug, state->client, "%s\n", __func__);
- if (!timings)
return -EINVAL;
These 'if (!timings)' tests are not needed and can be dropped here and elsewhere.
- if (v4l2_match_dv_timings(&state->timings, timings, 0, false))
return 0; /* no changes */
- if (!v4l2_valid_dv_timings(timings, &tda1997x_dv_timings_cap,
NULL, NULL))
return -ERANGE;
- mutex_lock(&state->lock);
- state->timings = *timings;
- /* setup frame detection window and VHREF timing generator */
- ret = tda1997x_configure_vhref(sd);
- if (ret)
goto error;
- /* configure colorspace conversion */
- ret = tda1997x_configure_csc(sd);
- if (ret)
goto error;
- mutex_unlock(&state->lock);
- return 0;
+error:
- mutex_unlock(&state->lock);
- return ret;
+}
+static int tda1997x_g_dv_timings(struct v4l2_subdev *sd,
struct v4l2_dv_timings *timings)
+{
- struct tda1997x_state *state = to_state(sd);
- v4l_dbg(1, debug, state->client, "%s\n", __func__);
- if (!timings)
return -EINVAL;
- mutex_lock(&state->lock);
- *timings = state->timings;
- mutex_unlock(&state->lock);
- return 0;
+}
+static int tda1997x_query_dv_timings(struct v4l2_subdev *sd,
struct v4l2_dv_timings *timings)
+{
- struct tda1997x_state *state = to_state(sd);
- int ret;
- v4l_dbg(1, debug, state->client, "%s\n", __func__);
- if (!timings)
return -EINVAL;
- memset(timings, 0, sizeof(struct v4l2_dv_timings));
- mutex_lock(&state->lock);
- ret = tda1997x_detect_std(state);
- if (!ret)
*timings = *state->detected_timings;
I think you can just drop the 'detected_timings' field and just do tda1997x_detect_std(state, timings); here. That way you are not tempted to use 'detected_timings' in places where you shouldn't :-)
- mutex_unlock(&state->lock);
- return ret;
+}
+static int tda1997x_s_stream(struct v4l2_subdev *sd, int enable) +{
- struct tda1997x_state *state = to_state(sd);
- v4l_dbg(1, debug, state->client, "%s %d\n", __func__, enable);
- mutex_lock(&state->lock);
- if (!state->detected_timings)
v4l_dbg(1, debug, state->client, "Invalid HDMI signal\n");
- mutex_unlock(&state->lock);
- return 0;
+}
I'd drop this. It isn't helpful.
+static const struct v4l2_subdev_video_ops tda1997x_video_ops = {
- .g_input_status = tda1997x_g_input_status,
- .s_dv_timings = tda1997x_s_dv_timings,
- .g_dv_timings = tda1997x_g_dv_timings,
- .query_dv_timings = tda1997x_query_dv_timings,
- .s_stream = tda1997x_s_stream,
+};
+/* -----------------------------------------------------------------------------
- v4l2_subdev_pad_ops
- */
+static int tda1997x_init_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg)
+{
- struct tda1997x_state *state = to_state(sd);
- struct v4l2_mbus_framefmt *mf;
- mf = v4l2_subdev_get_try_format(sd, cfg, 0);
- mf->code = state->mbus_codes[0];
- return 0;
+}
+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);
- v4l_dbg(1, debug, state->client, "%s %d/%d\n", __func__,
code->index, ARRAY_SIZE(state->mbus_codes));
- if (code->index >= ARRAY_SIZE(state->mbus_codes))
return -EINVAL;
- if (!state->mbus_codes[code->index])
return -EINVAL;
- code->code = state->mbus_codes[code->index];
- 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;
Ouch. Just drop this.
- memset(format, 0, sizeof(*format));
- bt = &state->detected_timings->bt;
and use &state->timings.bt here. That is how the receiver is configured.
- format->width = bt->width;
- format->height = bt->height;
- format->colorspace = state->colorimetry.colorspace;
- format->field = (bt->interlaced) ?
V4L2_FIELD_SEQ_TB : V4L2_FIELD_NONE;
- return 0;
+}
The *only* time you need to care about the actual detected timings is when userspace calls QUERY_DV_TIMINGS. Never anywhere else.
s_input_status is used to test some basics (do I have a signal, can I lock) but doesn't go 'in-depth'.
If the interrupt routine detects a resolution change or it loses a stable signal, then it sends an event to userspace, but nothing else should change.
Look at adv7604.c: the only time it attempts to detect the timings with read_stdi() is when called from query_dv_timings (and log_status for debugging).
Regards,
Hans