Hi, Sakari, > -----Original Message----- > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Sakari Ailus > Sent: Friday, October 20, 2017 4:15 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; Zheng, Jian > Xu <jian.xu.zheng@xxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; > arnd@xxxxxxxx; hch@xxxxxx; robin.murphy@xxxxxxx; iommu@lists.linux- > foundation.org; Tomasz Figa <tfiga@xxxxxxxxxxxx> > 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.zhi@xxxxxxxxx> > > Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > --- > > 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.ailus@xxxxxx
