diff mbox series

[v5,08/14] staging: media: starfive: Add for StarFive ISP 3A SC

Message ID 20240709083824.430473-9-changhuang.liang@starfivetech.com
State New
Headers show
Series Add ISP 3A for StarFive | expand

Commit Message

Changhuang Liang July 9, 2024, 8:38 a.m. UTC
Register ISP 3A "capture_scd" video device to receive statistics
collection data.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../staging/media/starfive/camss/stf-buffer.h |   1 +
 .../staging/media/starfive/camss/stf-camss.c  |  15 ++
 .../media/starfive/camss/stf-capture.c        |  21 ++-
 .../media/starfive/camss/stf-isp-hw-ops.c     |  66 ++++++++
 .../staging/media/starfive/camss/stf-isp.h    |  23 +++
 .../staging/media/starfive/camss/stf-video.c  | 146 +++++++++++++++++-
 .../staging/media/starfive/camss/stf-video.h  |   1 +
 7 files changed, 264 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi July 10, 2024, 11:57 a.m. UTC | #1
Hi Changhuang

On Tue, Jul 09, 2024 at 01:38:18AM GMT, Changhuang Liang wrote:
> Register ISP 3A "capture_scd" video device to receive statistics
> collection data.
>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../staging/media/starfive/camss/stf-buffer.h |   1 +
>  .../staging/media/starfive/camss/stf-camss.c  |  15 ++
>  .../media/starfive/camss/stf-capture.c        |  21 ++-
>  .../media/starfive/camss/stf-isp-hw-ops.c     |  66 ++++++++
>  .../staging/media/starfive/camss/stf-isp.h    |  23 +++
>  .../staging/media/starfive/camss/stf-video.c  | 146 +++++++++++++++++-
>  .../staging/media/starfive/camss/stf-video.h  |   1 +
>  7 files changed, 264 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/starfive/camss/stf-buffer.h b/drivers/staging/media/starfive/camss/stf-buffer.h
> index 9d1670fb05ed..727d00617448 100644
> --- a/drivers/staging/media/starfive/camss/stf-buffer.h
> +++ b/drivers/staging/media/starfive/camss/stf-buffer.h
> @@ -23,6 +23,7 @@ enum stf_v_state {
>  struct stfcamss_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	dma_addr_t addr[2];
> +	void *vaddr;
>  	struct list_head queue;
>  };
>
> diff --git a/drivers/staging/media/starfive/camss/stf-camss.c b/drivers/staging/media/starfive/camss/stf-camss.c
> index fecd3e67c7a1..fafa3ce2f6da 100644
> --- a/drivers/staging/media/starfive/camss/stf-camss.c
> +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> @@ -8,6 +8,7 @@
>   *
>   * Author: Jack Zhu <jack.zhu@starfivetech.com>
>   * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> + * Author: Keith Zhao <keith.zhao@starfivetech.com>
>   *
>   */
>  #include <linux/module.h>
> @@ -126,6 +127,7 @@ static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
>  static int stfcamss_register_devs(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>  	int ret;
>
> @@ -150,8 +152,18 @@ static int stfcamss_register_devs(struct stfcamss *stfcamss)
>
>  	cap_yuv->video.source_subdev = &isp_dev->subdev;
>
> +	ret = media_create_pad_link(&isp_dev->subdev.entity, STF_ISP_PAD_SRC_SCD,
> +				    &cap_scd->video.vdev.entity, 0, 0);
> +	if (ret)
> +		goto err_rm_links0;

or just 'err_rm_links'

> +
> +	cap_scd->video.source_subdev = &isp_dev->subdev;
> +
>  	return ret;

here you can return 0

>
> +err_rm_links0:
> +	media_entity_remove_links(&isp_dev->subdev.entity);
> +	media_entity_remove_links(&cap_yuv->video.vdev.entity);
>  err_cap_unregister:
>  	stf_capture_unregister(stfcamss);
>  err_isp_unregister:
> @@ -163,10 +175,12 @@ static int stfcamss_register_devs(struct stfcamss *stfcamss)
>  static void stfcamss_unregister_devs(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>
>  	media_entity_remove_links(&isp_dev->subdev.entity);
>  	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> +	media_entity_remove_links(&cap_scd->video.vdev.entity);
>
>  	stf_isp_unregister(&stfcamss->isp_dev);
>  	stf_capture_unregister(stfcamss);
> @@ -436,5 +450,6 @@ module_platform_driver(stfcamss_driver);
>
>  MODULE_AUTHOR("Jack Zhu <jack.zhu@starfivetech.com>");
>  MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
> +MODULE_AUTHOR("Keith Zhao <keith.zhao@starfivetech.com>");
>  MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/media/starfive/camss/stf-capture.c b/drivers/staging/media/starfive/camss/stf-capture.c
> index 75f6ef405e61..328b8c6e351d 100644
> --- a/drivers/staging/media/starfive/camss/stf-capture.c
> +++ b/drivers/staging/media/starfive/camss/stf-capture.c
> @@ -12,6 +12,7 @@
>  static const char * const stf_cap_names[] = {
>  	"capture_raw",
>  	"capture_yuv",
> +	"capture_scd",
>  };
>
>  static const struct stfcamss_format_info stf_wr_fmts[] = {
> @@ -55,6 +56,14 @@ static const struct stfcamss_format_info stf_isp_fmts[] = {
>  	},
>  };
>
> +/* 3A Statistics Collection Data */
> +static const struct stfcamss_format_info stf_isp_scd_fmts[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_METADATA_FIXED,
> +		.pixelformat = V4L2_META_FMT_STF_ISP_STAT_3A,
> +	},
> +};
> +
>  static inline struct stf_capture *to_stf_capture(struct stfcamss_video *video)
>  {
>  	return container_of(video, struct stf_capture, video);
> @@ -84,6 +93,8 @@ static void stf_init_addrs(struct stfcamss_video *video)
>  		stf_set_raw_addr(video->stfcamss, addr0);
>  	else if (cap->type == STF_CAPTURE_YUV)
>  		stf_set_yuv_addr(video->stfcamss, addr0, addr1);
> +	else
> +		stf_set_scd_addr(video->stfcamss, addr0, addr1, TYPE_AWB);
>  }
>
>  static void stf_cap_s_cfg(struct stfcamss_video *video)
> @@ -227,18 +238,24 @@ static void stf_capture_init(struct stfcamss *stfcamss, struct stf_capture *cap)
>  	INIT_LIST_HEAD(&cap->buffers.ready_bufs);
>  	spin_lock_init(&cap->buffers.lock);
>
> -	cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	cap->video.stfcamss = stfcamss;
>  	cap->video.bpl_alignment = 16 * 8;
>
>  	if (cap->type == STF_CAPTURE_RAW) {
> +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		cap->video.formats = stf_wr_fmts;
>  		cap->video.nformats = ARRAY_SIZE(stf_wr_fmts);
>  		cap->video.bpl_alignment = 8;
>  	} else if (cap->type == STF_CAPTURE_YUV) {
> +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		cap->video.formats = stf_isp_fmts;
>  		cap->video.nformats = ARRAY_SIZE(stf_isp_fmts);
>  		cap->video.bpl_alignment = 1;
> +	} else {
> +		cap->video.type = V4L2_BUF_TYPE_META_CAPTURE;
> +		cap->video.formats = stf_isp_scd_fmts;
> +		cap->video.nformats = ARRAY_SIZE(stf_isp_scd_fmts);
> +		cap->video.bpl_alignment = 16 * 8;
>  	}
>  }
>
> @@ -362,9 +379,11 @@ void stf_capture_unregister(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_raw = &stfcamss->captures[STF_CAPTURE_RAW];
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>
>  	stf_capture_unregister_one(cap_raw);
>  	stf_capture_unregister_one(cap_yuv);
> +	stf_capture_unregister_one(cap_scd);
>  }
>
>  int stf_capture_register(struct stfcamss *stfcamss,
> diff --git a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> index 6b3966ca18bf..3b18d09f2cc6 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> @@ -451,11 +451,57 @@ void stf_set_yuv_addr(struct stfcamss *stfcamss,
>  	stf_isp_reg_write(stfcamss, ISP_REG_UV_PLANE_START_ADDR, uv_addr);
>  }
>
> +static enum stf_isp_type_scd stf_isp_get_scd_type(struct stfcamss *stfcamss)
> +{
> +	int val;
> +
> +	val = stf_isp_reg_read(stfcamss, ISP_REG_SC_CFG_1);
> +	return (enum stf_isp_type_scd)(val & ISP_SC_SEL_MASK) >> 30;
> +}

So far used by a single caller, could be inlined

> +
> +void stf_set_scd_addr(struct stfcamss *stfcamss,
> +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> +		      enum stf_isp_type_scd type_scd)
> +{
> +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> +			    SEL_TYPE(type_scd));
> +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> +}
> +
> +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void *vaddr)
> +{
> +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> +	u32 i;
> +
> +	for (i = 0; i < 64; i++, reg_addr += 4)
> +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);

If you have a contigous memory space to read, could memcpy_fromio() help
instead of going through 64 reads ?

> +}
> +
> +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> +			      enum stf_isp_type_scd *type_scd)
> +{
> +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> +
> +	*type_scd = stf_isp_get_scd_type(stfcamss);
> +	if (*type_scd == TYPE_AWB) {
> +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> +		*type_scd = TYPE_OECF;
> +	} else {
> +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> +		*type_scd = TYPE_AWB;

Is this correct ? Why are you overwriting the value read from HW that
indicates AE/AF stats with AWB ones ?

> +	}
> +}
> +
>  irqreturn_t stf_line_irq_handler(int irq, void *priv)
>  {
>  	struct stfcamss *stfcamss = priv;
>  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stfcamss_buffer *change_buf;
> +	enum stf_isp_type_scd type_scd;
> +	u32 value;
>  	u32 status;
>
>  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> @@ -467,6 +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
>  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
>  							 change_buf->addr[1]);
>  			}
> +
> +			value = stf_isp_reg_read(stfcamss, ISP_REG_CSI_MODULE_CFG);
> +			if (value & CSI_SC_EN) {
> +				change_buf = stf_change_buffer(&cap_scd->buffers);
> +				if (change_buf) {
> +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> +							  &type_scd);
> +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> +							 change_buf->addr[1], type_scd);

Sorry if I'm un-familiar with the HW but this seems to be the
line-interrupt. Are you swapping buffers every line or it's just that
you have a single line irq for the stats ?

> +				}
> +			}
>  		}
>
>  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS,
> @@ -485,6 +542,7 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
>  {
>  	struct stfcamss *stfcamss = priv;
>  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stfcamss_buffer *ready_buf;
>  	u32 status;
>
> @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
>  				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  		}
>
> +		if (status & ISPC_SC) {
> +			ready_buf = stf_buf_done(&cap_scd->buffers);
> +			if (ready_buf) {
> +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> +				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +			}
> +		}
> +
>  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
>  				  (status & ~ISPC_INT_ALL_MASK) |
>  				  ISPC_ISP | ISPC_CSI | ISPC_SC);
> diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
> index fcda0502e3b0..0af7b367e57a 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp.h
> +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> @@ -10,6 +10,7 @@
>  #ifndef STF_ISP_H
>  #define STF_ISP_H
>
> +#include <linux/jh7110-isp.h>
>  #include <media/v4l2-subdev.h>
>
>  #include "stf-video.h"
> @@ -107,6 +108,12 @@
>  #define Y_COOR(y)				((y) << 16)
>  #define X_COOR(x)				((x) << 0)
>
> +#define ISP_REG_SCD_CFG_0			0x098
> +
> +#define ISP_REG_SC_CFG_1			0x0bc
> +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> +#define SEL_TYPE(n)				((n) << 30)
> +
>  #define ISP_REG_LCCF_CFG_2			0x0e0
>  #define ISP_REG_LCCF_CFG_3			0x0e4
>  #define ISP_REG_LCCF_CFG_4			0x0e8
> @@ -305,6 +312,10 @@
>  #define DNRM_F(n)				((n) << 16)
>  #define CCM_M_DAT(n)				((n) << 0)
>
> +#define ISP_REG_YHIST_CFG_4			0xcd8
> +
> +#define ISP_REG_YHIST_ACC_0			0xd00
> +
>  #define ISP_REG_GAMMA_VAL0			0xe00
>  #define ISP_REG_GAMMA_VAL1			0xe04
>  #define ISP_REG_GAMMA_VAL2			0xe08
> @@ -389,6 +400,15 @@
>  #define IMAGE_MAX_WIDTH				1920
>  #define IMAGE_MAX_HEIGH				1080
>
> +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))

Should this be in the uAPI header as it is useful to userspace as well ?

you could:

struct jh7110_isp_sc_buffer {
	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
	__u32 reserv0[33];
	__u32 bright_sc[4096];
	__u32 reserv1[96];
	__u32 ae_hist_y[128];
	__u32 reserv2[511];
	__u16 flag;
};

ofc if the size is made part of the uAPI you need a more proper name
such as JH7110_ISP_YHIST_SIZE

> +
> +enum stf_isp_type_scd {
> +	TYPE_DEC = 0,
> +	TYPE_OBC,
> +	TYPE_OECF,
> +	TYPE_AWB,
> +};
> +
>  /* pad id for media framework */
>  enum stf_isp_pad_id {
>  	STF_ISP_PAD_SINK = 0,
> @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev *isp_dev);
>
>  void stf_set_yuv_addr(struct stfcamss *stfcamss,
>  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> +void stf_set_scd_addr(struct stfcamss *stfcamss,
> +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> +		      enum stf_isp_type_scd type_scd);
>
>  #endif /* STF_ISP_H */
> diff --git a/drivers/staging/media/starfive/camss/stf-video.c b/drivers/staging/media/starfive/camss/stf-video.c
> index 989b5e82bae9..2203605ec9c7 100644
> --- a/drivers/staging/media/starfive/camss/stf-video.c
> +++ b/drivers/staging/media/starfive/camss/stf-video.c
> @@ -125,6 +125,14 @@ static int stf_video_init_format(struct stfcamss_video *video)
>  	return 0;
>  }
>
> +static int stf_video_scd_init_format(struct stfcamss_video *video)

Make it void if it can't fail (see below)

> +{
> +	video->active_fmt.fmt.meta.dataformat = video->formats[0].pixelformat;
> +	video->active_fmt.fmt.meta.buffersize = sizeof(struct jh7110_isp_sc_buffer);
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Video queue operations
>   */
> @@ -330,6 +338,78 @@ static const struct vb2_ops stf_video_vb2_q_ops = {
>  	.stop_streaming  = video_stop_streaming,
>  };
>
> +static int video_scd_queue_setup(struct vb2_queue *q,
> +				 unsigned int *num_buffers,
> +				 unsigned int *num_planes,
> +				 unsigned int sizes[],
> +				 struct device *alloc_devs[])
> +{
> +	if (*num_planes)
> +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL : 0;
> +
> +	*num_planes = 1;
> +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> +
> +	return 0;
> +}
> +
> +static int video_scd_buf_init(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> +	dma_addr_t *paddr;
> +
> +	paddr = vb2_plane_cookie(vb, 0);
> +	buffer->addr[0] = *paddr;
> +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;

Interesting, I don't see many users of vb2_plane_cookie() in mainline
and I'm not sure what this gives you as you use it to program the
following registers:

	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);


> +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> +
> +	return 0;
> +}
> +
> +static int video_scd_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +
> +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> +		return -EINVAL;
> +
> +	vb2_set_plane_payload(vb, 0, sizeof(struct jh7110_isp_sc_buffer));
> +
> +	vbuf->field = V4L2_FIELD_NONE;

is this necessary ?

> +
> +	return 0;
> +}
> +
> +static int video_scd_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> +
> +	video->ops->start_streaming(video);
> +
> +	return 0;
> +}
> +
> +static void video_scd_stop_streaming(struct vb2_queue *q)
> +{
> +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> +
> +	video->ops->stop_streaming(video);
> +
> +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> +}
> +
> +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> +	.queue_setup     = video_scd_queue_setup,
> +	.wait_prepare    = vb2_ops_wait_prepare,
> +	.wait_finish     = vb2_ops_wait_finish,
> +	.buf_init        = video_scd_buf_init,
> +	.buf_prepare     = video_scd_buf_prepare,
> +	.buf_queue       = video_buf_queue,
> +	.start_streaming = video_scd_start_streaming,
> +	.stop_streaming  = video_scd_stop_streaming,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 ioctls
>   */
> @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops stf_vid_ioctl_ops = {
>  	.vidioc_streamoff               = vb2_ioctl_streamoff,
>  };
>
> +static int video_scd_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> +	struct stfcamss_video *video = video_drvdata(file);
> +	struct v4l2_meta_format *meta = &f->fmt.meta;
> +
> +	if (f->type != video->type)
> +		return -EINVAL;
> +
> +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> +	.vidioc_querycap                = video_querycap,
> +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> +	.vidioc_streamon                = vb2_ioctl_streamon,
> +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 file operations
>   */
> @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
>  	struct stfcamss_video *video = video_get_drvdata(vdev);
>  	int ret;
>
> +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> +		return 0;
> +
>  	ret = stf_video_check_format(video);
>
>  	return ret;
> @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video *video,
>  	q = &video->vb2_q;
>  	q->drv_priv = video;
>  	q->mem_ops = &vb2_dma_contig_memops;
> -	q->ops = &stf_video_vb2_q_ops;
> +
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		q->ops = &stf_video_vb2_q_ops;
> +	else
> +		q->ops = &stf_video_scd_vb2_q_ops;
>  	q->type = video->type;
>  	q->io_modes = VB2_DMABUF | VB2_MMAP;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video *video,
>  		goto err_mutex_destroy;
>  	}
>
> -	ret = stf_video_init_format(video);
> -	if (ret < 0) {
> -		dev_err(video->stfcamss->dev,
> -			"Failed to init format: %d\n", ret);
> -		goto err_media_cleanup;
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +		ret = stf_video_init_format(video);

I don't think this can fail

> +		if (ret < 0) {
> +			dev_err(video->stfcamss->dev,
> +				"Failed to init format: %d\n", ret);
> +			goto err_media_cleanup;
> +		}
> +		vdev->ioctl_ops = &stf_vid_ioctl_ops;
> +		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
> +	} else {
> +		ret = stf_video_scd_init_format(video);

This can't fail as noted above

> +		if (ret < 0) {
> +			dev_err(video->stfcamss->dev,
> +				"Failed to init format: %d\n", ret);
> +			goto err_media_cleanup;
> +		}
> +		vdev->ioctl_ops = &stf_vid_scd_ioctl_ops;
> +		vdev->device_caps = V4L2_CAP_META_CAPTURE;
>  	}
>
>  	vdev->fops = &stf_vid_fops;
> -	vdev->ioctl_ops = &stf_vid_ioctl_ops;
> -	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	vdev->device_caps |= V4L2_CAP_STREAMING;
>  	vdev->entity.ops = &stf_media_ops;
>  	vdev->vfl_dir = VFL_DIR_RX;
>  	vdev->release = stf_video_release;
> diff --git a/drivers/staging/media/starfive/camss/stf-video.h b/drivers/staging/media/starfive/camss/stf-video.h
> index 59799b65cbe5..53a1cf4e59b7 100644
> --- a/drivers/staging/media/starfive/camss/stf-video.h
> +++ b/drivers/staging/media/starfive/camss/stf-video.h
> @@ -37,6 +37,7 @@ enum stf_v_line_id {
>  enum stf_capture_type {
>  	STF_CAPTURE_RAW = 0,
>  	STF_CAPTURE_YUV,
> +	STF_CAPTURE_SCD,
>  	STF_CAPTURE_NUM,
>  };
>
> --
> 2.25.1
>
>
Changhuang Liang July 11, 2024, 6:48 a.m. UTC | #2
Hi Jacopo

Thanks for your comments.

> Hi Changhuang
> 
> On Tue, Jul 09, 2024 at 01:38:18AM GMT, Changhuang Liang wrote:
> > Register ISP 3A "capture_scd" video device to receive statistics
> > collection data.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > ---
> >  .../staging/media/starfive/camss/stf-buffer.h |   1 +
> >  .../staging/media/starfive/camss/stf-camss.c  |  15 ++
> >  .../media/starfive/camss/stf-capture.c        |  21 ++-
> >  .../media/starfive/camss/stf-isp-hw-ops.c     |  66 ++++++++
> >  .../staging/media/starfive/camss/stf-isp.h    |  23 +++
> >  .../staging/media/starfive/camss/stf-video.c  | 146
> +++++++++++++++++-
> >  .../staging/media/starfive/camss/stf-video.h  |   1 +
> >  7 files changed, 264 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-buffer.h
> > b/drivers/staging/media/starfive/camss/stf-buffer.h
> > index 9d1670fb05ed..727d00617448 100644
> > --- a/drivers/staging/media/starfive/camss/stf-buffer.h
> > +++ b/drivers/staging/media/starfive/camss/stf-buffer.h
> > @@ -23,6 +23,7 @@ enum stf_v_state {
> >  struct stfcamss_buffer {
> >  	struct vb2_v4l2_buffer vb;
> >  	dma_addr_t addr[2];
> > +	void *vaddr;
> >  	struct list_head queue;
> >  };
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > b/drivers/staging/media/starfive/camss/stf-camss.c
> > index fecd3e67c7a1..fafa3ce2f6da 100644
> > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > @@ -8,6 +8,7 @@
> >   *
> >   * Author: Jack Zhu <jack.zhu@starfivetech.com>
> >   * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> > + * Author: Keith Zhao <keith.zhao@starfivetech.com>
> >   *
> >   */
> >  #include <linux/module.h>
> > @@ -126,6 +127,7 @@ static int stfcamss_of_parse_ports(struct stfcamss
> > *stfcamss)  static int stfcamss_register_devs(struct stfcamss
> > *stfcamss)  {
> >  	struct stf_capture *cap_yuv =
> &stfcamss->captures[STF_CAPTURE_YUV];
> > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> >  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> >  	int ret;
> >
> > @@ -150,8 +152,18 @@ static int stfcamss_register_devs(struct stfcamss
> > *stfcamss)
> >
> >  	cap_yuv->video.source_subdev = &isp_dev->subdev;
> >
> > +	ret = media_create_pad_link(&isp_dev->subdev.entity,
> STF_ISP_PAD_SRC_SCD,
> > +				    &cap_scd->video.vdev.entity, 0, 0);
> > +	if (ret)
> > +		goto err_rm_links0;
> 
> or just 'err_rm_links'
> 

Agreed.

> > +
> > +	cap_scd->video.source_subdev = &isp_dev->subdev;
> > +
> >  	return ret;
> 
> here you can return 0
> 

Okay.

> >
> > +err_rm_links0:
> > +	media_entity_remove_links(&isp_dev->subdev.entity);
> > +	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> >  err_cap_unregister:
> >  	stf_capture_unregister(stfcamss);
> >  err_isp_unregister:
> > @@ -163,10 +175,12 @@ static int stfcamss_register_devs(struct
> > stfcamss *stfcamss)  static void stfcamss_unregister_devs(struct
> > stfcamss *stfcamss)  {
> >  	struct stf_capture *cap_yuv =
> &stfcamss->captures[STF_CAPTURE_YUV];
> > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> >  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> >
> >  	media_entity_remove_links(&isp_dev->subdev.entity);
> >  	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> > +	media_entity_remove_links(&cap_scd->video.vdev.entity);
> >
> >  	stf_isp_unregister(&stfcamss->isp_dev);
> >  	stf_capture_unregister(stfcamss);
> > @@ -436,5 +450,6 @@ module_platform_driver(stfcamss_driver);
> >
> >  MODULE_AUTHOR("Jack Zhu <jack.zhu@starfivetech.com>");
> > MODULE_AUTHOR("Changhuang Liang
> <changhuang.liang@starfivetech.com>");
> > +MODULE_AUTHOR("Keith Zhao <keith.zhao@starfivetech.com>");
> >  MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
> > MODULE_LICENSE("GPL"); diff --git
> > a/drivers/staging/media/starfive/camss/stf-capture.c
> > b/drivers/staging/media/starfive/camss/stf-capture.c
> > index 75f6ef405e61..328b8c6e351d 100644
> > --- a/drivers/staging/media/starfive/camss/stf-capture.c
> > +++ b/drivers/staging/media/starfive/camss/stf-capture.c
> > @@ -12,6 +12,7 @@
> >  static const char * const stf_cap_names[] = {
> >  	"capture_raw",
> >  	"capture_yuv",
> > +	"capture_scd",
> >  };
> >
> >  static const struct stfcamss_format_info stf_wr_fmts[] = { @@ -55,6
> > +56,14 @@ static const struct stfcamss_format_info stf_isp_fmts[] = {
> >  	},
> >  };
> >
> > +/* 3A Statistics Collection Data */
> > +static const struct stfcamss_format_info stf_isp_scd_fmts[] = {
> > +	{
> > +		.code = MEDIA_BUS_FMT_METADATA_FIXED,
> > +		.pixelformat = V4L2_META_FMT_STF_ISP_STAT_3A,
> > +	},
> > +};
> > +
> >  static inline struct stf_capture *to_stf_capture(struct
> > stfcamss_video *video)  {
> >  	return container_of(video, struct stf_capture, video); @@ -84,6
> > +93,8 @@ static void stf_init_addrs(struct stfcamss_video *video)
> >  		stf_set_raw_addr(video->stfcamss, addr0);
> >  	else if (cap->type == STF_CAPTURE_YUV)
> >  		stf_set_yuv_addr(video->stfcamss, addr0, addr1);
> > +	else
> > +		stf_set_scd_addr(video->stfcamss, addr0, addr1, TYPE_AWB);
> >  }
> >
> >  static void stf_cap_s_cfg(struct stfcamss_video *video) @@ -227,18
> > +238,24 @@ static void stf_capture_init(struct stfcamss *stfcamss, struct
> stf_capture *cap)
> >  	INIT_LIST_HEAD(&cap->buffers.ready_bufs);
> >  	spin_lock_init(&cap->buffers.lock);
> >
> > -	cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  	cap->video.stfcamss = stfcamss;
> >  	cap->video.bpl_alignment = 16 * 8;
> >
> >  	if (cap->type == STF_CAPTURE_RAW) {
> > +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  		cap->video.formats = stf_wr_fmts;
> >  		cap->video.nformats = ARRAY_SIZE(stf_wr_fmts);
> >  		cap->video.bpl_alignment = 8;
> >  	} else if (cap->type == STF_CAPTURE_YUV) {
> > +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  		cap->video.formats = stf_isp_fmts;
> >  		cap->video.nformats = ARRAY_SIZE(stf_isp_fmts);
> >  		cap->video.bpl_alignment = 1;
> > +	} else {
> > +		cap->video.type = V4L2_BUF_TYPE_META_CAPTURE;
> > +		cap->video.formats = stf_isp_scd_fmts;
> > +		cap->video.nformats = ARRAY_SIZE(stf_isp_scd_fmts);
> > +		cap->video.bpl_alignment = 16 * 8;
> >  	}
> >  }
> >
> > @@ -362,9 +379,11 @@ void stf_capture_unregister(struct stfcamss
> > *stfcamss)  {
> >  	struct stf_capture *cap_raw =
> &stfcamss->captures[STF_CAPTURE_RAW];
> >  	struct stf_capture *cap_yuv =
> &stfcamss->captures[STF_CAPTURE_YUV];
> > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> >
> >  	stf_capture_unregister_one(cap_raw);
> >  	stf_capture_unregister_one(cap_yuv);
> > +	stf_capture_unregister_one(cap_scd);
> >  }
> >
> >  int stf_capture_register(struct stfcamss *stfcamss, diff --git
> > a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > index 6b3966ca18bf..3b18d09f2cc6 100644
> > --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > @@ -451,11 +451,57 @@ void stf_set_yuv_addr(struct stfcamss *stfcamss,
> >  	stf_isp_reg_write(stfcamss, ISP_REG_UV_PLANE_START_ADDR,
> uv_addr);
> > }
> >
> > +static enum stf_isp_type_scd stf_isp_get_scd_type(struct stfcamss
> > +*stfcamss) {
> > +	int val;
> > +
> > +	val = stf_isp_reg_read(stfcamss, ISP_REG_SC_CFG_1);
> > +	return (enum stf_isp_type_scd)(val & ISP_SC_SEL_MASK) >> 30; }
> 
> So far used by a single caller, could be inlined
> 

Okay.

> > +
> > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > +		      enum stf_isp_type_scd type_scd) {
> > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > +			    SEL_TYPE(type_scd));
> > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); }
> > +
> > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void
> > +*vaddr) {
> > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > +	u32 i;
> > +
> > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
> 
> If you have a contigous memory space to read, could memcpy_fromio() help
> instead of going through 64 reads ?
> 

I will try this function.

> > +}
> > +
> > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > +			      enum stf_isp_type_scd *type_scd) {
> > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> > +*)vaddr;
> > +
> > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > +	if (*type_scd == TYPE_AWB) {
> > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > +		*type_scd = TYPE_OECF;
> > +	} else {
> > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > +		*type_scd = TYPE_AWB;
> 
> Is this correct ? Why are you overwriting the value read from HW that
> indicates AE/AF stats with AWB ones ?

The AWB frame and AE/AF frames will alternate, so the current frame indicates the AE/AF,
then set AWB type just for next AWB frame.

> 
> > +	}
> > +}
> > +
> >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> >  	struct stfcamss *stfcamss = priv;
> >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> >  	struct stfcamss_buffer *change_buf;
> > +	enum stf_isp_type_scd type_scd;
> > +	u32 value;
> >  	u32 status;
> >
> >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0); @@ -467,6
> > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
> >  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
> >  							 change_buf->addr[1]);
> >  			}
> > +
> > +			value = stf_isp_reg_read(stfcamss,
> ISP_REG_CSI_MODULE_CFG);
> > +			if (value & CSI_SC_EN) {
> > +				change_buf = stf_change_buffer(&cap_scd->buffers);
> > +				if (change_buf) {
> > +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> > +							  &type_scd);
> > +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > +							 change_buf->addr[1], type_scd);
> 
> Sorry if I'm un-familiar with the HW but this seems to be the line-interrupt.
> Are you swapping buffers every line or it's just that you have a single line irq
> for the stats ?
> 

