Hi Laurent,
Thanks for your comments.
On 2017-12-08 11:47:13 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Friday, 8 December 2017 03:08:29 EET Niklas Söderlund wrote:
> > Add the register needed to work with Gen3 hardware. This patch adds
> > the logic for how to work with the Gen3 hardware. More work is required
> > to enable the subdevice structure needed to configure capturing.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> > Reviewed-by: Hans Verkuil <hans.verk...@cisco.com>
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 94 ++++++++++++++++++---------
> > drivers/media/platform/rcar-vin/rcar-vin.h | 1 +
> > 2 files changed, 64 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > d7660f485a2df9e4..ace95d5b543a17e3 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -33,21 +33,23 @@
> > #define VNELPRC_REG 0x10 /* Video n End Line Pre-Clip Register */
> > #define VNSPPRC_REG 0x14 /* Video n Start Pixel Pre-Clip
> > Register */
> > #define VNEPPRC_REG 0x18 /* Video n End Pixel Pre-Clip Register
> > */
> > -#define VNSLPOC_REG 0x1C /* Video n Start Line Post-Clip
> > Register */
> > -#define VNELPOC_REG 0x20 /* Video n End Line Post-Clip Register
> > */
> > -#define VNSPPOC_REG 0x24 /* Video n Start Pixel Post-Clip
> > Register */
> > -#define VNEPPOC_REG 0x28 /* Video n End Pixel Post-Clip Register
> > */
> > #define VNIS_REG 0x2C /* Video n Image Stride Register */
> > #define VNMB_REG(m) (0x30 + ((m) << 2)) /* Video n Memory Base m
> > Register
> > */ #define VNIE_REG 0x40 /* Video n Interrupt Enable Register */
> > #define VNINTS_REG 0x44 /* Video n Interrupt Status Register */
> > #define VNSI_REG 0x48 /* Video n Scanline Interrupt Register */
> > #define VNMTC_REG 0x4C /* Video n Memory Transfer Control Register */
> > -#define VNYS_REG 0x50 /* Video n Y Scale Register */
> > -#define VNXS_REG 0x54 /* Video n X Scale Register */
> > #define VNDMR_REG 0x58 /* Video n Data Mode Register */
> > #define VNDMR2_REG 0x5C /* Video n Data Mode Register 2 */
> > #define VNUVAOF_REG 0x60 /* Video n UV Address Offset Register */
> > +
> > +/* Register offsets specific for Gen2 */
> > +#define VNSLPOC_REG 0x1C /* Video n Start Line Post-Clip
> > Register */
> > +#define VNELPOC_REG 0x20 /* Video n End Line Post-Clip Register
> > */
> > +#define VNSPPOC_REG 0x24 /* Video n Start Pixel Post-Clip
> > Register */
> > +#define VNEPPOC_REG 0x28 /* Video n End Pixel Post-Clip Register
> > */
> > +#define VNYS_REG 0x50 /* Video n Y Scale Register */
> > +#define VNXS_REG 0x54 /* Video n X Scale Register */
> > #define VNC1A_REG 0x80 /* Video n Coefficient Set C1A Register */
> > #define VNC1B_REG 0x84 /* Video n Coefficient Set C1B Register */
> > #define VNC1C_REG 0x88 /* Video n Coefficient Set C1C Register */
> > @@ -73,9 +75,13 @@
> > #define VNC8B_REG 0xF4 /* Video n Coefficient Set C8B Register */
> > #define VNC8C_REG 0xF8 /* Video n Coefficient Set C8C Register */
> >
> > +/* Register offsets specific for Gen3 */
> > +#define VNCSI_IFMD_REG 0x20 /* Video n CSI2 Interface Mode
> > Register */
> >
> > /* Register bit fields for R-Car VIN */
> > /* Video n Main Control Register bits */
> > +#define VNMC_DPINE (1 << 27) /* Gen3 specific */
> > +#define VNMC_SCLE (1 << 26) /* Gen3 specific */
> > #define VNMC_FOC (1 << 21)
> > #define VNMC_YCAL (1 << 19)
> > #define VNMC_INF_YUV8_BT656 (0 << 16)
> > @@ -119,6 +125,13 @@
> > #define VNDMR2_FTEV (1 << 17)
> > #define VNDMR2_VLV(n) ((n & 0xf) << 12)
> >
> > +/* Video n CSI2 Interface Mode Register (Gen3) */
> > +#define VNCSI_IFMD_DES2 (1 << 27)
> > +#define VNCSI_IFMD_DES1 (1 << 26)
> > +#define VNCSI_IFMD_DES0 (1 << 25)
> > +#define VNCSI_IFMD_CSI_CHSEL(n) ((n & 0xf) << 0)
>
> *Always* enclose macro arguments in parentheses otherwise they are subject to
> side effects.
>
> #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
Thanks for spotting this.
>
> > +#define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
> > +
> > struct rvin_buffer {
> > struct vb2_v4l2_buffer vb;
> > struct list_head list;
> > @@ -514,28 +527,10 @@ static void rvin_set_coeff(struct rvin_dev *vin,
> > unsigned short xs) rvin_write(vin, p_set->coeff_set[23], VNC8C_REG);
> > }
> >
> > -static void rvin_crop_scale_comp(struct rvin_dev *vin)
> > +static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> > {
> > u32 xs, ys;
> >
> > - /* Set Start/End Pixel/Line Pre-Clip */
> > - rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > - rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > - switch (vin->format.field) {
> > - case V4L2_FIELD_INTERLACED:
> > - case V4L2_FIELD_INTERLACED_TB:
> > - case V4L2_FIELD_INTERLACED_BT:
> > - rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > - rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > - VNELPRC_REG);
> > - break;
> > - default:
> > - rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > - rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > - VNELPRC_REG);
> > - break;
> > - }
> > -
> > /* Set scaling coefficient */
> > ys = 0;
> > if (vin->crop.height != vin->compose.height)
> > @@ -573,11 +568,6 @@ static void rvin_crop_scale_comp(struct rvin_dev *vin)
> > break;
> > }
> >
> > - if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> > - rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> > - else
> > - rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> > -
> > vin_dbg(vin,
> > "Pre-Clip: %ux%u@%u:%u YS: %d XS: %d Post-Clip: %ux%u@%u:%u\n",
> > vin->crop.width, vin->crop.height, vin->crop.left,
>
> Would it make sense to keep the debug message in the common function, or ill
> cropping and composing be handled through subdevs for Gen3 ?
Yes, my current idea is to model the scaler as a subdevice. As I'm sure
you are aware not all VIN instances have a scaler. And those who do
share it with one other VIN instance, so both of them can't use the
scaler at the same time (see register VnMC.SCLE).
Modeling it as a separate subdevice and using media links ensure only
one VIN is using it at a time therefor I think is the best solution.
>
> With these two issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>
> > @@ -585,6 +575,37 @@ static void rvin_crop_scale_comp(struct rvin_dev *vin)
> > 0, 0);
> > }
> >
> > +static void rvin_crop_scale_comp(struct rvin_dev *vin)
> > +{
> > + /* Set Start/End Pixel/Line Pre-Clip */
> > + rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > + rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > +
> > + switch (vin->format.field) {
> > + case V4L2_FIELD_INTERLACED:
> > + case V4L2_FIELD_INTERLACED_TB:
> > + case V4L2_FIELD_INTERLACED_BT:
> > + rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > + rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > + VNELPRC_REG);
> > + break;
> > + default:
> > + rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > + rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > + VNELPRC_REG);
> > + break;
> > + }
> > +
> > + /* TODO: Add support for the UDS scaler. */
> > + if (vin->info->chip != RCAR_GEN3)
> > + rvin_crop_scale_comp_gen2(vin);
> > +
> > + if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> > + rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> > + else
> > + rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> > +}
> > +
> > /* ------------------------------------------------------------------------
> > * Hardware setup
> > */
> > @@ -659,7 +680,10 @@ static int rvin_setup(struct rvin_dev *vin)
> > }
> >
> > /* Enable VSYNC Field Toogle mode after one VSYNC input */
> > - dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> > + if (vin->info->chip == RCAR_GEN3)
> > + dmr2 = VNDMR2_FTEV;
> > + else
> > + dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> >
> > /* Hsync Signal Polarity Select */
> > if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> > @@ -711,6 +735,14 @@ static int rvin_setup(struct rvin_dev *vin)
> > if (input_is_yuv == output_is_yuv)
> > vnmc |= VNMC_BPS;
> >
> > + if (vin->info->chip == RCAR_GEN3) {
> > + /* Select between CSI-2 and Digital input */
> > + if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
> > + vnmc &= ~VNMC_DPINE;
> > + else
> > + vnmc |= VNMC_DPINE;
> > + }
> > +
> > /* Progressive or interlaced mode */
> > interrupts = progressive ? VNIE_FIE : VNIE_EFE;
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > 118f45b656920d39..a440effe4b86af31 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -33,6 +33,7 @@ enum chip_id {
> > RCAR_H1,
> > RCAR_M1,
> > RCAR_GEN2,
> > + RCAR_GEN3,
> > };
> >
> > /**
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
