| Message ID | 20230222125630.421020-2-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] media: ti: cal: Add support for 1X16 mbus formats | expand |
Hi Tomi On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote: > For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 > link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and > used by many drivers, so add 1X16 support to CAL. > > We keep the 2X8 formats for backward compatibility. Eh, this is the usual question about what we should consider a to be a userspace breakage or not. I presume the reason to maintain 2X8 formats is (assuming the CAL interface is CSI-2 only, right ?) because sensor drivers that only support 2X8 formats will then fail to link_validate() if you remove all 2X8 formats here. I'm of the opinion that we should bite the bullet and move to 1X16. If any driver that used to work with CAL now breaks, the sensor driver will have to be fixed. In my humble experience, that's what we did with the ov5640 sensor. We removed the 2X8 formats in CSI-2 mode and some platform driver broke and then had been fixed (it happened in the same release cycle), win win. I know it's controversial, let's see what others think. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++- > drivers/media/platform/ti/cal/cal.c | 25 +++++++++++++++++++++++ > drivers/media/platform/ti/cal/cal.h | 1 + > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c > index 4eade409d5d3..87db8761d94d 100644 > --- a/drivers/media/platform/ti/cal/cal-video.c > +++ b/drivers/media/platform/ti/cal/cal-video.c > @@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void *priv, > if (f->mbus_code && cal_formats[i].code != f->mbus_code) > continue; > > + /* > + * Skip legacy formats so that we don't return duplicate fourccs, > + * except in the case that the user explicitly asked for an > + * mbus_code which matches this legacy format. > + */ > + if (cal_formats[i].legacy && > + (!f->mbus_code || cal_formats[i].code != f->mbus_code)) This only works as there are no duplicate codes in cal_formats[] (iow a pixel format is assigned to a single media bus code). If that's how the CAL interface works (no colorspace conversion, no components re-ordering) I guess it's fine > + continue; > + > if (idx == f->index) { > f->pixelformat = cal_formats[i].fourcc; > f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > @@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx, > > static int cal_video_check_format(struct cal_ctx *ctx) > { > + const struct cal_format_info *rx_fmtinfo; > const struct v4l2_mbus_framefmt *format; > struct media_pad *remote_pad; > > @@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx) > > format = &ctx->phy->formats[remote_pad->index]; > > - if (ctx->fmtinfo->code != format->code || > + rx_fmtinfo = cal_format_by_code(format->code); > + if (!rx_fmtinfo) > + return -EINVAL; > + > + if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc || Is this meant to make 2X8 and 1X16 formats compatible during link validation ? > ctx->v_fmt.fmt.pix.height != format->height || > ctx->v_fmt.fmt.pix.width != format->width || > ctx->v_fmt.fmt.pix.field != format->field) > diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c > index 1236215ec70e..053bf1030af0 100644 > --- a/drivers/media/platform/ti/cal/cal.c > +++ b/drivers/media/platform/ti/cal/cal.c > @@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API"); > */ > > const struct cal_format_info cal_formats[] = { > + /* > + * 2X8 entries are legacy codes. All new drivers should use 1X16 modes. > + * The 2X8 modes must be before 1X16 in this list so that the driver > + * behavior for non-MC mode doesn't change. > + */ > { > .fourcc = V4L2_PIX_FMT_YUYV, > .code = MEDIA_BUS_FMT_YUYV8_2X8, > .bpp = 16, > + .legacy = true, I then wonder if it isn't better to associate multiple mbus codes to a fourcc instead of duplicating the entries with the same fourcc > }, { > .fourcc = V4L2_PIX_FMT_UYVY, > .code = MEDIA_BUS_FMT_UYVY8_2X8, > .bpp = 16, > + .legacy = true, > }, { > .fourcc = V4L2_PIX_FMT_YVYU, > .code = MEDIA_BUS_FMT_YVYU8_2X8, > .bpp = 16, > + .legacy = true, > }, { > .fourcc = V4L2_PIX_FMT_VYUY, > .code = MEDIA_BUS_FMT_VYUY8_2X8, > .bpp = 16, > + .legacy = true, > + }, { > + .fourcc = V4L2_PIX_FMT_YUYV, > + .code = MEDIA_BUS_FMT_YUYV8_1X16, > + .bpp = 16, > + }, { > + .fourcc = V4L2_PIX_FMT_UYVY, > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .bpp = 16, > + }, { > + .fourcc = V4L2_PIX_FMT_YVYU, > + .code = MEDIA_BUS_FMT_YVYU8_1X16, > + .bpp = 16, > + }, { > + .fourcc = V4L2_PIX_FMT_VYUY, > + .code = MEDIA_BUS_FMT_VYUY8_1X16, > + .bpp = 16, > }, { > .fourcc = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */ > .code = MEDIA_BUS_FMT_RGB565_2X8_LE, Also RGB formats with 2X8 and 2X12 should probably be changed to 1X16 and 1X24 versions.. Thanks j > diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h > index de73d6d21b6f..44ecae8a273a 100644 > --- a/drivers/media/platform/ti/cal/cal.h > +++ b/drivers/media/platform/ti/cal/cal.h > @@ -89,6 +89,7 @@ struct cal_format_info { > /* Bits per pixel */ > u8 bpp; > bool meta; > + bool legacy; > }; > > /* buffer for one video frame */ > -- > 2.34.1 >
On 23/02/2023 19:10, Jacopo Mondi wrote: > Hi Tomi > > On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote: >> For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 >> link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and >> used by many drivers, so add 1X16 support to CAL. >> >> We keep the 2X8 formats for backward compatibility. > > Eh, this is the usual question about what we should consider a > to be a userspace breakage or not. > > I presume the reason to maintain 2X8 formats is (assuming the CAL > interface is CSI-2 only, right ?) because sensor drivers that only > support 2X8 formats will then fail to link_validate() if you remove Yes. > all 2X8 formats here. I'm of the opinion that we should bite the > bullet and move to 1X16. If any driver that used to work with CAL now > breaks, the sensor driver will have to be fixed. > > In my humble experience, that's what we did with the ov5640 sensor. We Yes, you did. > removed the 2X8 formats in CSI-2 mode and some platform driver broke > and then had been fixed (it happened in the same release cycle), win win. No, CAL is still broken =). OV5640 is the only sensor I have. I just haven't had time to look at this and fix it (and no one has complained). > I know it's controversial, let's see what others think. I'm all for dropping the 2X8 formats, if that's the consensus. It would keep the driver simpler, as we could keep the 1:1 relationship between pixel formats and mbus codes. I'll look at your other comments tomorrow. Tomi
Hi Tomi On Thu, Feb 23, 2023 at 07:52:48PM +0200, Tomi Valkeinen wrote: > On 23/02/2023 19:10, Jacopo Mondi wrote: > > Hi Tomi > > > > On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote: > > > For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 > > > link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and > > > used by many drivers, so add 1X16 support to CAL. > > > > > > We keep the 2X8 formats for backward compatibility. > > > > Eh, this is the usual question about what we should consider a > > to be a userspace breakage or not. > > > > I presume the reason to maintain 2X8 formats is (assuming the CAL > > interface is CSI-2 only, right ?) because sensor drivers that only > > support 2X8 formats will then fail to link_validate() if you remove > > Yes. > > > all 2X8 formats here. I'm of the opinion that we should bite the > > bullet and move to 1X16. If any driver that used to work with CAL now > > breaks, the sensor driver will have to be fixed. > > > > In my humble experience, that's what we did with the ov5640 sensor. We > > Yes, you did. > > > removed the 2X8 formats in CSI-2 mode and some platform driver broke > > and then had been fixed (it happened in the same release cycle), win win. > > No, CAL is still broken =). OV5640 is the only sensor I have. I just haven't > had time to look at this and fix it (and no one has complained). > Ups, I was thinking at the ST and microchip receivers, I thought CAL was fixed already :) See ? win win (almost) > > I know it's controversial, let's see what others think. > > I'm all for dropping the 2X8 formats, if that's the consensus. It would keep > the driver simpler, as we could keep the 1:1 relationship between pixel > formats and mbus codes. > > I'll look at your other comments tomorrow. > And I'll look at your last patch tomorrow if I can get a media graph dump! > Tomi >
On 23/02/2023 20:03, Jacopo Mondi wrote: > Hi Tomi > > On Thu, Feb 23, 2023 at 07:52:48PM +0200, Tomi Valkeinen wrote: >> On 23/02/2023 19:10, Jacopo Mondi wrote: >>> Hi Tomi >>> >>> On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote: >>>> For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 >>>> link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and >>>> used by many drivers, so add 1X16 support to CAL. >>>> >>>> We keep the 2X8 formats for backward compatibility. >>> >>> Eh, this is the usual question about what we should consider a >>> to be a userspace breakage or not. >>> >>> I presume the reason to maintain 2X8 formats is (assuming the CAL >>> interface is CSI-2 only, right ?) because sensor drivers that only >>> support 2X8 formats will then fail to link_validate() if you remove >> >> Yes. >> >>> all 2X8 formats here. I'm of the opinion that we should bite the >>> bullet and move to 1X16. If any driver that used to work with CAL now >>> breaks, the sensor driver will have to be fixed. >>> >>> In my humble experience, that's what we did with the ov5640 sensor. We >> >> Yes, you did. >> >>> removed the 2X8 formats in CSI-2 mode and some platform driver broke >>> and then had been fixed (it happened in the same release cycle), win win. >> >> No, CAL is still broken =). OV5640 is the only sensor I have. I just haven't >> had time to look at this and fix it (and no one has complained). >> > > Ups, I was thinking at the ST and microchip receivers, I thought CAL > was fixed already :) > > See ? win win (almost) > >>> I know it's controversial, let's see what others think. >> >> I'm all for dropping the 2X8 formats, if that's the consensus. It would keep >> the driver simpler, as we could keep the 1:1 relationship between pixel >> formats and mbus codes. >> >> I'll look at your other comments tomorrow. >> > > And I'll look at your last patch tomorrow if I can get a media graph > dump! I have attached the txt and dot versions of the graph of my FPDLink setup with three cameras (MC mode with streams). Some clarifications, which may not be obvious: The current upstream driver supports two distinct modes, non-MC (legacy) and MC modes, selectable with the cal_mc_api module parameter. Supporting the legacy mode does make the driver rather confusing in some areas... With this series, the non-MC mode is unchanged, but the MC mode will be extended to support streams. The non-MC mode (afaics, I have never much used it) only supports a simple setup with a single sensor subdevice connected directly to CAL. As we don't have MC, the subdevice is not visible to the userspace, and thus the CAL driver does tricks like collects the controls from the sensor, and exposes them from CAL's video node. And this is why we have two sets of ioctl ops in cal-video.c, cal_ioctl_legacy_ops and cal_ioctl_mc_ops, as the behavior is quite different between the two modes. Describing this makes me wonder if the non-MC mode could be dropped... MC mode has been supported for some years now. Tomi Media controller API version 6.2.0 Media device information ------------------------ driver cal model CAL serial bus info platform:489b0000.cal hw revision 0x40000300 driver version 6.2.0 Device topology - entity 1: CAMERARX0 (9 pads, 65 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0: Sink [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "ds90ub960 4-003d":4 [ENABLED,IMMUTABLE] pad1: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [ENABLED] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad2: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [ENABLED] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad3: Source [stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [ENABLED] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad4: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad5: Source [stream:0 fmt:unknown/1936x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [ENABLED] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad6: Source [stream:0 fmt:unknown/1936x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [ENABLED] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad7: Source [stream:0 fmt:unknown/1280x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [ENABLED] -> "CAL output 7":0 [] pad8: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] - entity 11: CAMERARX1 (9 pads, 64 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev1 pad0: Sink [stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] pad1: Source [stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad2: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad3: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad4: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad5: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad6: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad7: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] pad8: Source -> "CAL output 0":0 [] -> "CAL output 1":0 [] -> "CAL output 2":0 [] -> "CAL output 3":0 [] -> "CAL output 4":0 [] -> "CAL output 5":0 [] -> "CAL output 6":0 [] -> "CAL output 7":0 [] - entity 21: ds90ub960 4-003d (6 pads, 4 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev2 pad0: Sink [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "ds90ub953 4-0044":1 [ENABLED,IMMUTABLE] pad1: Sink [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "ds90ub953 4-0045":1 [ENABLED,IMMUTABLE] pad2: Sink [stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "ds90ub913a 4-0046":1 [ENABLED,IMMUTABLE] pad3: Sink pad4: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "CAMERARX0":0 [ENABLED,IMMUTABLE] pad5: Source - entity 30: ds90ub913a 4-0046 (2 pads, 2 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev3 pad0: Sink [stream:0 fmt:UYVY8_2X8/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "ov10635 7-0030":0 [ENABLED,IMMUTABLE] pad1: Source [stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "ds90ub960 4-003d":2 [ENABLED,IMMUTABLE] - entity 35: ds90ub953 4-0045 (2 pads, 2 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev4 pad0: Sink [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "imx390 6-0021":0 [ENABLED,IMMUTABLE] pad1: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "ds90ub960 4-003d":1 [ENABLED,IMMUTABLE] - entity 40: ds90ub953 4-0044 (2 pads, 2 links, 0 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev5 pad0: Sink [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "imx390 5-0021":0 [ENABLED,IMMUTABLE] pad1: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "ds90ub960 4-003d":0 [ENABLED,IMMUTABLE] - entity 45: imx390 5-0021 (1 pad, 1 link, 0 route) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev6 pad0: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:smpte170m] -> "ds90ub953 4-0044":0 [ENABLED,IMMUTABLE] - entity 49: imx390 6-0021 (1 pad, 1 link, 0 route) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev7 pad0: Source [stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:smpte170m] -> "ds90ub953 4-0045":0 [ENABLED,IMMUTABLE] - entity 53: ov10635 7-0030 (1 pad, 1 link, 0 route) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev8 pad0: Source [stream:0 fmt:UYVY8_2X8/1280x720 field:none colorspace:smpte170m] -> "ds90ub913a 4-0046":0 [ENABLED,IMMUTABLE] - entity 57: CAL output 0 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "CAMERARX0":1 [ENABLED] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 93: CAL output 1 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video1 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [ENABLED] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 129: CAL output 2 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video2 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [ENABLED] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 165: CAL output 3 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video3 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 201: CAL output 4 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video4 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [ENABLED] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 237: CAL output 5 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video5 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [ENABLED] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 273: CAL output 6 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video6 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [ENABLED] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 [] - entity 309: CAL output 7 (1 pad, 16 links, 0 route) type Node subtype V4L flags 0 device node name /dev/video7 pad0: Sink <- "CAMERARX0":1 [] <- "CAMERARX0":2 [] <- "CAMERARX0":3 [] <- "CAMERARX0":4 [] <- "CAMERARX0":5 [] <- "CAMERARX0":6 [] <- "CAMERARX0":7 [] <- "CAMERARX0":8 [] <- "CAMERARX1":1 [] <- "CAMERARX1":2 [] <- "CAMERARX1":3 [] <- "CAMERARX1":4 [] <- "CAMERARX1":5 [] <- "CAMERARX1":6 [] <- "CAMERARX1":7 [] <- "CAMERARX1":8 []
On 23/02/2023 19:10, Jacopo Mondi wrote: > Hi Tomi > > On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote: >> For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 >> link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and >> used by many drivers, so add 1X16 support to CAL. >> >> We keep the 2X8 formats for backward compatibility. > > Eh, this is the usual question about what we should consider a > to be a userspace breakage or not. > > I presume the reason to maintain 2X8 formats is (assuming the CAL > interface is CSI-2 only, right ?) because sensor drivers that only > support 2X8 formats will then fail to link_validate() if you remove > all 2X8 formats here. I'm of the opinion that we should bite the > bullet and move to 1X16. If any driver that used to work with CAL now > breaks, the sensor driver will have to be fixed. > > In my humble experience, that's what we did with the ov5640 sensor. We > removed the 2X8 formats in CSI-2 mode and some platform driver broke > and then had been fixed (it happened in the same release cycle), win win. > > I know it's controversial, let's see what others think. After thinking about this, I feel we can just drop the 2X8 support from CAL. So I'll refresh this patch and just change the 2X8 formats to 1X16. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++- >> drivers/media/platform/ti/cal/cal.c | 25 +++++++++++++++++++++++ >> drivers/media/platform/ti/cal/cal.h | 1 + >> 3 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c >> index 4eade409d5d3..87db8761d94d 100644 >> --- a/drivers/media/platform/ti/cal/cal-video.c >> +++ b/drivers/media/platform/ti/cal/cal-video.c >> @@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void *priv, >> if (f->mbus_code && cal_formats[i].code != f->mbus_code) >> continue; >> >> + /* >> + * Skip legacy formats so that we don't return duplicate fourccs, >> + * except in the case that the user explicitly asked for an >> + * mbus_code which matches this legacy format. >> + */ >> + if (cal_formats[i].legacy && >> + (!f->mbus_code || cal_formats[i].code != f->mbus_code)) > > This only works as there are no duplicate codes in cal_formats[] (iow > a pixel format is assigned to a single media bus code). If that's how > the CAL interface works (no colorspace conversion, no components > re-ordering) I guess it's fine No duplicate codes. >> + continue; >> + >> if (idx == f->index) { >> f->pixelformat = cal_formats[i].fourcc; >> f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> @@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx, >> >> static int cal_video_check_format(struct cal_ctx *ctx) >> { >> + const struct cal_format_info *rx_fmtinfo; >> const struct v4l2_mbus_framefmt *format; >> struct media_pad *remote_pad; >> >> @@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx) >> >> format = &ctx->phy->formats[remote_pad->index]; >> >> - if (ctx->fmtinfo->code != format->code || >> + rx_fmtinfo = cal_format_by_code(format->code); >> + if (!rx_fmtinfo) >> + return -EINVAL; >> + >> + if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc || > > Is this meant to make 2X8 and 1X16 formats compatible during link > validation ? There's no link-validation as such here, but CAL driver checks internally (the code here) that the video node's and the camerarx subdevices formats match. Here we check that if the video node uses specifies, say, YUYV, then both 2X8 and 1X16 codes from the camerarx are accepted. >> ctx->v_fmt.fmt.pix.height != format->height || >> ctx->v_fmt.fmt.pix.width != format->width || >> ctx->v_fmt.fmt.pix.field != format->field) >> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c >> index 1236215ec70e..053bf1030af0 100644 >> --- a/drivers/media/platform/ti/cal/cal.c >> +++ b/drivers/media/platform/ti/cal/cal.c >> @@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API"); >> */ >> >> const struct cal_format_info cal_formats[] = { >> + /* >> + * 2X8 entries are legacy codes. All new drivers should use 1X16 modes. >> + * The 2X8 modes must be before 1X16 in this list so that the driver >> + * behavior for non-MC mode doesn't change. >> + */ >> { >> .fourcc = V4L2_PIX_FMT_YUYV, >> .code = MEDIA_BUS_FMT_YUYV8_2X8, >> .bpp = 16, >> + .legacy = true, > > I then wonder if it isn't better to associate multiple mbus codes to > a fourcc instead of duplicating the entries with the same fourcc Maybe, but this way the legacy ones are kept separate, and we don't "mess up" the other modes with arrays of mbus codes, when no arrays are needed. > >> }, { >> .fourcc = V4L2_PIX_FMT_UYVY, >> .code = MEDIA_BUS_FMT_UYVY8_2X8, >> .bpp = 16, >> + .legacy = true, >> }, { >> .fourcc = V4L2_PIX_FMT_YVYU, >> .code = MEDIA_BUS_FMT_YVYU8_2X8, >> .bpp = 16, >> + .legacy = true, >> }, { >> .fourcc = V4L2_PIX_FMT_VYUY, >> .code = MEDIA_BUS_FMT_VYUY8_2X8, >> .bpp = 16, >> + .legacy = true, >> + }, { >> + .fourcc = V4L2_PIX_FMT_YUYV, >> + .code = MEDIA_BUS_FMT_YUYV8_1X16, >> + .bpp = 16, >> + }, { >> + .fourcc = V4L2_PIX_FMT_UYVY, >> + .code = MEDIA_BUS_FMT_UYVY8_1X16, >> + .bpp = 16, >> + }, { >> + .fourcc = V4L2_PIX_FMT_YVYU, >> + .code = MEDIA_BUS_FMT_YVYU8_1X16, >> + .bpp = 16, >> + }, { >> + .fourcc = V4L2_PIX_FMT_VYUY, >> + .code = MEDIA_BUS_FMT_VYUY8_1X16, >> + .bpp = 16, >> }, { >> .fourcc = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */ >> .code = MEDIA_BUS_FMT_RGB565_2X8_LE, > > Also RGB formats with 2X8 and 2X12 should probably be changed to 1X16 > and 1X24 versions.. Good point, I'll change those too. Tomi
diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c index 4eade409d5d3..87db8761d94d 100644 --- a/drivers/media/platform/ti/cal/cal-video.c +++ b/drivers/media/platform/ti/cal/cal-video.c @@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void *priv, if (f->mbus_code && cal_formats[i].code != f->mbus_code) continue; + /* + * Skip legacy formats so that we don't return duplicate fourccs, + * except in the case that the user explicitly asked for an + * mbus_code which matches this legacy format. + */ + if (cal_formats[i].legacy && + (!f->mbus_code || cal_formats[i].code != f->mbus_code)) + continue; + if (idx == f->index) { f->pixelformat = cal_formats[i].fourcc; f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; @@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx, static int cal_video_check_format(struct cal_ctx *ctx) { + const struct cal_format_info *rx_fmtinfo; const struct v4l2_mbus_framefmt *format; struct media_pad *remote_pad; @@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx) format = &ctx->phy->formats[remote_pad->index]; - if (ctx->fmtinfo->code != format->code || + rx_fmtinfo = cal_format_by_code(format->code); + if (!rx_fmtinfo) + return -EINVAL; + + if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc || ctx->v_fmt.fmt.pix.height != format->height || ctx->v_fmt.fmt.pix.width != format->width || ctx->v_fmt.fmt.pix.field != format->field) diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c index 1236215ec70e..053bf1030af0 100644 --- a/drivers/media/platform/ti/cal/cal.c +++ b/drivers/media/platform/ti/cal/cal.c @@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API"); */ const struct cal_format_info cal_formats[] = { + /* + * 2X8 entries are legacy codes. All new drivers should use 1X16 modes. + * The 2X8 modes must be before 1X16 in this list so that the driver + * behavior for non-MC mode doesn't change. + */ { .fourcc = V4L2_PIX_FMT_YUYV, .code = MEDIA_BUS_FMT_YUYV8_2X8, .bpp = 16, + .legacy = true, }, { .fourcc = V4L2_PIX_FMT_UYVY, .code = MEDIA_BUS_FMT_UYVY8_2X8, .bpp = 16, + .legacy = true, }, { .fourcc = V4L2_PIX_FMT_YVYU, .code = MEDIA_BUS_FMT_YVYU8_2X8, .bpp = 16, + .legacy = true, }, { .fourcc = V4L2_PIX_FMT_VYUY, .code = MEDIA_BUS_FMT_VYUY8_2X8, .bpp = 16, + .legacy = true, + }, { + .fourcc = V4L2_PIX_FMT_YUYV, + .code = MEDIA_BUS_FMT_YUYV8_1X16, + .bpp = 16, + }, { + .fourcc = V4L2_PIX_FMT_UYVY, + .code = MEDIA_BUS_FMT_UYVY8_1X16, + .bpp = 16, + }, { + .fourcc = V4L2_PIX_FMT_YVYU, + .code = MEDIA_BUS_FMT_YVYU8_1X16, + .bpp = 16, + }, { + .fourcc = V4L2_PIX_FMT_VYUY, + .code = MEDIA_BUS_FMT_VYUY8_1X16, + .bpp = 16, }, { .fourcc = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */ .code = MEDIA_BUS_FMT_RGB565_2X8_LE, diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h index de73d6d21b6f..44ecae8a273a 100644 --- a/drivers/media/platform/ti/cal/cal.h +++ b/drivers/media/platform/ti/cal/cal.h @@ -89,6 +89,7 @@ struct cal_format_info { /* Bits per pixel */ u8 bpp; bool meta; + bool legacy; }; /* buffer for one video frame */
For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2 link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and used by many drivers, so add 1X16 support to CAL. We keep the 2X8 formats for backward compatibility. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++- drivers/media/platform/ti/cal/cal.c | 25 +++++++++++++++++++++++ drivers/media/platform/ti/cal/cal.h | 1 + 3 files changed, 41 insertions(+), 1 deletion(-)