Hi Niklas,
Thank you for the patch.
On Wednesday 25 May 2016 21:10:06 Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> Add the register needed to work with Gen3 hardware. This patch just adds
> the logic for how to work with the Gen3 hardware. More work is required
> to enable the subdevice structure needed to support capturing.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 99 ++++++++++++++++----------
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 15 ++++-
> drivers/media/platform/rcar-vin/rcar-vin.h | 1 +
> 3 files changed, 80 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index b3d3c5e..5196395 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 */
By the way, that's not directly related to this patch, but hex constants in
the kernel tend to use lowercase letters.
Also, feel free to give a bit more air to the register definitions even if it
makes them cross the 80 characters per line limit, if it helps improving
readability. If it was up to me I'd add one tab before the register address
for instance to align them with the register fields values a bit further
below.
Still from a readability point of view, I would extend the register address to
4 (or just 3 if you prefer) hex digits as we have registers above the first
0x100 bytes.
> -#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)
> @@ -118,6 +124,12 @@
> #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)
> +
> static void rvin_write(struct rvin_dev *vin, u32 value, u32 offset)
> {
> iowrite32(value, vin->base + offset);
> @@ -196,7 +208,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->chip == RCAR_GEN3)
I'm not too fond of scattering this kind of check across the code. My
preferred approach is to add a device info structure that stores information
about specific device models, and use it as appropriate. You can check the
vsp1 or rcar-du driver for examples.
This being said, given that you have a limited number of such checks, there's
no need to change this now, but please keep that in mind for the future when
more code will be conditioned to the device model.
On a side note, I would rename the field from chip to model, and the values
from RCAR_* to RCAR_VIN_*, as what matters is the VIN model, not the SoC.
> + dmr2 = VNDMR2_FTEV;
> + else
> + dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>
> /* Hsync Signal Polarity Select */
> if (!(mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> @@ -232,7 +247,8 @@ static int rvin_setup(struct rvin_dev *vin)
> dmr = 0;
> break;
> case V4L2_PIX_FMT_XBGR32:
> - if (vin->chip == RCAR_GEN2 || vin->chip == RCAR_H1) {
> + if (vin->chip == RCAR_GEN2 || vin->chip == RCAR_H1 ||
> + vin->chip == RCAR_GEN3) {
If V4L2_PIX_FMT_XBGR32 isn't allowed on M1, VIDIOC_S_FMT and VIDIOC_TRY_FMT
should disallow it. This isn't the right place for error checking.
I know the problem isn't introduced by this patch, so you can fix it in a
separate patch. Ideally I would fix the problem first, which would get rid of
this check, and then apply this patch with this hunk removed.
> dmr = VNDMR_EXRGB;
> break;
> }
> @@ -250,6 +266,14 @@ static int rvin_setup(struct rvin_dev *vin)
> if (input_is_yuv == output_is_yuv)
> vnmc |= VNMC_BPS;
>
> + if (vin->chip == RCAR_GEN3) {
> + /* Select between CSI-2 and Digital input */
> + if (mbus_cfg.type == V4L2_MBUS_CSI2)
> + vnmc &= ~VNMC_DPINE;
> + else
> + vnmc |= VNMC_DPINE;
> + }
> +
> /* Progressive or interlaced mode */
> interrupts = progressive ? VNIE_FIE : VNIE_EFE;
>
> @@ -740,28 +764,10 @@ static void rvin_set_coeff(struct rvin_dev *vin,
> unsigned short xs) rvin_write(vin, p_set->coeff_set[23], VNC8C_REG);
> }
>
> -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)
> @@ -799,11 +805,6 @@ 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,
> @@ -811,9 +812,43 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> 0, 0);
> }
>
> +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);
Please add a blank line here.
> + 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;
> + }
> +
> + /* Driver do not support UDS */
> + if (vin->chip != RCAR_GEN3)
> + rvin_crop_scale_comp_gen2(vin);
If you reorder the chip IDs as proposed below, I think
if (vin->chip <= RCAR_GEN2)
rvin_crop_scale_comp_gen2(vin);
would be clearer.
> +
> + 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);
> +}
> +
> void rvin_scale_try(struct rvin_dev *vin, struct v4l2_pix_format *pix,
> u32 width, u32 height)
> {
> + /* Driver do not support UDS */
s/do not/does not/
And I'd write it as
/* TODO: Add support for the UDS scaler. */
> + if (vin->chip == RCAR_GEN3)
> + return;
> +
> /* All VIN channels on Gen2 have scalers */
> pix->width = width;
> pix->height = height;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 2307f5b..f4eaef0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -25,6 +25,8 @@
> #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV
> #define RVIN_MAX_WIDTH 2048
> #define RVIN_MAX_HEIGHT 2048
> +#define RVIN_MAX_WIDTH_GEN3 4096
> +#define RVIN_MAX_HEIGHT_GEN3 4096
>
> /* ------------------------------------------------------------------------
> * Format Conversions
> @@ -139,7 +141,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> struct rvin_source_fmt *source)
> {
> const struct rvin_video_format *info;
> - u32 rwidth, rheight, walign;
> + u32 rwidth, rheight, walign, max_width, max_height;
>
> /* Requested */
> rwidth = pix->width;
> @@ -173,8 +175,15 @@ static int __rvin_try_format(struct rvin_dev *vin,
> walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
>
> /* Limit to VIN capabilities */
> - v4l_bound_align_image(&pix->width, 2, RVIN_MAX_WIDTH, walign,
> - &pix->height, 4, RVIN_MAX_HEIGHT, 2, 0);
> + if (vin->chip == RCAR_GEN3) {
> + max_width = RVIN_MAX_WIDTH_GEN3;
> + max_height = RVIN_MAX_HEIGHT_GEN3;
> + } else {
> + max_width = RVIN_MAX_WIDTH;
> + max_height = RVIN_MAX_HEIGHT;
> + }
This is one example where I think a device info structure would help. You
could store there the maximum width and height, and remove the condition here.
Again, no need to change this now as long as we don't have too many such
checks.
> + v4l_bound_align_image(&pix->width, 2, max_width, walign,
> + &pix->height, 4, max_height, 2, 0);
>
> switch (pix->field) {
> case V4L2_FIELD_NONE:
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index 81780f1..b97fa43 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -32,6 +32,7 @@
> #define HW_BUFFER_MASK 0x7f
>
> enum chip_id {
> + RCAR_GEN3,
> RCAR_GEN2,
> RCAR_H1,
> RCAR_M1,
Nitpicking, it looks weird to me to list them in reverse chronological order,
I'd order them as
RCAR_H1
RCAR_M1
RCAR_GEN2
RCAR_GEN3
Another node
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
