Re: [PATCH v2 2/2] rcar-vin: Add support for V4L2_FIELD_SEQ_{TB,BT}

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Jacopo,

Thanks for your feedback.

On 2019-11-12 12:53:11 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Oct 10, 2019 at 12:27:34AM +0200, Niklas Söderlund wrote:
> > The hardware does not support capturing the field types
> > V4L2_FIELD_SEQ_TB and V4L2_FIELD_SEQ_BT. To capture in these formats the
> > driver needs to adjust the offset of the capture buffer and capture
> > twice to each vb2 buffer.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 57 ++++++++++++++++++---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  7 ++-
> >  drivers/media/platform/rcar-vin/rcar-vin.h  | 19 +++++++
> >  3 files changed, 74 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index a9fffadc268e96ba..c46f6e90627d45fd 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -534,7 +534,7 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> >
> >  	/* Set scaling coefficient */
> >  	crop_height = vin->crop.height;
> > -	if (V4L2_FIELD_IS_INTERLACED(vin->format.field))
> > +	if (V4L2_FIELD_HAS_BOTH(vin->format.field))
> >  		crop_height *= 2;
> 
> Below in this same function there is a check for IS_INTERLACED which
> has not been updated. It seems to be related to the post-clipping
> image height, which I'm not sure happens on the full frame or on the
> filed first. Could you confirm this was intentionally left out ?

Nice catch, that too should be updated to V4L2_FIELD_HAS_BOTH().

> 
> >
> >  	ys = 0;
> > @@ -625,6 +625,8 @@ static int rvin_setup(struct rvin_dev *vin)
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> >  		break;
> > +	case V4L2_FIELD_SEQ_TB:
> > +	case V4L2_FIELD_SEQ_BT:
> 
> Shouldn't you handle FOC depending on TB or BT ?

FOC only matters if we use the VIN to interlace the two fields. As VIN 
have no hardware support for BT/TB there is not much we can do with FOC.

