Hi Sakari, On Thu, Aug 16, 2012 at 9:53 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote: > Hi Manju, > > On Thu, Aug 09, 2012 at 09:13:52AM +0530, Manjunath Hadli wrote: >> Hi Sakari, >> >> Thank you for the comments. > > Thanks for the graphs! > >> On Thursday 02 August 2012 05:37 AM, Sakari Ailus wrote: >> > Hi Manju, >> > >> > Thanks for the patch. >> > >> > Please make sure these patches reach linux-media next time. If they do >> > not, >> > it severely limits the number of potential reviewers. I don't know >> > why, but >> > the original patch isn't on linux-media even if the list was cc'd. >> > >> > Dropping linux-kernel from cc. >> > >> > Manjunath Hadli wrote: >> >> Add documentation on the Davinci VPFE driver. Document the subdevs, >> >> and private IOTCLs the driver implements >> >> >> >> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx> >> >> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx> >> >> --- >> >> Documentation/video4linux/davinci-vpfe-mc.txt | 263 >> >> +++++++++++++++++++++++++ >> >> 1 files changed, 263 insertions(+), 0 deletions(-) >> >> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt >> >> >> >> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt >> >> b/Documentation/video4linux/davinci-vpfe-mc.txt >> >> new file mode 100644 >> >> index 0000000..968194f >> >> --- /dev/null >> >> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt >> >> @@ -0,0 +1,263 @@ >> >> +Davinci Video processing Front End (VPFE) driver >> >> + >> >> +Copyright (C) 2012 Texas Instruments Inc >> >> + >> >> +Contacts: Manjunath Hadli <manjunath.hadli@xxxxxx> >> >> + >> >> +Introduction >> >> +============ >> >> + >> >> +This file documents the Texas Instruments Davinci Video processing >> >> Front End >> >> +(VPFE) driver located under drivers/media/video/davinci. The >> >> original driver >> >> +exists for Davinci VPFE, which is now being changed to Media Controller >> >> +Framework. >> >> + >> >> +Currently the driver has been successfully used on the following >> >> version of Davinci: >> >> + >> >> + DM365/DM368 >> >> + >> >> +The driver implements V4L2, Media controller and v4l2_subdev >> >> interfaces. >> >> +Sensor, lens and flash drivers using the v4l2_subdev interface in >> >> the kernel >> >> +are supported. >> >> + >> >> + >> >> +Split to subdevs >> >> +================ >> >> + >> >> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks >> >> inside the VPFE >> >> +having one subdev to represent it. Each of the subdevs provide a >> >> V4L2 subdev >> >> +interface to userspace. >> >> + >> >> + DAVINCI CCDC >> >> + DAVINCI PREVIEWER >> >> + DAVINCI RESIZER >> >> + DAVINCI AEW >> >> + DAVINCI AF >> >> + >> >> +Each possible link in the VPFE is modeled by a link in the Media >> >> controller >> >> +interface. For an example program see [1]. >> >> + >> >> + >> >> +Private IOCTLs >> >> +============== >> >> + >> >> +The Davinci Video processing Front End (VPFE) driver supports >> >> standard V4L2 >> >> +IOCTLs and controls where possible and practical. Much of the >> >> functions provided >> >> +by the VPFE, however, does not fall under the standard IOCTLs. >> >> + >> >> +In general, there is a private ioctl for configuring each of the blocks >> >> +containing hardware-dependent functions. >> >> + >> >> +The following private IOCTLs are supported: >> >> + >> >> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM >> >> +Description: >> >> + Sets/Gets the parameters required by the previewer module >> >> +Parameter: >> >> + /** >> >> + * struct prev_module_param- structure to configure preview modules >> >> + * @version: Version of the preview module >> >> + * @len: Length of the module config structure >> >> + * @module_id: Module id >> >> + * @param: pointer to module config parameter. >> >> + */ >> >> + struct prev_module_param { >> >> + char version[IMP_MAX_NAME_SIZE]; >> >> + unsigned short len; >> >> + unsigned short module_id; >> >> + void *param; >> >> + }; >> > >> > In addition to what Laurent commented on this, could the version >> > information be passed in struct media_entity_desc instead? >> I plan to leave out the version. >> > >> > As a general comment, it's a bad idea to design an API that allows >> > passing >> > blobs, especially when the expected size of the blobs isn't known. That >> > really equals to asking for trouble. >> > >> > That said, I know this is an area where complete documentation is acarce, >> > but I think that at least the memory layout of the current blob pointers >> > should be visible in the struct definitions whenever possible. See >> > e.g. the >> > OMAP 3 ISP driver. >> I have proposed using a union of structures instead of the void blob. >> I also saw the OMAP implementation, and they are pointers (but not void). >> To me the union approach looks better as it keeps the architecture >> intact and does not necessitate an >> explicit copy_from_user. Which of these ways do you suggest? > > I would suggest to use the approach taken in the OMAP 3 ISP driver as it has > one obvious advantage over the union of pointers: it makes it possible to > perform atomic changes to the configuration. > > However, changes to configuration done through controls and the private > IOCTL can never be atomic as they're done using a different IOCTL. This is > something that will require some work at the API level in the future. > > I'm fine with both union of struct pointers or a struct of struct pointers > (as in the OMAP 3 ISP driver). Laurent? > >> > >> >> +2: IOCTL: PREV_S_CONFIG/PREV_G_CONFIG >> >> +Description: >> >> + Sets/Gets the configuration required by the previewer channel >> >> +Parameter: >> >> + /** >> >> + * struct prev_channel_config - structure for configuring the >> >> previewer channel >> >> + * @len: Length of the user configuration >> >> + * @config: pointer to either single shot config or continuous >> >> + */ >> >> + struct prev_channel_config { >> >> + unsigned short len; >> >> + void *config; >> >> + }; >> >> + >> >> +3: IOCTL: PREV_ENUM_CAP >> >> +Description: >> >> + Queries the modules available in the image processor for preview >> >> the >> >> + input image. >> >> +Parameter: >> >> + /** >> >> + * struct prev_cap - structure to enumerate capabilities of >> >> previewer >> >> + * @index: application use this to iterate over the available >> >> modules >> >> + * @version: version of the preview module >> >> + * @module_id: module id >> >> + * @control: control operation allowed in continuous mode? 1 - >> >> allowed, 0 - not allowed >> >> + * @path: path on which the module is sitting >> >> + * @module_name: module name >> >> + */ >> >> + struct prev_cap { >> >> + unsigned short index; >> >> + char version[IMP_MAX_NAME_SIZE]; >> >> + unsigned short module_id; >> > >> > Huh? How many sub-modules do the preview modules have in different DM >> > series >> > chips, and which ones have the same? >> > >> > The user still has to know quite lot about the hardware; I'd give the >> > responsibility of knowing the hardware to the user also here --- the user >> > has to know this exactly anyway. >> I am going to remove this IOCTL as agreed. Will keep only a SET and a >> GET IOTCL. > > PREV_[GS]_CONFIG or something else? That's also a blob passing IOCTL. > >> >> + char control; >> >> + enum imp_data_paths path; >> >> + char module_name[IMP_MAX_NAME_SIZE]; >> >> + }; >> >> + >> >> +4: IOCTL: RSZ_S_CONFIG/RSZ_G_CONFIG >> >> +Description: >> >> + Sets/Gets the configuration required by the resizer channel >> >> +Parameter: >> >> + /** >> >> + * struct rsz_channel_config - structure for configuring the >> >> resizer channel >> >> + * @chain: chain this resizer at the previewer output >> >> + * @len: length of the user configuration >> >> + * @config: pointer to either single shot config or continuous >> >> + */ >> >> + struct rsz_channel_config { >> >> + unsigned char chain; >> > >> > How many resizers do you have? Wouldn't the Media controller link >> > configuration be the right way to configure this? >> Yes. The Media controller links the entities to act as single shot or >> continuous. >> The above variable can be removed. There are two resizers. > > If you have two independent resizer blocks then you should represent them as > two subdevs. The reason is that the scaling factor is configured using the > COMPOSE target on the sink pad, making the scaling factor specific to the > subdev. > >> > A media-ctl --print-dot graph on the device layout would be >> > appreciated if >> > the driver is in a state where it can be easily produced. >> Sure will send it. >> > >> >> + unsigned short len; >> >> + void *config; >> >> + }; >> >> + >> >> +5: IOCTL: VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS >> >> +Description: >> >> + Sets/Gets the CCDC parameter >> >> +Parameter: >> >> + /** >> >> + * struct ccdc_config_params_raw - structure for configuring >> >> ccdc params >> >> + * @linearize: linearization parameters for image sensor data input >> >> + * @df_csc: data formatter or CSC >> >> + * @dfc: defect Pixel Correction (DFC) configuration >> >> + * @bclamp: Black/Digital Clamp configuration >> >> + * @gain_offset: Gain, offset adjustments >> >> + * @culling: Culling >> >> + * @pred: predictor for DPCM compression >> >> + * @horz_offset: horizontal offset for Gain/LSC/DFC >> >> + * @vert_offset: vertical offset for Gain/LSC/DFC >> >> + * @col_pat_field0: color pattern for field 0 >> >> + * @col_pat_field1: color pattern for field 1 >> >> + * @data_size: data size from 8 to 16 bits >> >> + * @data_shift: data shift applied before storing to SDRAM >> >> + * @test_pat_gen: enable input test pattern generation >> >> + */ >> >> + struct ccdc_config_params_raw { >> >> + struct ccdc_linearize linearize; >> >> + struct ccdc_df_csc df_csc; >> >> + struct ccdc_dfc dfc; >> >> + struct ccdc_black_clamp bclamp; >> >> + struct ccdc_gain_offsets_adj gain_offset; >> >> + struct ccdc_cul culling; >> >> + enum ccdc_dpcm_predictor pred; >> >> + unsigned short horz_offset; >> >> + unsigned short vert_offset; >> >> + struct ccdc_col_pat col_pat_field0; >> >> + struct ccdc_col_pat col_pat_field1; >> >> + enum ccdc_data_size data_size; >> >> + enum ccdc_datasft data_shift; >> >> + unsigned char test_pat_gen; >> > >> > Are the struct definitions available somewhere? I bet more than the test >> > pattern Laurent suggested might be implementable as controls. The dpcm >> > predictor, for example. >> I will check on the DPSM test pattern. The definitions are available >> at:http://davinci-linux-open-source.1494791.n2.nabble.com/RESEND-RFC-PATCH-v4-00-15-RFC-for-Media-Controller-capture-driver-for-DM365-td7003648.html > > Thanks! > > I think the DPCM predictor should be made a control in the image processing > controls class. The test pattern would fit there as well I think. > For test pattern you meant control to enable/disable it ? Thanks and Regards, --Prabhakar > <URL:http://hverkuil.home.xs4all.nl/spec/media.html#image-process-controls> > >> > >> >> + }; >> >> + >> >> +6: IOCTL: AF_S_PARAM/AF_G_PARAM >> >> +Description: >> >> + AF_S_PARAM performs the hardware setup and sets the parameter for >> >> + AF engine.AF_G_PARAM gets the parameter setup in AF engine >> >> +Parameter: >> >> + /** >> >> + * struct af_configuration - struct to configure parameters of >> >> AF engine >> >> + * @alaw_enable: ALAW status >> >> + * @fv_sel: focus value selection >> >> + * @hmf_config: HMF configurations >> >> + * @rgb_pos: RGB Positions. Only applicable with AF_HFV_ONLY >> >> selection >> >> + * @iir_config: IIR filter configurations >> >> + * @fir_config: FIR filter configuration >> >> + * @paxel_config: Paxel parameters >> >> + * @mode: accumulator mode >> >> + */ >> >> + struct af_configuration { >> >> + enum af_enable_flag alaw_enable; >> > >> > What does alaw_enable do? Is it set by the user? >> This will be removed. We will take it from mbus format. > > Ok. > >> > It'd be nice to see what's behind these enums and structs. >> Please see the above link. >> > >> >> + enum af_focus_val_sel fv_sel; >> >> + struct af_hmf hmf_config; >> >> + enum rgbpos rgb_pos; > > ^ > This information is available in the media bus format. > >> >> + struct af_iir iir_config; >> >> + struct af_fir fir_config; >> >> + struct af_paxel paxel_config; >> >> + enum af_mode mode; >> >> + }; >> >> + >> >> +7: IOCTL: AF_GET_STAT >> >> +Description: >> >> + Copy the entire statistics located in application buffer >> >> + to user space from the AF engine >> >> +Parameter: >> >> + /** >> >> + * struct af_statdata - structure to get statistics from AF engine >> >> + * @buffer: pointer to buffer >> >> + * @buf_length: length of buffer >> >> + */ >> >> + struct af_statdata { >> >> + void *buffer; >> >> + int buf_length; >> >> + }; >> > >> > I think the proper way to pass statistics to the user space has been >> > discussed for years, but AFAIR --- please correct if I'm mistaken --- the >> > agreement was to implement statistics as video buffer queue. It is, after >> > all, very similar to regular image data in how it's handled by the >> > hardware >> > and when it's needed by the user and even some of the statistics can >> > be even >> > considered images themselves. >> Depending on which statistics we are talking about, the data size might >> vary, and >> in general much saller than a image that it is based on. I am not sure >> if we need a >> full fledged buffer exchange mechanism to exchange statistics data. >> Anyway, can you >> point me to the discussion? > > The data size varies according to the configuration which isn't unexpected. > The video buffers, such as the non-video buffers, just need to be large > enough to hold the biggest statistics the user intends to capture --- just > as for images. > > I can't find a discussion now --- it might have been a real verbal > discussion. :-) There are numerous benefits both in kernel and user space: > videobuf2 gives you the buffer handling; enabling and disabling works > through enabling or disabling the link between the statistics subdev and the > video node, and you get the standard user space API as well. For instance, > you only need to configure the statistics engine using a private IOCTL after > which you can use yavta to capture statistics (for testing). The OMAP 3 ISP > driver either copies the statistics data or used to use the buffers one by > one independently of whether the user was still accessing them. That's a lot > of complexity both in kernel and user space which can be almost completely > avoided by just using videobuf2. > > Hans, Laurent: could you ack this? > >> > So, this should be done using video buffers instead. I know the OMAP 3 >> > ISP >> > doesn't, but at the time of the implementation this was seen otherwise. >> > You'll save a lot of trouble by using video buffers since you won't >> > need to >> > implement the same functionality that already exists in videobuf2 for the >> > statistics. >> Is there any driver which uses video buffers for statistics data >> exchange using video buffers? If so can you point me to it? If it is a >> quickie, I lan to make the changes. Else I will plan to get this driver >> into the mainline without AF/AEW and add patches later. > > Currently no. At the time the OMAP 3 ISP driver was written, the rough > concensus was on using private IOCTLs. We had to have one driver to > implement statistics passing using private IOCTLs to find that's a bad idea. > :-) That's definitely my opinion on that. > >> > >> >> +8: IOCTL: AEW_S_PARAM/AEW_G_PARAM >> >> +Description: >> >> + AEW_S_PARAM performs the hardware setup and sets the parameter for >> >> + AEW engine.AEW_G_PARAM gets the parameter setup in AEW engine >> >> +Parameter: >> >> + /** >> >> + * struct aew_configuration - struct to configure parameters of >> >> AEW engine >> >> + * @alaw_enable: A-law status >> >> + * @format: AE/AWB output format >> >> + * @sum_shift: AW/AWB right shift value for sum of pixels >> >> + * @saturation_limit: Saturation Limit >> >> + * @hmf_config: HMF configurations >> >> + * @window_config: Window for AEW Engine >> >> + * @blackwindow_config: Black Window >> >> + */ >> >> + struct aew_configuration { >> >> + enum aew_enable_flag alaw_enable; >> >> + enum aew_output_format out_format; >> >> + char sum_shift; >> >> + int saturation_limit; >> >> + struct aew_hmf hmf_config; >> >> + struct aew_window window_config; >> >> + struct aew_black_window blackwindow_config; >> >> + }; >> >> + >> >> +9: IOCTL: AEW_GET_STAT >> >> +Description: >> >> + Copy the entire statistics located in application buffer >> >> + to user space from the AEW engine >> >> +Parameter: >> >> + /** >> >> + * struct aew_statdata - structure to get statistics from AEW >> >> engine >> >> + * @buffer: pointer to buffer >> >> + * @buf_length: length of buffer >> >> + */ >> >> + struct aew_statdata { >> >> + void *buffer; >> >> + int buf_length; >> >> + }; >> > >> > Same as for AF. >> > >> >> + >> >> + >> >> +Technical reference manuals (TRMs) and other documentation >> >> +========================================================== >> >> + >> >> +Davinci DM365 TRM: >> >> +<URL:http://www.ti.com/lit/ds/sprs457e/sprs457e.pdf> >> >> +Referenced MARCH 2009-REVISED JUNE 2011 >> >> + >> >> +Davinci DM368 TRM: >> >> +<URL:http://www.ti.com/lit/ds/sprs668c/sprs668c.pdf> >> >> +Referenced APRIL 2010-REVISED JUNE 2011 >> >> + >> >> +Davinci Video Processing Front End (VPFE) DM36x >> >> +<URL:http://www.ti.com/lit/ug/sprufg8c/sprufg8c.pdf> >> >> + >> >> + >> >> +References >> >> +========== >> >> + >> >> +[1] http://git.ideasonboard.org/?p=media-ctl.git;a=summary >> >> > > Cheers, > > -- > Sakari Ailus > e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html