[PATCH 4/4] media: staging: rkisp1: improve documentation of rkisp1-common.h
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Wed Jul 15 15:57:02 EDT 2020
Hi
On 13.07.20 13:52, Helen Koike wrote:
> 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 at collabora.com>
>> ---
>> 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.
But quantization is only copied to the params entity so I don't think it should
be moved to rkisp1_device.
Thanks,
Dafna
>
>> + * @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 at collabora.com>
>
> Thanks
> Helen
>
More information about the Linux-rockchip
mailing list