Every frame triggers a line-interrupt, and we will swap buffers in it.

> > +				}
> > +			}
> >  		}
> >
> >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@ -485,6 +542,7
> @@
> > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> >  	struct stfcamss *stfcamss = priv;
> >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> >  	struct stfcamss_buffer *ready_buf;
> >  	u32 status;
> >
> > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> VB2_BUF_STATE_DONE);
> >  		}
> >
> > +		if (status & ISPC_SC) {
> > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > +			if (ready_buf) {
> > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> VB2_BUF_STATE_DONE);
> > +			}
> > +		}
> > +
> >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> >  				  (status & ~ISPC_INT_ALL_MASK) |
> >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > a/drivers/staging/media/starfive/camss/stf-isp.h
> > b/drivers/staging/media/starfive/camss/stf-isp.h
> > index fcda0502e3b0..0af7b367e57a 100644
> > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > @@ -10,6 +10,7 @@
> >  #ifndef STF_ISP_H
> >  #define STF_ISP_H
> >
> > +#include <linux/jh7110-isp.h>
> >  #include <media/v4l2-subdev.h>
> >
> >  #include "stf-video.h"
> > @@ -107,6 +108,12 @@
> >  #define Y_COOR(y)				((y) << 16)
> >  #define X_COOR(x)				((x) << 0)
> >
> > +#define ISP_REG_SCD_CFG_0			0x098
> > +
> > +#define ISP_REG_SC_CFG_1			0x0bc
> > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > +#define SEL_TYPE(n)				((n) << 30)
> > +
> >  #define ISP_REG_LCCF_CFG_2			0x0e0
> >  #define ISP_REG_LCCF_CFG_3			0x0e4
> >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > @@ -305,6 +312,10 @@
> >  #define DNRM_F(n)				((n) << 16)
> >  #define CCM_M_DAT(n)				((n) << 0)
> >
> > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > +
> > +#define ISP_REG_YHIST_ACC_0			0xd00
> > +
> >  #define ISP_REG_GAMMA_VAL0			0xe00
> >  #define ISP_REG_GAMMA_VAL1			0xe04
> >  #define ISP_REG_GAMMA_VAL2			0xe08
> > @@ -389,6 +400,15 @@
> >  #define IMAGE_MAX_WIDTH				1920
> >  #define IMAGE_MAX_HEIGH				1080
> >
> > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> 
> Should this be in the uAPI header as it is useful to userspace as well ?
> 
> you could:
> 
> struct jh7110_isp_sc_buffer {
> 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> 	__u32 reserv0[33];
> 	__u32 bright_sc[4096];
> 	__u32 reserv1[96];
> 	__u32 ae_hist_y[128];
> 	__u32 reserv2[511];
> 	__u16 flag;
> };
> 
> ofc if the size is made part of the uAPI you need a more proper name such as
> JH7110_ISP_YHIST_SIZE
> 

OK, I will try this.

> > +
> > +enum stf_isp_type_scd {
> > +	TYPE_DEC = 0,
> > +	TYPE_OBC,
> > +	TYPE_OECF,
> > +	TYPE_AWB,
> > +};
> > +
> >  /* pad id for media framework */
> >  enum stf_isp_pad_id {
> >  	STF_ISP_PAD_SINK = 0,
> > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > *isp_dev);
> >
> >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > +		      enum stf_isp_type_scd type_scd);
> >
> >  #endif /* STF_ISP_H */
> > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > b/drivers/staging/media/starfive/camss/stf-video.c
> > index 989b5e82bae9..2203605ec9c7 100644
> > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> stfcamss_video *video)
> >  	return 0;
> >  }
> >
> > +static int stf_video_scd_init_format(struct stfcamss_video *video)
> 
> Make it void if it can't fail (see below)
> 

OK.

> > +{
> > +	video->active_fmt.fmt.meta.dataformat =
> video->formats[0].pixelformat;
> > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > +jh7110_isp_sc_buffer);
> > +
> > +	return 0;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Video queue operations
> >   */
> > @@ -330,6 +338,78 @@ static const struct vb2_ops stf_video_vb2_q_ops =
> {
> >  	.stop_streaming  = video_stop_streaming,  };
> >
> > +static int video_scd_queue_setup(struct vb2_queue *q,
> > +				 unsigned int *num_buffers,
> > +				 unsigned int *num_planes,
> > +				 unsigned int sizes[],
> > +				 struct device *alloc_devs[])
> > +{
> > +	if (*num_planes)
> > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL :
> > +0;
> > +
> > +	*num_planes = 1;
> > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > +
> > +	return 0;
> > +}
> > +
> > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > +	dma_addr_t *paddr;
> > +
> > +	paddr = vb2_plane_cookie(vb, 0);
> > +	buffer->addr[0] = *paddr;
> > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> 
> Interesting, I don't see many users of vb2_plane_cookie() in mainline and I'm
> not sure what this gives you as you use it to program the following registers:
> 
> 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> 

We set the value for ISP hardware, then ISP will transfer the statistics to the buffer.
when the stf_isp_irq_handler interrupt is triggered, indicates that the buffer fill is 
complete.

> 
> > +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int video_scd_buf_prepare(struct vb2_buffer *vb) {
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> > +		return -EINVAL;
> > +
> > +	vb2_set_plane_payload(vb, 0, sizeof(struct jh7110_isp_sc_buffer));
> > +
> > +	vbuf->field = V4L2_FIELD_NONE;
> 
> is this necessary ?
> 

Will drop it.

> > +
> > +	return 0;
> > +}
> > +
> > +static int video_scd_start_streaming(struct vb2_queue *q, unsigned
> > +int count) {
> > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > +
> > +	video->ops->start_streaming(video);
> > +
> > +	return 0;
> > +}
> > +
> > +static void video_scd_stop_streaming(struct vb2_queue *q) {
> > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > +
> > +	video->ops->stop_streaming(video);
> > +
> > +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR); }
> > +
> > +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> > +	.queue_setup     = video_scd_queue_setup,
> > +	.wait_prepare    = vb2_ops_wait_prepare,
> > +	.wait_finish     = vb2_ops_wait_finish,
> > +	.buf_init        = video_scd_buf_init,
> > +	.buf_prepare     = video_scd_buf_prepare,
> > +	.buf_queue       = video_buf_queue,
> > +	.start_streaming = video_scd_start_streaming,
> > +	.stop_streaming  = video_scd_stop_streaming, };
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 ioctls
> >   */
> > @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops stf_vid_ioctl_ops
> = {
> >  	.vidioc_streamoff               = vb2_ioctl_streamoff,
> >  };
> >
> > +static int video_scd_g_fmt(struct file *file, void *fh, struct
> > +v4l2_format *f) {
> > +	struct stfcamss_video *video = video_drvdata(file);
> > +	struct v4l2_meta_format *meta = &f->fmt.meta;
> > +
> > +	if (f->type != video->type)
> > +		return -EINVAL;
> > +
> > +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> > +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> > +	.vidioc_querycap                = video_querycap,
> > +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> > +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> > +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> > +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> > +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> > +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> > +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> > +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> > +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> > +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> > +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> > +	.vidioc_streamon                = vb2_ioctl_streamon,
> > +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > +};
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 file operations
> >   */
> > @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
> >  	struct stfcamss_video *video = video_get_drvdata(vdev);
> >  	int ret;
> >
> > +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> > +		return 0;
> > +
> >  	ret = stf_video_check_format(video);
> >
> >  	return ret;
> > @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video
> *video,
> >  	q = &video->vb2_q;
> >  	q->drv_priv = video;
> >  	q->mem_ops = &vb2_dma_contig_memops;
> > -	q->ops = &stf_video_vb2_q_ops;
> > +
> > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		q->ops = &stf_video_vb2_q_ops;
> > +	else
> > +		q->ops = &stf_video_scd_vb2_q_ops;
> >  	q->type = video->type;
> >  	q->io_modes = VB2_DMABUF | VB2_MMAP;
> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video
> *video,
> >  		goto err_mutex_destroy;
> >  	}
> >
> > -	ret = stf_video_init_format(video);
> > -	if (ret < 0) {
> > -		dev_err(video->stfcamss->dev,
> > -			"Failed to init format: %d\n", ret);
> > -		goto err_media_cleanup;
> > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > +		ret = stf_video_init_format(video);
> 
> I don't think this can fail
> 

This already exists, and I probably will not change it here.

> > +		if (ret < 0) {
> > +			dev_err(video->stfcamss->dev,
> > +				"Failed to init format: %d\n", ret);
> > +			goto err_media_cleanup;
> > +		}
> > +		vdev->ioctl_ops = &stf_vid_ioctl_ops;
> > +		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > +	} else {
> > +		ret = stf_video_scd_init_format(video);
> 
> This can't fail as noted above
> 

I will change this return void.

> > +		if (ret < 0) {
> > +			dev_err(video->stfcamss->dev,
> > +				"Failed to init format: %d\n", ret);
> > +			goto err_media_cleanup;
> > +		}
> > +		vdev->ioctl_ops = &stf_vid_scd_ioctl_ops;
> > +		vdev->device_caps = V4L2_CAP_META_CAPTURE;
> >  	}
> >
> >  	vdev->fops = &stf_vid_fops;
> > -	vdev->ioctl_ops = &stf_vid_ioctl_ops;
> > -	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING;
> > +	vdev->device_caps |= V4L2_CAP_STREAMING;
> >  	vdev->entity.ops = &stf_media_ops;
> >  	vdev->vfl_dir = VFL_DIR_RX;
> >  	vdev->release = stf_video_release;
> > diff --git a/drivers/staging/media/starfive/camss/stf-video.h
> > b/drivers/staging/media/starfive/camss/stf-video.h
> > index 59799b65cbe5..53a1cf4e59b7 100644
> > --- a/drivers/staging/media/starfive/camss/stf-video.h
> > +++ b/drivers/staging/media/starfive/camss/stf-video.h
> > @@ -37,6 +37,7 @@ enum stf_v_line_id {  enum stf_capture_type {
> >  	STF_CAPTURE_RAW = 0,
> >  	STF_CAPTURE_YUV,
> > +	STF_CAPTURE_SCD,
> >  	STF_CAPTURE_NUM,
> >  };
> >
> > --
> > 2.25.1
> >
> >

Regards,
Changhuang
Jacopo Mondi July 11, 2024, 8:26 a.m. UTC | #3
Hi Changhuang

On Thu, Jul 11, 2024 at 06:48:21AM GMT, Changhuang Liang wrote:
> Hi Jacopo
>
> Thanks for your comments.
>
> > Hi Changhuang
> >
> > On Tue, Jul 09, 2024 at 01:38:18AM GMT, Changhuang Liang wrote:
> > > Register ISP 3A "capture_scd" video device to receive statistics
> > > collection data.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  .../staging/media/starfive/camss/stf-buffer.h |   1 +
> > >  .../staging/media/starfive/camss/stf-camss.c  |  15 ++
> > >  .../media/starfive/camss/stf-capture.c        |  21 ++-
> > >  .../media/starfive/camss/stf-isp-hw-ops.c     |  66 ++++++++
> > >  .../staging/media/starfive/camss/stf-isp.h    |  23 +++
> > >  .../staging/media/starfive/camss/stf-video.c  | 146
> > +++++++++++++++++-
> > >  .../staging/media/starfive/camss/stf-video.h  |   1 +
> > >  7 files changed, 264 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/starfive/camss/stf-buffer.h
> > > b/drivers/staging/media/starfive/camss/stf-buffer.h
> > > index 9d1670fb05ed..727d00617448 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-buffer.h
> > > +++ b/drivers/staging/media/starfive/camss/stf-buffer.h
> > > @@ -23,6 +23,7 @@ enum stf_v_state {
> > >  struct stfcamss_buffer {
> > >  	struct vb2_v4l2_buffer vb;
> > >  	dma_addr_t addr[2];
> > > +	void *vaddr;
> > >  	struct list_head queue;
> > >  };
> > >
> > > diff --git a/drivers/staging/media/starfive/camss/stf-camss.c
> > > b/drivers/staging/media/starfive/camss/stf-camss.c
> > > index fecd3e67c7a1..fafa3ce2f6da 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-camss.c
> > > +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> > > @@ -8,6 +8,7 @@
> > >   *
> > >   * Author: Jack Zhu <jack.zhu@starfivetech.com>
> > >   * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > + * Author: Keith Zhao <keith.zhao@starfivetech.com>
> > >   *
> > >   */
> > >  #include <linux/module.h>
> > > @@ -126,6 +127,7 @@ static int stfcamss_of_parse_ports(struct stfcamss
> > > *stfcamss)  static int stfcamss_register_devs(struct stfcamss
> > > *stfcamss)  {
> > >  	struct stf_capture *cap_yuv =
> > &stfcamss->captures[STF_CAPTURE_YUV];
> > > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > >  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > >  	int ret;
> > >
> > > @@ -150,8 +152,18 @@ static int stfcamss_register_devs(struct stfcamss
> > > *stfcamss)
> > >
> > >  	cap_yuv->video.source_subdev = &isp_dev->subdev;
> > >
> > > +	ret = media_create_pad_link(&isp_dev->subdev.entity,
> > STF_ISP_PAD_SRC_SCD,
> > > +				    &cap_scd->video.vdev.entity, 0, 0);
> > > +	if (ret)
> > > +		goto err_rm_links0;
> >
> > or just 'err_rm_links'
> >
>
> Agreed.
>
> > > +
> > > +	cap_scd->video.source_subdev = &isp_dev->subdev;
> > > +
> > >  	return ret;
> >
> > here you can return 0
> >
>
> Okay.
>
> > >
> > > +err_rm_links0:
> > > +	media_entity_remove_links(&isp_dev->subdev.entity);
> > > +	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> > >  err_cap_unregister:
> > >  	stf_capture_unregister(stfcamss);
> > >  err_isp_unregister:
> > > @@ -163,10 +175,12 @@ static int stfcamss_register_devs(struct
> > > stfcamss *stfcamss)  static void stfcamss_unregister_devs(struct
> > > stfcamss *stfcamss)  {
> > >  	struct stf_capture *cap_yuv =
> > &stfcamss->captures[STF_CAPTURE_YUV];
> > > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > >  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
> > >
> > >  	media_entity_remove_links(&isp_dev->subdev.entity);
> > >  	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> > > +	media_entity_remove_links(&cap_scd->video.vdev.entity);
> > >
> > >  	stf_isp_unregister(&stfcamss->isp_dev);
> > >  	stf_capture_unregister(stfcamss);
> > > @@ -436,5 +450,6 @@ module_platform_driver(stfcamss_driver);
> > >
> > >  MODULE_AUTHOR("Jack Zhu <jack.zhu@starfivetech.com>");
> > > MODULE_AUTHOR("Changhuang Liang
> > <changhuang.liang@starfivetech.com>");
> > > +MODULE_AUTHOR("Keith Zhao <keith.zhao@starfivetech.com>");
> > >  MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
> > > MODULE_LICENSE("GPL"); diff --git
> > > a/drivers/staging/media/starfive/camss/stf-capture.c
> > > b/drivers/staging/media/starfive/camss/stf-capture.c
> > > index 75f6ef405e61..328b8c6e351d 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-capture.c
> > > +++ b/drivers/staging/media/starfive/camss/stf-capture.c
> > > @@ -12,6 +12,7 @@
> > >  static const char * const stf_cap_names[] = {
> > >  	"capture_raw",
> > >  	"capture_yuv",
> > > +	"capture_scd",
> > >  };
> > >
> > >  static const struct stfcamss_format_info stf_wr_fmts[] = { @@ -55,6
> > > +56,14 @@ static const struct stfcamss_format_info stf_isp_fmts[] = {
> > >  	},
> > >  };
> > >
> > > +/* 3A Statistics Collection Data */
> > > +static const struct stfcamss_format_info stf_isp_scd_fmts[] = {
> > > +	{
> > > +		.code = MEDIA_BUS_FMT_METADATA_FIXED,
> > > +		.pixelformat = V4L2_META_FMT_STF_ISP_STAT_3A,
> > > +	},
> > > +};
> > > +
> > >  static inline struct stf_capture *to_stf_capture(struct
> > > stfcamss_video *video)  {
> > >  	return container_of(video, struct stf_capture, video); @@ -84,6
> > > +93,8 @@ static void stf_init_addrs(struct stfcamss_video *video)
> > >  		stf_set_raw_addr(video->stfcamss, addr0);
> > >  	else if (cap->type == STF_CAPTURE_YUV)
> > >  		stf_set_yuv_addr(video->stfcamss, addr0, addr1);
> > > +	else
> > > +		stf_set_scd_addr(video->stfcamss, addr0, addr1, TYPE_AWB);
> > >  }
> > >
> > >  static void stf_cap_s_cfg(struct stfcamss_video *video) @@ -227,18
> > > +238,24 @@ static void stf_capture_init(struct stfcamss *stfcamss, struct
> > stf_capture *cap)
> > >  	INIT_LIST_HEAD(&cap->buffers.ready_bufs);
> > >  	spin_lock_init(&cap->buffers.lock);
> > >
> > > -	cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >  	cap->video.stfcamss = stfcamss;
> > >  	cap->video.bpl_alignment = 16 * 8;
> > >
> > >  	if (cap->type == STF_CAPTURE_RAW) {
> > > +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >  		cap->video.formats = stf_wr_fmts;
> > >  		cap->video.nformats = ARRAY_SIZE(stf_wr_fmts);
> > >  		cap->video.bpl_alignment = 8;
> > >  	} else if (cap->type == STF_CAPTURE_YUV) {
> > > +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >  		cap->video.formats = stf_isp_fmts;
> > >  		cap->video.nformats = ARRAY_SIZE(stf_isp_fmts);
> > >  		cap->video.bpl_alignment = 1;
> > > +	} else {
> > > +		cap->video.type = V4L2_BUF_TYPE_META_CAPTURE;
> > > +		cap->video.formats = stf_isp_scd_fmts;
> > > +		cap->video.nformats = ARRAY_SIZE(stf_isp_scd_fmts);
> > > +		cap->video.bpl_alignment = 16 * 8;
> > >  	}
> > >  }
> > >
> > > @@ -362,9 +379,11 @@ void stf_capture_unregister(struct stfcamss
> > > *stfcamss)  {
> > >  	struct stf_capture *cap_raw =
> > &stfcamss->captures[STF_CAPTURE_RAW];
> > >  	struct stf_capture *cap_yuv =
> > &stfcamss->captures[STF_CAPTURE_YUV];
> > > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > >
> > >  	stf_capture_unregister_one(cap_raw);
> > >  	stf_capture_unregister_one(cap_yuv);
> > > +	stf_capture_unregister_one(cap_scd);
> > >  }
> > >
> > >  int stf_capture_register(struct stfcamss *stfcamss, diff --git
> > > a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > > b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > > index 6b3966ca18bf..3b18d09f2cc6 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > > +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > > @@ -451,11 +451,57 @@ void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > >  	stf_isp_reg_write(stfcamss, ISP_REG_UV_PLANE_START_ADDR,
> > uv_addr);
> > > }
> > >
> > > +static enum stf_isp_type_scd stf_isp_get_scd_type(struct stfcamss
> > > +*stfcamss) {
> > > +	int val;
> > > +
> > > +	val = stf_isp_reg_read(stfcamss, ISP_REG_SC_CFG_1);
> > > +	return (enum stf_isp_type_scd)(val & ISP_SC_SEL_MASK) >> 30; }
> >
> > So far used by a single caller, could be inlined
> >
>
> Okay.
>
> > > +
> > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > +		      enum stf_isp_type_scd type_scd) {
> > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > > +			    SEL_TYPE(type_scd));
> > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); }
> > > +
> > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void
> > > +*vaddr) {
> > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > +	u32 i;
> > > +
> > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
> >
> > If you have a contigous memory space to read, could memcpy_fromio() help
> > instead of going through 64 reads ?
> >
>
> I will try this function.
>
> > > +}
> > > +
> > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > > +			      enum stf_isp_type_scd *type_scd) {
> > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> > > +*)vaddr;
> > > +
> > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > +	if (*type_scd == TYPE_AWB) {
> > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > +		*type_scd = TYPE_OECF;
> > > +	} else {
> > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > +		*type_scd = TYPE_AWB;
> >
> > Is this correct ? Why are you overwriting the value read from HW that
> > indicates AE/AF stats with AWB ones ?
>
> The AWB frame and AE/AF frames will alternate, so the current frame indicates the AE/AF,
> then set AWB type just for next AWB frame.
>

Ah! Shouldn't it be userspace configuring which type of statistics it
wants to receive instead of the driver alternating the two ?

> >
> > > +	}
> > > +}
> > > +
> > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > >  	struct stfcamss *stfcamss = priv;
> > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > >  	struct stfcamss_buffer *change_buf;
> > > +	enum stf_isp_type_scd type_scd;
> > > +	u32 value;
> > >  	u32 status;
> > >
> > >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0); @@ -467,6
> > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
> > >  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
> > >  							 change_buf->addr[1]);
> > >  			}
> > > +
> > > +			value = stf_isp_reg_read(stfcamss,
> > ISP_REG_CSI_MODULE_CFG);
> > > +			if (value & CSI_SC_EN) {
> > > +				change_buf = stf_change_buffer(&cap_scd->buffers);
> > > +				if (change_buf) {
> > > +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> > > +							  &type_scd);
> > > +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > +							 change_buf->addr[1], type_scd);
> >
> > Sorry if I'm un-familiar with the HW but this seems to be the line-interrupt.
> > Are you swapping buffers every line or it's just that you have a single line irq
> > for the stats ?
> >
>
> Every frame triggers a line-interrupt, and we will swap buffers in it.
>

