Hello Sakari, 2011/9/6 Sakari Ailus <sakari.ailus@xxxxxx>: > On Tue, Sep 06, 2011 at 09:35:24AM +0000, Bastian Hecht wrote: >> Hello Sakari, >> >> 2011/9/6 Sakari Ailus <sakari.ailus@xxxxxx>: >> > On Tue, Sep 06, 2011 at 09:01:15AM +0000, Bastian Hecht wrote: >> >> 2011/9/6 Sakari Ailus <sakari.ailus@xxxxxx>: >> >> > On Tue, Sep 06, 2011 at 07:56:40AM +0000, Bastian Hecht wrote: >> >> >> Hello Sakari! >> >> > >> >> > Hi Bastian, >> >> > >> >> >> 2011/9/6 Sakari Ailus <sakari.ailus@xxxxxx>: >> >> >> > Hi Bastian, >> >> >> > >> >> >> > On Mon, Sep 05, 2011 at 09:32:55AM +0000, Bastian Hecht wrote: >> >> >> >> 2011/9/1 Sakari Ailus <sakari.ailus@xxxxxx>: >> >> >> >> > On Thu, Sep 01, 2011 at 11:14:08AM +0200, Sylwester Nawrocki wrote: >> >> >> >> >> Hi Sakari, >> >> >> >> >> >> >> >> >> >> On 09/01/2011 10:47 AM, Sakari Ailus wrote: >> >> >> >> >> > On Thu, Sep 01, 2011 at 09:15:20AM +0200, Guennadi Liakhovetski wrote: >> >> >> >> >> >> On Thu, 1 Sep 2011, Sakari Ailus wrote: >> >> >> >> >> >> >> >> >> >> >> >>> On Wed, Aug 31, 2011 at 03:27:49PM +0000, Bastian Hecht wrote: >> >> >> >> >> >>>> 2011/8/28 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: >> >> >> >> >> >>> [clip] >> >> >> >> >> >>>>> If I'm not mistaken V4L2_CID_PRIVATE_BASE is deprecated. >> >> >> >> >> >>>> >> >> >> >> >> >>>> I checked at http://v4l2spec.bytesex.org/spec/x542.htm, googled >> >> >> >> >> >>>> "V4L2_CID_PRIVATE_BASE deprecated" and read >> >> >> >> >> >>>> Documentation/feature-removal-schedule.txt. I couldn't find anything. >> >> >> >> >> >>> >> >> >> >> >> >>> Hmm. Did you happen to check when that has been written? :) >> >> >> >> >> >>> >> >> >> >> >> >>> Please use this one instead: >> >> >> >> >> >>> >> >> >> >> >> >>> <URL:http://hverkuil.home.xs4all.nl/spec/media.html> >> >> >> >> >> >> >> >> >> >> >> >> "Drivers can also implement their own custom controls using >> >> >> >> >> >> V4L2_CID_PRIVATE_BASE and higher values." >> >> >> >> >> >> >> >> >> >> >> >> Which specific location describes V4L2_CID_PRIVATE_BASE differently there? >> >> >> >> >> > >> >> >> >> >> > That was a general comment, not related to the private base. There's no >> >> >> >> >> > use for a three-year-old spec as a reference! >> >> >> >> >> > >> >> >> >> >> > The control framework does not support private controls, for example. The >> >> >> >> >> > controls should be put to their own class in videodev2.h nowadays, that's my >> >> >> >> >> > understanding. Cc Hans. >> >> >> >> >> >> >> >> >> >> Is this really the case that we close the door for private controls in >> >> >> >> >> the mainline kernel ? Or am I misunderstanding something ? >> >> >> >> >> How about v4l2_ctrl_new_custom() ? >> >> >> >> >> >> >> >> >> >> What if there are controls applicable to single driver only ? >> >> >> >> >> Do we really want to have plenty of such in videodev2.h ? >> >> >> >> > >> >> >> >> > We have some of those already in videodev2.h. I'm not certain if I'm happy >> >> >> >> > with this myself, considering e.g. that we could get a few truckloads of >> >> >> >> > only camera lens hardware specific controls in the near future. >> >> >> >> >> >> >> >> So in my case (as these are controls that might be used by others too) >> >> >> >> I should add something like >> >> >> >> >> >> >> >> #define V4L2_CID_BLUE_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+19) >> >> >> >> #define V4L2_CID_RED_SATURATION (V4L2_CID_CAMERA_CLASS_BASE+20) >> >> >> > >> >> >> > What do these two controls do? Do they control gain or something else? >> >> >> >> >> >> Hmm. Maybe I named them a bit unsharp. It is the U Saturation and V >> >> >> Saturation. To me it looks like turning up the saturation in HSV >> >> >> space, but only for either the blue or the red channel. This would >> >> >> correspond to V4L2_CID_{RED,BLUE}_BALANCE when I read the docs. They >> >> >> say it is "{Red,Blue} chroma balance". >> >> >> >> >> >> I have other controls for that I used V4L2_CID_{RED,BLUE}_BALANCE. >> >> >> These are gains. So in fact I should swap them in my code and the >> >> >> remaining question is, how to name the red and blue gain controls. >> >> > >> >> > I think Laurent had a similar issue in his Aptina sensor driver. In my >> >> > opinion we need a class for low level controls such as the gain ones. Do I >> >> > understand correctly they control the red and blue pixel gain in the sensor >> >> > pixel matrix? Do you also have gain controls for the two greens? >> >> >> >> Yes, I assume that this is done there. Either in the analog circuit by >> >> decreasing the preload or digitally then. Don't know exactly. There >> >> are registers for the green pixels as well. As I used the >> >> V4L2_CID_{RED,BLUE}_BALANCE controls and there was no >> >> V4L2_CID_GREEN_BALANCE, I just skipped green as one can >> >> increase/decrease the global gain and get an arbitrary mix as well. >> >> >> >> So for these gain settings we should add these? >> >> V4L2_CID_RED_GAIN >> >> V4L2_CID_BLUE_GAIN >> >> V4L2_CID_GREEN_GAIN >> > >> > Do you have two or just one green gains? In all sensors I've seen there are >> > two. >> >> No, here is only one. > > It is a raw bayer sensor, isn't it? > >> > I think I could send an RFC on this to the list and cc you and Laurent. >> >> Ok fine, thanks! But hmmm - what do I do with my driver in the >> meantime actually? Stall the upstream process or remove my controls >> temporarily - or is there a better way? > > It is also possible to expose these controls just for this sensor, but I > would wait a little bit if that's okay for you. Your sensor driver isn't the > only one depending on these new controls --- Laurent also has one. > > I don't think this should take too long, but I can't promise that. :-) > > If you want the driver to mainline fast, then you could also submit it > without these controls implemented, and implement them in another patch when > the controls have been standardised. Ok, thanks for the info. I'll try it this way: I'll post an [PATCH/RFC] with the final version except the right control names for gain and chroma balance. So when this rfc gets through, I can simply change names and am done. >> >> >> >> #define V4L2_CID_GRAY_SCALE_IMAGE (V4L2_CID_CAMERA_CLASS_BASE+21) >> >> >> > >> >> >> > V4L2_CID_COLOR_KILLER looks like something which would fit for the purpose. >> >> >> >> >> >> Oh great! So I just take this. >> >> >> >> >> >> >> #define V4L2_CID_SOLARIZE_EFFECT (V4L2_CID_CAMERA_CLASS_BASE+22) >> >> >> > >> >> >> > Sounds interesting for a sensor. I wonder if this would fall under a menu >> >> >> > control, V4L2_CID_COLORFX. >> >> >> >> >> >> When I read the the possible enums for V4L2_CID_COLORFX, it indeed >> >> >> sounds very much like my solarize effect should be added there too. I >> >> >> found V4L2_COLORFX_BW there, too. Isn't that a duplicate of the color >> >> >> killer control then? >> >> > >> >> > In my opinion V4L2_CID_COLORFX should never be implemented in drivers for >> >> > which the hardware doesn't implement these effects in a non-parametrisable >> >> > way. This control was originally added for the OMAP 3 ISP driver but the >> >> > driver never implemented it. >> >> >> >> Your triple negation (never, doesn't, non-) is quite tricky xD >> >> If I get it right, you say that one should not use V4L2_CID_COLORFX >> >> for hardware with parametrisable effects. >> > >> > Yes. I could have written that in a more clear way. ;-) >> >> After starring dazzled for 2 minutes on it, I realized at some point >> that formal logic is your friend ;) >> >> >> My BW and Solarize effects are non-parametrisable and they can be >> >> turned on together (which makes not so much sense though - but these >> >> fun-effects like "solarize" aren't here to make sense, I guess :-) ). >> > >> > Good. >> > >> > The OMAP 3 ISP actually provides a way to set gamma tables, any effects >> > implemented using them are more or less use case specific. There are also >> > other uses for those same gamma tables, making a driver implementation for >> > effects using them non-functional in practice. >> >> Ok I see. Luckily (for me) in my sensor it is binary on/off only. >> >> >> > I think you have a valid case using this control. I think the main >> >> > difference between the two is that V4L2_COLORFX_BW is something that you >> >> > can't use with other effects while V4L2_CID_COLOR_KILLER can be used with >> >> > any of the effects. >> >> >> >> > Based on your original proposal the black/white should stay as a separate >> >> > control but the solarise should be configurable through V4L2_CID_COLORFX >> >> > menu control. So it boils down to the question whether you can use them at >> >> > the same time. >> >> >> >> I can - so it is still working to enable V4L2_COLORFX_BW and >> >> V4L2_CID_COLORFX with a new enum value, right? Is that the way to go >> >> now? >> > >> > That's my opinion, yes. >> >> So I will post an additional patch for videodev2.h with >> enum v4l2_colorfx { >> ... >> V4L2_COLORFX_SOLARIZE = 10, >> }; > > That's correct. I think the COLORFX hasn't been very well documented so far, > but it should be. I don't think many know what the solarize effect would do. > :-) I think the patch should also add that. The user control documentation > may be found in Documentation/DocBook/media/v4l/controls.xml . > > Cheers, In fact, I had no idea as well, but wikipedia teached me :) So I will prepare a documentation patch as well. http://en.wikipedia.org/wiki/Solarisation > -- > Sakari Ailus > e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx > best, Bastian -- 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
