Re: Improving ov7670 sensor driver.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mauro,
thank you for your interest.

On 13 September 2012 15:00, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> wrote:
> Hi Javier,
>
> I'm not too familiar with soc_camera and ov7670 drivers, so my comments
> reflects my understanding of the question, without taking into account
> drivers specifics.
>
> Em 13-09-2012 06:48, javier Martin escreveu:
>> Hi,
>> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor
>> attached to the CSI interface. Apparently, this sensor is fully
>> compatible with the old ov7670. For this reason, it seems rather
>> sensible that they should share the same driver: ov7670.c
>> One of the challenges we have to face is that capture video support
>> for our platform is mx2_camera.c, which is a soc-camera host driver;
>> while ov7670.c was developed for being used as part of a more complex
>> video card.
>>
>> Here is the list of current users of ov7670:
>>
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c
>> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c
>
> In order to avoid breakages on those drivers, we need to be sure that
> none of the changes will alter the register settings used there.
>
> (C/C Hans de Goede, as he is the gspca maintainer)
>
>>
>> These are basically the improvements we need to make to this driver in
>> order to satisfy our needs:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>> 4.- Disable pixclk during horizontal blanking.
>> 5.- min_height, min_width should be respected in try_fmt().
>> 6.- Pass platform data when used with a soc-camera host driver.
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>
> Doing one patch per change helps to review the changes individually.
> I suspect that it will needed to be tested with the above drivers,
> anyway.
>
>> I will try to summarize below why we need to accomplish each of the
>> previous tasks and what solution we propose for them:
>>
>> 1.- Adapt v4l2 controls to the subvevice control framework, with a
>> proper ctrl handler, etc...
>>
>> Why? Because soc-camera needs to inherit v4l2 subdevice controls in
>> order to expose them to user space.
>> How? Something like the following, incomplete, patch:
>>
>> ---
>> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
>>  struct ov7670_format_struct;  /* coming later */
>>  struct ov7670_info {
>>         struct v4l2_subdev sd;
>> +       struct v4l2_ctrl_handler hdl;
>>         struct ov7670_format_struct *fmt;  /* Current format */
>>         unsigned char sat;              /* Saturation value */
>>         int hue;                        /* Hue value */
>>
>>
>> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct
>> v4l2_subdev *sd, struct v4l2_dbg_register *r
>>
>>  /* ----------------------------------------------------------------------- */
>>
>> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
>> +       .s_ctrl = ov7670_s_ctrl,
>> +};
>> +
>>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>>         .g_chip_ident = ov7670_g_chip_ident,
>> -       .g_ctrl = ov7670_g_ctrl,
>> -       .s_ctrl = ov7670_s_ctrl,
>> +       .g_ctrl = v4l2_subdev_g_ctrl,
>> +       .s_ctrl = v4l2_subdev_s_ctrl,
>>         .queryctrl = ov7670_queryctrl,
>>         .reset = ov7670_reset,
>>         .init = ov7670_init,
>>
>> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client,
>>         v4l_info(client, "chip found @ 0x%02x (%s)\n",
>>                         client->addr << 1, client->adapter->name);
>>
>> +       v4l2_ctrl_handler_init(&info->hdl, 1);
>> +       v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> V4L2_CID_VFLIP, 0, 1, 1, 0);
>> ...
>> ...
>> +       sd->ctrl_handler = &info->hdl;
>> +       if (info->hdl.error) {
>> +               v4l2_ctrl_handler_free(&info->hdl);
>> +               kfree(info);
>> +               return info->hdl.error;
>> +       }
>> +       v4l2_ctrl_handler_setup(&info->hdl);
>> +
>> ---
>
> Tests are required here, but I don't think this would break anything.
>>
>> 2.- Add the possibility to bypass PLL and clkrc preescaler.
>>
>> Why? The formula to get the desired frame rate in this chip in YUV is
>> the following: fps = fpclk / (2 * 510 * 784) This means that for a
>> desired fps = 30 we need fpclk = 24MHz. For that reason we have a
>> clean 24MHz xvclk input that comes from an oscillator. If we enable
>> the PLL it internally transforms the 24MHz in 22MHz and thus fps is
>> not 30 but 27. In order to get 30fps we need to bypass the PLL.
>> How? Defining a platform flag 'direct_clk' or similar that allows
>> xvclk being used directly as the pixel clock.
>
> As this should be a new platform data, provided that the old behavior
> is to use the old formula, this shouldn't break anything.
>
>>
>> 3.- Adjust vstart/vstop in order to remove an horizontal green line.
>>
>> Why? Currently, in the driver, for VGA, vstart =  10 and vstop = 490.
>>>From our tests we found out that vstart = 14, vstop = 494 in order to
>> remove a disgusting horizontal green line in ov7675.
>> How? It seems these sensor aren't provided with a version register or
>> anything similar so I can't think of a clean solution for this yet.
>> Suggestions will be much appreciated.
>
> If it is not possible to differentiate between ov7670 and ov7675, just
> add a platform data flag, in order to identify the ov7675
>
>> 4.- Disable pixclk during horizontal blanking.
>>
>> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods.
>> How? Through a private V4L2 control.
>
> Hmm... I'm assuming that there is a register that controls pixclk disable
> during horizontal blanking. In this case, the better is to add support for
> it also via platform data.
>
>>
>> 5.- min_height, min_width should be respected in try_fmt().
>> Why? Otherwise you are telling the user you are going to use a
>> different size than the one you are going to use.
>> How? With a patch similar to this:
>>
>> ---
>> @@ -759,8 +772,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
>>                 struct ov7670_format_struct **ret_fmt,
>>                 struct ov7670_win_size **ret_wsize)
>>  {
>> -       int index;
>> +       int index, i;
>> +       int win_sizes_limit = N_WIN_SIZES;
>>         struct ov7670_win_size *wsize;
>> +       struct ov7670_info *info = to_state(sd);
>>
>>         for (index = 0; index < N_OV7670_FMTS; index++)
>>                 if (ov7670_formats[index].mbus_code == fmt->code)
>> @@ -776,15 +791,30 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
>>          * Fields: the OV devices claim to be progressive.
>>          */
>>         fmt->field = V4L2_FIELD_NONE;
>> +
>> +       /*
>> +        * Don't consider values that don't match min_height and min_width
>> +        * constraints.
>> +        */
>> +       if (info->min_width || info->min_height)
>> +               for (i=0; i < N_WIN_SIZES; i++) {
>> +                       wsize = ov7670_win_sizes + i;
>> +
>> +                       if (wsize->width < info->min_width ||
>> +                           wsize->height < info->min_height) {
>> +                               win_sizes_limit = i;
>> +                               break;
>> +                       }
>> +               }
>>         /*
>>          * Round requested image size down to the nearest
>>          * we support, but not below the smallest.
>>          */
>> -       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes + N_WIN_SIZES;
>> +       for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes +
>> win_sizes_limit;
>>              wsize++)
>>                 if (fmt->width >= wsize->width && fmt->height >= wsize->height)
>>                         break;
>> -       if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
>> +       if (wsize >= ov7670_win_sizes + win_sizes_limit)
>>                 wsize--;   /* Take the smallest one */
>>         if (ret_wsize != NULL)
>>                 *ret_wsize = wsize;
>
> Of course, patch need to be tested, but that change looks fine on my eyes.
>
>> ---
>>
>> 6.- Pass platform data when used with a soc-camera host driver.
>> Why? We need to set several platform data like 'min_height',
>> 'min_width' and others.
>> How? This is an old subject we discussed in January. We agreed that
>> some soc-camera core changes were needed, but I couldn't find the time
>> and I think nobody else has addressed it either. Please, correct me if
>> I am wrong:http://patchwork.linuxtv.org/patch/8860/
>
> Hmm... patch 8860 is related to s_input logic, and not to pass platform data.

I know but in the end we discussed passing platform data to tvp5150
sensor using soc-camera. This is a similar case for the ov7670. Here
is the specific e-mail in the thread:
http://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg41437.html


> For sure passing platform data is needed, as different boards may require
> different sensor configurations. If soc_camera doesn't support it yet,
> this needs to be added there.
>
>>
>> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl.
>> Why? Because the platform will be used in several countries.
>> How? As long as point 1 is solved this is quite trivial.
>>
>>
>> The reason of this e-mail is to discuss whether you find these
>> solution suitable or not and, more important, whether you think the
>> suggested changes could break existing drivers.
>>
>> Regards.
>>
>
> Regards,
> Mauro
>



-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux