diff mbox series

[v3,06/12] media: staging: rkisp1: remove atomic operations for frame sequence

Message ID 20200922113402.12442-7-dafna.hirschfeld@collabora.com
State Accepted
Commit 21f4417263b6c87662dc7610ab0d88d23a70a90e
Headers show
Series [v3,01/12] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers | expand

Commit Message

Dafna Hirschfeld Sept. 22, 2020, 11:33 a.m. UTC 
The isp.frame_sequence is now read only from the irq handlers
that are all fired from the same interrupt, so there is not need
for atomic operation.
In addition, the frame seq incrementation is moved from
rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
clearer and the incorrect inline comment is removed.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>

---
changes since v2:
add a closing "}" to if condition
remove usless inline comment
---
 drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
 drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
 drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
 5 files changed, 9 insertions(+), 16 deletions(-)

Comments

Tomasz Figa Sept. 25, 2020, 8:42 p.m. UTC | #1 
Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:
> The isp.frame_sequence is now read only from the irq handlers

> that are all fired from the same interrupt, so there is not need

> for atomic operation.

> In addition, the frame seq incrementation is moved from

> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code

> clearer and the incorrect inline comment is removed.

> 

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

> Acked-by: Helen Koike <helen.koike@collabora.com>

> 

> ---

> changes since v2:

> add a closing "}" to if condition

> remove usless inline comment

> ---

>  drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-

>  drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-

>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------

>  drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-

>  drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--

>  5 files changed, 9 insertions(+), 16 deletions(-)

> 


Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c

> index 0632582a95b4..1c762a369b63 100644

> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c

> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c

> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)

>  	curr_buf = cap->buf.curr;

>  

>  	if (curr_buf) {

> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);

> +		curr_buf->vb.sequence = isp->frame_sequence;


I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
memory-intensive system load, like video encoding and graphics rendering,
the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
the V_START for the next frame.

I recall you did some testing back in time [1], showing that the two are
interleaved. Do you remember what CAPTURE resolution was it?

>  		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();

>  		curr_buf->vb.field = V4L2_FIELD_NONE;

>  		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);

> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h

> index 232bee92d0eb..51c92a251ea5 100644

> --- a/drivers/staging/media/rkisp1/rkisp1-common.h

> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h

