Hi Dafna,
Thanks for the patch.
On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
> Add more detailed documentation of the structs in rkisp1-common.h
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/staging/media/rkisp1/TODO | 1 -
> drivers/staging/media/rkisp1/rkisp1-common.h | 133 +++++++++++++++----
> 2 files changed, 106 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index bdb1b8f73556..f0c90d1c86a8 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -2,7 +2,6 @@
> * Fix checkpatch errors.
> * Review and comment every lock
> * Handle quantization
> -* Document rkisp1-common.h
> * streaming paths (mainpath and selfpath) check if the other path is streaming
> in several places of the code, review this, specially that it doesn't seem it
> supports streaming from both paths at the same time.
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 4185487c520c..47b03a1dfe1b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -73,8 +73,16 @@ enum rkisp1_isp_pad {
> };
>
> /*
> - * struct rkisp1_sensor_async - Sensor information
> - * @mbus: media bus configuration
> + * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier
> + * of the v4l2-async API.
> + *
> + * @asd: the async_subdev var for the sensor
I think you can remove "the" here, same for other fields in the rest of this patch.
> + * @lanes: number of lanes
> + * @mbus_type: type of bus (currently only CSI2 is supported)
> + * @mbus_flags: media bus (V4L2_MBUS_*) flags
> + * @sd: a pointer to the v4l2_subdev struct of the sensor
> + * @pixel_rate_ctrl: the pixel rate of the sensor, used to initialize the phy
> + * @dphy: a pointer to the phy
> */
Just a nit, could you align the comments? I think it is easier to read.
> struct rkisp1_sensor_async {
> struct v4l2_async_subdev asd;
> @@ -87,19 +95,16 @@ struct rkisp1_sensor_async {
> };
>
> /*
> - * struct rkisp1_isp - ISP sub-device
> + * struct rkisp1_isp - ISP subdev entity
> *
> - * See Cropping regions of ISP in rkisp1.c for details
> - * @sink_frm: input size, don't have to be equal to sensor size
> + * @sd: the v4l2_subdev var
> + * @pads: the media pads
> + * @pad_cfg: the pads configurations
> * @sink_fmt: input format
> - * @sink_crop: crop for sink pad
> * @src_fmt: output format
> - * @src_crop: output size
> * @ops_lock: ops serialization
> - *
> * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
> * @frame_sequence: used to synchronize frame_id between video devices.
> - * @quantization: output quantization
> */
> struct rkisp1_isp {
> struct v4l2_subdev sd;
> @@ -107,11 +112,20 @@ struct rkisp1_isp {
> struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
> const struct rkisp1_isp_mbus_info *sink_fmt;
> const struct rkisp1_isp_mbus_info *src_fmt;
> - struct mutex ops_lock;
> + struct mutex ops_lock; /* serialize the subdevice ops */
> bool is_dphy_errctrl_disabled;
> atomic_t frame_sequence;
> };
>
> +/*
> + * struct rkisp1_vdev_node - A Container for the video nodes of the video devices:
> + * params, stats, mainpath, selfpath
Or maybe just:
Container for the video devices nodes: params, stats, mainpath, selfpath
> + *
> + * @buf_queue: the queue of buffers.
> + * @vlock: the lock of the video node
> + * @vdev: the video node
> + * @pad: the media pad
> + */
> struct rkisp1_vdev_node {
> struct vb2_queue buf_queue;
> struct mutex vlock; /* ioctl serialization mutex */
> @@ -119,6 +133,15 @@ struct rkisp1_vdev_node {
> struct media_pad pad;
> };
>
> +/*
> + * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
> + * params, stats, mainpath, selfpath
> + *
> + * @vb: the vb2 buffer
> + * @queue: the entry of the buffer in the queue
> + * @buff_addr: the dma addresses of each plain, used only by the capture devices: selfpath, mainpath
s/plain/plane
> + * @vaddr: the virtual addresses, used only by the params and stats devices
maybe:
virtual addresses for buffers used by params and stats devices
I also noticed that params and stats only use vaddr[0], maybe we can remove the array and
replaced by a simple variable.
> + */
> struct rkisp1_buffer {
> struct vb2_v4l2_buffer vb;
> struct list_head queue;
> @@ -128,6 +151,9 @@ struct rkisp1_buffer {
> };
> };
>
> +/*
> + * A dummy buffer to write the next frame to in case there are no vb2 buffers available.
> + */
Could you please follow the same standard used in the rest of this patch?
> struct rkisp1_dummy_buffer {
> void *vaddr;
> dma_addr_t dma_addr;
> @@ -139,17 +165,28 @@ struct rkisp1_device;
> /*
> * struct rkisp1_capture - ISP capture video device
> *
> - * @pix.fmt: buffer format
> - * @pix.info: pixel information
> - * @pix.cfg: pixel configuration
> - *
> - * @buf.lock: lock to protect buf_queue
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
> + * @id: the id of the capture, one of RKISP1_SELFPATH, RKISP1_MAINPATH
> + * @ops: list of callbacks to configure the capture device.
> + * @config: a pointer to the list of registers to configure the capture format.
> + * @is_streaming: device is streaming
> + * @is_stopping: stop_streaming callback was called and the device is in the process of stopping
> + * the streaming.
> + * @done: when stop_streaming callback is called, the device waits for the next irq handler to stop
> + * the streaming from there by waiting on the 'done' wait queue.
s/to stop the streaming from there by waiting/to stop the streaming by waiting/
> + * If the irq handler is not called, the stream is stopped from the callback after timeout.
s/from/by
> + * @sp_y_stride: the selfpath allows to configure a y stride that is longer than the image width.
> + * @buf.lock: lock to protect buf.queue
> * @buf.queue: queued buffer list
> * @buf.dummy: dummy space to store dropped data
> *
> - * rkisp1 use shadowsock registers, so it need two buffer at a time
> + * rkisp1 uses shadow registers, so it needs two buffers at a time
> * @buf.curr: the buffer used for current frame
> * @buf.next: the buffer used for next frame
> + * @pix.cfg: pixel configuration
> + * @pix.info: a pointer to the v4l2_format_info of the pixel format
> + * @pix.fmt: buffer format
> */
> struct rkisp1_capture {
> struct rkisp1_vdev_node vnode;
> @@ -179,14 +216,18 @@ struct rkisp1_capture {
> /*
> * struct rkisp1_stats - ISP Statistics device
> *
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
> * @lock: locks the buffer list 'stat' and 'is_streaming'
> - * @stat: stats buffer list
> + * @stat: the queue of rkisp1_buffer
> + * @vdev_fmt: the v4l2_format
> + * @is_streaming: device is streaming
> */
> struct rkisp1_stats {
> struct rkisp1_vdev_node vnode;
> struct rkisp1_device *rkisp1;
>
> - spinlock_t lock; /* locks 'is_streaming', and 'stats' */
> + spinlock_t lock; /* locks the buffers list 'stats' and 'is_streaming' */
> struct list_head stat;
> struct v4l2_format vdev_fmt;
> bool is_streaming;
> @@ -195,14 +236,22 @@ struct rkisp1_stats {
> /*
> * struct rkisp1_params - ISP input parameters device
> *
> - * @cur_params: Current ISP parameters
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
> + * @config_lock: locks the buffer list 'params' and 'is_streaming'
> + * @params: the queue of rkisp1_buffer
> + * @cur_params: the first params values from userspace
> + * @vdev_fmt: the v4l2_format
> + * @is_streaming: device is streaming
> * @is_first_params: the first params should take effect immediately
> + * @quantization: the quantization configured on the isp's src pad
Just a note unrelated to this patch:
Maybe quantization could be moved to struct rkisp1_device, so it is accessible by all the entities
without the need to maintain copies around.
> + * @raw_type: the bayer pattern on the isp video sink pad
> */
> struct rkisp1_params {
> struct rkisp1_vdev_node vnode;
> struct rkisp1_device *rkisp1;
>
> - spinlock_t config_lock;
> + spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
> struct list_head params;
> struct rkisp1_params_cfg cur_params;
> struct v4l2_format vdev_fmt;
> @@ -213,6 +262,18 @@ struct rkisp1_params {
> enum rkisp1_fmt_raw_pat_type raw_type;
> };
>
> +/*
> + * struct rkisp1_resizer - Resizer subdev
> + *
> + * @sd: the v4l2_subdev var
> + * @id: the id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
> + * @rkisp1: pointer to the rkisp1 device
> + * @pads: the media pads
> + * @pad_cfg: the configurations for the pads
> + * @config: the set of registers to configure the resizer
> + * @pixel_enc: the pixel encoding of the resizer
> + * @ops_lock: a lock for the subdev ops
> + */
> struct rkisp1_resizer {
> struct v4l2_subdev sd;
> enum rkisp1_stream_id id;
> @@ -221,9 +282,12 @@ struct rkisp1_resizer {
> struct v4l2_subdev_pad_config pad_cfg[RKISP1_RSZ_PAD_MAX];
> const struct rkisp1_rsz_config *config;
> enum v4l2_pixel_encoding pixel_enc;
> - struct mutex ops_lock;
> + struct mutex ops_lock; /* serialize the subdevice ops */
> };
>
> +/*
> + * struct rkisp1_debug - values to be exposed on debugfs
> + */
> struct rkisp1_debug {
> struct dentry *debugfs_dir;
> unsigned long data_loss;
> @@ -237,12 +301,22 @@ struct rkisp1_debug {
> /*
> * struct rkisp1_device - ISP platform device
Missing new line here
> * @base_addr: base register address
> + * @irq: the irq number
> + * @dev: a pointer to the struct device
> + * @clk_size: the number of clocks
> + * @clks: the array of clocks
> + * @v4l2_dev: the v4l2_device
> + * @media_dev: the media_device
> + * @notifier: a notifier to register on the v4l2-async API to be notified on the sensor
> * @active_sensor: sensor in-use, set when streaming on
> * @isp: ISP sub-device
> - * @rkisp1_capture: capture video device
> + * @resizer_devs: the two resizer sub-devices
> + * @capture_devs: the two capture devices
> * @stats: ISP statistics output device
> * @params: ISP input parameters device
> - * @stream_lock: lock to serialize start/stop streaming in capture devices.
> + * @pipe: the media pipeline
> + * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
> + * @debug: the debug params to be exposed on debugfs
> */
> struct rkisp1_device {
> void __iomem *base_addr;
> @@ -260,16 +334,21 @@ struct rkisp1_device {
> struct rkisp1_stats stats;
> struct rkisp1_params params;
> struct media_pipeline pipe;
> - struct mutex stream_lock;
> + struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */
> struct rkisp1_debug debug;
> };
>
> /*
> - * struct rkisp1_isp_mbus_info - ISP pad format info
> + * struct rkisp1_isp_mbus_info - ISP media bus info
> *
> - * Translate mbus_code to hardware format values
> + * Translates mbus_code to hardware format values
> *
> - * @bus_width: used for parallel
> + * @mbus_code: the media bus code
> + * @pixel_enc: the pixel encoding
> + * @mipi_dt: the mipi data type
> + * @bus_width: the bus width
> + * @bayer_pat: the bayer pattern
> + * @direction: a bitmask of the flags indicating on which pad the format is supported on
> */
> struct rkisp1_isp_mbus_info {
> u32 mbus_code;
>
It would also be nice to add docs to the enums, macros and functions, but since this is an internal
header, it's fine, we can add them later.
With these changes:
Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
Thanks
Helen
