Re: [PATCH] [media] davinci: vpfe: Add documentation

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

Thanks for the patch.

On Wednesday 11 July 2012 21:09:26 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@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> ---
>  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@ti.com>
> +
> +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

Does the driver still support the DM644x ?

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

s/Davinic/Davinci/

> VPFE
> +having one subdev to represent it. Each of the subdevs provide a V4L2
> subdev
> +interface to userspace.
> +
> +	DAVINCI CCDC
> +	DAVINCI PREVIEWER
> +	DAVINCI RESIZER

the DM36x VPFE documentation doesn't split the hardware in CCDC, PREVIEWER and 
RESIZER modules, but in ISIF, IPIPEIF and IPIPE. Why don't you use those names 
? It looks like you're introducing an abstraction layer on top of the existing 
driver. Why is that needed, why don't you just port the driver to the MC API 
instead ?

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

Who is responsible for filling this field, the application or the driver ?

> +	 * @len: Length of the module config structure
> +	 * @module_id: Module id
> +	 * @param: pointer to module config parameter.

What is module_id for ? What does param point to ?

> +	 */
> +	struct prev_module_param {
> +		char version[IMP_MAX_NAME_SIZE];

Is there a need to express the version as a string instead of an integer ?

> +		unsigned short len;
> +		unsigned short module_id;
> +		void *param;
> +	};
> +
> +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;
> +	};

What's the difference between parameters and configuration ? What does config 
point to ?

> +
> +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;
> +		char control;
> +		enum imp_data_paths path;
> +		char module_name[IMP_MAX_NAME_SIZE];
> +	};

Enumerating internal modules is exactly what the MC API was designed for. Why 
do you reimplement that using private ioctls ?

> +
> +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;
> +		unsigned short len;
> +		void *config;
> +	};

Same question as for the preview engine, what does this do, what does config 
point to ? What is the chain parameter for ?

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

Can't you use subdev V4L2 controls for gains ?

> +	 * @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

Shouldn't color patterns be computed automatically by the driver based on the 
media bus pixel code ?

> +	 * @data_size: data size from 8 to 16 bits
> +	 * @data_shift: data shift applied before storing to SDRAM

Ditto, this should probably be computed automatically.

> +	 * @test_pat_gen: enable input test pattern generation

You could use a subdev V4L2 control for that.

> +	 */
> +	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;
> +	};
> +
> +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;

Can this be computed automatically based on the media bus pixel code ? 

> +		enum af_focus_val_sel fv_sel;
> +		struct af_hmf hmf_config;
> +		enum rgbpos rgb_pos;

Same here ?

> +		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;
> +	};

The OMAP3 ISP driver also needs to export statistics data to userspace. We 
should design a common API here.

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

Computed automatically as well ?

> +		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 comment as for AF_GET_STAT.

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

-- 
Regards,

Laurent Pinchart

RE: [PATCH] [media] davinci: vpfe: Add documentation

[Hadli, Manjunath]

Hi Laurent,
  Thank you for your comments. 
 
On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> Thanks for the patch.
> 
> On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > ---
> >  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@ti.com>
> > +
> > +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
> 
> Does the driver still support the DM644x ?

Yes. The driver supports DM6446. We will add the Dm6446x patches on these.

> 
> > +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
> 
> s/Davinic/Davinci/
OK. Thanks.

> 
> > VPFE
> > +having one subdev to represent it. Each of the subdevs provide a V4L2
> > subdev
> > +interface to userspace.
> > +
> > +	DAVINCI CCDC
> > +	DAVINCI PREVIEWER
> > +	DAVINCI RESIZER
> 
> the DM36x VPFE documentation doesn't split the hardware in CCDC, PREVIEWER and 
> RESIZER modules, but in ISIF, IPIPEIF and IPIPE. Why don't you use those names 
> ? It looks like you're introducing an abstraction layer on top of the existing 
> driver. Why is that needed, why don't you just port the driver to the MC API 
> instead ?
Please see below my comment for "enumerating internal modules".
> 
> > +	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
> 
> Who is responsible for filling this field, the application or the driver ?
The application is responsible for filling this info. He would enumerate the
capabilities first and  set them using S_PARAM/G_PARAM.

> 
> > +	 * @len: Length of the module config structure
> > +	 * @module_id: Module id
> > +	 * @param: pointer to module config parameter.
> 
> What is module_id for ? What does param point to ?
There are a lot of tiny modules in the previewer/resizer which are enumerated 
as individual modules. The param points to the parameter set that the module
expects to be set.

> 
> > +	 */
> > +	struct prev_module_param {
> > +		char version[IMP_MAX_NAME_SIZE];
> 
> Is there a need to express the version as a string instead of an integer ?
It could be integer. It is generally a fixed point num, and easy to read it
as a string than an integer. Can I keep it as a string?

> 
> > +		unsigned short len;
> > +		unsigned short module_id;
> > +		void *param;
> > +	};
> > +
> > +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;
> > +	};
> 
> What's the difference between parameters and configuration ? What does config 
> point to ?
Config is setting which is required for a subdev to function based on what it is
set for (single shot/continuous.) common to all platforms. Parameters are the
settings for individual small sub-ips which might be slightly different from one
platform to another. Config points to prev_single_shot_config or  prev_continuous_config
currently defined in linux/dm3656ipipe.h. I think we will move it to a common location.

> 
> > +
> > +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;
> > +		char control;
> > +		enum imp_data_paths path;
> > +		char module_name[IMP_MAX_NAME_SIZE];
> > +	};
> 
> Enumerating internal modules is exactly what the MC API was designed for. Why 
> do you reimplement that using private ioctls ?
The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot of them 
In a way that may be bewildering to the end-user to be able to connect them quickly and properly.
But overall, these are nothing but exposed subips of what we call as CCDC,Previewer  and Resizer.It
Made a lot of logical sense to keep it that way, give a default configuration for everything, and if
at all the user wants the fine grain config control, be able to give (mainly for the configurations- 
not so much for connections). In most of the cases the param IOTCLs are only used for fine-tuning the image and not expected to be used as a regular flow of a normal application. I do not think there could be any justification for making all these nitty gritty which keep changing for each IPs as part of regular V4L2 IOCTLs. In future, if there is a common theme that emerges, we could definitely relook into this.
> 
> > +
> > +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;
> > +		unsigned short len;
> > +		void *config;
> > +	};
> 
> Same question as for the preview engine, what does this do, what does config 
> point to ? What is the chain parameter for ?
Config points to rsz_single_shot_config and rsz_continuous_config defined in dm365ipipe.h As mentioned earlier, we could move the definition to a common header. 
The chain param is to indicate if resizer and previewer are chained. We will remove this.
> 
> > +
> > +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
> 
> Can't you use subdev V4L2 controls for gains ?
In that case only gain has to be taken out as a generic IOCTL. Since that is is 
The parameter which could be taken out of this big structure
> 
> > +	 * @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
> 
> Shouldn't color patterns be computed automatically by the driver based on the 
> media bus pixel code ?
OK.
> 
> > +	 * @data_size: data size from 8 to 16 bits
> > +	 * @data_shift: data shift applied before storing to SDRAM
> 
> Ditto, this should probably be computed automatically.
Do you want to define new MBUS formats for these?
> 
> > +	 * @test_pat_gen: enable input test pattern generation
> 
> You could use a subdev V4L2 control for that.
Ok.
> 
> > +	 */
> > +	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;
> > +	};
> > +
> > +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;
> 
> Can this be computed automatically based on the media bus pixel code ? 
Ok. Sure.
> 
> > +		enum af_focus_val_sel fv_sel;
> > +		struct af_hmf hmf_config;
> > +		enum rgbpos rgb_pos;
> 
> Same here ?
New MBUS formats here as well?
> 
> > +		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;
> > +	};
> 
> The OMAP3 ISP driver also needs to export statistics data to userspace. We 
> should design a common API here.
 Sure we can take it up sometime later.
> 
> > +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;
> 
> Computed automatically as well ?
OK.
> 
> > +		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 comment as for AF_GET_STAT.
Yes, we can discuss about it to make it common. I would prefer we get this driver in and make amends when you are doing it for OMAP.
> 
> > +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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

RE: [PATCH] [media] davinci: vpfe: Add documentation

[Hadli, Manjunath]

Hi Laurent,
  Thank you for your comments. 
 
On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> Thanks for the patch.
> 
> On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > ---
> >  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@ti.com>
> > +
> > +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
> 
> Does the driver still support the DM644x ?

Yes. The driver supports DM6446. We will add the Dm6446x patches on these.

> 
> > +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
> 
> s/Davinic/Davinci/
OK. Thanks.

> 
> > VPFE
> > +having one subdev to represent it. Each of the subdevs provide a V4L2
> > subdev
> > +interface to userspace.
> > +
> > +	DAVINCI CCDC
> > +	DAVINCI PREVIEWER
> > +	DAVINCI RESIZER
> 
> the DM36x VPFE documentation doesn't split the hardware in CCDC, PREVIEWER and 
> RESIZER modules, but in ISIF, IPIPEIF and IPIPE. Why don't you use those names 
> ? It looks like you're introducing an abstraction layer on top of the existing 
> driver. Why is that needed, why don't you just port the driver to the MC API 
> instead ?
Please see below my comment for "enumerating internal modules".
> 
> > +	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
> 
> Who is responsible for filling this field, the application or the driver ?
The application is responsible for filling this info. He would enumerate the
capabilities first and  set them using S_PARAM/G_PARAM.

> 
> > +	 * @len: Length of the module config structure
> > +	 * @module_id: Module id
> > +	 * @param: pointer to module config parameter.
> 
> What is module_id for ? What does param point to ?
There are a lot of tiny modules in the previewer/resizer which are enumerated 
as individual modules. The param points to the parameter set that the module
expects to be set.

> 
> > +	 */
> > +	struct prev_module_param {
> > +		char version[IMP_MAX_NAME_SIZE];
> 
> Is there a need to express the version as a string instead of an integer ?
It could be integer. It is generally a fixed point num, and easy to read it
as a string than an integer. Can I keep it as a string?

> 
> > +		unsigned short len;
> > +		unsigned short module_id;
> > +		void *param;
> > +	};
> > +
> > +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;
> > +	};
> 
> What's the difference between parameters and configuration ? What does config 
> point to ?
Config is setting which is required for a subdev to function based on what it is
set for (single shot/continuous.) common to all platforms. Parameters are the
settings for individual small sub-ips which might be slightly different from one
platform to another. Config points to prev_single_shot_config or  prev_continuous_config
currently defined in linux/dm3656ipipe.h. I think we will move it to a common location.

> 
> > +
> > +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;
> > +		char control;
> > +		enum imp_data_paths path;
> > +		char module_name[IMP_MAX_NAME_SIZE];
> > +	};
> 
> Enumerating internal modules is exactly what the MC API was designed for. Why 
> do you reimplement that using private ioctls ?
The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot of them 
In a way that may be bewildering to the end-user to be able to connect them quickly and properly.
But overall, these are nothing but exposed subips of what we call as CCDC,Previewer  and Resizer.It
Made a lot of logical sense to keep it that way, give a default configuration for everything, and if
at all the user wants the fine grain config control, be able to give (mainly for the configurations- 
not so much for connections). In most of the cases the param IOTCLs are only used for fine-tuning the image and not expected to be used as a regular flow of a normal application. I do not think there could be any justification for making all these nitty gritty which keep changing for each IPs as part of regular V4L2 IOCTLs. In future, if there is a common theme that emerges, we could definitely relook into this.
> 
> > +
> > +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;
> > +		unsigned short len;
> > +		void *config;
> > +	};
> 
> Same question as for the preview engine, what does this do, what does config 
> point to ? What is the chain parameter for ?
Config points to rsz_single_shot_config and rsz_continuous_config defined in dm365ipipe.h As mentioned earlier, we could move the definition to a common header. 
The chain param is to indicate if resizer and previewer are chained. We will remove this.
> 
> > +
> > +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
> 
> Can't you use subdev V4L2 controls for gains ?
In that case only gain has to be taken out as a generic IOCTL. Since that is is 
The parameter which could be taken out of this big structure
> 
> > +	 * @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
> 
> Shouldn't color patterns be computed automatically by the driver based on the 
> media bus pixel code ?
OK.
> 
> > +	 * @data_size: data size from 8 to 16 bits
> > +	 * @data_shift: data shift applied before storing to SDRAM
> 
> Ditto, this should probably be computed automatically.
Do you want to define new MBUS formats for these?
> 
> > +	 * @test_pat_gen: enable input test pattern generation
> 
> You could use a subdev V4L2 control for that.
Ok.
> 
> > +	 */
> > +	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;
> > +	};
> > +
> > +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;
> 
> Can this be computed automatically based on the media bus pixel code ? 
Ok. Sure.
> 
> > +		enum af_focus_val_sel fv_sel;
> > +		struct af_hmf hmf_config;
> > +		enum rgbpos rgb_pos;
> 
> Same here ?
New MBUS formats here as well?
> 
> > +		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;
> > +	};
> 
> The OMAP3 ISP driver also needs to export statistics data to userspace. We 
> should design a common API here.
 Sure we can take it up sometime later.
