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:For the time being, I've restored the functionality along the same lines
On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote:There is more work, because the way you've merged my changes to
What I had was this patch for your v3. I never got to testing yourYou added the patches to this driver that adds the bayer support,
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.
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.
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.
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