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@xxxxxx>
> > > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx>
> > 
> > [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

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