> 
> > +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;
> 
> Computed automatically as well ?
OK.
> 
> > +		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 comment as for AF_GET_STAT.
Yes, we can discuss about it to make it common. I would prefer we get this driver in and make amends when you are doing it for OMAP.
> 
> > +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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>

[snip]

> > > +	DAVINCI CCDC
> > > +	DAVINCI PREVIEWER
> > > +	DAVINCI RESIZER
> > 
> > the DM36x VPFE documentation doesn't split the hardware in CCDC, PREVIEWER
> > and RESIZER modules, but in ISIF, IPIPEIF and IPIPE. Why don't you use
> > those names ? It looks like you're introducing an abstraction layer on
> > top of the existing driver. Why is that needed, why don't you just port
> > the driver to the MC API instead ?
> 
> Please see below my comment for "enumerating internal modules".
> 
> > > +	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
> > 
> > Who is responsible for filling this field, the application or the driver ?
> 
> The application is responsible for filling this info. He would enumerate the
> capabilities first and  set them using S_PARAM/G_PARAM.
> 
> > > +	 * @len: Length of the module config structure
> > > +	 * @module_id: Module id
> > > +	 * @param: pointer to module config parameter.
> > 
> > What is module_id for ? What does param point to ?
> 
> There are a lot of tiny modules in the previewer/resizer which are
> enumerated as individual modules. The param points to the parameter set
> that the module expects to be set.
> 
> > > +	 */
> > > +	struct prev_module_param {
> > > +		char version[IMP_MAX_NAME_SIZE];
> > 
> > Is there a need to express the version as a string instead of an integer ?
> 
> It could be integer. It is generally a fixed point num, and easy to read it
> as a string than an integer. Can I keep it as a string?
> 
> > > +		unsigned short len;
> > > +		unsigned short module_id;
> > > +		void *param;
> > > +	};
> > > +
> > > +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;
> > > +	};
> > 
> > What's the difference between parameters and configuration ? What does
> > config point to ?
> 
> Config is setting which is required for a subdev to function based on what
> it is set for (single shot/continuous.) common to all platforms. Parameters
> are the settings for individual small sub-ips which might be slightly
> different from one platform to another. Config points to
> prev_single_shot_config or  prev_continuous_config currently defined in
> linux/dm3656ipipe.h. I think we will move it to a common location.
> > > +
> > > +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;
> > > +		char control;
> > > +		enum imp_data_paths path;
> > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > +	};
> > 
> > Enumerating internal modules is exactly what the MC API was designed for.
> > Why do you reimplement that using private ioctls ?
> 
> The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> of them In a way that may be bewildering to the end-user to be able to
> connect them quickly and properly. But overall, these are nothing but
> exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> of logical sense to keep it that way, give a default configuration for
> everything, and if at all the user wants the fine grain config control, be
> able to give (mainly for the configurations- not so much for connections).
> In most of the cases the param IOTCLs are only used for fine-tuning the
> image and not expected to be used as a regular flow of a normal
> application. I do not think there could be any justification for making all
> these nitty gritty which keep changing for each IPs as part of regular V4L2
> IOCTLs. In future, if there is a common theme that emerges, we could
> definitely relook into this.
> > > +
> > > +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;
> > > +		unsigned short len;
> > > +		void *config;
> > > +	};
> > 
> > Same question as for the preview engine, what does this do, what does
> > config point to ? What is the chain parameter for ?
> 
> Config points to rsz_single_shot_config and rsz_continuous_config defined in
> dm365ipipe.h As mentioned earlier, we could move the definition to a common
> header. The chain param is to indicate if resizer and previewer are
> chained. We will remove this.
> > > +
> > > +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
> > 
> > Can't you use subdev V4L2 controls for gains ?
> 
> In that case only gain has to be taken out as a generic IOCTL. Since that is
> is The parameter which could be taken out of this big structure
> 
> > > +	 * @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
> > 
> > Shouldn't color patterns be computed automatically by the driver based on
> > the media bus pixel code ?
> 
> OK.
> 
> > > +	 * @data_size: data size from 8 to 16 bits
> > > +	 * @data_shift: data shift applied before storing to SDRAM
> > 
> > Ditto, this should probably be computed automatically.
> 
> Do you want to define new MBUS formats for these?
> 
> > > +	 * @test_pat_gen: enable input test pattern generation
> > 
> > You could use a subdev V4L2 control for that.
> 
> Ok.
> 
> > > +	 */
> > > +	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;
> > > +	};

[snip
> > > +
> > > +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;
> > 
> > Can this be computed automatically based on the media bus pixel code ?
> 
> Ok. Sure.
> 
> > > +		enum af_focus_val_sel fv_sel;
> > > +		struct af_hmf hmf_config;
> > > +		enum rgbpos rgb_pos;
> > 
> > Same here ?
> 
> New MBUS formats here as well?
> 
> > > +		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;
> > > +	};
> > 
> > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > should design a common API here.
> 
>  Sure we can take it up sometime later.

[snip]

> > > +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 comment as for AF_GET_STAT.
> 
> Yes, we can discuss about it to make it common. I would prefer we get this
> driver in and make amends when you are doing it for OMAP.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>

[snip]

> > > +	DAVINCI CCDC
> > > +	DAVINCI PREVIEWER
> > > +	DAVINCI RESIZER
> > 
> > the DM36x VPFE documentation doesn't split the hardware in CCDC, PREVIEWER
> > and RESIZER modules, but in ISIF, IPIPEIF and IPIPE. Why don't you use
> > those names ? It looks like you're introducing an abstraction layer on
> > top of the existing driver. Why is that needed, why don't you just port
> > the driver to the MC API instead ?
> 
> Please see below my comment for "enumerating internal modules".
> 
> > > +	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
> > 
> > Who is responsible for filling this field, the application or the driver ?
> 
> The application is responsible for filling this info. He would enumerate the
> capabilities first and  set them using S_PARAM/G_PARAM.
> 
> > > +	 * @len: Length of the module config structure
> > > +	 * @module_id: Module id
> > > +	 * @param: pointer to module config parameter.
> > 
> > What is module_id for ? What does param point to ?
> 
> There are a lot of tiny modules in the previewer/resizer which are
> enumerated as individual modules. The param points to the parameter set
> that the module expects to be set.
> 
> > > +	 */
> > > +	struct prev_module_param {
> > > +		char version[IMP_MAX_NAME_SIZE];
> > 
> > Is there a need to express the version as a string instead of an integer ?
> 
> It could be integer. It is generally a fixed point num, and easy to read it
> as a string than an integer. Can I keep it as a string?
> 
> > > +		unsigned short len;
> > > +		unsigned short module_id;
> > > +		void *param;
> > > +	};
> > > +
> > > +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;
> > > +	};
> > 
> > What's the difference between parameters and configuration ? What does
> > config point to ?
> 
> Config is setting which is required for a subdev to function based on what
> it is set for (single shot/continuous.) common to all platforms. Parameters
> are the settings for individual small sub-ips which might be slightly
> different from one platform to another. Config points to
> prev_single_shot_config or  prev_continuous_config currently defined in
> linux/dm3656ipipe.h. I think we will move it to a common location.
> > > +
> > > +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;
> > > +		char control;
> > > +		enum imp_data_paths path;
> > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > +	};
> > 
> > Enumerating internal modules is exactly what the MC API was designed for.
> > Why do you reimplement that using private ioctls ?
> 
> The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> of them In a way that may be bewildering to the end-user to be able to
> connect them quickly and properly. But overall, these are nothing but
> exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> of logical sense to keep it that way, give a default configuration for
> everything, and if at all the user wants the fine grain config control, be
> able to give (mainly for the configurations- not so much for connections).
> In most of the cases the param IOTCLs are only used for fine-tuning the
> image and not expected to be used as a regular flow of a normal
> application. I do not think there could be any justification for making all
> these nitty gritty which keep changing for each IPs as part of regular V4L2
> IOCTLs. In future, if there is a common theme that emerges, we could
> definitely relook into this.
> > > +
> > > +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;
> > > +		unsigned short len;
> > > +		void *config;
> > > +	};
> > 
> > Same question as for the preview engine, what does this do, what does
> > config point to ? What is the chain parameter for ?
> 
> Config points to rsz_single_shot_config and rsz_continuous_config defined in
> dm365ipipe.h As mentioned earlier, we could move the definition to a common
> header. The chain param is to indicate if resizer and previewer are
> chained. We will remove this.
> > > +
> > > +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
> > 
> > Can't you use subdev V4L2 controls for gains ?
> 
> In that case only gain has to be taken out as a generic IOCTL. Since that is
> is The parameter which could be taken out of this big structure
> 
> > > +	 * @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
> > 
> > Shouldn't color patterns be computed automatically by the driver based on
> > the media bus pixel code ?
> 
> OK.
> 
> > > +	 * @data_size: data size from 8 to 16 bits
> > > +	 * @data_shift: data shift applied before storing to SDRAM
> > 
> > Ditto, this should probably be computed automatically.
> 
> Do you want to define new MBUS formats for these?
> 
> > > +	 * @test_pat_gen: enable input test pattern generation
> > 
> > You could use a subdev V4L2 control for that.
> 
> Ok.
> 
> > > +	 */
> > > +	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;
> > > +	};

[snip
> > > +
> > > +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;
> > 
> > Can this be computed automatically based on the media bus pixel code ?
> 
> Ok. Sure.
> 
> > > +		enum af_focus_val_sel fv_sel;
> > > +		struct af_hmf hmf_config;
> > > +		enum rgbpos rgb_pos;
> > 
> > Same here ?
> 
> New MBUS formats here as well?
> 
> > > +		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;
> > > +	};
> > 
> > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > should design a common API here.
> 
>  Sure we can take it up sometime later.

[snip]

> > > +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 comment as for AF_GET_STAT.
> 
> Yes, we can discuss about it to make it common. I would prefer we get this
> driver in and make amends when you are doing it for OMAP.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

Please ignore the previous reply, I've hit the sent button too soon by 
mistake.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>

[snip]

> > > +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
> > 
> > Who is responsible for filling this field, the application or the driver ?
> 
> The application is responsible for filling this info. He would enumerate the
> capabilities first and  set them using S_PARAM/G_PARAM.

And what's the point of the application setting the version field ? How does 
the driver use it ?

> > > +	 * @len: Length of the module config structure
> > > +	 * @module_id: Module id
> > > +	 * @param: pointer to module config parameter.
> > 
> > What is module_id for ? What does param point to ?
> 
> There are a lot of tiny modules in the previewer/resizer which are
> enumerated as individual modules. The param points to the parameter set
> that the module expects to be set.

Why don't you implement something similar to 
VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?

