Message-ID: <vxlefgqtdy6qucjxk7npq5snjful3apnsd43jefpxqs42ozkfz@v6ixvqw2vxvh>
Date: Fri, 12 Jul 2024 18:25:06 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Changhuang Liang <changhuang.liang@...rfivetech.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans Verkuil <hverkuil-cisco@...all.nl>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Jean-Michel Hautbois <jeanmichel.hautbois@...asonboard.com>, 
	Benjamin Gaignard <benjamin.gaignard@...labora.com>, Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>, 
	Mingjia Zhang <mingjia.zhang@...iatek.com>, Jack Zhu <jack.zhu@...rfivetech.com>, 
	Keith Zhao <keith.zhao@...rfivetech.com>, "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>
Subject: Re: 回复: 回复: 回复: 回复: [PATCH v5 08/14] staging: media: starfive: Add for
 StarFive ISP 3A SC

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
> > >
> > >

Powered by blists - more mailing lists