Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Am 07.08.20 um 18:08 schrieb Dafna Hirschfeld:
Hi
Am 06.08.20 um 14:22 schrieb Tomasz Figa:
On Thu, Aug 6, 2020 at 11:21 AM Dafna Hirschfeld
wrote:
Am 05.08.20 um 23:10 schrieb Dafna Hirschfeld:
Hi
On 22.07.20 17:24, Tomasz Figa wrote:
Hi Dafna,
On Sat, Jul 11, 2020 at 01:04:31PM +0200, Dafna Hirschfeld wrote:
Hi Laurent,
On 16.08.19 02:13, Laurent Pinchart wrote:
Hello Helen,
Thank you for the patch.
On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
[snip]
+static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
+{
+ struct v4l2_event event = {
+ .type = V4L2_EVENT_FRAME_SYNC,
+ .u.frame_sync.frame_sequence =
+ atomic_inc_return(>frm_sync_seq) - 1,
I would move the increment to the caller, hiding it in this function is
error-prone (and if you look at the caller I'm pointing out one possible
error :-)).
In general usage of frm_sync_seq through the driver seems to be very
race-prone. It's read in various IRQ handling functions, all coming from
the same IRQ, so that part is fine (and wouldn't require an atomic
variable), but when read from the buffer queue handlers I really get a
red light flashing in my head. I'll try to investigate more when
reviewing the next patches.
I see that the only place were 'frame_sequence' is read outside of the irq
handlers is in the capture in 'rkisp1_vb2_buf_queue':
/*
* If there's no next buffer assigned, queue this buffer directly
* as the next buffer, and update the memory interface.
*/
if (cap->is_streaming && !cap->buf.next &&
atomic_read(>rkisp1->isp.frame_sequence) == -1) {
cap->buf.next = ispbuf;
rkisp1_set_next_buf(cap);
} else {
list_add_tail(>queue, >buf.queue);
}
This "if" condition seems very specific, a case where we already stream but
v-start was not yet received.
I think it is possible to remove the test
'atomic_read(>rkisp1->isp.frame_sequence) == -1'
from the above condition so that the next buffer is updated in case it is null
not just before the first
v-start signal.
We don't have this special case in the Chrome OS code.
I suppose it would make it possible to resume the capture 1 frame
earlier after a queue underrun, as otherwise the new buffer would be
only programmed after the next frame start interrupt and used for the
next-next frame. However, it's racy, because programming of the buffer
addresses is not atomic and could end up with the hardware using few
plane addresses from the new buffer and few from the dummy buffer.
Given that and also the fact that a queue underrun is a very special
case, where the system was already having problems catching up, I'd just
remove this special case.
[snip]
+void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
+{
+ void __iomem *base = dev->base_addr;
+ unsigned int isp_mis_tmp = 0;
_tmp are never good names :-S
+ unsigned int isp_err = 0;
Neither of these variable need to be initialised to 0.
+
+ /* start edge of v_sync */
+ if (isp_mis & CIF_ISP_V_START) {
+ rkisp1_isp_queue_event_sof(>isp_sdev);
This will increment the frame sequence number. What if the interrupt is
slightly delayed and the next frame starts before we get a change to
copy the sequence number to the buffers (before they will complete
below) ?
Do you mean that we get two sequental v-start signals and then the next
frame-end signal in MI_MIS belongs to the first v-start signal of the two?
How can this be solved? I wonder if any v-start signal has a later signal
that correspond to the same frame so that we can follow it?
Maybe we should have one counter that is incremented on v-start signal,
and another counter that is incremented uppon some other signal?
We're talking about a hard IRQ. I can't imagine the interrupt handler
being delayed for a time close to a full frame interval (~16ms for 60
fps) to trigger such scenario.
+
+ writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
Do you need to clear all interrupt bits individually, can't you write
isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
all in one go ?
+ isp_mis_tmp = readl(base + CIF_ISP_MIS);
+ if (isp_mis_tmp & CIF_ISP_V_START)
+ v4l2_err(>v4l2_dev, "isp icr v_statr err: 0x%x\n",
+ isp_mis_tmp);
This require some explanation. It looks like a naive way to protect
against something, but I think it could trigger under normal
circumstances if IRQ handling is delayed, and wouldn't do much anyway.
Same for the similar constructs below.
+ }
+
+ if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
+ /* Clear pic_size_error */
+ writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
+ isp_err = readl(base + CIF_ISP_ERR);
+ v4l2_err(>v4l2_dev,
+ "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
What does this mean ?
+
Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Hi
Am 06.08.20 um 14:22 schrieb Tomasz Figa:
On Thu, Aug 6, 2020 at 11:21 AM Dafna Hirschfeld
wrote:
Am 05.08.20 um 23:10 schrieb Dafna Hirschfeld:
Hi
On 22.07.20 17:24, Tomasz Figa wrote:
Hi Dafna,
On Sat, Jul 11, 2020 at 01:04:31PM +0200, Dafna Hirschfeld wrote:
Hi Laurent,
On 16.08.19 02:13, Laurent Pinchart wrote:
Hello Helen,
Thank you for the patch.
On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
[snip]
+static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
+{
+struct v4l2_event event = {
+.type = V4L2_EVENT_FRAME_SYNC,
+.u.frame_sync.frame_sequence =
+atomic_inc_return(>frm_sync_seq) - 1,
I would move the increment to the caller, hiding it in this function is
error-prone (and if you look at the caller I'm pointing out one possible
error :-)).
In general usage of frm_sync_seq through the driver seems to be very
race-prone. It's read in various IRQ handling functions, all coming from
the same IRQ, so that part is fine (and wouldn't require an atomic
variable), but when read from the buffer queue handlers I really get a
red light flashing in my head. I'll try to investigate more when
reviewing the next patches.
I see that the only place were 'frame_sequence' is read outside of the irq
handlers is in the capture in 'rkisp1_vb2_buf_queue':
/*
* If there's no next buffer assigned, queue this buffer directly
* as the next buffer, and update the memory interface.
*/
if (cap->is_streaming && !cap->buf.next &&
atomic_read(>rkisp1->isp.frame_sequence) == -1) {
cap->buf.next = ispbuf;
rkisp1_set_next_buf(cap);
} else {
list_add_tail(>queue, >buf.queue);
}
This "if" condition seems very specific, a case where we already stream but
v-start was not yet received.
I think it is possible to remove the test
'atomic_read(>rkisp1->isp.frame_sequence) == -1'
from the above condition so that the next buffer is updated in case it is null
not just before the first
v-start signal.
We don't have this special case in the Chrome OS code.
I suppose it would make it possible to resume the capture 1 frame
earlier after a queue underrun, as otherwise the new buffer would be
only programmed after the next frame start interrupt and used for the
next-next frame. However, it's racy, because programming of the buffer
addresses is not atomic and could end up with the hardware using few
plane addresses from the new buffer and few from the dummy buffer.
Given that and also the fact that a queue underrun is a very special
case, where the system was already having problems catching up, I'd just
remove this special case.
[snip]
+void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
+{
+void __iomem *base = dev->base_addr;
+unsigned int isp_mis_tmp = 0;
_tmp are never good names :-S
+unsigned int isp_err = 0;
Neither of these variable need to be initialised to 0.
+
+/* start edge of v_sync */
+if (isp_mis & CIF_ISP_V_START) {
+rkisp1_isp_queue_event_sof(>isp_sdev);
This will increment the frame sequence number. What if the interrupt is
slightly delayed and the next frame starts before we get a change to
copy the sequence number to the buffers (before they will complete
below) ?
Do you mean that we get two sequental v-start signals and then the next
frame-end signal in MI_MIS belongs to the first v-start signal of the two?
How can this be solved? I wonder if any v-start signal has a later signal
that correspond to the same frame so that we can follow it?
Maybe we should have one counter that is incremented on v-start signal,
and another counter that is incremented uppon some other signal?
We're talking about a hard IRQ. I can't imagine the interrupt handler
being delayed for a time close to a full frame interval (~16ms for 60
fps) to trigger such scenario.
+
+writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
Do you need to clear all interrupt bits individually, can't you write
isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
all in one go ?
+isp_mis_tmp = readl(base + CIF_ISP_MIS);
+if (isp_mis_tmp & CIF_ISP_V_START)
+v4l2_err(>v4l2_dev, "isp icr v_statr err: 0x%x\n",
+ isp_mis_tmp);
This require some explanation. It looks like a naive way to protect
against something, but I think it could trigger under normal
circumstances if IRQ handling is delayed, and wouldn't do much anyway.
Same for the similar constructs below.
+}
+
+if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
+/* Clear pic_size_error */
+writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
+isp_err = readl(base + CIF_ISP_ERR);
+v4l2_err(>v4l2_dev,
+ "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
What does this mean ?
+writel(isp_err, base + CIF_ISP_ERR_CLR);
+
Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Hi,
Am 06.08.20 um 14:08 schrieb Tomasz Figa:
On Wed, Aug 5, 2020 at 11:10 PM Dafna Hirschfeld
wrote:
Hi
On 22.07.20 17:24, Tomasz Figa wrote:
Hi Dafna,
On Sat, Jul 11, 2020 at 01:04:31PM +0200, Dafna Hirschfeld wrote:
Hi Laurent,
On 16.08.19 02:13, Laurent Pinchart wrote:
Hello Helen,
Thank you for the patch.
On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
[snip]
+static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
+{
+ struct v4l2_event event = {
+ .type = V4L2_EVENT_FRAME_SYNC,
+ .u.frame_sync.frame_sequence =
+ atomic_inc_return(>frm_sync_seq) - 1,
I would move the increment to the caller, hiding it in this function is
error-prone (and if you look at the caller I'm pointing out one possible
error :-)).
In general usage of frm_sync_seq through the driver seems to be very
race-prone. It's read in various IRQ handling functions, all coming from
the same IRQ, so that part is fine (and wouldn't require an atomic
variable), but when read from the buffer queue handlers I really get a
red light flashing in my head. I'll try to investigate more when
reviewing the next patches.
I see that the only place were 'frame_sequence' is read outside of the irq
handlers is in the capture in 'rkisp1_vb2_buf_queue':
/*
* If there's no next buffer assigned, queue this buffer directly
* as the next buffer, and update the memory interface.
*/
if (cap->is_streaming && !cap->buf.next &&
atomic_read(>rkisp1->isp.frame_sequence) == -1) {
cap->buf.next = ispbuf;
rkisp1_set_next_buf(cap);
} else {
list_add_tail(>queue, >buf.queue);
}
This "if" condition seems very specific, a case where we already stream but
v-start was not yet received.
I think it is possible to remove the test
'atomic_read(>rkisp1->isp.frame_sequence) == -1'
from the above condition so that the next buffer is updated in case it is null
not just before the first
v-start signal.
We don't have this special case in the Chrome OS code.
I suppose it would make it possible to resume the capture 1 frame
earlier after a queue underrun, as otherwise the new buffer would be
only programmed after the next frame start interrupt and used for the
next-next frame. However, it's racy, because programming of the buffer
addresses is not atomic and could end up with the hardware using few
plane addresses from the new buffer and few from the dummy buffer.
Given that and also the fact that a queue underrun is a very special
case, where the system was already having problems catching up, I'd just
remove this special case.
[snip]
+void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
+{
+ void __iomem *base = dev->base_addr;
+ unsigned int isp_mis_tmp = 0;
_tmp are never good names :-S
+ unsigned int isp_err = 0;
Neither of these variable need to be initialised to 0.
+
+ /* start edge of v_sync */
+ if (isp_mis & CIF_ISP_V_START) {
+ rkisp1_isp_queue_event_sof(>isp_sdev);
This will increment the frame sequence number. What if the interrupt is
slightly delayed and the next frame starts before we get a change to
copy the sequence number to the buffers (before they will complete
below) ?
Do you mean that we get two sequental v-start signals and then the next
frame-end signal in MI_MIS belongs to the first v-start signal of the two?
How can this be solved? I wonder if any v-start signal has a later signal
that correspond to the same frame so that we can follow it?
Maybe we should have one counter that is incremented on v-start signal,
and another counter that is incremented uppon some other signal?
We're talking about a hard IRQ. I can't imagine the interrupt handler
being delayed for a time close to a full frame interval (~16ms for 60
fps) to trigger such scenario.
+
+ writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
Do you need to clear all interrupt bits individually, can't you write
isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
all in one go ?
+ isp_mis_tmp = readl(base + CIF_ISP_MIS);
+ if (isp_mis_tmp & CIF_ISP_V_START)
+ v4l2_err(>v4l2_dev, "isp icr v_statr err: 0x%x\n",
+ isp_mis_tmp);
This require some explanation. It looks like a naive way to protect
against something, but I think it could trigger under normal
circumstances if IRQ handling is delayed, and wouldn't do much anyway.
Same for the similar constructs below.
+ }
+
+ if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
+ /* Clear pic_size_error */
+ writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
+ isp_err = readl(base + CIF_ISP_ERR);
+ v4l2_err(>v4l2_dev,
+ "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
What does this mean ?
+ writel(isp_err, base + CIF_ISP_ERR_CLR);
+ } else if
Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
On Thu, Aug 15, 2019 at 07:29:59PM +0900, Tomasz Figa wrote:
> On Thu, Aug 15, 2019 at 5:25 PM Sakari Ailus
> wrote:
> >
> > Hi Helen,
> >
> > On Wed, Aug 14, 2019 at 09:58:05PM -0300, Helen Koike wrote:
> >
> > ...
> >
> > > >> +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd,
> > > >> + struct v4l2_subdev_pad_config *cfg,
> > > >> + struct v4l2_subdev_format *fmt)
> > > >> +{
> > > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > >> + struct rkisp1_isp_subdev *isp_sd = _dev->isp_sdev;
> > > >> + struct v4l2_mbus_framefmt *mf = >format;
> > > >> +
> > > >
> > > > Note that for sub-device nodes, the driver is itself responsible for
> > > > serialising the access to its data structures.
> > >
> > > But looking at subdev_do_ioctl_lock(), it seems that it serializes the
> > > ioctl calls for subdevs, no? Or I'm misunderstanding something (which is
> > > most probably) ?
> >
> > Good question. I had missed this change --- subdev_do_ioctl_lock() is
> > relatively new. But setting that lock is still not possible as the struct
> > is allocated in the framework and the device is registered before the
> > driver gets hold of it. It's a good idea to provide the same serialisation
> > for subdevs as well.
> >
> > I'll get back to this later.
> >
> > ...
> >
> > > >> +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > > >
> > > > If you support runtime PM, you shouldn't implement the s_power op.
> > >
> > > Is is ok to completly remove the usage of runtime PM then?
> > > Like this http://ix.io/1RJb ?
> >
> > Please use runtime PM instead. In the long run we should get rid of the
> > s_power op. Drivers themselves know better when the hardware they control
> > should be powered on or off.
> >
>
> One also needs to use runtime PM to handle power domains and power
> dependencies on auxiliary devices, e.g. IOMMU.
>
> > >
> > > tbh I'm not that familar with runtime PM and I'm not sure what is the
> > > difference of it and using s_power op (and
> > > Documentation/power/runtime_pm.rst
> > > is not being that helpful tbh).
> >
> > You can find a simple example e.g. in
> > drivers/media/platform/atmel/atmel-isi.c .
> >
> > >
> > > >
> > > > You'll still need to call s_power on external subdevs though.
> > > >
> > > >> +{
> > > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > >> + int ret;
> > > >> +
> > > >> + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n", on);
> > > >> +
> > > >> + if (on) {
> > > >> + ret = pm_runtime_get_sync(isp_dev->dev);
> > >
> > > If this is not ok to remove suport for runtime PM, then where should I put
> > > the call to pm_runtime_get_sync() if not in this s_power op ?
> >
> > Basically the runtime_resume and runtime_suspend callbacks are where the
> > device power state changes are implemented, and pm_runtime_get_sync and
> > pm_runtime_put are how the driver controls the power state.
> >
> > So you no longer need the s_power() op at all. The op needs to be called on
> > the pipeline however, as there are drivers that still use it.
> >
>
> For this driver, I suppose we would _get_sync() when we start
> streaming (in the hardware, i.e. we want the ISP to start capturing
> frames) and _put() when we stop and the driver shouldn't perform any
> access to the hardware when the streaming is not active.
Agreed.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
On Thu, Aug 15, 2019 at 5:25 PM Sakari Ailus
wrote:
>
> Hi Helen,
>
> On Wed, Aug 14, 2019 at 09:58:05PM -0300, Helen Koike wrote:
>
> ...
>
> > >> +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd,
> > >> + struct v4l2_subdev_pad_config *cfg,
> > >> + struct v4l2_subdev_format *fmt)
> > >> +{
> > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > >> + struct rkisp1_isp_subdev *isp_sd = _dev->isp_sdev;
> > >> + struct v4l2_mbus_framefmt *mf = >format;
> > >> +
> > >
> > > Note that for sub-device nodes, the driver is itself responsible for
> > > serialising the access to its data structures.
> >
> > But looking at subdev_do_ioctl_lock(), it seems that it serializes the
> > ioctl calls for subdevs, no? Or I'm misunderstanding something (which is
> > most probably) ?
>
> Good question. I had missed this change --- subdev_do_ioctl_lock() is
> relatively new. But setting that lock is still not possible as the struct
> is allocated in the framework and the device is registered before the
> driver gets hold of it. It's a good idea to provide the same serialisation
> for subdevs as well.
>
> I'll get back to this later.
>
> ...
>
> > >> +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > >
> > > If you support runtime PM, you shouldn't implement the s_power op.
> >
> > Is is ok to completly remove the usage of runtime PM then?
> > Like this http://ix.io/1RJb ?
>
> Please use runtime PM instead. In the long run we should get rid of the
> s_power op. Drivers themselves know better when the hardware they control
> should be powered on or off.
>
One also needs to use runtime PM to handle power domains and power
dependencies on auxiliary devices, e.g. IOMMU.
> >
> > tbh I'm not that familar with runtime PM and I'm not sure what is the
> > difference of it and using s_power op (and
> > Documentation/power/runtime_pm.rst
> > is not being that helpful tbh).
>
> You can find a simple example e.g. in
> drivers/media/platform/atmel/atmel-isi.c .
>
> >
> > >
> > > You'll still need to call s_power on external subdevs though.
> > >
> > >> +{
> > >> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > >> + int ret;
> > >> +
> > >> + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n", on);
> > >> +
> > >> + if (on) {
> > >> + ret = pm_runtime_get_sync(isp_dev->dev);
> >
> > If this is not ok to remove suport for runtime PM, then where should I put
> > the call to pm_runtime_get_sync() if not in this s_power op ?
>
> Basically the runtime_resume and runtime_suspend callbacks are where the
> device power state changes are implemented, and pm_runtime_get_sync and
> pm_runtime_put are how the driver controls the power state.
>
> So you no longer need the s_power() op at all. The op needs to be called on
> the pipeline however, as there are drivers that still use it.
>
For this driver, I suppose we would _get_sync() when we start
streaming (in the hardware, i.e. we want the ISP to start capturing
frames) and _put() when we stop and the driver shouldn't perform any
access to the hardware when the streaming is not active.
Best regards,
Tomasz
