Re: [PATCH v4 3/3] media: i2c: Introduce a driver for the Techwell TW9900 decoder

Hi Hans, and thanks for the review.

On Thu, 4 Mar 2021 16:37:53 +0100
Hans Verkuil [off-list ref] wrote:

Hi Maxime,

Some more code review comments:

quoted

+static int tw9900_set_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
+
+	mbus_fmt->width = tw9900->cur_mode->width;
+	mbus_fmt->height = tw9900->cur_mode->height;  

These two lines are already done in tw9900_fill_fmt.

Yes right, I'll remove that

[...]

quoted

+
+	return 0;
+}
+
+static int tw9900_get_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
+
+	return 0;
+}  

In fact, tw9900_set_fmt is identical to tw9900_get_fmt. I would just drop
tw9900_set_fmt and point both .get_fmt and .set_fmt to the same function.

OK, that will be cleaner indeed

[...]

quoted

+
+static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index >= 1)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+
+	return 0;
+}
+
+static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	u32 index = fse->index;
+
+	if (index >= 1)
+		return -EINVAL;
+
+	fse->code = MEDIA_BUS_FMT_UYVY8_2X8;
+
+	fse->min_width  = supported_modes[index].width;
+	fse->max_width  = supported_modes[index].width;
+	fse->max_height = supported_modes[index].height;
+	fse->min_height = supported_modes[index].height;
+
+	return 0;
+}  

This function is not typically used by video receivers since the framesize is
fixed depending on the standard. So there is nothing to enumerate.

It is wrong in any case since it reports just supported_modes[0] (i.e. PAL)
even if the current mode is NTSC. But it can just be dropped for video receivers.

Ok thanks, I'll drop that then.

[...]

quoted

+		/* Wait for VSync lock */
+		for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) {
+			u8 status = tw9900_read_reg(tw9900->client,
+						    TW9900_REG_CHIP_STATUS);
+			if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) &&
+			    (status & TW9900_REG_CHIP_STATUS_VLOCK))
+				break;
+
+			msleep(VSYNC_POLL_INTERVAL_MS);
+		}  

Why do you have to wait for a vsync lock?

Most drivers just start regardless of lock. If there is no lock, then there
is either no data being streamed (so the DMA of the video bridge will be idle
as well) or it is just transmitting noise (typical for SDTV receivers). At least
until a valid signal appears eventually.

In this case, it will transmit noise.

As stated a bit below, this was implemented because this decoder
actually supports automatic standard detection, but the reported
standard can only be read once the vsync lock is acquired.

So this is a remainder of what I implemented to try to get the standard
detection work, but I can drop that for now.

[...]

quoted

+static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *try_fmt;
+
+	try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0);
+
+	/* Initialize try_fmt */
+	tw9900_fill_fmt(tw9900->cur_mode, try_fmt);
+
+	return 0;
+}  

Since the format is fixed based on the current standard, there is no point
in initializing try_fmt as it won't be used. So just drop tw9900_open altogether.

Ok I'll drop that :)

[...]

quoted

+static const struct v4l2_subdev_video_ops tw9900_video_ops = {
+	.s_std		= tw9900_s_std,
+	.g_std		= tw9900_g_std,  

Can the tw9900 detect the standard? (I.e. PAL, SECAM, NTSC)

If so, you should implement querystd. I see that none of the other tw*.c drivers
support this, so I suspect there is no hardware support for this.

So, there's hardware support for this, and I've been trying to get this
to work for a while. I've come to a point where the standard detection
"almost" works, but detects the wrong standard about once every 10
occurences. I don't know if this is due to the hardware I'm testing
this on, my setup, or the decoder itself.

This is in a setup where the standard can change on the fly (I have 2
cameras, one that streams PAL, one that streams NTSC, that are connected
to the TW9900 through a switch, and I have to make so that we can
detect a standard change (due to switching a camera) on the fly while
the stream is started.

The standard detection is also a process that is quite long and that
has to be manually started, and then checked regularly to see if the
decoder successfully identified a standard.

I do have a followup question, which is when querystd() would be called
in a "normal" scenarion (I feel that my usecase seems a bit off-track
compared to classic usecases). Is it when the stream is started, or
stopped ?

You also must implement g_tvnorms to report the TV standards that the hardware
can understand.

Ok I didn't know about that, I'll implement that then.

[...]

quoted

+
+	tw9900->subdev.internal_ops = &tw9900_internal_ops;
+	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;  

This is a duplicate of a similar '|=' above.

My bad, I'll remove that line.

Regards,

Hans

Thanks,

Maxime