Hi Hans,
On Mon, 2018-09-03 at 14:55 +0200, Hans Verkuil wrote:
> Hi Philipp,
>
> Apologies for the delay, but the Request API took most of my time.
Understandable. Also, I expect I'll profit from that work when adding
CODA960 JPEG support. So I have no reason to complain.
> But I finally got around to it:
>
> On 08/10/2018 05:18 PM, Philipp Zabel wrote:
> > Add a V4L2 mem-to-mem scaler/CSC driver for the Pixel Pipeline (PXP)
> > version found on i.MX6ULL SoCs. A similar variant is used on i.MX7D.
> >
> > Since this driver only uses the legacy pipeline, it should be reasonably
> > easy to extend it to work with the older PXP versions found on i.MX6UL,
> > i.MX6SX, i.MX6SL, i.MX28, and i.MX23.
> >
> > The driver supports scaling and colorspace conversion. There is
> > currently no support for rotation, alpha-blending, and the LUTs.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > ---
> > drivers/media/platform/Kconfig | 9 +
> > drivers/media/platform/Makefile | 2 +
> > drivers/media/platform/imx-pxp.c | 1455 ++++++++++++++++++++++++++
> > drivers/media/platform/imx-pxp.h | 1685 ++++++++++++++++++++++++++++++
>
> Missing MAINTAINERS entry.
I'll add one for v2.
[...]
> > +static void pxp_setup_csc(struct pxp_ctx *ctx)
> > +{
> > + struct pxp_dev *dev = ctx->dev;
> > +
> > + if (pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) &&
> > + !pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_DST].fmt->fourcc)) {
[...]
> > + if (ctx->ycbcr_enc == V4L2_YCBCR_ENC_601 &&
> > + ctx->quant == V4L2_QUANTIZATION_FULL_RANGE)
> > + csc1_coef = csc1_coef_bt601_full;
> > + else
> > + csc1_coef = csc1_coef_bt601_lim;
>
> This is weird: setting ycbcr_enc to V4L2_YCBCR_ENC_709 would result in
> limited range BT601.
>
> > +
> > + writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
> > + writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
> > + writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
>
> No support for Rec. 709?
Will add coefficients for Rec. 709.
> > + } else {
> > + writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + HW_PXP_CSC1_COEF0);
> > + }
> > +
> > + if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) &&
> > + pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_DST].fmt->fourcc)) {
[...]
> > + if (ctx->ycbcr_enc == V4L2_YCBCR_ENC_601 &&
> > + ctx->quant == V4L2_QUANTIZATION_FULL_RANGE) {
>
> This makes no sense. ctx->ycbcr_enc and ctx->quant refer to the output queue,
> i.e. the RGB side. ycbcr_enc is ignored for RGB and quant is in practice always
> full range. While you can have limited range RGB as well, that's not however
> what you are trying to implement here.
>
> The problem is that you cannot at the moment specify what ycbcr_enc etc.
> format you want for the capture queue. See below for a link to an RFC to add
> support for this.
Right. I'll split ycbcr_enc and quantization into per-queue settings for
v2 and revive your RFC to allow configuring capture queue colorimetry.
[...]
> > +/*
> > + * video ioctls
> > + */
> > +static int vidioc_querycap(struct file *file, void *priv,
> > + struct v4l2_capability *cap)
> > +{
> > + strncpy(cap->driver, MEM2MEM_NAME, sizeof(cap->driver) - 1);
> > + strncpy(cap->card, MEM2MEM_NAME, sizeof(cap->card) - 1);
>
> Use strlcpy.
Ok.
> > + snprintf(cap->bus_info, sizeof(cap->bus_info),
> > + "platform:%s", MEM2MEM_NAME);
> > + cap->device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING;
> > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>
> Please set the device_caps field of video_device, and then you can drop
> these two lines since the core will fill this in for you.
Ok.
[...]
> > +static int vidioc_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
> > +{
> > + struct vb2_queue *vq;
> > + struct pxp_q_data *q_data;
> > +
> > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > + if (!vq)
> > + return -EINVAL;
> > +
> > + q_data = get_q_data(ctx, f->type);
> > +
> > + f->fmt.pix.width = q_data->width;
> > + f->fmt.pix.height = q_data->height;
> > + f->fmt.pix.field = V4L2_FIELD_NONE;
> > + f->fmt.pix.pixelformat = q_data->fmt->fourcc;
> > + f->fmt.pix.bytesperline = q_data->bytesperline;
> > + f->fmt.pix.sizeimage = q_data->sizeimage;
> > + f->fmt.pix.colorspace = ctx->colorspace;
> > + f->fmt.pix.xfer_func = ctx->xfer_func;
> > + f->fmt.pix.ycbcr_enc = ctx->ycbcr_enc;
> > + f->fmt.pix.quantization = ctx->quant;
>
> Since you do colorspace conversion, these can't be the same for
> both output and capture.
>
> colorspace and xfer_func will be the same (those are not converted),
> but ycbcr_enc should be set to 0 for RGB and quantization range
> will be different as well (either set to 0 for capture, or set
> to FULL for RGB capture and LIMITED for YUV).
>
> Please note that it is not possible at the moment to request
> different values for these four field when capturing: they are currently
> set by the driver, and userspace cann't request something else.
>
> There is an old RFC to add support for this:
>
> https://patchwork.linuxtv.org/patch/28847/
>
> This driver might be a good opportunity to resurrect this RFC patch.
Yes, I'll fix these to defaults for v2 and add a new patch to enable
capture colorimetry configuration as per this RFC on top.
[...]
> > +static int vidioc_try_fmt_vid_out(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct pxp_fmt *fmt;
> > + struct pxp_ctx *ctx = file2ctx(file);
> > +
> > + fmt = find_format(f);
> > + if (!fmt) {
> > + f->fmt.pix.pixelformat = formats[0].fourcc;
> > + fmt = find_format(f);
> > + }
> > + if (!(fmt->types & MEM2MEM_OUTPUT)) {
> > + v4l2_err(&ctx->dev->v4l2_dev,
> > + "Fourcc format (0x%08x) invalid.\n",
> > + f->fmt.pix.pixelformat);
> > + return -EINVAL;
> > + }
> > +
> > + if (!f->fmt.pix.colorspace)
> > + f->fmt.pix.colorspace = V4L2_COLORSPACE_REC709;
>
> REC709 implies that ycbcr_enc also uses REC709. But you only support
> 601. To make this a practical driver you want to support both 601
> and 709.
Ack.
> Does this hardware also support gamma tables? Without it you can't
> convert between different transfer functions. Just curious if the hw
> can do this at all.
There is, but it is kind of useless for my purposes. It is located at
the end of the pipeline and can do RGB/YUV565 -> RGB/YUV565 lookups at
most (implemented as 16 KiB cache into a 128 KiB table in memory).
So no scaling/blending in linear space, and no useful conversion between
transfer functions.
[...]
> > +static int pxp_queue_setup(struct vb2_queue *vq,
> > + unsigned int *nbuffers, unsigned int *nplanes,
> > + unsigned int sizes[], struct device *alloc_devs[])
> > +{
> > + struct pxp_ctx *ctx = vb2_get_drv_priv(vq);
> > + struct pxp_q_data *q_data;
> > + unsigned int size, count = *nbuffers;
> > +
> > + q_data = get_q_data(ctx, vq->type);
> > +
> > + size = q_data->sizeimage;
> > +
> > + while (size * count > MEM2MEM_VID_MEM_LIMIT)
> > + (count)--;
>
> Why do this? It never made sense to me. If you try to allocate more memory then
> there is, you get -ENOMEM, which is just fine.
>
> I'd drop this.
This is copied from vim2m. I'll remove it.
[...]
> > +static int pxp_probe(struct platform_device *pdev)
> > +{
[...]
> > + /* Enable clocks and dump registers */
> > + clk_prepare_enable(dev->clk);
> > + pxp_soft_reset(dev);
> > +
> > + spin_lock_init(&dev->irqlock);
> > +
> > + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > + if (ret)
> > + return ret;
> > +
> > + atomic_set(&dev->num_inst, 0);
> > + mutex_init(&dev->dev_mutex);
> > +
> > + dev->vfd = pxp_videodev;
> > + vfd = &dev->vfd;
> > + vfd->lock = &dev->dev_mutex;
> > + vfd->v4l2_dev = &dev->v4l2_dev;
> > +
> > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
>
> Please move this down until after v4l2_m2m_init: that should be done
> before you register the video device.
Yes, and I'll also fix the clock not being disabled on error. Thank you
for the review!
regards
Philipp
