Re: [PATCH 1/2] media: i2c: cvs: Add driver of Intel Computer Vision Sensing Controller(CVS)

["Vadillo, Miguel" <miguel.vadillo@intel.com>]

Hi Kieran,

On 5/6/26 1:38 AM, Kieran Bingham wrote:
> Hi Miguel,
> 
> 
> 
> Quoting Miguel Vadillo (2026-05-05 23:30:04)
>> Add driver for Intel Computer Vision Sensing (CVS) devices found on
>> Intel Luna Lake (LNL), Panther Lake (PTL), and Arrow Lake (ARL)
>> platforms.
> 
> I haven't heard of the CVS component yet.
> 
> The main thing I can determine is that it controls the privacy led for
> the cameras in the pipeline automatically if there is a stream active?

CVS is more than a privacy LED controller — it runs always-on Computer 
Vision Sensing algorithms (e.g., Human Presence Detection) directly on 
its own firmware, though that functionality is not exposed by this 
driver. Because CVS sits between the camera sensor and the IPU on the 
MIPI CSI-2 bus and continuously accesses sensor data, the driver's 
primary job is ownership arbitration: when the host wants to capture 
frames, CVS must relinquish the MIPI lanes. This patch series implements 
that handoff mechanism.
> 
> What else does the CVS do ? I see there are 'firmwares' to load to the
> devices, so it seems like a far more complex part that simply turning an
> LED on and off...

This patchset implements the communication protocol and handles the 
multiple hardware configurations currently deployed in the field. 
Without it, the host cannot acquire ownership of the MIPI lanes and 
therefore cannot access the camera sensor for image capture.

The CVS firmware is resident on the device — nothing needs to be loaded 
from the host side. Firmware update support will be addressed in a 
separate patch.

> 
> Trimming below as I'm not specifically reviewing the driver here, but
> reading this patch made me think about the other devices that are
> essentially a media-bus pass through device (like other video
> multiplexors).

Correct, other solution "similar" to this one is the 
iVSC:drivers/media/pci/intel/ivsc/ but the implementation is different 
enough to be able to extend that implementation.