ah, frames completion triggers a line-interrupt ?

> > > +				}
> > > +			}
> > >  		}
> > >
> > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@ -485,6 +542,7
> > @@
> > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > >  	struct stfcamss *stfcamss = priv;
> > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > >  	struct stfcamss_buffer *ready_buf;
> > >  	u32 status;
> > >
> > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > VB2_BUF_STATE_DONE);
> > >  		}
> > >
> > > +		if (status & ISPC_SC) {
> > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > +			if (ready_buf) {
> > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > VB2_BUF_STATE_DONE);
> > > +			}
> > > +		}
> > > +
> > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > index fcda0502e3b0..0af7b367e57a 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > @@ -10,6 +10,7 @@
> > >  #ifndef STF_ISP_H
> > >  #define STF_ISP_H
> > >
> > > +#include <linux/jh7110-isp.h>
> > >  #include <media/v4l2-subdev.h>
> > >
> > >  #include "stf-video.h"
> > > @@ -107,6 +108,12 @@
> > >  #define Y_COOR(y)				((y) << 16)
> > >  #define X_COOR(x)				((x) << 0)
> > >
> > > +#define ISP_REG_SCD_CFG_0			0x098
> > > +
> > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > +#define SEL_TYPE(n)				((n) << 30)
> > > +
> > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > @@ -305,6 +312,10 @@
> > >  #define DNRM_F(n)				((n) << 16)
> > >  #define CCM_M_DAT(n)				((n) << 0)
> > >
> > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > +
> > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > +
> > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > @@ -389,6 +400,15 @@
> > >  #define IMAGE_MAX_WIDTH				1920
> > >  #define IMAGE_MAX_HEIGH				1080
> > >
> > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> >
> > Should this be in the uAPI header as it is useful to userspace as well ?
> >
> > you could:
> >
> > struct jh7110_isp_sc_buffer {
> > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > 	__u32 reserv0[33];
> > 	__u32 bright_sc[4096];
> > 	__u32 reserv1[96];
> > 	__u32 ae_hist_y[128];
> > 	__u32 reserv2[511];
> > 	__u16 flag;
> > };
> >
> > ofc if the size is made part of the uAPI you need a more proper name such as
> > JH7110_ISP_YHIST_SIZE
> >
>
> OK, I will try this.
>
> > > +
> > > +enum stf_isp_type_scd {
> > > +	TYPE_DEC = 0,
> > > +	TYPE_OBC,
> > > +	TYPE_OECF,
> > > +	TYPE_AWB,
> > > +};
> > > +
> > >  /* pad id for media framework */
> > >  enum stf_isp_pad_id {
> > >  	STF_ISP_PAD_SINK = 0,
> > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > > *isp_dev);
> > >
> > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > +		      enum stf_isp_type_scd type_scd);
> > >
> > >  #endif /* STF_ISP_H */
> > > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > index 989b5e82bae9..2203605ec9c7 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> > stfcamss_video *video)
> > >  	return 0;
> > >  }
> > >
> > > +static int stf_video_scd_init_format(struct stfcamss_video *video)
> >
> > Make it void if it can't fail (see below)
> >
>
> OK.
>
> > > +{
> > > +	video->active_fmt.fmt.meta.dataformat =
> > video->formats[0].pixelformat;
> > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > +jh7110_isp_sc_buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Video queue operations
> > >   */
> > > @@ -330,6 +338,78 @@ static const struct vb2_ops stf_video_vb2_q_ops =
> > {
> > >  	.stop_streaming  = video_stop_streaming,  };
> > >
> > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > +				 unsigned int *num_buffers,
> > > +				 unsigned int *num_planes,
> > > +				 unsigned int sizes[],
> > > +				 struct device *alloc_devs[])
> > > +{
> > > +	if (*num_planes)
> > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL :
> > > +0;
> > > +
> > > +	*num_planes = 1;
> > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > +	dma_addr_t *paddr;
> > > +
> > > +	paddr = vb2_plane_cookie(vb, 0);
> > > +	buffer->addr[0] = *paddr;
> > > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> >
> > Interesting, I don't see many users of vb2_plane_cookie() in mainline and I'm
> > not sure what this gives you as you use it to program the following registers:
> >
> > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> >
>
> We set the value for ISP hardware, then ISP will transfer the statistics to the buffer.
> when the stf_isp_irq_handler interrupt is triggered, indicates that the buffer fill is
> complete.
>

So I take this as

	paddr = vb2_plane_cookie(vb, 0);
	buffer->addr[0] = *paddr;
	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;

        stf_set_scd_addr(stfcamss, change_buf->addr[0],
                         change_buf->addr[1], type_scd);

Makes the ISP transfer data directly to the memory areas in addr[0]
and addr[1] (which explains why struct jh7110_isp_sc_buffer is packed,
as it has to match the HW registers layout)

If this is the case, why are you then manually copying the histograms
and the flags to vaddr ?

                ready_buf = stf_buf_done(&cap_scd->buffers);
                if (ready_buf) {
                        stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
                        vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
                }

                change_buf = stf_change_buffer(&cap_scd->buffers);
                if (change_buf) {
                        stf_isp_fill_flag(stfcamss, change_buf->vaddr,
                                          &type_scd);

If I read vb2_dc_alloc_coherent() right 'cookie' == 'vaddr'

static int vb2_dc_alloc_coherent(struct vb2_dc_buf *buf)
{
	struct vb2_queue *q = buf->vb->vb2_queue;

	buf->cookie = dma_alloc_attrs(buf->dev,
				      buf->size,
				      &buf->dma_addr,
				      GFP_KERNEL | q->gfp_flags,
				      buf->attrs);
	if (!buf->cookie)
		return -ENOMEM;

	if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
		return 0;

	buf->vaddr = buf->cookie;
	return 0;
}

Could you verify what you get by printing out 'paddr' and 'vaddr' in
video_scd_buf_init() ?


> >
> > > +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int video_scd_buf_prepare(struct vb2_buffer *vb) {
> > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > +
> > > +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> > > +		return -EINVAL;
> > > +
> > > +	vb2_set_plane_payload(vb, 0, sizeof(struct jh7110_isp_sc_buffer));
> > > +
> > > +	vbuf->field = V4L2_FIELD_NONE;
> >
> > is this necessary ?
> >
>
> Will drop it.
>
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int video_scd_start_streaming(struct vb2_queue *q, unsigned
> > > +int count) {
> > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > +
> > > +	video->ops->start_streaming(video);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void video_scd_stop_streaming(struct vb2_queue *q) {
> > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > +
> > > +	video->ops->stop_streaming(video);
> > > +
> > > +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR); }
> > > +
> > > +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> > > +	.queue_setup     = video_scd_queue_setup,
> > > +	.wait_prepare    = vb2_ops_wait_prepare,
> > > +	.wait_finish     = vb2_ops_wait_finish,
> > > +	.buf_init        = video_scd_buf_init,
> > > +	.buf_prepare     = video_scd_buf_prepare,
> > > +	.buf_queue       = video_buf_queue,
> > > +	.start_streaming = video_scd_start_streaming,
> > > +	.stop_streaming  = video_scd_stop_streaming, };
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * V4L2 ioctls
> > >   */
> > > @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops stf_vid_ioctl_ops
> > = {
> > >  	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > >  };
> > >
> > > +static int video_scd_g_fmt(struct file *file, void *fh, struct
> > > +v4l2_format *f) {
> > > +	struct stfcamss_video *video = video_drvdata(file);
> > > +	struct v4l2_meta_format *meta = &f->fmt.meta;
> > > +
> > > +	if (f->type != video->type)
> > > +		return -EINVAL;
> > > +
> > > +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> > > +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> > > +	.vidioc_querycap                = video_querycap,
> > > +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> > > +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> > > +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> > > +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> > > +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> > > +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> > > +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> > > +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> > > +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> > > +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> > > +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> > > +	.vidioc_streamon                = vb2_ioctl_streamon,
> > > +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > > +};
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * V4L2 file operations
> > >   */
> > > @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
> > >  	struct stfcamss_video *video = video_get_drvdata(vdev);
> > >  	int ret;
> > >
> > > +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> > > +		return 0;
> > > +
> > >  	ret = stf_video_check_format(video);
> > >
> > >  	return ret;
> > > @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video
> > *video,
> > >  	q = &video->vb2_q;
> > >  	q->drv_priv = video;
> > >  	q->mem_ops = &vb2_dma_contig_memops;
> > > -	q->ops = &stf_video_vb2_q_ops;
> > > +
> > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		q->ops = &stf_video_vb2_q_ops;
> > > +	else
> > > +		q->ops = &stf_video_scd_vb2_q_ops;
> > >  	q->type = video->type;
> > >  	q->io_modes = VB2_DMABUF | VB2_MMAP;
> > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video
> > *video,
> > >  		goto err_mutex_destroy;
> > >  	}
> > >
> > > -	ret = stf_video_init_format(video);
> > > -	if (ret < 0) {
> > > -		dev_err(video->stfcamss->dev,
> > > -			"Failed to init format: %d\n", ret);
> > > -		goto err_media_cleanup;
> > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > > +		ret = stf_video_init_format(video);
> >
> > I don't think this can fail
> >
>
> This already exists, and I probably will not change it here.
>

No problem! Maybe something for later when de-staging the driver.

Thanks
  j

> > > +		if (ret < 0) {
> > > +			dev_err(video->stfcamss->dev,
> > > +				"Failed to init format: %d\n", ret);
> > > +			goto err_media_cleanup;
> > > +		}
> > > +		vdev->ioctl_ops = &stf_vid_ioctl_ops;
> > > +		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > > +	} else {
> > > +		ret = stf_video_scd_init_format(video);
> >
> > This can't fail as noted above
> >
>
> I will change this return void.
>
> > > +		if (ret < 0) {
> > > +			dev_err(video->stfcamss->dev,
> > > +				"Failed to init format: %d\n", ret);
> > > +			goto err_media_cleanup;
> > > +		}
> > > +		vdev->ioctl_ops = &stf_vid_scd_ioctl_ops;
> > > +		vdev->device_caps = V4L2_CAP_META_CAPTURE;
> > >  	}
> > >
> > >  	vdev->fops = &stf_vid_fops;
> > > -	vdev->ioctl_ops = &stf_vid_ioctl_ops;
> > > -	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> > V4L2_CAP_STREAMING;
> > > +	vdev->device_caps |= V4L2_CAP_STREAMING;
> > >  	vdev->entity.ops = &stf_media_ops;
> > >  	vdev->vfl_dir = VFL_DIR_RX;
> > >  	vdev->release = stf_video_release;
> > > diff --git a/drivers/staging/media/starfive/camss/stf-video.h
> > > b/drivers/staging/media/starfive/camss/stf-video.h
> > > index 59799b65cbe5..53a1cf4e59b7 100644
> > > --- a/drivers/staging/media/starfive/camss/stf-video.h
> > > +++ b/drivers/staging/media/starfive/camss/stf-video.h
> > > @@ -37,6 +37,7 @@ enum stf_v_line_id {  enum stf_capture_type {
> > >  	STF_CAPTURE_RAW = 0,
> > >  	STF_CAPTURE_YUV,
> > > +	STF_CAPTURE_SCD,
> > >  	STF_CAPTURE_NUM,
> > >  };
> > >
> > > --
> > > 2.25.1
> > >
> > >
>
> Regards,
> Changhuang
>
Changhuang Liang July 12, 2024, 8:36 a.m. UTC | #4
Hi, Jacopo

[...]
> > > > +
> > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > +		      enum stf_isp_type_scd type_scd) {
> > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > > > +			    SEL_TYPE(type_scd));
> > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); }
> > > > +
> > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void
> > > > +*vaddr) {
> > > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> *)vaddr;
> > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > +	u32 i;
> > > > +
> > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
> > >
> > > If you have a contigous memory space to read, could memcpy_fromio()
> > > help instead of going through 64 reads ?
> > >
> >
> > I will try this function.
> >
> > > > +}
> > > > +
> > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> > > > +*)vaddr;
> > > > +
> > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > +	if (*type_scd == TYPE_AWB) {
> > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > +		*type_scd = TYPE_OECF;
> > > > +	} else {
> > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > +		*type_scd = TYPE_AWB;
> > >
> > > Is this correct ? Why are you overwriting the value read from HW
> > > that indicates AE/AF stats with AWB ones ?
> >
> > The AWB frame and AE/AF frames will alternate, so the current frame
> > indicates the AE/AF, then set AWB type just for next AWB frame.
> >
> 
> Ah! Shouldn't it be userspace configuring which type of statistics it wants to
> receive instead of the driver alternating the two ?
> 

No, this is determined by hardware, cannot be configured by userspace.

> > >
> > > > +	}
> > > > +}
> > > > +
> > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > >  	struct stfcamss *stfcamss = priv;
> > > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > > +	struct stf_capture *cap_scd =
> > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > >  	struct stfcamss_buffer *change_buf;
> > > > +	enum stf_isp_type_scd type_scd;
> > > > +	u32 value;
> > > >  	u32 status;
> > > >
> > > >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0); @@
> > > > -467,6
> > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
> > > >  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
> > > >  							 change_buf->addr[1]);
> > > >  			}
> > > > +
> > > > +			value = stf_isp_reg_read(stfcamss,
> > > ISP_REG_CSI_MODULE_CFG);
> > > > +			if (value & CSI_SC_EN) {
> > > > +				change_buf = stf_change_buffer(&cap_scd->buffers);
> > > > +				if (change_buf) {
> > > > +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> > > > +							  &type_scd);
> > > > +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > > +							 change_buf->addr[1], type_scd);
> > >
> > > Sorry if I'm un-familiar with the HW but this seems to be the line-interrupt.
> > > Are you swapping buffers every line or it's just that you have a
> > > single line irq for the stats ?
> > >
> >
> > Every frame triggers a line-interrupt, and we will swap buffers in it.
> >
> 
> ah, frames completion triggers a line-interrupt ?
> 

Every frame will trigger line-interrupt and stf_isp_irq_handler.
We use line-interrupt changing buffer, the stf_isp_irq_handler will indicate that
image transfer to DDR is complete.


