Message-ID: <57ebeab6-0e98-cd12-9fe3-6a33c5ec0cad@gmail.com>
Date:   Mon, 13 Mar 2017 16:39:01 -0700
From:   Steve Longerbeam <slongerbeam@...il.com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>,
        Steve Longerbeam <slongerbeam@...il.com>
Cc:     mark.rutland@....com, andrew-ct.chen@...iatek.com,
        minghsiu.tsai@...iatek.com, sakari.ailus@...ux.intel.com,
        nick@...anahar.org, songjun.wu@...rochip.com, hverkuil@...all.nl,
        pavel@....cz, robert.jarzmik@...e.fr, devel@...verdev.osuosl.org,
        markus.heiser@...marIT.de,
        laurent.pinchart+renesas@...asonboard.com, shuah@...nel.org,
        geert@...ux-m68k.org, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, kernel@...gutronix.de, arnd@...db.de,
        mchehab@...nel.org, bparrot@...com, robh+dt@...nel.org,
        horms+renesas@...ge.net.au, tiffany.lin@...iatek.com,
        linux-arm-kernel@...ts.infradead.org,
        niklas.soderlund+renesas@...natech.se, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, jean-christophe.trotin@...com,
        p.zabel@...gutronix.de, fabio.estevam@....com, shawnguo@...nel.org,
        sudipm.mukherjee@...il.com
Subject: Re: [PATCH v5 00/39] i.MX Media Driver

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
>
>

Powered by blists - more mailing lists