| Message ID | 20201109164934.134919-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series | media: ov5647: Support RaspberryPi Camera Module v1 | expand |
Hi Jacopo, On Mon, Nov 09, 2020 at 05:49:18PM +0100, Jacopo Mondi wrote: > Break format handling out from the main driver structure. > > This commit prepares for the introduction of more sensor formats and > resolutions by instrumenting the existing operation to work on multiple > modes instead of assuming a single one supported. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5647.c | 90 +++++++++++++++++++++++++++----------- > 1 file changed, 65 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index 91193706b6fe1..d41c11afe1216 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -84,18 +84,28 @@ struct regval_list { > u8 data; > }; > > +struct ov5647_mode { > + struct v4l2_mbus_framefmt format; > + const struct regval_list *reg_list; > + unsigned int num_regs; > +}; > + > +struct ov5647_format_list { > + unsigned int mbus_code; > + const struct ov5647_mode *modes; > + unsigned int num_modes; > +}; > + > struct ov5647 { > struct v4l2_subdev sd; > struct media_pad pad; > struct mutex lock; > - struct v4l2_mbus_framefmt format; > - unsigned int width; > - unsigned int height; > int power_count; > struct clk *xclk; > struct gpio_desc *pwdn; > bool clock_ncont; > struct v4l2_ctrl_handler ctrls; > + const struct ov5647_mode *mode; > }; > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd) > @@ -115,7 +125,7 @@ static struct regval_list sensor_oe_enable_regs[] = { > {0x3002, 0xe4}, > }; > > -static struct regval_list ov5647_640x480[] = { > +static const struct regval_list ov5647_640x480[] = { > {0x0100, 0x00}, > {0x0103, 0x01}, > {0x3034, 0x08}, > @@ -205,6 +215,33 @@ static struct regval_list ov5647_640x480[] = { > {0x0100, 0x01}, > }; > > +static const struct ov5647_mode ov5647_8bit_modes[] = { > + { > + .format = { > + .code = MEDIA_BUS_FMT_SBGGR8_1X8, > + .colorspace = V4L2_COLORSPACE_SRGB, > + .field = V4L2_FIELD_NONE, > + .width = 640, > + .height = 480 > + }, > + .reg_list = ov5647_640x480, > + .num_regs = ARRAY_SIZE(ov5647_640x480) > + }, > +}; > + > +static const struct ov5647_format_list ov5647_formats[] = { > + { > + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8, > + .modes = ov5647_8bit_modes, > + .num_modes = ARRAY_SIZE(ov5647_8bit_modes), > + }, > +}; > + > +#define OV5647_NUM_FORMATS (ARRAY_SIZE(ov5647_formats)) > + > +#define OV5647_DEFAULT_MODE (&ov5647_formats[0].modes[0]) > +#define OV5647_DEFAULT_FORMAT (ov5647_formats[0].modes[0].format) > + > static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) > { > unsigned char data[3] = { reg >> 8, reg & 0xff, val}; > @@ -245,7 +282,7 @@ static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > } > > static int ov5647_write_array(struct v4l2_subdev *sd, > - struct regval_list *regs, int array_size) > + const struct regval_list *regs, int array_size) > { > int i, ret; > > @@ -275,6 +312,7 @@ static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel) > static int ov5647_set_mode(struct v4l2_subdev *sd) > { > struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct ov5647 *sensor = to_sensor(sd); > u8 resetval, rdval; > int ret; > > @@ -282,8 +320,8 @@ static int ov5647_set_mode(struct v4l2_subdev *sd) > if (ret < 0) > return ret; > > - ret = ov5647_write_array(sd, ov5647_640x480, > - ARRAY_SIZE(ov5647_640x480)); > + ret = ov5647_write_array(sd, sensor->mode->reg_list, > + sensor->mode->num_regs); > if (ret < 0) { > dev_err(&client->dev, "write sensor default regs error\n"); > return ret; > @@ -494,10 +532,10 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > - if (code->index > 0) > + if (code->index >= OV5647_NUM_FORMATS) > return -EINVAL; > > - code->code = MEDIA_BUS_FMT_SBGGR8_1X8; > + code->code = ov5647_formats[code->index].mbus_code; > > return 0; > } > @@ -506,16 +544,24 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > - if (fse->index) > + const struct v4l2_mbus_framefmt *fmt; > + unsigned int i = 0; > + > + for (; i < OV5647_NUM_FORMATS; ++i) { > + if (ov5647_formats[i].mbus_code == fse->code) > + break; > + } > + if (i == OV5647_NUM_FORMATS) > return -EINVAL; > > - if (fse->code != MEDIA_BUS_FMT_SBGGR8_1X8) > + if (fse->index >= ov5647_formats[i].num_modes) > return -EINVAL; > > - fse->min_width = 640; > - fse->max_width = 640; > - fse->min_height = 480; > - fse->max_height = 480; > + fmt = &ov5647_formats[i].modes[fse->index].format; > + fse->min_width = fmt->width; > + fse->max_width = fmt->width; > + fse->min_height = fmt->height; > + fse->max_height = fmt->height; > > return 0; > } > @@ -528,11 +574,7 @@ static int ov5647_set_get_fmt(struct v4l2_subdev *sd, > > /* Only one format is supported, so return that. */ > memset(fmt, 0, sizeof(*fmt)); > - fmt->code = MEDIA_BUS_FMT_SBGGR8_1X8; > - fmt->colorspace = V4L2_COLORSPACE_SRGB; > - fmt->field = V4L2_FIELD_NONE; > - fmt->width = 640; > - fmt->height = 480; > + *fmt = OV5647_DEFAULT_FORMAT; With this change, the memset above becomes redundant. > > return 0; > } > @@ -591,11 +633,7 @@ static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > crop->width = OV5647_WINDOW_WIDTH_DEF; > crop->height = OV5647_WINDOW_HEIGHT_DEF; > > - format->code = MEDIA_BUS_FMT_SBGGR8_1X8; > - format->width = 640; > - format->height = 480; > - format->field = V4L2_FIELD_NONE; > - format->colorspace = V4L2_COLORSPACE_SRGB; > + *format = OV5647_DEFAULT_FORMAT; > > return 0; > } > @@ -817,6 +855,8 @@ static int ov5647_probe(struct i2c_client *client) > > mutex_init(&sensor->lock); > > + sensor->mode = OV5647_DEFAULT_MODE; > + > ret = ov5647_init_controls(sensor); > if (ret) > goto mutex_destroy; -- Regards, Sakari Ailus
Hi Jacopo, On Mon, Nov 09, 2020 at 05:49:34PM +0100, Jacopo Mondi wrote: > Capturing in 640x480 SBGGR8_1X8 hangs the system when capturing > with the unicam driver on RaspberryPi 4 platform. > > Remove it. Is this somehow a property of the sensor driver? Is the sensor driver in use somewhere else where this configuration could be useful? Can other 8 bpp configurations captured with the Unicam driver? -- Regards, Sakari Ailus
Hi Jacopo, On Mon, Nov 09, 2020 at 05:49:05PM +0100, Jacopo Mondi wrote: > Third iteration following: > v2: https://patchwork.linuxtv.org/project/linux-media/list/?series=3782 > v1: https://patchwork.linuxtv.org/project/linux-media/list/?series=2765 Thanks for the update. This set really improves the driver in many ways. I had a few small comments on specific patches. Also, in general, please try to maintain coding style, 80 characters per line in particular --- unless there are tangible reasons to do otherwise. -- Kind regards, Sakari Ailus
Hi Sakari, On Wed, Nov 18, 2020 at 10:48:13PM +0200, Sakari Ailus wrote: > Hi Jacopo, > > On Mon, Nov 09, 2020 at 05:49:34PM +0100, Jacopo Mondi wrote: > > Capturing in 640x480 SBGGR8_1X8 hangs the system when capturing > > with the unicam driver on RaspberryPi 4 platform. > > > > Remove it. > > Is this somehow a property of the sensor driver? Is the sensor driver in More than a property I would say a failure of the sensor driver :) > use somewhere else where this configuration could be useful? Can other 8 I'm not sure in which other platforms the 8bpp mode has been tested with > bpp configurations captured with the Unicam driver? But unicam can capture 8bpp modes with imx219 (just tested) and imx477 according to Dave > > -- > Regards, > > Sakari Ailus
Third iteration following: v2: https://patchwork.linuxtv.org/project/linux-media/list/?series=3782 v1: https://patchwork.linuxtv.org/project/linux-media/list/?series=2765 Major changes are: - Address Dave's comments - Remove superseded patch: "media: ov5647: Program mode only if it has changed" - Remove Raw Bayer pattern from mode names - Remove cosmetic re-flow of register-value tables - Address Hans' comment: - Add support for V4L2_SEL_TGT_CROP_BOUNDS - Redefine the CROP rectangle relatively to the native pixel array as per "[PATCH 4/4] media: i2c: imx219: Selection compliance fixes" - New patches: - media: ov5647: Support VIDIOC_SUBSCRIBE_EVENT Fix v4l2-compliance error: fail: v4l2-test-controls.cpp(823): subscribe event for control 'User Controls' failed - media: ov5647: Remove 640x480 SBGGR8 mode The 640x480 8-bit mode is broken, at least when capturing on RaspberryPi4. As the receiver works when capturing in 8-bit mode with other sensors, it might be legit to think the mode is broken. As I've no way to fix it, it's probably better to remove it completely. v4l2-compliance output: ------------------------------------------------------------------------------- v4l2-compliance 1.21.0-4679, 32 bits, 32-bit time_t v4l2-compliance SHA: 225c6c2a17be 2020-10-30 15:13:07 Compliance test for device /dev/v4l-subdev0: Required ioctls: Allow for multiple opens: test second /dev/v4l-subdev0 open: OK test for unlimited opens: OK test invalid ioctls: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 12 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for device /dev/v4l-subdev0: 41, Succeeded: 41, Failed: 0, Warnings: 0 ------------------------------------------------------------------------------- Thanks j Dave Stevenson (8): media: ov5647: Add support for PWDN GPIO. media: ov5647: Add support for non-continuous clock mode media: ov5647: Add set_fmt and get_fmt calls. media: ov5647: Add support for get_selection() media: ov5647: Set V4L2_SUBDEV_FL_HAS_EVENTS flag media: ov5647: Support V4L2_CID_PIXEL_RATE media: ov5647: Support V4L2_CID_VBLANK control media: ov5647: Advertise the correct exposure range David Plowman (1): media: ov5647: Support gain, exposure and AWB controls Jacopo Mondi (20): media: ov5647: Fix format initialization media: ov5647: Fix style issues media: ov5647: Replace license with SPDX identifier media: ov5647: Fix return value from read/write media: ov5647: Program mode at s_stream(1) time media: ov5647: Implement enum_frame_size() media: ov5647: Protect s_stream() with mutex media: ov5647: Rationalize driver structure name media: ov5647: Break out format handling media: ov5647: Rename SBGGR8 VGA mode media: ov5647: Add SGGBR10_1X10 modes media: ov5647: Use SBGGR10_1X10 640x480 as default media: ov5647: Implement set_fmt pad operation media: ov5647: Support V4L2_CID_HBLANK control media: ov5647: Use pm_runtime infrastructure media: ov5647: Rework s_stream() operation media: ov5647: Apply controls only when powered media: ov5647: Constify oe_enable/disable reglist media: ov5647: Support VIDIOC_SUBSCRIBE_EVENT media: ov5647: Remove 640x480 SBGGR8 mode drivers/media/i2c/ov5647.c | 1290 ++++++++++++++++++++++++++++++------ 1 file changed, 1079 insertions(+), 211 deletions(-) -- 2.29.1