Re: [PATCH v2 3/7] media: imx: imx7-media-csi: Use dual sampling for YUV 1X16

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 21, 2022 at 10:49:15AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Feb 21, 2022 at 09:43:07AM +0100, Jacopo Mondi wrote:
> > On Sun, Feb 20, 2022 at 10:16:15AM +0200, Laurent Pinchart wrote:
> > > 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:
> >
> > Indeed, that's why I said two pixels -samples-.
> >
> > Maybe the usage of 'sample' is not right ? Two pixel samples in YUYV
> > mode to me means two bytes which form 1 pixel.
>
> I think it's a bit unclear. Maybe you could write
>
> The CSI bridge should operate in dual component mode when it is
> connected to a pixel transmitter that transfers two components at a time
> in YUV 422 formats (16 bits, Y + U/V).
>
> > > 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.
> >
> > Ack
> >
> > > > 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
> >
> > s/workd/word
> >
> > > 		 * 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 :-)
> >
> > Thanks, very clear. I didn't get the last part about the MQ. I thought
> > the NW csi-2 receiver worked with 8-bit samples that's why we need to
> > maintain the CSI bridge compatible with the 2X8 format variant (this
> > and also for imx7 + parallel).
> >
> > Actually, looking at the MQ reference manual
> >
> > "15.2.1.3.5 CSI-2 Controller Core Configurations"  reports:
> > The CSI-2 RX Controller Core User Interface supports either a single,
> > double, or quad pixel wide data path. The double and quad wide pixel
> > modes can help reduce the frequency the user interface must run at
> > while the single pixel wide mode can be easier to interface to for
> > some applications.
> >
> > This chip supports the following:
> > • Single Pixel Configuration
> >
> > It seems the csi-2 receiver on the MQ works in single pixel mode only.
> > It should then expose the 2X8 format variant only, which is
> > technically incorrect for a serial transmitter. Cul de sac ?
>
> It's a different CSI-2 receiver, and uses a different terminology.
> "Single pixel mode" there refers to one pixel per clock cycle, which
> means, for YUV422 8-bit, 16 bits per cycle. The MIPI_DOUBLE_CMPNT bit is
> ignored by the gasket on i.MX8MQ.

   -Lovely-

I'll send patch to drop 2X8 variant from the mq csi-2 receiver.

Thanks
  j

>
> > > With these changes,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > Thanks
> >    j
> >
> > > >  		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




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux