RE: [Review PATCH 3/3] OMAP2/3 V4L2 Display Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hardik,

Just a few comments on your comments :-)

> Hi Hans,
> My Comments inlined,
> Most of the comments are taken care off.
> Thanks for review.
>
> Hardik,
>
>> -----Original Message-----
>> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx]
>> Sent: Saturday, April 18, 2009 7:29 PM
>> To: Shah, Hardik
>> Cc: linux-media@xxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx; Jadav,
>> Brijesh R;
>> Hiremath, Vaibhav
>> Subject: Re: [Review PATCH 3/3] OMAP2/3 V4L2 Display Driver

>> > +config VID2_LCD_MANAGER
>> > +   bool "Use LCD Managaer"
>> > +   help
>> > +     Select this option if you want VID2 pipeline on LCD Overlay
>> manager
>> > +endchoice
>> > +
>> > +choice
>> > +        prompt "TV Mode"
>> > +        depends on VID2_TV_MANAGER || VID1_TV_MANAGER
>> > +        default NTSC_M
>> > +
>> > +config NTSC_M
>> > +        bool "Use NTSC_M mode"
>> > +        help
>> > +          Select this option if you want NTSC_M mode on TV
>> > +
>> > +config PAL_BDGHI
>> > +        bool "Use PAL_BDGHI mode"
>> > +        help
>> > +          Select this option if you want PAL_BDGHI mode on TV
>>
>> Terminology: PAL and NTSC etc. refer to broadcast standards. That is no
>> generally applicable to omap. When it comes to streaming digital video
>> there is only the 50 and 60 Hz SDTV standards. For output over a
>> Composite
>> or S-Video connector you can also choose between PAL and SECAM. There
>> are some differences between the two, although I'm not sure about the
>> details. The common saa7128 i2c device definitely has support for both.
>>
>> In this particular case you probably mean 50 or 60 Hz SDTV rather than
>> NTSC/PAL.
>>
> [Shah, Hardik] OMAP DSS is having the internal video encoder for
> converting the digital to analog standards like NTSC and PAL with a
> s-video and composite output.  Currently DSS does not support changing of
> the TV standards dynamically so I have made it as a compile time option.
> Once DSS will support that I will add the S_STD and G_STD for standards.
> Internally it will call the DSS2 library APIs to change the standard.

I don't have a problem with these config options, it's more the names
NTSC_M and PAL_BDGHI that are misleading IMHO, since it's really a matter
of 50Hz vs 60Hz and not of NTSC vs PAL.

>> > +   if (1 == vout->mirror && vout->rotation >= 0) {
>> > +           rotation_deg = (vout->rotation == 1) ?
>> > +                   3 : (vout->rotation == 3) ?
>> > +                   1 : (vout->rotation ==  2) ?
>> > +                   0 : 2;
>>
>> Or: rotation = (4 - vout->rotation) % 4;
> [Shah, Hardik] It will not work when rotation will be 0 and I want 2
> because of mirroring. (4-0) % 2 is 0 I want 2 here.  So not changing it.

You are right. Sorry about that.

>> > +static int vidioc_s_ctrl(struct file *file, void *fh, struct
>> v4l2_control
>> *a)
>> > +{
>> > +   struct omap_vout_device *vout = ((struct omap_vout_fh *)
>> fh)->vout;
>> > +
>> > +   switch (a->id) {
>> > +   case V4L2_CID_ROTATE:
>> > +   {
>> > +           int rotation = a->value;
>> > +
>> > +           if (vout->pix.pixelformat == V4L2_PIX_FMT_RGB24 &&
>> > +                           rotation != -1)
>>
>> Huh? Shouldn't this be rotation != 0?
> [Shah, Hardik] Rotation 0 means the rotation using Virtual Frame Buffer
> Rotation (VRFB) engine.  It does not support rotation with packed RGB24
> format.  -1 means VRFB is not used.  So it should be -1;

This really isn't right. a->value is the rotate control value. And that's
defined as 0, 90, 180 or 270 according to queryctrl. Not -1. The value -1
is a purely internal value which is also why I am opposed to its use. It
is very confusing.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

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

[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux