Re: [PATCH v11 1/2] media: pci: intel: ivsc: Add CSI submodule

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Aug 04, 2023 at 06:20:36AM +0000, Sakari Ailus wrote:
> On Fri, Aug 04, 2023 at 01:08:40AM +0300, Laurent Pinchart wrote:
> > > > > +static int mei_csi_set_fmt(struct v4l2_subdev *sd,
> > > > > +			   struct v4l2_subdev_state *sd_state,
> > > > > +			   struct v4l2_subdev_format *format)
> > > > > +{
> > > > > +	struct v4l2_mbus_framefmt *source_mbusformat;
> > > > > +	struct v4l2_mbus_framefmt *mbusformat;
> > > > > +	struct mei_csi *csi = sd_to_csi(sd);
> > > > > +	struct media_pad *pad;
> > > > > +
> > > > > +	mbusformat = mei_csi_get_pad_format(sd, sd_state, format->pad,
> > > > > +					    format->which);
> > > > > +	if (!mbusformat)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	source_mbusformat = mei_csi_get_pad_format(sd, sd_state, CSI_PAD_SOURCE,
> > > > > +						   format->which);
> > > > > +	if (!source_mbusformat)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	v4l_bound_align_image(&format->format.width, 1, 65536, 0,
> > > > > +			      &format->format.height, 1, 65536, 0, 0);
> > > > > +
> > > > > +	switch (format->format.code) {
> > > > > +	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;
> > > > > +	default:
> > > > > +		format->format.code = MEDIA_BUS_FMT_Y8_1X8;
> > > > > +		break;
> > > > > +	}
> > > > 
> > > > I wonder if we should simply accept all formats. The IVSC doesn't seem
> > > > to cara.
> > > 
> > > The video mux needs something similar. I was thinking of adding a generic
> > > helper for such functions, perhaps with flags to suggest which formats to
> > > accept.
> > 
> > What would be the drawbacks of accepting all formats ?
> 
> Also the ones that aren't defined at the moment?
> 
> They're not valid, although in practice there might not be issues, as they
> are compared across links in any case.

That was my reasoning too.

> I guess there should also be support for enum_mbus_codes, and for that we
> need a list of some sort.

Hmmm... If a subdev really implements pass-through for video data, I
wonder if we could lift the requirement of implementing format
enumeration.

-- 
Regards,

Laurent Pinchart