Messages in this thread | | | Date | Tue, 31 Jul 2012 13:15:27 +0530 | From | Manju <> | Subject | Re: [PATCH] [media] davinci: vpfe: Add documentation |
| |
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
| |