Hi, Sakari,
> -----Original Message-----
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, October 20, 2017 4:15 AM
> To: Zhi, Yong <yong....@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux-
> foundation.org; Tomasz Figa <tf...@chromium.org>
> Subject: Re: [PATCH v4 12/12] intel-ipu3: imgu top level pci device
>
> On Tue, Oct 17, 2017 at 10:55:59PM -0500, Yong Zhi wrote:
> > This patch adds support for the Intel IPU v3 as found on Skylake and
> > Kaby Lake SoCs. The driver has a dependency on the firmware binary to
> > function properly.
> >
> > Signed-off-by: Yong Zhi <yong....@intel.com>
> > Signed-off-by: Tomasz Figa <tf...@chromium.org>
> > ---
> > drivers/media/pci/intel/ipu3/Kconfig | 17 +
> > drivers/media/pci/intel/ipu3/Makefile | 6 +
> > drivers/media/pci/intel/ipu3/ipu3.c | 882
> ++++++++++++++++++++++++++++++++++
> > drivers/media/pci/intel/ipu3/ipu3.h | 186 +++++++
> > 4 files changed, 1091 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3.h
> >
(snip)
> > +/*
> > + * imgu_mem2mem2_ops - used by v4l2 and vb2 */ static const struct
> > +ipu3_mem2mem2_ops imgu_mem2mem2_ops = {
> > + .s_stream = imgu_mem2mem2_s_stream,
>
> You have a single instance of this. How about just using
> imgu_mem2mem2_s_stream instead?
>
Yes, we can remove another level of indirectness.
(snip)
> > +++ b/drivers/media/pci/intel/ipu3/ipu3.h
> > @@ -0,0 +1,186 @@
> > +/*
> > + * Copyright (c) 2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef __IPU3_H
> > +#define __IPU3_H
> > +
> > +#include <linux/iova.h>
> > +#include <linux/pci.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include "ipu3-css.h"
> > +
> > +/*
> > + * The semantics of the driver is that whenever there is a buffer
> > +available in
> > + * master queue, the driver queues a buffer also to all other active nodes.
> > + * If user space hasn't provided a buffer to all other video nodes
> > +first,
> > + * the driver gets an internal dummy buffer and queues it.
> > + */
> > +#define IMGU_QUEUE_MASTER IPU3_CSS_QUEUE_IN
> > +#define IMGU_QUEUE_FIRST_INPUT IPU3_CSS_QUEUE_OUT
> > +#define IMGU_MAX_QUEUE_DEPTH (2 + 2)
> > +
> > +#define IMGU_NODE_IN 0 /* Input RAW image */
> > +#define IMGU_NODE_PARAMS 1 /* Input parameters */
> > +#define IMGU_NODE_OUT 2 /* Main output for still or
> video */
> > +#define IMGU_NODE_VF 3 /* Preview */
> > +#define IMGU_NODE_PV 4 /* Postview for still capture
> */
> > +#define IMGU_NODE_STAT_3A 5 /* 3A statistics */
> > +#define IMGU_NODE_STAT_DVS 6 /* DVS statistics */
> > +#define IMGU_NODE_STAT_LACE 7 /* Lace statistics */
> > +#define IMGU_NODE_NUM 8
> > +
> > +#define file_to_intel_ipu3_node(__file) \
> > + container_of(video_devdata(__file), struct imgu_video_device, vdev)
> > +
> > +#define IPU3_INPUT_MIN_WIDTH 0U
> > +#define IPU3_INPUT_MIN_HEIGHT 0U
> > +#define IPU3_INPUT_MAX_WIDTH 5120U
> > +#define IPU3_INPUT_MAX_HEIGHT 38404U
> > +#define IPU3_OUTPUT_MIN_WIDTH 2U
> > +#define IPU3_OUTPUT_MIN_HEIGHT 2U
> > +#define IPU3_OUTPUT_MAX_WIDTH 4480U
> > +#define IPU3_OUTPUT_MAX_HEIGHT 34004U
> > +
> > +struct ipu3_mem2mem2_buffer {
> > + /* Public fields */
> > + struct vb2_v4l2_buffer vbb; /* Must be the first field */
> > +
> > + /* Private fields */
> > + struct list_head list;
> > +};
> > +
> > +struct imgu_buffer {
> > + struct ipu3_mem2mem2_buffer m2m2_buf; /* Must be the first
> field */
> > + struct ipu3_css_buffer css_buf;
> > + struct ipu3_css_map map;
> > +};
> > +
> > +struct imgu_node_mapping {
> > + int css_queue;
> > + const char *name;
> > +};
> > +
> > +/**
> > + * struct imgu_video_device
> > + * each node registers as video device and maintains its
> > + * own vb2_queue.
> > + */
> > +struct imgu_video_device {
> > + const char *name;
> > + bool output; /* Frames to the driver? */
> > + bool immutable; /* Can not be enabled/disabled */
> > + bool enabled;
> > + int queued; /* Buffers already queued */
> > + struct v4l2_format vdev_fmt; /* Currently set format */
> > +
> > + /* Private fields */
> > + struct video_device vdev;
> > + struct media_pad vdev_pad;
> > + struct v4l2_mbus_framefmt pad_fmt;
> > + struct vb2_queue vbq;
> > + struct list_head buffers;
> > + /* Protect vb2_queue and vdev structs*/
> > + struct mutex lock;
> > + atomic_t sequence;
> > +};
> > +
> > +/**
> > + * struct ipu3_mem2mem2_device - mem2mem device
> > + * this is the top level helper struct used by parent PCI device
> > + * to bind everything together for media operations.
> > + */
> > +struct ipu3_mem2mem2_device {
>
> There's always 1:1 mapping between ipu3_mem2mem2_device and
> imgu_device.
> Could you merge the two?
>
You mean remove the hierarchy relationship between ipu3_mem2mem2_device and
imgu_video_device? in other word, remove imgu_video_device and let
ipu3_mem2mem2_device directly own nodes' member data(with redundancy removed)?
Just want to have a clear picture before attempt to change, thanks!!
> > + /* Public fields, fill before registering */
> > + const char *name;
> > + const char *model;
> > + struct device *dev;
> > + int num_nodes;
> > + struct imgu_video_device *nodes;
> > + struct device *vb2_alloc_dev;
> > + const struct ipu3_mem2mem2_ops *ops;
> > + const struct vb2_mem_ops *vb2_mem_ops;
> > + unsigned int buf_struct_size;
> > + bool streaming; /* Public read only */
> > + struct v4l2_ctrl_handler *ctrl_handler;
> > +
> > + /* Private fields */
> > + struct v4l2_device v4l2_dev;
> > + struct media_device media_dev;
> > + struct media_pipeline pipeline;
> > + struct v4l2_subdev subdev;
> > + struct media_pad *subdev_pads;
> > + struct v4l2_file_operations v4l2_file_ops; };
> > +
> > +/**
> > + * struct ipu3_mem2mem2_ops - mem2mem2 ops
> > + * defines driver specific callback APIs like
> > + * start stream.
> > + */
> > +struct ipu3_mem2mem2_ops {
> > + int (*s_stream)(struct ipu3_mem2mem2_device *m2m2_dev, int
> enable);
> > +};
> > +
> > +/*
> > + * imgu_device -- ImgU (Imaging Unit) driver */ struct imgu_device {
> > + struct pci_dev *pci_dev;
> > + void __iomem *base;
> > +
> > + /* Internally enabled queues */
> > + struct {
> > + struct ipu3_css_map dmap;
> > + struct ipu3_css_buffer
> dummybufs[IMGU_MAX_QUEUE_DEPTH];
> > + } queues[IPU3_CSS_QUEUES];
> > + struct imgu_video_device mem2mem2_nodes[IMGU_NODE_NUM];
> > + bool queue_enabled[IMGU_NODE_NUM];
> > +
> > + /* Delegate v4l2 support */
> > + struct ipu3_mem2mem2_device mem2mem2;
> > + /* MMU driver for css */
> > + struct device *mmu;
> > + struct iommu_domain *domain;
> > + struct iova_domain iova_domain;
> > + /* css - Camera Sub-System */
> > + struct ipu3_css css;
> > +
> > + /*
> > + * Coarse-grained lock to protect
> > + * m2m2_buf.list and css->queue
> > + */
> > + struct mutex lock;
> > + /* Forbit streaming and buffer queuing during system suspend. */
> > + struct mutex qbuf_lock;
> > + struct {
> > + struct v4l2_rect eff; /* effective resolution */
> > + struct v4l2_rect bds; /* bayer-domain scaled resolution*/
> > + struct v4l2_rect gdc; /* gdc output resolution */
> > + } rect;
> > + /* Indicate if system suspend take place while imgu is streaming. */
> > + bool suspend_in_stream;
> > + /* Used to wait for FW buffer queue drain. */
> > + wait_queue_head_t buf_drain_wq;
> > +};
> > +
> > +int imgu_node_to_queue(int node);
> > +int imgu_map_node(struct imgu_device *imgu, int css_queue); int
> > +imgu_queue_buffers(struct imgu_device *imgu, bool initial);
> > +
> > +int ipu3_v4l2_register(struct imgu_device *dev); int
> > +ipu3_v4l2_unregister(struct imgu_device *dev); void
> > +ipu3_v4l2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state
> > +state);
> > +
> > +#endif
> > --
> > 2.7.4
> >
>
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi
