media: i2c: vgxy61: Report stream using frame descriptors

[PATCH] media: i2c: vgxy61: Report stream using frame descriptors

Posted by Julien Massot 3 months ago

Add support for .get_frame_desc() to report CSI-2 virtual channel
and data type information. This allows CSI-2 receivers to properly
interpret the stream without inferring the data type from the pixel
format.

Signed-off-by: Julien Massot <julien.massot@collabora.com>
---
 drivers/media/i2c/vgxy61.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
index 5b0479f3a3c0592be430cefe5a1ab9a76812ba84..44d6c8d8fbf8d6182e42d44e129bc45945ee0da5 100644
--- a/drivers/media/i2c/vgxy61.c
+++ b/drivers/media/i2c/vgxy61.c
@@ -1181,6 +1181,21 @@ static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int vgxy61_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *fd)
+{
+	struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+	fd->num_entries = 1;
+	fd->entry[0].pixelcode = sensor->fmt.code;
+	fd->entry[0].stream = 0;
+	fd->entry[0].bus.csi2.vc = 0;
+	fd->entry[0].bus.csi2.dt = get_data_type_by_code(sensor->fmt.code);
+
+	return 0;
+}
+
 static int vgxy61_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
@@ -1402,6 +1417,7 @@ static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = {
 	.set_fmt = vgxy61_set_fmt,
 	.get_selection = vgxy61_get_selection,
 	.enum_frame_size = vgxy61_enum_frame_size,
+	.get_frame_desc = vgxy61_get_frame_desc,
 };
 
 static const struct v4l2_subdev_ops vgxy61_subdev_ops = {

---
base-commit: 1343433ed38923a21425c602e92120a1f1db5f7a
change-id: 20250704-vgxy61-frame-desc-2a6d3c6cab43

Best regards,
-- 
Julien Massot <julien.massot@collabora.com>

Re: [PATCH] media: i2c: vgxy61: Report stream using frame descriptors

Posted by Sakari Ailus 1 month, 3 weeks ago

Hi Julien,

On Fri, Jul 04, 2025 at 11:28:24AM +0200, Julien Massot wrote:
> Add support for .get_frame_desc() to report CSI-2 virtual channel
> and data type information. This allows CSI-2 receivers to properly
> interpret the stream without inferring the data type from the pixel
> format.
> 
> Signed-off-by: Julien Massot <julien.massot@collabora.com>
> ---
>  drivers/media/i2c/vgxy61.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
> index 5b0479f3a3c0592be430cefe5a1ab9a76812ba84..44d6c8d8fbf8d6182e42d44e129bc45945ee0da5 100644
> --- a/drivers/media/i2c/vgxy61.c
> +++ b/drivers/media/i2c/vgxy61.c
> @@ -1181,6 +1181,21 @@ static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int vgxy61_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +	fd->entry[0].pixelcode = sensor->fmt.code;
> +	fd->entry[0].stream = 0;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = get_data_type_by_code(sensor->fmt.code);
> +
> +	return 0;
> +}
> +
>  static int vgxy61_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *sd_state,
>  			  struct v4l2_subdev_format *format)
> @@ -1402,6 +1417,7 @@ static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = {
>  	.set_fmt = vgxy61_set_fmt,
>  	.get_selection = vgxy61_get_selection,
>  	.enum_frame_size = vgxy61_enum_frame_size,
> +	.get_frame_desc = vgxy61_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops vgxy61_subdev_ops = {
> 

I guess this is correct as such, but does it provide any information that
isn't already available? I.e. I wouldn't add it just for the sake of it.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: vgxy61: Report stream using frame descriptors

Posted by Julien Massot 1 month, 1 week ago

Hi Sakari,

On Fri, 2025-08-15 at 06:04 +0000, Sakari Ailus wrote:
> Hi Julien,
> 
> On Fri, Jul 04, 2025 at 11:28:24AM +0200, Julien Massot wrote:
> > Add support for .get_frame_desc() to report CSI-2 virtual channel
> > and data type information. This allows CSI-2 receivers to properly
> > interpret the stream without inferring the data type from the pixel
> > format.
> > 
> > Signed-off-by: Julien Massot <julien.massot@collabora.com>
> > ---
> >  drivers/media/i2c/vgxy61.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
> > index
> > 5b0479f3a3c0592be430cefe5a1ab9a76812ba84..44d6c8d8fbf8d6182e42d44e129b
> > c45945ee0da5 100644
> > --- a/drivers/media/i2c/vgxy61.c
> > +++ b/drivers/media/i2c/vgxy61.c
> > @@ -1181,6 +1181,21 @@ static int vgxy61_s_stream(struct v4l2_subdev
> > *sd, int enable)
> >  	return ret;
> >  }
> >  
> > +static int vgxy61_get_frame_desc(struct v4l2_subdev *sd, unsigned int
> > pad,
> > +				 struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> > +
> > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > +	fd->num_entries = 1;
> > +	fd->entry[0].pixelcode = sensor->fmt.code;
> > +	fd->entry[0].stream = 0;
> > +	fd->entry[0].bus.csi2.vc = 0;
> > +	fd->entry[0].bus.csi2.dt = get_data_type_by_code(sensor-
> > >fmt.code);
> > +
> > +	return 0;
> > +}
> > +
> >  static int vgxy61_set_fmt(struct v4l2_subdev *sd,
> >  			  struct v4l2_subdev_state *sd_state,
> >  			  struct v4l2_subdev_format *format)
> > @@ -1402,6 +1417,7 @@ static const struct v4l2_subdev_pad_ops
> > vgxy61_pad_ops = {
> >  	.set_fmt = vgxy61_set_fmt,
> >  	.get_selection = vgxy61_get_selection,
> >  	.enum_frame_size = vgxy61_enum_frame_size,
> > +	.get_frame_desc = vgxy61_get_frame_desc,
> >  };
> >  
> >  static const struct v4l2_subdev_ops vgxy61_subdev_ops = {
> > 
> 
> I guess this is correct as such, but does it provide any information
> that
> isn't already available? I.e. I wouldn't add it just for the sake of it.
Perhaps, I missed something. Without the get_frame_desc the CSI receiver
have to infers the datatype based on the pixel format.

I made this patch because some CSI receivers, such as the upcoming GMSL2
framework are relying on the frame desc to update its routing table.
So, it will only support sensors implementing the get_frame_desc function.

Regards,
Julien

Re: [PATCH] media: i2c: vgxy61: Report stream using frame descriptors

Posted by Sakari Ailus 1 month, 1 week ago

Hi Julien,

On Tue, Aug 26, 2025 at 09:21:29AM +0200, Julien Massot wrote:
> Hi Sakari,
> 
> On Fri, 2025-08-15 at 06:04 +0000, Sakari Ailus wrote:
> > Hi Julien,
> > 
> > On Fri, Jul 04, 2025 at 11:28:24AM +0200, Julien Massot wrote:
> > > Add support for .get_frame_desc() to report CSI-2 virtual channel
> > > and data type information. This allows CSI-2 receivers to properly
> > > interpret the stream without inferring the data type from the pixel
> > > format.
> > > 
> > > Signed-off-by: Julien Massot <julien.massot@collabora.com>
> > > ---
> > >  drivers/media/i2c/vgxy61.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
> > > index
> > > 5b0479f3a3c0592be430cefe5a1ab9a76812ba84..44d6c8d8fbf8d6182e42d44e129b
> > > c45945ee0da5 100644
> > > --- a/drivers/media/i2c/vgxy61.c
> > > +++ b/drivers/media/i2c/vgxy61.c
> > > @@ -1181,6 +1181,21 @@ static int vgxy61_s_stream(struct v4l2_subdev
> > > *sd, int enable)
> > >  	return ret;
> > >  }
> > >  
> > > +static int vgxy61_get_frame_desc(struct v4l2_subdev *sd, unsigned int
> > > pad,
> > > +				 struct v4l2_mbus_frame_desc *fd)
> > > +{
> > > +	struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> > > +
> > > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > > +	fd->num_entries = 1;
> > > +	fd->entry[0].pixelcode = sensor->fmt.code;
> > > +	fd->entry[0].stream = 0;
> > > +	fd->entry[0].bus.csi2.vc = 0;
> > > +	fd->entry[0].bus.csi2.dt = get_data_type_by_code(sensor-
> > > >fmt.code);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int vgxy61_set_fmt(struct v4l2_subdev *sd,
> > >  			  struct v4l2_subdev_state *sd_state,
> > >  			  struct v4l2_subdev_format *format)
> > > @@ -1402,6 +1417,7 @@ static const struct v4l2_subdev_pad_ops
> > > vgxy61_pad_ops = {
> > >  	.set_fmt = vgxy61_set_fmt,
> > >  	.get_selection = vgxy61_get_selection,
> > >  	.enum_frame_size = vgxy61_enum_frame_size,
> > > +	.get_frame_desc = vgxy61_get_frame_desc,
> > >  };
> > >  
> > >  static const struct v4l2_subdev_ops vgxy61_subdev_ops = {
> > > 
> > 
> > I guess this is correct as such, but does it provide any information
> > that
> > isn't already available? I.e. I wouldn't add it just for the sake of it.
> Perhaps, I missed something. Without the get_frame_desc the CSI receiver
> have to infers the datatype based on the pixel format.

I think we should have a function in the framework to do that. Oh, we
actually already do have the function, call_get_mbus_config(), it just
doesn't get the format yet. Currently receiver drivers handle this by
themselves but that doesn't scale very well.

> 
> I made this patch because some CSI receivers, such as the upcoming GMSL2
> framework are relying on the frame desc to update its routing table.
> So, it will only support sensors implementing the get_frame_desc function.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: vgxy61: Report stream using frame descriptors

Posted by Benjamin Mugnier 2 months, 2 weeks ago

Hi Julien,

Thank you for your patch.

On 7/4/25 11:28, Julien Massot wrote:
> Add support for .get_frame_desc() to report CSI-2 virtual channel
> and data type information. This allows CSI-2 receivers to properly
> interpret the stream without inferring the data type from the pixel
> format.
> 
> Signed-off-by: Julien Massot <julien.massot@collabora.com>

Reviewed-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

> ---
>  drivers/media/i2c/vgxy61.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/i2c/vgxy61.c b/drivers/media/i2c/vgxy61.c
> index 5b0479f3a3c0592be430cefe5a1ab9a76812ba84..44d6c8d8fbf8d6182e42d44e129bc45945ee0da5 100644
> --- a/drivers/media/i2c/vgxy61.c
> +++ b/drivers/media/i2c/vgxy61.c
> @@ -1181,6 +1181,21 @@ static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int vgxy61_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +	fd->entry[0].pixelcode = sensor->fmt.code;
> +	fd->entry[0].stream = 0;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = get_data_type_by_code(sensor->fmt.code);
> +
> +	return 0;
> +}
> +
>  static int vgxy61_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *sd_state,
>  			  struct v4l2_subdev_format *format)
> @@ -1402,6 +1417,7 @@ static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = {
>  	.set_fmt = vgxy61_set_fmt,
>  	.get_selection = vgxy61_get_selection,
>  	.enum_frame_size = vgxy61_enum_frame_size,
> +	.get_frame_desc = vgxy61_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops vgxy61_subdev_ops = {
> 
> ---
> base-commit: 1343433ed38923a21425c602e92120a1f1db5f7a
> change-id: 20250704-vgxy61-frame-desc-2a6d3c6cab43
> 
> Best regards,

-- 
Regards,
Benjamin