Hi, Reviewing and testing this one was, erm, interesting. See comments inline. On 04/28/2012 05:09 PM, Hans Verkuil wrote: > From: Hans Verkuil > > Signed-off-by: Hans Verkuil > --- > drivers/media/video/gspca/zc3xx.c | 451 +++++++++++++++---------------------- > 1 file changed, 182 insertions(+), 269 deletions(-) > > diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c > index 7d9a4f1..e7b7599 100644 > --- a/drivers/media/video/gspca/zc3xx.c > +++ b/drivers/media/video/gspca/zc3xx.c > +static int sd_setautogain(struct gspca_dev *gspca_dev, s32 val) > +{ > + if (!gspca_dev->streaming) > + return 0; > + setautogain(gspca_dev, val); > + > + return gspca_dev->usb_err; > +} > + zcxx_s_ctrl can just as well call setautogain directly as it does for the others. It should gspca_dev->streaming for all controls anyways (more on that below). > +static int sd_setquality(struct gspca_dev *gspca_dev, s32 val) > +{ > + struct sd *sd = (struct sd *) gspca_dev; > + int i; > + > + for (i = 0; i< ARRAY_SIZE(jpeg_qual) - 1; i++) { > + if (val<= jpeg_qual[i]) > + break; > + } > + sd->reg08 = i; > + if (!gspca_dev->streaming) > + return 0; > + jpeg_set_qual(sd->jpeg_hdr, val); > + return gspca_dev->usb_err; > +} > + This for 99% duplicates the special handling you already have for this in zcxx_s_ctrl, which is also the only caller. More over this gets its wrong as under certain conditions it will end up with a different value of i then the code in zcxx_s_ctrl, and zcxx_s_ctrl bases the value it returns to the caller as the "achieved" value on its own calculation of i. So I thought lets refactor this a bit to get rid of this duplication and that turned out to be a long journey rather then a quick patch :) While testing my changes I found that there are various (pre-existing) problems with how zc3xx handles JPEG quality so I've fixed those first. The result is a 6 patch patchset (attached), which besides many fixes to the JPEG quality handling, also includes a new version of this patch removing the duplication. > +static int sd_set_jcomp(struct gspca_dev *gspca_dev, > + struct v4l2_jpegcompression *jcomp) > +{ > + struct sd *sd = (struct sd *) gspca_dev; > + > + if (sd->jpegqual) { > + v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality); > + jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual); > + return gspca_dev->usb_err; > + } > + jcomp->quality = jpeg_qual[sd->reg08]; > + return 0; > +} > + > +static int sd_get_jcomp(struct gspca_dev *gspca_dev, > + struct v4l2_jpegcompression *jcomp) > +{ > + struct sd *sd = (struct sd *) gspca_dev; > + > + memset(jcomp, 0, sizeof *jcomp); > + jcomp->quality = jpeg_qual[sd->reg08]; > + jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT > + | V4L2_JPEG_MARKER_DQT; > + return 0; > +} > + No need to move these upwards in the source file, leaving them where they are makes it easier to see what is actually changed. > +static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler); > + > + switch (ctrl->id) { > + case V4L2_CID_AUTOGAIN: > + if (ctrl->val&& sd->exposure&& sd->gspca_dev.streaming) > + sd->exposure->val = getexposure(&sd->gspca_dev); > + break; > + } > + return 0; > +} The call to getexposure needs to be surrounded by locking / unlocking usb_lock, and gspca_de->usb_err should be checked after the call. In general all calls to the actual hardware need to be made with the usb_lock hold, so that they cannot race with for example hardware accesses done from sd_start. This is important since although the usb-core will ensure that we never can do more then one control request at a time sometimes several control requests need to be done in order without interference (for example for accessing the i2c bus between the bridge and the sensor). > + > +static int zcxx_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler); > + int ret; > + int i; > + > + switch (ctrl->id) { > + /* gamma/brightness/contrast cluster */ > + case V4L2_CID_GAMMA: > + setcontrast(&sd->gspca_dev, sd->gamma->val, > + sd->brightness->val, sd->contrast->val); > + return 0; > + /* autogain/exposure cluster */ > + case V4L2_CID_AUTOGAIN: > + ret = sd_setautogain(&sd->gspca_dev, ctrl->val); > + if (!ret&& !ctrl->val&& sd->exposure) > + setexposure(&sd->gspca_dev, sd->exposure->val); > + return ret; > + case V4L2_CID_POWER_LINE_FREQUENCY: > + setlightfreq(&sd->gspca_dev, ctrl->val); > + return 0; > + case V4L2_CID_SHARPNESS: > + setsharpness(&sd->gspca_dev, ctrl->val); > + return 0; > + case V4L2_CID_JPEG_COMPRESSION_QUALITY: > + for (i = 0; i< ARRAY_SIZE(jpeg_qual) - 1; i++) { > + if (ctrl->val<= jpeg_qual[i]) > + break; > + } > + if (i> 0&& i == sd->reg08&& ctrl->val< jpeg_qual[sd->reg08]) > + i--; > + ctrl->val = jpeg_qual[i]; > + return sd_setquality(&sd->gspca_dev, ctrl->val); > + } > + return -EINVAL; > +} > + Like zcxx_g_volatile_ctrl this function to should take the usb_lock before calling setfoo. Also after acquiring the lock it should check if the device is streaming and if it is not streaming it should not call any setfoo functions, as those will be done on sd_start then. Note that setfoo may work when calling them on this bridge while not streaming, but there are no guarantees the same holds for other bridges. In general the control paradigm in gspca is to not send any control values to the camera until streaming starts. Last this function to should check gspca_dev->usb_err (which works a bit like hdl->error accumulating io errors made by setfoo calls). So the order for any s_ctrl in gspca should normally be: 1) take usb_lock 2) clear gspca_dev->usb_err 3) check gspca_dev->streaming if not streaming normally goto unlock and return success immediately But there may be special cases where the passed in value should first be rounded to the nearest supported value (ie the jpeg quality in zc3xx.c) 4) call setfoo functions 5) store gspca_dev->usb_err in a local "ret" variable 6) unlock 7) return ret > +static const struct v4l2_ctrl_ops zcxx_ctrl_ops = { > + .g_volatile_ctrl = zcxx_g_volatile_ctrl, > + .s_ctrl = zcxx_s_ctrl, > +}; > + > /* this function is called at probe and resume time */ > static int sd_init(struct gspca_dev *gspca_dev) > { > struct sd *sd = (struct sd *) gspca_dev; > + struct v4l2_ctrl_handler *hdl =&sd->ctrl_handler; > struct cam *cam; > int sensor; > static const u8 gamma[SENSOR_MAX] = { > @@ -6688,7 +6673,6 @@ static int sd_init(struct gspca_dev *gspca_dev) > case 0x2030: > PDEBUG(D_PROBE, "Find Sensor PO2030"); > sd->sensor = SENSOR_PO2030; > - sd->ctrls[SHARPNESS].def = 0; /* from win traces */ > break; > case 0x7620: > PDEBUG(D_PROBE, "Find Sensor OV7620"); > @@ -6730,30 +6714,40 @@ static int sd_init(struct gspca_dev *gspca_dev) > break; > } > > - sd->ctrls[GAMMA].def = gamma[sd->sensor]; > - sd->reg08 = reg08_tb[sd->sensor]; > - sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08]; > - sd->ctrls[QUALITY].min = jpeg_qual[0]; > - sd->ctrls[QUALITY].max = jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1]; > - > - switch (sd->sensor) { > - case SENSOR_HV7131R: > - gspca_dev->ctrl_dis = (1<< QUALITY); > - break; > - case SENSOR_OV7630C: > - gspca_dev->ctrl_dis = (1<< LIGHTFREQ) | (1<< EXPOSURE); > - break; > - case SENSOR_PAS202B: > - gspca_dev->ctrl_dis = (1<< QUALITY) | (1<< EXPOSURE); > - break; > - default: > - gspca_dev->ctrl_dis = (1<< EXPOSURE); > - break; > + gspca_dev->vdev.ctrl_handler = hdl; > + v4l2_ctrl_handler_init(hdl, 8); > + sd->brightness = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_BRIGHTNESS, 0, 255, 1, 128); > + sd->contrast = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_CONTRAST, 0, 255, 1, 128); > + sd->gamma = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_GAMMA, 1, 6, 1, gamma[sd->sensor]); > + if (sd->sensor == SENSOR_HV7131R) > + sd->exposure = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_EXPOSURE, 0x30d, 0x493e, 1, 0x927); > + sd->autogain = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_AUTOGAIN, 0, 1, 1, 1); > + if (sd->sensor != SENSOR_OV7630C&& sd->sensor != SENSOR_PAS202B) > + sd->plfreq = v4l2_ctrl_new_std_menu(hdl,&zcxx_ctrl_ops, > + V4L2_CID_POWER_LINE_FREQUENCY, > + V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0, > + V4L2_CID_POWER_LINE_FREQUENCY_DISABLED); This is wrong the PAS202B should have a powerlinefreq control. > + sd->sharpness = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_SHARPNESS, 0, 3, 1, > + sd->sensor == SENSOR_PO2030 ? 0 : 2); > + if (sd->sensor != SENSOR_HV7131R&& sd->sensor != SENSOR_PAS202B) > + sd->jpegqual = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops, > + V4L2_CID_JPEG_COMPRESSION_QUALITY, > + jpeg_qual[0], jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1], 1, > + jpeg_qual[sd->reg08]); > + if (hdl->error) { > + pr_err("Could not initialize controls\n"); > + return hdl->error; > } > -#if AUTOGAIN_DEF > - if (sd->ctrls[AUTOGAIN].val) > - gspca_dev->ctrl_inac = (1<< EXPOSURE); > -#endif > + v4l2_ctrl_cluster(3,&sd->gamma); > + if (sd->sensor == SENSOR_HV7131R) > + v4l2_ctrl_auto_cluster(2,&sd->autogain, 0, true); > + sd->reg08 = reg08_tb[sd->sensor]; > > /* switch off the led */ > reg_w(gspca_dev, 0x01, 0x0000); > @@ -6864,7 +6858,7 @@ static int sd_start(struct gspca_dev *gspca_dev) > reg_w(gspca_dev, 0x03, 0x0008); > break; > } > - setsharpness(gspca_dev); > + setsharpness(gspca_dev, v4l2_ctrl_g_ctrl(sd->sharpness)); > > /* set the gamma tables when not set */ > switch (sd->sensor) { > @@ -6873,7 +6867,9 @@ static int sd_start(struct gspca_dev *gspca_dev) > case SENSOR_OV7630C: > break; > default: > - setcontrast(gspca_dev); > + setcontrast(&sd->gspca_dev, v4l2_ctrl_g_ctrl(sd->gamma), > + v4l2_ctrl_g_ctrl(sd->brightness), > + v4l2_ctrl_g_ctrl(sd->contrast)); > break; > } > setmatrix(gspca_dev); /* one more time? */ > @@ -6886,7 +6882,7 @@ static int sd_start(struct gspca_dev *gspca_dev) > } > setquality(gspca_dev); > jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]); > - setlightfreq(gspca_dev); > + setlightfreq(gspca_dev, v4l2_ctrl_g_ctrl(sd->plfreq)); sd->plfreq can be null, so this needs an if (sd->plfreq) > > switch (sd->sensor) { > case SENSOR_ADCM2700: Moving on to the next patch in the series now, hopefully that one will go quicker :) Regards, Hans