| Message ID | 20170313093007.GD21222@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
er, I meant I will integrate this patch. And verify/fix possible breakage for non-bayer passthrough. Steve On 03/13/2017 02:30 AM, Russell King - ARM Linux wrote: > On Mon, Mar 13, 2017 at 08:16:25AM +0000, Russell King - ARM Linux wrote: >> On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote: >>> On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote: >>>> What I had was this patch for your v3. I never got to testing your >>>> v4 because of the LP-11 problem. >>>> >>>> In v5, you've changed to propagate the ipu_cpmem_set_image() error >>>> code to avoid the resulting corruption, but that leaves the other bits >>>> of this patch unaddressed, along my "media: imx: smfc: add support >>>> for bayer formats" patch. >>>> >>>> Your driver basically has no support for bayer formats. >>> You added the patches to this driver that adds the bayer support, >>> I don't think there is anything more required of the driver at this >>> point to support bayer, the remaining work needs to happen in the IPUv3 >>> driver. >> There is more work, because the way you've merged my changes to >> imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with >> respect to the burst size. >> >> You always set it to 8 or 16 depending on the width: >> >> burst_size = (image.pix.width & 0xf) ? 8 : 16; >> >> ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); >> >> and then you have my switch() statement which assigns burst_size. >> My _tested_ code removed the above, added the switch, which had >> a default case which reflected the above setting: >> >> default: >> burst_size = (outfmt->width & 0xf) ? 8 : 16; >> >> and then went on to set the burst size _after_ the switch statement: >> >> ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size); >> >> The effect is unchanged for non-bayer formats. For bayer formats, the >> burst size is determined by the bayer data size. >> >> So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the >> above is still required. >> >> I'm not convinced that fixing ipu_cpmem_set_image() is even the best >> solution - it's not as trivial as it looks on the surface: >> >> ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height); >> ipu_cpmem_set_stride(ch, pix->bytesperline); >> >> this is fine, it doesn't depend on the format. However, the next line: >> >> ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat)); >> >> does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it >> isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc. >> DRM knows nothing about bayer formats, there aren't fourcc codes in >> DRM for it. The result is that v4l2_pix_fmt_to_drm_fourcc() returns >> -EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt(). >> >> ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and >> it's a bug that this is not checked and propagated. If it is checked and >> propagated, then we need this to support bayer formats, and I don't see >> DRM people wanting bayer format fourcc codes added without there being >> a real DRM driver wanting to use them. >> >> Then there's the business of calculating the top-left offset of the image, >> which for bayer always needs to be an even number of pixels - as this >> function takes the top-left offset, it ought to respect it, but if it >> doesn't meet this criteria, what should it do? csi_idmac_setup_channel() >> always sets them to zero, but that's not really something that >> ipu_cpmem_set_image() should assume. > For the time being, I've restored the functionality along the same lines > as I originally had. This seems to get me working capture, but might > break non-bayer passthrough mode: > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index fc0036aa84d0..df336971a009 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > image.phys0 = phys[0]; > image.phys1 = phys[1]; > > - ret = ipu_cpmem_set_image(priv->idmac_ch, &image); > - if (ret) > - return ret; > - > - burst_size = (image.pix.width & 0xf) ? 8 : 16; > - > - ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); > - > /* > * Check for conditions that require the IPU to handle the > * data internally as generic data, aka passthrough mode: > @@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > passthrough_bits = 16; > break; > default: > + burst_size = (image.pix.width & 0xf) ? 8 : 16; > passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 && > sensor_ep->bus.parallel.bus_width >= 16); > passthrough_bits = 16; > break; > } > > - if (passthrough) > + if (passthrough) { > + ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width, > + image.rect.height); > + ipu_cpmem_set_stride(priv->idmac_ch, image.pix.bytesperline); > + ipu_cpmem_set_buffer(priv->idmac_ch, 0, image.phys0); > + ipu_cpmem_set_buffer(priv->idmac_ch, 1, image.phys1); > + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); > ipu_cpmem_set_format_passthrough(priv->idmac_ch, > passthrough_bits); > + } else { > + ret = ipu_cpmem_set_image(priv->idmac_ch, &image); > + if (ret) > + return ret; > + > + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); > + } > > /* > * Set the channel for the direct CSI-->memory via SMFC > >
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index fc0036aa84d0..df336971a009 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) image.phys0 = phys[0]; image.phys1 = phys[1]; - ret = ipu_cpmem_set_image(priv->idmac_ch, &image); - if (ret) - return ret; - - burst_size = (image.pix.width & 0xf) ? 8 : 16; - - ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); - /* * Check for conditions that require the IPU to handle the * data internally as generic data, aka passthrough mode: @@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) passthrough_bits = 16; break; default: + burst_size = (image.pix.width & 0xf) ? 8 : 16; passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 && sensor_ep->bus.parallel.bus_width >= 16); passthrough_bits = 16; break; } - if (passthrough) + if (passthrough) { + ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width, + image.rect.height); + ipu_cpmem_set_stride(priv->idmac_ch, image.pix.bytesperline); + ipu_cpmem_set_buffer(priv->idmac_ch, 0, image.phys0); + ipu_cpmem_set_buffer(priv->idmac_ch, 1, image.phys1); + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); ipu_cpmem_set_format_passthrough(priv->idmac_ch, passthrough_bits); + } else { + ret = ipu_cpmem_set_image(priv->idmac_ch, &image); + if (ret) + return ret; + + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); + } /* * Set the channel for the direct CSI-->memory via SMFC