> > > +	 */
> > > +	struct prev_module_param {
> > > +		char version[IMP_MAX_NAME_SIZE];
> > 
> > Is there a need to express the version as a string instead of an integer ?
> 
> It could be integer. It is generally a fixed point num, and easy to read it
> as a string than an integer. Can I keep it as a string?

Let's first decide whether a version field is needed at all :-)

> > > +		unsigned short len;
> > > +		unsigned short module_id;
> > > +		void *param;
> > > +	};
> > > +
> > > +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;
> > > +	};
> > 
> > What's the difference between parameters and configuration ? What does
> > config point to ?
> 
> Config is setting which is required for a subdev to function based on what
> it is set for (single shot/continuous.) common to all platforms. Parameters
> are the settings for individual small sub-ips which might be slightly
> different from one platform to another. Config points to
> prev_single_shot_config or  prev_continuous_config currently defined in
> linux/dm3656ipipe.h. I think we will move it to a common location.

Why don't you implement something similar to 
VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same for 
the resizer configuration ioctls) ?

> > > +
> > > +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;
> > > +		char control;
> > > +		enum imp_data_paths path;
> > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > +	};
> > 
> > Enumerating internal modules is exactly what the MC API was designed for.
> > Why do you reimplement that using private ioctls ?
> 
> The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> of them In a way that may be bewildering to the end-user to be able to
> connect them quickly and properly. But overall, these are nothing but
> exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> of logical sense to keep it that way, give a default configuration for
> everything, and if at all the user wants the fine grain config control, be
> able to give (mainly for the configurations- not so much for connections).
> In most of the cases the param IOTCLs are only used for fine-tuning the
> image and not expected to be used as a regular flow of a normal
> application. I do not think there could be any justification for making all
> these nitty gritty which keep changing for each IPs as part of regular V4L2
> IOCTLs. In future, if there is a common theme that emerges, we could
> definitely relook into this.

I totally agree with you on this, the tiny sub-blocks should not be exposed as 
through the MC API. However, I would go one step further : I wouldn't expose 
them through a private ioctl either. What would a userspace application do 
with this information that it couldn't do with just the entity name and its 
revision number ?

[snip]

> > > +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
> > 
> > Can't you use subdev V4L2 controls for gains ?
> 
> In that case only gain has to be taken out as a generic IOCTL. Since that is
> is The parameter which could be taken out of this big structure

That's correct.

> > > +	 * @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
> > 
> > Shouldn't color patterns be computed automatically by the driver based on
> > the media bus pixel code ?
> 
> OK.
> 
> > > +	 * @data_size: data size from 8 to 16 bits
> > > +	 * @data_shift: data shift applied before storing to SDRAM
> > 
> > Ditto, this should probably be computed automatically.
> 
> Do you want to define new MBUS formats for these?

The media bus format contains information about the data width, so I think 
those fields are redundant.

> > > +	 * @test_pat_gen: enable input test pattern generation
> > 
> > You could use a subdev V4L2 control for that.
> 
> Ok.
> 
> > > +	 */
> > > +	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;
> > > +	};
> > > +

[snip]

> > > +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;
> > > +	};
> > 
> > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > should design a common API here.
> 
>  Sure we can take it up sometime later.

[snip]

> > > +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 comment as for AF_GET_STAT.
> 
> Yes, we can discuss about it to make it common. I would prefer we get this
> driver in and make amends when you are doing it for OMAP.

OK, but then please start a discussion on the mailing list about this topic 
(CC'ing David Cohen as he might be interested).

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>

[snip]

> > > +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
> > 
> > Who is responsible for filling this field, the application or the driver ?
> 
> The application is responsible for filling this info. He would enumerate the
> capabilities first and  set them using S_PARAM/G_PARAM.

And what's the point of the application setting the version field ? How does 
the driver use it ?

> > > +	 * @len: Length of the module config structure
> > > +	 * @module_id: Module id
> > > +	 * @param: pointer to module config parameter.
> > 
> > What is module_id for ? What does param point to ?
> 
> There are a lot of tiny modules in the previewer/resizer which are
> enumerated as individual modules. The param points to the parameter set
> that the module expects to be set.

Why don't you implement something similar to 
VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?

> > > +	 */
> > > +	struct prev_module_param {
> > > +		char version[IMP_MAX_NAME_SIZE];
> > 
> > Is there a need to express the version as a string instead of an integer ?
> 
> It could be integer. It is generally a fixed point num, and easy to read it
> as a string than an integer. Can I keep it as a string?

Let's first decide whether a version field is needed at all :-)

> > > +		unsigned short len;
> > > +		unsigned short module_id;
> > > +		void *param;
> > > +	};
> > > +
> > > +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;
> > > +	};
> > 
> > What's the difference between parameters and configuration ? What does
> > config point to ?
> 
> Config is setting which is required for a subdev to function based on what
> it is set for (single shot/continuous.) common to all platforms. Parameters
> are the settings for individual small sub-ips which might be slightly
> different from one platform to another. Config points to
> prev_single_shot_config or  prev_continuous_config currently defined in
> linux/dm3656ipipe.h. I think we will move it to a common location.

Why don't you implement something similar to 
VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same for 
the resizer configuration ioctls) ?

> > > +
> > > +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;
> > > +		char control;
> > > +		enum imp_data_paths path;
> > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > +	};
> > 
> > Enumerating internal modules is exactly what the MC API was designed for.
> > Why do you reimplement that using private ioctls ?
> 
> The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> of them In a way that may be bewildering to the end-user to be able to
> connect them quickly and properly. But overall, these are nothing but
> exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> of logical sense to keep it that way, give a default configuration for
> everything, and if at all the user wants the fine grain config control, be
> able to give (mainly for the configurations- not so much for connections).
> In most of the cases the param IOTCLs are only used for fine-tuning the
> image and not expected to be used as a regular flow of a normal
> application. I do not think there could be any justification for making all
> these nitty gritty which keep changing for each IPs as part of regular V4L2
> IOCTLs. In future, if there is a common theme that emerges, we could
> definitely relook into this.

I totally agree with you on this, the tiny sub-blocks should not be exposed as 
through the MC API. However, I would go one step further : I wouldn't expose 
them through a private ioctl either. What would a userspace application do 
with this information that it couldn't do with just the entity name and its 
revision number ?

[snip]

> > > +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
> > 
> > Can't you use subdev V4L2 controls for gains ?
> 
> In that case only gain has to be taken out as a generic IOCTL. Since that is
> is The parameter which could be taken out of this big structure

That's correct.

> > > +	 * @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
> > 
> > Shouldn't color patterns be computed automatically by the driver based on
> > the media bus pixel code ?
> 
> OK.
> 
> > > +	 * @data_size: data size from 8 to 16 bits
> > > +	 * @data_shift: data shift applied before storing to SDRAM
> > 
> > Ditto, this should probably be computed automatically.
> 
> Do you want to define new MBUS formats for these?

The media bus format contains information about the data width, so I think 
those fields are redundant.

> > > +	 * @test_pat_gen: enable input test pattern generation
> > 
> > You could use a subdev V4L2 control for that.
> 
> Ok.
> 
> > > +	 */
> > > +	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;
> > > +	};
> > > +

[snip]

> > > +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;
> > > +	};
> > 
> > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > should design a common API here.
> 
>  Sure we can take it up sometime later.

[snip]

> > > +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 comment as for AF_GET_STAT.
> 
> Yes, we can discuss about it to make it common. I would prefer we get this
> driver in and make amends when you are doing it for OMAP.

OK, but then please start a discussion on the mailing list about this topic 
(CC'ing David Cohen as he might be interested).

-- 
Regards,

Laurent Pinchart

RE: [PATCH] [media] davinci: vpfe: Add documentation

[Hadli, Manjunath]

Laurent,
 Thank you for your comments.

On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> > On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> 
> [snip]
> 
> > > > +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
> > > 
> > > Who is responsible for filling this field, the application or the driver ?
> > 
> > The application is responsible for filling this info. He would enumerate the
> > capabilities first and  set them using S_PARAM/G_PARAM.
> 
> And what's the point of the application setting the version field ? How does 
> the driver use it ?
The version may not be required. Will remove it.
> 
> > > > +	 * @len: Length of the module config structure
> > > > +	 * @module_id: Module id
> > > > +	 * @param: pointer to module config parameter.
> > > 
> > > What is module_id for ? What does param point to ?
> > 
> > There are a lot of tiny modules in the previewer/resizer which are
> > enumerated as individual modules. The param points to the parameter set
> > that the module expects to be set.
> 
> Why don't you implement something similar to 
> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
I feel if we implement direct IOCTLS there might be many of them. To make sure than independent of the number of internal modules present, having the same IOCTL used for all modules is a good idea.

> 
> > > > +	 */
> > > > +	struct prev_module_param {
> > > > +		char version[IMP_MAX_NAME_SIZE];
> > > 
> > > Is there a need to express the version as a string instead of an integer ?
> > 
> > It could be integer. It is generally a fixed point num, and easy to read it
> > as a string than an integer. Can I keep it as a string?
> 
> Let's first decide whether a version field is needed at all :-)
Will remove.
> 
> > > > +		unsigned short len;
> > > > +		unsigned short module_id;
> > > > +		void *param;
> > > > +	};
> > > > +
> > > > +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;
> > > > +	};
> > > 
> > > What's the difference between parameters and configuration ? What does
> > > config point to ?
> > 
> > Config is setting which is required for a subdev to function based on what
> > it is set for (single shot/continuous.) common to all platforms. Parameters
> > are the settings for individual small sub-ips which might be slightly
> > different from one platform to another. Config points to
> > prev_single_shot_config or  prev_continuous_config currently defined in
> > linux/dm3656ipipe.h. I think we will move it to a common location.
> 
> Why don't you implement something similar to 
> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same for 
> the resizer configuration ioctls) ?
>
Ditto. 
> > > > +
> > > > +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;
> > > > +		char control;
> > > > +		enum imp_data_paths path;
> > > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > > +	};
> > > 
> > > Enumerating internal modules is exactly what the MC API was designed for.
> > > Why do you reimplement that using private ioctls ?
> > 
> > The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> > of them In a way that may be bewildering to the end-user to be able to
> > connect them quickly and properly. But overall, these are nothing but
> > exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> > of logical sense to keep it that way, give a default configuration for
> > everything, and if at all the user wants the fine grain config control, be
> > able to give (mainly for the configurations- not so much for connections).
> > In most of the cases the param IOTCLs are only used for fine-tuning the
> > image and not expected to be used as a regular flow of a normal
> > application. I do not think there could be any justification for making all
> > these nitty gritty which keep changing for each IPs as part of regular V4L2
> > IOCTLs. In future, if there is a common theme that emerges, we could
> > definitely relook into this.
> 
> I totally agree with you on this, the tiny sub-blocks should not be exposed as 
> through the MC API. However, I would go one step further : I wouldn't expose 
> them through a private ioctl either. What would a userspace application do 
> with this information that it couldn't do with just the entity name and its 
> revision number ?
Not exposing the full functionality might not be an option. The driver gets used by different kinds of users. Some might want to use only the basic features, but many would like to have the full control in terms of setting all the parameters. Since IPIPE is so much about tuning, not having a fine grain control on its parameters is not an option.

> 
> [snip]
> 
> > > > +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
> > > 
> > > Can't you use subdev V4L2 controls for gains ?
> > 
> > In that case only gain has to be taken out as a generic IOCTL. Since that is
> > is The parameter which could be taken out of this big structure
> 
> That's correct.
> 
> > > > +	 * @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
> > > 
> > > Shouldn't color patterns be computed automatically by the driver based on
> > > the media bus pixel code ?
> > 
> > OK.
> > 
> > > > +	 * @data_size: data size from 8 to 16 bits
> > > > +	 * @data_shift: data shift applied before storing to SDRAM
> > > 
> > > Ditto, this should probably be computed automatically.
> > 
> > Do you want to define new MBUS formats for these?
> 
> The media bus format contains information about the data width, so I think 
> those fields are redundant.
The specific fields here have the control of specifying the datawidth from 9 bits to 16 bits. Did you want us to implement media bus format for all these variations? Just to make sure we do not get held up, I will send a separate patch on mediabus formats for these variations for review. In the mean time, we will go ahead with this.

