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