[PATCH v2 2/2] media: imx: vdic: Introduce mem2mem VDI deinterlacer driver

[Marek Vasut]

On 9/27/24 9:33 PM, Nicolas Dufresne wrote:
> 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).

OK, fixed in V3.

> [...]
> 
>>>
>>> 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.

This is a workaround so that older gstreamer versions would work, what 
else can I do here ?

>>> 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.

It seems v4l2convert disable-passthrough=true works with deinterlacers 
just fine , except for this one reused frame at stream start ?

> 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.

As far as I understand this, when the EOF interrupt happens, 
v4l2_m2m_src_buf_remove() pulls the oldest input buffer from the queue 
and that buffer is then marked as DONE (or ERROR in v3), that is the 
->prev buffer, isn't it ?

Once the next frame deinterlacing starts, the (new) current frame and 
the prev frame are both active, the deinterlacing happens and then in 
the EOF interrupt, the ->prev frame gets marked as DONE again.

What am I missing here ?

> [...]
> 
>>>> +
>>>> +	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.

Do you want me to introduce some new generic helper ? Because I don't 
see an existing generic one.

>>>> +	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).
The input is from the VDI and that always has to be YUV. Can you maybe 
just CC me on the CSC-scaler patches ? Then I'll see what can be done here.

Thanks