> 
> > > > +	 * @test_pat_gen: enable input test pattern generation
> > > 
> > > You could use a subdev V4L2 control for that.
> > 
> > Ok.
> > 
> > > > +	 */
> > > > +	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;
> > > > +	};
> > > > +
> 
> [snip]
> 
> > > > +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;
> > > > +	};
> > > 
> > > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > > should design a common API here.
> > 
> >  Sure we can take it up sometime later.
> 
> [snip]
> 
> > > > +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 comment as for AF_GET_STAT.
> > 
> > Yes, we can discuss about it to make it common. I would prefer we get this
> > driver in and make amends when you are doing it for OMAP.
> 
> OK, but then please start a discussion on the mailing list about this topic 
> (CC'ing David Cohen as he might be interested).
I will. Let us get the current driver in. In the meantime I will do some analysis and send an RFC.
If possible, I would request for your ACK on this patch and driver. 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
Thanks and Regards,
-Manju

RE: [PATCH] [media] davinci: vpfe: Add documentation

[Hadli, Manjunath]

Laurent,
 Thank you for your comments.

On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> > On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> 
> [snip]
> 
> > > > +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
> > > 
> > > Who is responsible for filling this field, the application or the driver ?
> > 
> > The application is responsible for filling this info. He would enumerate the
> > capabilities first and  set them using S_PARAM/G_PARAM.
> 
> And what's the point of the application setting the version field ? How does 
> the driver use it ?
The version may not be required. Will remove it.
> 
> > > > +	 * @len: Length of the module config structure
> > > > +	 * @module_id: Module id
> > > > +	 * @param: pointer to module config parameter.
> > > 
> > > What is module_id for ? What does param point to ?
> > 
> > There are a lot of tiny modules in the previewer/resizer which are
> > enumerated as individual modules. The param points to the parameter set
> > that the module expects to be set.
> 
> Why don't you implement something similar to 
> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
I feel if we implement direct IOCTLS there might be many of them. To make sure than independent of the number of internal modules present, having the same IOCTL used for all modules is a good idea.

> 
> > > > +	 */
> > > > +	struct prev_module_param {
> > > > +		char version[IMP_MAX_NAME_SIZE];
> > > 
> > > Is there a need to express the version as a string instead of an integer ?
> > 
> > It could be integer. It is generally a fixed point num, and easy to read it
> > as a string than an integer. Can I keep it as a string?
> 
> Let's first decide whether a version field is needed at all :-)
Will remove.
> 
> > > > +		unsigned short len;
> > > > +		unsigned short module_id;
> > > > +		void *param;
> > > > +	};
> > > > +
> > > > +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;
> > > > +	};
> > > 
> > > What's the difference between parameters and configuration ? What does
> > > config point to ?
> > 
> > Config is setting which is required for a subdev to function based on what
> > it is set for (single shot/continuous.) common to all platforms. Parameters
> > are the settings for individual small sub-ips which might be slightly
> > different from one platform to another. Config points to
> > prev_single_shot_config or  prev_continuous_config currently defined in
> > linux/dm3656ipipe.h. I think we will move it to a common location.
> 
> Why don't you implement something similar to 
> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same for 
> the resizer configuration ioctls) ?
>
Ditto. 
> > > > +
> > > > +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;
> > > > +		char control;
> > > > +		enum imp_data_paths path;
> > > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > > +	};
> > > 
> > > Enumerating internal modules is exactly what the MC API was designed for.
> > > Why do you reimplement that using private ioctls ?
> > 
> > The number of these sub-Ips are quite a few in DM365 and Dm355, having a lot
> > of them In a way that may be bewildering to the end-user to be able to
> > connect them quickly and properly. But overall, these are nothing but
> > exposed subips of what we call as CCDC,Previewer  and Resizer.It Made a lot
> > of logical sense to keep it that way, give a default configuration for
> > everything, and if at all the user wants the fine grain config control, be
> > able to give (mainly for the configurations- not so much for connections).
> > In most of the cases the param IOTCLs are only used for fine-tuning the
> > image and not expected to be used as a regular flow of a normal
> > application. I do not think there could be any justification for making all
> > these nitty gritty which keep changing for each IPs as part of regular V4L2
> > IOCTLs. In future, if there is a common theme that emerges, we could
> > definitely relook into this.
> 
> I totally agree with you on this, the tiny sub-blocks should not be exposed as 
> through the MC API. However, I would go one step further : I wouldn't expose 
> them through a private ioctl either. What would a userspace application do 
> with this information that it couldn't do with just the entity name and its 
> revision number ?
Not exposing the full functionality might not be an option. The driver gets used by different kinds of users. Some might want to use only the basic features, but many would like to have the full control in terms of setting all the parameters. Since IPIPE is so much about tuning, not having a fine grain control on its parameters is not an option.

> 
> [snip]
> 
> > > > +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
> > > 
> > > Can't you use subdev V4L2 controls for gains ?
> > 
> > In that case only gain has to be taken out as a generic IOCTL. Since that is
> > is The parameter which could be taken out of this big structure
> 
> That's correct.
> 
> > > > +	 * @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
> > > 
> > > Shouldn't color patterns be computed automatically by the driver based on
> > > the media bus pixel code ?
> > 
> > OK.
> > 
> > > > +	 * @data_size: data size from 8 to 16 bits
> > > > +	 * @data_shift: data shift applied before storing to SDRAM
> > > 
> > > Ditto, this should probably be computed automatically.
> > 
> > Do you want to define new MBUS formats for these?
> 
> The media bus format contains information about the data width, so I think 
> those fields are redundant.
The specific fields here have the control of specifying the datawidth from 9 bits to 16 bits. Did you want us to implement media bus format for all these variations? Just to make sure we do not get held up, I will send a separate patch on mediabus formats for these variations for review. In the mean time, we will go ahead with this.

> 
> > > > +	 * @test_pat_gen: enable input test pattern generation
> > > 
> > > You could use a subdev V4L2 control for that.
> > 
> > Ok.
> > 
> > > > +	 */
> > > > +	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;
> > > > +	};
> > > > +
> 
> [snip]
> 
> > > > +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;
> > > > +	};
> > > 
> > > The OMAP3 ISP driver also needs to export statistics data to userspace. We
> > > should design a common API here.
> > 
> >  Sure we can take it up sometime later.
> 
> [snip]
> 
> > > > +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 comment as for AF_GET_STAT.
> > 
> > Yes, we can discuss about it to make it common. I would prefer we get this
> > driver in and make amends when you are doing it for OMAP.
> 
> OK, but then please start a discussion on the mailing list about this topic 
> (CC'ing David Cohen as he might be interested).
I will. Let us get the current driver in. In the meantime I will do some analysis and send an RFC.
If possible, I would request for your ACK on this patch and driver. 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> > On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> > > On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > > > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > 
> > [snip]
> > 
> > > > > +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
> > > > 
> > > > Who is responsible for filling this field, the application or the
> > > > driver ?
> > > 
> > > The application is responsible for filling this info. He would enumerate
> > > the capabilities first and  set them using S_PARAM/G_PARAM.
> > 
> > And what's the point of the application setting the version field ? How
> > does the driver use it ?
> 
> The version may not be required. Will remove it.
> 
> > > > > +	 * @len: Length of the module config structure
> > > > > +	 * @module_id: Module id
> > > > > +	 * @param: pointer to module config parameter.
> > > > 
> > > > What is module_id for ? What does param point to ?
> > > 
> > > There are a lot of tiny modules in the previewer/resizer which are
> > > enumerated as individual modules. The param points to the parameter set
> > > that the module expects to be set.
> > 
> > Why don't you implement something similar to
> > VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
> 
> I feel if we implement direct IOCTLS there might be many of them. To make
> sure than independent of the number of internal modules present, having the
> same IOCTL used for all modules is a good idea.

You can set several parameters using a single ioctl, much like 
VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter. 

PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially 
reinventing V4L2 controls, and I don't think that's a good idea.

> > > > > +	 */
> > > > > +	struct prev_module_param {
> > > > > +		char version[IMP_MAX_NAME_SIZE];
> > > > 
> > > > Is there a need to express the version as a string instead of an
> > > > integer ?
> > > 
> > > It could be integer. It is generally a fixed point num, and easy to read
> > > it as a string than an integer. Can I keep it as a string?
> > 
> > Let's first decide whether a version field is needed at all :-)
> 
> Will remove.
> 
> > > > > +		unsigned short len;
> > > > > +		unsigned short module_id;
> > > > > +		void *param;
> > > > > +	};
> > > > > +
> > > > > +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;
> > > > > +	};
> > > > 
> > > > What's the difference between parameters and configuration ? What does
> > > > config point to ?
> > > 
> > > Config is setting which is required for a subdev to function based on
> > > what it is set for (single shot/continuous.) common to all platforms.
> > > Parameters are the settings for individual small sub-ips which might be
> > > slightly different from one platform to another. Config points to
> > > prev_single_shot_config or  prev_continuous_config currently defined in
> > > linux/dm3656ipipe.h. I think we will move it to a common location.
> > 
> > Why don't you implement something similar to
> > VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same
> > for the resizer configuration ioctls) ?
> 
> Ditto.
> 
> > > > > +
> > > > > +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;
> > > > > +		char control;
> > > > > +		enum imp_data_paths path;
> > > > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > > > +	};
> > > > 
> > > > Enumerating internal modules is exactly what the MC API was designed
> > > > for.
> > > > Why do you reimplement that using private ioctls ?
> > > 
> > > The number of these sub-Ips are quite a few in DM365 and Dm355, having a
> > > lot of them In a way that may be bewildering to the end-user to be able
> > > to connect them quickly and properly. But overall, these are nothing
> > > but exposed subips of what we call as CCDC,Previewer  and Resizer.It
> > > Made a lot of logical sense to keep it that way, give a default
> > > configuration for everything, and if at all the user wants the fine
> > > grain config control, be able to give (mainly for the configurations-
> > > not so much for connections). In most of the cases the param IOTCLs are
> > > only used for fine-tuning the image and not expected to be used as a
> > > regular flow of a normal application. I do not think there could be any
> > > justification for making all these nitty gritty which keep changing for
> > > each IPs as part of regular V4L2 IOCTLs. In future, if there is a common
> > > theme that emerges, we could definitely relook into this.
> > 
> > I totally agree with you on this, the tiny sub-blocks should not be
> > exposed as through the MC API. However, I would go one step further : I
> > wouldn't expose them through a private ioctl either. What would a
> > userspace application do with this information that it couldn't do with
> > just the entity name and its revision number ?
> 
> Not exposing the full functionality might not be an option. The driver gets
> used by different kinds of users. Some might want to use only the basic
> features, but many would like to have the full control in terms of setting
> all the parameters. Since IPIPE is so much about tuning, not having a fine
> grain control on its parameters is not an option.

My point wasn't that you shouldn't expose all device features, but that you 
don't need userspace to be able to dynamically enumerate the content of the 
entity. Applications need to use your private ioctls so they know what 
hardware they deal with. Knowing the entity name (and possibly revision) 
should be enough.

> > [snip]
> > 
> > > > > +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
> > > > 
> > > > Can't you use subdev V4L2 controls for gains ?
> > > 
> > > In that case only gain has to be taken out as a generic IOCTL. Since
> > > that is is The parameter which could be taken out of this big structure
> > 
> > That's correct.
> > 
> > > > > +	 * @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
> > > > 
> > > > Shouldn't color patterns be computed automatically by the driver based
> > > > on
> > > > the media bus pixel code ?
> > > 
> > > OK.
> > > 
> > > > > +	 * @data_size: data size from 8 to 16 bits
> > > > > +	 * @data_shift: data shift applied before storing to SDRAM
> > > > 
> > > > Ditto, this should probably be computed automatically.
> > > 
> > > Do you want to define new MBUS formats for these?
> > 
> > The media bus format contains information about the data width, so I think
> > those fields are redundant.
> 
> The specific fields here have the control of specifying the datawidth from 9
> bits to 16 bits. Did you want us to implement media bus format for all
> these variations?

