Hi Manjunath,
On Tuesday 31 July 2012 13:15:27 Manju wrote:
> On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote:
> > On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote:
> >> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote:
> >>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote:
> >>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote:
> >>>>> On Wednesday 11 July 2012 21:09:26 Manjunath Hadli wrote:
> >>>>>> Add documentation on the Davinci VPFE driver. Document the subdevs,
> >>>>>> and private IOTCLs the driver implements
> >>>>>>
> >>>>>> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx>
> >>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx>
> >>>
> >>> [snip]
> >>>
> >>>>>> +Private IOCTLs
> >>>>>> +==============
> >>>>>> +
> >>>>>> +The Davinci Video processing Front End (VPFE) driver supports
> >>>>>> standard V4L2
> >>>>>> +IOCTLs and controls where possible and practical. Much of the
> >>>>>> functions provided
> >>>>>> +by the VPFE, however, does not fall under the standard IOCTLs.
> >>>>>> +
> >>>>>> +In general, there is a private ioctl for configuring each of the
> >>>>>> blocks
> >>>>>> +containing hardware-dependent functions.
> >>>>>> +
> >>>>>> +The following private IOCTLs are supported:
> >>>>>> +
> >>>>>> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM
> >>>>>> +Description:
> >>>>>> + Sets/Gets the parameters required by the previewer module
> >>>>>> +Parameter:
> >>>>>> + /**
> >>>>>> + * struct prev_module_param- structure to configure preview
> >>>>>> modules
> >>>>>> + * @version: Version of the preview module
> >>>>>
> >>>>> Who is responsible for filling this field, the application or the
> >>>>> driver ?
> >>>>
> >>>> The application is responsible for filling this info. He would
> >>>> enumerate the capabilities first and set them using S_PARAM/G_PARAM.
> >>>
> >>> And what's the point of the application setting the version field ? How
> >>> does the driver use it ?
> >>
> >> The version may not be required. Will remove it.
> >>
> >>>>>> + * @len: Length of the module config structure
> >>>>>> + * @module_id: Module id
> >>>>>> + * @param: pointer to module config parameter.
> >>>>>
> >>>>> What is module_id for ? What does param point to ?
> >>>>
> >>>> There are a lot of tiny modules in the previewer/resizer which are
> >>>> enumerated as individual modules. The param points to the parameter set
> >>>> that the module expects to be set.
> >>>
> >>> Why don't you implement something similar to
> >>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ?
> >>
> >> I feel if we implement direct IOCTLS there might be many of them. To make
> >> sure than independent of the number of internal modules present, having
> >> the
> >> same IOCTL used for all modules is a good idea.
> >
> > You can set several parameters using a single ioctl, much like
> > VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter.
> >
> > PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially
> > reinventing V4L2 controls, and I don't think that's a good idea.
>
> Ok. I looked into this, and found that the structure needed to pass
> all the parameters is going to be huge. just to avoid a big structure
> from the user space, I propose:
>
> Having a union of structures and a parameter identifying the structure.
>
> In that way, we will remove the enumeration and all the other
> things except for a SET and GET, much like the CCDC_RAW_PARAMS
> like you suggested. So essentially we will have only 2 IOCTLS for setting
> the private params/configs and remove the rest. I hope that was your
> point and this proposal will solve it?
What about something like the following structure, from the OMAP3 ISP driver ?
struct omap3isp_prev_update_config {
__u32 update;
__u32 flag;
__u32 shading_shift;
struct omap3isp_prev_luma __user *luma;
struct omap3isp_prev_hmed __user *hmed;
struct omap3isp_prev_cfa __user *cfa;
struct omap3isp_prev_csup __user *csup;
struct omap3isp_prev_wbal __user *wbal;
struct omap3isp_prev_blkadj __user *blkadj;
struct omap3isp_prev_rgbtorgb __user *rgb2rgb;
struct omap3isp_prev_csc __user *csc;
struct omap3isp_prev_yclimit __user *yclimit;
struct omap3isp_prev_dcor __user *dcor;
struct omap3isp_prev_nf __user *nf;
struct omap3isp_prev_gtables __user *gamma;
};
I'll probably have more comments when I'll see the complete list of parameters
you need to expose.
> >>>>>> + */
> >>>>>> + struct prev_module_param {
> >>>>>> + char version[IMP_MAX_NAME_SIZE];
> >>>>>
> >>>>> Is there a need to express the version as a string instead of an
> >>>>> integer ?
> >>>>
> >>>> It could be integer. It is generally a fixed point num, and easy to
> >>>> read
> >>>> it as a string than an integer. Can I keep it as a string?
> >>>
> >>> Let's first decide whether a version field is needed at all :-)
> >>
> >> Will remove.
> >>
> >>>>>> + unsigned short len;
> >>>>>> + unsigned short module_id;
> >>>>>> + void *param;
> >>>>>> + };
--
Regards,
Laurent Pinchart
--
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
