Re: [PATCH] media: i2c: imx219: Implement .get_frame_desc()

Hi Mattijs,
Thanks for the patch.

On 13/08/26 20:41, Mattijs Korpershoek wrote:
> From: Vaishnav Achath <[email protected]>
> 
> The next subdev in the media graph may want to enquire information such
> as bus format, virtual channel, bus data type to route the stream from
> this sensor correctly.
> 
> Add support for sharing this information using the .get_frame_desc()
> callback.
> 
> Signed-off-by: Vaishnav Achath <[email protected]>
> Signed-off-by: Jai Luthra <[email protected]>
> Signed-off-by: Yemike Abhilash Chandra <[email protected]>
> Signed-off-by: Mattijs Korpershoek <[email protected]>
> ---

FYI, similar variant of this is already posted by Tomi recently [1].

On that patch, quoting Sakari [2]:

"I've been recently working on
<URL:https://lore.kernel.org/linux-media/[email protected]/>.
In other words, drivers that have a single stream don't need this. We could
probably extend that further by making use of the routing information but I
think that should be left for later."

I don't really know the status of that series. I will let Sakari to comment.

Thanks and Regards,
Yemike Abhilash Chandra


[1]: 
https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/#t


> This has been tested on top of linus/master based on commit
> 3d6d817622b0 ("Merge tag 'scsi-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi")
> 
> I used a AM69-SK with the Arducam FPD V3Link[1] using
> the following device tree overlays:
>    ti/k3-am68-sk-v3link-fusion.dtbo ti/k3-v3link-imx219-0-0.dtbo
> 
> See TI's documentation about this [2]
> 
> This is based on a patch [3] from TI's public vendor tree.
> 
> I've tried to polish the patch a bit with the following changes:
> * Use existing imx219_get_format_bpp() instead of open coding it
> * Use MIPI_CSI2_DT_RAW{,10} instead of magic numbers
> * Don't memset(*fd) since already handled by the core
> * Simplify frame_desc entries by removing fd->num_entries++
> * Add new imx219_get_data_type_by_code() helper and use it
> * Add error handling for v4l2_subdev_state_get_format()
> * Don't hard-code pad number (is always 0)
> * Remove 'ret' variable
> 
> [1] https://www.arducam.com/arducam-v3link-camera-kit-for-ti-development-boards.html
> [2] https://software-dl.ti.com/jacinto7/esd/processor-sdk-linux-am69/11_00_10_01/exports/docs/linux/Foundational_Components/Kernel/Kernel_Drivers/Camera/CSI2RX.html
> [3] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit?id=4268e58970c119e8dda8ad951f329d267eacc7a7&dt=2
> ---
>   drivers/media/i2c/imx219.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 223d3753cc93..fc237a2dba60 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -23,11 +23,13 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/regulator/consumer.h>
>   
> +#include <media/mipi-csi2.h>
>   #include <media/v4l2-cci.h>
>   #include <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-fwnode.h>
>   #include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
>   
>   /* Chip ID */
>   #define IMX219_REG_CHIP_ID		CCI_REG16(0x0000)
> @@ -429,6 +431,24 @@ static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)
>   	return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1;
>   }
>   
> +static u8 imx219_get_data_type_by_code(__u32 code)
> +{
> +	switch (code) {
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		return MIPI_CSI2_DT_RAW8;
> +
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	default:
> +		return MIPI_CSI2_DT_RAW10;
> +	}
> +}
> +
>   /* -----------------------------------------------------------------------------
>    * Controls
>    */
> @@ -539,6 +559,37 @@ static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
>   	return (imx219->lanes == 2) ? IMX219_PIXEL_RATE : IMX219_PIXEL_RATE_4LANE;
>   }
>   
> +static int imx219_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
> +	u32 bpp;
> +
> +	if (pad != 0)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +	if (!state)
> +		return -EINVAL;
> +
> +	format = v4l2_subdev_state_get_format(state, pad);
> +	bpp = imx219_get_format_bpp(format);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +	fd->entry[0].pixelcode = format->code;
> +	fd->entry[0].stream = 0;
> +	fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +	fd->entry[0].length = (format->width * format->height * bpp) / 8;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = imx219_get_data_type_by_code(format->code);
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	return 0;
> +}
> +
>   /* Initialize control handlers */
>   static int imx219_init_controls(struct imx219 *imx219)
>   {
> @@ -994,6 +1045,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
>   	.get_fmt = v4l2_subdev_get_fmt,
>   	.set_fmt = imx219_set_pad_format,
>   	.get_selection = imx219_get_selection,
> +	.get_frame_desc = imx219_get_frame_desc,
>   	.enum_frame_size = imx219_enum_frame_size,
>   	.enable_streams = imx219_enable_streams,
>   	.disable_streams = imx219_disable_streams,
> 
> ---
> base-commit: 3d6d817622b0a9721e3cc404df3469171582be13
> change-id: 20260813-imx219-frame-desc-35e9cbb004cd
> 
> Best regards,
> --
> Mattijs Korpershoek <[email protected]>
>