> 
> >  	case V4L2_FIELD_NONE:
> >  		vnmc = VNMC_IM_ODD_EVEN;
> >  		progressive = true;
> > @@ -839,15 +841,23 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> >  	struct rvin_buffer *buf;
> >  	struct vb2_v4l2_buffer *vbuf;
> >  	dma_addr_t phys_addr;
> > +	int prev;
> >
> >  	/* A already populated slot shall never be overwritten. */
> >  	if (WARN_ON(vin->buf_hw[slot].buffer != NULL))
> >  		return;
> >
> > -	vin_dbg(vin, "Filling HW slot: %d\n", slot);
> > +	prev = (slot == 0 ? HW_BUFFER_NUM : slot) - 1;
> >
> > -	if (list_empty(&vin->buf_list)) {
> > +	if (vin->buf_hw[prev].type == HALF_TOP) {
> > +		vbuf = vin->buf_hw[prev].buffer;
> > +		vin->buf_hw[slot].buffer = vbuf;
> > +		vin->buf_hw[slot].type = HALF_BOTTOM;
> > +		phys_addr = vin->buf_hw[prev].phys + vin->format.sizeimage /
> > +			(vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 4 : 2);
> 
> Shouldn't NV12 be handled as well ? in that case the sizeimage field
> is set to bytesperlin * height * 3 / 2, and should here be halved
> accordingly, doesn't it ?

You are right NV12 support have now landed upstream and should be 
handled here. But the halving of the sizeimage is just to find the start 
of the Y plane and not to undo the sizeimae callculation. Here the goal 
is to have the first half of the frame filled with Y plane (from two
captures to the same buffer) data and the second half the UV plane. Do 
do that we need to increase the phys offset + 1/4 while keeping the 
offset for the UV plane at 1/2 of output sizeimage.

> 
> 
> > +	} else if (list_empty(&vin->buf_list)) {
> >  		vin->buf_hw[slot].buffer = NULL;
> > +		vin->buf_hw[slot].type = FULL;
> >  		phys_addr = vin->scratch_phys;
> >  	} else {
> >  		/* Keep track of buffer we give to HW */
> > @@ -856,10 +866,18 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
> >  		list_del_init(to_buf_list(vbuf));
> >  		vin->buf_hw[slot].buffer = vbuf;
> >
> > +		vin->buf_hw[slot].type =
> > +			V4L2_FIELD_IS_SEQUENTIAL(vin->format.field) ?
> > +			HALF_TOP : FULL;
> > +
> >  		/* Setup DMA */
> >  		phys_addr = vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> >  	}
> >
> > +	vin_dbg(vin, "Filling HW slot: %d type: %d buffer: %p\n",
> > +		slot, vin->buf_hw[slot].type, vin->buf_hw[slot].buffer);
> > +
> > +	vin->buf_hw[slot].phys = phys_addr;
> >  	rvin_set_slot_addr(vin, slot, phys_addr);
> >  }
> >
> > @@ -867,6 +885,11 @@ static int rvin_capture_start(struct rvin_dev *vin)
> >  {
> >  	int slot, ret;
> >
> > +	for (slot = 0; slot < HW_BUFFER_NUM; slot++) {
> > +		vin->buf_hw[slot].buffer = NULL;
> > +		vin->buf_hw[slot].type = FULL;
> > +	}
> > +
> >  	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
> >  		rvin_fill_hw_slot(vin, slot);
> >
> > @@ -951,6 +974,16 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >
> >  	/* Capture frame */
> >  	if (vin->buf_hw[slot].buffer) {
> > +		/*
> > +		 * Nothing to do but refill the hardware slot if
> > +		 * capture only filled first half of vb2 buffer.
> > +		 */
> > +		if (vin->buf_hw[slot].type == HALF_TOP) {
> > +			vin->buf_hw[slot].buffer = NULL;
> > +			rvin_fill_hw_slot(vin, slot);
> > +			goto done;
> > +		}
> > +
> >  		vin->buf_hw[slot].buffer->field =
> >  			rvin_get_active_field(vin, vnms);
> >  		vin->buf_hw[slot].buffer->sequence = vin->sequence;
> > @@ -978,14 +1011,22 @@ static void return_all_buffers(struct rvin_dev *vin,
> >  			       enum vb2_buffer_state state)
> >  {
> >  	struct rvin_buffer *buf, *node;
> > -	int i;
> > +	struct vb2_v4l2_buffer *freed[HW_BUFFER_NUM];
> > +	unsigned int i, n;
> >
> >  	for (i = 0; i < HW_BUFFER_NUM; i++) {
> > -		if (vin->buf_hw[i].buffer) {
> > -			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
> > -					state);
> > -			vin->buf_hw[i].buffer = NULL;
> > +		freed[i] = vin->buf_hw[i].buffer;
> > +		vin->buf_hw[i].buffer = NULL;
> > +
> > +		for (n = 0; n < i; n++) {
> > +			if (freed[i] == freed[n]) {
> > +				freed[i] = NULL;
> > +				break;
> > +			}
> 
> I'm not sure I get this...

The same buffer might exists in the vin->buf_hw array (once as top and 
once as bottom). We only wish to call vb2_buffer_done() once for each so 
we need this little dance to make sure we only call it once for each vb2 
buffer.

> 
> >  		}
> > +
> > +		if (freed[i])
> > +			vb2_buffer_done(&freed[i]->vb2_buf, state);
> >  	}
> >
> >  	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 9a9b89c0dc0b3be4..cbdb0a43aa439830 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -90,7 +90,10 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
> >  	if (WARN_ON(!fmt))
> >  		return -EINVAL;
> >
> > -	align = pix->pixelformat == V4L2_PIX_FMT_NV16 ? 0x20 : 0x10;
> > +	if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > +		align = 0x80;
> > +	else
> > +		align = pix->pixelformat == V4L2_PIX_FMT_NV16 ? 0x20 : 0x10;
> >
> >  	return ALIGN(pix->width, align) * fmt->bpp;
> >  }
> > @@ -118,6 +121,8 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  	case V4L2_FIELD_INTERLACED:
> >  	case V4L2_FIELD_ALTERNATE:
> > +	case V4L2_FIELD_SEQ_TB:
> > +	case V4L2_FIELD_SEQ_BT:
> >  		break;
> >  	default:
> >  		pix->field = RVIN_DEFAULT_FIELD;
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 9031fe7f569b908e..8d48894bc49e4db6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -60,6 +60,23 @@ enum rvin_dma_state {
> >  	STOPPING,
> >  };
> >
> > +/**
> > + * enum rvin_buffer_type
> > + *
> > + * Describes how a buffer is given to the hardware. To be able
> > + * to capture SEQ_TB/BT it's needed to capture to the same vb2
> > + * buffer twice so the type of buffer needs to be kept.
> > + *
> > + * FULL - One capture fills the whole vb2 buffer
> > + * HALF_TOP- One capture fills the top half of the vb2 buffer
> 
> nit: HALF_TOP -

Thanks.

> 
> > + * HALF_BOTTOM - One capture fills the bottom half of the vb2 buffer
> 
> Isn't documentation generated for VIN ? I was about to suggest to
> prefix the enum members documentation with @ but I don't see any VIN
> documentation...

It's on my todo list to look into this.

> 
> Thanks
>    j
> 
> 
> > + */
> > +enum rvin_buffer_type {
> > +	FULL,
> > +	HALF_TOP,
> > +	HALF_BOTTOM,
> > +};
> > +
> >  /**
> >   * struct rvin_video_format - Data format stored in memory
> >   * @fourcc:	Pixelformat
> > @@ -204,6 +221,8 @@ struct rvin_dev {
> >  	spinlock_t qlock;
> >  	struct {
> >  		struct vb2_v4l2_buffer *buffer;
> > +		enum rvin_buffer_type type;
> > +		dma_addr_t phys;
> >  	} buf_hw[HW_BUFFER_NUM];
> >  	struct list_head buf_list;
> >  	unsigned int sequence;
> > --
> > 2.23.0
> >



-- 
Regards,
Niklas Söderlund