Re: [PATCH v2 2/3] rcar-vin: add support for UDS (Up Down Scaler)

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:04:01 EEST Niklas Söderlund wrote:
> Some VIN instances have access to a Up Down Scaler (UDS). The UDS are on
> most SoCs shared between two VINs, the UDS can of course only be used by
> one VIN at a time.

I would drop the "of course" as it's not so evident.

s/the UDS can of course only/but a UDS can only/

> Add support to configure the UDS registers which are mapped to both VINs
> sharing the UDS address-space. While validations the format at stream

s/validations/validating/

> start make sure the companion VIN is not already using the scaler. If
> the scaler already is in use return -EBUSY.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 134 ++++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-vin.h |  24 ++++
>  2 files changed, 152 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> e752bc86e40153b1..f33146bda9300c21 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -74,6 +74,10 @@
> 
>  /* Register offsets specific for Gen3 */
>  #define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
> +#define VNUDS_CTRL_REG		0x80 /* Video n scaling control register */
> +#define VNUDS_SCALE_REG		0x84 /* Video n scaling factor register */
> +#define VNUDS_PASS_BWIDTH_REG	0x90 /* Video n passband register */
> +#define VNUDS_CLIP_SIZE_REG	0xa4 /* Video n UDS output size clipping reg */
> 
>  /* Register bit fields for R-Car VIN */
>  /* Video n Main Control Register bits */
> @@ -129,6 +133,9 @@
>  #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
>  #define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
> 
> +/* Video n scaling control register (Gen3) */
> +#define VNUDS_CTRL_AMD		(1 << 30)
> +
>  struct rvin_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head list;
> @@ -572,6 +579,78 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev
> *vin) 0, 0);
>  }
> 
> +static unsigned int rvin_scale_ratio(unsigned int in, unsigned int out)
> +{
> +	unsigned int ratio;
> +
> +	ratio = in * 4096 / out;
> +	return ratio >= 0x10000 ? 0xffff : ratio;
> +}
> +
> +static unsigned int rvin_ratio_to_bwidth(unsigned int ratio)

I initially thought this referred to a bandwidth as in a throughput. How about 
renaming the function to rvin_ratio_to_filter_width() ? Or possibly 
standardize the UDS function naming to rvin_uds_*, and name it 
rvin_uds_filter_width() or rvin_uds_passband_width() ?

> +{
> +	unsigned int mant, frac;
> +
> +	mant = (ratio & 0xF000) >> 12;
> +	frac = ratio & 0x0FFF;

Maybe lowercase hex constants ?

> +	if (mant)
> +		return 64 * 4096 * mant / (4096 * mant + frac);

Isn't the denumerator equal to ratio ? How about

	if (ratio >= 0x1000)
		return 64 * (ratio & 0xf000) / ratio;
	else
		return 64;

> +	return 64;
> +}
> +
> +static bool rvin_gen3_need_scaling(struct rvin_dev *vin)
> +{
> +	if (vin->info->model != RCAR_GEN3)
> +		return false;

One of the two callers don't need this check, so you could move it to the 
caller that does.

> +	return vin->crop.width != vin->format.width ||
> +		vin->crop.height != vin->format.height;
> +}
> +
> +static void rvin_crop_scale_comp_gen3(struct rvin_dev *vin)
> +{
> +	unsigned int ratio_h, ratio_v;
> +	unsigned int bwidth_h, bwidth_v;
> +	u32 vnmc, clip_size;
> +
> +	if (!rvin_gen3_need_scaling(vin))
> +		return;
> +
> +	ratio_h = rvin_scale_ratio(vin->crop.width, vin->format.width);

Is there anything that prevents the output width to be so massively bigger 
than the input width to result in a ratio of 0, crashing the passband filter 
width computation ?

> +	bwidth_h = rvin_ratio_to_bwidth(ratio_h);
> +
> +	ratio_v = rvin_scale_ratio(vin->crop.height, vin->format.height);
> +	bwidth_v = rvin_ratio_to_bwidth(ratio_v);
> +
> +	clip_size = vin->format.width << 16;
> +
> +	switch (vin->format.field) {
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
> +		clip_size |= vin->format.height / 2;
> +		break;
> +	default:
> +		clip_size |= vin->format.height;
> +		break;
> +	}
> +
> +	vnmc = rvin_read(vin, VNMC_REG);
> +	rvin_write(vin, (vnmc & ~VNMC_VUP) | VNMC_SCLE, VNMC_REG);
> +	rvin_write(vin, VNUDS_CTRL_AMD, VNUDS_CTRL_REG);
> +	rvin_write(vin, (ratio_h << 16) | ratio_v, VNUDS_SCALE_REG);
> +	rvin_write(vin, (bwidth_h << 16) | bwidth_v, VNUDS_PASS_BWIDTH_REG);
> +	rvin_write(vin, clip_size, VNUDS_CLIP_SIZE_REG);
> +	rvin_write(vin, vnmc, VNMC_REG);
> +
> +	vin_dbg(vin, "Pre-Clip: %ux%u@%u:%u Post-Clip: %ux%u@%u:%u\n",
> +		vin->crop.width, vin->crop.height, vin->crop.left,
> +		vin->crop.top, vin->format.width, vin->format.height, 0, 0);

Now that you have (hopefully :-)) debugged the code, do you still need this ?

