diff mbox series

[01/10] media: ar0521: Implement enum_frame_sizes

Message ID 20221005190613.394277-2-jacopo@jmondi.org (mailing list archive)
State Changes Requested
Headers
Series media: ar0521: Add analog gain, rework clock tree |

Commit Message

Jacopo Mondi Oct. 5, 2022, 7:06 p.m. UTC 
  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(+)

Comments

Dave Stevenson Oct. 6, 2022, 2:37 p.m. UTC | #1 
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
>
Laurent Pinchart Oct. 6, 2022, 4:33 p.m. UTC | #2 
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,
>  };
Krzysztof Hałasa Oct. 7, 2022, 4:57 a.m. UTC | #3 
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?
Jacopo Mondi Oct. 7, 2022, 7:29 a.m. UTC | #4 
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
Laurent Pinchart Oct. 7, 2022, 8:08 a.m. UTC | #5 
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
Laurent Pinchart Oct. 7, 2022, 8:11 a.m. UTC | #6 
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,
> > >  };
Dave Stevenson Oct. 7, 2022, 10:32 a.m. UTC | #7 
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
Sakari Ailus Oct. 7, 2022, 12:05 p.m. UTC | #8 
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.
Laurent Pinchart Oct. 7, 2022, 12:12 p.m. UTC | #9 
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 mbox series

Patch

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,
 };