> > > > +				}
> > > > +			}
> > > >  		}
> > > >
> > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@ -485,6
> +542,7
> > > @@
> > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > >  	struct stfcamss *stfcamss = priv;
> > > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > > +	struct stf_capture *cap_scd =
> > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > >  	struct stfcamss_buffer *ready_buf;
> > > >  	u32 status;
> > > >
> > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > VB2_BUF_STATE_DONE);
> > > >  		}
> > > >
> > > > +		if (status & ISPC_SC) {
> > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > +			if (ready_buf) {
> > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > VB2_BUF_STATE_DONE);
> > > > +			}
> > > > +		}
> > > > +
> > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > @@ -10,6 +10,7 @@
> > > >  #ifndef STF_ISP_H
> > > >  #define STF_ISP_H
> > > >
> > > > +#include <linux/jh7110-isp.h>
> > > >  #include <media/v4l2-subdev.h>
> > > >
> > > >  #include "stf-video.h"
> > > > @@ -107,6 +108,12 @@
> > > >  #define Y_COOR(y)				((y) << 16)
> > > >  #define X_COOR(x)				((x) << 0)
> > > >
> > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > +
> > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > +
> > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > @@ -305,6 +312,10 @@
> > > >  #define DNRM_F(n)				((n) << 16)
> > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > >
> > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > +
> > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > +
> > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > @@ -389,6 +400,15 @@
> > > >  #define IMAGE_MAX_WIDTH				1920
> > > >  #define IMAGE_MAX_HEIGH				1080
> > > >
> > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > >
> > > Should this be in the uAPI header as it is useful to userspace as well ?
> > >
> > > you could:
> > >
> > > struct jh7110_isp_sc_buffer {
> > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > 	__u32 reserv0[33];
> > > 	__u32 bright_sc[4096];
> > > 	__u32 reserv1[96];
> > > 	__u32 ae_hist_y[128];
> > > 	__u32 reserv2[511];
> > > 	__u16 flag;
> > > };
> > >
> > > ofc if the size is made part of the uAPI you need a more proper name
> > > such as JH7110_ISP_YHIST_SIZE
> > >
> >
> > OK, I will try this.
> >
> > > > +
> > > > +enum stf_isp_type_scd {
> > > > +	TYPE_DEC = 0,
> > > > +	TYPE_OBC,
> > > > +	TYPE_OECF,
> > > > +	TYPE_AWB,
> > > > +};
> > > > +
> > > >  /* pad id for media framework */
> > > >  enum stf_isp_pad_id {
> > > >  	STF_ISP_PAD_SINK = 0,
> > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > > > *isp_dev);
> > > >
> > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > +		      enum stf_isp_type_scd type_scd);
> > > >
> > > >  #endif /* STF_ISP_H */
> > > > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> > > stfcamss_video *video)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int stf_video_scd_init_format(struct stfcamss_video
> > > > +*video)
> > >
> > > Make it void if it can't fail (see below)
> > >
> >
> > OK.
> >
> > > > +{
> > > > +	video->active_fmt.fmt.meta.dataformat =
> > > video->formats[0].pixelformat;
> > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > +jh7110_isp_sc_buffer);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * Video queue operations
> > > >   */
> > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > stf_video_vb2_q_ops =
> > > {
> > > >  	.stop_streaming  = video_stop_streaming,  };
> > > >
> > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > +				 unsigned int *num_buffers,
> > > > +				 unsigned int *num_planes,
> > > > +				 unsigned int sizes[],
> > > > +				 struct device *alloc_devs[]) {
> > > > +	if (*num_planes)
> > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL :
> > > > +0;
> > > > +
> > > > +	*num_planes = 1;
> > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > +	dma_addr_t *paddr;
> > > > +
> > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > +	buffer->addr[0] = *paddr;
> > > > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > >
> > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > mainline and I'm not sure what this gives you as you use it to program the
> following registers:
> > >
> > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> > >
> >
> > We set the value for ISP hardware, then ISP will transfer the statistics to the
> buffer.
> > when the stf_isp_irq_handler interrupt is triggered, indicates that
> > the buffer fill is complete.
> >
> 
> So I take this as
> 
> 	paddr = vb2_plane_cookie(vb, 0);
> 	buffer->addr[0] = *paddr;
> 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> 
>         stf_set_scd_addr(stfcamss, change_buf->addr[0],
>                          change_buf->addr[1], type_scd);
> 
> Makes the ISP transfer data directly to the memory areas in addr[0] and addr[1]
> (which explains why struct jh7110_isp_sc_buffer is packed, as it has to match
> the HW registers layout)
> 
> If this is the case, why are you then manually copying the histograms and the
> flags to vaddr ?
> 

Yes, your are right.
But actually there is a problem with our ISP RTL.
We set this yhist_addr to ISP, but it actually not work.
	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
or I will drop this line in next version.

So, in this structure
struct jh7110_isp_sc_buffer {
	__u32 y_histogram[64];
	__u32 reserv0[33];
	__u32 bright_sc[4096];
	__u32 reserv1[96];
	__u32 ae_hist_y[128];
	__u32 reserv2[511];
	__u16 flag;
};

Only 

	__u32 reserv0[33];
	__u32 bright_sc[4096];
	__u32 reserv1[96];
	__u32 ae_hist_y[128];
	__u32 reserv2[511];

transfer by ISP hardware.

I need to fill 
	__u32 y_histogram[64];
	__u16 flag;

by vaddr.

Regards,
Changhuang

>                 ready_buf = stf_buf_done(&cap_scd->buffers);
>                 if (ready_buf) {
>                         stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
>                         vb2_buffer_done(&ready_buf->vb.vb2_buf,
> VB2_BUF_STATE_DONE);
>                 }
> 
>                 change_buf = stf_change_buffer(&cap_scd->buffers);
>                 if (change_buf) {
>                         stf_isp_fill_flag(stfcamss, change_buf->vaddr,
>                                           &type_scd);
> 
> If I read vb2_dc_alloc_coherent() right 'cookie' == 'vaddr'
> 
> static int vb2_dc_alloc_coherent(struct vb2_dc_buf *buf) {
> 	struct vb2_queue *q = buf->vb->vb2_queue;
> 
> 	buf->cookie = dma_alloc_attrs(buf->dev,
> 				      buf->size,
> 				      &buf->dma_addr,
> 				      GFP_KERNEL | q->gfp_flags,
> 				      buf->attrs);
> 	if (!buf->cookie)
> 		return -ENOMEM;
> 
> 	if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
> 		return 0;
> 
> 	buf->vaddr = buf->cookie;
> 	return 0;
> }
> 
> Could you verify what you get by printing out 'paddr' and 'vaddr' in
> video_scd_buf_init() ?
> 
> 
> > >
> > > > +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int video_scd_buf_prepare(struct vb2_buffer *vb) {
> > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > +
> > > > +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> > > > +		return -EINVAL;
> > > > +
> > > > +	vb2_set_plane_payload(vb, 0, sizeof(struct
> > > > +jh7110_isp_sc_buffer));
> > > > +
> > > > +	vbuf->field = V4L2_FIELD_NONE;
> > >
> > > is this necessary ?
> > >
> >
> > Will drop it.
> >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int video_scd_start_streaming(struct vb2_queue *q,
> > > > +unsigned int count) {
> > > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > > +
> > > > +	video->ops->start_streaming(video);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void video_scd_stop_streaming(struct vb2_queue *q) {
> > > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > > +
> > > > +	video->ops->stop_streaming(video);
> > > > +
> > > > +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR); }
> > > > +
> > > > +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> > > > +	.queue_setup     = video_scd_queue_setup,
> > > > +	.wait_prepare    = vb2_ops_wait_prepare,
> > > > +	.wait_finish     = vb2_ops_wait_finish,
> > > > +	.buf_init        = video_scd_buf_init,
> > > > +	.buf_prepare     = video_scd_buf_prepare,
> > > > +	.buf_queue       = video_buf_queue,
> > > > +	.start_streaming = video_scd_start_streaming,
> > > > +	.stop_streaming  = video_scd_stop_streaming, };
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * V4L2 ioctls
> > > >   */
> > > > @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops
> > > > stf_vid_ioctl_ops
> > > = {
> > > >  	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > > >  };
> > > >
> > > > +static int video_scd_g_fmt(struct file *file, void *fh, struct
> > > > +v4l2_format *f) {
> > > > +	struct stfcamss_video *video = video_drvdata(file);
> > > > +	struct v4l2_meta_format *meta = &f->fmt.meta;
> > > > +
> > > > +	if (f->type != video->type)
> > > > +		return -EINVAL;
> > > > +
> > > > +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> > > > +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> > > > +	.vidioc_querycap                = video_querycap,
> > > > +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> > > > +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> > > > +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> > > > +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> > > > +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> > > > +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> > > > +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> > > > +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> > > > +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> > > > +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> > > > +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> > > > +	.vidioc_streamon                = vb2_ioctl_streamon,
> > > > +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > > > +};
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * V4L2 file operations
> > > >   */
> > > > @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
> > > >  	struct stfcamss_video *video = video_get_drvdata(vdev);
> > > >  	int ret;
> > > >
> > > > +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> > > > +		return 0;
> > > > +
> > > >  	ret = stf_video_check_format(video);
> > > >
> > > >  	return ret;
> > > > @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video
> > > *video,
> > > >  	q = &video->vb2_q;
> > > >  	q->drv_priv = video;
> > > >  	q->mem_ops = &vb2_dma_contig_memops;
> > > > -	q->ops = &stf_video_vb2_q_ops;
> > > > +
> > > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > > +		q->ops = &stf_video_vb2_q_ops;
> > > > +	else
> > > > +		q->ops = &stf_video_scd_vb2_q_ops;
> > > >  	q->type = video->type;
> > > >  	q->io_modes = VB2_DMABUF | VB2_MMAP;
> > > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video
> > > *video,
> > > >  		goto err_mutex_destroy;
> > > >  	}
> > > >
> > > > -	ret = stf_video_init_format(video);
> > > > -	if (ret < 0) {
> > > > -		dev_err(video->stfcamss->dev,
> > > > -			"Failed to init format: %d\n", ret);
> > > > -		goto err_media_cleanup;
> > > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > > > +		ret = stf_video_init_format(video);
> > >
> > > I don't think this can fail
> > >
> >
> > This already exists, and I probably will not change it here.
> >
> 
> No problem! Maybe something for later when de-staging the driver.
> 
> Thanks
>   j
> > >
Jacopo Mondi July 12, 2024, 11:37 a.m. UTC | #5
Hi Changhuang

On Fri, Jul 12, 2024 at 08:36:21AM GMT, Changhuang Liang wrote:
> Hi, Jacopo
>
> [...]
> > > > > +
> > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > +		      enum stf_isp_type_scd type_scd) {
> > > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > > > > +			    SEL_TYPE(type_scd));
> > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); }
> > > > > +
> > > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void
> > > > > +*vaddr) {
> > > > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> > *)vaddr;
> > > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > > +	u32 i;
> > > > > +
> > > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
> > > >
> > > > If you have a contigous memory space to read, could memcpy_fromio()
> > > > help instead of going through 64 reads ?
> > > >
> > >
> > > I will try this function.
> > >
> > > > > +}
> > > > > +
> > > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > > +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer
> > > > > +*)vaddr;
> > > > > +
> > > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > > +	if (*type_scd == TYPE_AWB) {
> > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > > +		*type_scd = TYPE_OECF;
> > > > > +	} else {
> > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > > +		*type_scd = TYPE_AWB;
> > > >
> > > > Is this correct ? Why are you overwriting the value read from HW
> > > > that indicates AE/AF stats with AWB ones ?
> > >
> > > The AWB frame and AE/AF frames will alternate, so the current frame
> > > indicates the AE/AF, then set AWB type just for next AWB frame.
> > >
> >
> > Ah! Shouldn't it be userspace configuring which type of statistics it wants to
> > receive instead of the driver alternating the two ?
> >
>
> No, this is determined by hardware, cannot be configured by userspace.
>


So this
	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
			    SEL_TYPE(type_scd));

doesn't actually select which stats type you get from the HW

> > > >
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > > >  	struct stfcamss *stfcamss = priv;
> > > > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > +	struct stf_capture *cap_scd =
> > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > >  	struct stfcamss_buffer *change_buf;
> > > > > +	enum stf_isp_type_scd type_scd;
> > > > > +	u32 value;
> > > > >  	u32 status;
> > > > >
> > > > >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0); @@
> > > > > -467,6
> > > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
> > > > >  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
> > > > >  							 change_buf->addr[1]);
> > > > >  			}
> > > > > +
> > > > > +			value = stf_isp_reg_read(stfcamss,
> > > > ISP_REG_CSI_MODULE_CFG);
> > > > > +			if (value & CSI_SC_EN) {
> > > > > +				change_buf = stf_change_buffer(&cap_scd->buffers);
> > > > > +				if (change_buf) {
> > > > > +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> > > > > +							  &type_scd);
> > > > > +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > > > +							 change_buf->addr[1], type_scd);
> > > >
> > > > Sorry if I'm un-familiar with the HW but this seems to be the line-interrupt.
> > > > Are you swapping buffers every line or it's just that you have a
> > > > single line irq for the stats ?
> > > >
> > >
> > > Every frame triggers a line-interrupt, and we will swap buffers in it.
> > >
> >
> > ah, frames completion triggers a line-interrupt ?
> >
>
> Every frame will trigger line-interrupt and stf_isp_irq_handler.
> We use line-interrupt changing buffer, the stf_isp_irq_handler will indicate that
> image transfer to DDR is complete.
>
>
> > > > > +				}
> > > > > +			}
> > > > >  		}
> > > > >
> > > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@ -485,6
> > +542,7
> > > > @@
> > > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > > >  	struct stfcamss *stfcamss = priv;
> > > > >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > +	struct stf_capture *cap_scd =
> > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > >  	struct stfcamss_buffer *ready_buf;
> > > > >  	u32 status;
> > > > >
> > > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> > > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > VB2_BUF_STATE_DONE);
> > > > >  		}
> > > > >
> > > > > +		if (status & ISPC_SC) {
> > > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > > +			if (ready_buf) {
> > > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > VB2_BUF_STATE_DONE);
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > @@ -10,6 +10,7 @@
> > > > >  #ifndef STF_ISP_H
> > > > >  #define STF_ISP_H
> > > > >
> > > > > +#include <linux/jh7110-isp.h>
> > > > >  #include <media/v4l2-subdev.h>
> > > > >
> > > > >  #include "stf-video.h"
> > > > > @@ -107,6 +108,12 @@
> > > > >  #define Y_COOR(y)				((y) << 16)
> > > > >  #define X_COOR(x)				((x) << 0)
> > > > >
> > > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > > +
> > > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > > +
> > > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > > @@ -305,6 +312,10 @@
> > > > >  #define DNRM_F(n)				((n) << 16)
> > > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > > >
> > > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > > +
> > > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > > +
> > > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > > @@ -389,6 +400,15 @@
> > > > >  #define IMAGE_MAX_WIDTH				1920
> > > > >  #define IMAGE_MAX_HEIGH				1080
> > > > >
> > > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > > >
> > > > Should this be in the uAPI header as it is useful to userspace as well ?
> > > >
> > > > you could:
> > > >
> > > > struct jh7110_isp_sc_buffer {
> > > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > > 	__u32 reserv0[33];
> > > > 	__u32 bright_sc[4096];
> > > > 	__u32 reserv1[96];
> > > > 	__u32 ae_hist_y[128];
> > > > 	__u32 reserv2[511];
> > > > 	__u16 flag;
> > > > };
> > > >
> > > > ofc if the size is made part of the uAPI you need a more proper name
> > > > such as JH7110_ISP_YHIST_SIZE
> > > >
> > >
> > > OK, I will try this.
> > >
> > > > > +
> > > > > +enum stf_isp_type_scd {
> > > > > +	TYPE_DEC = 0,
> > > > > +	TYPE_OBC,
> > > > > +	TYPE_OECF,
> > > > > +	TYPE_AWB,
> > > > > +};
> > > > > +
> > > > >  /* pad id for media framework */
> > > > >  enum stf_isp_pad_id {
> > > > >  	STF_ISP_PAD_SINK = 0,
> > > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > > > > *isp_dev);
> > > > >
> > > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > +		      enum stf_isp_type_scd type_scd);
> > > > >
> > > > >  #endif /* STF_ISP_H */
> > > > > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> > > > stfcamss_video *video)
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int stf_video_scd_init_format(struct stfcamss_video
> > > > > +*video)
> > > >
> > > > Make it void if it can't fail (see below)
> > > >
> > >
> > > OK.
> > >
> > > > > +{
> > > > > +	video->active_fmt.fmt.meta.dataformat =
> > > > video->formats[0].pixelformat;
> > > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > > +jh7110_isp_sc_buffer);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * Video queue operations
> > > > >   */
> > > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > > stf_video_vb2_q_ops =
> > > > {
> > > > >  	.stop_streaming  = video_stop_streaming,  };
> > > > >
> > > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > > +				 unsigned int *num_buffers,
> > > > > +				 unsigned int *num_planes,
> > > > > +				 unsigned int sizes[],
> > > > > +				 struct device *alloc_devs[]) {
> > > > > +	if (*num_planes)
> > > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL :
> > > > > +0;
> > > > > +
> > > > > +	*num_planes = 1;
> > > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > > +	dma_addr_t *paddr;
> > > > > +
> > > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > > +	buffer->addr[0] = *paddr;
> > > > > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > >
> > > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > > mainline and I'm not sure what this gives you as you use it to program the
> > following registers:
> > > >
> > > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> > > >
> > >
> > > We set the value for ISP hardware, then ISP will transfer the statistics to the
> > buffer.
> > > when the stf_isp_irq_handler interrupt is triggered, indicates that
> > > the buffer fill is complete.
> > >
> >
> > So I take this as
> >
> > 	paddr = vb2_plane_cookie(vb, 0);
> > 	buffer->addr[0] = *paddr;
> > 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> >
> >         stf_set_scd_addr(stfcamss, change_buf->addr[0],
> >                          change_buf->addr[1], type_scd);
> >
> > Makes the ISP transfer data directly to the memory areas in addr[0] and addr[1]
> > (which explains why struct jh7110_isp_sc_buffer is packed, as it has to match
> > the HW registers layout)
> >
> > If this is the case, why are you then manually copying the histograms and the
> > flags to vaddr ?
> >
>
> Yes, your are right.
> But actually there is a problem with our ISP RTL.
> We set this yhist_addr to ISP, but it actually not work.
> 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> or I will drop this line in next version.
>
> So, in this structure
> struct jh7110_isp_sc_buffer {
> 	__u32 y_histogram[64];
> 	__u32 reserv0[33];
> 	__u32 bright_sc[4096];
> 	__u32 reserv1[96];
> 	__u32 ae_hist_y[128];
> 	__u32 reserv2[511];
> 	__u16 flag;
> };
>
> Only
>
> 	__u32 reserv0[33];
> 	__u32 bright_sc[4096];
> 	__u32 reserv1[96];
> 	__u32 ae_hist_y[128];
> 	__u32 reserv2[511];
>
> transfer by ISP hardware.
>
> I need to fill
> 	__u32 y_histogram[64];
> 	__u16 flag;
>
> by vaddr.

I see.

Apart from the fact you can drop paddr and vb2_plane_cookie() and use
vaddr for all (if I'm not mistaken), could you please record the above
rationale for manually filling y_histogram and flag by hand in a
comment to avoid future readers being confused by this as I was ?

Thank you
   j

>
> Regards,
> Changhuang
>
> >                 ready_buf = stf_buf_done(&cap_scd->buffers);
> >                 if (ready_buf) {
> >                         stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> >                         vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > VB2_BUF_STATE_DONE);
> >                 }
> >
> >                 change_buf = stf_change_buffer(&cap_scd->buffers);
> >                 if (change_buf) {
> >                         stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> >                                           &type_scd);
> >
> > If I read vb2_dc_alloc_coherent() right 'cookie' == 'vaddr'
> >
> > static int vb2_dc_alloc_coherent(struct vb2_dc_buf *buf) {
> > 	struct vb2_queue *q = buf->vb->vb2_queue;
> >
> > 	buf->cookie = dma_alloc_attrs(buf->dev,
> > 				      buf->size,
> > 				      &buf->dma_addr,
> > 				      GFP_KERNEL | q->gfp_flags,
> > 				      buf->attrs);
> > 	if (!buf->cookie)
> > 		return -ENOMEM;
> >
> > 	if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
> > 		return 0;
> >
> > 	buf->vaddr = buf->cookie;
> > 	return 0;
> > }
> >
> > Could you verify what you get by printing out 'paddr' and 'vaddr' in
> > video_scd_buf_init() ?
> >
> >
> > > >
> > > > > +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int video_scd_buf_prepare(struct vb2_buffer *vb) {
> > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > +
> > > > > +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	vb2_set_plane_payload(vb, 0, sizeof(struct
> > > > > +jh7110_isp_sc_buffer));
> > > > > +
> > > > > +	vbuf->field = V4L2_FIELD_NONE;
> > > >
> > > > is this necessary ?
> > > >
> > >
> > > Will drop it.
> > >
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int video_scd_start_streaming(struct vb2_queue *q,
> > > > > +unsigned int count) {
> > > > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > > > +
> > > > > +	video->ops->start_streaming(video);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void video_scd_stop_streaming(struct vb2_queue *q) {
> > > > > +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> > > > > +
> > > > > +	video->ops->stop_streaming(video);
> > > > > +
> > > > > +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR); }
> > > > > +
> > > > > +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> > > > > +	.queue_setup     = video_scd_queue_setup,
> > > > > +	.wait_prepare    = vb2_ops_wait_prepare,
> > > > > +	.wait_finish     = vb2_ops_wait_finish,
> > > > > +	.buf_init        = video_scd_buf_init,
> > > > > +	.buf_prepare     = video_scd_buf_prepare,
> > > > > +	.buf_queue       = video_buf_queue,
> > > > > +	.start_streaming = video_scd_start_streaming,
> > > > > +	.stop_streaming  = video_scd_stop_streaming, };
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * V4L2 ioctls
> > > > >   */
> > > > > @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops
> > > > > stf_vid_ioctl_ops
> > > > = {
> > > > >  	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > > > >  };
> > > > >
> > > > > +static int video_scd_g_fmt(struct file *file, void *fh, struct
> > > > > +v4l2_format *f) {
> > > > > +	struct stfcamss_video *video = video_drvdata(file);
> > > > > +	struct v4l2_meta_format *meta = &f->fmt.meta;
> > > > > +
> > > > > +	if (f->type != video->type)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> > > > > +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> > > > > +	.vidioc_querycap                = video_querycap,
> > > > > +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> > > > > +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> > > > > +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> > > > > +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> > > > > +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> > > > > +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> > > > > +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> > > > > +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> > > > > +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> > > > > +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> > > > > +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> > > > > +	.vidioc_streamon                = vb2_ioctl_streamon,
> > > > > +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> > > > > +};
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * V4L2 file operations
> > > > >   */
> > > > > @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
> > > > >  	struct stfcamss_video *video = video_get_drvdata(vdev);
> > > > >  	int ret;
> > > > >
> > > > > +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> > > > > +		return 0;
> > > > > +
> > > > >  	ret = stf_video_check_format(video);
> > > > >
> > > > >  	return ret;
> > > > > @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video
> > > > *video,
> > > > >  	q = &video->vb2_q;
> > > > >  	q->drv_priv = video;
> > > > >  	q->mem_ops = &vb2_dma_contig_memops;
> > > > > -	q->ops = &stf_video_vb2_q_ops;
> > > > > +
> > > > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > > > +		q->ops = &stf_video_vb2_q_ops;
> > > > > +	else
> > > > > +		q->ops = &stf_video_scd_vb2_q_ops;
> > > > >  	q->type = video->type;
> > > > >  	q->io_modes = VB2_DMABUF | VB2_MMAP;
> > > > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > > @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video
> > > > *video,
> > > > >  		goto err_mutex_destroy;
> > > > >  	}
> > > > >
> > > > > -	ret = stf_video_init_format(video);
> > > > > -	if (ret < 0) {
> > > > > -		dev_err(video->stfcamss->dev,
> > > > > -			"Failed to init format: %d\n", ret);
> > > > > -		goto err_media_cleanup;
> > > > > +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > > > > +		ret = stf_video_init_format(video);
> > > >
> > > > I don't think this can fail
> > > >
> > >
> > > This already exists, and I probably will not change it here.
> > >
> >
> > No problem! Maybe something for later when de-staging the driver.
> >
> > Thanks
> >   j
> > > >
Changhuang Liang July 12, 2024, 1 p.m. UTC | #6
Hi, Jacopo

> 
> Hi Changhuang
> 
> On Fri, Jul 12, 2024 at 08:36:21AM GMT, Changhuang Liang wrote:
> > Hi, Jacopo
> >
> > [...]
> > > > > > +
> > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > +		      enum stf_isp_type_scd type_scd) {
> > > > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1,
> ISP_SC_SEL_MASK,
> > > > > > +			    SEL_TYPE(type_scd));
> > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > +yhist_addr); }
> > > > > > +
> > > > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss,
> > > > > > +void
> > > > > > +*vaddr) {
> > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > +jh7110_isp_sc_buffer
> > > *)vaddr;
> > > > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > > > +	u32 i;
> > > > > > +
> > > > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss,
> reg_addr);
> > > > >
> > > > > If you have a contigous memory space to read, could
> > > > > memcpy_fromio() help instead of going through 64 reads ?
> > > > >
> > > >
> > > > I will try this function.
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > > > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > +jh7110_isp_sc_buffer *)vaddr;
> > > > > > +
> > > > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > > > +	if (*type_scd == TYPE_AWB) {
> > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > > > +		*type_scd = TYPE_OECF;
> > > > > > +	} else {
> > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > > > +		*type_scd = TYPE_AWB;
> > > > >
> > > > > Is this correct ? Why are you overwriting the value read from HW
> > > > > that indicates AE/AF stats with AWB ones ?
> > > >
> > > > The AWB frame and AE/AF frames will alternate, so the current
> > > > frame indicates the AE/AF, then set AWB type just for next AWB frame.
> > > >
> > >
> > > Ah! Shouldn't it be userspace configuring which type of statistics
> > > it wants to receive instead of the driver alternating the two ?
> > >
> >
> > No, this is determined by hardware, cannot be configured by userspace.
> >
> 
> 
> So this
> 	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> 			    SEL_TYPE(type_scd));
> 
> doesn't actually select which stats type you get from the HW
> 

You can understand it that way. But it still needs to be written to work with the hardware.

