Re: [PATCH] [media] davinci: vpfe: Add documentation
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
>> Signed-off-by: Lad, Prabhakar
>> ---
>> Documentation/video4linux/davinci-vpfe-mc.txt | 263
>> +
>> 1 files changed, 263 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
>>
>> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
>> b/Documentation/video4linux/davinci-vpfe-mc.txt
>> new file mode 100644
>> index 000..968194f
>> --- /dev/null
>> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
>> @@ -0,0 +1,263 @@
>> +Davinci Video processing Front End (VPFE) driver
>> +
>> +Copyright (C) 2012 Texas Instruments Inc
>> +
>> +Contacts: Manjunath Hadli
>> +
>> +Introduction
>> +
>> +
>> +This file documents the Texas Instruments Davinci Video processing
>> Front End
>> +(VPFE) driver located under drivers/media/video/davinci. The
>> original driver
>> +exists for Davinci VPFE, which is now being changed to Media Controller
>> +Framework.
>> +
>> +Currently the driver has been successfully used on the following
>> version of Davinci:
>> +
>> +DM365/DM368
>> +
>> +The driver implements V4L2, Media controller and v4l2_subdev
>> interfaces.
>> +Sensor, lens and flash drivers using the v4l2_subdev interface in
>> the kernel
>> +are supported.
>> +
>> +
>> +Split to subdevs
>> +
>> +
>> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks
>> inside the VPFE
>> +having one subdev to represent it. Each of the subdevs provide a
>> V4L2 subdev
>> +interface to userspace.
>> +
>> +DAVINCI CCDC
>> +DAVINCI PREVIEWER
>> +DAVINCI RESIZER
>> +DAVINCI AEW
>> +DAVINCI AF
>> +
>> +Each possible link in the VPFE is modeled by a link in the Media
>> controller
>> +interface. For an example program see [1].
>> +
>> +
>> +Private IOCTLs
>> +==
>> +
>> +The Davinci Video processing Front End (VPFE) driver supports
>> standard V4L2
>> +IOCTLs and controls where possible and practical. Much of the
>> functions provided
>> +by the VPFE, however, does not fall under the standard IOCTLs.
>> +
>> +In general, there is a private ioctl for configuring each of the blocks
>> +containing hardware-dependent functions.
>> +
>> +The following private IOCTLs are supported:
>> +
>> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
>> +Description:
>> +Sets/Gets the parameters required by the previewer module
>> +Parameter:
>> +/**
>> + * struct prev_module_param- structure to configure preview modules
>> + * @version: Version of the preview module
>> + * @len: Length of the module config structure
>> + * @module_id: Module id
>> + * @param: pointer to module config parameter.
>> + */
>> +struct prev_module_param {
>> +char version[IMP_MAX_NAME_SIZE];
>> +unsigned short len;
>> +unsigned short module_id;
>> +void *param;
>> +};
>
> In addition to what Laurent commented on this, could the version
> information be passed in struct media_entity_desc instead?
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 voi
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> Signed-off-by: Lad, Prabhakar
> ---
> Documentation/video4linux/davinci-vpfe-mc.txt | 263
> +
> 1 files changed, 263 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
>
> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
> b/Documentation/video4linux/davinci-vpfe-mc.txt
> new file mode 100644
> index 000..968194f
> --- /dev/null
> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
> @@ -0,0 +1,263 @@
> +Davinci Video processing Front End (VPFE) driver
> +
> +Copyright (C) 2012 Texas Instruments Inc
> +
> +Contacts: Manjunath Hadli
> +
> +Introduction
> +
> +
> +This file documents the Texas Instruments Davinci Video processing
> Front End
> +(VPFE) driver located under drivers/media/video/davinci. The
> original driver
> +exists for Davinci VPFE, which is now being changed to Media Controller
> +Framework.
> +
> +Currently the driver has been successfully used on the following
> version of Davinci:
> +
> +DM365/DM368
> +
> +The driver implements V4L2, Media controller and v4l2_subdev
> interfaces.
> +Sensor, lens and flash drivers using the v4l2_subdev interface in
> the kernel
> +are supported.
> +
> +
> +Split to subdevs
> +
> +
> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks
> inside the VPFE
> +having one subdev to represent it. Each of the subdevs provide a
> V4L2 subdev
> +interface to userspace.
> +
> +DAVINCI CCDC
> +DAVINCI PREVIEWER
> +DAVINCI RESIZER
> +DAVINCI AEW
> +DAVINCI AF
> +
> +Each possible link in the VPFE is modeled by a link in the Media
> controller
> +interface. For an example program see [1].
> +
> +
> +Private IOCTLs
> +==
> +
> +The Davinci Video processing Front End (VPFE) driver supports
> standard V4L2
> +IOCTLs and controls where possible and practical. Much of the
> functions provided
> +by the VPFE, however, does not fall under the standard IOCTLs.
> +
> +In general, there is a private ioctl for configuring each of the blocks
> +containing hardware-dependent functions.
> +
> +The following private IOCTLs are supported:
> +
> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
> +Description:
> +Sets/Gets the parameters required by the previewer module
> +Parameter:
> +/**
> + * struct prev_module_param- structure to configure preview modules
> + * @version: Version of the preview module
> + * @len: Length of the module config structure
> + * @module_id: Module id
> + * @param: pointer to module config parameter.
> + */
> +struct prev_module_param {
> +char version[IMP_MAX_NAME_SIZE];
> +unsigned short len;
> +unsigned short module_id;
> +void *param;
> +};
> >>>
> >>> In addition to what Laurent commented on this, could the version
> >>> information be passed in struct media_entity_desc instead?
> >> 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
> >> i
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 > 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.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
Hi Laurent, On Sat, Sep 1, 2012 at 7:52 PM, 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. > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
Hi Sakari,
On Thu, Aug 16, 2012 at 9: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
>> >> Signed-off-by: Lad, Prabhakar
>> >> ---
>> >> Documentation/video4linux/davinci-vpfe-mc.txt | 263
>> >> +
>> >> 1 files changed, 263 insertions(+), 0 deletions(-)
>> >> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
>> >>
>> >> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
>> >> b/Documentation/video4linux/davinci-vpfe-mc.txt
>> >> new file mode 100644
>> >> index 000..968194f
>> >> --- /dev/null
>> >> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
>> >> @@ -0,0 +1,263 @@
>> >> +Davinci Video processing Front End (VPFE) driver
>> >> +
>> >> +Copyright (C) 2012 Texas Instruments Inc
>> >> +
>> >> +Contacts: Manjunath Hadli
>> >> +
>> >> +Introduction
>> >> +
>> >> +
>> >> +This file documents the Texas Instruments Davinci Video processing
>> >> Front End
>> >> +(VPFE) driver located under drivers/media/video/davinci. The
>> >> original driver
>> >> +exists for Davinci VPFE, which is now being changed to Media Controller
>> >> +Framework.
>> >> +
>> >> +Currently the driver has been successfully used on the following
>> >> version of Davinci:
>> >> +
>> >> +DM365/DM368
>> >> +
>> >> +The driver implements V4L2, Media controller and v4l2_subdev
>> >> interfaces.
>> >> +Sensor, lens and flash drivers using the v4l2_subdev interface in
>> >> the kernel
>> >> +are supported.
>> >> +
>> >> +
>> >> +Split to subdevs
>> >> +
>> >> +
>> >> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks
>> >> inside the VPFE
>> >> +having one subdev to represent it. Each of the subdevs provide a
>> >> V4L2 subdev
>> >> +interface to userspace.
>> >> +
>> >> +DAVINCI CCDC
>> >> +DAVINCI PREVIEWER
>> >> +DAVINCI RESIZER
>> >> +DAVINCI AEW
>> >> +DAVINCI AF
>> >> +
>> >> +Each possible link in the VPFE is modeled by a link in the Media
>> >> controller
>> >> +interface. For an example program see [1].
>> >> +
>> >> +
>> >> +Private IOCTLs
>> >> +==
>> >> +
>> >> +The Davinci Video processing Front End (VPFE) driver supports
>> >> standard V4L2
>> >> +IOCTLs and controls where possible and practical. Much of the
>> >> functions provided
>> >> +by the VPFE, however, does not fall under the standard IOCTLs.
>> >> +
>> >> +In general, there is a private ioctl for configuring each of the blocks
>> >> +containing hardware-dependent functions.
>> >> +
>> >> +The following private IOCTLs are supported:
>> >> +
>> >> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
>> >> +Description:
>> >> +Sets/Gets the parameters required by the previewer module
>> >> +Parameter:
>> >> +/**
>> >> + * struct prev_module_param- structure to configure preview modules
>> >> + * @version: Version of the preview module
>> >> + * @len: Length of the module config structure
>> >> + * @module_id: Module id
>> >> + * @param: pointer to module config parameter.
>> >> + */
>> >> +struct prev_module_param {
>> >> +char version[IMP_MAX_NAME_SIZE];
>> >> +unsigned short len;
>> >> +unsigned short module_id;
>> >> +void *param;
>> >> +};
>> >
>> > In addition to what Laurent commented on this, could the version
>> > information be passed in struct media_entity_desc instead?
>> 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
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
Signed-off-by: Lad, Prabhakar
---
Documentation/video4linux/davinci-vpfe-mc.txt | 263
+
1 files changed, 263 insertions(+), 0 deletions(-)
create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
b/Documentation/video4linux/davinci-vpfe-mc.txt
new file mode 100644
index 000..968194f
--- /dev/null
+++ b/Documentation/video4linux/davinci-vpfe-mc.txt
@@ -0,0 +1,263 @@
+Davinci Video processing Front End (VPFE) driver
+
+Copyright (C) 2012 Texas Instruments Inc
+
+Contacts: Manjunath Hadli
+
+Introduction
+
+
+This file documents the Texas Instruments Davinci Video processing
Front End
+(VPFE) driver located under drivers/media/video/davinci. The
original driver
+exists for Davinci VPFE, which is now being changed to Media Controller
+Framework.
+
+Currently the driver has been successfully used on the following
version of Davinci:
+
+DM365/DM368
+
+The driver implements V4L2, Media controller and v4l2_subdev
interfaces.
+Sensor, lens and flash drivers using the v4l2_subdev interface in
the kernel
+are supported.
+
+
+Split to subdevs
+
+
+The Davinic VPFE is split into V4L2 subdevs, each of the blocks
inside the VPFE
+having one subdev to represent it. Each of the subdevs provide a
V4L2 subdev
+interface to userspace.
+
+DAVINCI CCDC
+DAVINCI PREVIEWER
+DAVINCI RESIZER
+DAVINCI AEW
+DAVINCI AF
+
+Each possible link in the VPFE is modeled by a link in the Media
controller
+interface. For an example program see [1].
+
+
+Private IOCTLs
+==
+
+The Davinci Video processing Front End (VPFE) driver supports
standard V4L2
+IOCTLs and controls where possible and practical. Much of the
functions provided
+by the VPFE, however, does not fall under the standard IOCTLs.
+
+In general, there is a private ioctl for configuring each of the blocks
+containing hardware-dependent functions.
+
+The following private IOCTLs are supported:
+
+1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
+Description:
+Sets/Gets the parameters required by the previewer module
+Parameter:
+/**
+ * struct prev_module_param- structure to configure preview modules
+ * @version: Version of the preview module
+ * @len: Length of the module config structure
+ * @module_id: Module id
+ * @param: pointer to module config parameter.
+ */
+struct prev_module_param {
+char version[IMP_MAX_NAME_SIZE];
+unsigned short len;
+unsigned short module_id;
+void *param;
+};
>>>
>>> In addition to what Laurent commented on this, could the version
>>> information be passed in struct media_entity_desc instead?
>> 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 ato
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> >> Signed-off-by: Lad, Prabhakar
> >> ---
> >> Documentation/video4linux/davinci-vpfe-mc.txt | 263
> >> +
> >> 1 files changed, 263 insertions(+), 0 deletions(-)
> >> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
> >>
> >> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
> >> b/Documentation/video4linux/davinci-vpfe-mc.txt
> >> new file mode 100644
> >> index 000..968194f
> >> --- /dev/null
> >> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
> >> @@ -0,0 +1,263 @@
> >> +Davinci Video processing Front End (VPFE) driver
> >> +
> >> +Copyright (C) 2012 Texas Instruments Inc
> >> +
> >> +Contacts: Manjunath Hadli
> >> +
> >> +Introduction
> >> +
> >> +
> >> +This file documents the Texas Instruments Davinci Video processing
> >> Front End
> >> +(VPFE) driver located under drivers/media/video/davinci. The
> >> original driver
> >> +exists for Davinci VPFE, which is now being changed to Media Controller
> >> +Framework.
> >> +
> >> +Currently the driver has been successfully used on the following
> >> version of Davinci:
> >> +
> >> +DM365/DM368
> >> +
> >> +The driver implements V4L2, Media controller and v4l2_subdev
> >> interfaces.
> >> +Sensor, lens and flash drivers using the v4l2_subdev interface in
> >> the kernel
> >> +are supported.
> >> +
> >> +
> >> +Split to subdevs
> >> +
> >> +
> >> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks
> >> inside the VPFE
> >> +having one subdev to represent it. Each of the subdevs provide a
> >> V4L2 subdev
> >> +interface to userspace.
> >> +
> >> +DAVINCI CCDC
> >> +DAVINCI PREVIEWER
> >> +DAVINCI RESIZER
> >> +DAVINCI AEW
> >> +DAVINCI AF
> >> +
> >> +Each possible link in the VPFE is modeled by a link in the Media
> >> controller
> >> +interface. For an example program see [1].
> >> +
> >> +
> >> +Private IOCTLs
> >> +==
> >> +
> >> +The Davinci Video processing Front End (VPFE) driver supports
> >> standard V4L2
> >> +IOCTLs and controls where possible and practical. Much of the
> >> functions provided
> >> +by the VPFE, however, does not fall under the standard IOCTLs.
> >> +
> >> +In general, there is a private ioctl for configuring each of the blocks
> >> +containing hardware-dependent functions.
> >> +
> >> +The following private IOCTLs are supported:
> >> +
> >> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
> >> +Description:
> >> +Sets/Gets the parameters required by the previewer module
> >> +Parameter:
> >> +/**
> >> + * struct prev_module_param- structure to configure preview modules
> >> + * @version: Version of the preview module
> >> + * @len: Length of the module config structure
> >> + * @module_id: Module id
> >> + * @param: pointer to module config parameter.
> >> + */
> >> +struct prev_module_param {
> >> +char version[IMP_MAX_NAME_SIZE];
> >> +unsigned short len;
> >> +unsigned short module_id;
> >> +void *param;
> >> +};
> >
> > In addition to what Laurent commented on this, could the version
> > information be passed in struct media_entity_desc instead?
> 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
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 >> Signed-off-by: Lad, Prabhakar > > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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 > Signed-off-by: Lad, Prabhakar 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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
>> Signed-off-by: Lad, Prabhakar
>> ---
>> Documentation/video4linux/davinci-vpfe-mc.txt | 263
>> +
>> 1 files changed, 263 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
>>
>> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
>> b/Documentation/video4linux/davinci-vpfe-mc.txt
>> new file mode 100644
>> index 000..968194f
>> --- /dev/null
>> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
>> @@ -0,0 +1,263 @@
>> +Davinci Video processing Front End (VPFE) driver
>> +
>> +Copyright (C) 2012 Texas Instruments Inc
>> +
>> +Contacts: Manjunath Hadli
>> +
>> +Introduction
>> +
>> +
>> +This file documents the Texas Instruments Davinci Video processing
>> Front End
>> +(VPFE) driver located under drivers/media/video/davinci. The
>> original driver
>> +exists for Davinci VPFE, which is now being changed to Media Controller
>> +Framework.
>> +
>> +Currently the driver has been successfully used on the following
>> version of Davinci:
>> +
>> +DM365/DM368
>> +
>> +The driver implements V4L2, Media controller and v4l2_subdev
>> interfaces.
>> +Sensor, lens and flash drivers using the v4l2_subdev interface in
>> the kernel
>> +are supported.
>> +
>> +
>> +Split to subdevs
>> +
>> +
>> +The Davinic VPFE is split into V4L2 subdevs, each of the blocks
>> inside the VPFE
>> +having one subdev to represent it. Each of the subdevs provide a
>> V4L2 subdev
>> +interface to userspace.
>> +
>> +DAVINCI CCDC
>> +DAVINCI PREVIEWER
>> +DAVINCI RESIZER
>> +DAVINCI AEW
>> +DAVINCI AF
>> +
>> +Each possible link in the VPFE is modeled by a link in the Media
>> controller
>> +interface. For an example program see [1].
>> +
>> +
>> +Private IOCTLs
>> +==
>> +
>> +The Davinci Video processing Front End (VPFE) driver supports
>> standard V4L2
>> +IOCTLs and controls where possible and practical. Much of the
>> functions provided
>> +by the VPFE, however, does not fall under the standard IOCTLs.
>> +
>> +In general, there is a private ioctl for configuring each of the blocks
>> +containing hardware-dependent functions.
>> +
>> +The following private IOCTLs are supported:
>> +
>> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
>> +Description:
>> +Sets/Gets the parameters required by the previewer module
>> +Parameter:
>> +/**
>> + * struct prev_module_param- structure to configure preview modules
>> + * @version: Version of the preview module
>> + * @len: Length of the module config structure
>> + * @module_id: Module id
>> + * @param: pointer to module config parameter.
>> + */
>> +struct prev_module_param {
>> +char version[IMP_MAX_NAME_SIZE];
>> +unsigned short len;
>> +unsigned short module_id;
>> +void *param;
>> +};
>
> In addition to what Laurent commented on this, could the version
> information be passed in struct media_entity_desc instead?
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
>> +inpu
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> >> Signed-off-by: Lad, Prabhakar
> >>>
> >>> [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 n
Re: [PATCH] [media] davinci: vpfe: Add documentation
Hi Manju,
Thanks for the patch.
Please make sure these patches reach linux-media next time. If they do not,
it severely limits the number of potential reviewers. I don't know why, but
the original patch isn't on linux-media even if the list was cc'd.
Dropping linux-kernel from cc.
Manjunath Hadli wrote:
Add documentation on the Davinci VPFE driver. Document the subdevs,
and private IOTCLs the driver implements
Signed-off-by: Manjunath Hadli
Signed-off-by: Lad, Prabhakar
---
Documentation/video4linux/davinci-vpfe-mc.txt | 263 +
1 files changed, 263 insertions(+), 0 deletions(-)
create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
b/Documentation/video4linux/davinci-vpfe-mc.txt
new file mode 100644
index 000..968194f
--- /dev/null
+++ b/Documentation/video4linux/davinci-vpfe-mc.txt
@@ -0,0 +1,263 @@
+Davinci Video processing Front End (VPFE) driver
+
+Copyright (C) 2012 Texas Instruments Inc
+
+Contacts: Manjunath Hadli
+
+Introduction
+
+
+This file documents the Texas Instruments Davinci Video processing Front End
+(VPFE) driver located under drivers/media/video/davinci. The original driver
+exists for Davinci VPFE, which is now being changed to Media Controller
+Framework.
+
+Currently the driver has been successfully used on the following version of
Davinci:
+
+ DM365/DM368
+
+The driver implements V4L2, Media controller and v4l2_subdev interfaces.
+Sensor, lens and flash drivers using the v4l2_subdev interface in the kernel
+are supported.
+
+
+Split to subdevs
+
+
+The Davinic VPFE is split into V4L2 subdevs, each of the blocks inside the VPFE
+having one subdev to represent it. Each of the subdevs provide a V4L2 subdev
+interface to userspace.
+
+ DAVINCI CCDC
+ DAVINCI PREVIEWER
+ DAVINCI RESIZER
+ DAVINCI AEW
+ DAVINCI AF
+
+Each possible link in the VPFE is modeled by a link in the Media controller
+interface. For an example program see [1].
+
+
+Private IOCTLs
+==
+
+The Davinci Video processing Front End (VPFE) driver supports standard V4L2
+IOCTLs and controls where possible and practical. Much of the functions
provided
+by the VPFE, however, does not fall under the standard IOCTLs.
+
+In general, there is a private ioctl for configuring each of the blocks
+containing hardware-dependent functions.
+
+The following private IOCTLs are supported:
+
+1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
+Description:
+ Sets/Gets the parameters required by the previewer module
+Parameter:
+ /**
+* struct prev_module_param- structure to configure preview modules
+* @version: Version of the preview module
+* @len: Length of the module config structure
+* @module_id: Module id
+* @param: pointer to module config parameter.
+*/
+ struct prev_module_param {
+ char version[IMP_MAX_NAME_SIZE];
+ unsigned short len;
+ unsigned short module_id;
+ void *param;
+ };
In addition to what Laurent commented on this, could the version
information be passed in struct media_entity_desc instead?
As a general comment, it's a bad idea to design an API that allows passing
blobs, especially when the expected size of the blobs isn't known. That
really equals to asking for trouble.
That said, I know this is an area where complete documentation is acarce,
but I think that at least the memory layout of the current blob pointers
should be visible in the struct definitions whenever possible. See e.g. the
OMAP 3 ISP driver.
+2: IOCTL: PREV_S_CONFIG/PREV_G_CONFIG
+Description:
+ Sets/Gets the configuration required by the previewer channel
+Parameter:
+ /**
+* struct prev_channel_config - structure for configuring the previewer
channel
+* @len: Length of the user configuration
+* @config: pointer to either single shot config or continuous
+*/
+ struct prev_channel_config {
+ unsigned short len;
+ void *config;
+ };
+
+3: IOCTL: PREV_ENUM_CAP
+Description:
+ Queries the modules available in the image processor for preview the
+ input image.
+Parameter:
+ /**
+* struct prev_cap - structure to enumerate capabilities of previewer
+* @index: application use this to iterate over the available modules
+* @version: version of the preview module
+* @module_id: module id
+* @control: control operation allowed in continuous mode? 1 - allowed,
0 - not allowed
+* @path: path on which the module is sitting
+* @module_name: module name
+*/
+ struct prev_cap {
+ unsigned short index;
+ char version[IMP_MAX_NAME_SIZE];
+ unsigned short module_id;
Huh? How many sub-modules do the preview modules have in different DM series
chips
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
Signed-off-by: Lad, Prabhakar
[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 enumerat
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> > > > > Signed-off-by: Lad, Prabhakar
> >
> > [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
> > > > > +De
RE: [PATCH] [media] davinci: vpfe: Add documentation
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
> > > > Signed-off-by: Lad, Prabhakar
>
> [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
> > > > +* @cont
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> > > Signed-off-by: Lad, Prabhakar
[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 ov
Re: [PATCH] [media] davinci: vpfe: Add documentation
Hi Manjunath, Please ignore the previous reply, I've hit the sent button too soon by mistake. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> > > Signed-off-by: Lad, Prabhakar
[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 f
RE: [PATCH] [media] davinci: vpfe: Add documentation
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
> > Signed-off-by: Lad, Prabhakar
> > ---
> > Documentation/video4linux/davinci-vpfe-mc.txt | 263
> > + 1 files changed, 263 insertions(+), 0
> > deletions(-)
> > create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
> >
> > diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
> > b/Documentation/video4linux/davinci-vpfe-mc.txt new file mode 100644
> > index 000..968194f
> > --- /dev/null
> > +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
> > @@ -0,0 +1,263 @@
> > +Davinci Video processing Front End (VPFE) driver
> > +
> > +Copyright (C) 2012 Texas Instruments Inc
> > +
> > +Contacts: Manjunath Hadli
> > +
> > +Introduction
> > +
> > +
> > +This file documents the Texas Instruments Davinci Video processing Front
> > End
> > +(VPFE) driver located under drivers/media/video/davinci. The original
> > driver
> > +exists for Davinci VPFE, which is now being changed to Media Controller
> > +Framework.
> > +
> > +Currently the driver has been successfully used on the following version of
> > Davinci:
> > +
> > + DM365/DM368
>
> 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 {
> > + unsigne
Re: [PATCH] [media] davinci: vpfe: Add documentation
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
> Signed-off-by: Lad, Prabhakar
> ---
> Documentation/video4linux/davinci-vpfe-mc.txt | 263
> + 1 files changed, 263 insertions(+), 0
> deletions(-)
> create mode 100644 Documentation/video4linux/davinci-vpfe-mc.txt
>
> diff --git a/Documentation/video4linux/davinci-vpfe-mc.txt
> b/Documentation/video4linux/davinci-vpfe-mc.txt new file mode 100644
> index 000..968194f
> --- /dev/null
> +++ b/Documentation/video4linux/davinci-vpfe-mc.txt
> @@ -0,0 +1,263 @@
> +Davinci Video processing Front End (VPFE) driver
> +
> +Copyright (C) 2012 Texas Instruments Inc
> +
> +Contacts: Manjunath Hadli
> +
> +Introduction
> +
> +
> +This file documents the Texas Instruments Davinci Video processing Front
> End
> +(VPFE) driver located under drivers/media/video/davinci. The original
> driver
> +exists for Davinci VPFE, which is now being changed to Media Controller
> +Framework.
> +
> +Currently the driver has been successfully used on the following version of
> Davinci:
> +
> + DM365/DM368
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;
> +
