| Message ID | 20221005190613.394277-2-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ar0521: Add analog gain, rework clock tree | expand |
Hi Jacopo On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Implement the enum_frame_size pad operation. > > The sensor supports a continuous size range of resolutions. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks reasonable based on the driver. Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > index c7bdfc69b9be..89f3c01f18ce 100644 > --- a/drivers/media/i2c/ar0521.c > +++ b/drivers/media/i2c/ar0521.c > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, > return 0; > } > > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->index) > + return -EINVAL; > + > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) > + return -EINVAL; > + > + fse->min_width = AR0521_WIDTH_MIN; > + fse->max_width = AR0521_WIDTH_MAX; > + fse->min_height = AR0521_HEIGHT_MIN; > + fse->max_height = AR0521_HEIGHT_MAX; > + > + return 0; > +} > + > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) > { > struct ar0521_dev *sensor = to_ar0521_dev(sd); > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { > > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { > .enum_mbus_code = ar0521_enum_mbus_code, > + .enum_frame_size = ar0521_enum_frame_size, > .get_fmt = ar0521_get_fmt, > .set_fmt = ar0521_set_fmt, > }; > -- > 2.37.3 >
Hi Jacopo, Thank you for the patch. On Wed, Oct 05, 2022 at 09:06:04PM +0200, Jacopo Mondi wrote: > Implement the enum_frame_size pad operation. > > The sensor supports a continuous size range of resolutions. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > index c7bdfc69b9be..89f3c01f18ce 100644 > --- a/drivers/media/i2c/ar0521.c > +++ b/drivers/media/i2c/ar0521.c > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, > return 0; > } > > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->index) > + return -EINVAL; > + > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) > + return -EINVAL; > + > + fse->min_width = AR0521_WIDTH_MIN; > + fse->max_width = AR0521_WIDTH_MAX; > + fse->min_height = AR0521_HEIGHT_MIN; > + fse->max_height = AR0521_HEIGHT_MAX; This matches the driver implementation of .set_fmt(), but that's because the driver is *really* wrong :-( It uses the format to configure the crop rectangle, which is not right. This needs to be fixed. > + > + return 0; > +} > + > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) > { > struct ar0521_dev *sensor = to_ar0521_dev(sd); > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { > > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { > .enum_mbus_code = ar0521_enum_mbus_code, > + .enum_frame_size = ar0521_enum_frame_size, > .get_fmt = ar0521_get_fmt, > .set_fmt = ar0521_set_fmt, > };
Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > This matches the driver implementation of .set_fmt(), but that's because > the driver is *really* wrong :-( It uses the format to configure the > crop rectangle, which is not right. This needs to be fixed. How should it work?
Hi Laurent On Thu, Oct 06, 2022 at 07:33:45PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Oct 05, 2022 at 09:06:04PM +0200, Jacopo Mondi wrote: > > Implement the enum_frame_size pad operation. > > > > The sensor supports a continuous size range of resolutions. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > index c7bdfc69b9be..89f3c01f18ce 100644 > > --- a/drivers/media/i2c/ar0521.c > > +++ b/drivers/media/i2c/ar0521.c > > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, > > return 0; > > } > > > > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + if (fse->index) > > + return -EINVAL; > > + > > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) > > + return -EINVAL; > > + > > + fse->min_width = AR0521_WIDTH_MIN; > > + fse->max_width = AR0521_WIDTH_MAX; > > + fse->min_height = AR0521_HEIGHT_MIN; > > + fse->max_height = AR0521_HEIGHT_MAX; > > This matches the driver implementation of .set_fmt(), but that's because > the driver is *really* wrong :-( It uses the format to configure the > crop rectangle, which is not right. This needs to be fixed. > As far as I understand it, the driver supports smaller resolutions by cropping only, while the sensor would be actually capable of binning. As the driver currently only crops, the analog rectangle always matches the output size hence adding s_selection(CROP) just to replicate what s_ftm() does feels a little dumb ? I concur that ideally the driver should be capable of producing smaller resolution by binning, and in that case being able to configure the analog crop rectangle would be useful. But as long as it doesn't... > > + > > + return 0; > > +} > > + > > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) > > { > > struct ar0521_dev *sensor = to_ar0521_dev(sd); > > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { > > > > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { > > .enum_mbus_code = ar0521_enum_mbus_code, > > + .enum_frame_size = ar0521_enum_frame_size, > > .get_fmt = ar0521_get_fmt, > > .set_fmt = ar0521_set_fmt, > > }; > > -- > Regards, > > Laurent Pinchart
Hi Krzysztof, On Fri, Oct 07, 2022 at 06:57:19AM +0200, Krzysztof Hałasa wrote: > Laurent Pinchart writes: > > > This matches the driver implementation of .set_fmt(), but that's because > > the driver is *really* wrong :-( It uses the format to configure the > > crop rectangle, which is not right. This needs to be fixed. > > How should it work? The crop rectangle should be configured using .set_selection() ([1]). Following the V4L2 subdev API to the latter, the pad format exposed by the subdev should then be identical to the crop rectangle size. Many sensor drivers however use .set_fmt() to configure binning or skipping (after cropping), which in theory should be done by exposing a second subdev (the CCS driver does that for instance, to my knowledge it's the only sensor driver compliant with [1]). This is an area that requires improvements in the API, the topic was on the agenda of the media summit we held at the ELC-E conference a few weeks ago. [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/dev-subdev.html#types-of-selection-targets
Hi Jacopo, On Fri, Oct 07, 2022 at 09:29:59AM +0200, Jacopo Mondi wrote: > On Thu, Oct 06, 2022 at 07:33:45PM +0300, Laurent Pinchart wrote: > > On Wed, Oct 05, 2022 at 09:06:04PM +0200, Jacopo Mondi wrote: > > > Implement the enum_frame_size pad operation. > > > > > > The sensor supports a continuous size range of resolutions. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > index c7bdfc69b9be..89f3c01f18ce 100644 > > > --- a/drivers/media/i2c/ar0521.c > > > +++ b/drivers/media/i2c/ar0521.c > > > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, > > > return 0; > > > } > > > > > > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *sd_state, > > > + struct v4l2_subdev_frame_size_enum *fse) > > > +{ > > > + if (fse->index) > > > + return -EINVAL; > > > + > > > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) > > > + return -EINVAL; > > > + > > > + fse->min_width = AR0521_WIDTH_MIN; > > > + fse->max_width = AR0521_WIDTH_MAX; > > > + fse->min_height = AR0521_HEIGHT_MIN; > > > + fse->max_height = AR0521_HEIGHT_MAX; > > > > This matches the driver implementation of .set_fmt(), but that's because > > the driver is *really* wrong :-( It uses the format to configure the > > crop rectangle, which is not right. This needs to be fixed. > > As far as I understand it, the driver supports smaller resolutions by > cropping only, while the sensor would be actually capable of binning. > > As the driver currently only crops, the analog rectangle always matches the > output size hence adding s_selection(CROP) just to replicate what > s_ftm() does feels a little dumb ? > > I concur that ideally the driver should be capable of producing > smaller resolution by binning, and in that case being able to > configure the analog crop rectangle would be useful. But as long as it > doesn't... We have enough issues with sensor drivers implementing binning or skipping in different ways to not make it worse by implementing cropping in a creative way as wall :-) The first step is to fix the driver to implement the selection API. Then binning and skipping can be implemented on top, if anyone becomes interested in them. > > > + > > > + return 0; > > > +} > > > + > > > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) > > > { > > > struct ar0521_dev *sensor = to_ar0521_dev(sd); > > > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { > > > > > > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { > > > .enum_mbus_code = ar0521_enum_mbus_code, > > > + .enum_frame_size = ar0521_enum_frame_size, > > > .get_fmt = ar0521_get_fmt, > > > .set_fmt = ar0521_set_fmt, > > > };
Hi Laurent and Jacopo. On Fri, 7 Oct 2022 at 09:11, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jacopo, > > On Fri, Oct 07, 2022 at 09:29:59AM +0200, Jacopo Mondi wrote: > > On Thu, Oct 06, 2022 at 07:33:45PM +0300, Laurent Pinchart wrote: > > > On Wed, Oct 05, 2022 at 09:06:04PM +0200, Jacopo Mondi wrote: > > > > Implement the enum_frame_size pad operation. > > > > > > > > The sensor supports a continuous size range of resolutions. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ > > > > 1 file changed, 19 insertions(+) > > > > > > > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c > > > > index c7bdfc69b9be..89f3c01f18ce 100644 > > > > --- a/drivers/media/i2c/ar0521.c > > > > +++ b/drivers/media/i2c/ar0521.c > > > > @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, > > > > return 0; > > > > } > > > > > > > > +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *sd_state, > > > > + struct v4l2_subdev_frame_size_enum *fse) > > > > +{ > > > > + if (fse->index) > > > > + return -EINVAL; > > > > + > > > > + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) > > > > + return -EINVAL; > > > > + > > > > + fse->min_width = AR0521_WIDTH_MIN; > > > > + fse->max_width = AR0521_WIDTH_MAX; > > > > + fse->min_height = AR0521_HEIGHT_MIN; > > > > + fse->max_height = AR0521_HEIGHT_MAX; > > > > > > This matches the driver implementation of .set_fmt(), but that's because > > > the driver is *really* wrong :-( It uses the format to configure the > > > crop rectangle, which is not right. This needs to be fixed. > > > > As far as I understand it, the driver supports smaller resolutions by > > cropping only, while the sensor would be actually capable of binning. > > > > As the driver currently only crops, the analog rectangle always matches the > > output size hence adding s_selection(CROP) just to replicate what > > s_ftm() does feels a little dumb ? > > > > I concur that ideally the driver should be capable of producing > > smaller resolution by binning, and in that case being able to > > configure the analog crop rectangle would be useful. But as long as it > > doesn't... > > We have enough issues with sensor drivers implementing binning or > skipping in different ways to not make it worse by implementing cropping > in a creative way as wall :-) > > The first step is to fix the driver to implement the selection API. Then > binning and skipping can be implemented on top, if anyone becomes > interested in them. The datasheet and register reference have a fair number of references to SMIA standards. I wonder if the CCS driver can take over from this driver entirely.... Dave > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) > > > > { > > > > struct ar0521_dev *sensor = to_ar0521_dev(sd); > > > > @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { > > > > > > > > static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { > > > > .enum_mbus_code = ar0521_enum_mbus_code, > > > > + .enum_frame_size = ar0521_enum_frame_size, > > > > .get_fmt = ar0521_get_fmt, > > > > .set_fmt = ar0521_set_fmt, > > > > }; > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Fri, Oct 07, 2022 at 11:32:27AM +0100, Dave Stevenson wrote: > The datasheet and register reference have a fair number of references > to SMIA standards. I wonder if the CCS driver can take over from this > driver entirely.... A lot of the configuration of basic features in the driver appears to be going to CCS MSR space. While it's possible to do additional writes to the MSR register space based on standard CCS register writes, semantics still needs to match. It is possible to support many sensors with additional CCS static data that provides the limit and capability values, too, but to me this sensor doesn't appear like an obvious candidate for that.
On Fri, Oct 07, 2022 at 03:05:22PM +0300, Sakari Ailus wrote: > Hi Dave, > > On Fri, Oct 07, 2022 at 11:32:27AM +0100, Dave Stevenson wrote: > > The datasheet and register reference have a fair number of references > > to SMIA standards. I wonder if the CCS driver can take over from this > > driver entirely.... > > A lot of the configuration of basic features in the driver appears to be > going to CCS MSR space. While it's possible to do additional writes to the > MSR register space based on standard CCS register writes, semantics still > needs to match. > > It is possible to support many sensors with additional CCS static data that > provides the limit and capability values, too, but to me this sensor > doesn't appear like an obvious candidate for that. There are also lots of registers in the standard CCS space that don't match the CCS specification. I think a custom driver is needed.
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c index c7bdfc69b9be..89f3c01f18ce 100644 --- a/drivers/media/i2c/ar0521.c +++ b/drivers/media/i2c/ar0521.c @@ -798,6 +798,24 @@ static int ar0521_enum_mbus_code(struct v4l2_subdev *sd, return 0; } +static int ar0521_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->index) + return -EINVAL; + + if (fse->code != MEDIA_BUS_FMT_SGRBG8_1X8) + return -EINVAL; + + fse->min_width = AR0521_WIDTH_MIN; + fse->max_width = AR0521_WIDTH_MAX; + fse->min_height = AR0521_HEIGHT_MIN; + fse->max_height = AR0521_HEIGHT_MAX; + + return 0; +} + static int ar0521_pre_streamon(struct v4l2_subdev *sd, u32 flags) { struct ar0521_dev *sensor = to_ar0521_dev(sd); @@ -864,6 +882,7 @@ static const struct v4l2_subdev_video_ops ar0521_video_ops = { static const struct v4l2_subdev_pad_ops ar0521_pad_ops = { .enum_mbus_code = ar0521_enum_mbus_code, + .enum_frame_size = ar0521_enum_frame_size, .get_fmt = ar0521_get_fmt, .set_fmt = ar0521_set_fmt, };
Implement the enum_frame_size pad operation. The sensor supports a continuous size range of resolutions. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ar0521.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)