> @@ -131,7 +131,7 @@ struct rkisp1_isp {

>  	const struct rkisp1_isp_mbus_info *src_fmt;

>  	struct mutex ops_lock; /* serialize the subdevice ops */

>  	bool is_dphy_errctrl_disabled;

> -	atomic_t frame_sequence;

> +	__u32 frame_sequence;


nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
types. For kernel types please just use the plain u32.

Best regards,
Tomasz
Dafna Hirschfeld Oct. 2, 2020, 9:16 a.m. UTC | #2 
Hi,

Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:
>> The isp.frame_sequence is now read only from the irq handlers
>> that are all fired from the same interrupt, so there is not need
>> for atomic operation.
>> In addition, the frame seq incrementation is moved from
>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
>> clearer and the incorrect inline comment is removed.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> changes since v2:
>> add a closing "}" to if condition
>> remove usless inline comment
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
>>   5 files changed, 9 insertions(+), 16 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 0632582a95b4..1c762a369b63 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
>>   	curr_buf = cap->buf.curr;
>>   
>>   	if (curr_buf) {
>> -		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
>> +		curr_buf->vb.sequence = isp->frame_sequence;
> 
> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> memory-intensive system load, like video encoding and graphics rendering,
> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> the V_START for the next frame.
> 
> I recall you did some testing back in time [1], showing that the two are
> interleaved. Do you remember what CAPTURE resolution was it?

I ran the testing again, I added a patch to allow streaming simultanously from
both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
Then I ran two tests:
stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR

the pixelformat for both is 422P.
I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
The functionality can probably only be tested on the scarlet.

Thanks,
Dafna



> 
>>   		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
>>   		curr_buf->vb.field = V4L2_FIELD_NONE;
>>   		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 232bee92d0eb..51c92a251ea5 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -131,7 +131,7 @@ struct rkisp1_isp {
>>   	const struct rkisp1_isp_mbus_info *src_fmt;
>>   	struct mutex ops_lock; /* serialize the subdevice ops */
>>   	bool is_dphy_errctrl_disabled;
>> -	atomic_t frame_sequence;
>> +	__u32 frame_sequence;
> 
> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> types. For kernel types please just use the plain u32.
> 
> Best regards,
> Tomasz
>
Tomasz Figa Oct. 2, 2020, 3:30 p.m. UTC | #3 
On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi,
>
> Am 25.09.20 um 22:42 schrieb Tomasz Figa:
> > Hi Dafna,
> >
> > On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:
> >> The isp.frame_sequence is now read only from the irq handlers
> >> that are all fired from the same interrupt, so there is not need
> >> for atomic operation.
> >> In addition, the frame seq incrementation is moved from
> >> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code
> >> clearer and the incorrect inline comment is removed.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> changes since v2:
> >> add a closing "}" to if condition
> >> remove usless inline comment
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------
> >>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-
> >>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--
> >>   5 files changed, 9 insertions(+), 16 deletions(-)
> >>
> >
> > Thank you for the patch. Please see my comments inline.
> >
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >> index 0632582a95b4..1c762a369b63 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
> >>      curr_buf = cap->buf.curr;
> >>
> >>      if (curr_buf) {
> >> -            curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
> >> +            curr_buf->vb.sequence = isp->frame_sequence;
> >
> > I wonder if with higher resolutions, let's say full 5 Mpix, and/or some
> > memory-intensive system load, like video encoding and graphics rendering,
> > the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after
> > the V_START for the next frame.
> >
> > I recall you did some testing back in time [1], showing that the two are
> > interleaved. Do you remember what CAPTURE resolution was it?
>
> I ran the testing again, I added a patch to allow streaming simultanously from
> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da
> Then I ran two tests:
> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP
> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR
>
> the pixelformat for both is 422P.
> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.
> The functionality can probably only be tested on the scarlet.

Okay, thanks. It looks like there is always plenty of time margin on
the hardware side and if the interrupt handling is delayed for a short
time and both are handled by the same handler call, it's also going to
be handled fine because of rkisp1_capture_isr() being called before
rkisp1_isp_isr().

By the way, do we need the MIPI interrupts every frame? Perhaps we
could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,
when we get an error, we disable it and enable
RKISP1_CIF_MIPI_FRAME_END, which would then re-enable
RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?

Best regards,
Tomasz

>
> Thanks,
> Dafna
>
>
>
> >
> >>              curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
> >>              curr_buf->vb.field = V4L2_FIELD_NONE;
> >>              vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> >> index 232bee92d0eb..51c92a251ea5 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> >> @@ -131,7 +131,7 @@ struct rkisp1_isp {
> >>      const struct rkisp1_isp_mbus_info *src_fmt;
> >>      struct mutex ops_lock; /* serialize the subdevice ops */
> >>      bool is_dphy_errctrl_disabled;
> >> -    atomic_t frame_sequence;
> >> +    __u32 frame_sequence;
> >
> > nit: The __ prefixed types are defined for the UAPI to avoid covering userspace
> > types. For kernel types please just use the plain u32.
> >
> > Best regards,
> > Tomasz
> >
Helen Koike Nov. 6, 2020, 2:43 p.m. UTC | #4 
Hi Tomasz,

(sorry for not had replied this earlier)

On 10/2/20 12:30 PM, Tomasz Figa wrote:
> On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld

> <dafna.hirschfeld@collabora.com> wrote:

>>

>> Hi,

>>

>> Am 25.09.20 um 22:42 schrieb Tomasz Figa:

>>> Hi Dafna,

>>>

>>> On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:

>>>> The isp.frame_sequence is now read only from the irq handlers

>>>> that are all fired from the same interrupt, so there is not need

>>>> for atomic operation.

>>>> In addition, the frame seq incrementation is moved from

>>>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code

>>>> clearer and the incorrect inline comment is removed.

>>>>

>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

>>>> Acked-by: Helen Koike <helen.koike@collabora.com>

>>>>

>>>> ---

>>>> changes since v2:

>>>> add a closing "}" to if condition

>>>> remove usless inline comment

>>>> ---

>>>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-

>>>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-

>>>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------

>>>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-

>>>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--

>>>>   5 files changed, 9 insertions(+), 16 deletions(-)

>>>>

>>>

>>> Thank you for the patch. Please see my comments inline.

>>>

>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c

>>>> index 0632582a95b4..1c762a369b63 100644

>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c

>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c

>>>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)

>>>>      curr_buf = cap->buf.curr;

>>>>

>>>>      if (curr_buf) {

>>>> -            curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);

>>>> +            curr_buf->vb.sequence = isp->frame_sequence;

>>>

>>> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some

>>> memory-intensive system load, like video encoding and graphics rendering,

>>> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after

>>> the V_START for the next frame.

>>>

>>> I recall you did some testing back in time [1], showing that the two are

>>> interleaved. Do you remember what CAPTURE resolution was it?

>>

>> I ran the testing again, I added a patch to allow streaming simultanously from

>> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da

>> Then I ran two tests:

>> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP

>> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR

>>

>> the pixelformat for both is 422P.

>> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.

>> The functionality can probably only be tested on the scarlet.

> 

> Okay, thanks. It looks like there is always plenty of time margin on

> the hardware side and if the interrupt handling is delayed for a short

> time and both are handled by the same handler call, it's also going to

> be handled fine because of rkisp1_capture_isr() being called before

> rkisp1_isp_isr().

> 

> By the way, do we need the MIPI interrupts every frame? Perhaps we

> could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,

> when we get an error, we disable it and enable

> RKISP1_CIF_MIPI_FRAME_END, which would then re-enable

> RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?


The driver already do this in a sense, it disables these interrupts on
the first MIPI error and re-enable them on RKISP1_CIF_MIPI_FRAME_END.

Please check:

https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/rkisp1/rkisp1-isp.c#n1069

For convenience:

void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
{
	u32 val, status;

	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
	if (!status)
		return;

	rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);

	/*
	 * Disable DPHY errctrl interrupt, because this dphy
	 * erctrl signal is asserted until the next changes
	 * of line state. This time is may be too long and cpu
	 * is hold in this interrupt.
	 */
	if (status & RKISP1_CIF_MIPI_ERR_CTRL(0x0f)) {
		val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
		rkisp1_write(rkisp1, val & ~RKISP1_CIF_MIPI_ERR_CTRL(0x0f),
			     RKISP1_CIF_MIPI_IMSC);
		rkisp1->isp.is_dphy_errctrl_disabled = true;
	}

	/*
	 * Enable DPHY errctrl interrupt again, if mipi have receive
	 * the whole frame without any error.
	 */
	if (status == RKISP1_CIF_MIPI_FRAME_END) {
		/*
		 * Enable DPHY errctrl interrupt again, if mipi have receive
		 * the whole frame without any error.
		 */
		if (rkisp1->isp.is_dphy_errctrl_disabled) {
			val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
			val |= RKISP1_CIF_MIPI_ERR_CTRL(0x0f);
			rkisp1_write(rkisp1, val, RKISP1_CIF_MIPI_IMSC);
			rkisp1->isp.is_dphy_errctrl_disabled = false;
		}
	} else {
		rkisp1->debug.mipi_error++;
	}
}

Regards,
Helen

> 

> Best regards,

> Tomasz

> 

>>

>> Thanks,

>> Dafna

>>

>>

>>

>>>

>>>>              curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();

>>>>              curr_buf->vb.field = V4L2_FIELD_NONE;

>>>>              vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);

>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h

>>>> index 232bee92d0eb..51c92a251ea5 100644

>>>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h

>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h

>>>> @@ -131,7 +131,7 @@ struct rkisp1_isp {

>>>>      const struct rkisp1_isp_mbus_info *src_fmt;

>>>>      struct mutex ops_lock; /* serialize the subdevice ops */

>>>>      bool is_dphy_errctrl_disabled;

>>>> -    atomic_t frame_sequence;

>>>> +    __u32 frame_sequence;

>>>

>>> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace

>>> types. For kernel types please just use the plain u32.

>>>

>>> Best regards,

>>> Tomasz

>>>

>
Tomasz Figa Nov. 10, 2020, 10:40 a.m. UTC | #5 
Hi Helen,

On Fri, Nov 6, 2020 at 11:43 PM Helen Koike <helen.koike@collabora.com> wrote:
>

> Hi Tomasz,

>

> (sorry for not had replied this earlier)

>

> On 10/2/20 12:30 PM, Tomasz Figa wrote:

> > On Fri, Oct 2, 2020 at 11:16 AM Dafna Hirschfeld

> > <dafna.hirschfeld@collabora.com> wrote:

> >>

> >> Hi,

> >>

> >> Am 25.09.20 um 22:42 schrieb Tomasz Figa:

> >>> Hi Dafna,

> >>>

> >>> On Tue, Sep 22, 2020 at 01:33:56PM +0200, Dafna Hirschfeld wrote:

> >>>> The isp.frame_sequence is now read only from the irq handlers

> >>>> that are all fired from the same interrupt, so there is not need

> >>>> for atomic operation.

> >>>> In addition, the frame seq incrementation is moved from

> >>>> rkisp1_isp_queue_event_sof to rkisp1_isp_isr to make the code

> >>>> clearer and the incorrect inline comment is removed.

> >>>>

> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

> >>>> Acked-by: Helen Koike <helen.koike@collabora.com>

> >>>>

> >>>> ---

> >>>> changes since v2:

> >>>> add a closing "}" to if condition

> >>>> remove usless inline comment

> >>>> ---

> >>>>   drivers/staging/media/rkisp1/rkisp1-capture.c |  2 +-

> >>>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-

> >>>>   drivers/staging/media/rkisp1/rkisp1-isp.c     | 16 +++++-----------

> >>>>   drivers/staging/media/rkisp1/rkisp1-params.c  |  2 +-

> >>>>   drivers/staging/media/rkisp1/rkisp1-stats.c   |  3 +--

> >>>>   5 files changed, 9 insertions(+), 16 deletions(-)

> >>>>

> >>>

> >>> Thank you for the patch. Please see my comments inline.

> >>>

> >>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c

> >>>> index 0632582a95b4..1c762a369b63 100644

> >>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c

> >>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c

> >>>> @@ -632,7 +632,7 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)

> >>>>      curr_buf = cap->buf.curr;

> >>>>

> >>>>      if (curr_buf) {

> >>>> -            curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);

> >>>> +            curr_buf->vb.sequence = isp->frame_sequence;

> >>>

> >>> I wonder if with higher resolutions, let's say full 5 Mpix, and/or some

> >>> memory-intensive system load, like video encoding and graphics rendering,

> >>> the DMA wouldn't take enough time to have the MI_FRAME interrupt fire after

> >>> the V_START for the next frame.

> >>>

> >>> I recall you did some testing back in time [1], showing that the two are

> >>> interleaved. Do you remember what CAPTURE resolution was it?

> >>

> >> I ran the testing again, I added a patch to allow streaming simultanously from

> >> both pathes: https://gitlab.collabora.com/dafna/linux/-/commit/8df0d15567b27cb88674fbbe33d1906c3c5a91da

> >> Then I ran two tests:

> >> stream simultaneously with 3280x2464 frames from the camera, and then downscaling them to 1920x1080 on selfpath, this is http://ix.io/2zoP

> >> stream simultaneously with 640x480 frames from the camera, and upscaling them to 1920x1080 on the selfpath. this is http://ix.io/2zoR

> >>

> >> the pixelformat for both is 422P.

> >> I don't know how meaningful and useful is to test it on the rockchip-pi4 board, I only use it with a serial console.

> >> The functionality can probably only be tested on the scarlet.

> >

> > Okay, thanks. It looks like there is always plenty of time margin on

> > the hardware side and if the interrupt handling is delayed for a short

> > time and both are handled by the same handler call, it's also going to

> > be handled fine because of rkisp1_capture_isr() being called before

> > rkisp1_isp_isr().

> >

> > By the way, do we need the MIPI interrupts every frame? Perhaps we

> > could enable the RKISP1_CIF_MIPI_ERR_CTRL* interrupts only and then,

> > when we get an error, we disable it and enable

> > RKISP1_CIF_MIPI_FRAME_END, which would then re-enable

> > RKISP1_CIF_MIPI_ERR_CTRL* and disable itself? WDYT?

>

> The driver already do this in a sense, it disables these interrupts on

> the first MIPI error and re-enable them on RKISP1_CIF_MIPI_FRAME_END.


Yes, it disables the ERR interrupts, but doesn't it keep the FRAME_END
interrupt enabled all the time? (At least that seems to be the case in
your traces.) Is it necessary?

Best regards,
Tomasz

>

> Please check:

>

> https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/rkisp1/rkisp1-isp.c#n1069

>

> For convenience:

>

> void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)

> {

>         u32 val, status;

>

>         status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);

>         if (!status)

>                 return;

>

>         rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);

