Hi Jacopo, Thank you for the patch. On Fri, Feb 18, 2022 at 07:34:17PM +0100, Jacopo Mondi wrote: > The CSI bridge should operate in dual pixel sampling mode when it is s/dual pixel sampling mode/dual component mode/ > connected to a pixel transmitter that transfers two pixel samples (16 > bits) at the time in YUYV formats. It's actually one pixel per clock. Without BIT_TWO_8BIT_SENSOR and BIT_MIPI_DOUBLE_CMPNT, the CSI bridge expects 8-bit data, which requires two clock cycles to transfer one pixel. When setting those bits, it expects 16-bit data, with one clock cycle per pixel. The MIPI_DOUBLE_CMPNT documentation states this clearly: 20 MIPI_DOUBLE_CMPNT Double component per clock cycle in YUV422 formats. 0 Single component per clock cycle (half pixel per clock cycle) 1 Double component per clock cycle (a pixel per clock cycle) > Use the image format variants to determine if single or dual pixel mode "dual pixel" is a concept of the CSIS, not the CSI bridge. This should mention single or double component mode. > should be used. > > Add a note to the TODO file to record that the list of supported formats > should be restricted to the SoC model the CSI bridge is integrated on > to avoid potential pipeline mis-configurations. > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > --- > drivers/staging/media/imx/TODO | 26 ++++++++++++++++++++++ > drivers/staging/media/imx/imx7-media-csi.c | 8 +++++-- > 2 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO > index 06c94f20ecf8..e15eba32cc94 100644 > --- a/drivers/staging/media/imx/TODO > +++ b/drivers/staging/media/imx/TODO > @@ -27,3 +27,29 @@ > - i.MX7: all of the above, since it uses the imx media core > > - i.MX7: use Frame Interval Monitor > + > +- imx7-media-csi: Restrict the supported formats list to the SoC version. > + > + The imx7 CSI bridge can be configured to sample pixel components from the Rx > + queue in single (8bpp) or double (16bpp) modes. Image format variations with > + different sample sizes (ie YUYV_2X8 vs YUYV_1X16) determine the sampling size > + (see imx7_csi_configure()). > + > + As the imx7 CSI bridge can be interfaced with different IP blocks depending on > + the SoC model it is integrated on, the Rx queue sampling size should match > + the size of samples transferred by the transmitting IP block. > + > + To avoid mis-configurations of the capture pipeline, the enumeration of the > + supported formats should be restricted to match the pixel source > + transmitting mode. > + > + Examples: i.MX8MM SoC integrates the CSI bridge with the Samsung CSIS CSI-2 > + receiver which operates in dual pixel sampling mode. The CSI bridge should > + only expose the 1X16 formats variant which instructs it to operate in dual > + pixel sampling mode. When the CSI bridge is instead integrated on an i.MX8MQ > + SoC, which features a CSI-2 receiver that operates in single sampling mode, it > + should only expose the 2X8 formats variant which instruct it to operate in > + single sampling mode. > + > + This currently only applies to YUYV formats, but other formats might need > + to be treated in the same way. > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index 32311fc0e2a4..108360ae3710 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -503,11 +503,15 @@ static void imx7_csi_configure(struct imx7_csi *csi) > * all of them comply. Support both variants. > */ You can drop this comment, as the two variants are not related to the CSI-2 bus format (which should always be UYVY8_1X16) but to the format output by the CSI-2 RX. I would add another comment to explain this clearly: /* * The CSI bridge has a 16-bit input bus. Depending on * the connected source, data may be transmitted with 8 * or 10 bits per clock sample (in bits [9:2] or [9:0] * respectively) or with 16 bits per clock sample (in * bits [15:0]). The data is then packed into a 32-bit * FIFO (as shown in figure 13-11 of the i.MX8MM * reference manual rev. 3). * * The data packing in a 32-bit FIFO input workd is * controlled by the CR3 TWO_8BIT_SENSOR field (also * known as SENSOR_16BITS in the i.MX8MM reference * manual). When set to 0, data packing groups four * 8-bit input samples (bits [9:2]). When set to 1, data * packing groups two 16-bit input samples (bits * [15:0]). * * The register field CR18 MIPI_DOUBLE_CMPNT also needs * to be configured according to the input format for * YUV 4:2:2 data. The field controls the gasket between * the CSI-2 receiver and the CSI bridge. On i.MX7 and * i.MX8MM, the field must be set to 1 when the CSIS * outputs 16-bit samples. On i.MX8MQ, the gasket * ignores the MIPI_DOUBLE_CMPNT bit and YUV 4:2:2 * always uses 16-bit samples. Setting MIPI_DOUBLE_CMPNT * in that case has no effect, but doesn't cause any * issue. */ With this, someone trying to figure out what those bits do will hopefully be able to get it right :-) With these changes, Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > case MEDIA_BUS_FMT_UYVY8_2X8: > - case MEDIA_BUS_FMT_UYVY8_1X16: > case MEDIA_BUS_FMT_YUYV8_2X8: > - case MEDIA_BUS_FMT_YUYV8_1X16: > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + case MEDIA_BUS_FMT_YUYV8_1X16: > + cr3 |= BIT_TWO_8BIT_SENSOR; > + cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | > + BIT_MIPI_DOUBLE_CMPNT; > + break; > } > } > -- Regards, Laurent Pinchart
