Em 03-10-2011 03:30, Hans Verkuil escreveu:
> On Monday, October 03, 2011 04:17:06 Mauro Carvalho Chehab wrote:
>> Em 02-10-2011 18:18, Javier Martinez Canillas escreveu:
>>> On Sun, Oct 2, 2011 at 6:30 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
>>>> Hi Javier,
>>>>
>>>> Thanks for the patch! It's very interesting to see a driver for a video
>>>> decoder using the MC interface. Before this we've had just image sensors.
>>>>
>>>
>>> Hello Sakari,
>>>
>>> Thanks for your comments.
>>>
>>>> Javier Martinez Canillas wrote:
>>>>> + /* use the standard status register */
>>>>> + std_status = tvp5150_read(sd, TVP5150_STATUS_REG_5);
>>>>> + else
>>>>> + /* use the standard register itself */
>>>>> + std_status = std;
>>>>
>>>> Braces would be nice here.
>>>>
>>>
>>> Ok.
>>>
>>>>> + switch (std_status & VIDEO_STD_MASK) {
>>>>> + case VIDEO_STD_NTSC_MJ_BIT:
>>>>> + case VIDEO_STD_NTSC_MJ_BIT_AS:
>>>>> + return STD_NTSC_MJ;
>>>>> +
>>>>> + case VIDEO_STD_PAL_BDGHIN_BIT:
>>>>> + case VIDEO_STD_PAL_BDGHIN_BIT_AS:
>>>>> + return STD_PAL_BDGHIN;
>>>>> +
>>>>> + default:
>>>>> + return STD_INVALID;
>>>>> + }
>>>>> +
>>>>> + return STD_INVALID;
>>>>
>>>> This return won't do anything.
>>>>
>>>
>>> Yes, will clean this.
>>>
>>>>> @@ -704,19 +812,19 @@ static int tvp5150_set_std(struct v4l2_subdev *sd, v4l2_std_id std)
>>>>> if (std == V4L2_STD_ALL) {
>>>>> fmt = 0; /* Autodetect mode */
>>>>> } else if (std & V4L2_STD_NTSC_443) {
>>>>> - fmt = 0xa;
>>>>> + fmt = VIDEO_STD_NTSC_4_43_BIT;
>>>>> } else if (std & V4L2_STD_PAL_M) {
>>>>> - fmt = 0x6;
>>>>> + fmt = VIDEO_STD_PAL_M_BIT;
>>>>> } else if (std & (V4L2_STD_PAL_N | V4L2_STD_PAL_Nc)) {
>>>>> - fmt = 0x8;
>>>>> + fmt = VIDEO_STD_PAL_COMBINATION_N_BIT;
>>>>> } else {
>>>>> /* Then, test against generic ones */
>>>>> if (std & V4L2_STD_NTSC)
>>>>> - fmt = 0x2;
>>>>> + fmt = VIDEO_STD_NTSC_MJ_BIT;
>>>>> else if (std & V4L2_STD_PAL)
>>>>> - fmt = 0x4;
>>>>> + fmt = VIDEO_STD_PAL_BDGHIN_BIT;
>>>>> else if (std & V4L2_STD_SECAM)
>>>>> - fmt = 0xc;
>>>>> + fmt = VIDEO_STD_SECAM_BIT;
>>>>> }
>>>>
>>>> Excellent! Less magic numbers...
>>>>
>>>>>
>>>>> +static struct v4l2_mbus_framefmt *
>>>>> +__tvp5150_get_pad_format(struct tvp5150 *tvp5150, struct v4l2_subdev_fh *fh,
>>>>> + unsigned int pad, enum v4l2_subdev_format_whence which)
>>>>> +{
>>>>> + switch (which) {
>>>>> + case V4L2_SUBDEV_FORMAT_TRY:
>>>>> + return v4l2_subdev_get_try_format(fh, pad);
>>>>> + case V4L2_SUBDEV_FORMAT_ACTIVE:
>>>>> + return tvp5150->format;
>>>>> + default:
>>>>> + return NULL;
>>>>
>>>> Hmm. This will never happen, but is returning NULL the right thing to
>>>> do? An easy alternative is to just replace this with if (which may only
>>>> have either of the two values).
>>>>
>>>
>>> Ok I'll cleanup, I was being a bit paranoid there :)
>>>
>>>>> +
>>>>> +static int tvp5150_set_pad_format(struct v4l2_subdev *subdev,
>>>>> + struct v4l2_subdev_fh *fh,
>>>>> + struct v4l2_subdev_format *format)
>>>>> +{
>>>>> + struct tvp5150 *tvp5150 = to_tvp5150(subdev);
>>>>> + tvp5150->std_idx = STD_INVALID;
>>>>
>>>> The above assignment will always be overwritten immediately.
>>>>
>>>
>>> Yes, since tvp515x_query_current_std() already returns STD_INVALID on
>>> error the assignment is not needed. Will change that.
>>>
>>>>> + tvp5150->std_idx = tvp515x_query_current_std(subdev);
>>>>> + if (tvp5150->std_idx == STD_INVALID) {
>>>>> + v4l2_err(subdev, "Unable to query std\n");
>>>>> + return 0;
>>>>
>>>> Isn't this an error?
>>>>
>>>
>>> Yes, I'll change to report the error to the caller.
>>>
>>>>> + * tvp515x_mbus_fmt_cap() - V4L2 decoder interface handler for try/s/g_mbus_fmt
>>>>
>>>> The name of the function is different.
>>>>
>>>
>>> Yes, I'll change that.
>>>
>>>>> static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
>>>>> .s_routing = tvp5150_s_routing,
>>>>> + .s_stream = tvp515x_s_stream,
>>>>> + .enum_mbus_fmt = tvp515x_enum_mbus_fmt,
>>>>> + .g_mbus_fmt = tvp515x_mbus_fmt,
>>>>> + .try_mbus_fmt = tvp515x_mbus_fmt,
>>>>> + .s_mbus_fmt = tvp515x_mbus_fmt,
>>>>> + .g_parm = tvp515x_g_parm,
>>>>> + .s_parm = tvp515x_s_parm,
>>>>> + .s_std_output = tvp5150_s_std,
>>>>
>>>> Do we really need both video and pad format ops?
>>>>
>>>
>>> Good question, I don't know. Can this device be used as a standalone
>>> v4l2 device? Or is supposed to always be a part of a video streaming
>>> pipeline as a sub-device with a source pad? Sorry if my questions are
>>> silly but as I stated before, I'm a newbie with v4l2 and MCF.
>>
>> The tvp5150 driver is used on some em28xx devices. It is nice to add auto-detection
>> code to the driver, but converting it to the media bus should be done with
>> enough care to not break support for the existing devices.
>
> So in other words, the tvp5150 driver needs both pad and non-pad ops.
> Eventually all non-pad variants in subdev drivers should be replaced by the
> pad variants so you don't have duplication of ops. But that will take a lot
> more work.
>
>> Also, as I've argued with Laurent before, the expected behavior is that the standards
>> format selection should be done via the video node, and not via the media
>> controller node. The V4L2 API has enough support for all you need to do with the
>> video decoder, so there's no excuse to duplicate it with any other API.
>
> This is relevant for bridge drivers, not for subdev drivers.
>
>> The media controller API is there not to replace V4L2, but to complement
>> it where needed.
>
> That will be a nice discussion during the workshop :-)
>
>> In the specific code of standards auto-detection, a few drivers currently support
>> this feature. They're (or should be) coded to do is:
>>
>> If V4L2_STD_ALL is used, the driver should autodetect the video standard of the
>> currently tuned channel.
>
> Actually, this is optional. As per the spec:
>
> "When the standard set is ambiguous drivers may return EINVAL or choose any of
> the requested standards."
Yes. The entire auto-detection thing is optional. Several devices aren't capable of
auto-detecting standards. Btw, userspace support for standards auto-detection is
another chapter. I don't think they implement standards auto-detection.
There are even some applications (qv4l2 and tvtime, for example) that aren't capable of
refreshing the maximum height when the standard changes.
> Nor does the spec say anything about doing an autodetect when STD_ALL is passed
> in. Most drivers will just set the std to PAL or NTSC in this case.
Auto-detection is device specific. That's why most applications don't handle it
well (or don't even care on trying to do it). Btw, I did some tests with autodetection
on a device with a saa7115, and I found some bugs. Just sent a fix for it.
> If you want to autodetect, then use QUERYSTD. Applications cannot rely on drivers
> to handle V4L2_STD_ALL the way you say.
>
>> The detected standard can be returned to userspace via VIDIOC_G_STD.
>
> No! G_STD always returns the current *selected* standard. Only QUERYSTD returns
> the detected standard.
Yes, you're right. I should not try to answer emails when I'm lazy enough to not
look in to the code ;)
Anyway, the thing is that V4L2 API has enough support for auto-detection. There's
no need to duplicate stuff with MC API.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