>

>         /*

>          * Disable DPHY errctrl interrupt, because this dphy

>          * erctrl signal is asserted until the next changes

>          * of line state. This time is may be too long and cpu

>          * is hold in this interrupt.

>          */

>         if (status & RKISP1_CIF_MIPI_ERR_CTRL(0x0f)) {

>                 val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);

>                 rkisp1_write(rkisp1, val & ~RKISP1_CIF_MIPI_ERR_CTRL(0x0f),

>                              RKISP1_CIF_MIPI_IMSC);

>                 rkisp1->isp.is_dphy_errctrl_disabled = true;

>         }

>

>         /*

>          * Enable DPHY errctrl interrupt again, if mipi have receive

>          * the whole frame without any error.

>          */

>         if (status == RKISP1_CIF_MIPI_FRAME_END) {

>                 /*

>                  * Enable DPHY errctrl interrupt again, if mipi have receive

>                  * the whole frame without any error.

>                  */

>                 if (rkisp1->isp.is_dphy_errctrl_disabled) {

>                         val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);

>                         val |= RKISP1_CIF_MIPI_ERR_CTRL(0x0f);

>                         rkisp1_write(rkisp1, val, RKISP1_CIF_MIPI_IMSC);

>                         rkisp1->isp.is_dphy_errctrl_disabled = false;

>                 }

