| Message ID | 20170312202240.GT21222@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote: > On Sun, Mar 12, 2017 at 01:05:06PM -0700, Steve Longerbeam wrote: >> >> >> On 03/12/2017 12:57 PM, Russell King - ARM Linux wrote: >>> On Sat, Mar 11, 2017 at 04:30:53PM -0800, Steve Longerbeam wrote: >>>> If it's too difficult to get the imx219 csi-2 transmitter into the >>>> LP-11 state on power on, perhaps the csi-2 receiver can be a little >>>> more lenient on the transmitter and make the LP-11 timeout a warning >>>> instead of error-out. >>>> >>>> Can you try the attached change on top of the version 5 patchset? >>>> >>>> If that doesn't work then you're just going to have to fix the bug >>>> in imx219. >>> >>> That patch gets me past that hurdle, only to reveal that there's another >>> issue: >> >> Yeah, ipu_cpmem_set_image() failed because it doesn't recognize the >> bayer formats. Wait, didn't we fix this already? I've lost track. >> Ah, right, we were going to move this support into the IPUv3 driver, >> but in the meantime I think you had some patches to get around this. > > 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. I'll see if I have time to write that patch to IPUv3, but it's simple, in fact what you wrote below can be translate directly into ipu_cpmem_set_image(). There's a few other places bayer needs to be treated in IPUv3, but it should be obvious by grepping for the reference to pixel formats. Steve > > diff --git a/drivers/staging/media/imx/imx-smfc.c b/drivers/staging/media/imx/imx-smfc.c > index 313732201a52..4351c0365cf4 100644 > --- a/drivers/staging/media/imx/imx-smfc.c > +++ b/drivers/staging/media/imx/imx-smfc.c > @@ -234,11 +234,6 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) > buf1 = imx_media_dma_buf_get_next_queued(priv->out_ring); > priv->next = buf1; > > - image.phys0 = buf0->phys; > - image.phys1 = buf1->phys; > - ipu_cpmem_set_image(priv->smfc_ch, &image); > - > - > switch (image.pix.pixelformat) { > case V4L2_PIX_FMT_SBGGR8: > case V4L2_PIX_FMT_SGBRG8: > @@ -247,6 +242,10 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) > burst_size = 8; > passthrough = true; > passthrough_bits = 8; > + ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, image.rect.height); > + ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline); > + ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys); > + ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys); > break; > > case V4L2_PIX_FMT_SBGGR16: > @@ -256,9 +255,17 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) > burst_size = 4; > passthrough = true; > passthrough_bits = 16; > + ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, image.rect.height); > + ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline); > + ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys); > + ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys); > break; > > default: > + image.phys0 = buf0->phys; > + image.phys1 = buf1->phys; > + ipu_cpmem_set_image(priv->smfc_ch, &image); > + > burst_size = (outfmt->width & 0xf) ? 8 : 16; > > /* >
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.
On 03/13/2017 01:16 AM, 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. Oops, sorry missed that. I'll fix. > > 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. right, yeah that's a problem. > 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(). Ugh. > > 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. true. > > 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. Well, I will integrate your patch above. Thanks for doing this work for me. We do need to address the issues you brought up in ipu_cpmem at some point. Steve
diff --git a/drivers/staging/media/imx/imx-smfc.c b/drivers/staging/media/imx/imx-smfc.c index 313732201a52..4351c0365cf4 100644 --- a/drivers/staging/media/imx/imx-smfc.c +++ b/drivers/staging/media/imx/imx-smfc.c @@ -234,11 +234,6 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) buf1 = imx_media_dma_buf_get_next_queued(priv->out_ring); priv->next = buf1; - image.phys0 = buf0->phys; - image.phys1 = buf1->phys; - ipu_cpmem_set_image(priv->smfc_ch, &image); - - switch (image.pix.pixelformat) { case V4L2_PIX_FMT_SBGGR8: case V4L2_PIX_FMT_SGBRG8: @@ -247,6 +242,10 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) burst_size = 8; passthrough = true; passthrough_bits = 8; + ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, image.rect.height); + ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline); + ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys); + ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys); break; case V4L2_PIX_FMT_SBGGR16: @@ -256,9 +255,17 @@ static void imx_smfc_setup_channel(struct imx_smfc_priv *priv) burst_size = 4; passthrough = true; passthrough_bits = 16; + ipu_cpmem_set_resolution(priv->smfc_ch, image.rect.width, image.rect.height); + ipu_cpmem_set_stride(priv->smfc_ch, image.pix.bytesperline); + ipu_cpmem_set_buffer(priv->smfc_ch, 0, buf0->phys); + ipu_cpmem_set_buffer(priv->smfc_ch, 1, buf1->phys); break; default: + image.phys0 = buf0->phys; + image.phys1 = buf1->phys; + ipu_cpmem_set_image(priv->smfc_ch, &image); + burst_size = (outfmt->width & 0xf) ? 8 : 16; /*