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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Hans, and thanks for the review.

On Thu, 4 Mar 2021 16:37:53 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

>Hi Maxime,
>
>Some more code review comments:
>

>> +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

[...]

>> +
>> +	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

[...]

>> +
>> +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.

[...]

>> +		/* 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.

[...]

>> +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 :)

[...]

>> +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.

[...]

>> +
>> +	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