| Message ID | 1343085042-19695-1-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, Thanks for the patch. On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/video/mt9v032.c | 36 +++++++++++++++++++++++++++++++++--- > 1 files changed, 33 insertions(+), 3 deletions(-) > > Changes since v1: > > - Make sure the total horizontal time will not go below 660 when setting the > horizontal blanking control > - Restrict the vertical blanking value to 3000 as documented in the datasheet. > Increasing the exposure time actually extends vertical blanking, as long as > the user doesn't forget to turn auto-exposure off... Does binning either horizontally or vertically affect the blanking limits? If the process is analogue then the answer is typically "yes". It's not directly related to this patch, but the effect of the driver just exposing one sub-device really shows better now. Besides lacking the way to specify binning as in the V4L2 subdev API (compose selection target), the user also can't use the crop bounds selection target to get the size of the pixel array. We could either accept this for the time being and fix it later on of fix it now. I prefer fixing it right now but admit that this patch isn't breaking anything, it rather is missing quite relevant functionality to control the sensor in a generic way. Kind regards,
Hi Sakari, On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: > On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > drivers/media/video/mt9v032.c | 36 +++++++++++++++++++++++++++++++++--- > > 1 files changed, 33 insertions(+), 3 deletions(-) > > > > Changes since v1: > > > > - Make sure the total horizontal time will not go below 660 when setting > > the horizontal blanking control > > > > - Restrict the vertical blanking value to 3000 as documented in the > > datasheet. Increasing the exposure time actually extends vertical > > blanking, as long as the user doesn't forget to turn auto-exposure > > off... > > Does binning either horizontally or vertically affect the blanking limits? > If the process is analogue then the answer is typically "yes". The datasheet doesn't specify whether binning and blanking can influence each other. > It's not directly related to this patch, but the effect of the driver just > exposing one sub-device really shows better now. Besides lacking the way to > specify binning as in the V4L2 subdev API (compose selection target), the > user also can't use the crop bounds selection target to get the size of the > pixel array. > > We could either accept this for the time being and fix it later on of fix it > now. > > I prefer fixing it right now but admit that this patch isn't breaking > anything, it rather is missing quite relevant functionality to control the > sensor in a generic way. I'd rather apply this patch first, as it doesn't break anything :-) Fixing the problem will require discussions, and that will take time. Binning/skipping is a pretty common feature in sensors. Exposing two sub- devices like the SMIA++ driver does is one way to fix the problem, but do we really want to do that for the vast majority of sensors ?
Hi Laurent, On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote: > On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: > > On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > > > drivers/media/video/mt9v032.c | 36 +++++++++++++++++++++++++++++++++--- > > > 1 files changed, 33 insertions(+), 3 deletions(-) > > > > > > Changes since v1: > > > > > > - Make sure the total horizontal time will not go below 660 when setting > > > the horizontal blanking control > > > > > > - Restrict the vertical blanking value to 3000 as documented in the > > > datasheet. Increasing the exposure time actually extends vertical > > > blanking, as long as the user doesn't forget to turn auto-exposure > > > off... > > > > Does binning either horizontally or vertically affect the blanking limits? > > If the process is analogue then the answer is typically "yes". > > The datasheet doesn't specify whether binning and blanking can influence each > other. Vertical binning is often analogue since digital binning would require as much temporary memory as the row holds pixels. This means the hardware already does binning before a/d conversion and there's only need to actually read half the number of rows, hence the effect on frame length. > > It's not directly related to this patch, but the effect of the driver just > > exposing one sub-device really shows better now. Besides lacking the way to > > specify binning as in the V4L2 subdev API (compose selection target), the > > user also can't use the crop bounds selection target to get the size of the > > pixel array. > > > > We could either accept this for the time being and fix it later on of fix it > > now. > > > > I prefer fixing it right now but admit that this patch isn't breaking > > anything, it rather is missing quite relevant functionality to control the > > sensor in a generic way. > > I'd rather apply this patch first, as it doesn't break anything :-) Fixing the > problem will require discussions, and that will take time. How so? There's nothing special in this sensor as far as I can see. > Binning/skipping is a pretty common feature in sensors. Exposing two sub- > devices like the SMIA++ driver does is one way to fix the problem, but do we > really want to do that for the vast majority of sensors ? If we want to expose the sensor's functionality using the V4L2 subdev API and not a sensor specific API, the answer is "yes". The bottom line is that we have just one API to configure scaling and cropping and that API (selections) is the same independently of where the operation is being performed; whether it's the sensor or the ISP. Kind regards,
Hi Sakari, On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote: > On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote: > > On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: > > > On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > > > > > drivers/media/video/mt9v032.c | 36 ++++++++++++++++++++++++++++--- > > > > 1 files changed, 33 insertions(+), 3 deletions(-) > > > > > > > > Changes since v1: > > > > > > > > - Make sure the total horizontal time will not go below 660 when > > > > setting the horizontal blanking control > > > > > > > > - Restrict the vertical blanking value to 3000 as documented in the > > > > datasheet. Increasing the exposure time actually extends vertical > > > > blanking, as long as the user doesn't forget to turn auto-exposure > > > > off... > > > > > > Does binning either horizontally or vertically affect the blanking > > > limits? If the process is analogue then the answer is typically "yes". > > > > The datasheet doesn't specify whether binning and blanking can influence > > each other. > > Vertical binning is often analogue since digital binning would require as > much temporary memory as the row holds pixels. This means the hardware > already does binning before a/d conversion and there's only need to actually > read half the number of rows, hence the effect on frame length. That will affect the frame length, but why would it affect vertical blanking ? > > > It's not directly related to this patch, but the effect of the driver > > > just exposing one sub-device really shows better now. Besides lacking > > > the way to specify binning as in the V4L2 subdev API (compose selection > > > target), the user also can't use the crop bounds selection target to get > > > the size of the pixel array. > > > > > > We could either accept this for the time being and fix it later on of > > > fix it now. > > > > > > I prefer fixing it right now but admit that this patch isn't breaking > > > anything, it rather is missing quite relevant functionality to control > > > the sensor in a generic way. > > > > I'd rather apply this patch first, as it doesn't break anything :-) Fixing > > the problem will require discussions, and that will take time. > > How so? There's nothing special in this sensor as far as I can see. > > > Binning/skipping is a pretty common feature in sensors. Exposing two sub- > > devices like the SMIA++ driver does is one way to fix the problem, but do > > we really want to do that for the vast majority of sensors ? > > If we want to expose the sensor's functionality using the V4L2 subdev API > and not a sensor specific API, the answer is "yes". The bottom line is that > we have just one API to configure scaling and cropping and that API > (selections) is the same independently of where the operation is being > performed; whether it's the sensor or the ISP. If we want to expose two subdevices for every sensor we will need to get both kernelspace (ISP drivers) and userspace ready for this. I'm not against the idea, but we need to plan it properly.
Hi Laurent, Laurent Pinchart wrote: > On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote: >> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote: >>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: >>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>> --- >>>>> >>>>> drivers/media/video/mt9v032.c | 36 ++++++++++++++++++++++++++++--- >>>>> 1 files changed, 33 insertions(+), 3 deletions(-) >>>>> >>>>> Changes since v1: >>>>> >>>>> - Make sure the total horizontal time will not go below 660 when >>>>> setting the horizontal blanking control >>>>> >>>>> - Restrict the vertical blanking value to 3000 as documented in the >>>>> datasheet. Increasing the exposure time actually extends vertical >>>>> blanking, as long as the user doesn't forget to turn auto-exposure >>>>> off... >>>> >>>> Does binning either horizontally or vertically affect the blanking >>>> limits? If the process is analogue then the answer is typically "yes". >>> >>> The datasheet doesn't specify whether binning and blanking can influence >>> each other. >> >> Vertical binning is often analogue since digital binning would require as >> much temporary memory as the row holds pixels. This means the hardware >> already does binning before a/d conversion and there's only need to actually >> read half the number of rows, hence the effect on frame length. > > That will affect the frame length, but why would it affect vertical blanking ? Frame length == image height + vertical blanking. The SMIA++ driver (at least) associates the blanking controls to the pixel array subdev. They might be more naturally placed to the source (either binner or scaler) but the width and height (to calculate the frame and line length) are related to the dimensions of the pixel array crop rectangle. So when the binning configuration changes, that changes the limits for blanking and thus possibly also blanking itself. >>>> It's not directly related to this patch, but the effect of the driver >>>> just exposing one sub-device really shows better now. Besides lacking >>>> the way to specify binning as in the V4L2 subdev API (compose selection >>>> target), the user also can't use the crop bounds selection target to get >>>> the size of the pixel array. >>>> >>>> We could either accept this for the time being and fix it later on of >>>> fix it now. >>>> >>>> I prefer fixing it right now but admit that this patch isn't breaking >>>> anything, it rather is missing quite relevant functionality to control >>>> the sensor in a generic way. >>> >>> I'd rather apply this patch first, as it doesn't break anything :-) Fixing >>> the problem will require discussions, and that will take time. >> >> How so? There's nothing special in this sensor as far as I can see. >> >>> Binning/skipping is a pretty common feature in sensors. Exposing two sub- >>> devices like the SMIA++ driver does is one way to fix the problem, but do >>> we really want to do that for the vast majority of sensors ? >> >> If we want to expose the sensor's functionality using the V4L2 subdev API >> and not a sensor specific API, the answer is "yes". The bottom line is that >> we have just one API to configure scaling and cropping and that API >> (selections) is the same independently of where the operation is being >> performed; whether it's the sensor or the ISP. > > If we want to expose two subdevices for every sensor we will need to get both > kernelspace (ISP drivers) and userspace ready for this. I'm not against the > idea, but we need to plan it properly. I fully agree that this has to be planned properly; e.g. libv4l support for controlling low level devices required by this is completely missing right now. Cheers,
Hi Sakari, On Monday 13 August 2012 17:18:20 Sakari Ailus wrote: > Laurent Pinchart wrote: > > On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote: > >> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote: > >>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: > >>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>> --- > >>>>> > >>>>> drivers/media/video/mt9v032.c | 36 > >>>>> ++++++++++++++++++++++++++++--- > >>>>> 1 files changed, 33 insertions(+), 3 deletions(-) > >>>>> > >>>>> Changes since v1: > >>>>> > >>>>> - Make sure the total horizontal time will not go below 660 when > >>>>> setting the horizontal blanking control > >>>>> > >>>>> - Restrict the vertical blanking value to 3000 as documented in the > >>>>> datasheet. Increasing the exposure time actually extends vertical > >>>>> blanking, as long as the user doesn't forget to turn auto-exposure > >>>>> off... > >>>> > >>>> Does binning either horizontally or vertically affect the blanking > >>>> limits? If the process is analogue then the answer is typically "yes". > >>> > >>> The datasheet doesn't specify whether binning and blanking can influence > >>> each other. > >> > >> Vertical binning is often analogue since digital binning would require as > >> much temporary memory as the row holds pixels. This means the hardware > >> already does binning before a/d conversion and there's only need to > >> actually read half the number of rows, hence the effect on frame length. > > > > That will affect the frame length, but why would it affect vertical > > blanking ? > > Frame length == image height + vertical blanking. > > The SMIA++ driver (at least) associates the blanking controls to the > pixel array subdev. They might be more naturally placed to the source > (either binner or scaler) but the width and height (to calculate the > frame and line length) are related to the dimensions of the pixel array > crop rectangle. > > So when the binning configuration changes, that changes the limits for > blanking and thus possibly also blanking itself. Do the blanking controls expose blanking after binning or before binning ? In the later case I don't see how binning would influence them. > >>>> It's not directly related to this patch, but the effect of the driver > >>>> just exposing one sub-device really shows better now. Besides lacking > >>>> the way to specify binning as in the V4L2 subdev API (compose selection > >>>> target), the user also can't use the crop bounds selection target to > >>>> get the size of the pixel array. > >>>> > >>>> We could either accept this for the time being and fix it later on of > >>>> fix it now. > >>>> > >>>> I prefer fixing it right now but admit that this patch isn't breaking > >>>> anything, it rather is missing quite relevant functionality to control > >>>> the sensor in a generic way. > >>> > >>> I'd rather apply this patch first, as it doesn't break anything :-) > >>> Fixing the problem will require discussions, and that will take time. > >> > >> How so? There's nothing special in this sensor as far as I can see. > >> > >>> Binning/skipping is a pretty common feature in sensors. Exposing two > >>> sub-devices like the SMIA++ driver does is one way to fix the problem, > >>> but do we really want to do that for the vast majority of sensors ? > >> > >> If we want to expose the sensor's functionality using the V4L2 subdev API > >> and not a sensor specific API, the answer is "yes". The bottom line is > >> that we have just one API to configure scaling and cropping and that API > >> (selections) is the same independently of where the operation is being > >> performed; whether it's the sensor or the ISP. > > > > If we want to expose two subdevices for every sensor we will need to get > > both kernelspace (ISP drivers) and userspace ready for this. I'm not > > against the idea, but we need to plan it properly. > > I fully agree that this has to be planned properly; e.g. libv4l support for > controlling low level devices required by this is completely missing right > now.
Laurent Pinchart wrote: > Hi Sakari, > > On Monday 13 August 2012 17:18:20 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote: >>>> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote: >>>>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote: >>>>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote: >>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>> --- >>>>>>> >>>>>>> drivers/media/video/mt9v032.c | 36 >>>>>>> ++++++++++++++++++++++++++++--- >>>>>>> 1 files changed, 33 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> Changes since v1: >>>>>>> >>>>>>> - Make sure the total horizontal time will not go below 660 when >>>>>>> setting the horizontal blanking control >>>>>>> >>>>>>> - Restrict the vertical blanking value to 3000 as documented in the >>>>>>> datasheet. Increasing the exposure time actually extends vertical >>>>>>> blanking, as long as the user doesn't forget to turn auto-exposure >>>>>>> off... >>>>>> >>>>>> Does binning either horizontally or vertically affect the blanking >>>>>> limits? If the process is analogue then the answer is typically "yes". >>>>> >>>>> The datasheet doesn't specify whether binning and blanking can influence >>>>> each other. >>>> >>>> Vertical binning is often analogue since digital binning would require as >>>> much temporary memory as the row holds pixels. This means the hardware >>>> already does binning before a/d conversion and there's only need to >>>> actually read half the number of rows, hence the effect on frame length. >>> >>> That will affect the frame length, but why would it affect vertical >>> blanking ? >> >> Frame length == image height + vertical blanking. >> >> The SMIA++ driver (at least) associates the blanking controls to the >> pixel array subdev. They might be more naturally placed to the source >> (either binner or scaler) but the width and height (to calculate the >> frame and line length) are related to the dimensions of the pixel array >> crop rectangle. >> >> So when the binning configuration changes, that changes the limits for >> blanking and thus possibly also blanking itself. > > Do the blanking controls expose blanking after binning or before binning ? In > the later case I don't see how binning would influence them. Some sensors control the blanking in pixel array directly whereas some, like the SMIA++, control the frame length in the source (scaler or binner) source instead. So it is up to the sensor hardware --- I think it's still better to keep all the controls in a single subdev. Otherwise it'd be quite difficult for the user to figure out how to calculate the frame rate. Kind regards,
diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c index 2203a6f..e713ad9 100644 --- a/drivers/media/video/mt9v032.c +++ b/drivers/media/video/mt9v032.c @@ -50,9 +50,11 @@ #define MT9V032_WINDOW_WIDTH_MAX 752 #define MT9V032_HORIZONTAL_BLANKING 0x05 #define MT9V032_HORIZONTAL_BLANKING_MIN 43 +#define MT9V032_HORIZONTAL_BLANKING_DEF 94 #define MT9V032_HORIZONTAL_BLANKING_MAX 1023 #define MT9V032_VERTICAL_BLANKING 0x06 #define MT9V032_VERTICAL_BLANKING_MIN 4 +#define MT9V032_VERTICAL_BLANKING_DEF 45 #define MT9V032_VERTICAL_BLANKING_MAX 3000 #define MT9V032_CHIP_CONTROL 0x07 #define MT9V032_CHIP_CONTROL_MASTER_MODE (1 << 3) @@ -129,8 +131,10 @@ struct mt9v032 { int power_count; struct mt9v032_platform_data *pdata; + u16 chip_control; u16 aec_agc; + u16 hblank; }; static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd) @@ -188,6 +192,16 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable) return 0; } +static int +mt9v032_update_hblank(struct mt9v032 *mt9v032) +{ + struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev); + struct v4l2_rect *crop = &mt9v032->crop; + + return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING, + max_t(s32, mt9v032->hblank, 660 - crop->width)); +} + #define EXT_CLK 25000000 static int mt9v032_power_on(struct mt9v032 *mt9v032) @@ -322,8 +336,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable) if (ret < 0) return ret; - ret = mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING, - max(43, 660 - crop->width)); + ret = mt9v032_update_hblank(mt9v032); if (ret < 0) return ret; @@ -505,6 +518,14 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl) return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH, ctrl->val); + case V4L2_CID_HBLANK: + mt9v032->hblank = ctrl->val; + return mt9v032_update_hblank(mt9v032); + + case V4L2_CID_VBLANK: + return mt9v032_write(client, MT9V032_VERTICAL_BLANKING, + ctrl->val); + case V4L2_CID_TEST_PATTERN: switch (ctrl->val) { case 0: @@ -701,7 +722,7 @@ static int mt9v032_probe(struct i2c_client *client, mutex_init(&mt9v032->power_lock); mt9v032->pdata = client->dev.platform_data; - v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 5); + v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 7); v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops, V4L2_CID_AUTOGAIN, 0, 1, 1, 1); @@ -715,6 +736,14 @@ static int mt9v032_probe(struct i2c_client *client, V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN, MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1, MT9V032_TOTAL_SHUTTER_WIDTH_DEF); + v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops, + V4L2_CID_HBLANK, MT9V032_HORIZONTAL_BLANKING_MIN, + MT9V032_HORIZONTAL_BLANKING_MAX, 1, + MT9V032_HORIZONTAL_BLANKING_DEF); + v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops, + V4L2_CID_VBLANK, MT9V032_VERTICAL_BLANKING_MIN, + MT9V032_VERTICAL_BLANKING_MAX, 1, + MT9V032_VERTICAL_BLANKING_DEF); mt9v032->pixel_rate = v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops, V4L2_CID_PIXEL_RATE, 0, 0, 1, 0); @@ -740,6 +769,7 @@ static int mt9v032_probe(struct i2c_client *client, mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB; mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE; + mt9v032->hblank = MT9V032_HORIZONTAL_BLANKING_DEF; v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops); mt9v032->subdev.internal_ops = &mt9v032_subdev_internal_ops;
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/video/mt9v032.c | 36 +++++++++++++++++++++++++++++++++--- 1 files changed, 33 insertions(+), 3 deletions(-) Changes since v1: - Make sure the total horizontal time will not go below 660 when setting the horizontal blanking control - Restrict the vertical blanking value to 3000 as documented in the datasheet. Increasing the exposure time actually extends vertical blanking, as long as the user doesn't forget to turn auto-exposure off...