Re: 回复: [PATCH v5 08/14] staging: media: starfive: Add for StarFive ISP 3A SC

From: Jacopo Mondi
Date: Thu Jul 11 2024 - 04:27:07 EST

Next message: Aleksandar Rikalo: "[PATCH v5 00/11] MIPS: Support I6500 multi-cluster configuration"
Previous message: Christoph Hellwig: "Re: [PATCH 0/3] Add {init, exit}_sequence_fs() helper function"
In reply to: Changhuang Liang: " 回复: [PATCH v5 08/14] staging: media: starfive: Add for StarFive ISP 3A SC"
Next in thread: Changhuang Liang: " 回复: 回复: [PATCH v5 08/14] staging: media: starfive: Add for StarFive ISP 3A SC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxxx>
> > > ---
> > > .../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@xxxxxxxxxxxxxxxx>
> > > * Author: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > > + * Author: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
> > > *
> > > */
> > > #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@xxxxxxxxxxxxxxxx>");
> > > MODULE_AUTHOR("Changhuang Liang
> > <changhuang.liang@xxxxxxxxxxxxxxxx>");
> > > +MODULE_AUTHOR("Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>");
> > > 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
>

Next message: Aleksandar Rikalo: "[PATCH v5 00/11] MIPS: Support I6500 multi-cluster configuration"
Previous message: Christoph Hellwig: "Re: [PATCH 0/3] Add {init, exit}_sequence_fs() helper function"
In reply to: Changhuang Liang: " 回复: [PATCH v5 08/14] staging: media: starfive: Add for StarFive ISP 3A SC"
Next in thread: Changhuang Liang: " 回复: 回复: [PATCH v5 08/14] staging: media: starfive: Add for StarFive ISP 3A SC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]