> > > > >
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > >  	struct stf_capture *cap =
> > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > +	struct stf_capture *cap_scd =
> > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > >  	struct stfcamss_buffer *change_buf;
> > > > > > +	enum stf_isp_type_scd type_scd;
> > > > > > +	u32 value;
> > > > > >  	u32 status;
> > > > > >
> > > > > >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> @@
> > > > > > -467,6
> > > > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void
> > > > > > +*priv)
> > > > > >  					stf_set_yuv_addr(stfcamss,
> change_buf->addr[0],
> > > > > >  							 change_buf->addr[1]);
> > > > > >  			}
> > > > > > +
> > > > > > +			value = stf_isp_reg_read(stfcamss,
> > > > > ISP_REG_CSI_MODULE_CFG);
> > > > > > +			if (value & CSI_SC_EN) {
> > > > > > +				change_buf =
> stf_change_buffer(&cap_scd->buffers);
> > > > > > +				if (change_buf) {
> > > > > > +					stf_isp_fill_flag(stfcamss,
> change_buf->vaddr,
> > > > > > +							  &type_scd);
> > > > > > +					stf_set_scd_addr(stfcamss,
> change_buf->addr[0],
> > > > > > +							 change_buf->addr[1], type_scd);
> > > > >
> > > > > Sorry if I'm un-familiar with the HW but this seems to be the
> line-interrupt.
> > > > > Are you swapping buffers every line or it's just that you have a
> > > > > single line irq for the stats ?
> > > > >
> > > >
> > > > Every frame triggers a line-interrupt, and we will swap buffers in it.
> > > >
> > >
> > > ah, frames completion triggers a line-interrupt ?
> > >
> >
> > Every frame will trigger line-interrupt and stf_isp_irq_handler.
> > We use line-interrupt changing buffer, the stf_isp_irq_handler will
> > indicate that image transfer to DDR is complete.
> >
> >
> > > > > > +				}
> > > > > > +			}
> > > > > >  		}
> > > > > >
> > > > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@
> -485,6
> > > +542,7
> > > > > @@
> > > > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > >  	struct stf_capture *cap =
> > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > +	struct stf_capture *cap_scd =
> > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > >  	struct stfcamss_buffer *ready_buf;
> > > > > >  	u32 status;
> > > > > >
> > > > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void
> *priv)
> > > > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > VB2_BUF_STATE_DONE);
> > > > > >  		}
> > > > > >
> > > > > > +		if (status & ISPC_SC) {
> > > > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > > > +			if (ready_buf) {
> > > > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > VB2_BUF_STATE_DONE);
> > > > > > +			}
> > > > > > +		}
> > > > > > +
> > > > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > @@ -10,6 +10,7 @@
> > > > > >  #ifndef STF_ISP_H
> > > > > >  #define STF_ISP_H
> > > > > >
> > > > > > +#include <linux/jh7110-isp.h>
> > > > > >  #include <media/v4l2-subdev.h>
> > > > > >
> > > > > >  #include "stf-video.h"
> > > > > > @@ -107,6 +108,12 @@
> > > > > >  #define Y_COOR(y)				((y) << 16)
> > > > > >  #define X_COOR(x)				((x) << 0)
> > > > > >
> > > > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > > > +
> > > > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > > > +
> > > > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > > > @@ -305,6 +312,10 @@
> > > > > >  #define DNRM_F(n)				((n) << 16)
> > > > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > > > >
> > > > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > > > +
> > > > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > > > +
> > > > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > > > @@ -389,6 +400,15 @@
> > > > > >  #define IMAGE_MAX_WIDTH				1920
> > > > > >  #define IMAGE_MAX_HEIGH				1080
> > > > > >
> > > > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > > > >
> > > > > Should this be in the uAPI header as it is useful to userspace as well ?
> > > > >
> > > > > you could:
> > > > >
> > > > > struct jh7110_isp_sc_buffer {
> > > > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > > > 	__u32 reserv0[33];
> > > > > 	__u32 bright_sc[4096];
> > > > > 	__u32 reserv1[96];
> > > > > 	__u32 ae_hist_y[128];
> > > > > 	__u32 reserv2[511];
> > > > > 	__u16 flag;
> > > > > };
> > > > >
> > > > > ofc if the size is made part of the uAPI you need a more proper
> > > > > name such as JH7110_ISP_YHIST_SIZE
> > > > >
> > > >
> > > > OK, I will try this.
> > > >
> > > > > > +
> > > > > > +enum stf_isp_type_scd {
> > > > > > +	TYPE_DEC = 0,
> > > > > > +	TYPE_OBC,
> > > > > > +	TYPE_OECF,
> > > > > > +	TYPE_AWB,
> > > > > > +};
> > > > > > +
> > > > > >  /* pad id for media framework */  enum stf_isp_pad_id {
> > > > > >  	STF_ISP_PAD_SINK = 0,
> > > > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > > > > > *isp_dev);
> > > > > >
> > > > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > +		      enum stf_isp_type_scd type_scd);
> > > > > >
> > > > > >  #endif /* STF_ISP_H */
> > > > > > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> > > > > stfcamss_video *video)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int stf_video_scd_init_format(struct stfcamss_video
> > > > > > +*video)
> > > > >
> > > > > Make it void if it can't fail (see below)
> > > > >
> > > >
> > > > OK.
> > > >
> > > > > > +{
> > > > > > +	video->active_fmt.fmt.meta.dataformat =
> > > > > video->formats[0].pixelformat;
> > > > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > > > +jh7110_isp_sc_buffer);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  /* -----------------------------------------------------------------------------
> > > > > >   * Video queue operations
> > > > > >   */
> > > > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > > > stf_video_vb2_q_ops =
> > > > > {
> > > > > >  	.stop_streaming  = video_stop_streaming,  };
> > > > > >
> > > > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > > > +				 unsigned int *num_buffers,
> > > > > > +				 unsigned int *num_planes,
> > > > > > +				 unsigned int sizes[],
> > > > > > +				 struct device *alloc_devs[]) {
> > > > > > +	if (*num_planes)
> > > > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ?
> -EINVAL :
> > > > > > +0;
> > > > > > +
> > > > > > +	*num_planes = 1;
> > > > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > > > +	dma_addr_t *paddr;
> > > > > > +
> > > > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > > > +	buffer->addr[0] = *paddr;
> > > > > > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > > >
> > > > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > > > mainline and I'm not sure what this gives you as you use it to
> > > > > program the
> > > following registers:
> > > > >
> > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> > > > >
> > > >
> > > > We set the value for ISP hardware, then ISP will transfer the
> > > > statistics to the
> > > buffer.
> > > > when the stf_isp_irq_handler interrupt is triggered, indicates
> > > > that the buffer fill is complete.
> > > >
> > >
> > > So I take this as
> > >
> > > 	paddr = vb2_plane_cookie(vb, 0);
> > > 	buffer->addr[0] = *paddr;
> > > 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > >
> > >         stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > >                          change_buf->addr[1], type_scd);
> > >
> > > Makes the ISP transfer data directly to the memory areas in addr[0]
> > > and addr[1] (which explains why struct jh7110_isp_sc_buffer is
> > > packed, as it has to match the HW registers layout)
> > >
> > > If this is the case, why are you then manually copying the
> > > histograms and the flags to vaddr ?
> > >
> >
> > Yes, your are right.
> > But actually there is a problem with our ISP RTL.
> > We set this yhist_addr to ISP, but it actually not work.
> > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); or I
> > will drop this line in next version.
> >
> > So, in this structure
> > struct jh7110_isp_sc_buffer {
> > 	__u32 y_histogram[64];
> > 	__u32 reserv0[33];
> > 	__u32 bright_sc[4096];
> > 	__u32 reserv1[96];
> > 	__u32 ae_hist_y[128];
> > 	__u32 reserv2[511];
> > 	__u16 flag;
> > };
> >
> > Only
> >
> > 	__u32 reserv0[33];
> > 	__u32 bright_sc[4096];
> > 	__u32 reserv1[96];
> > 	__u32 ae_hist_y[128];
> > 	__u32 reserv2[511];
> >
> > transfer by ISP hardware.
> >
> > I need to fill
> > 	__u32 y_histogram[64];
> > 	__u16 flag;
> >
> > by vaddr.
> 
> I see.
> 
> Apart from the fact you can drop paddr and vb2_plane_cookie() and use vaddr
> for all (if I'm not mistaken), could you please record the above rationale for
> manually filling y_histogram and flag by hand in a comment to avoid future
> readers being confused by this as I was ?
> 

Still need to keep the paddr and vb2_plane_cookie() for 
	__u32 reserv0[33];
	__u32 bright_sc[4096];
	__u32 reserv1[96];
	__u32 ae_hist_y[128];
 	__u32 reserv2[511];

Because this part is filled by the hardware.

I will add more information for struct jh7110_isp_sc_buffer

Regards,
Changhuang
Jacopo Mondi July 12, 2024, 1:27 p.m. UTC | #7
Hi Changhuang

On Fri, Jul 12, 2024 at 01:00:03PM GMT, Changhuang Liang wrote:
> Hi, Jacopo
>
> >
> > Hi Changhuang
> >
> > On Fri, Jul 12, 2024 at 08:36:21AM GMT, Changhuang Liang wrote:
> > > Hi, Jacopo
> > >
> > > [...]
> > > > > > > +
> > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > +		      enum stf_isp_type_scd type_scd) {
> > > > > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1,
> > ISP_SC_SEL_MASK,
> > > > > > > +			    SEL_TYPE(type_scd));
> > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > > +yhist_addr); }
> > > > > > > +
> > > > > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss,
> > > > > > > +void
> > > > > > > +*vaddr) {
> > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > +jh7110_isp_sc_buffer
> > > > *)vaddr;
> > > > > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > > > > +	u32 i;
> > > > > > > +
> > > > > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss,
> > reg_addr);
> > > > > >
> > > > > > If you have a contigous memory space to read, could
> > > > > > memcpy_fromio() help instead of going through 64 reads ?
> > > > > >
> > > > >
> > > > > I will try this function.
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> > > > > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > +jh7110_isp_sc_buffer *)vaddr;
> > > > > > > +
> > > > > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > > > > +	if (*type_scd == TYPE_AWB) {
> > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > > > > +		*type_scd = TYPE_OECF;
> > > > > > > +	} else {
> > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > > > > +		*type_scd = TYPE_AWB;
> > > > > >
> > > > > > Is this correct ? Why are you overwriting the value read from HW
> > > > > > that indicates AE/AF stats with AWB ones ?
> > > > >
> > > > > The AWB frame and AE/AF frames will alternate, so the current
> > > > > frame indicates the AE/AF, then set AWB type just for next AWB frame.
> > > > >
> > > >
> > > > Ah! Shouldn't it be userspace configuring which type of statistics
> > > > it wants to receive instead of the driver alternating the two ?
> > > >
> > >
> > > No, this is determined by hardware, cannot be configured by userspace.
> > >
> >
> >
> > So this
> > 	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > 			    SEL_TYPE(type_scd));
> >
> > doesn't actually select which stats type you get from the HW
> >
>
> You can understand it that way. But it still needs to be written to work with the hardware.
>

ack, maybe a comment here as well would help

> > > > > >
> > > > > > > +	}
> > > > > > > +}
> > > > > > > +
> > > > > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > >  	struct stf_capture *cap =
> > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > >  	struct stfcamss_buffer *change_buf;
> > > > > > > +	enum stf_isp_type_scd type_scd;
> > > > > > > +	u32 value;
> > > > > > >  	u32 status;
> > > > > > >
> > > > > > >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> > @@
> > > > > > > -467,6
> > > > > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void
> > > > > > > +*priv)
> > > > > > >  					stf_set_yuv_addr(stfcamss,
> > change_buf->addr[0],
> > > > > > >  							 change_buf->addr[1]);
> > > > > > >  			}
> > > > > > > +
> > > > > > > +			value = stf_isp_reg_read(stfcamss,
> > > > > > ISP_REG_CSI_MODULE_CFG);
> > > > > > > +			if (value & CSI_SC_EN) {
> > > > > > > +				change_buf =
> > stf_change_buffer(&cap_scd->buffers);
> > > > > > > +				if (change_buf) {
> > > > > > > +					stf_isp_fill_flag(stfcamss,
> > change_buf->vaddr,
> > > > > > > +							  &type_scd);
> > > > > > > +					stf_set_scd_addr(stfcamss,
> > change_buf->addr[0],
> > > > > > > +							 change_buf->addr[1], type_scd);
> > > > > >
> > > > > > Sorry if I'm un-familiar with the HW but this seems to be the
> > line-interrupt.
> > > > > > Are you swapping buffers every line or it's just that you have a
> > > > > > single line irq for the stats ?
> > > > > >
> > > > >
> > > > > Every frame triggers a line-interrupt, and we will swap buffers in it.
> > > > >
> > > >
> > > > ah, frames completion triggers a line-interrupt ?
> > > >
> > >
> > > Every frame will trigger line-interrupt and stf_isp_irq_handler.
> > > We use line-interrupt changing buffer, the stf_isp_irq_handler will
> > > indicate that image transfer to DDR is complete.
> > >
> > >
> > > > > > > +				}
> > > > > > > +			}
> > > > > > >  		}
> > > > > > >
> > > > > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@
> > -485,6
> > > > +542,7
> > > > > > @@
> > > > > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > >  	struct stf_capture *cap =
> > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > >  	struct stfcamss_buffer *ready_buf;
> > > > > > >  	u32 status;
> > > > > > >
> > > > > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void
> > *priv)
> > > > > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > VB2_BUF_STATE_DONE);
> > > > > > >  		}
> > > > > > >
> > > > > > > +		if (status & ISPC_SC) {
> > > > > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > > > > +			if (ready_buf) {
> > > > > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > VB2_BUF_STATE_DONE);
> > > > > > > +			}
> > > > > > > +		}
> > > > > > > +
> > > > > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > > > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > > > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > @@ -10,6 +10,7 @@
> > > > > > >  #ifndef STF_ISP_H
> > > > > > >  #define STF_ISP_H
> > > > > > >
> > > > > > > +#include <linux/jh7110-isp.h>
> > > > > > >  #include <media/v4l2-subdev.h>
> > > > > > >
> > > > > > >  #include "stf-video.h"
> > > > > > > @@ -107,6 +108,12 @@
> > > > > > >  #define Y_COOR(y)				((y) << 16)
> > > > > > >  #define X_COOR(x)				((x) << 0)
> > > > > > >
> > > > > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > > > > +
> > > > > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > > > > +
> > > > > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > > > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > > > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > > > > @@ -305,6 +312,10 @@
> > > > > > >  #define DNRM_F(n)				((n) << 16)
> > > > > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > > > > >
> > > > > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > > > > +
> > > > > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > > > > +
> > > > > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > > > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > > > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > > > > @@ -389,6 +400,15 @@
> > > > > > >  #define IMAGE_MAX_WIDTH				1920
> > > > > > >  #define IMAGE_MAX_HEIGH				1080
> > > > > > >
> > > > > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > > > > >
> > > > > > Should this be in the uAPI header as it is useful to userspace as well ?
> > > > > >
> > > > > > you could:
> > > > > >
> > > > > > struct jh7110_isp_sc_buffer {
> > > > > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > > > > 	__u32 reserv0[33];
> > > > > > 	__u32 bright_sc[4096];
> > > > > > 	__u32 reserv1[96];
> > > > > > 	__u32 ae_hist_y[128];
> > > > > > 	__u32 reserv2[511];
> > > > > > 	__u16 flag;
> > > > > > };
> > > > > >
> > > > > > ofc if the size is made part of the uAPI you need a more proper
> > > > > > name such as JH7110_ISP_YHIST_SIZE
> > > > > >
> > > > >
> > > > > OK, I will try this.
> > > > >
> > > > > > > +
> > > > > > > +enum stf_isp_type_scd {
> > > > > > > +	TYPE_DEC = 0,
> > > > > > > +	TYPE_OBC,
> > > > > > > +	TYPE_OECF,
> > > > > > > +	TYPE_AWB,
> > > > > > > +};
> > > > > > > +
> > > > > > >  /* pad id for media framework */  enum stf_isp_pad_id {
> > > > > > >  	STF_ISP_PAD_SINK = 0,
> > > > > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev
> > > > > > > *isp_dev);
> > > > > > >
> > > > > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > > > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > +		      enum stf_isp_type_scd type_scd);
> > > > > > >
> > > > > > >  #endif /* STF_ISP_H */
> > > > > > > diff --git a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > @@ -125,6 +125,14 @@ static int stf_video_init_format(struct
> > > > > > stfcamss_video *video)
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int stf_video_scd_init_format(struct stfcamss_video
> > > > > > > +*video)
> > > > > >
> > > > > > Make it void if it can't fail (see below)
> > > > > >
> > > > >
> > > > > OK.
> > > > >
> > > > > > > +{
> > > > > > > +	video->active_fmt.fmt.meta.dataformat =
> > > > > > video->formats[0].pixelformat;
> > > > > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > > > > +jh7110_isp_sc_buffer);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* -----------------------------------------------------------------------------
> > > > > > >   * Video queue operations
> > > > > > >   */
> > > > > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > > > > stf_video_vb2_q_ops =
> > > > > > {
> > > > > > >  	.stop_streaming  = video_stop_streaming,  };
> > > > > > >
> > > > > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > > > > +				 unsigned int *num_buffers,
> > > > > > > +				 unsigned int *num_planes,
> > > > > > > +				 unsigned int sizes[],
> > > > > > > +				 struct device *alloc_devs[]) {
> > > > > > > +	if (*num_planes)
> > > > > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ?
> > -EINVAL :
> > > > > > > +0;
> > > > > > > +
> > > > > > > +	*num_planes = 1;
> > > > > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > > > > +	dma_addr_t *paddr;
> > > > > > > +
> > > > > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > > > > +	buffer->addr[0] = *paddr;
> > > > > > > +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > > > >
> > > > > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > > > > mainline and I'm not sure what this gives you as you use it to
> > > > > > program the
> > > > following registers:
> > > > > >
> > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> > > > > >
> > > > >
> > > > > We set the value for ISP hardware, then ISP will transfer the
> > > > > statistics to the
> > > > buffer.
> > > > > when the stf_isp_irq_handler interrupt is triggered, indicates
> > > > > that the buffer fill is complete.
> > > > >
> > > >
> > > > So I take this as
> > > >
> > > > 	paddr = vb2_plane_cookie(vb, 0);
> > > > 	buffer->addr[0] = *paddr;
> > > > 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > >
> > > >         stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > >                          change_buf->addr[1], type_scd);
> > > >
> > > > Makes the ISP transfer data directly to the memory areas in addr[0]
> > > > and addr[1] (which explains why struct jh7110_isp_sc_buffer is
> > > > packed, as it has to match the HW registers layout)
> > > >
> > > > If this is the case, why are you then manually copying the
> > > > histograms and the flags to vaddr ?
> > > >
> > >
> > > Yes, your are right.
> > > But actually there is a problem with our ISP RTL.
> > > We set this yhist_addr to ISP, but it actually not work.
> > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); or I
> > > will drop this line in next version.
> > >
> > > So, in this structure
> > > struct jh7110_isp_sc_buffer {
> > > 	__u32 y_histogram[64];
> > > 	__u32 reserv0[33];
> > > 	__u32 bright_sc[4096];
> > > 	__u32 reserv1[96];
> > > 	__u32 ae_hist_y[128];
> > > 	__u32 reserv2[511];
> > > 	__u16 flag;
> > > };
> > >
> > > Only
> > >
> > > 	__u32 reserv0[33];
> > > 	__u32 bright_sc[4096];
> > > 	__u32 reserv1[96];
> > > 	__u32 ae_hist_y[128];
> > > 	__u32 reserv2[511];
> > >
> > > transfer by ISP hardware.
> > >
> > > I need to fill
> > > 	__u32 y_histogram[64];
> > > 	__u16 flag;
> > >
> > > by vaddr.
> >
> > I see.
> >
> > Apart from the fact you can drop paddr and vb2_plane_cookie() and use vaddr
> > for all (if I'm not mistaken), could you please record the above rationale for
> > manually filling y_histogram and flag by hand in a comment to avoid future
> > readers being confused by this as I was ?
> >
>
> Still need to keep the paddr and vb2_plane_cookie() for

If you were using the dma API to issue DMA transfers then I would understand
you would have to use the dma_handles as set by dma_alloc_attrs(), but
in the vb2 world buf->vaddr = buf->cookie.

Anyway, I haven't run this part of the code, if you could verify the
addresses of paddr and vaddr are different, then I'll be happy to shut
up :)


> 	__u32 reserv0[33];
> 	__u32 bright_sc[4096];
> 	__u32 reserv1[96];
> 	__u32 ae_hist_y[128];
>  	__u32 reserv2[511];
>
> Because this part is filled by the hardware.
>
> I will add more information for struct jh7110_isp_sc_buffer
>

Thanks, a comment in buf_init() to explain that part of the struct is
filled by hw and part has to be manually filled would be enough. From
an userspace API point of view 'struct jh7110_isp_sc_buffer' will just
match the HW layout, regardless of how it is filled up.

> Regards,
> Changhuang
>
>
Changhuang Liang July 12, 2024, 1:37 p.m. UTC | #8
Hi, Jacopo

> Hi Changhuang
> 
> On Fri, Jul 12, 2024 at 01:00:03PM GMT, Changhuang Liang wrote:
> > Hi, Jacopo
> >
> > >
> > > Hi Changhuang
> > >
> > > On Fri, Jul 12, 2024 at 08:36:21AM GMT, Changhuang Liang wrote:
> > > > Hi, Jacopo
> > > >
> > > > [...]
> > > > > > > > +
> > > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > > +		      enum stf_isp_type_scd type_scd) {
> > > > > > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1,
> > > ISP_SC_SEL_MASK,
> > > > > > > > +			    SEL_TYPE(type_scd));
> > > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > > > +yhist_addr); }
> > > > > > > > +
> > > > > > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss,
> > > > > > > > +void
> > > > > > > > +*vaddr) {
> > > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > > +jh7110_isp_sc_buffer
> > > > > *)vaddr;
> > > > > > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > > > > > +	u32 i;
> > > > > > > > +
> > > > > > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > > > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss,
> > > reg_addr);
> > > > > > >
> > > > > > > If you have a contigous memory space to read, could
> > > > > > > memcpy_fromio() help instead of going through 64 reads ?
> > > > > > >
> > > > > >
> > > > > > I will try this function.
> > > > > >
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void
> *vaddr,
> > > > > > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > > +jh7110_isp_sc_buffer *)vaddr;
> > > > > > > > +
> > > > > > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > > > > > +	if (*type_scd == TYPE_AWB) {
> > > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > > > > > +		*type_scd = TYPE_OECF;
> > > > > > > > +	} else {
> > > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > > > > > +		*type_scd = TYPE_AWB;
> > > > > > >
> > > > > > > Is this correct ? Why are you overwriting the value read
> > > > > > > from HW that indicates AE/AF stats with AWB ones ?
> > > > > >
> > > > > > The AWB frame and AE/AF frames will alternate, so the current
> > > > > > frame indicates the AE/AF, then set AWB type just for next AWB
> frame.
> > > > > >
> > > > >
> > > > > Ah! Shouldn't it be userspace configuring which type of
> > > > > statistics it wants to receive instead of the driver alternating the two ?
> > > > >
> > > >
> > > > No, this is determined by hardware, cannot be configured by userspace.
> > > >
> > >
> > >
> > > So this
> > > 	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > > 			    SEL_TYPE(type_scd));
> > >
> > > doesn't actually select which stats type you get from the HW
> > >
> >
> > You can understand it that way. But it still needs to be written to work with
> the hardware.
> >
> 
> ack, maybe a comment here as well would help
> 

Agreed.

