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

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

 



Hi Hans,
My comments inlined.

Hardik,
 


> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx]
> Sent: Tuesday, April 21, 2009 5:30 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
> 
> 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.
[Shah, Hardik] Video Encoder of the DSS2 supports number of PAL and NTSC standards out of them only two are supported to I just tried to be specific as 50Hz may mean any PAL standards other than PAL_BDGHI and 60 may mean any NTSC standard other than NTSC_M.  Any way this is not going to be permanent.
> 
> >> > +   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.
[Shah, Hardik] Agreed,
I will change that.
> 
> Regards,
> 
>         Hans
> 
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux