Le mercredi 25 septembre 2024 à 22:45 +0200, Marek Vasut a écrit :
> On 9/25/24 7:58 PM, Nicolas Dufresne wrote:
>
>
[...]
>
> > > +static irqreturn_t ipu_mem2mem_vdic_nfb4eof_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct ipu_mem2mem_vdic_priv *priv = dev_id;
> > > +
> > > + /* That is about all we can do about it, report it. */
> > > + dev_warn_ratelimited(priv->dev, "NFB4EOF error interrupt occurred\n");
> >
> > Not sure this is right. If that means ipu_mem2mem_vdic_eof_interrupt won't fire,
> > then it means streamoff/close after that will hang forever, leaving a zombie
> > process behind.
> >
> > Perhaps mark the buffers as ERROR, and finish the job.
>
> The NFB4EOF interrupt is generated when the VDIC didn't write (all of)
> output frame . I think it stands for "New Frame Before EOF" or some
> such. Basically the currently written frame will be corrupted and the
> next frame(s) are likely going to be OK again.
So the other IRQ will be triggered ? After this one ? Is so, perhaps take a
moment to mark the frames as ERROR (which means corrupted).
[...]
> >
> > The driver is not taking ownership of prev_buf, only curr_buf is guaranteed to
> > exist until v4l2_m2m_job_finish() is called. Usespace could streamoff, allocate
> > new buffers, and then an old freed buffer may endup being used.
>
> So, what should I do about this ? Is there some way to ref the buffer to
> keep it around ?
>
> > Its also unclear to me how userspace can avoid this ugly warning, how can you
> > have curr_buf set the first time ? (I might be missing something you this one
> > though).
>
> The warning happens when streaming starts and there is only one input
> frame available for the VDIC, which needs three fields to work
> correctly. So, if there in only one input frame, VDI uses the input
> frame bottom field as PREV field for the prediction, and input frame top
> and bottom fields as CURR and NEXT fields for the prediction, the result
> may be one sub-optimal deinterlaced output frame (the first one). Once
> another input frame gets enqueued, the VDIC uses the previous frame
> bottom field as PREV and the newly enqueued frame top and bottom fields
> as CURR and NEXT and the prediction works correctly from that point on.
Warnings by default are not acceptable.
>
> > Perhaps what you want is a custom job_ready() callback, that ensure you have 2
> > buffers in the OUTPUT queue ? You also need to ajust the CID
> > MIN_BUFFERS_FOR_OUTPUT accordingly.
>
> I had that before, but gstreamer didn't enqueue the two frames for me,
> so I got back to this variant for maximum compatibility.
Its well known that GStreamer v4l2convert element have no support for
detinterlacing and need to be improved to support any deinterlace drivers out
there.
Other drivers will simply holds on output buffers until it has enough to produce
the first valid picture. Holding meaning not marking them done, which keeps then
in the ACTIVE state, which is being tracked by the core for your.
[...]
> > > +
> > > + if (ipu_mem2mem_vdic_format_is_yuv420(f->fmt.pix.pixelformat))
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width * 3 / 2;
> > > + else if (ipu_mem2mem_vdic_format_is_yuv422(f->fmt.pix.pixelformat))
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width * 2;
> > > + else if (ipu_mem2mem_vdic_format_is_rgb16(f->fmt.pix.pixelformat))
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width * 2;
> > > + else if (ipu_mem2mem_vdic_format_is_rgb24(f->fmt.pix.pixelformat))
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width * 3;
> > > + else if (ipu_mem2mem_vdic_format_is_rgb32(f->fmt.pix.pixelformat))
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width * 4;
> > > + else
> > > + f->fmt.pix.bytesperline = f->fmt.pix.width;
> > > +
> > > + f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
> >
> > And use v4l2-common ?
>
> I don't really understand, there is nothing in v4l2-common.c that would
> be really useful replacement for this ?
Not sure I get your response, v4l2-common is used in many drivers already, and
we intent to keep improving it so that all driver uses it in the long term. It
been created because folks believed they can calculate bytesperline and
sizeimage, but as the number of format grows, it always endup wrong, causing the
HW to overflow and break the system at a larger scale.
>
> > > + return 0;
> > > +}
> > > +
> > > +static int ipu_mem2mem_vdic_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
> > > +{
> > > + struct ipu_mem2mem_vdic_ctx *ctx = fh_to_ctx(fh);
> > > + struct ipu_mem2mem_vdic_priv *priv = ctx->priv;
> > > + struct v4l2_pix_format *fmt, *infmt, *outfmt;
> > > + struct vb2_queue *vq;
> > > + int ret;
> > > +
> > > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > + if (vb2_is_busy(vq)) {
> > > + dev_err(priv->dev, "%s queue busy\n", __func__);
> > > + return -EBUSY;
> > > + }
> > > +
> > > + ret = ipu_mem2mem_vdic_try_fmt(file, fh, f);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + fmt = ipu_mem2mem_vdic_get_format(priv, f->type);
> > > + *fmt = f->fmt.pix;
> > > +
> > > + /* Propagate colorimetry to the capture queue */
> > > + infmt = ipu_mem2mem_vdic_get_format(priv, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > > + outfmt = ipu_mem2mem_vdic_get_format(priv, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > + outfmt->colorspace = infmt->colorspace;
> > > + outfmt->ycbcr_enc = infmt->ycbcr_enc;
> > > + outfmt->xfer_func = infmt->xfer_func;
> > > + outfmt->quantization = infmt->quantization;
> >
> > So you can do CSC conversion but not colorimetry ? We have
> > V4L2_PIX_FMT_FLAG_SET_CSC if you can do colorimetry transforms too. I have
> > patches that I'll send for the csc-scaler driver.
>
> See ipu_ic_calc_csc() , that's what does the colorspace conversion in
> this driver (on output from VDI).
int ipu_ic_calc_csc(struct ipu_ic_csc *csc,
enum v4l2_ycbcr_encoding in_enc,
enum v4l2_quantization in_quant,
enum ipu_color_space in_cs,
enum v4l2_ycbcr_encoding out_enc,
enum v4l2_quantization out_quant,
enum ipu_color_space out_cs)
So instead of simply overriding CSC like you do, let userspace set different CSC
in and out, so that IPU can handle the conversion properly with correct colors.
That requires to flag these in the fmt_desc structure during enum format, and to
only read acknowledge the CSC if userspace have set V4L2_PIX_FMT_FLAG_SET_CSC,
in other condition, the information must be ignored (which you don't).
Nicolas