--
regards.
Miguel
> 
> <snip>
> 
> 
>> +
>> +/**
>> + * cvs_csi_set_fmt - Negotiate pad format
>> + * @sd: Sub-device
>> + * @state: State
>> + * @format: Desired / returned format
>> + *
>> + * Mirrors sink format onto source pad. Accepts many media bus codes, falling
>> + * back to Y8 if unsupported. Normalizes field setting.
>> + *
>> + * Return: 0.
>> + */
>> +static int cvs_csi_set_fmt(struct v4l2_subdev *sd,
>> +                          struct v4l2_subdev_state *state,
>> +                          struct v4l2_subdev_format *format)
>> +{
>> +       struct v4l2_mbus_framefmt *src =
>> +               v4l2_subdev_state_get_format(state, ICVS_CSI_PAD_SOURCE);
>> +       struct v4l2_mbus_framefmt *sink =
>> +               v4l2_subdev_state_get_format(state, ICVS_CSI_PAD_SINK);
>> +
>> +       if (format->pad == ICVS_CSI_PAD_SOURCE) { /* source pad mirrors sink */
>> +               *src = *sink;
>> +               return 0;
>> +       }
>> +
>> +       v4l_bound_align_image(&format->format.width, 1, 65536, 0,
>> +                             &format->format.height, 1, 65536, 0, 0);
>> +
>> +       switch (format->format.code) {
>> +       /* Accept a large list; default fallback to Y8 */
>> +       case MEDIA_BUS_FMT_RGB444_1X12:
>> +       case MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE:
>> +       case MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE:
>> +       case MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE:
>> +       case MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE:
>> +       case MEDIA_BUS_FMT_RGB565_1X16:
>> +       case MEDIA_BUS_FMT_BGR565_2X8_BE:
>> +       case MEDIA_BUS_FMT_BGR565_2X8_LE:
>> +       case MEDIA_BUS_FMT_RGB565_2X8_BE:
>> +       case MEDIA_BUS_FMT_RGB565_2X8_LE:
>> +       case MEDIA_BUS_FMT_RGB666_1X18:
>> +       case MEDIA_BUS_FMT_RBG888_1X24:
>> +       case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
>> +       case MEDIA_BUS_FMT_BGR888_1X24:
>> +       case MEDIA_BUS_FMT_GBR888_1X24:
>> +       case MEDIA_BUS_FMT_RGB888_1X24:
>> +       case MEDIA_BUS_FMT_RGB888_2X12_BE:
>> +       case MEDIA_BUS_FMT_RGB888_2X12_LE:
>> +       case MEDIA_BUS_FMT_ARGB8888_1X32:
>> +       case MEDIA_BUS_FMT_RGB888_1X32_PADHI:
>> +       case MEDIA_BUS_FMT_RGB101010_1X30:
>> +       case MEDIA_BUS_FMT_RGB121212_1X36:
>> +       case MEDIA_BUS_FMT_RGB161616_1X48:
>> +       case MEDIA_BUS_FMT_Y8_1X8:
>> +       case MEDIA_BUS_FMT_UV8_1X8:
>> +       case MEDIA_BUS_FMT_UYVY8_1_5X8:
>> +       case MEDIA_BUS_FMT_VYUY8_1_5X8:
>> +       case MEDIA_BUS_FMT_YUYV8_1_5X8:
>> +       case MEDIA_BUS_FMT_YVYU8_1_5X8:
>> +       case MEDIA_BUS_FMT_UYVY8_2X8:
>> +       case MEDIA_BUS_FMT_VYUY8_2X8:
>> +       case MEDIA_BUS_FMT_YUYV8_2X8:
>> +       case MEDIA_BUS_FMT_YVYU8_2X8:
>> +       case MEDIA_BUS_FMT_Y10_1X10:
>> +       case MEDIA_BUS_FMT_UYVY10_2X10:
>> +       case MEDIA_BUS_FMT_VYUY10_2X10:
>> +       case MEDIA_BUS_FMT_YUYV10_2X10:
>> +       case MEDIA_BUS_FMT_YVYU10_2X10:
>> +       case MEDIA_BUS_FMT_Y12_1X12:
>> +       case MEDIA_BUS_FMT_UYVY12_2X12:
>> +       case MEDIA_BUS_FMT_VYUY12_2X12:
>> +       case MEDIA_BUS_FMT_YUYV12_2X12:
>> +       case MEDIA_BUS_FMT_YVYU12_2X12:
>> +       case MEDIA_BUS_FMT_UYVY8_1X16:
>> +       case MEDIA_BUS_FMT_VYUY8_1X16:
>> +       case MEDIA_BUS_FMT_YUYV8_1X16:
>> +       case MEDIA_BUS_FMT_YVYU8_1X16:
>> +       case MEDIA_BUS_FMT_YDYUYDYV8_1X16:
>> +       case MEDIA_BUS_FMT_UYVY10_1X20:
>> +       case MEDIA_BUS_FMT_VYUY10_1X20:
>> +       case MEDIA_BUS_FMT_YUYV10_1X20:
>> +       case MEDIA_BUS_FMT_YVYU10_1X20:
>> +       case MEDIA_BUS_FMT_VUY8_1X24:
>> +       case MEDIA_BUS_FMT_YUV8_1X24:
>> +       case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
>> +       case MEDIA_BUS_FMT_UYVY12_1X24:
>> +       case MEDIA_BUS_FMT_VYUY12_1X24:
>> +       case MEDIA_BUS_FMT_YUYV12_1X24:
>> +       case MEDIA_BUS_FMT_YVYU12_1X24:
>> +       case MEDIA_BUS_FMT_YUV10_1X30:
>> +       case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
>> +       case MEDIA_BUS_FMT_AYUV8_1X32:
>> +       case MEDIA_BUS_FMT_UYYVYY12_0_5X36:
>> +       case MEDIA_BUS_FMT_YUV12_1X36:
>> +       case MEDIA_BUS_FMT_YUV16_1X48:
>> +       case MEDIA_BUS_FMT_UYYVYY16_0_5X48:
>> +       case MEDIA_BUS_FMT_JPEG_1X8:
>> +       case MEDIA_BUS_FMT_AHSV8888_1X32:
>> +       case MEDIA_BUS_FMT_SBGGR8_1X8:
>> +       case MEDIA_BUS_FMT_SGBRG8_1X8:
>> +       case MEDIA_BUS_FMT_SGRBG8_1X8:
>> +       case MEDIA_BUS_FMT_SRGGB8_1X8:
>> +       case MEDIA_BUS_FMT_SBGGR10_1X10:
>> +       case MEDIA_BUS_FMT_SGBRG10_1X10:
>> +       case MEDIA_BUS_FMT_SGRBG10_1X10:
>> +       case MEDIA_BUS_FMT_SRGGB10_1X10:
>> +       case MEDIA_BUS_FMT_SBGGR12_1X12:
>> +       case MEDIA_BUS_FMT_SGBRG12_1X12:
>> +       case MEDIA_BUS_FMT_SGRBG12_1X12:
>> +       case MEDIA_BUS_FMT_SRGGB12_1X12:
>> +       case MEDIA_BUS_FMT_SBGGR14_1X14:
>> +       case MEDIA_BUS_FMT_SGBRG14_1X14:
>> +       case MEDIA_BUS_FMT_SGRBG14_1X14:
>> +       case MEDIA_BUS_FMT_SRGGB14_1X14:
>> +       case MEDIA_BUS_FMT_SBGGR16_1X16:
>> +       case MEDIA_BUS_FMT_SGBRG16_1X16:
>> +       case MEDIA_BUS_FMT_SGRBG16_1X16:
>> +       case MEDIA_BUS_FMT_SRGGB16_1X16:
>> +               break;
> 
> I've seen this on other pass through devices, and I wonder if really
> there's nothing to filter here - it could be anything. Or perhaps if we
> did need to ensure the driver only supported a specific bus type - that
> could be factored out.
> 
> 
>> +       default:
>> +               format->format.code = MEDIA_BUS_FMT_Y8_1X8;
>> +               break;
>> +       }
>> +
>> +       if (format->format.field == V4L2_FIELD_ANY)
>> +               format->format.field = V4L2_FIELD_NONE;
>> +
>> +       *sink = format->format;
>> +       *src = *sink;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * cvs_csi_get_mbus_config - Provide current CSI-2 bus configuration
>> + * @sd: Sub-device
>> + * @pad: Pad index
>> + * @cfg: Returned bus config
>> + *
>> + * Fills lane ordering and number of lanes; retrieves link frequency from
>> + * remote entity.
>> + *
>> + * Return: 0 on success or negative errno.
>> + */
>> +static int cvs_csi_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
>> +                                  struct v4l2_mbus_config *cfg)
>> +{
>> +       struct icvs *ctx = sd_to_csi(sd);
>> +       s64 freq;
>> +
>> +       cfg->type = V4L2_MBUS_CSI2_DPHY;
>> +       for (unsigned int i = 0; i < V4L2_MBUS_CSI2_MAX_DATA_LANES; i++)
>> +               cfg->bus.mipi_csi2.data_lanes[i] = i + 1;
>> +       cfg->bus.mipi_csi2.num_data_lanes = ctx->nr_of_lanes;
>> +
>> +       freq = v4l2_get_link_freq(ctx->remote, 0, 0);
>> +       if (freq < 0)
>> +               return -EINVAL;
>> +
>> +       ctx->link_freq = freq;
>> +       cfg->link_freq = freq;
>> +
>> +       return 0;
>> +}
> 
> So more of a question for linux-media community, I've seen this pattern
> a couple of times - should or could we consider a way to factor out a
> passthrough device which doesn't modify the stream(s) formats and is
> simply representing almost a connector type ?
> 
> --
> Kieran