> +}
> +
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
>  	/* Set Start/End Pixel/Line Pre-Clip */
> @@ -593,8 +672,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		break;
>  	}
> 
> -	/* TODO: Add support for the UDS scaler. */
> -	if (vin->info->model != RCAR_GEN3)
> +	if (vin->info->model == RCAR_GEN3)
> +		rvin_crop_scale_comp_gen3(vin);
> +	else
>  		rvin_crop_scale_comp_gen2(vin);
> 
>  	rvin_write(vin, vin->format.width, VNIS_REG);
> @@ -748,6 +828,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  			vnmc |= VNMC_DPINE;
>  	}
> 
> +	if (rvin_gen3_need_scaling(vin))
> +		vnmc |= VNMC_SCLE;
> +
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
> 
> @@ -1078,10 +1161,42 @@ static int rvin_mc_validate_format(struct rvin_dev
> *vin, struct v4l2_subdev *sd, return -EPIPE;
>  	}
> 
> -	if (fmt.format.width != vin->format.width ||
> -	    fmt.format.height != vin->format.height ||
> -	    fmt.format.code != vin->mbus_code)
> -		return -EPIPE;
> +	vin->crop.width = fmt.format.width;
> +	vin->crop.height = fmt.format.height;

Isn't the crop rectangle supposed to come from userspace ?

> +	if (rvin_gen3_need_scaling(vin)) {
> +		const struct rvin_group_scaler *scaler;
> +		struct rvin_dev *companion;
> +
> +		if (fmt.format.code != vin->mbus_code)
> +			return -EPIPE;

As this check is needed in both cases I'd move it above the scaling check.

> +		if (!vin->info->scalers)
> +			return -EPIPE;
> +
> +		for (scaler = vin->info->scalers;
> +		     scaler->vin || scaler->companion; scaler++)
> +			if (scaler->vin == vin->id)
> +				break;
> +
> +		/* No scaler found for VIN. */
> +		if (!scaler->vin && !scaler->companion)
> +			return -EPIPE;
> +
> +		/* Make sure companion not using scaler. */
> +		if (scaler->companion != -1) {
> +			companion = vin->group->vin[scaler->companion];
> +			if (companion &&
> +			    companion->state != STOPPED &&
> +			    rvin_gen3_need_scaling(companion))
> +				return -EBUSY;

Without any locking, this screams of a race condition :-)

> +		}
> +	} else {
> +		if (fmt.format.width != vin->format.width ||
> +		    fmt.format.height != vin->format.height ||
> +		    fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +	}
> 
>  	return 0;
>  }
> @@ -1189,6 +1304,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int retries = 0;
> +	u32 vnmc;
> 
>  	spin_lock_irqsave(&vin->qlock, flags);
> 
> @@ -1220,6 +1336,12 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> vin->state = STOPPED;
>  	}
> 
> +	/* Clear UDS usage after we have stopped */
> +	if (vin->info->model == RCAR_GEN3) {
> +		vnmc = rvin_read(vin, VNMC_REG) & ~(VNMC_SCLE | VNMC_VUP);
> +		rvin_write(vin, vnmc, VNMC_REG);
> +	}

You have quite a few read-modify-write sequences for the VNMC register in the 
driver, would it make sense to cache its value ?

>  	/* Release all active buffers */
>  	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 0b13b34d03e3dce4..5a617a30ba8c9a5a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,6 +122,28 @@ struct rvin_group_route {
>  	unsigned int mask;
>  };
> 
> +/**
> + * struct rvin_group_scaler - describes a scaler attached to a VIN
> + *
> + * @vin:	Numerical VIN id that have access to a UDS.

s/have/has/

> + * @companion:  Numerical VIN id that @vin share the UDS with.

s/share/shares/

And I think I would write "Numerical ID of the VIN ..." for both.

> + *
> + * -- note::
> + *	Some R-Car VIN instances have access to a Up Down Scaler (UDS).
> + *	If a VIN have a UDS attached it's almost always shared between

s/have/has/

> + *	two VIN instances. The UDS can only be used by one VIN at a time,
> + *	so the companion relationship needs to be described as well.
> + *
> + *	There are at most two VINs sharing a UDS. For each UDS shared
> + *	between two VINs there needs to be two instances of struct
> + *	rvin_group_scaler describing each of the VINs individually. If
> + *	a VIN do not share its UDS set companion to -1.

s/do/does/
s/companion/@companion/

> + */
> +struct rvin_group_scaler {
> +	int vin;

unsigned int ?

> +	int companion;
> +};
> +
>  /**
>   * struct rvin_info - Information about the particular VIN implementation
>   * @model:		VIN model
> @@ -130,6 +152,7 @@ struct rvin_group_route {
>   * @max_height:		max input height the VIN supports
>   * @routes:		list of possible routes from the CSI-2 recivers to
>   *			all VINs. The list mush be NULL terminated.
> + * @scalers:		List of available scalers, must be NULL terminated.
>   */
>  struct rvin_info {
>  	enum model_id model;
> @@ -138,6 +161,7 @@ struct rvin_info {
>  	unsigned int max_width;
>  	unsigned int max_height;
>  	const struct rvin_group_route *routes;
> +	const struct rvin_group_scaler *scalers;
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart