From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:55504 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751818Ab2HBAID (ORCPT ); Wed, 1 Aug 2012 20:08:03 -0400 Date: Thu, 2 Aug 2012 03:07:56 +0300 From: Sakari Ailus To: Manjunath Hadli Cc: LMML , dlos , linux-doc@vger.kernel.org, Rob Landley , Mauro Carvalho Chehab , laurent.pinchart@ideasonboard.com Subject: Re: [PATCH] [media] davinci: vpfe: Add documentation Message-ID: <20120802000756.GM26642@valkosipuli.retiisi.org.uk> References: <1342021166-6092-1-git-send-email-manjunath.hadli@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <1342021166-6092-1-git-send-email-manjunath.hadli@ti.com> Sender: linux-media-owner@vger.kernel.org List-ID: 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 > Signed-off-by: Lad, Prabhakar > --- > 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 > + > +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? 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. > +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. > + 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? 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. > + 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. > + }; > + > +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? It'd be nice to see what's behind these enums and structs. > + enum af_focus_val_sel fv_sel; > + struct af_hmf hmf_config; > + enum rgbpos rgb_pos; > + 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. 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. > +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: > + > +Referenced MARCH 2009-REVISED JUNE 2011 > + > +Davinci DM368 TRM: > + > +Referenced APRIL 2010-REVISED JUNE 2011 > + > +Davinci Video Processing Front End (VPFE) DM36x > + > + > + > +References > +========== > + > +[1] http://git.ideasonboard.org/?p=media-ctl.git;a=summary > Kind regards, -- Sakari Ailus sakari.ailus@iki.fi