Re: [PATCH v5 00/39] i.MX Media Driver
From: Steve Longerbeam
Date: Mon Mar 13 2017 - 19:39:18 EST
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