lkml.org 
[lkml]   [2021]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 <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

\
 
 \ /
  Last update: 2021-03-11 14:32    [W:0.074 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site