| Message ID | 20210316115635.4096574-1-martin.kepplinger@puri.sm |
|---|---|
| State | New |
| Headers | show |
| Series | imx7-media-csi: csi2 only | expand |
Hi Martin, On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger wrote: > --- > > hi Laurent, > > thanks a lot for posting this series! > > First: I only test imx7-media-csi (csi bridge) because I run it on imx8mq. > overall, I'm very happy with all of this and I get the same image out > of it as I get with the mx6s_capture nxp driver. That's good news :-) > one issue I have is with is_csi2, so I post this patch that I need in > order to test. It's obviously no solution, just to describe the issue: > > I'm not sure why but imx7_csi_pad_link_validate() isn't called in my case > and is_csi2 doesn't get set, so I force it. Would it make sense to make > a dts property for this? Some platforms support both parallel and CSI-2 inputs, so we can't hardcode which one is used in DT. I'd advise trying to debug why the function is never called in your case, it's meant to be called with the following call stack - imx7_csi_pad_link_validate() (through v4l2_subdev_pad_ops.link_validate) - v4l2_subdev_link_validate() (through media_entity_operations.link_validate) - __media_pipeline_start() - imx_media_pipeline_set_stream() - capture_start_streaming() - ... > The other thing is that > v4l2-ctl --set-fmt-video=width=1280,height=720,pixelformat=0 > doesn't call the sensor drivers' set_fmt() pad function. That means that > only the one mode I hard-code as default will work. instead I just see this: That's expected. With a driver based on the media controller API, you have to configure the format on each subdev manually. You can use the media-ctl utility for that. The video node is only used to control the DMA engine, the kernel driver doesn't propagate the configuration to subdevices. > [ 742.977677] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' > [ 742.977702] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack > [ 742.977709] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) > [ 742.977714] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' > [ 742.977720] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' > [ 742.977985] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' > [ 742.977992] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack > [ 742.977997] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) > [ 742.978002] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' > [ 742.978008] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' > [ 742.978025] mc: media_release: Media Release > > Does anything come to your mind that would prevent the set_fmt call here? > That's what the (nxp) mipi driver looks like I'm running here: > https://source.puri.sm/martin.kepplinger/linux-next/-/blob/5.12-rc3/librem5__integration_byzantium/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c > > drivers/staging/media/imx/imx7-media-csi.c | 135 +++++++++------------ > 1 file changed, 60 insertions(+), 75 deletions(-) > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index 56b92ba97d25..95c359019725 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -435,82 +435,67 @@ static void imx7_csi_configure(struct imx7_csi *csi) > stride = out_pix->width; > } > > - if (!csi->is_csi2) { > - dev_dbg(csi->dev, "%s: NOT is_csi2\n", __func__); > - cr1 = BIT_SOF_POL | BIT_REDGE | BIT_GCLK_MODE | BIT_HSYNC_POL > - | BIT_FCC | BIT_MCLKDIV(1) | BIT_MCLKEN; > - > - cr18 |= BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | > - BIT_BASEADDR_CHG_ERR_EN; > - > - if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY || > - out_pix->pixelformat == V4L2_PIX_FMT_YUYV) > - width *= 2; > - } else { > - dev_dbg(csi->dev, "%s: is_csi2\n", __func__); > - > - cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC > - | BIT_MCLKDIV(1) | BIT_MCLKEN; > - > - cr18 |= BIT_DATA_FROM_MIPI; > - > - switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) { > - case MEDIA_BUS_FMT_Y8_1X8: > - case MEDIA_BUS_FMT_SBGGR8_1X8: > - case MEDIA_BUS_FMT_SGBRG8_1X8: > - case MEDIA_BUS_FMT_SGRBG8_1X8: > - case MEDIA_BUS_FMT_SRGGB8_1X8: > - cr18 |= BIT_MIPI_DATA_FORMAT_RAW8; > - break; > - case MEDIA_BUS_FMT_Y10_1X10: > - case MEDIA_BUS_FMT_SBGGR10_1X10: > - case MEDIA_BUS_FMT_SGBRG10_1X10: > - case MEDIA_BUS_FMT_SGRBG10_1X10: > - case MEDIA_BUS_FMT_SRGGB10_1X10: > - dev_dbg(csi->dev, "%s: RAW10\n", __func__); > - cr18 |= BIT_MIPI_DATA_FORMAT_RAW10; > - break; > - case MEDIA_BUS_FMT_Y12_1X12: > - case MEDIA_BUS_FMT_SBGGR12_1X12: > - case MEDIA_BUS_FMT_SGBRG12_1X12: > - case MEDIA_BUS_FMT_SGRBG12_1X12: > - case MEDIA_BUS_FMT_SRGGB12_1X12: > - cr18 |= BIT_MIPI_DATA_FORMAT_RAW12; > - break; > - case MEDIA_BUS_FMT_Y14_1X14: > - case MEDIA_BUS_FMT_SBGGR14_1X14: > - case MEDIA_BUS_FMT_SGBRG14_1X14: > - case MEDIA_BUS_FMT_SGRBG14_1X14: > - case MEDIA_BUS_FMT_SRGGB14_1X14: > - cr18 |= BIT_MIPI_DATA_FORMAT_RAW14; > - break; > - /* > - * CSI-2 sources are supposed to use the 1X16 formats, but not > - * all of them comply. Support both variants. > - */ > - 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; > - } > + cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC > + | BIT_MCLKDIV(1) | BIT_MCLKEN; > + > + cr18 |= BIT_DATA_FROM_MIPI; > + > + switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) { > + case MEDIA_BUS_FMT_Y8_1X8: > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + cr18 |= BIT_MIPI_DATA_FORMAT_RAW8; > + break; > + case MEDIA_BUS_FMT_Y10_1X10: > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + dev_dbg(csi->dev, "%s: RAW10\n", __func__); > + cr18 |= BIT_MIPI_DATA_FORMAT_RAW10; > + break; > + case MEDIA_BUS_FMT_Y12_1X12: > + case MEDIA_BUS_FMT_SBGGR12_1X12: > + case MEDIA_BUS_FMT_SGBRG12_1X12: > + case MEDIA_BUS_FMT_SGRBG12_1X12: > + case MEDIA_BUS_FMT_SRGGB12_1X12: > + cr18 |= BIT_MIPI_DATA_FORMAT_RAW12; > + break; > + case MEDIA_BUS_FMT_Y14_1X14: > + case MEDIA_BUS_FMT_SBGGR14_1X14: > + case MEDIA_BUS_FMT_SGBRG14_1X14: > + case MEDIA_BUS_FMT_SGRBG14_1X14: > + case MEDIA_BUS_FMT_SRGGB14_1X14: > + cr18 |= BIT_MIPI_DATA_FORMAT_RAW14; > + break; > + /* > + * CSI-2 sources are supposed to use the 1X16 formats, but not > + * all of them comply. Support both variants. > + */ > + 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; > + } > > - switch (out_pix->pixelformat) { > - case V4L2_PIX_FMT_Y10: > - case V4L2_PIX_FMT_Y12: > - case V4L2_PIX_FMT_SBGGR8: > - case V4L2_PIX_FMT_SGBRG8: > - case V4L2_PIX_FMT_SGRBG8: > - case V4L2_PIX_FMT_SRGGB8: > - case V4L2_PIX_FMT_SBGGR16: > - case V4L2_PIX_FMT_SGBRG16: > - case V4L2_PIX_FMT_SGRBG16: > - case V4L2_PIX_FMT_SRGGB16: > - dev_dbg(csi->dev, "%s: PIXEL_BIT\n", __func__); > - cr1 |= BIT_PIXEL_BIT; > - break; > - } > + switch (out_pix->pixelformat) { > + case V4L2_PIX_FMT_Y10: > + case V4L2_PIX_FMT_Y12: > + case V4L2_PIX_FMT_SBGGR8: > + case V4L2_PIX_FMT_SGBRG8: > + case V4L2_PIX_FMT_SGRBG8: > + case V4L2_PIX_FMT_SRGGB8: > + case V4L2_PIX_FMT_SBGGR16: > + case V4L2_PIX_FMT_SGBRG16: > + case V4L2_PIX_FMT_SGRBG16: > + case V4L2_PIX_FMT_SRGGB16: > + dev_dbg(csi->dev, "%s: PIXEL_BIT\n", __func__); > + cr1 |= BIT_PIXEL_BIT; > + break; > } > > imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
Am Dienstag, dem 16.03.2021 um 20:05 +0200 schrieb Laurent Pinchart: > Hi Martin, > > On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger wrote: > > --- > > > > hi Laurent, > > > > thanks a lot for posting this series! > > > > First: I only test imx7-media-csi (csi bridge) because I run it on > > imx8mq. > > overall, I'm very happy with all of this and I get the same image > > out > > of it as I get with the mx6s_capture nxp driver. > > That's good news :-) > > > one issue I have is with is_csi2, so I post this patch that I need > > in > > order to test. It's obviously no solution, just to describe the > > issue: > > > > I'm not sure why but imx7_csi_pad_link_validate() isn't called in > > my case > > and is_csi2 doesn't get set, so I force it. Would it make sense to > > make > > a dts property for this? > > Some platforms support both parallel and CSI-2 inputs, so we can't > hardcode which one is used in DT. I'd advise trying to debug why the > function is never called in your case, it's meant to be called with > the > following call stack > > - imx7_csi_pad_link_validate() (through > v4l2_subdev_pad_ops.link_validate) > - v4l2_subdev_link_validate() (through > media_entity_operations.link_validate) > - __media_pipeline_start() > - imx_media_pipeline_set_stream() > - capture_start_streaming() > - ... possible the 2 issues are related. I have to say that media-ctl (and the api) is kind of new to me and this is not strictly related to the patchset anymore. The patchset is certainly required for us to have. this is just me trying to test it properly... I tried to set what I know that my sensor driver sends to mipi: media-ctl -V "'csi':0 [fmt:SBGGR10/640x480 colorspace:raw]" Device topology - entity 1: csi (2 pads, 1 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0: Sink [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] pad1: Source [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] -> "csi capture":0 [ENABLED,IMMUTABLE] - entity 4: csi capture (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "csi":1 [ENABLED,IMMUTABLE] - entity 10: mxc-mipi-csi2.0 (0 pad, 0 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev1 now the mipi and camera drivers don't seem to be a "media" drivers, but is that strictly necessary? anyways, capture_start_streaming() calls capture_validate_fmt() which fails due to the colorspace 0 it finds: [ 77.754721] capture_start_streaming: starting [ 77.759758] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err [ 77.768150] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x0 != 0x1 [ 77.778372] imx7-csi 30a90000.csi1_bridge: capture format not valid thanks so far, martin > > > The other thing is that > > v4l2-ctl --set-fmt-video=width=1280,height=720,pixelformat=0 > > doesn't call the sensor drivers' set_fmt() pad function. That means > > that > > only the one mode I hard-code as default will work. instead I just > > see this: > > That's expected. With a driver based on the media controller API, you > have to configure the format on each subdev manually. You can use the > media-ctl utility for that. The video node is only used to control > the > DMA engine, the kernel driver doesn't propagate the configuration to > subdevices. > > > [ 742.977677] imx7-csi 30a90000.csi1_bridge: begin graph walk at > > 'csi capture' > > [ 742.977702] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' > > on stack > > [ 742.977709] imx7-csi 30a90000.csi1_bridge: walk: skipping entity > > 'csi capture' (already seen) > > [ 742.977714] imx7-csi 30a90000.csi1_bridge: walk: returning > > entity 'csi' > > [ 742.977720] imx7-csi 30a90000.csi1_bridge: walk: returning > > entity 'csi capture' > > [ 742.977985] imx7-csi 30a90000.csi1_bridge: begin graph walk at > > 'csi capture' > > [ 742.977992] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' > > on stack > > [ 742.977997] imx7-csi 30a90000.csi1_bridge: walk: skipping entity > > 'csi capture' (already seen) > > [ 742.978002] imx7-csi 30a90000.csi1_bridge: walk: returning > > entity 'csi' > > [ 742.978008] imx7-csi 30a90000.csi1_bridge: walk: returning > > entity 'csi capture' > > [ 742.978025] mc: media_release: Media Release > > > > Does anything come to your mind that would prevent the set_fmt call > > here? > > That's what the (nxp) mipi driver looks like I'm running here: > > https://source.puri.sm/martin.kepplinger/linux-next/-/blob/5.12-rc3/librem5__integration_byzantium/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c > >
Hi Martin, On Wed, Mar 17, 2021 at 11:08:57AM +0100, Martin Kepplinger wrote: > Am Dienstag, dem 16.03.2021 um 20:05 +0200 schrieb Laurent Pinchart: > > On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger wrote: > > > --- > > > > > > hi Laurent, > > > > > > thanks a lot for posting this series! > > > > > > First: I only test imx7-media-csi (csi bridge) because I run it on imx8mq. > > > overall, I'm very happy with all of this and I get the same image out > > > of it as I get with the mx6s_capture nxp driver. > > > > That's good news :-) > > > > > one issue I have is with is_csi2, so I post this patch that I need in > > > order to test. It's obviously no solution, just to describe the issue: > > > > > > I'm not sure why but imx7_csi_pad_link_validate() isn't called in my case > > > and is_csi2 doesn't get set, so I force it. Would it make sense to> make > > > a dts property for this? > > > > Some platforms support both parallel and CSI-2 inputs, so we can't > > hardcode which one is used in DT. I'd advise trying to debug why the > > function is never called in your case, it's meant to be called with the > > following call stack > > > > - imx7_csi_pad_link_validate() (through > > v4l2_subdev_pad_ops.link_validate) > > - v4l2_subdev_link_validate() (through > > media_entity_operations.link_validate) > > - __media_pipeline_start() > > - imx_media_pipeline_set_stream() > > - capture_start_streaming() > > - ... > > possible the 2 issues are related. I have to say that media-ctl (and > the api) is kind of new to me and this is not strictly related to the > patchset anymore. The patchset is certainly required for us to have. > this is just me trying to test it properly... > > > I tried to set what I know that my sensor driver sends to mipi: > > media-ctl -V "'csi':0 [fmt:SBGGR10/640x480 colorspace:raw]" > > > Device topology > - entity 1: csi (2 pads, 1 link) > type V4L2 subdev subtype Unknown flags 0 > device node name /dev/v4l-subdev0 > pad0: Sink > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > pad1: Source > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > -> "csi capture":0 [ENABLED,IMMUTABLE] > > - entity 4: csi capture (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "csi":1 [ENABLED,IMMUTABLE] > > - entity 10: mxc-mipi-csi2.0 (0 pad, 0 link) > type V4L2 subdev subtype Unknown flags 0 > device node name /dev/v4l-subdev1 The sensor is missing from the media graph, so that's not very likely to work :-) Has the sensor driver probed correctly ? Also, the mxc-mipi-csi2.0 entity doesn't have pads, that's not right. > now the mipi and camera drivers don't seem to be a "media" drivers, but > is that strictly necessary? > > anyways, capture_start_streaming() calls capture_validate_fmt() which > fails due to the colorspace 0 it finds: > > [ 77.754721] capture_start_streaming: starting > [ 77.759758] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err > [ 77.768150] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x0 != 0x1 > [ 77.778372] imx7-csi 30a90000.csi1_bridge: capture format not valid > > > > The other thing is that > > > v4l2-ctl --set-fmt-video=width=1280,height=720,pixelformat=0 > > > doesn't call the sensor drivers' set_fmt() pad function. That means that > > > only the one mode I hard-code as default will work. instead I just > > > see this: > > > > That's expected. With a driver based on the media controller API, you > > have to configure the format on each subdev manually. You can use the > > media-ctl utility for that. The video node is only used to control the > > DMA engine, the kernel driver doesn't propagate the configuration to > > subdevices. > > > > > [ 742.977677] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' > > > [ 742.977702] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack > > > [ 742.977709] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) > > > [ 742.977714] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' > > > [ 742.977720] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' > > > [ 742.977985] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' > > > [ 742.977992] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack > > > [ 742.977997] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) > > > [ 742.978002] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' > > > [ 742.978008] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' > > > [ 742.978025] mc: media_release: Media Release > > > > > > Does anything come to your mind that would prevent the set_fmt call here? > > > That's what the (nxp) mipi driver looks like I'm running here: > > > https://source.puri.sm/martin.kepplinger/linux-next/-/blob/5.12-rc3/librem5__integration_byzantium/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c -- Regards, Laurent Pinchart
Am Mittwoch, dem 17.03.2021 um 12:19 +0200 schrieb Laurent Pinchart: > Hi Martin, > > On Wed, Mar 17, 2021 at 11:08:57AM +0100, Martin Kepplinger wrote: > > Am Dienstag, dem 16.03.2021 um 20:05 +0200 schrieb Laurent > > Pinchart: > > > On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger > > > wrote: > > > > --- > > > > > > > > hi Laurent, > > > > > > > > thanks a lot for posting this series! > > > > > > > > First: I only test imx7-media-csi (csi bridge) because I run it > > > > on imx8mq. > > > > overall, I'm very happy with all of this and I get the same > > > > image out > > > > of it as I get with the mx6s_capture nxp driver. > > > > > > That's good news :-) > > > > > > > one issue I have is with is_csi2, so I post this patch that I > > > > need in > > > > order to test. It's obviously no solution, just to describe the > > > > issue: > > > > > > > > I'm not sure why but imx7_csi_pad_link_validate() isn't called > > > > in my case > > > > and is_csi2 doesn't get set, so I force it. Would it make sense > > > > to> make > > > > a dts property for this? > > > > > > Some platforms support both parallel and CSI-2 inputs, so we > > > can't > > > hardcode which one is used in DT. I'd advise trying to debug why > > > the > > > function is never called in your case, it's meant to be called > > > with the > > > following call stack > > > > > > - imx7_csi_pad_link_validate() (through > > > v4l2_subdev_pad_ops.link_validate) > > > - v4l2_subdev_link_validate() (through > > > media_entity_operations.link_validate) > > > - __media_pipeline_start() > > > - imx_media_pipeline_set_stream() > > > - capture_start_streaming() > > > - ... > > > > possible the 2 issues are related. I have to say that media-ctl > > (and > > the api) is kind of new to me and this is not strictly related to > > the > > patchset anymore. The patchset is certainly required for us to > > have. > > this is just me trying to test it properly... > > > > > > I tried to set what I know that my sensor driver sends to mipi: > > > > media-ctl -V "'csi':0 [fmt:SBGGR10/640x480 colorspace:raw]" > > > > > > Device topology > > - entity 1: csi (2 pads, 1 link) > > type V4L2 subdev subtype Unknown flags 0 > > device node name /dev/v4l-subdev0 > > pad0: Sink > > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw > > xfer:none ycbcr:601 quantization:full-range] > > pad1: Source > > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw > > xfer:none ycbcr:601 quantization:full-range] > > -> "csi capture":0 [ENABLED,IMMUTABLE] > > > > - entity 4: csi capture (1 pad, 1 link) > > type Node subtype V4L flags 0 > > device node name /dev/video0 > > pad0: Sink > > <- "csi":1 [ENABLED,IMMUTABLE] > > > > - entity 10: mxc-mipi-csi2.0 (0 pad, 0 link) > > type V4L2 subdev subtype Unknown flags 0 > > device node name /dev/v4l-subdev1 > > The sensor is missing from the media graph, so that's not very likely > to > work :-) Has the sensor driver probed correctly ? Also, the > mxc-mipi-csi2.0 entity doesn't have pads, that's not right. > > ok. what I try is adding a mipi driver (based on nxps') to the staging drivers and what media-ctl -p says so far is below. Despite no sensor being shown (not sure what's missing there), the sensors' get_format() and get_frame_interval() are being called during "media-ctl -p" and hence the "mxc-mipi-csi2.0" sink format is correct. somehow commands like these just don't change anything: media-ctl -l "'mxc-mipi-csi2.0':4" -> "'csi':0[1]" or setting already set up links to active [1] or inactive [0] doesn't change anything either: media-ctl -l "'csi':1" -> "'csi capture':0" "[1]" does anything come to mind what's missing here by chance? thanks a lot for all your help! Media device information ------------------------ driver imx7-csi model imx-media serial bus info hw revision 0x0 driver version 5.12.0 Device topology - entity 1: csi (2 pads, 1 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0: Sink [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] pad1: Source [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "csi capture":0 [ENABLED,IMMUTABLE] - entity 4: csi capture (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video1 pad0: Sink <- "csi":1 [ENABLED,IMMUTABLE] - entity 10: mxc-mipi-csi2.0 (8 pads, 0 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev1 pad0: Sink [fmt:SBGGR10_1X10/640x480@1/30 field:none colorspace:unknown] pad1: Unknown pad2: Unknown pad3: Unknown pad4: Source pad5: Unknown pad6: Unknown pad7: Unknown
Hi Martin, On Sun, Mar 28, 2021 at 04:25:26PM +0200, Martin Kepplinger wrote: > Am Mittwoch, dem 17.03.2021 um 12:19 +0200 schrieb Laurent Pinchart: > > On Wed, Mar 17, 2021 at 11:08:57AM +0100, Martin Kepplinger wrote: > > > Am Dienstag, dem 16.03.2021 um 20:05 +0200 schrieb Laurent Pinchart: > > > > On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger wrote: > > > > > --- > > > > > > > > > > hi Laurent, > > > > > > > > > > thanks a lot for posting this series! > > > > > > > > > > First: I only test imx7-media-csi (csi bridge) because I run it on imx8mq. > > > > > overall, I'm very happy with all of this and I get the same image out > > > > > of it as I get with the mx6s_capture nxp driver. > > > > > > > > That's good news :-) > > > > > > > > > one issue I have is with is_csi2, so I post this patch that I need in > > > > > order to test. It's obviously no solution, just to describe the issue: > > > > > > > > > > I'm not sure why but imx7_csi_pad_link_validate() isn't called in my case > > > > > and is_csi2 doesn't get set, so I force it. Would it make sense to make > > > > > a dts property for this? > > > > > > > > Some platforms support both parallel and CSI-2 inputs, so we can't > > > > hardcode which one is used in DT. I'd advise trying to debug why the > > > > function is never called in your case, it's meant to be called with the > > > > following call stack > > > > > > > > - imx7_csi_pad_link_validate() (through > > > > v4l2_subdev_pad_ops.link_validate) > > > > - v4l2_subdev_link_validate() (through > > > > media_entity_operations.link_validate) > > > > - __media_pipeline_start() > > > > - imx_media_pipeline_set_stream() > > > > - capture_start_streaming() > > > > - ... > > > > > > possible the 2 issues are related. I have to say that media-ctl (and > > > the api) is kind of new to me and this is not strictly related to the > > > patchset anymore. The patchset is certainly required for us to have. > > > this is just me trying to test it properly... > > > > > > > > > I tried to set what I know that my sensor driver sends to mipi: > > > > > > media-ctl -V "'csi':0 [fmt:SBGGR10/640x480 colorspace:raw]" > > > > > > > > > Device topology > > > - entity 1: csi (2 pads, 1 link) > > > type V4L2 subdev subtype Unknown flags 0 > > > device node name /dev/v4l-subdev0 > > > pad0: Sink > > > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw > > > xfer:none ycbcr:601 quantization:full-range] > > > pad1: Source > > > [fmt:SBGGR10_1X10/640x480 field:none colorspace:raw > > > xfer:none ycbcr:601 quantization:full-range] > > > -> "csi capture":0 [ENABLED,IMMUTABLE] > > > > > > - entity 4: csi capture (1 pad, 1 link) > > > type Node subtype V4L flags 0 > > > device node name /dev/video0 > > > pad0: Sink > > > <- "csi":1 [ENABLED,IMMUTABLE] > > > > > > - entity 10: mxc-mipi-csi2.0 (0 pad, 0 link) > > > type V4L2 subdev subtype Unknown flags 0 > > > device node name /dev/v4l-subdev1 > > > > The sensor is missing from the media graph, so that's not very likely to > > work :-) Has the sensor driver probed correctly ? Also, the > > mxc-mipi-csi2.0 entity doesn't have pads, that's not right. > > ok. what I try is adding a mipi driver (based on nxps') to the staging > drivers and what media-ctl -p says so far is below. Despite no sensor > being shown (not sure what's missing there), the sensors' get_format() > and get_frame_interval() are being called during "media-ctl -p" and > hence the "mxc-mipi-csi2.0" sink format is correct. > > somehow commands like these just don't change anything: > > media-ctl -l "'mxc-mipi-csi2.0':4" -> "'csi':0[1]" > or setting already set up links to active [1] or inactive [0] doesn't > change anything either: > media-ctl -l "'csi':1" -> "'csi capture':0" "[1]" > > does anything come to mind what's missing here by chance? thanks a lot > for all your help! > > > Media device information > ------------------------ > driver imx7-csi > model imx-media > serial > bus info > hw revision 0x0 > driver version 5.12.0 > > Device topology > - entity 1: csi (2 pads, 1 link) > type V4L2 subdev subtype Unknown flags 0 > device node name /dev/v4l-subdev0 > pad0: Sink > [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:lim-range] > pad1: Source > [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb > xfer:srgb ycbcr:601 quantization:lim-range] > -> "csi capture":0 [ENABLED,IMMUTABLE] > > - entity 4: csi capture (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video1 > pad0: Sink > <- "csi":1 [ENABLED,IMMUTABLE] > > - entity 10: mxc-mipi-csi2.0 (8 pads, 0 link) > type V4L2 subdev subtype Unknown flags 0 > device node name /dev/v4l-subdev1 > pad0: Sink > [fmt:SBGGR10_1X10/640x480@1/30 field:none colorspace:unknown] > pad1: Unknown > pad2: Unknown > pad3: Unknown > pad4: Source > pad5: Unknown > pad6: Unknown > pad7: Unknown I can't tell what wrong without looking at the code, but for instance the 8 pads above also look fishy. I'd recommend extending the existing imx7-mipi-csis driver instead of creating a new driver. The CSI-2 receivers in i.MX7 and i.MX8 are very similar, the latter having a more recent version of the same IP core (originating from Samsung, and actually also supported in one separate driver for Exynos which should ideally be merged with imx7-mipi-csis into a single implementation, but that's a separate matter). A new i.MX8-specific driver will have little chance of being accepted upstream given the above. -- Regards, Laurent Pinchart
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index 56b92ba97d25..95c359019725 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -435,82 +435,67 @@ static void imx7_csi_configure(struct imx7_csi *csi) stride = out_pix->width; } - if (!csi->is_csi2) { - dev_dbg(csi->dev, "%s: NOT is_csi2\n", __func__); - cr1 = BIT_SOF_POL | BIT_REDGE | BIT_GCLK_MODE | BIT_HSYNC_POL - | BIT_FCC | BIT_MCLKDIV(1) | BIT_MCLKEN; - - cr18 |= BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | - BIT_BASEADDR_CHG_ERR_EN; - - if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY || - out_pix->pixelformat == V4L2_PIX_FMT_YUYV) - width *= 2; - } else { - dev_dbg(csi->dev, "%s: is_csi2\n", __func__); - - cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC - | BIT_MCLKDIV(1) | BIT_MCLKEN; - - cr18 |= BIT_DATA_FROM_MIPI; - - switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) { - case MEDIA_BUS_FMT_Y8_1X8: - case MEDIA_BUS_FMT_SBGGR8_1X8: - case MEDIA_BUS_FMT_SGBRG8_1X8: - case MEDIA_BUS_FMT_SGRBG8_1X8: - case MEDIA_BUS_FMT_SRGGB8_1X8: - cr18 |= BIT_MIPI_DATA_FORMAT_RAW8; - break; - case MEDIA_BUS_FMT_Y10_1X10: - case MEDIA_BUS_FMT_SBGGR10_1X10: - case MEDIA_BUS_FMT_SGBRG10_1X10: - case MEDIA_BUS_FMT_SGRBG10_1X10: - case MEDIA_BUS_FMT_SRGGB10_1X10: - dev_dbg(csi->dev, "%s: RAW10\n", __func__); - cr18 |= BIT_MIPI_DATA_FORMAT_RAW10; - break; - case MEDIA_BUS_FMT_Y12_1X12: - case MEDIA_BUS_FMT_SBGGR12_1X12: - case MEDIA_BUS_FMT_SGBRG12_1X12: - case MEDIA_BUS_FMT_SGRBG12_1X12: - case MEDIA_BUS_FMT_SRGGB12_1X12: - cr18 |= BIT_MIPI_DATA_FORMAT_RAW12; - break; - case MEDIA_BUS_FMT_Y14_1X14: - case MEDIA_BUS_FMT_SBGGR14_1X14: - case MEDIA_BUS_FMT_SGBRG14_1X14: - case MEDIA_BUS_FMT_SGRBG14_1X14: - case MEDIA_BUS_FMT_SRGGB14_1X14: - cr18 |= BIT_MIPI_DATA_FORMAT_RAW14; - break; - /* - * CSI-2 sources are supposed to use the 1X16 formats, but not - * all of them comply. Support both variants. - */ - 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; - } + cr1 = BIT_SOF_POL | BIT_REDGE | BIT_HSYNC_POL | BIT_FCC + | BIT_MCLKDIV(1) | BIT_MCLKEN; + + cr18 |= BIT_DATA_FROM_MIPI; + + switch (csi->format_mbus[IMX7_CSI_PAD_SINK].code) { + case MEDIA_BUS_FMT_Y8_1X8: + case MEDIA_BUS_FMT_SBGGR8_1X8: + case MEDIA_BUS_FMT_SGBRG8_1X8: + case MEDIA_BUS_FMT_SGRBG8_1X8: + case MEDIA_BUS_FMT_SRGGB8_1X8: + cr18 |= BIT_MIPI_DATA_FORMAT_RAW8; + break; + case MEDIA_BUS_FMT_Y10_1X10: + case MEDIA_BUS_FMT_SBGGR10_1X10: + case MEDIA_BUS_FMT_SGBRG10_1X10: + case MEDIA_BUS_FMT_SGRBG10_1X10: + case MEDIA_BUS_FMT_SRGGB10_1X10: + dev_dbg(csi->dev, "%s: RAW10\n", __func__); + cr18 |= BIT_MIPI_DATA_FORMAT_RAW10; + break; + case MEDIA_BUS_FMT_Y12_1X12: + case MEDIA_BUS_FMT_SBGGR12_1X12: + case MEDIA_BUS_FMT_SGBRG12_1X12: + case MEDIA_BUS_FMT_SGRBG12_1X12: + case MEDIA_BUS_FMT_SRGGB12_1X12: + cr18 |= BIT_MIPI_DATA_FORMAT_RAW12; + break; + case MEDIA_BUS_FMT_Y14_1X14: + case MEDIA_BUS_FMT_SBGGR14_1X14: + case MEDIA_BUS_FMT_SGBRG14_1X14: + case MEDIA_BUS_FMT_SGRBG14_1X14: + case MEDIA_BUS_FMT_SRGGB14_1X14: + cr18 |= BIT_MIPI_DATA_FORMAT_RAW14; + break; + /* + * CSI-2 sources are supposed to use the 1X16 formats, but not + * all of them comply. Support both variants. + */ + 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; + } - switch (out_pix->pixelformat) { - case V4L2_PIX_FMT_Y10: - case V4L2_PIX_FMT_Y12: - case V4L2_PIX_FMT_SBGGR8: - case V4L2_PIX_FMT_SGBRG8: - case V4L2_PIX_FMT_SGRBG8: - case V4L2_PIX_FMT_SRGGB8: - case V4L2_PIX_FMT_SBGGR16: - case V4L2_PIX_FMT_SGBRG16: - case V4L2_PIX_FMT_SGRBG16: - case V4L2_PIX_FMT_SRGGB16: - dev_dbg(csi->dev, "%s: PIXEL_BIT\n", __func__); - cr1 |= BIT_PIXEL_BIT; - break; - } + switch (out_pix->pixelformat) { + case V4L2_PIX_FMT_Y10: + case V4L2_PIX_FMT_Y12: + case V4L2_PIX_FMT_SBGGR8: + case V4L2_PIX_FMT_SGBRG8: + case V4L2_PIX_FMT_SGRBG8: + case V4L2_PIX_FMT_SRGGB8: + case V4L2_PIX_FMT_SBGGR16: + case V4L2_PIX_FMT_SGBRG16: + case V4L2_PIX_FMT_SGRBG16: + case V4L2_PIX_FMT_SRGGB16: + dev_dbg(csi->dev, "%s: PIXEL_BIT\n", __func__); + cr1 |= BIT_PIXEL_BIT; + break; } imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
--- hi Laurent, thanks a lot for posting this series! First: I only test imx7-media-csi (csi bridge) because I run it on imx8mq. overall, I'm very happy with all of this and I get the same image out of it as I get with the mx6s_capture nxp driver. one issue I have is with is_csi2, so I post this patch that I need in order to test. It's obviously no solution, just to describe the issue: I'm not sure why but imx7_csi_pad_link_validate() isn't called in my case and is_csi2 doesn't get set, so I force it. Would it make sense to make a dts property for this? The other thing is that v4l2-ctl --set-fmt-video=width=1280,height=720,pixelformat=0 doesn't call the sensor drivers' set_fmt() pad function. That means that only the one mode I hard-code as default will work. instead I just see this: [ 742.977677] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' [ 742.977702] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack [ 742.977709] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) [ 742.977714] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' [ 742.977720] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' [ 742.977985] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture' [ 742.977992] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack [ 742.977997] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen) [ 742.978002] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi' [ 742.978008] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture' [ 742.978025] mc: media_release: Media Release Does anything come to your mind that would prevent the set_fmt call here? That's what the (nxp) mipi driver looks like I'm running here: https://source.puri.sm/martin.kepplinger/linux-next/-/blob/5.12-rc3/librem5__integration_byzantium/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c martin drivers/staging/media/imx/imx7-media-csi.c | 135 +++++++++------------ 1 file changed, 60 insertions(+), 75 deletions(-)