> > > > > > >
> > > > > > > > +	}
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > > >  	struct stf_capture *cap =
> > > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > > >  	struct stfcamss_buffer *change_buf;
> > > > > > > > +	enum stf_isp_type_scd type_scd;
> > > > > > > > +	u32 value;
> > > > > > > >  	u32 status;
> > > > > > > >
> > > > > > > >  	status = stf_isp_reg_read(stfcamss,
> ISP_REG_ISP_CTRL_0);
> > > @@
> > > > > > > > -467,6
> > > > > > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void
> > > > > > > > +*priv)
> > > > > > > >  					stf_set_yuv_addr(stfcamss,
> > > change_buf->addr[0],
> > > > > > > >  							 change_buf->addr[1]);
> > > > > > > >  			}
> > > > > > > > +
> > > > > > > > +			value = stf_isp_reg_read(stfcamss,
> > > > > > > ISP_REG_CSI_MODULE_CFG);
> > > > > > > > +			if (value & CSI_SC_EN) {
> > > > > > > > +				change_buf =
> > > stf_change_buffer(&cap_scd->buffers);
> > > > > > > > +				if (change_buf) {
> > > > > > > > +					stf_isp_fill_flag(stfcamss,
> > > change_buf->vaddr,
> > > > > > > > +							  &type_scd);
> > > > > > > > +					stf_set_scd_addr(stfcamss,
> > > change_buf->addr[0],
> > > > > > > > +							 change_buf->addr[1], type_scd);
> > > > > > >
> > > > > > > Sorry if I'm un-familiar with the HW but this seems to be
> > > > > > > the
> > > line-interrupt.
> > > > > > > Are you swapping buffers every line or it's just that you
> > > > > > > have a single line irq for the stats ?
> > > > > > >
> > > > > >
> > > > > > Every frame triggers a line-interrupt, and we will swap buffers in it.
> > > > > >
> > > > >
> > > > > ah, frames completion triggers a line-interrupt ?
> > > > >
> > > >
> > > > Every frame will trigger line-interrupt and stf_isp_irq_handler.
> > > > We use line-interrupt changing buffer, the stf_isp_irq_handler
> > > > will indicate that image transfer to DDR is complete.
> > > >
> > > >
> > > > > > > > +				}
> > > > > > > > +			}
> > > > > > > >  		}
> > > > > > > >
> > > > > > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@
> > > -485,6
> > > > > +542,7
> > > > > > > @@
> > > > > > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > > >  	struct stf_capture *cap =
> > > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > > >  	struct stfcamss_buffer *ready_buf;
> > > > > > > >  	u32 status;
> > > > > > > >
> > > > > > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int
> > > > > > > > irq, void
> > > *priv)
> > > > > > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > > VB2_BUF_STATE_DONE);
> > > > > > > >  		}
> > > > > > > >
> > > > > > > > +		if (status & ISPC_SC) {
> > > > > > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > > > > > +			if (ready_buf) {
> > > > > > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > > > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > > VB2_BUF_STATE_DONE);
> > > > > > > > +			}
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > > > > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > > > > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > > > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > > > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > >  #ifndef STF_ISP_H
> > > > > > > >  #define STF_ISP_H
> > > > > > > >
> > > > > > > > +#include <linux/jh7110-isp.h>
> > > > > > > >  #include <media/v4l2-subdev.h>
> > > > > > > >
> > > > > > > >  #include "stf-video.h"
> > > > > > > > @@ -107,6 +108,12 @@
> > > > > > > >  #define Y_COOR(y)				((y) << 16)
> > > > > > > >  #define X_COOR(x)				((x) << 0)
> > > > > > > >
> > > > > > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > > > > > +
> > > > > > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > > > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > > > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > > > > > +
> > > > > > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > > > > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > > > > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > > > > > @@ -305,6 +312,10 @@
> > > > > > > >  #define DNRM_F(n)				((n) << 16)
> > > > > > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > > > > > >
> > > > > > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > > > > > +
> > > > > > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > > > > > +
> > > > > > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > > > > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > > > > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > > > > > @@ -389,6 +400,15 @@
> > > > > > > >  #define IMAGE_MAX_WIDTH				1920
> > > > > > > >  #define IMAGE_MAX_HEIGH				1080
> > > > > > > >
> > > > > > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > > > > > >
> > > > > > > Should this be in the uAPI header as it is useful to userspace as
> well ?
> > > > > > >
> > > > > > > you could:
> > > > > > >
> > > > > > > struct jh7110_isp_sc_buffer {
> > > > > > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > > > > > 	__u32 reserv0[33];
> > > > > > > 	__u32 bright_sc[4096];
> > > > > > > 	__u32 reserv1[96];
> > > > > > > 	__u32 ae_hist_y[128];
> > > > > > > 	__u32 reserv2[511];
> > > > > > > 	__u16 flag;
> > > > > > > };
> > > > > > >
> > > > > > > ofc if the size is made part of the uAPI you need a more
> > > > > > > proper name such as JH7110_ISP_YHIST_SIZE
> > > > > > >
> > > > > >
> > > > > > OK, I will try this.
> > > > > >
> > > > > > > > +
> > > > > > > > +enum stf_isp_type_scd {
> > > > > > > > +	TYPE_DEC = 0,
> > > > > > > > +	TYPE_OBC,
> > > > > > > > +	TYPE_OECF,
> > > > > > > > +	TYPE_AWB,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >  /* pad id for media framework */  enum stf_isp_pad_id {
> > > > > > > >  	STF_ISP_PAD_SINK = 0,
> > > > > > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct
> > > > > > > > stf_isp_dev *isp_dev);
> > > > > > > >
> > > > > > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > > > > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > > +		      enum stf_isp_type_scd type_scd);
> > > > > > > >
> > > > > > > >  #endif /* STF_ISP_H */
> > > > > > > > diff --git
> > > > > > > > a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > > > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > @@ -125,6 +125,14 @@ static int
> > > > > > > > stf_video_init_format(struct
> > > > > > > stfcamss_video *video)
> > > > > > > >  	return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static int stf_video_scd_init_format(struct
> > > > > > > > +stfcamss_video
> > > > > > > > +*video)
> > > > > > >
> > > > > > > Make it void if it can't fail (see below)
> > > > > > >
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > > +{
> > > > > > > > +	video->active_fmt.fmt.meta.dataformat =
> > > > > > > video->formats[0].pixelformat;
> > > > > > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > > > > > +jh7110_isp_sc_buffer);
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /*
> -----------------------------------------------------------------------------
> > > > > > > >   * Video queue operations
> > > > > > > >   */
> > > > > > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > > > > > stf_video_vb2_q_ops =
> > > > > > > {
> > > > > > > >  	.stop_streaming  = video_stop_streaming,  };
> > > > > > > >
> > > > > > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > > > > > +				 unsigned int *num_buffers,
> > > > > > > > +				 unsigned int *num_planes,
> > > > > > > > +				 unsigned int sizes[],
> > > > > > > > +				 struct device *alloc_devs[]) {
> > > > > > > > +	if (*num_planes)
> > > > > > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ?
> > > -EINVAL :
> > > > > > > > +0;
> > > > > > > > +
> > > > > > > > +	*num_planes = 1;
> > > > > > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > > > > > +	dma_addr_t *paddr;
> > > > > > > > +
> > > > > > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > > > > > +	buffer->addr[0] = *paddr;
> > > > > > > > +	buffer->addr[1] = buffer->addr[0] +
> > > > > > > > +ISP_YHIST_BUFFER_SIZE;
> > > > > > >
> > > > > > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > > > > > mainline and I'm not sure what this gives you as you use it
> > > > > > > to program the
> > > > > following registers:
> > > > > > >
> > > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > > yhist_addr);
> > > > > > >
> > > > > >
> > > > > > We set the value for ISP hardware, then ISP will transfer the
> > > > > > statistics to the
> > > > > buffer.
> > > > > > when the stf_isp_irq_handler interrupt is triggered, indicates
> > > > > > that the buffer fill is complete.
> > > > > >
> > > > >
> > > > > So I take this as
> > > > >
> > > > > 	paddr = vb2_plane_cookie(vb, 0);
> > > > > 	buffer->addr[0] = *paddr;
> > > > > 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > > >
> > > > >         stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > > >                          change_buf->addr[1], type_scd);
> > > > >
> > > > > Makes the ISP transfer data directly to the memory areas in
> > > > > addr[0] and addr[1] (which explains why struct
> > > > > jh7110_isp_sc_buffer is packed, as it has to match the HW
> > > > > registers layout)
> > > > >
> > > > > If this is the case, why are you then manually copying the
> > > > > histograms and the flags to vaddr ?
> > > > >
> > > >
> > > > Yes, your are right.
> > > > But actually there is a problem with our ISP RTL.
> > > > We set this yhist_addr to ISP, but it actually not work.
> > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); or
> > > > I will drop this line in next version.
> > > >
> > > > So, in this structure
> > > > struct jh7110_isp_sc_buffer {
> > > > 	__u32 y_histogram[64];
> > > > 	__u32 reserv0[33];
> > > > 	__u32 bright_sc[4096];
> > > > 	__u32 reserv1[96];
> > > > 	__u32 ae_hist_y[128];
> > > > 	__u32 reserv2[511];
> > > > 	__u16 flag;
> > > > };
> > > >
> > > > Only
> > > >
> > > > 	__u32 reserv0[33];
> > > > 	__u32 bright_sc[4096];
> > > > 	__u32 reserv1[96];
> > > > 	__u32 ae_hist_y[128];
> > > > 	__u32 reserv2[511];
> > > >
> > > > transfer by ISP hardware.
> > > >
> > > > I need to fill
> > > > 	__u32 y_histogram[64];
> > > > 	__u16 flag;
> > > >
> > > > by vaddr.
> > >
> > > I see.
> > >
> > > Apart from the fact you can drop paddr and vb2_plane_cookie() and
> > > use vaddr for all (if I'm not mistaken), could you please record the
> > > above rationale for manually filling y_histogram and flag by hand in
> > > a comment to avoid future readers being confused by this as I was ?
> > >
> >
> > Still need to keep the paddr and vb2_plane_cookie() for
> 
> If you were using the dma API to issue DMA transfers then I would understand
> you would have to use the dma_handles as set by dma_alloc_attrs(), but in
> the vb2 world buf->vaddr = buf->cookie.
> 

Yes, but vb2_plane_cookie() actually called the vb2_dc_cookie,

const struct vb2_mem_ops vb2_dma_contig_memops = {
	.alloc		= vb2_dc_alloc,
	.put		= vb2_dc_put,
	.get_dmabuf	= vb2_dc_get_dmabuf,
	.cookie		= vb2_dc_cookie,
	.vaddr		= vb2_dc_vaddr,
	.mmap		= vb2_dc_mmap,
	.get_userptr	= vb2_dc_get_userptr,
	.put_userptr	= vb2_dc_put_userptr,
	.prepare	= vb2_dc_prepare,
	.finish		= vb2_dc_finish,
	.map_dmabuf	= vb2_dc_map_dmabuf,
	.unmap_dmabuf	= vb2_dc_unmap_dmabuf,
	.attach_dmabuf	= vb2_dc_attach_dmabuf,
	.detach_dmabuf	= vb2_dc_detach_dmabuf,
	.num_users	= vb2_dc_num_users,
};

static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv)
{
	struct vb2_dc_buf *buf = buf_priv;

	return &buf->dma_addr;
}

It not return the buf->vaddr = buf->cookie.

Regards, 
Changhuang

> Anyway, I haven't run this part of the code, if you could verify the addresses of
> paddr and vaddr are different, then I'll be happy to shut up :)
> 
> 
> > 	__u32 reserv0[33];
> > 	__u32 bright_sc[4096];
> > 	__u32 reserv1[96];
> > 	__u32 ae_hist_y[128];
> >  	__u32 reserv2[511];
> >
> > Because this part is filled by the hardware.
> >
> > I will add more information for struct jh7110_isp_sc_buffer
> >
> 
> Thanks, a comment in buf_init() to explain that part of the struct is filled by
> hw and part has to be manually filled would be enough. From an userspace
> API point of view 'struct jh7110_isp_sc_buffer' will just match the HW layout,
> regardless of how it is filled up.
> 
> > Regards,
> > Changhuang
> >
> >
Jacopo Mondi July 12, 2024, 4:25 p.m. UTC | #9
Hi again