If you have hardware that can generate data in a given width, it needs a media 
bus format, yes. Just don't add media bus formats for widths that are not 
implemented in any hardware.

> Just to make sure we do not get held up, I will send a separate patch on
> mediabus formats for these variations for review. In the mean time, we will
> go ahead with this.
>
> > > > > +	 * @test_pat_gen: enable input test pattern generation
> > > > 
> > > > You could use a subdev V4L2 control for that.
> > > 
> > > Ok.
> > > 
> > > > > +	 */
> > > > > +	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;
> > > > > +	};
> > > > > +
> > 
> > [snip]
> > 
> > > > > +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;
> > > > > +	};
> > > > 
> > > > The OMAP3 ISP driver also needs to export statistics data to
> > > > userspace. We should design a common API here.
> > >  
> > >  Sure we can take it up sometime later.
> > 
> > [snip]
> > 
> > > > > +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 comment as for AF_GET_STAT.
> > > 
> > > Yes, we can discuss about it to make it common. I would prefer we get
> > > this driver in and make amends when you are doing it for OMAP.
> > 
> > OK, but then please start a discussion on the mailing list about this
> > topic (CC'ing David Cohen as he might be interested).
> 
> I will. Let us get the current driver in. In the meantime I will do some
> analysis and send an RFC.

OK.

> If possible, I would request for your ACK on this patch and driver.

I can't ack this before we solve the PREV_ENUM_CAP, PREV_[GS]_PARAM and 
PREV_[GS]_CONFIG issue.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> > On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> > > On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> > > > On Wednesday 11 July 2012 21:09:26 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@ti.com>
> > > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > 
> > [snip]
> > 
> > > > > +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
> > > > 
> > > > Who is responsible for filling this field, the application or the
> > > > driver ?
> > > 
> > > The application is responsible for filling this info. He would enumerate
> > > the capabilities first and  set them using S_PARAM/G_PARAM.
> > 
> > And what's the point of the application setting the version field ? How
> > does the driver use it ?
> 
> The version may not be required. Will remove it.
> 
> > > > > +	 * @len: Length of the module config structure
> > > > > +	 * @module_id: Module id
> > > > > +	 * @param: pointer to module config parameter.
> > > > 
> > > > What is module_id for ? What does param point to ?
> > > 
> > > There are a lot of tiny modules in the previewer/resizer which are
> > > enumerated as individual modules. The param points to the parameter set
> > > that the module expects to be set.
> > 
> > Why don't you implement something similar to
> > VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
> 
> I feel if we implement direct IOCTLS there might be many of them. To make
> sure than independent of the number of internal modules present, having the
> same IOCTL used for all modules is a good idea.

You can set several parameters using a single ioctl, much like 
VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter. 

PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially 
reinventing V4L2 controls, and I don't think that's a good idea.

> > > > > +	 */
> > > > > +	struct prev_module_param {
> > > > > +		char version[IMP_MAX_NAME_SIZE];
> > > > 
> > > > Is there a need to express the version as a string instead of an
> > > > integer ?
> > > 
> > > It could be integer. It is generally a fixed point num, and easy to read
> > > it as a string than an integer. Can I keep it as a string?
> > 
> > Let's first decide whether a version field is needed at all :-)
> 
> Will remove.
> 
> > > > > +		unsigned short len;
> > > > > +		unsigned short module_id;
> > > > > +		void *param;
> > > > > +	};
> > > > > +
> > > > > +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;
> > > > > +	};
> > > > 
> > > > What's the difference between parameters and configuration ? What does
> > > > config point to ?
> > > 
> > > Config is setting which is required for a subdev to function based on
> > > what it is set for (single shot/continuous.) common to all platforms.
> > > Parameters are the settings for individual small sub-ips which might be
> > > slightly different from one platform to another. Config points to
> > > prev_single_shot_config or  prev_continuous_config currently defined in
> > > linux/dm3656ipipe.h. I think we will move it to a common location.
> > 
> > Why don't you implement something similar to
> > VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same
> > for the resizer configuration ioctls) ?
> 
> Ditto.
> 
> > > > > +
> > > > > +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;
> > > > > +		char control;
> > > > > +		enum imp_data_paths path;
> > > > > +		char module_name[IMP_MAX_NAME_SIZE];
> > > > > +	};
> > > > 
> > > > Enumerating internal modules is exactly what the MC API was designed
> > > > for.
> > > > Why do you reimplement that using private ioctls ?
> > > 
> > > The number of these sub-Ips are quite a few in DM365 and Dm355, having a
> > > lot of them In a way that may be bewildering to the end-user to be able
> > > to connect them quickly and properly. But overall, these are nothing
> > > but exposed subips of what we call as CCDC,Previewer  and Resizer.It
> > > Made a lot of logical sense to keep it that way, give a default
> > > configuration for everything, and if at all the user wants the fine
> > > grain config control, be able to give (mainly for the configurations-
> > > not so much for connections). In most of the cases the param IOTCLs are
> > > only used for fine-tuning the image and not expected to be used as a
> > > regular flow of a normal application. I do not think there could be any
> > > justification for making all these nitty gritty which keep changing for
> > > each IPs as part of regular V4L2 IOCTLs. In future, if there is a common
> > > theme that emerges, we could definitely relook into this.
> > 
> > I totally agree with you on this, the tiny sub-blocks should not be
> > exposed as through the MC API. However, I would go one step further : I
> > wouldn't expose them through a private ioctl either. What would a
> > userspace application do with this information that it couldn't do with
> > just the entity name and its revision number ?
> 
> Not exposing the full functionality might not be an option. The driver gets
> used by different kinds of users. Some might want to use only the basic
> features, but many would like to have the full control in terms of setting
> all the parameters. Since IPIPE is so much about tuning, not having a fine
> grain control on its parameters is not an option.

My point wasn't that you shouldn't expose all device features, but that you 
don't need userspace to be able to dynamically enumerate the content of the 
entity. Applications need to use your private ioctls so they know what 
hardware they deal with. Knowing the entity name (and possibly revision) 
should be enough.

> > [snip]
> > 
> > > > > +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
> > > > 
> > > > Can't you use subdev V4L2 controls for gains ?
> > > 
> > > In that case only gain has to be taken out as a generic IOCTL. Since
> > > that is is The parameter which could be taken out of this big structure
> > 
> > That's correct.
> > 
> > > > > +	 * @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
> > > > 
> > > > Shouldn't color patterns be computed automatically by the driver based
> > > > on
> > > > the media bus pixel code ?
> > > 
> > > OK.
> > > 
> > > > > +	 * @data_size: data size from 8 to 16 bits
> > > > > +	 * @data_shift: data shift applied before storing to SDRAM
> > > > 
> > > > Ditto, this should probably be computed automatically.
> > > 
> > > Do you want to define new MBUS formats for these?
> > 
> > The media bus format contains information about the data width, so I think
> > those fields are redundant.
> 
> The specific fields here have the control of specifying the datawidth from 9
> bits to 16 bits. Did you want us to implement media bus format for all
> these variations?

If you have hardware that can generate data in a given width, it needs a media 
bus format, yes. Just don't add media bus formats for widths that are not 
implemented in any hardware.

> Just to make sure we do not get held up, I will send a separate patch on
> mediabus formats for these variations for review. In the mean time, we will
> go ahead with this.
>
> > > > > +	 * @test_pat_gen: enable input test pattern generation
> > > > 
> > > > You could use a subdev V4L2 control for that.
> > > 
> > > Ok.
> > > 
> > > > > +	 */
> > > > > +	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;
> > > > > +	};
> > > > > +
> > 
> > [snip]
> > 
> > > > > +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;
> > > > > +	};
> > > > 
> > > > The OMAP3 ISP driver also needs to export statistics data to
> > > > userspace. We should design a common API here.
> > >  
> > >  Sure we can take it up sometime later.
> > 
> > [snip]
> > 
> > > > > +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 comment as for AF_GET_STAT.
> > > 
> > > Yes, we can discuss about it to make it common. I would prefer we get
> > > this driver in and make amends when you are doing it for OMAP.
> > 
> > OK, but then please start a discussion on the mailing list about this
> > topic (CC'ing David Cohen as he might be interested).
> 
> I will. Let us get the current driver in. In the meantime I will do some
> analysis and send an RFC.

OK.

> If possible, I would request for your ACK on this patch and driver.

I can't ack this before we solve the PREV_ENUM_CAP, PREV_[GS]_PARAM and 
PREV_[GS]_CONFIG issue.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Manju]

Hi Laurent,

On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote:
> Hi Manjunath,
>
> On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
>> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
>>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
>>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
>>>>> On Wednesday 11 July 2012 21:09:26 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@ti.com>
>>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>>> [snip]
>>>
>>>>>> +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
>>>>> Who is responsible for filling this field, the application or the
>>>>> driver ?
>>>> The application is responsible for filling this info. He would enumerate
>>>> the capabilities first and  set them using S_PARAM/G_PARAM.
>>> And what's the point of the application setting the version field ? How
>>> does the driver use it ?
>> The version may not be required. Will remove it.
>>
>>>>>> +	 * @len: Length of the module config structure
>>>>>> +	 * @module_id: Module id
>>>>>> +	 * @param: pointer to module config parameter.
>>>>> What is module_id for ? What does param point to ?
>>>> There are a lot of tiny modules in the previewer/resizer which are
>>>> enumerated as individual modules. The param points to the parameter set
>>>> that the module expects to be set.
>>> Why don't you implement something similar to
>>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
>> I feel if we implement direct IOCTLS there might be many of them. To make
>> sure than independent of the number of internal modules present, having the
>> same IOCTL used for all modules is a good idea.
> You can set several parameters using a single ioctl, much like
> VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter.
>
> PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially
> reinventing V4L2 controls, and I don't think that's a good idea.
Ok. I looked into this, and found that the structure needed to pass
all the parameters is going to be huge. just to avoid a big structure
from the user space, I propose:

Having a union of structures and a parameter identifying the structure.

In that way, we will remove the enumeration and all the other
things except for a SET and GET, much like the CCDC_RAW_PARAMS
like you suggested. So essentially we will have only 2 IOCTLS for setting
  the private params/configs and remove the rest.  I hope that was your
point and this proposal will solve it?

