[PATCH] staging: media: starfive: Add multiple resolution support

[PATCH] staging: media: starfive: Add multiple resolution support

[Changhuang Liang]

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;
 	u32 val;

 	if (cap->type == STF_CAPTURE_RAW) {
+		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);

 		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
 	} else if (cap->type == STF_CAPTURE_YUV) {
--
2.25.1

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

[Changhuang Liang]

Hi, Hans

> Add multiple resolution support for video "capture_raw" device. Otherwise it
> will capture the wrong image data if the width is not 1920.
> 

Can you help to review this patch, Thanks for your times.

Regards,
Changhuang

> 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;
>  	u32 val;
> 
>  	if (cap->type == STF_CAPTURE_RAW) {
> +		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);
> 
>  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
>  	} else if (cap->type == STF_CAPTURE_YUV) {
> --
> 2.25.1

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

[Hans Verkuil]

On 09/08/2024 10:29, Changhuang Liang wrote:
> Hi, Hans
> 
>> Add multiple resolution support for video "capture_raw" device. Otherwise it
>> will capture the wrong image data if the width is not 1920.
>>
> 
> Can you help to review this patch, Thanks for your times.

Patch looks OK, it will be part of my next 'various fixes' Pull Request.

Regards,

	Hans

> 
> Regards,
> Changhuang
> 
>> 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;
>>  	u32 val;
>>
>>  	if (cap->type == STF_CAPTURE_RAW) {
>> +		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);
>>
>>  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
>>  	} else if (cap->type == STF_CAPTURE_YUV) {
>> --
>> 2.25.1

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

[Laurent Pinchart]

Hi Changhuang,

Thank you for the patch.

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 ?

> 
>  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
>  	} else if (cap->type == STF_CAPTURE_YUV) {

-- 
Regards,

Laurent Pinchart

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

[Changhuang Liang]

Hi, Laurent

Thanks for your comments.

> Hi Changhuang,
> 
> Thank you for the patch.
> 
> 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.

Ragards,
Changhuang

> >
> >  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
> >  	} else if (cap->type == STF_CAPTURE_YUV) {
> 
> --
> Regards,
> 
> Laurent Pinchart

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

[Laurent Pinchart]

On Fri, Aug 09, 2024 at 12:12:01PM +0000, Changhuang Liang wrote:
> Hi, Laurent
> 
> Thanks for your comments.
> 
> > Hi Changhuang,
> > 
> > Thank you for the patch.
> > 
> > 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 ?

> > >
> > >  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
> > >  	} else if (cap->type == STF_CAPTURE_YUV) {

-- 
Regards,

Laurent Pinchart

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

[Changhuang Liang]

Hi, Laurent

> 
> On Fri, Aug 09, 2024 at 12:12:01PM +0000, Changhuang Liang wrote:
> > Hi, Laurent
> >
> > Thanks for your comments.
> >
> > > Hi Changhuang,
> > >
> > > Thank you for the patch.
> > >
> > > 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;

Regards,
Changhuang

> > > >
> > > >  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
> > > >  	} else if (cap->type == STF_CAPTURE_YUV) {
> 
> --
> Regards,
> 
> Laurent Pinchart

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

[Laurent Pinchart]

Hi Changhuang,

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 ?

> > > > >
> > > > >  		stf_syscon_reg_write(stfcamss, VIN_INRT_PIX_CFG, val);
> > > > >  	} else if (cap->type == STF_CAPTURE_YUV) {

-- 
Regards,

Laurent Pinchart

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

[Changhuang Liang]

Hi, Laurent

> 
> Hi Changhuang,
> 
> 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.
If is "capture_yuv" video device, it will be set by stf_isp_config_crop.

Regards,
Changhuang

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

[Laurent Pinchart]

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

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

[Changhuang Liang]

Hi, Lanrent

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

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

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

[Laurent Pinchart]

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 ?

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

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

[Changhuang Liang]

Hi, Laurent

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

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

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

[Laurent Pinchart]

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