Re: [EXT] Re: [PATCH v13 08/13] media: amphion: add v4l2 m2m vpu decoder stateful driver

[Nicolas Dufresne]

Le vendredi 03 décembre 2021 à 06:01 +0000, Ming Qian a écrit :
> > -----Original Message-----
> > From: Ming Qian
> > Sent: Friday, December 3, 2021 1:43 PM
> > To: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> > shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx
> > Cc: hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Subject: RE: [EXT] Re: [PATCH v13 08/13] media: amphion: add v4l2 m2m vpu
> > decoder stateful driver
> > 
> > > -----Original Message-----
> > > From: Nicolas Dufresne [mailto:nicolas@xxxxxxxxxxxx]
> > > Sent: Friday, December 3, 2021 12:56 PM
> > > To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
> > > shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx
> > > Cc: hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> > > festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong
> > > <aisheng.dong@xxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > Subject: [EXT] Re: [PATCH v13 08/13] media: amphion: add v4l2 m2m vpu
> > > decoder stateful driver
> > > 
> > > Caution: EXT Email
> > > 
> > > Le mardi 30 novembre 2021 à 17:48 +0800, Ming Qian a écrit :
> > > > This consists of video decoder implementation plus decoder controls.
> > > > 
> > > > Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> > > > Signed-off-by: Shijie Qin <shijie.qin@xxxxxxx>
> > > > Signed-off-by: Zhou Peng <eagle.zhou@xxxxxxx>
> > > > ---
> > > >  drivers/media/platform/amphion/vdec.c | 1680
> > > +++++++++++++++++++++++++
> > 
> > 
> > > > +
> > > > +static void vdec_init_fmt(struct vpu_inst *inst) {
> > > > +     struct vdec_t *vdec = inst->priv;
> > > > +     const struct vpu_format *fmt;
> > > > +     int i;
> > > > +
> > > > +     fmt = vpu_helper_find_format(inst, inst->cap_format.type,
> > > vdec->codec_info.pixfmt);
> > > > +     inst->out_format.width = vdec->codec_info.width;
> > > > +     inst->out_format.height = vdec->codec_info.height;
> > > > +     inst->cap_format.width = vdec->codec_info.decoded_width;
> > > > +     inst->cap_format.height = vdec->codec_info.decoded_height;
> > > > +     inst->cap_format.pixfmt = vdec->codec_info.pixfmt;
> > > > +     if (fmt) {
> > > > +             inst->cap_format.num_planes = fmt->num_planes;
> > > > +             inst->cap_format.flags = fmt->flags;
> > > > +     }
> > > > +     for (i = 0; i < inst->cap_format.num_planes; i++) {
> > > > +             inst->cap_format.bytesperline[i] =
> > > vdec->codec_info.bytesperline[i];
> > > > +             inst->cap_format.sizeimage[i] =
> > > vdec->codec_info.sizeimage[i];
> > > > +     }
> > > > +     if (vdec->codec_info.progressive)
> > > > +             inst->cap_format.field = V4L2_FIELD_NONE;
> > > > +     else
> > > > +             inst->cap_format.field = V4L2_FIELD_INTERLACED;
> > > 
> > > As a followup, this should be conditional to the chosen pixel format.
> > > If I understood correct, you produce interlaced is only produce for
> > > linear NV12, for tiled the fields are outputed seperated in their
> > > respective v4l2_buffer. Note sure where yet, but the V4L2 spec
> > > requires you to pair the fields by using the same seq_num on both.
> > 
> > The amphion vpu will store the two fields into one v4l2_buf, So I'll change
> > V4L2_FIELD_INTERLACED to V4L2_FIELD_SEQ_TB
> > 
> 
> Hi Nicolas,
>     Seems gstreamer doesn't support V4L2_FIELD_SEQ_TB yet.
> 
>   switch (fmt.fmt.pix.field) {
>     case V4L2_FIELD_ANY:
>     case V4L2_FIELD_NONE:
>       interlace_mode = GST_VIDEO_INTERLACE_MODE_PROGRESSIVE;
>       break;
>     case V4L2_FIELD_INTERLACED:
>     case V4L2_FIELD_INTERLACED_TB:
>     case V4L2_FIELD_INTERLACED_BT:
>       interlace_mode = GST_VIDEO_INTERLACE_MODE_INTERLEAVED;
>       break;
>     case V4L2_FIELD_ALTERNATE:
>       interlace_mode = GST_VIDEO_INTERLACE_MODE_ALTERNATE;
>       break;
>     default:
>       goto unsupported_field;
>   }

This is correct, I had never had the chance to implement it. So far I only know
IMX6 camera pipeline producing that, but rarely used in practice. What matters
here is that your driver does report the right information so that userspace
don't get fooled into thinking it's interleaved.

> 
> > > 
> > > > +     if (vdec->codec_info.color_primaries ==
> > V4L2_COLORSPACE_DEFAULT)
> > > > +             vdec->codec_info.color_primaries =
> > > V4L2_COLORSPACE_REC709;
> > > > +     if (vdec->codec_info.transfer_chars == V4L2_XFER_FUNC_DEFAULT)
> > > > +             vdec->codec_info.transfer_chars = V4L2_XFER_FUNC_709;
> > > > +     if (vdec->codec_info.matrix_coeffs == V4L2_YCBCR_ENC_DEFAULT)
> > > > +             vdec->codec_info.matrix_coeffs = V4L2_YCBCR_ENC_709;
> > > > +     if (vdec->codec_info.full_range == V4L2_QUANTIZATION_DEFAULT)
> > > > +             vdec->codec_info.full_range =
> > > V4L2_QUANTIZATION_LIM_RANGE;
> > > > +}
> > > > +