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@xxxxxxxxxxxx>
> ---
> 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
