Re: [PATCH v3 2/3] media: rockchip: Introduce driver for Rockhip's camera interface

[Ezequiel Garcia]

Hi Maxime,

On Mon, 23 Nov 2020 at 04:21, Maxime Chevallier
<maxime.chevallier@xxxxxxxxxxx> wrote:
>
> Hi Ezequiel, and thanks a lot for the review !
>
> On Fri, 2 Oct 2020 14:35:28 -0300
> Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > Hi Maxime,
> >
> >Thanks to Dafna, I found the patch ^_^
> >
> >The driver looks real good. Just a few comments below.
> >
> >Is the driver passing latest v4l2-compliance tests?
>
> I'll post them along with the next iteration of the series.
>
> >> +config VIDEO_ROCKCHIP_VIP
> >> +       tristate "Rockchip VIP (Video InPut) Camera Interface"
> >> +       depends on VIDEO_DEV && VIDEO_V4L2
> >> +       depends on ARCH_ROCKCHIP || COMPILE_TEST
> >> +       select VIDEOBUF2_DMA_SG
> >> +       select VIDEOBUF2_DMA_CONTIG
> >> +       select V4L2_FWNODE
> >> +       select V4L2_MEM2MEM_DEV
> >> +       help
> >> +         This is a v4l2 driver for Rockchip SOC Camera interface.
> >> +
> >> +         To compile this driver as a module choose m here.
> >> +
> >
> >Please add ... "the module will be called {the name}".
>
> Sure, I will do !
>
> [...]
>
> >> +#define VIP_REQ_BUFS_MIN       1
> >
> >I think you might want to have more than 1 buffer
> >as minimum. How about 3? Two for the ping and pong,
> >and one more in the queue.
>
> Yes you're correct, 3 should be the strict minimum required buffers
> here, I didn't update that after adding the dual-buffering mode.
>
> >> +#define VIP_MIN_WIDTH          64
> >> +#define VIP_MIN_HEIGHT         64
> >> +#define VIP_MAX_WIDTH          8192
> >> +#define VIP_MAX_HEIGHT         8192
> >> +
> >> +#define RK_VIP_PLANE_Y                 0
> >> +#define RK_VIP_PLANE_CBCR              1
> >> +
> >> +#define VIP_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
> >> +/* Check if swap y and c in bt1120 mode */
> >> +#define VIP_FETCH_IS_Y_FIRST(VAL) ((VAL) & 0xf)
> >> +
> >> +/* Get xsubs and ysubs for fourcc formats
> >> + *
> >> + * @xsubs: horizontal color samples in a 4*4 matrix, for yuv
> >> + * @ysubs: vertical color samples in a 4*4 matrix, for yuv
> >> + */
> >> +static int fcc_xysubs(u32 fcc, u32 *xsubs, u32 *ysubs)
> >
> >See below, you should be using v4l2_fill_pixfmt_mp.
> >
> >> +{
> >> +       switch (fcc) {
> >> +       case V4L2_PIX_FMT_NV16:
> >> +       case V4L2_PIX_FMT_NV61:
> >> +               *xsubs = 2;
> >> +               *ysubs = 1;
> >> +               break;
> >> +       case V4L2_PIX_FMT_NV21:
> >> +       case V4L2_PIX_FMT_NV12:
> >> +               *xsubs = 2;
> >> +               *ysubs = 2;
> >> +               break;
> >> +       default:
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static const struct vip_output_fmt out_fmts[] = {
> >> +       {
> >> +               .fourcc = V4L2_PIX_FMT_NV16,
> >> +               .cplanes = 2,
> >
> >From what I can see, you are only using this
> >information to calculate bytesperline and sizeimage,
> >so you should be using the v4l2_fill_pixfmt_mp() helper.
>
> You're correct, it indeed makes things much easier and allowed to
> removed a lot of redundant info here !
>
>
> >> +static void rk_vip_set_fmt(struct rk_vip_stream *stream,
> >> +                          struct v4l2_pix_format_mplane *pixm,
> >> +                          bool try)
> >> +{
> >> +       struct rk_vip_device *dev = stream->vipdev;
> >> +       struct v4l2_subdev_format sd_fmt;
> >> +       const struct vip_output_fmt *fmt;
> >> +       struct v4l2_rect input_rect;
> >> +       unsigned int planes, imagesize = 0;
> >> +       u32 xsubs = 1, ysubs = 1;
> >> +       int i;
> >> +
> >
> >I was expecting to see some is_busy or is_streaming check
> >here, have you tested what happens if you change the format
> >while streaming, or after buffers are queued?
>
> Yes correct. I used the stream->state private flag here, but I it was
> also brought to my attention that there also exists a vb2_is_busy()
> helper, but I'm unsure if it would be correct to use it here.
>

Long story, short: when the application creates buffers,
with e.g. REQBUF (see vb2_core_reqbufs), it will call
the driver (vb2_ops.queue_setup), to get the planes' sizes.

In the current model, for a given vb2 queue, all the buffers
are the same size. In practice, the simpler way to express
this is not allowing S_FMT if there are buffers allocated
in the queue (vb2_is_busy).

You could relax the vb2_is_busy requirement in your driver,
but I usually find it's not worth the trouble.

>
> >> +
> >> +static int rk_vip_g_input(struct file *file, void *fh, unsigned int *i)
> >> +{
> >> +       *i = 0;
> >> +       return 0;
> >> +}
> >> +
> >> +static int rk_vip_s_input(struct file *file, void *fh, unsigned int i)
> >> +{
> >
> >Only one input, why do you need to support this ioctl at all?
>
> I actually saw a fair amount of existing drivers implementing these
> callbacks even for only one input, so I don't really know if I should
> remove it or not ?
>

S_INPUT is used e.g. on capture devices that have multiple
inputs and can capture from one input at a time.

If the ioctl is empty like this, the driver can simply not support
the ioctl.

Best regards,
Ezequiel