>
>>>>>> +	 */
>>>>>> +	struct prev_module_param {
>>>>>> +		char version[IMP_MAX_NAME_SIZE];
>>>>> Is there a need to express the version as a string instead of an
>>>>> integer ?
>>>> It could be integer. It is generally a fixed point num, and easy to read
>>>> it as a string than an integer. Can I keep it as a string?
>>> Let's first decide whether a version field is needed at all :-)
>> Will remove.
>>
>>>>>> +		unsigned short len;
>>>>>> +		unsigned short module_id;
>>>>>> +		void *param;
>>>>>> +	};
>>>>>> +
>>>>>> +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;
>>>>>> +	};
>>>>> What's the difference between parameters and configuration ? What does
>>>>> config point to ?
>>>> Config is setting which is required for a subdev to function based on
>>>> what it is set for (single shot/continuous.) common to all platforms.
>>>> Parameters are the settings for individual small sub-ips which might be
>>>> slightly different from one platform to another. Config points to
>>>> prev_single_shot_config or  prev_continuous_config currently defined in
>>>> linux/dm3656ipipe.h. I think we will move it to a common location.
>>> Why don't you implement something similar to
>>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same
>>> for the resizer configuration ioctls) ?
>> Ditto.
>>
>>>>>> +
>>>>>> +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;
>>>>>> +		char control;
>>>>>> +		enum imp_data_paths path;
>>>>>> +		char module_name[IMP_MAX_NAME_SIZE];
>>>>>> +	};
>>>>> Enumerating internal modules is exactly what the MC API was designed
>>>>> for.
>>>>> Why do you reimplement that using private ioctls ?
>>>> The number of these sub-Ips are quite a few in DM365 and Dm355, having a
>>>> lot of them In a way that may be bewildering to the end-user to be able
>>>> to connect them quickly and properly. But overall, these are nothing
>>>> but exposed subips of what we call as CCDC,Previewer  and Resizer.It
>>>> Made a lot of logical sense to keep it that way, give a default
>>>> configuration for everything, and if at all the user wants the fine
>>>> grain config control, be able to give (mainly for the configurations-
>>>> not so much for connections). In most of the cases the param IOTCLs are
>>>> only used for fine-tuning the image and not expected to be used as a
>>>> regular flow of a normal application. I do not think there could be any
>>>> justification for making all these nitty gritty which keep changing for
>>>> each IPs as part of regular V4L2 IOCTLs. In future, if there is a common
>>>> theme that emerges, we could definitely relook into this.
>>> I totally agree with you on this, the tiny sub-blocks should not be
>>> exposed as through the MC API. However, I would go one step further : I
>>> wouldn't expose them through a private ioctl either. What would a
>>> userspace application do with this information that it couldn't do with
>>> just the entity name and its revision number ?
>> Not exposing the full functionality might not be an option. The driver gets
>> used by different kinds of users. Some might want to use only the basic
>> features, but many would like to have the full control in terms of setting
>> all the parameters. Since IPIPE is so much about tuning, not having a fine
>> grain control on its parameters is not an option.
> My point wasn't that you shouldn't expose all device features, but that you
> don't need userspace to be able to dynamically enumerate the content of the
> entity. Applications need to use your private ioctls so they know what
> hardware they deal with. Knowing the entity name (and possibly revision)
> should be enough.
ditto.
>
>>> [snip]
>>>
>>>>>> +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
>>>>> Can't you use subdev V4L2 controls for gains ?
>>>> In that case only gain has to be taken out as a generic IOCTL. Since
>>>> that is is The parameter which could be taken out of this big structure
>>> That's correct.
>>>
>>>>>> +	 * @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
>>>>> Shouldn't color patterns be computed automatically by the driver based
>>>>> on
>>>>> the media bus pixel code ?
>>>> OK.
>>>>
>>>>>> +	 * @data_size: data size from 8 to 16 bits
>>>>>> +	 * @data_shift: data shift applied before storing to SDRAM
>>>>> Ditto, this should probably be computed automatically.
>>>> Do you want to define new MBUS formats for these?
>>> The media bus format contains information about the data width, so I think
>>> those fields are redundant.
>> The specific fields here have the control of specifying the datawidth from 9
>> bits to 16 bits. Did you want us to implement media bus format for all
>> these variations?
> If you have hardware that can generate data in a given width, it needs a media
> bus format, yes. Just don't add media bus formats for widths that are not
> implemented in any hardware.
Sure. Like we agreed,  will send a separate patch for this.
>
>> Just to make sure we do not get held up, I will send a separate patch on
>> mediabus formats for these variations for review. In the mean time, we will
>> go ahead with this.
>>
>>>>>> +	 * @test_pat_gen: enable input test pattern generation
>>>>> You could use a subdev V4L2 control for that.
>>>> Ok.
>>>>
>>>>>> +	 */
>>>>>> +	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;
>>>>>> +	};
>>>>>> +
>>> [snip]
>>>
>>>>>> +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;
>>>>>> +	};
>>>>> The OMAP3 ISP driver also needs to export statistics data to
>>>>> userspace. We should design a common API here.
>>>>   
>>>>   Sure we can take it up sometime later.
>>> [snip]
>>>
>>>>>> +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 comment as for AF_GET_STAT.
>>>> Yes, we can discuss about it to make it common. I would prefer we get
>>>> this driver in and make amends when you are doing it for OMAP.
>>> OK, but then please start a discussion on the mailing list about this
>>> topic (CC'ing David Cohen as he might be interested).
>> I will. Let us get the current driver in. In the meantime I will do some
>> analysis and send an RFC.
> OK.
>
>> If possible, I would request for your ACK on this patch and driver.
> I can't ack this before we solve the PREV_ENUM_CAP, PREV_[GS]_PARAM and
> PREV_[GS]_CONFIG issue.
>
Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Manju]

Hi Laurent,

On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote:
> Hi Manjunath,
>
> On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
>> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
>>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
>>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
>>>>> On Wednesday 11 July 2012 21:09:26 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@ti.com>
>>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>>> [snip]
>>>
>>>>>> +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
>>>>> Who is responsible for filling this field, the application or the
>>>>> driver ?
>>>> The application is responsible for filling this info. He would enumerate
>>>> the capabilities first and  set them using S_PARAM/G_PARAM.
>>> And what's the point of the application setting the version field ? How
>>> does the driver use it ?
>> The version may not be required. Will remove it.
>>
>>>>>> +	 * @len: Length of the module config structure
>>>>>> +	 * @module_id: Module id
>>>>>> +	 * @param: pointer to module config parameter.
>>>>> What is module_id for ? What does param point to ?
>>>> There are a lot of tiny modules in the previewer/resizer which are
>>>> enumerated as individual modules. The param points to the parameter set
>>>> that the module expects to be set.
>>> Why don't you implement something similar to
>>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
>> I feel if we implement direct IOCTLS there might be many of them. To make
>> sure than independent of the number of internal modules present, having the
>> same IOCTL used for all modules is a good idea.
> You can set several parameters using a single ioctl, much like
> VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter.
>
> PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially
> reinventing V4L2 controls, and I don't think that's a good idea.
Ok. I looked into this, and found that the structure needed to pass
all the parameters is going to be huge. just to avoid a big structure
from the user space, I propose:

Having a union of structures and a parameter identifying the structure.

In that way, we will remove the enumeration and all the other
things except for a SET and GET, much like the CCDC_RAW_PARAMS
like you suggested. So essentially we will have only 2 IOCTLS for setting
  the private params/configs and remove the rest.  I hope that was your
point and this proposal will solve it?