On Fri, Jul 12, 2024 at 01:37:58PM GMT, Changhuang Liang wrote:
> Hi, Jacopo
>
> > Hi Changhuang
> >
> > On Fri, Jul 12, 2024 at 01:00:03PM GMT, Changhuang Liang wrote:
> > > Hi, Jacopo
> > >
> > > >
> > > > Hi Changhuang
> > > >
> > > > On Fri, Jul 12, 2024 at 08:36:21AM GMT, Changhuang Liang wrote:
> > > > > Hi, Jacopo
> > > > >
> > > > > [...]
> > > > > > > > > +
> > > > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > > > +		      enum stf_isp_type_scd type_scd) {
> > > > > > > > > +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1,
> > > > ISP_SC_SEL_MASK,
> > > > > > > > > +			    SEL_TYPE(type_scd));
> > > > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > > > > +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > > > > +yhist_addr); }
> > > > > > > > > +
> > > > > > > > > +static void stf_isp_fill_yhist(struct stfcamss *stfcamss,
> > > > > > > > > +void
> > > > > > > > > +*vaddr) {
> > > > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > > > +jh7110_isp_sc_buffer
> > > > > > *)vaddr;
> > > > > > > > > +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> > > > > > > > > +	u32 i;
> > > > > > > > > +
> > > > > > > > > +	for (i = 0; i < 64; i++, reg_addr += 4)
> > > > > > > > > +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss,
> > > > reg_addr);
> > > > > > > >
> > > > > > > > If you have a contigous memory space to read, could
> > > > > > > > memcpy_fromio() help instead of going through 64 reads ?
> > > > > > > >
> > > > > > >
> > > > > > > I will try this function.
> > > > > > >
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void
> > *vaddr,
> > > > > > > > > +			      enum stf_isp_type_scd *type_scd) {
> > > > > > > > > +	struct jh7110_isp_sc_buffer *sc = (struct
> > > > > > > > > +jh7110_isp_sc_buffer *)vaddr;
> > > > > > > > > +
> > > > > > > > > +	*type_scd = stf_isp_get_scd_type(stfcamss);
> > > > > > > > > +	if (*type_scd == TYPE_AWB) {
> > > > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> > > > > > > > > +		*type_scd = TYPE_OECF;
> > > > > > > > > +	} else {
> > > > > > > > > +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> > > > > > > > > +		*type_scd = TYPE_AWB;
> > > > > > > >
> > > > > > > > Is this correct ? Why are you overwriting the value read
> > > > > > > > from HW that indicates AE/AF stats with AWB ones ?
> > > > > > >
> > > > > > > The AWB frame and AE/AF frames will alternate, so the current
> > > > > > > frame indicates the AE/AF, then set AWB type just for next AWB
> > frame.
> > > > > > >
> > > > > >
> > > > > > Ah! Shouldn't it be userspace configuring which type of
> > > > > > statistics it wants to receive instead of the driver alternating the two ?
> > > > > >
> > > > >
> > > > > No, this is determined by hardware, cannot be configured by userspace.
> > > > >
> > > >
> > > >
> > > > So this
> > > > 	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> > > > 			    SEL_TYPE(type_scd));
> > > >
> > > > doesn't actually select which stats type you get from the HW
> > > >
> > >
> > > You can understand it that way. But it still needs to be written to work with
> > the hardware.
> > >
> >
> > ack, maybe a comment here as well would help
> >
>
> Agreed.
>
> > > > > > > >
> > > > > > > > > +	}
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> > > > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > > > >  	struct stf_capture *cap =
> > > > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > > > >  	struct stfcamss_buffer *change_buf;
> > > > > > > > > +	enum stf_isp_type_scd type_scd;
> > > > > > > > > +	u32 value;
> > > > > > > > >  	u32 status;
> > > > > > > > >
> > > > > > > > >  	status = stf_isp_reg_read(stfcamss,
> > ISP_REG_ISP_CTRL_0);
> > > > @@
> > > > > > > > > -467,6
> > > > > > > > > +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void
> > > > > > > > > +*priv)
> > > > > > > > >  					stf_set_yuv_addr(stfcamss,
> > > > change_buf->addr[0],
> > > > > > > > >  							 change_buf->addr[1]);
> > > > > > > > >  			}
> > > > > > > > > +
> > > > > > > > > +			value = stf_isp_reg_read(stfcamss,
> > > > > > > > ISP_REG_CSI_MODULE_CFG);
> > > > > > > > > +			if (value & CSI_SC_EN) {
> > > > > > > > > +				change_buf =
> > > > stf_change_buffer(&cap_scd->buffers);
> > > > > > > > > +				if (change_buf) {
> > > > > > > > > +					stf_isp_fill_flag(stfcamss,
> > > > change_buf->vaddr,
> > > > > > > > > +							  &type_scd);
> > > > > > > > > +					stf_set_scd_addr(stfcamss,
> > > > change_buf->addr[0],
> > > > > > > > > +							 change_buf->addr[1], type_scd);
> > > > > > > >
> > > > > > > > Sorry if I'm un-familiar with the HW but this seems to be
> > > > > > > > the
> > > > line-interrupt.
> > > > > > > > Are you swapping buffers every line or it's just that you
> > > > > > > > have a single line irq for the stats ?
> > > > > > > >
> > > > > > >
> > > > > > > Every frame triggers a line-interrupt, and we will swap buffers in it.
> > > > > > >
> > > > > >
> > > > > > ah, frames completion triggers a line-interrupt ?
> > > > > >
> > > > >
> > > > > Every frame will trigger line-interrupt and stf_isp_irq_handler.
> > > > > We use line-interrupt changing buffer, the stf_isp_irq_handler
> > > > > will indicate that image transfer to DDR is complete.
> > > > >
> > > > >
> > > > > > > > > +				}
> > > > > > > > > +			}
> > > > > > > > >  		}
> > > > > > > > >
> > > > > > > > >  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS, @@
> > > > -485,6
> > > > > > +542,7
> > > > > > > > @@
> > > > > > > > > irqreturn_t stf_isp_irq_handler(int irq, void *priv)  {
> > > > > > > > >  	struct stfcamss *stfcamss = priv;
> > > > > > > > >  	struct stf_capture *cap =
> > > > > > > > > &stfcamss->captures[STF_CAPTURE_YUV];
> > > > > > > > > +	struct stf_capture *cap_scd =
> > > > > > > > > +&stfcamss->captures[STF_CAPTURE_SCD];
> > > > > > > > >  	struct stfcamss_buffer *ready_buf;
> > > > > > > > >  	u32 status;
> > > > > > > > >
> > > > > > > > > @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int
> > > > > > > > > irq, void
> > > > *priv)
> > > > > > > > >  				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > > > VB2_BUF_STATE_DONE);
> > > > > > > > >  		}
> > > > > > > > >
> > > > > > > > > +		if (status & ISPC_SC) {
> > > > > > > > > +			ready_buf = stf_buf_done(&cap_scd->buffers);
> > > > > > > > > +			if (ready_buf) {
> > > > > > > > > +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> > > > > > > > > +				vb2_buffer_done(&ready_buf->vb.vb2_buf,
> > > > > > > > VB2_BUF_STATE_DONE);
> > > > > > > > > +			}
> > > > > > > > > +		}
> > > > > > > > > +
> > > > > > > > >  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
> > > > > > > > >  				  (status & ~ISPC_INT_ALL_MASK) |
> > > > > > > > >  				  ISPC_ISP | ISPC_CSI | ISPC_SC); diff --git
> > > > > > > > > a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > > b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > > index fcda0502e3b0..0af7b367e57a 100644
> > > > > > > > > --- a/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > >  #ifndef STF_ISP_H
> > > > > > > > >  #define STF_ISP_H
> > > > > > > > >
> > > > > > > > > +#include <linux/jh7110-isp.h>
> > > > > > > > >  #include <media/v4l2-subdev.h>
> > > > > > > > >
> > > > > > > > >  #include "stf-video.h"
> > > > > > > > > @@ -107,6 +108,12 @@
> > > > > > > > >  #define Y_COOR(y)				((y) << 16)
> > > > > > > > >  #define X_COOR(x)				((x) << 0)
> > > > > > > > >
> > > > > > > > > +#define ISP_REG_SCD_CFG_0			0x098
> > > > > > > > > +
> > > > > > > > > +#define ISP_REG_SC_CFG_1			0x0bc
> > > > > > > > > +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> > > > > > > > > +#define SEL_TYPE(n)				((n) << 30)
> > > > > > > > > +
> > > > > > > > >  #define ISP_REG_LCCF_CFG_2			0x0e0
> > > > > > > > >  #define ISP_REG_LCCF_CFG_3			0x0e4
> > > > > > > > >  #define ISP_REG_LCCF_CFG_4			0x0e8
> > > > > > > > > @@ -305,6 +312,10 @@
> > > > > > > > >  #define DNRM_F(n)				((n) << 16)
> > > > > > > > >  #define CCM_M_DAT(n)				((n) << 0)
> > > > > > > > >
> > > > > > > > > +#define ISP_REG_YHIST_CFG_4			0xcd8
> > > > > > > > > +
> > > > > > > > > +#define ISP_REG_YHIST_ACC_0			0xd00
> > > > > > > > > +
> > > > > > > > >  #define ISP_REG_GAMMA_VAL0			0xe00
> > > > > > > > >  #define ISP_REG_GAMMA_VAL1			0xe04
> > > > > > > > >  #define ISP_REG_GAMMA_VAL2			0xe08
> > > > > > > > > @@ -389,6 +400,15 @@
> > > > > > > > >  #define IMAGE_MAX_WIDTH				1920
> > > > > > > > >  #define IMAGE_MAX_HEIGH				1080
> > > > > > > > >
> > > > > > > > > +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> > > > > > > >
> > > > > > > > Should this be in the uAPI header as it is useful to userspace as
> > well ?
> > > > > > > >
> > > > > > > > you could:
> > > > > > > >
> > > > > > > > struct jh7110_isp_sc_buffer {
> > > > > > > > 	__u8 y_histogram[ISP_YHIST_BUFFER_SIZE];
> > > > > > > > 	__u32 reserv0[33];
> > > > > > > > 	__u32 bright_sc[4096];
> > > > > > > > 	__u32 reserv1[96];
> > > > > > > > 	__u32 ae_hist_y[128];
> > > > > > > > 	__u32 reserv2[511];
> > > > > > > > 	__u16 flag;
> > > > > > > > };
> > > > > > > >
> > > > > > > > ofc if the size is made part of the uAPI you need a more
> > > > > > > > proper name such as JH7110_ISP_YHIST_SIZE
> > > > > > > >
> > > > > > >
> > > > > > > OK, I will try this.
> > > > > > >
> > > > > > > > > +
> > > > > > > > > +enum stf_isp_type_scd {
> > > > > > > > > +	TYPE_DEC = 0,
> > > > > > > > > +	TYPE_OBC,
> > > > > > > > > +	TYPE_OECF,
> > > > > > > > > +	TYPE_AWB,
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > >  /* pad id for media framework */  enum stf_isp_pad_id {
> > > > > > > > >  	STF_ISP_PAD_SINK = 0,
> > > > > > > > > @@ -429,5 +449,8 @@ int stf_isp_unregister(struct
> > > > > > > > > stf_isp_dev *isp_dev);
> > > > > > > > >
> > > > > > > > >  void stf_set_yuv_addr(struct stfcamss *stfcamss,
> > > > > > > > >  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> > > > > > > > > +void stf_set_scd_addr(struct stfcamss *stfcamss,
> > > > > > > > > +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> > > > > > > > > +		      enum stf_isp_type_scd type_scd);
> > > > > > > > >
> > > > > > > > >  #endif /* STF_ISP_H */
> > > > > > > > > diff --git
> > > > > > > > > a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > > b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > > index 989b5e82bae9..2203605ec9c7 100644
> > > > > > > > > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > > > > > > > > @@ -125,6 +125,14 @@ static int
> > > > > > > > > stf_video_init_format(struct
> > > > > > > > stfcamss_video *video)
> > > > > > > > >  	return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +static int stf_video_scd_init_format(struct
> > > > > > > > > +stfcamss_video
> > > > > > > > > +*video)
> > > > > > > >
> > > > > > > > Make it void if it can't fail (see below)
> > > > > > > >
> > > > > > >
> > > > > > > OK.
> > > > > > >
> > > > > > > > > +{
> > > > > > > > > +	video->active_fmt.fmt.meta.dataformat =
> > > > > > > > video->formats[0].pixelformat;
> > > > > > > > > +	video->active_fmt.fmt.meta.buffersize = sizeof(struct
> > > > > > > > > +jh7110_isp_sc_buffer);
> > > > > > > > > +
> > > > > > > > > +	return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  /*
> > -----------------------------------------------------------------------------
> > > > > > > > >   * Video queue operations
> > > > > > > > >   */
> > > > > > > > > @@ -330,6 +338,78 @@ static const struct vb2_ops
> > > > > > > > > stf_video_vb2_q_ops =
> > > > > > > > {
> > > > > > > > >  	.stop_streaming  = video_stop_streaming,  };
> > > > > > > > >
> > > > > > > > > +static int video_scd_queue_setup(struct vb2_queue *q,
> > > > > > > > > +				 unsigned int *num_buffers,
> > > > > > > > > +				 unsigned int *num_planes,
> > > > > > > > > +				 unsigned int sizes[],
> > > > > > > > > +				 struct device *alloc_devs[]) {
> > > > > > > > > +	if (*num_planes)
> > > > > > > > > +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ?
> > > > -EINVAL :
> > > > > > > > > +0;
> > > > > > > > > +
> > > > > > > > > +	*num_planes = 1;
> > > > > > > > > +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> > > > > > > > > +
> > > > > > > > > +	return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int video_scd_buf_init(struct vb2_buffer *vb) {
> > > > > > > > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > > > > > +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> > > > > > > > > +	dma_addr_t *paddr;
> > > > > > > > > +
> > > > > > > > > +	paddr = vb2_plane_cookie(vb, 0);
> > > > > > > > > +	buffer->addr[0] = *paddr;
> > > > > > > > > +	buffer->addr[1] = buffer->addr[0] +
> > > > > > > > > +ISP_YHIST_BUFFER_SIZE;
> > > > > > > >
> > > > > > > > Interesting, I don't see many users of vb2_plane_cookie() in
> > > > > > > > mainline and I'm not sure what this gives you as you use it
> > > > > > > > to program the
> > > > > > following registers:
> > > > > > > >
> > > > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> > > > > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4,
> > > > > > > > yhist_addr);
> > > > > > > >
> > > > > > >
> > > > > > > We set the value for ISP hardware, then ISP will transfer the
> > > > > > > statistics to the
> > > > > > buffer.
> > > > > > > when the stf_isp_irq_handler interrupt is triggered, indicates
> > > > > > > that the buffer fill is complete.
> > > > > > >
> > > > > >
> > > > > > So I take this as
> > > > > >
> > > > > > 	paddr = vb2_plane_cookie(vb, 0);
> > > > > > 	buffer->addr[0] = *paddr;
> > > > > > 	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> > > > > >
> > > > > >         stf_set_scd_addr(stfcamss, change_buf->addr[0],
> > > > > >                          change_buf->addr[1], type_scd);
> > > > > >
> > > > > > Makes the ISP transfer data directly to the memory areas in
> > > > > > addr[0] and addr[1] (which explains why struct
> > > > > > jh7110_isp_sc_buffer is packed, as it has to match the HW
> > > > > > registers layout)
> > > > > >
> > > > > > If this is the case, why are you then manually copying the
> > > > > > histograms and the flags to vaddr ?
> > > > > >
> > > > >
> > > > > Yes, your are right.
> > > > > But actually there is a problem with our ISP RTL.
> > > > > We set this yhist_addr to ISP, but it actually not work.
> > > > > 	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr); or
> > > > > I will drop this line in next version.
> > > > >
> > > > > So, in this structure
> > > > > struct jh7110_isp_sc_buffer {
> > > > > 	__u32 y_histogram[64];
> > > > > 	__u32 reserv0[33];
> > > > > 	__u32 bright_sc[4096];
> > > > > 	__u32 reserv1[96];
> > > > > 	__u32 ae_hist_y[128];
> > > > > 	__u32 reserv2[511];
> > > > > 	__u16 flag;
> > > > > };
> > > > >
> > > > > Only
> > > > >
> > > > > 	__u32 reserv0[33];
> > > > > 	__u32 bright_sc[4096];
> > > > > 	__u32 reserv1[96];
> > > > > 	__u32 ae_hist_y[128];
> > > > > 	__u32 reserv2[511];
> > > > >
> > > > > transfer by ISP hardware.
> > > > >
> > > > > I need to fill
> > > > > 	__u32 y_histogram[64];
> > > > > 	__u16 flag;
> > > > >
> > > > > by vaddr.
> > > >
> > > > I see.
> > > >
> > > > Apart from the fact you can drop paddr and vb2_plane_cookie() and
> > > > use vaddr for all (if I'm not mistaken), could you please record the
> > > > above rationale for manually filling y_histogram and flag by hand in
> > > > a comment to avoid future readers being confused by this as I was ?
> > > >
> > >
> > > Still need to keep the paddr and vb2_plane_cookie() for
> >
> > If you were using the dma API to issue DMA transfers then I would understand
> > you would have to use the dma_handles as set by dma_alloc_attrs(), but in
> > the vb2 world buf->vaddr = buf->cookie.
> >
>
> Yes, but vb2_plane_cookie() actually called the vb2_dc_cookie,
>
> const struct vb2_mem_ops vb2_dma_contig_memops = {
> 	.alloc		= vb2_dc_alloc,
> 	.put		= vb2_dc_put,
> 	.get_dmabuf	= vb2_dc_get_dmabuf,
> 	.cookie		= vb2_dc_cookie,
> 	.vaddr		= vb2_dc_vaddr,
> 	.mmap		= vb2_dc_mmap,
> 	.get_userptr	= vb2_dc_get_userptr,
> 	.put_userptr	= vb2_dc_put_userptr,
> 	.prepare	= vb2_dc_prepare,
> 	.finish		= vb2_dc_finish,
> 	.map_dmabuf	= vb2_dc_map_dmabuf,
> 	.unmap_dmabuf	= vb2_dc_unmap_dmabuf,
> 	.attach_dmabuf	= vb2_dc_attach_dmabuf,
> 	.detach_dmabuf	= vb2_dc_detach_dmabuf,
> 	.num_users	= vb2_dc_num_users,
> };
>
> static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv)
> {
> 	struct vb2_dc_buf *buf = buf_priv;
>
> 	return &buf->dma_addr;
> }
>
> It not return the buf->vaddr = buf->cookie.

right right

I was only aware of vb2_dma_contig_plane_dma_addr() to get the
dma_addr handle, and I just realized it internally gets the plane
cookie:

static inline dma_addr_t
vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
{
	dma_addr_t *addr = vb2_plane_cookie(vb, plane_no);

	return *addr;
}

>
> Regards,
> Changhuang
>
> > Anyway, I haven't run this part of the code, if you could verify the addresses of
> > paddr and vaddr are different, then I'll be happy to shut up :)

Now I'm happy to shut up then :)

Thanks for explaining
   j

> >
> >
> > > 	__u32 reserv0[33];
> > > 	__u32 bright_sc[4096];
> > > 	__u32 reserv1[96];
> > > 	__u32 ae_hist_y[128];
> > >  	__u32 reserv2[511];
> > >
> > > Because this part is filled by the hardware.
> > >
> > > I will add more information for struct jh7110_isp_sc_buffer
> > >
> >
> > Thanks, a comment in buf_init() to explain that part of the struct is filled by
> > hw and part has to be manually filled would be enough. From an userspace
> > API point of view 'struct jh7110_isp_sc_buffer' will just match the HW layout,
> > regardless of how it is filled up.
> >
> > > Regards,
> > > Changhuang
> > >
> > >
Jacopo Mondi July 12, 2024, 4:33 p.m. UTC | #10
Hi Changhuang

Sorry, I missed this: the subject

    staging: media: starfive: Add for StarFive ISP 3A SC

Seems to be missing a word.

    staging: media: starfive: Add stats for StarFive ISP 3A

maybe ?

On Tue, Jul 09, 2024 at 01:38:18AM GMT, Changhuang Liang wrote:
> Register ISP 3A "capture_scd" video device to receive statistics
> collection data.
>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../staging/media/starfive/camss/stf-buffer.h |   1 +
>  .../staging/media/starfive/camss/stf-camss.c  |  15 ++
>  .../media/starfive/camss/stf-capture.c        |  21 ++-
>  .../media/starfive/camss/stf-isp-hw-ops.c     |  66 ++++++++
>  .../staging/media/starfive/camss/stf-isp.h    |  23 +++
>  .../staging/media/starfive/camss/stf-video.c  | 146 +++++++++++++++++-
>  .../staging/media/starfive/camss/stf-video.h  |   1 +
>  7 files changed, 264 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/media/starfive/camss/stf-buffer.h b/drivers/staging/media/starfive/camss/stf-buffer.h
> index 9d1670fb05ed..727d00617448 100644
> --- a/drivers/staging/media/starfive/camss/stf-buffer.h
> +++ b/drivers/staging/media/starfive/camss/stf-buffer.h
> @@ -23,6 +23,7 @@ enum stf_v_state {
>  struct stfcamss_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	dma_addr_t addr[2];
> +	void *vaddr;
>  	struct list_head queue;
>  };
>
> diff --git a/drivers/staging/media/starfive/camss/stf-camss.c b/drivers/staging/media/starfive/camss/stf-camss.c
> index fecd3e67c7a1..fafa3ce2f6da 100644
> --- a/drivers/staging/media/starfive/camss/stf-camss.c
> +++ b/drivers/staging/media/starfive/camss/stf-camss.c
> @@ -8,6 +8,7 @@
>   *
>   * Author: Jack Zhu <jack.zhu@starfivetech.com>
>   * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> + * Author: Keith Zhao <keith.zhao@starfivetech.com>
>   *
>   */
>  #include <linux/module.h>
> @@ -126,6 +127,7 @@ static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
>  static int stfcamss_register_devs(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>  	int ret;
>
> @@ -150,8 +152,18 @@ static int stfcamss_register_devs(struct stfcamss *stfcamss)
>
>  	cap_yuv->video.source_subdev = &isp_dev->subdev;
>
> +	ret = media_create_pad_link(&isp_dev->subdev.entity, STF_ISP_PAD_SRC_SCD,
> +				    &cap_scd->video.vdev.entity, 0, 0);
> +	if (ret)
> +		goto err_rm_links0;
> +
> +	cap_scd->video.source_subdev = &isp_dev->subdev;
> +
>  	return ret;
>
> +err_rm_links0:
> +	media_entity_remove_links(&isp_dev->subdev.entity);
> +	media_entity_remove_links(&cap_yuv->video.vdev.entity);
>  err_cap_unregister:
>  	stf_capture_unregister(stfcamss);
>  err_isp_unregister:
> @@ -163,10 +175,12 @@ static int stfcamss_register_devs(struct stfcamss *stfcamss)
>  static void stfcamss_unregister_devs(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>
>  	media_entity_remove_links(&isp_dev->subdev.entity);
>  	media_entity_remove_links(&cap_yuv->video.vdev.entity);
> +	media_entity_remove_links(&cap_scd->video.vdev.entity);
>
>  	stf_isp_unregister(&stfcamss->isp_dev);
>  	stf_capture_unregister(stfcamss);
> @@ -436,5 +450,6 @@ module_platform_driver(stfcamss_driver);
>
>  MODULE_AUTHOR("Jack Zhu <jack.zhu@starfivetech.com>");
>  MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
> +MODULE_AUTHOR("Keith Zhao <keith.zhao@starfivetech.com>");
>  MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/media/starfive/camss/stf-capture.c b/drivers/staging/media/starfive/camss/stf-capture.c
> index 75f6ef405e61..328b8c6e351d 100644
> --- a/drivers/staging/media/starfive/camss/stf-capture.c
> +++ b/drivers/staging/media/starfive/camss/stf-capture.c
> @@ -12,6 +12,7 @@
>  static const char * const stf_cap_names[] = {
>  	"capture_raw",
>  	"capture_yuv",
> +	"capture_scd",
>  };
>
>  static const struct stfcamss_format_info stf_wr_fmts[] = {
> @@ -55,6 +56,14 @@ static const struct stfcamss_format_info stf_isp_fmts[] = {
>  	},
>  };
>
> +/* 3A Statistics Collection Data */
> +static const struct stfcamss_format_info stf_isp_scd_fmts[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_METADATA_FIXED,
> +		.pixelformat = V4L2_META_FMT_STF_ISP_STAT_3A,
> +	},
> +};
> +
>  static inline struct stf_capture *to_stf_capture(struct stfcamss_video *video)
>  {
>  	return container_of(video, struct stf_capture, video);
> @@ -84,6 +93,8 @@ static void stf_init_addrs(struct stfcamss_video *video)
>  		stf_set_raw_addr(video->stfcamss, addr0);
>  	else if (cap->type == STF_CAPTURE_YUV)
>  		stf_set_yuv_addr(video->stfcamss, addr0, addr1);
> +	else
> +		stf_set_scd_addr(video->stfcamss, addr0, addr1, TYPE_AWB);
>  }
>
>  static void stf_cap_s_cfg(struct stfcamss_video *video)
> @@ -227,18 +238,24 @@ static void stf_capture_init(struct stfcamss *stfcamss, struct stf_capture *cap)
>  	INIT_LIST_HEAD(&cap->buffers.ready_bufs);
>  	spin_lock_init(&cap->buffers.lock);
>
> -	cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	cap->video.stfcamss = stfcamss;
>  	cap->video.bpl_alignment = 16 * 8;
>
>  	if (cap->type == STF_CAPTURE_RAW) {
> +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		cap->video.formats = stf_wr_fmts;
>  		cap->video.nformats = ARRAY_SIZE(stf_wr_fmts);
>  		cap->video.bpl_alignment = 8;
>  	} else if (cap->type == STF_CAPTURE_YUV) {
> +		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		cap->video.formats = stf_isp_fmts;
>  		cap->video.nformats = ARRAY_SIZE(stf_isp_fmts);
>  		cap->video.bpl_alignment = 1;
> +	} else {
> +		cap->video.type = V4L2_BUF_TYPE_META_CAPTURE;
> +		cap->video.formats = stf_isp_scd_fmts;
> +		cap->video.nformats = ARRAY_SIZE(stf_isp_scd_fmts);
> +		cap->video.bpl_alignment = 16 * 8;
>  	}
>  }
>
> @@ -362,9 +379,11 @@ void stf_capture_unregister(struct stfcamss *stfcamss)
>  {
>  	struct stf_capture *cap_raw = &stfcamss->captures[STF_CAPTURE_RAW];
>  	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>
>  	stf_capture_unregister_one(cap_raw);
>  	stf_capture_unregister_one(cap_yuv);
> +	stf_capture_unregister_one(cap_scd);
>  }
>
>  int stf_capture_register(struct stfcamss *stfcamss,
> diff --git a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> index 6b3966ca18bf..3b18d09f2cc6 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> @@ -451,11 +451,57 @@ void stf_set_yuv_addr(struct stfcamss *stfcamss,
>  	stf_isp_reg_write(stfcamss, ISP_REG_UV_PLANE_START_ADDR, uv_addr);
>  }
>
> +static enum stf_isp_type_scd stf_isp_get_scd_type(struct stfcamss *stfcamss)
> +{
> +	int val;
> +
> +	val = stf_isp_reg_read(stfcamss, ISP_REG_SC_CFG_1);
> +	return (enum stf_isp_type_scd)(val & ISP_SC_SEL_MASK) >> 30;
> +}
> +
> +void stf_set_scd_addr(struct stfcamss *stfcamss,
> +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> +		      enum stf_isp_type_scd type_scd)
> +{
> +	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
> +			    SEL_TYPE(type_scd));
> +	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
> +	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
> +}
> +
> +static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void *vaddr)
> +{
> +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> +	u32 reg_addr = ISP_REG_YHIST_ACC_0;
> +	u32 i;
> +
> +	for (i = 0; i < 64; i++, reg_addr += 4)
> +		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
> +}
> +
> +static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
> +			      enum stf_isp_type_scd *type_scd)
> +{
> +	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
> +
> +	*type_scd = stf_isp_get_scd_type(stfcamss);
> +	if (*type_scd == TYPE_AWB) {
> +		sc->flag = JH7110_ISP_SC_FLAG_AWB;
> +		*type_scd = TYPE_OECF;
> +	} else {
> +		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
> +		*type_scd = TYPE_AWB;
> +	}
> +}
> +
>  irqreturn_t stf_line_irq_handler(int irq, void *priv)
>  {
>  	struct stfcamss *stfcamss = priv;
>  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stfcamss_buffer *change_buf;
> +	enum stf_isp_type_scd type_scd;
> +	u32 value;
>  	u32 status;
>
>  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> @@ -467,6 +513,17 @@ irqreturn_t stf_line_irq_handler(int irq, void *priv)
>  					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
>  							 change_buf->addr[1]);
>  			}
> +
> +			value = stf_isp_reg_read(stfcamss, ISP_REG_CSI_MODULE_CFG);
> +			if (value & CSI_SC_EN) {
> +				change_buf = stf_change_buffer(&cap_scd->buffers);
> +				if (change_buf) {
> +					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
> +							  &type_scd);
> +					stf_set_scd_addr(stfcamss, change_buf->addr[0],
> +							 change_buf->addr[1], type_scd);
> +				}
> +			}
>  		}
>
>  		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS,
> @@ -485,6 +542,7 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
>  {
>  	struct stfcamss *stfcamss = priv;
>  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> +	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
>  	struct stfcamss_buffer *ready_buf;
>  	u32 status;
>
> @@ -496,6 +554,14 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
>  				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  		}
>
> +		if (status & ISPC_SC) {
> +			ready_buf = stf_buf_done(&cap_scd->buffers);
> +			if (ready_buf) {
> +				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
> +				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +			}
> +		}
> +
>  		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
>  				  (status & ~ISPC_INT_ALL_MASK) |
>  				  ISPC_ISP | ISPC_CSI | ISPC_SC);
> diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
> index fcda0502e3b0..0af7b367e57a 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp.h
> +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> @@ -10,6 +10,7 @@
>  #ifndef STF_ISP_H
>  #define STF_ISP_H
>
> +#include <linux/jh7110-isp.h>
>  #include <media/v4l2-subdev.h>
>
>  #include "stf-video.h"
> @@ -107,6 +108,12 @@
>  #define Y_COOR(y)				((y) << 16)
>  #define X_COOR(x)				((x) << 0)
>
> +#define ISP_REG_SCD_CFG_0			0x098
> +
> +#define ISP_REG_SC_CFG_1			0x0bc
> +#define ISP_SC_SEL_MASK				GENMASK(31, 30)
> +#define SEL_TYPE(n)				((n) << 30)
> +
>  #define ISP_REG_LCCF_CFG_2			0x0e0
>  #define ISP_REG_LCCF_CFG_3			0x0e4
>  #define ISP_REG_LCCF_CFG_4			0x0e8
> @@ -305,6 +312,10 @@
>  #define DNRM_F(n)				((n) << 16)
>  #define CCM_M_DAT(n)				((n) << 0)
>
> +#define ISP_REG_YHIST_CFG_4			0xcd8
> +
> +#define ISP_REG_YHIST_ACC_0			0xd00
> +
>  #define ISP_REG_GAMMA_VAL0			0xe00
>  #define ISP_REG_GAMMA_VAL1			0xe04
>  #define ISP_REG_GAMMA_VAL2			0xe08
> @@ -389,6 +400,15 @@
>  #define IMAGE_MAX_WIDTH				1920
>  #define IMAGE_MAX_HEIGH				1080
>
> +#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
> +
> +enum stf_isp_type_scd {
> +	TYPE_DEC = 0,
> +	TYPE_OBC,
> +	TYPE_OECF,
> +	TYPE_AWB,
> +};
> +
>  /* pad id for media framework */
>  enum stf_isp_pad_id {
>  	STF_ISP_PAD_SINK = 0,
> @@ -429,5 +449,8 @@ int stf_isp_unregister(struct stf_isp_dev *isp_dev);
>
>  void stf_set_yuv_addr(struct stfcamss *stfcamss,
>  		      dma_addr_t y_addr, dma_addr_t uv_addr);
> +void stf_set_scd_addr(struct stfcamss *stfcamss,
> +		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
> +		      enum stf_isp_type_scd type_scd);
>
>  #endif /* STF_ISP_H */
> diff --git a/drivers/staging/media/starfive/camss/stf-video.c b/drivers/staging/media/starfive/camss/stf-video.c
> index 989b5e82bae9..2203605ec9c7 100644
> --- a/drivers/staging/media/starfive/camss/stf-video.c
> +++ b/drivers/staging/media/starfive/camss/stf-video.c
> @@ -125,6 +125,14 @@ static int stf_video_init_format(struct stfcamss_video *video)
>  	return 0;
>  }
>
> +static int stf_video_scd_init_format(struct stfcamss_video *video)
> +{
> +	video->active_fmt.fmt.meta.dataformat = video->formats[0].pixelformat;
> +	video->active_fmt.fmt.meta.buffersize = sizeof(struct jh7110_isp_sc_buffer);
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Video queue operations
>   */
> @@ -330,6 +338,78 @@ static const struct vb2_ops stf_video_vb2_q_ops = {
>  	.stop_streaming  = video_stop_streaming,
>  };
>
> +static int video_scd_queue_setup(struct vb2_queue *q,
> +				 unsigned int *num_buffers,
> +				 unsigned int *num_planes,
> +				 unsigned int sizes[],
> +				 struct device *alloc_devs[])
> +{
> +	if (*num_planes)
> +		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL : 0;
> +
> +	*num_planes = 1;
> +	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
> +
> +	return 0;
> +}
> +
> +static int video_scd_buf_init(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
> +	dma_addr_t *paddr;
> +
> +	paddr = vb2_plane_cookie(vb, 0);
> +	buffer->addr[0] = *paddr;
> +	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
> +	buffer->vaddr = vb2_plane_vaddr(vb, 0);
> +
> +	return 0;
> +}
> +
> +static int video_scd_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +
> +	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
> +		return -EINVAL;
> +
> +	vb2_set_plane_payload(vb, 0, sizeof(struct jh7110_isp_sc_buffer));
> +
> +	vbuf->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +static int video_scd_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> +
> +	video->ops->start_streaming(video);
> +
> +	return 0;
> +}
> +
> +static void video_scd_stop_streaming(struct vb2_queue *q)
> +{
> +	struct stfcamss_video *video = vb2_get_drv_priv(q);
> +
> +	video->ops->stop_streaming(video);
> +
> +	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
> +}
> +
> +static const struct vb2_ops stf_video_scd_vb2_q_ops = {
> +	.queue_setup     = video_scd_queue_setup,
> +	.wait_prepare    = vb2_ops_wait_prepare,
> +	.wait_finish     = vb2_ops_wait_finish,
> +	.buf_init        = video_scd_buf_init,
> +	.buf_prepare     = video_scd_buf_prepare,
> +	.buf_queue       = video_buf_queue,
> +	.start_streaming = video_scd_start_streaming,
> +	.stop_streaming  = video_scd_stop_streaming,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 ioctls
>   */
> @@ -448,6 +528,37 @@ static const struct v4l2_ioctl_ops stf_vid_ioctl_ops = {
>  	.vidioc_streamoff               = vb2_ioctl_streamoff,
>  };
>
> +static int video_scd_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> +{
> +	struct stfcamss_video *video = video_drvdata(file);
> +	struct v4l2_meta_format *meta = &f->fmt.meta;
> +
> +	if (f->type != video->type)
> +		return -EINVAL;
> +
> +	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
> +	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
> +	.vidioc_querycap                = video_querycap,
> +	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
> +	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
> +	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
> +	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
> +	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
> +	.vidioc_querybuf                = vb2_ioctl_querybuf,
> +	.vidioc_qbuf                    = vb2_ioctl_qbuf,
> +	.vidioc_expbuf                  = vb2_ioctl_expbuf,
> +	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
> +	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
> +	.vidioc_streamon                = vb2_ioctl_streamon,
> +	.vidioc_streamoff               = vb2_ioctl_streamoff,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 file operations
>   */
> @@ -473,6 +584,9 @@ static int stf_link_validate(struct media_link *link)
>  	struct stfcamss_video *video = video_get_drvdata(vdev);
>  	int ret;
>
> +	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
> +		return 0;
> +
>  	ret = stf_video_check_format(video);
>
>  	return ret;
> @@ -506,7 +620,11 @@ int stf_video_register(struct stfcamss_video *video,
>  	q = &video->vb2_q;
>  	q->drv_priv = video;
>  	q->mem_ops = &vb2_dma_contig_memops;
> -	q->ops = &stf_video_vb2_q_ops;
> +
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		q->ops = &stf_video_vb2_q_ops;
> +	else
> +		q->ops = &stf_video_scd_vb2_q_ops;
>  	q->type = video->type;
>  	q->io_modes = VB2_DMABUF | VB2_MMAP;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> @@ -529,16 +647,28 @@ int stf_video_register(struct stfcamss_video *video,
>  		goto err_mutex_destroy;
>  	}
>
> -	ret = stf_video_init_format(video);
> -	if (ret < 0) {
> -		dev_err(video->stfcamss->dev,
> -			"Failed to init format: %d\n", ret);
> -		goto err_media_cleanup;
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +		ret = stf_video_init_format(video);
> +		if (ret < 0) {
> +			dev_err(video->stfcamss->dev,
> +				"Failed to init format: %d\n", ret);
> +			goto err_media_cleanup;
> +		}
> +		vdev->ioctl_ops = &stf_vid_ioctl_ops;
> +		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
> +	} else {
> +		ret = stf_video_scd_init_format(video);
> +		if (ret < 0) {
> +			dev_err(video->stfcamss->dev,
> +				"Failed to init format: %d\n", ret);
> +			goto err_media_cleanup;
> +		}
> +		vdev->ioctl_ops = &stf_vid_scd_ioctl_ops;
> +		vdev->device_caps = V4L2_CAP_META_CAPTURE;
>  	}
>
>  	vdev->fops = &stf_vid_fops;
> -	vdev->ioctl_ops = &stf_vid_ioctl_ops;
> -	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	vdev->device_caps |= V4L2_CAP_STREAMING;
>  	vdev->entity.ops = &stf_media_ops;
>  	vdev->vfl_dir = VFL_DIR_RX;
>  	vdev->release = stf_video_release;
> diff --git a/drivers/staging/media/starfive/camss/stf-video.h b/drivers/staging/media/starfive/camss/stf-video.h
> index 59799b65cbe5..53a1cf4e59b7 100644
> --- a/drivers/staging/media/starfive/camss/stf-video.h
> +++ b/drivers/staging/media/starfive/camss/stf-video.h
> @@ -37,6 +37,7 @@ enum stf_v_line_id {
>  enum stf_capture_type {
>  	STF_CAPTURE_RAW = 0,
>  	STF_CAPTURE_YUV,
> +	STF_CAPTURE_SCD,
>  	STF_CAPTURE_NUM,
>  };
>
> --
> 2.25.1
>
>
diff mbox series

Patch

diff --git a/drivers/staging/media/starfive/camss/stf-buffer.h b/drivers/staging/media/starfive/camss/stf-buffer.h
index 9d1670fb05ed..727d00617448 100644
--- a/drivers/staging/media/starfive/camss/stf-buffer.h
+++ b/drivers/staging/media/starfive/camss/stf-buffer.h
@@ -23,6 +23,7 @@  enum stf_v_state {
 struct stfcamss_buffer {
 	struct vb2_v4l2_buffer vb;
 	dma_addr_t addr[2];
+	void *vaddr;
 	struct list_head queue;
 };
 
diff --git a/drivers/staging/media/starfive/camss/stf-camss.c b/drivers/staging/media/starfive/camss/stf-camss.c
index fecd3e67c7a1..fafa3ce2f6da 100644
--- a/drivers/staging/media/starfive/camss/stf-camss.c
+++ b/drivers/staging/media/starfive/camss/stf-camss.c
@@ -8,6 +8,7 @@ 
  *
  * Author: Jack Zhu <jack.zhu@starfivetech.com>
  * Author: Changhuang Liang <changhuang.liang@starfivetech.com>
+ * Author: Keith Zhao <keith.zhao@starfivetech.com>
  *
  */
 #include <linux/module.h>
@@ -126,6 +127,7 @@  static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
 static int stfcamss_register_devs(struct stfcamss *stfcamss)
 {
 	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
+	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
 	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
 	int ret;
 
@@ -150,8 +152,18 @@  static int stfcamss_register_devs(struct stfcamss *stfcamss)
 
 	cap_yuv->video.source_subdev = &isp_dev->subdev;
 
+	ret = media_create_pad_link(&isp_dev->subdev.entity, STF_ISP_PAD_SRC_SCD,
+				    &cap_scd->video.vdev.entity, 0, 0);
+	if (ret)
+		goto err_rm_links0;
+
+	cap_scd->video.source_subdev = &isp_dev->subdev;
+
 	return ret;
 
+err_rm_links0:
+	media_entity_remove_links(&isp_dev->subdev.entity);
+	media_entity_remove_links(&cap_yuv->video.vdev.entity);
 err_cap_unregister:
 	stf_capture_unregister(stfcamss);
 err_isp_unregister:
@@ -163,10 +175,12 @@  static int stfcamss_register_devs(struct stfcamss *stfcamss)
 static void stfcamss_unregister_devs(struct stfcamss *stfcamss)
 {
 	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
+	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
 	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
 
 	media_entity_remove_links(&isp_dev->subdev.entity);
 	media_entity_remove_links(&cap_yuv->video.vdev.entity);
+	media_entity_remove_links(&cap_scd->video.vdev.entity);
 
 	stf_isp_unregister(&stfcamss->isp_dev);
 	stf_capture_unregister(stfcamss);
@@ -436,5 +450,6 @@  module_platform_driver(stfcamss_driver);
 
 MODULE_AUTHOR("Jack Zhu <jack.zhu@starfivetech.com>");
 MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
+MODULE_AUTHOR("Keith Zhao <keith.zhao@starfivetech.com>");
 MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/staging/media/starfive/camss/stf-capture.c b/drivers/staging/media/starfive/camss/stf-capture.c
index 75f6ef405e61..328b8c6e351d 100644
--- a/drivers/staging/media/starfive/camss/stf-capture.c
+++ b/drivers/staging/media/starfive/camss/stf-capture.c
@@ -12,6 +12,7 @@ 
 static const char * const stf_cap_names[] = {
 	"capture_raw",
 	"capture_yuv",
+	"capture_scd",
 };
 
 static const struct stfcamss_format_info stf_wr_fmts[] = {
@@ -55,6 +56,14 @@  static const struct stfcamss_format_info stf_isp_fmts[] = {
 	},
 };
 
+/* 3A Statistics Collection Data */
+static const struct stfcamss_format_info stf_isp_scd_fmts[] = {
+	{
+		.code = MEDIA_BUS_FMT_METADATA_FIXED,
+		.pixelformat = V4L2_META_FMT_STF_ISP_STAT_3A,
+	},
+};
+
 static inline struct stf_capture *to_stf_capture(struct stfcamss_video *video)
 {
 	return container_of(video, struct stf_capture, video);
@@ -84,6 +93,8 @@  static void stf_init_addrs(struct stfcamss_video *video)
 		stf_set_raw_addr(video->stfcamss, addr0);
 	else if (cap->type == STF_CAPTURE_YUV)
 		stf_set_yuv_addr(video->stfcamss, addr0, addr1);
+	else
+		stf_set_scd_addr(video->stfcamss, addr0, addr1, TYPE_AWB);
 }
 
 static void stf_cap_s_cfg(struct stfcamss_video *video)
@@ -227,18 +238,24 @@  static void stf_capture_init(struct stfcamss *stfcamss, struct stf_capture *cap)
 	INIT_LIST_HEAD(&cap->buffers.ready_bufs);
 	spin_lock_init(&cap->buffers.lock);
 
-	cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	cap->video.stfcamss = stfcamss;
 	cap->video.bpl_alignment = 16 * 8;
 
 	if (cap->type == STF_CAPTURE_RAW) {
+		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 		cap->video.formats = stf_wr_fmts;
 		cap->video.nformats = ARRAY_SIZE(stf_wr_fmts);
 		cap->video.bpl_alignment = 8;
 	} else if (cap->type == STF_CAPTURE_YUV) {
+		cap->video.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 		cap->video.formats = stf_isp_fmts;
 		cap->video.nformats = ARRAY_SIZE(stf_isp_fmts);
 		cap->video.bpl_alignment = 1;
+	} else {
+		cap->video.type = V4L2_BUF_TYPE_META_CAPTURE;
+		cap->video.formats = stf_isp_scd_fmts;
+		cap->video.nformats = ARRAY_SIZE(stf_isp_scd_fmts);
+		cap->video.bpl_alignment = 16 * 8;
 	}
 }
 
@@ -362,9 +379,11 @@  void stf_capture_unregister(struct stfcamss *stfcamss)
 {
 	struct stf_capture *cap_raw = &stfcamss->captures[STF_CAPTURE_RAW];
 	struct stf_capture *cap_yuv = &stfcamss->captures[STF_CAPTURE_YUV];
+	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
 
 	stf_capture_unregister_one(cap_raw);
 	stf_capture_unregister_one(cap_yuv);
+	stf_capture_unregister_one(cap_scd);
 }
 
 int stf_capture_register(struct stfcamss *stfcamss,
diff --git a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
index 6b3966ca18bf..3b18d09f2cc6 100644
--- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
+++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
@@ -451,11 +451,57 @@  void stf_set_yuv_addr(struct stfcamss *stfcamss,
 	stf_isp_reg_write(stfcamss, ISP_REG_UV_PLANE_START_ADDR, uv_addr);
 }
 
+static enum stf_isp_type_scd stf_isp_get_scd_type(struct stfcamss *stfcamss)
+{
+	int val;
+
+	val = stf_isp_reg_read(stfcamss, ISP_REG_SC_CFG_1);
+	return (enum stf_isp_type_scd)(val & ISP_SC_SEL_MASK) >> 30;
+}
+
+void stf_set_scd_addr(struct stfcamss *stfcamss,
+		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
+		      enum stf_isp_type_scd type_scd)
+{
+	stf_isp_reg_set_bit(stfcamss, ISP_REG_SC_CFG_1, ISP_SC_SEL_MASK,
+			    SEL_TYPE(type_scd));
+	stf_isp_reg_write(stfcamss, ISP_REG_SCD_CFG_0, scd_addr);
+	stf_isp_reg_write(stfcamss, ISP_REG_YHIST_CFG_4, yhist_addr);
+}
+
+static void stf_isp_fill_yhist(struct stfcamss *stfcamss, void *vaddr)
+{
+	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
+	u32 reg_addr = ISP_REG_YHIST_ACC_0;
+	u32 i;
+
+	for (i = 0; i < 64; i++, reg_addr += 4)
+		sc->y_histogram[i] = stf_isp_reg_read(stfcamss, reg_addr);
+}
+
+static void stf_isp_fill_flag(struct stfcamss *stfcamss, void *vaddr,
+			      enum stf_isp_type_scd *type_scd)
+{
+	struct jh7110_isp_sc_buffer *sc = (struct jh7110_isp_sc_buffer *)vaddr;
+
+	*type_scd = stf_isp_get_scd_type(stfcamss);
+	if (*type_scd == TYPE_AWB) {
+		sc->flag = JH7110_ISP_SC_FLAG_AWB;
+		*type_scd = TYPE_OECF;
+	} else {
+		sc->flag = JH7110_ISP_SC_FLAG_AE_AF;
+		*type_scd = TYPE_AWB;
+	}
+}
+
 irqreturn_t stf_line_irq_handler(int irq, void *priv)
 {
 	struct stfcamss *stfcamss = priv;
 	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
+	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
 	struct stfcamss_buffer *change_buf;
+	enum stf_isp_type_scd type_scd;
+	u32 value;
 	u32 status;
 
 	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
@@ -467,6 +513,17 @@  irqreturn_t stf_line_irq_handler(int irq, void *priv)
 					stf_set_yuv_addr(stfcamss, change_buf->addr[0],
 							 change_buf->addr[1]);
 			}
+
+			value = stf_isp_reg_read(stfcamss, ISP_REG_CSI_MODULE_CFG);
+			if (value & CSI_SC_EN) {
+				change_buf = stf_change_buffer(&cap_scd->buffers);
+				if (change_buf) {
+					stf_isp_fill_flag(stfcamss, change_buf->vaddr,
+							  &type_scd);
+					stf_set_scd_addr(stfcamss, change_buf->addr[0],
+							 change_buf->addr[1], type_scd);
+				}
+			}
 		}
 
 		stf_isp_reg_set_bit(stfcamss, ISP_REG_CSIINTS,
@@ -485,6 +542,7 @@  irqreturn_t stf_isp_irq_handler(int irq, void *priv)
 {
 	struct stfcamss *stfcamss = priv;
 	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
+	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
 	struct stfcamss_buffer *ready_buf;
 	u32 status;
 
@@ -496,6 +554,14 @@  irqreturn_t stf_isp_irq_handler(int irq, void *priv)
 				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
 		}
 
+		if (status & ISPC_SC) {
+			ready_buf = stf_buf_done(&cap_scd->buffers);
+			if (ready_buf) {
+				stf_isp_fill_yhist(stfcamss, ready_buf->vaddr);
+				vb2_buffer_done(&ready_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+			}
+		}
+
 		stf_isp_reg_write(stfcamss, ISP_REG_ISP_CTRL_0,
 				  (status & ~ISPC_INT_ALL_MASK) |
 				  ISPC_ISP | ISPC_CSI | ISPC_SC);
diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
index fcda0502e3b0..0af7b367e57a 100644
--- a/drivers/staging/media/starfive/camss/stf-isp.h
+++ b/drivers/staging/media/starfive/camss/stf-isp.h
@@ -10,6 +10,7 @@ 
 #ifndef STF_ISP_H
 #define STF_ISP_H
 
+#include <linux/jh7110-isp.h>
 #include <media/v4l2-subdev.h>
 
 #include "stf-video.h"
@@ -107,6 +108,12 @@ 
 #define Y_COOR(y)				((y) << 16)
 #define X_COOR(x)				((x) << 0)
 
+#define ISP_REG_SCD_CFG_0			0x098
+
+#define ISP_REG_SC_CFG_1			0x0bc
+#define ISP_SC_SEL_MASK				GENMASK(31, 30)
+#define SEL_TYPE(n)				((n) << 30)
+
 #define ISP_REG_LCCF_CFG_2			0x0e0
 #define ISP_REG_LCCF_CFG_3			0x0e4
 #define ISP_REG_LCCF_CFG_4			0x0e8
@@ -305,6 +312,10 @@ 
 #define DNRM_F(n)				((n) << 16)
 #define CCM_M_DAT(n)				((n) << 0)
 
+#define ISP_REG_YHIST_CFG_4			0xcd8
+
+#define ISP_REG_YHIST_ACC_0			0xd00
+
 #define ISP_REG_GAMMA_VAL0			0xe00
 #define ISP_REG_GAMMA_VAL1			0xe04
 #define ISP_REG_GAMMA_VAL2			0xe08
@@ -389,6 +400,15 @@ 
 #define IMAGE_MAX_WIDTH				1920
 #define IMAGE_MAX_HEIGH				1080
 
+#define ISP_YHIST_BUFFER_SIZE			(64 * sizeof(__u32))
+
+enum stf_isp_type_scd {
+	TYPE_DEC = 0,
+	TYPE_OBC,
+	TYPE_OECF,
+	TYPE_AWB,
+};
+
 /* pad id for media framework */
 enum stf_isp_pad_id {
 	STF_ISP_PAD_SINK = 0,
@@ -429,5 +449,8 @@  int stf_isp_unregister(struct stf_isp_dev *isp_dev);
 
 void stf_set_yuv_addr(struct stfcamss *stfcamss,
 		      dma_addr_t y_addr, dma_addr_t uv_addr);
+void stf_set_scd_addr(struct stfcamss *stfcamss,
+		      dma_addr_t yhist_addr, dma_addr_t scd_addr,
+		      enum stf_isp_type_scd type_scd);
 
 #endif /* STF_ISP_H */
diff --git a/drivers/staging/media/starfive/camss/stf-video.c b/drivers/staging/media/starfive/camss/stf-video.c
index 989b5e82bae9..2203605ec9c7 100644
--- a/drivers/staging/media/starfive/camss/stf-video.c
+++ b/drivers/staging/media/starfive/camss/stf-video.c
@@ -125,6 +125,14 @@  static int stf_video_init_format(struct stfcamss_video *video)
 	return 0;
 }
 
+static int stf_video_scd_init_format(struct stfcamss_video *video)
+{
+	video->active_fmt.fmt.meta.dataformat = video->formats[0].pixelformat;
+	video->active_fmt.fmt.meta.buffersize = sizeof(struct jh7110_isp_sc_buffer);
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Video queue operations
  */
@@ -330,6 +338,78 @@  static const struct vb2_ops stf_video_vb2_q_ops = {
 	.stop_streaming  = video_stop_streaming,
 };
 
+static int video_scd_queue_setup(struct vb2_queue *q,
+				 unsigned int *num_buffers,
+				 unsigned int *num_planes,
+				 unsigned int sizes[],
+				 struct device *alloc_devs[])
+{
+	if (*num_planes)
+		return sizes[0] < sizeof(struct jh7110_isp_sc_buffer) ? -EINVAL : 0;
+
+	*num_planes = 1;
+	sizes[0] = sizeof(struct jh7110_isp_sc_buffer);
+
+	return 0;
+}
+
+static int video_scd_buf_init(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_buffer *buffer = to_stfcamss_buffer(vbuf);
+	dma_addr_t *paddr;
+
+	paddr = vb2_plane_cookie(vb, 0);
+	buffer->addr[0] = *paddr;
+	buffer->addr[1] = buffer->addr[0] + ISP_YHIST_BUFFER_SIZE;
+	buffer->vaddr = vb2_plane_vaddr(vb, 0);
+
+	return 0;
+}
+
+static int video_scd_buf_prepare(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+
+	if (sizeof(struct jh7110_isp_sc_buffer) > vb2_plane_size(vb, 0))
+		return -EINVAL;
+
+	vb2_set_plane_payload(vb, 0, sizeof(struct jh7110_isp_sc_buffer));
+
+	vbuf->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static int video_scd_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+
+	video->ops->start_streaming(video);
+
+	return 0;
+}
+
+static void video_scd_stop_streaming(struct vb2_queue *q)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+
+	video->ops->stop_streaming(video);
+
+	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
+}
+
+static const struct vb2_ops stf_video_scd_vb2_q_ops = {
+	.queue_setup     = video_scd_queue_setup,
+	.wait_prepare    = vb2_ops_wait_prepare,
+	.wait_finish     = vb2_ops_wait_finish,
+	.buf_init        = video_scd_buf_init,
+	.buf_prepare     = video_scd_buf_prepare,
+	.buf_queue       = video_buf_queue,
+	.start_streaming = video_scd_start_streaming,
+	.stop_streaming  = video_scd_stop_streaming,
+};
+
 /* -----------------------------------------------------------------------------
  * V4L2 ioctls
  */
@@ -448,6 +528,37 @@  static const struct v4l2_ioctl_ops stf_vid_ioctl_ops = {
 	.vidioc_streamoff               = vb2_ioctl_streamoff,
 };
 
+static int video_scd_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	struct v4l2_meta_format *meta = &f->fmt.meta;
+
+	if (f->type != video->type)
+		return -EINVAL;
+
+	meta->dataformat = video->active_fmt.fmt.meta.dataformat;
+	meta->buffersize = video->active_fmt.fmt.meta.buffersize;
+
+	return 0;
+}
+
+static const struct v4l2_ioctl_ops stf_vid_scd_ioctl_ops = {
+	.vidioc_querycap                = video_querycap,
+	.vidioc_enum_fmt_meta_cap       = video_enum_fmt,
+	.vidioc_g_fmt_meta_cap          = video_scd_g_fmt,
+	.vidioc_s_fmt_meta_cap          = video_scd_g_fmt,
+	.vidioc_try_fmt_meta_cap        = video_scd_g_fmt,
+	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
+	.vidioc_querybuf                = vb2_ioctl_querybuf,
+	.vidioc_qbuf                    = vb2_ioctl_qbuf,
+	.vidioc_expbuf                  = vb2_ioctl_expbuf,
+	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
+	.vidioc_streamon                = vb2_ioctl_streamon,
+	.vidioc_streamoff               = vb2_ioctl_streamoff,
+};
+
 /* -----------------------------------------------------------------------------
  * V4L2 file operations
  */
@@ -473,6 +584,9 @@  static int stf_link_validate(struct media_link *link)
 	struct stfcamss_video *video = video_get_drvdata(vdev);
 	int ret;
 
+	if (video->type == V4L2_BUF_TYPE_META_CAPTURE)
+		return 0;
+
 	ret = stf_video_check_format(video);
 
 	return ret;
@@ -506,7 +620,11 @@  int stf_video_register(struct stfcamss_video *video,
 	q = &video->vb2_q;
 	q->drv_priv = video;
 	q->mem_ops = &vb2_dma_contig_memops;
-	q->ops = &stf_video_vb2_q_ops;
+
+	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		q->ops = &stf_video_vb2_q_ops;
+	else
+		q->ops = &stf_video_scd_vb2_q_ops;
 	q->type = video->type;
 	q->io_modes = VB2_DMABUF | VB2_MMAP;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
@@ -529,16 +647,28 @@  int stf_video_register(struct stfcamss_video *video,
 		goto err_mutex_destroy;
 	}
 
-	ret = stf_video_init_format(video);
-	if (ret < 0) {
-		dev_err(video->stfcamss->dev,
-			"Failed to init format: %d\n", ret);
-		goto err_media_cleanup;
+	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+		ret = stf_video_init_format(video);
+		if (ret < 0) {
+			dev_err(video->stfcamss->dev,
+				"Failed to init format: %d\n", ret);
+			goto err_media_cleanup;
+		}
+		vdev->ioctl_ops = &stf_vid_ioctl_ops;
+		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
+	} else {
+		ret = stf_video_scd_init_format(video);
+		if (ret < 0) {
+			dev_err(video->stfcamss->dev,
+				"Failed to init format: %d\n", ret);
+			goto err_media_cleanup;
+		}
+		vdev->ioctl_ops = &stf_vid_scd_ioctl_ops;
+		vdev->device_caps = V4L2_CAP_META_CAPTURE;
 	}
 
 	vdev->fops = &stf_vid_fops;
-	vdev->ioctl_ops = &stf_vid_ioctl_ops;
-	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	vdev->device_caps |= V4L2_CAP_STREAMING;
 	vdev->entity.ops = &stf_media_ops;
 	vdev->vfl_dir = VFL_DIR_RX;
 	vdev->release = stf_video_release;
diff --git a/drivers/staging/media/starfive/camss/stf-video.h b/drivers/staging/media/starfive/camss/stf-video.h
index 59799b65cbe5..53a1cf4e59b7 100644
--- a/drivers/staging/media/starfive/camss/stf-video.h
+++ b/drivers/staging/media/starfive/camss/stf-video.h
@@ -37,6 +37,7 @@  enum stf_v_line_id {
 enum stf_capture_type {
 	STF_CAPTURE_RAW = 0,
 	STF_CAPTURE_YUV,
+	STF_CAPTURE_SCD,
 	STF_CAPTURE_NUM,
 };