>         } else {

>                 rkisp1->debug.mipi_error++;

>         }

> }

>

> Regards,

> Helen

>

> >

> > Best regards,

> > Tomasz

> >

> >>

> >> Thanks,

> >> Dafna

> >>

> >>

> >>

> >>>

> >>>>              curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();

> >>>>              curr_buf->vb.field = V4L2_FIELD_NONE;

> >>>>              vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);

> >>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h

> >>>> index 232bee92d0eb..51c92a251ea5 100644

> >>>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h

> >>>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h

> >>>> @@ -131,7 +131,7 @@ struct rkisp1_isp {

> >>>>      const struct rkisp1_isp_mbus_info *src_fmt;

> >>>>      struct mutex ops_lock; /* serialize the subdevice ops */

> >>>>      bool is_dphy_errctrl_disabled;

> >>>> -    atomic_t frame_sequence;

> >>>> +    __u32 frame_sequence;

> >>>

> >>> nit: The __ prefixed types are defined for the UAPI to avoid covering userspace

> >>> types. For kernel types please just use the plain u32.

> >>>

> >>> Best regards,

> >>> Tomasz

> >>>

> >
diff mbox series

Patch

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 0632582a95b4..1c762a369b63 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -632,7 +632,7 @@  static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
 	curr_buf = cap->buf.curr;
 
 	if (curr_buf) {
-		curr_buf->vb.sequence = atomic_read(&isp->frame_sequence);
+		curr_buf->vb.sequence = isp->frame_sequence;
 		curr_buf->vb.vb2_buf.timestamp = ktime_get_boottime_ns();
 		curr_buf->vb.field = V4L2_FIELD_NONE;
 		vb2_buffer_done(&curr_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 232bee92d0eb..51c92a251ea5 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -131,7 +131,7 @@  struct rkisp1_isp {
 	const struct rkisp1_isp_mbus_info *src_fmt;
 	struct mutex ops_lock; /* serialize the subdevice ops */
 	bool is_dphy_errctrl_disabled;
-	atomic_t frame_sequence;
+	__u32 frame_sequence;
 };
 
 /*
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 02eafea92863..0e71d600dd2b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -940,7 +940,7 @@  static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	if (rkisp1->active_sensor->mbus_type != V4L2_MBUS_CSI2_DPHY)
 		return -EINVAL;
 
-	atomic_set(&rkisp1->isp.frame_sequence, -1);
+	rkisp1->isp.frame_sequence = -1;
 	mutex_lock(&isp->ops_lock);
 	ret = rkisp1_config_cif(rkisp1);
 	if (ret)
@@ -1092,15 +1092,8 @@  static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
 	struct v4l2_event event = {
 		.type = V4L2_EVENT_FRAME_SYNC,
 	};
+	event.u.frame_sync.frame_sequence = isp->frame_sequence;
 
-	/*
-	 * Increment the frame sequence on the vsync signal.
-	 * This will allow applications to detect dropped.
-	 * Note that there is a debugfs counter for dropped
-	 * frames, but using this event is more accurate.
-	 */
-	event.u.frame_sync.frame_sequence =
-		atomic_inc_return(&isp->frame_sequence);
 	v4l2_event_queue(isp->sd.devnode, &event);
 }
 
@@ -1115,9 +1108,10 @@  void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
 
 	/* Vertical sync signal, starting generating new frame */
-	if (status & RKISP1_CIF_ISP_V_START)
+	if (status & RKISP1_CIF_ISP_V_START) {
+		rkisp1->isp.frame_sequence++;
 		rkisp1_isp_queue_event_sof(&rkisp1->isp);
-
+	}
 	if (status & RKISP1_CIF_ISP_PIC_SIZE_ERROR) {
 		/* Clear pic_size_error */
 		isp_err = rkisp1_read(rkisp1, RKISP1_CIF_ISP_ERR);
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 24a49368df22..7aa76f032728 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1220,7 +1220,7 @@  void rkisp1_params_isr(struct rkisp1_device *rkisp1)
 	 * frame_sequence + 1 here to indicate to userspace on which frame these parameters
 	 * are being applied.
 	 */
-	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
+	unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
 	struct rkisp1_params *params = &rkisp1->params;
 
 	spin_lock(&params->config_lock);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 1daab7a318dc..6aa18d970f2b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -307,8 +307,7 @@  rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 {
 	struct rkisp1_stat_buffer *cur_stat_buf;
 	struct rkisp1_buffer *cur_buf = NULL;
-	unsigned int frame_sequence =
-		atomic_read(&stats->rkisp1->isp.frame_sequence);
+	unsigned int frame_sequence = stats->rkisp1->isp.frame_sequence;
 	u64 timestamp = ktime_get_ns();
 
 	/* get one empty buffer */