>
>>>>>> +	 */
>>>>>> +	struct prev_module_param {
>>>>>> +		char version[IMP_MAX_NAME_SIZE];
>>>>> Is there a need to express the version as a string instead of an
>>>>> integer ?
>>>> It could be integer. It is generally a fixed point num, and easy to read
>>>> it as a string than an integer. Can I keep it as a string?
>>> Let's first decide whether a version field is needed at all :-)
>> Will remove.
>>
>>>>>> +		unsigned short len;
>>>>>> +		unsigned short module_id;
>>>>>> +		void *param;
>>>>>> +	};
>>>>>> +
>>>>>> +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;
>>>>>> +	};
>>>>> What's the difference between parameters and configuration ? What does
>>>>> config point to ?
>>>> Config is setting which is required for a subdev to function based on
>>>> what it is set for (single shot/continuous.) common to all platforms.
>>>> Parameters are the settings for individual small sub-ips which might be
>>>> slightly different from one platform to another. Config points to
>>>> prev_single_shot_config or  prev_continuous_config currently defined in
>>>> linux/dm3656ipipe.h. I think we will move it to a common location.
>>> Why don't you implement something similar to
>>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS here as well (same
>>> for the resizer configuration ioctls) ?
>> Ditto.
>>
>>>>>> +
>>>>>> +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;
>>>>>> +		char control;
>>>>>> +		enum imp_data_paths path;
>>>>>> +		char module_name[IMP_MAX_NAME_SIZE];
>>>>>> +	};
>>>>> Enumerating internal modules is exactly what the MC API was designed
>>>>> for.
>>>>> Why do you reimplement that using private ioctls ?
>>>> The number of these sub-Ips are quite a few in DM365 and Dm355, having a
>>>> lot of them In a way that may be bewildering to the end-user to be able
>>>> to connect them quickly and properly. But overall, these are nothing
>>>> but exposed subips of what we call as CCDC,Previewer  and Resizer.It
>>>> Made a lot of logical sense to keep it that way, give a default
>>>> configuration for everything, and if at all the user wants the fine
>>>> grain config control, be able to give (mainly for the configurations-
>>>> not so much for connections). In most of the cases the param IOTCLs are
>>>> only used for fine-tuning the image and not expected to be used as a
>>>> regular flow of a normal application. I do not think there could be any
>>>> justification for making all these nitty gritty which keep changing for
>>>> each IPs as part of regular V4L2 IOCTLs. In future, if there is a common
>>>> theme that emerges, we could definitely relook into this.
>>> I totally agree with you on this, the tiny sub-blocks should not be
>>> exposed as through the MC API. However, I would go one step further : I
>>> wouldn't expose them through a private ioctl either. What would a
>>> userspace application do with this information that it couldn't do with
>>> just the entity name and its revision number ?
>> Not exposing the full functionality might not be an option. The driver gets
>> used by different kinds of users. Some might want to use only the basic
>> features, but many would like to have the full control in terms of setting
>> all the parameters. Since IPIPE is so much about tuning, not having a fine
>> grain control on its parameters is not an option.
> My point wasn't that you shouldn't expose all device features, but that you
> don't need userspace to be able to dynamically enumerate the content of the
> entity. Applications need to use your private ioctls so they know what
> hardware they deal with. Knowing the entity name (and possibly revision)
> should be enough.
ditto.
>
>>> [snip]
>>>
>>>>>> +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
>>>>> Can't you use subdev V4L2 controls for gains ?
>>>> In that case only gain has to be taken out as a generic IOCTL. Since
>>>> that is is The parameter which could be taken out of this big structure
>>> That's correct.
>>>
>>>>>> +	 * @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
>>>>> Shouldn't color patterns be computed automatically by the driver based
>>>>> on
>>>>> the media bus pixel code ?
>>>> OK.
>>>>
>>>>>> +	 * @data_size: data size from 8 to 16 bits
>>>>>> +	 * @data_shift: data shift applied before storing to SDRAM
>>>>> Ditto, this should probably be computed automatically.
>>>> Do you want to define new MBUS formats for these?
>>> The media bus format contains information about the data width, so I think
>>> those fields are redundant.
>> The specific fields here have the control of specifying the datawidth from 9
>> bits to 16 bits. Did you want us to implement media bus format for all
>> these variations?
> If you have hardware that can generate data in a given width, it needs a media
> bus format, yes. Just don't add media bus formats for widths that are not
> implemented in any hardware.
Sure. Like we agreed,  will send a separate patch for this.
>
>> Just to make sure we do not get held up, I will send a separate patch on
>> mediabus formats for these variations for review. In the mean time, we will
>> go ahead with this.
>>
>>>>>> +	 * @test_pat_gen: enable input test pattern generation
>>>>> You could use a subdev V4L2 control for that.
>>>> Ok.
>>>>
>>>>>> +	 */
>>>>>> +	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;
>>>>>> +	};
>>>>>> +
>>> [snip]
>>>
>>>>>> +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;
>>>>>> +	};
>>>>> The OMAP3 ISP driver also needs to export statistics data to
>>>>> userspace. We should design a common API here.
>>>>   
>>>>   Sure we can take it up sometime later.
>>> [snip]
>>>
>>>>>> +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 comment as for AF_GET_STAT.
>>>> Yes, we can discuss about it to make it common. I would prefer we get
>>>> this driver in and make amends when you are doing it for OMAP.
>>> OK, but then please start a discussion on the mailing list about this
>>> topic (CC'ing David Cohen as he might be interested).
>> I will. Let us get the current driver in. In the meantime I will do some
>> analysis and send an RFC.
> OK.
>
>> If possible, I would request for your ACK on this patch and driver.
> I can't ack this before we solve the PREV_ENUM_CAP, PREV_[GS]_PARAM and
> PREV_[GS]_CONFIG issue.
>
Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 31 July 2012 13:15:27 Manju wrote:
> On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote:
> > On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
> >> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> >>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> >>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> >>>>> On Wednesday 11 July 2012 21:09:26 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@ti.com>
> >>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >>> 
> >>> [snip]
> >>> 
> >>>>>> +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
> >>>>> 
> >>>>> Who is responsible for filling this field, the application or the
> >>>>> driver ?
> >>>> 
> >>>> The application is responsible for filling this info. He would
> >>>> enumerate the capabilities first and  set them using S_PARAM/G_PARAM.
> >>> 
> >>> And what's the point of the application setting the version field ? How
> >>> does the driver use it ?
> >> 
> >> The version may not be required. Will remove it.
> >> 
> >>>>>> +	 * @len: Length of the module config structure
> >>>>>> +	 * @module_id: Module id
> >>>>>> +	 * @param: pointer to module config parameter.
> >>>>> 
> >>>>> What is module_id for ? What does param point to ?
> >>>> 
> >>>> There are a lot of tiny modules in the previewer/resizer which are
> >>>> enumerated as individual modules. The param points to the parameter set
> >>>> that the module expects to be set.
> >>> 
> >>> Why don't you implement something similar to
> >>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
> >> 
> >> I feel if we implement direct IOCTLS there might be many of them. To make
> >> sure than independent of the number of internal modules present, having
> >> the
> >> same IOCTL used for all modules is a good idea.
> > 
> > You can set several parameters using a single ioctl, much like
> > VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter.
> > 
> > PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially
> > reinventing V4L2 controls, and I don't think that's a good idea.
> 
> Ok. I looked into this, and found that the structure needed to pass
> all the parameters is going to be huge. just to avoid a big structure
> from the user space, I propose:
> 
> Having a union of structures and a parameter identifying the structure.
> 
> In that way, we will remove the enumeration and all the other
> things except for a SET and GET, much like the CCDC_RAW_PARAMS
> like you suggested. So essentially we will have only 2 IOCTLS for setting
> the private params/configs and remove the rest.  I hope that was your
> point and this proposal will solve it?

What about something like the following structure, from the OMAP3 ISP driver ?

struct omap3isp_prev_update_config {
        __u32 update;
        __u32 flag;
        __u32 shading_shift;
        struct omap3isp_prev_luma __user *luma;
        struct omap3isp_prev_hmed __user *hmed;
        struct omap3isp_prev_cfa __user *cfa;
        struct omap3isp_prev_csup __user *csup;
        struct omap3isp_prev_wbal __user *wbal;
        struct omap3isp_prev_blkadj __user *blkadj;
        struct omap3isp_prev_rgbtorgb __user *rgb2rgb;
        struct omap3isp_prev_csc __user *csc;
        struct omap3isp_prev_yclimit __user *yclimit;
        struct omap3isp_prev_dcor __user *dcor;
        struct omap3isp_prev_nf __user *nf;
        struct omap3isp_prev_gtables __user *gamma;
};

I'll probably have more comments when I'll see the complete list of parameters 
you need to expose.

> >>>>>> +	 */
> >>>>>> +	struct prev_module_param {
> >>>>>> +		char version[IMP_MAX_NAME_SIZE];
> >>>>> 
> >>>>> Is there a need to express the version as a string instead of an
> >>>>> integer ?
> >>>> 
> >>>> It could be integer. It is generally a fixed point num, and easy to
> >>>> read
> >>>> it as a string than an integer. Can I keep it as a string?
> >>> 
> >>> Let's first decide whether a version field is needed at all :-)
> >> 
> >> Will remove.
> >> 
> >>>>>> +		unsigned short len;
> >>>>>> +		unsigned short module_id;
> >>>>>> +		void *param;
> >>>>>> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Manjunath,

On Tuesday 31 July 2012 13:15:27 Manju wrote:
> On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote:
> > On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
> >> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> >>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> >>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> >>>>> On Wednesday 11 July 2012 21:09:26 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@ti.com>
> >>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >>> 
> >>> [snip]
> >>> 
> >>>>>> +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
> >>>>> 
> >>>>> Who is responsible for filling this field, the application or the
> >>>>> driver ?
> >>>> 
> >>>> The application is responsible for filling this info. He would
> >>>> enumerate the capabilities first and  set them using S_PARAM/G_PARAM.
> >>> 
> >>> And what's the point of the application setting the version field ? How
> >>> does the driver use it ?
> >> 
> >> The version may not be required. Will remove it.
> >> 
> >>>>>> +	 * @len: Length of the module config structure
> >>>>>> +	 * @module_id: Module id
> >>>>>> +	 * @param: pointer to module config parameter.
> >>>>> 
> >>>>> What is module_id for ? What does param point to ?
> >>>> 
> >>>> There are a lot of tiny modules in the previewer/resizer which are
> >>>> enumerated as individual modules. The param points to the parameter set
> >>>> that the module expects to be set.
> >>> 
> >>> Why don't you implement something similar to
> >>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
> >> 
> >> I feel if we implement direct IOCTLS there might be many of them. To make
> >> sure than independent of the number of internal modules present, having
> >> the
> >> same IOCTL used for all modules is a good idea.
> > 
> > You can set several parameters using a single ioctl, much like
> > VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter.
> > 
> > PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially
> > reinventing V4L2 controls, and I don't think that's a good idea.
> 
> Ok. I looked into this, and found that the structure needed to pass
> all the parameters is going to be huge. just to avoid a big structure
> from the user space, I propose:
> 
> Having a union of structures and a parameter identifying the structure.
> 
> In that way, we will remove the enumeration and all the other
> things except for a SET and GET, much like the CCDC_RAW_PARAMS
> like you suggested. So essentially we will have only 2 IOCTLS for setting
> the private params/configs and remove the rest.  I hope that was your
> point and this proposal will solve it?

What about something like the following structure, from the OMAP3 ISP driver ?

struct omap3isp_prev_update_config {
        __u32 update;
        __u32 flag;
        __u32 shading_shift;
        struct omap3isp_prev_luma __user *luma;
        struct omap3isp_prev_hmed __user *hmed;
        struct omap3isp_prev_cfa __user *cfa;
        struct omap3isp_prev_csup __user *csup;
        struct omap3isp_prev_wbal __user *wbal;
        struct omap3isp_prev_blkadj __user *blkadj;
        struct omap3isp_prev_rgbtorgb __user *rgb2rgb;
        struct omap3isp_prev_csc __user *csc;
        struct omap3isp_prev_yclimit __user *yclimit;
        struct omap3isp_prev_dcor __user *dcor;
        struct omap3isp_prev_nf __user *nf;
        struct omap3isp_prev_gtables __user *gamma;
};

I'll probably have more comments when I'll see the complete list of parameters 
you need to expose.

> >>>>>> +	 */
> >>>>>> +	struct prev_module_param {
> >>>>>> +		char version[IMP_MAX_NAME_SIZE];
> >>>>> 
> >>>>> Is there a need to express the version as a string instead of an
> >>>>> integer ?
> >>>> 
> >>>> It could be integer. It is generally a fixed point num, and easy to
> >>>> read
> >>>> it as a string than an integer. Can I keep it as a string?
> >>> 
> >>> Let's first decide whether a version field is needed at all :-)
> >> 
> >> Will remove.
> >> 
> >>>>>> +		unsigned short len;
> >>>>>> +		unsigned short module_id;
> >>>>>> +		void *param;
> >>>>>> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

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@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> ---
>   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@ti.com>
> +
> +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:
> +<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
>

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Manjunath Hadli]

Hi Sakari,
 
 Thank you for the comments.

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@ti.com>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> ---
>>   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@ti.com>
>> +
>> +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?
>
>> +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.
>
>> +        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.
>
> 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

>
>> +    };
>> +
>> +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.
>
> 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;
>> +        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?
>
> 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.
>
>> +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
>>
>
> Kind regards,
>
Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

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@ti.com>
> >> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >> ---
> >>   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@ti.com>
> >> +
> >> +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.

<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@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Manjunath Hadli]

Hi Sakari,

On Thursday 16 August 2012 09:53 PM, Sakari Ailus 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@ti.com>
>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>>>> ---
>>>>   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@ti.com>
>>>> +
>>>> +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?
> 
What I meant was using a union of structures and not strct pointers.
That way there would be no need to have a copy_from_user in the
driver.Hovewer ,I am open to either methods.So We will go ahead with the
OMAP way for now.
>>>
>>>> +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.
> 

We will implement this is a similar way as above. Will probably merge
with the above to make it a single blob 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.
> 
Oh! in that sense it is more of a single resizer which a lot of common
hardware blocks and has a facility that we can enable the second output.
Most of registers are common as well. I think it is much better to keep
it the way it is. The scaling factor is not taken as an explicit
parameter but managed using the w and h of sources and sinks.
>>> 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.
> 
> <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.
Right now our priority is to have at least the base driver into the
mainline and add features quickly after that as we have done sufficient
numbers of changes and do not want to get stuck in a loop between
framework advancements vs our changes. With that in mind, we will drop
the support for statistics, and add it later with 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.
OK.
> 
>>>
>>>> +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,
> 
With the above changes, I will float another version of the document,
and hopefully we will have your ACK on that. :)


Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

Hi Manju,

My apologies for the delayed answer.

On Wed, Aug 22, 2012 at 02:26:50PM +0530, Manjunath Hadli wrote:
> On Thursday 16 August 2012 09:53 PM, Sakari Ailus wrote:
> > On Thu, Aug 09, 2012 at 09:13:52AM +0530, Manjunath Hadli wrote:
> >> 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@ti.com>
> >>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >>>> ---
> >>>>   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@ti.com>
> >>>> +
> >>>> +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?
> > 
> What I meant was using a union of structures and not strct pointers.
> That way there would be no need to have a copy_from_user in the
> driver.Hovewer ,I am open to either methods.So We will go ahead with the
> OMAP way for now.

Agreed. An obvious issue with the approach of having an union of structures
(not pointers) is that you end up copying a lot more memory (the maximum
size of all of the configurations) every time between user and kernel
spaces.

> >>>
> >>>> +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.
> > 
> 
> We will implement this is a similar way as above. Will probably merge
> with the above to make it a single blob IOCTL.

I'm not comfortable with the idea of a blob IOCTL. As the kernel driver will
even parse the data, is there a single valid reason not to specify the
memory layout of the data as well, and thus enforce the proper use of the
interface?

> >>>> +        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.
> > 
> Oh! in that sense it is more of a single resizer which a lot of common
> hardware blocks and has a facility that we can enable the second output.
> Most of registers are common as well. I think it is much better to keep
> it the way it is. The scaling factor is not taken as an explicit
> parameter but managed using the w and h of sources and sinks.

That would be against the V4L2 spec. It's explicitly defined that the source
compose rectangle defines the output size of the scaler.

<URL:http://hverkuil.home.xs4all.nl/spec/media.html#pad-level-formats>

The reason for the source width and height not being used for the purpose is
that it'd be impossible to specify cropping after scaling in the same
subdev.

I'll reply to the rest later on.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

On Sat, Sep 01, 2012 at 08:25:30PM +0300, Sakari Ailus wrote:
> That would be against the V4L2 spec. It's explicitly defined that the source
> compose rectangle defines the output size of the scaler.

This should be sink, not source.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Manjunath Hadli]

Hi Sakari,

