Message ID | 20200922113402.12442-7-dafna.hirschfeld@collabora.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: staging: rkisp1: various bug fixes | expand |
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
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 >
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 > >
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 >>> >
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 --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(¶ms->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 */