[PATCH v2 3/4] media: imx-pxp: add i.MX Pixel Pipeline driver

[p.zabel@pengutronix.de (Philipp Zabel)]

On Wed, 2018-09-05 at 14:50 +0200, Hans Verkuil wrote: 
[...]
> > +config VIDEO_IMX_PXP
> > +	tristate "i.MX Pixel Pipeline (PXP)"
> > +	depends on VIDEO_DEV && VIDEO_V4L2 && (ARCH_MXC || COMPILE_TEST)
> > +	select VIDEOBUF2_DMA_CONTIG
> > +	select V4L2_MEM2MEM_DEV
> > +	help
> > +	  The i.MX Pixel Pipeline is a memory-to-memory engine for scaling,
> > +          color space conversion, and rotation.
> 
> Weird indentation.

Huh, a tab got replaced with spaces.

[...]
> > +/* Per queue */
> > +#define MEM2MEM_DEF_NUM_BUFS	VIDEO_MAX_FRAME
> > +/* In bytes, per queue */
> > +#define MEM2MEM_VID_MEM_LIMIT	(16 * 1024 * 1024)
> 
> These two defines are now unused and can be removed.

Will be gone in v3.

[...]
> > +static enum v4l2_ycbcr_encoding pxp_default_ycbcr_enc(struct pxp_ctx *ctx)
> > +{
> > +	if (ctx->xfer_func)
> > +		return V4L2_MAP_YCBCR_ENC_DEFAULT(ctx->colorspace);
> > +	else
> > +		return V4L2_YCBCR_ENC_DEFAULT;
> > +}
> > +
> > +static enum v4l2_quantization
> > +pxp_default_quant(struct pxp_ctx *ctx, u32 pixelformat,
> > +		  enum v4l2_ycbcr_encoding ycbcr_enc)
> > +{
> > +	bool is_rgb = !pxp_v4l2_pix_fmt_is_yuv(pixelformat);
> > +
> > +	if (ctx->xfer_func)
> 
> Why check for xfer_func? (same question for the previous function)

That way if userspace sets
	V4L2_XFER_FUNC_DEFAULT
	V4L2_YCBCR_ENC_DEFAULT
	V4L2_QUANTIZATION_DEFAULT
on the output queue, it will get
	V4L2_XFER_FUNC_DEFAULT
	V4L2_YCBCR_ENC_DEFAULT
	V4L2_QUANTIZATION_DEFAULT
on the capture queue.

If userspace sets xfer_func explicitly, it will get the explicit default
ycbcr_enc and quantization values.

I think I did this to make v4l2-compliance at some point, but it could
be that the explicit output->capture colorimetry copy for RGB->RGB and
YUV->YUV conversions has me covered now.

[...]
> > +static int pxp_remove(struct platform_device *pdev)
> > +{
> > +	struct pxp_dev *dev = platform_get_drvdata(pdev);
> > +
> > +	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
> > +	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> > +
> > +	clk_disable_unprepare(dev->clk);
> > +
> > +	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> > +	v4l2_m2m_release(dev->m2m_dev);
> > +	video_unregister_device(&dev->vfd);
> 
> Swap these two lines: first unreg the device, Then call m2m_release.

Ok, thank you.

regards
Philipp