On Saturday 01 September 2012 10:55 PM, Sakari Ailus wrote:
> Hi Manju,
> 
> My apologies for the delayed answer.
> 
> On Wed, Aug 22, 2012 at 02:26:50PM +0530, Manjunath Hadli wrote:
>> On Thursday 16 August 2012 09:53 PM, Sakari Ailus wrote:
>>> On Thu, Aug 09, 2012 at 09:13:52AM +0530, Manjunath Hadli wrote:
>>>> 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@ti.com>
>>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>>>>>> ---
>>>>>>   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@ti.com>
>>>>>> +
>>>>>> +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?
>>>
>> What I meant was using a union of structures and not strct pointers.
>> That way there would be no need to have a copy_from_user in the
>> driver.Hovewer ,I am open to either methods.So We will go ahead with the
>> OMAP way for now.
> 
> Agreed. An obvious issue with the approach of having an union of structures
> (not pointers) is that you end up copying a lot more memory (the maximum
> size of all of the configurations) every time between user and kernel
> spaces.
> 
OK.
>>>>>
>>>>>> +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.
>>>
>>
>> We will implement this is a similar way as above. Will probably merge
>> with the above to make it a single blob IOCTL.
> 
> I'm not comfortable with the idea of a blob IOCTL. As the kernel driver will
> even parse the data, is there a single valid reason not to specify the
> memory layout of the data as well, and thus enforce the proper use of the
> interface?
I guess you meant blob = void *. No I will not have a void pointer but
an actual struct pointer from the user space. I think that is what you
wanted.

> 
>>>>>> +        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.
>>>
>> Oh! in that sense it is more of a single resizer which a lot of common
>> hardware blocks and has a facility that we can enable the second output.
>> Most of registers are common as well. I think it is much better to keep
>> it the way it is. The scaling factor is not taken as an explicit
>> parameter but managed using the w and h of sources and sinks.
> 
> That would be against the V4L2 spec. It's explicitly defined that the source
> compose rectangle defines the output size of the scaler.
> 
> <URL:http://hverkuil.home.xs4all.nl/spec/media.html#pad-level-formats>
> 
> The reason for the source width and height not being used for the purpose is
> that it'd be impossible to specify cropping after scaling in the same
> subdev.
There is no cropping here. I am using the pad's mbus_framefmt  structure
to determine the scaling. It is according to the v4L2 scheme of things,
and could not see any discrepancy.  How else did you like it to be
implemented? As I stated before, if you see problems in Resizer 2, i am
willing to drop the feature. Let me know your view.
> 
> I'll reply to the rest later on.
> 
> Kind regards,
> 

Thanks and Regards,
-Manju

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Prabhakar Lad]

Hi Sakari,

On Thu, Aug 16, 2012 at 9:53 PM, Sakari Ailus <sakari.ailus@iki.fi> 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@ti.com>
>> >> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >> ---
>> >>   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@ti.com>
>> >> +
>> >> +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@iki.fi     jabber/XMPP/Gmail: sailus@retiisi.org.uk
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

Hi Prabhakar,

On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:
...
> >> >> +        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 ?

There are two approaches I can think of.

One is a menu control which can be used to choose the test pattern (or
disable it). The control could be standardised but the menu items would have
to be hardware-specific since the test patterns themselves are not
standardised.

The alternative is to have a boolean control to enable (and disable) the
test pattern and then a menu control to choose which one to use. Using or
implemeting the control to select the test pattern isn't even strictly
necessary to get a test pattern out of the device: one can enable it without
knowing which one it is.

So which one would be better? Similar cases include V4L2_CID_SCENE_MODE
which is used to choose the scene mode from a list of alternatives. The main
difference to this case is that the menu items of the scene mode control are
standardised, too.

I'd be inclined to have a single menu control, even if the other menu items
will be device-specific. The first value (0) still has to be documented to
mean the test pattern is disabled.

Laurent, Hans: what do you think?

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

Hi Sakari,

On Saturday 01 September 2012 12:57:07 Sakari Ailus wrote:
> On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:

[snip]

> > For test pattern you meant control to enable/disable it ?
> 
> There are two approaches I can think of.
> 
> One is a menu control which can be used to choose the test pattern (or
> disable it). The control could be standardised but the menu items would have
> to be hardware-specific since the test patterns themselves are not
> standardised.

Agreed. The test patterns themselves are highly hardware-specific.

>From personal experience with sensors, most devices implement a small, fixed 
set of test patterns that can be exposed through a menu control. However, some 
devices also implement more "configurable" test patterns. For instance the 
MT9V032 can generate horizontal, vertical or diagonal test patterns, or a 
uniform grey test pattern with a user-configurable value. This would then 
require two controls.

> The alternative is to have a boolean control to enable (and disable) the
> test pattern and then a menu control to choose which one to use. Using or
> implemeting the control to select the test pattern isn't even strictly
> necessary to get a test pattern out of the device: one can enable it without
> knowing which one it is.
> 
> So which one would be better? Similar cases include V4L2_CID_SCENE_MODE
> which is used to choose the scene mode from a list of alternatives. The main
> difference to this case is that the menu items of the scene mode control
> are standardised, too.
> 
> I'd be inclined to have a single menu control, even if the other menu items
> will be device-specific. The first value (0) still has to be documented to
> mean the test pattern is disabled.
> 
> Laurent, Hans: what do you think?

A menu control with value 0 meaning test pattern disabled has my preference as 
well.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Prabhakar Lad]

Hi Laurent,

On Sat, Sep 1, 2012 at 7:52 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Sakari,
>
> On Saturday 01 September 2012 12:57:07 Sakari Ailus wrote:
>> On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:
>
> [snip]
>
>> > For test pattern you meant control to enable/disable it ?
>>
>> There are two approaches I can think of.
>>
>> One is a menu control which can be used to choose the test pattern (or
>> disable it). The control could be standardised but the menu items would have
>> to be hardware-specific since the test patterns themselves are not
>> standardised.
>
> Agreed. The test patterns themselves are highly hardware-specific.
>
> From personal experience with sensors, most devices implement a small, fixed
> set of test patterns that can be exposed through a menu control. However, some
> devices also implement more "configurable" test patterns. For instance the
> MT9V032 can generate horizontal, vertical or diagonal test patterns, or a
> uniform grey test pattern with a user-configurable value. This would then
> require two controls.
>
two controls I didn't get it ? When we have menu itself with a list of standard
patterns why would two controls be required ?

Thx,
--Prabhakar Lad

>> The alternative is to have a boolean control to enable (and disable) the
>> test pattern and then a menu control to choose which one to use. Using or
>> implemeting the control to select the test pattern isn't even strictly
>> necessary to get a test pattern out of the device: one can enable it without
>> knowing which one it is.
>>
>> So which one would be better? Similar cases include V4L2_CID_SCENE_MODE
>> which is used to choose the scene mode from a list of alternatives. The main
>> difference to this case is that the menu items of the scene mode control
>> are standardised, too.
>>
>> I'd be inclined to have a single menu control, even if the other menu items
>> will be device-specific. The first value (0) still has to be documented to
>> mean the test pattern is disabled.
>>
>> Laurent, Hans: what do you think?
>
> A menu control with value 0 meaning test pattern disabled has my preference as
> well.
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Sakari Ailus]

Hi Prabhakar,

On Sat, Sep 01, 2012 at 08:23:58PM +0530, Prabhakar Lad wrote:
> On Sat, Sep 1, 2012 at 7:52 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Sakari,
> >
> > On Saturday 01 September 2012 12:57:07 Sakari Ailus wrote:
> >> On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:
> >
> > [snip]
> >
> >> > For test pattern you meant control to enable/disable it ?
> >>
> >> There are two approaches I can think of.
> >>
> >> One is a menu control which can be used to choose the test pattern (or
> >> disable it). The control could be standardised but the menu items would have
> >> to be hardware-specific since the test patterns themselves are not
> >> standardised.
> >
> > Agreed. The test patterns themselves are highly hardware-specific.
> >
> > From personal experience with sensors, most devices implement a small, fixed
> > set of test patterns that can be exposed through a menu control. However, some
> > devices also implement more "configurable" test patterns. For instance the
> > MT9V032 can generate horizontal, vertical or diagonal test patterns, or a
> > uniform grey test pattern with a user-configurable value. This would then
> > require two controls.
> >
> two controls I didn't get it ? When we have menu itself with a list of standard
> patterns why would two controls be required ?

Two are not required. A single menu control will do.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Laurent Pinchart]

On Saturday 01 September 2012 19:11:56 Sakari Ailus wrote:
> Hi Prabhakar,
> 
> On Sat, Sep 01, 2012 at 08:23:58PM +0530, Prabhakar Lad wrote:
> > On Sat, Sep 1, 2012 at 7:52 PM, Laurent Pinchart
> > 
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > Hi Sakari,
> > > 
> > > On Saturday 01 September 2012 12:57:07 Sakari Ailus wrote:
> > >> On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:
> > > [snip]
> > > 
> > >> > For test pattern you meant control to enable/disable it ?
> > >> 
> > >> There are two approaches I can think of.
> > >> 
> > >> One is a menu control which can be used to choose the test pattern (or
> > >> disable it). The control could be standardised but the menu items would
> > >> have to be hardware-specific since the test patterns themselves are
> > >> not standardised.
> > > 
> > > Agreed. The test patterns themselves are highly hardware-specific.
> > > 
> > > From personal experience with sensors, most devices implement a small,
> > > fixed set of test patterns that can be exposed through a menu control.
> > > However, some devices also implement more "configurable" test patterns.
> > > For instance the MT9V032 can generate horizontal, vertical or diagonal
> > > test patterns, or a uniform grey test pattern with a user-configurable
> > > value. This would then require two controls.
> > 
> > two controls I didn't get it ? When we have menu itself with a list of
> > standard patterns why would two controls be required ?
> 
> Two are not required. A single menu control will do.

That's correct, in this case a single menu control will do. We would only need 
multiple controls if the device exposes test pattern parameters, as in the 
MT9V032 sensor example.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Hans Verkuil]

On Sat September 1 2012 16:22:30 Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 01 September 2012 12:57:07 Sakari Ailus wrote:
> > On Wed, Aug 29, 2012 at 08:11:50PM +0530, Prabhakar Lad wrote:
> 
> [snip]
> 
> > > For test pattern you meant control to enable/disable it ?
> > 
> > There are two approaches I can think of.
> > 
> > One is a menu control which can be used to choose the test pattern (or
> > disable it). The control could be standardised but the menu items would have
> > to be hardware-specific since the test patterns themselves are not
> > standardised.
> 
> Agreed. The test patterns themselves are highly hardware-specific.
> 
> From personal experience with sensors, most devices implement a small, fixed 
> set of test patterns that can be exposed through a menu control. However, some 
> devices also implement more "configurable" test patterns. For instance the 
> MT9V032 can generate horizontal, vertical or diagonal test patterns, or a 
> uniform grey test pattern with a user-configurable value. This would then 
> require two controls.
> 
> > The alternative is to have a boolean control to enable (and disable) the
> > test pattern and then a menu control to choose which one to use. Using or
> > implemeting the control to select the test pattern isn't even strictly
> > necessary to get a test pattern out of the device: one can enable it without
> > knowing which one it is.
> > 
> > So which one would be better? Similar cases include V4L2_CID_SCENE_MODE
> > which is used to choose the scene mode from a list of alternatives. The main
> > difference to this case is that the menu items of the scene mode control
> > are standardised, too.
> > 
> > I'd be inclined to have a single menu control, even if the other menu items
> > will be device-specific. The first value (0) still has to be documented to
> > mean the test pattern is disabled.
> > 
> > Laurent, Hans: what do you think?
> 
> A menu control with value 0 meaning test pattern disabled has my preference as 
> well.

+1

	Hans

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Rob Landley]

On 07/11/2012 10:39 AM, 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@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>

I saw the comment on the 8th, is there another version of this
documentation coming...?

Rob
-- 
GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code.
Either it's "mere aggregation", or a license violation.  Pick one.

Re: [PATCH] [media] davinci: vpfe: Add documentation

[Prabhakar Lad]

Hi Rob,

On Thursday 16 August 2012 06:40 PM, Rob Landley wrote:
> On 07/11/2012 10:39 AM, 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@ti.com>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> 
> I saw the comment on the 8th, is there another version of this
> documentation coming...?
> 
 I was waiting for comments from Sakari/Laurent, If they are happy
 from what Manju has proposed, depending on the outcome of the
discussion I'll soon post another version soon.

Thanks and Regards,
--Prabhakar

> Rob
>