Hi Luca,
On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
> On 19/04/20 20:02, Laurent Pinchart wrote:
> [...]
> >> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> >> +{
> >> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> >> + struct xcsi2rxss_core *core = &state->core;
> >> + u32 status;
> >> +
> >> + status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
> >> + dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
> >
> > As this is expected to occur for every frame, I would drop the message,
> > even if rate-limited.
> >
> >> +
> >> + if (!status)
> >> + return IRQ_NONE;
> >> +
> >> + /* Received a short packet */
> >> + if (status & XCSI_ISR_SPFIFONE) {
> >> + dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
> >> + xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
> >> + }
> >
> > Same here, this will occur all the time, I'd remove this message. You
> > need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
> > until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
> > multiple short packets are received before the interrupt handler
> > executes.
> >
> > I also wonder if it would make sense to extract the frame number from
> > the FS short packet, and make it available through the subdev API. I
> > think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
> > can be implemented later.
> >
> >> +
> >> + /* Short packet FIFO overflow */
> >> + if (status & XCSI_ISR_SPFIFOF)
> >> + dev_dbg_ratelimited(core->dev, "Short packet FIFO overflowed\n");
> >> +
> >> + /*
> >> + * Stream line buffer full
> >> + * This means there is a backpressure from downstream IP
> >> + */
> >> + if (status & XCSI_ISR_SLBF) {
> >> + dev_alert_ratelimited(core->dev, "Stream Line Buffer Full!\n");
> >> + xcsi2rxss_stop_stream(state);
> >> + if (core->rst_gpio) {
> >> + gpiod_set_value(core->rst_gpio, 1);
> >> + /* minimum 40 dphy_clk_200M cycles */
> >> + ndelay(250);
> >> + gpiod_set_value(core->rst_gpio, 0);
> >> + }
> >
> > I don't think you should stop the core here. xcsi2rxss_stop_stream()
> > calls the source .s_stream(0) operation, which usually involves I2C
> > writes that will sleep.
> >
> > You should instead report an event to userspace (it looks like we have
> > no error event defined in V4L2, one should be added), and rely on the
> > normal stop procedure.
>
> FWIW, since a long time I've been using a modified version of this
> routine, where after a Stream Line Buffer Full condition I just stop and
> restart the csi2rx core and the stream continues after a minimal glitch.
> Other subdev are unaware that anything has happened and keep on streaming.
>
> Not sure this is the correct thing to do, but it's working for me. Also
> I proposed this topic in one of the previous iterations of this patch,
> but the situation was different because the stream on/off was not
> propagated back at that time.
Thanks for the feedback. How often does this occur in practice ?
--
Regards,
Laurent Pinchart
