| Message ID | 20240709083824.430473-9-changhuang.liang@starfivetech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add ISP 3A for StarFive | expand |
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 > >
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
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 >
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 > > >
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 > > > >
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
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 > >
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 > > > >
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 > > > > > >
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
>
>
Hi Jacopo > > 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 ? > Yes, thank you for your reminder. Regards, Changhuang
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, };
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(-)