Re: 回复: 回复: 回复: 回复: 回复: [PATCH] staging: media: starfive: Add multiple resolution support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Aug 19, 2024 at 01:18:01PM +0000, Changhuang Liang wrote:
> > On Mon, Aug 19, 2024 at 01:37:30AM +0000, Changhuang Liang wrote:
> > > > On Mon, Aug 12, 2024 at 12:13:03PM +0000, Changhuang Liang wrote:
> > > > > > On Mon, Aug 12, 2024 at 09:43:47AM +0000, Changhuang Liang wrote:
> > > > > > > > On Fri, Aug 09, 2024 at 12:12:01PM +0000, Changhuang Liang wrote:
> > > > > > > > > > On Fri, Apr 19, 2024 at 01:19:55AM -0700, Changhuang Liang wrote:
> > > > > > > > > > > Add multiple resolution support for video "capture_raw" device.
> > > > > > > > > > > Otherwise it will capture the wrong image data if the width is not 1920.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: e080f339c80a ("media: staging: media: starfive: camss: Add capture driver")
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/staging/media/starfive/camss/stf-capture.c | 5 ++++-
> > > > > > > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git
> > > > > > > > > > > a/drivers/staging/media/starfive/camss/stf-capture.c
> > > > > > > > > > > b/drivers/staging/media/starfive/camss/stf-capture.c
> > > > > > > > > > > index ec5169e7b391..9e853ff2596a 100644
> > > > > > > > > > > --- a/drivers/staging/media/starfive/camss/stf-capture.c
> > > > > > > > > > > +++ b/drivers/staging/media/starfive/camss/stf-capture.c
> > > > > > > > > > > @@ -177,9 +177,12 @@ static void stf_channel_set(struct stfcamss_video *video)  {
> > > > > > > > > > >  	struct stf_capture *cap = to_stf_capture(video);
> > > > > > > > > > >  	struct stfcamss *stfcamss = cap->video.stfcamss;
> > > > > > > > > > > +	struct v4l2_pix_format *pix;
> > > > > > > > > >
> > > > > > > > > > This variable can be const as you don't modify the format.
> > > > > > > > > >
> > > > > > > > > > >  	u32 val;
> > > > > > > > > > >
> > > > > > > > > > >  	if (cap->type == STF_CAPTURE_RAW) {
> > > > > > > > > > > +		pix = &video->active_fmt.fmt.pix;
> > > > > > > > > >
> > > > > > > > > > And it can be declared and initialized here:
> > > > > > > > > >
> > > > > > > > > > 		const struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
> > > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > >  		val = stf_syscon_reg_read(stfcamss, VIN_CHANNEL_SEL_EN);
> > > > > > > > > > >  		val &= ~U0_VIN_CHANNEL_SEL_MASK;
> > > > > > > > > > >  		val |= CHANNEL(0);
> > > > > > > > > > > @@ -193,7 +196,7 @@ static void stf_channel_set(struct stfcamss_video *video)
> > > > > > > > > > >  		val |= PIXEL_HEIGH_BIT_SEL(0);
> > > > > > > > > > >
> > > > > > > > > > >  		val &= ~U0_VIN_PIX_CNT_END_MASK;
> > > > > > > > > > > -		val |= PIX_CNT_END(IMAGE_MAX_WIDTH / 4 - 1);
> > > > > > > > > > > +		val |= PIX_CNT_END(pix->width / 4 - 1);
> > > > > > > > > >
> > > > > > > > > > Is there no need to consider the image height as well ?
> > > > > > > > > > How does the driver prevent buffer overflows if the
> > > > > > > > > > sensor sends more data than expected ?
> > > > > > > > >
> > > > > > > > > Our hardware will confirm a frame of data through vblank
> > > > > > > > > signal, so there is no image height configuration.
> > > > > > > >
> > > > > > > > What happens if the system expects, for instance, a 1920x1080
> > > > > > > > RAW8 image, and allocates a buffer of of 1920x1080 bytes,
> > > > > > > > but the sensor outputs more lines ? Does the camera hardware
> > > > > > > > in the SoC offer an option to prevent buffer overruns ?
> > > > > > >
> > > > > > > The hardware can confirm the image height by using the VSYNC signal.
> > > > > > >
> > > > > > > Image will transfer when VSYNC is high.
> > > > > > >
> > > > > > > VSYNC time = (width + h_blank) * height;
> > > > > >
> > > > > > What I'm trying to understand is what happens if the ISP is configured for
> > > > > > 1080 lines, but the camera sensor sends more than 1080 lines
> > > > > > (the VSYNC signal is active for more than 1080 lines). Where in
> > > > > > the driver is the hardware configure with the 1080 lines
> > > > > > limit to avoid buffer overflows ?
> > > > >
> > > > > If is "capture_raw" video device, no image height can be configured.
> > > >
> > > > In that case what happens if the camera sensor sends more lines than
> > > > expected ? Will the raw video device write past the end of the buffer ?
> > >
> > > Yes, the buffer will overflows, so we will use the software restrictions.
> > > Implement .link_validate hooks for the CSI2RX subdev and
> > > "capture_raw" video device.
> > 
> > Is there an IOMMU in the system that could help preventing buffer overflows
> > to reach system memory ?
> 
> JH7110 SoC has no IOMMU.

Thank you for the information.

Based on this, the best we can do is hoping that the video source will
never send more lines than expected. In most cases there will be no
issue, but signal glitches or other hardware glitches on the sensor side
may cause memory corruption. That sounds like a worrying hardware
vulnerability :-(

> > > > If so, is there a way to guard against that ?
> > > >
> > > > > If is "capture_yuv" video device, it will be set by stf_isp_config_crop.
> > > >
> > > > Thank you, that's the information I was looking for.

-- 
Regards,

Laurent Pinchart