2020-02-28 16:50:40

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 0/2] media: rcar-vin: feature enhancement and fixes

First patch of the series fixes registering endpoints to the vin module and
the second patch adds supports for SRGGB8_1X8 format.

Lad Prabhakar (2):
media: rcar-csi2: Fix registering camera endpoint to VIN
media: rcar-vin: Add support for SRGGB8_1X8

drivers/media/platform/rcar-vin/rcar-core.c | 1 +
drivers/media/platform/rcar-vin/rcar-csi2.c | 15 +++++++--------
drivers/media/platform/rcar-vin/rcar-dma.c | 14 ++++++++++++--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++++
4 files changed, 24 insertions(+), 10 deletions(-)

--
2.20.1


2020-02-28 16:51:41

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 1/2] media: rcar-csi2: Fix registering camera endpoint to VIN

CSI2 registers camera/sensor as v4l2 async sub device with fwnode is
remote endpoint and the camera/sensor register itself as v4l2 sub device
with fwnode is remote device as a result the match.fwnode should be
fwnode_graph_get_remote_port_parent and not
fwnode_graph_get_remote_endpoint.

This patch makes use of v4l2 helper function
v4l2_async_notifier_add_fwnode_remote_subdev() which uses
fwnode_graph_get_remote_port_parent as match.fwnode fixing the issue
of registering camera endpoint to the driver.

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb23a2e9..5b04e4768eb1 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -833,20 +833,18 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
return ret;
}

- priv->asd.match.fwnode =
- fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
- priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-
- of_node_put(ep);
-
v4l2_async_notifier_init(&priv->notifier);

- ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
+ ret = v4l2_async_notifier_add_fwnode_remote_subdev(&priv->notifier,
+ of_fwnode_handle(ep),
+ &priv->asd);
if (ret) {
- fwnode_handle_put(priv->asd.match.fwnode);
+ of_node_put(ep);
return ret;
}

+ of_node_put(ep);
+
priv->notifier.ops = &rcar_csi2_notify_ops;

dev_dbg(priv->dev, "Found '%pOF'\n",
--
2.20.1

2020-02-28 16:51:57

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH 2/2] media: rcar-vin: Add support for SRGGB8_1X8

Most of the RZ/G2x boards can support capturing frames in SRGGB8_1X8
format, add support for this with a runtime check that the running
hardware supports it.

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/media/platform/rcar-vin/rcar-core.c | 1 +
drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
drivers/media/platform/rcar-vin/rcar-dma.c | 14 ++++++++++++--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++++
4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c8965d27..76daf2fe5bcd 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
case MEDIA_BUS_FMT_UYVY8_2X8:
case MEDIA_BUS_FMT_UYVY10_2X10:
case MEDIA_BUS_FMT_RGB888_1X24:
+ case MEDIA_BUS_FMT_SRGGB8_1X8:
vin->mbus_code = code.code;
vin_dbg(vin, "Found media bus format for %s: %d\n",
subdev->name, vin->mbus_code);
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 5b04e4768eb1..97e4886f63f0 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = {
{ .code = MEDIA_BUS_FMT_YUYV8_1X16, .datatype = 0x1e, .bpp = 16 },
{ .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 },
{ .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 20 },
+ { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = 8 },
};

static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd036371..63709b4bbf7a 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -85,6 +85,7 @@
#define VNMC_INF_YUV8_BT601 (1 << 16)
#define VNMC_INF_YUV10_BT656 (2 << 16)
#define VNMC_INF_YUV10_BT601 (3 << 16)
+#define VNMC_INF_RAW8 (4 << 16)
#define VNMC_INF_YUV16 (5 << 16)
#define VNMC_INF_RGB888 (6 << 16)
#define VNMC_VUP (1 << 10)
@@ -587,13 +588,15 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
rvin_write(vin, vin->crop.top, VNSLPRC_REG);
rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);

-
/* TODO: Add support for the UDS scaler. */
if (vin->info->model != RCAR_GEN3)
rvin_crop_scale_comp_gen2(vin);

fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
- stride = vin->format.bytesperline / fmt->bpp;
+ if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
+ stride = ALIGN(vin->format.bytesperline / 2, 0x10);
+ else
+ stride = vin->format.bytesperline / fmt->bpp;
rvin_write(vin, stride, VNIS_REG);
}

@@ -676,6 +679,9 @@ static int rvin_setup(struct rvin_dev *vin)

input_is_yuv = true;
break;
+ case MEDIA_BUS_FMT_SRGGB8_1X8:
+ vnmc |= VNMC_INF_RAW8;
+ break;
default:
break;
}
@@ -737,6 +743,9 @@ static int rvin_setup(struct rvin_dev *vin)
case V4L2_PIX_FMT_ABGR32:
dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
break;
+ case V4L2_PIX_FMT_SRGGB8:
+ dmr = 0;
+ break;
default:
vin_err(vin, "Invalid pixelformat (0x%x)\n",
vin->format.pixelformat);
@@ -1110,6 +1119,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
case MEDIA_BUS_FMT_UYVY8_2X8:
case MEDIA_BUS_FMT_UYVY10_2X10:
case MEDIA_BUS_FMT_RGB888_1X24:
+ case MEDIA_BUS_FMT_SRGGB8_1X8:
vin->mbus_code = fmt.format.code;
break;
default:
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b12b3f88836a..d56cf85ba065 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
.fourcc = V4L2_PIX_FMT_ABGR32,
.bpp = 4,
},
+ {
+ .fourcc = V4L2_PIX_FMT_SRGGB8,
+ .bpp = 1,
+ },
};

const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
--
2.20.1

2020-03-04 20:47:44

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: rcar-csi2: Fix registering camera endpoint to VIN

Hi Lad,

Thanks for your work.

On 2020-02-28 16:50:10 +0000, Lad Prabhakar wrote:
> CSI2 registers camera/sensor as v4l2 async sub device with fwnode is
> remote endpoint and the camera/sensor register itself as v4l2 sub device
> with fwnode is remote device as a result the match.fwnode should be
> fwnode_graph_get_remote_port_parent and not
> fwnode_graph_get_remote_endpoint.
>
> This patch makes use of v4l2 helper function
> v4l2_async_notifier_add_fwnode_remote_subdev() which uses
> fwnode_graph_get_remote_port_parent as match.fwnode fixing the issue
> of registering camera endpoint to the driver.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

I'm afraid this is not the right solution. The rcar-csi2 driver uses the
v4l2-async framework to do endpoint matching instead of node matching.
This is needed as it needs to work with the adv748x driver which
register it self in v4l2-async using endpoints instead of nodes. The
reason for this is that from a single DT node it creates multiple
subdevices, one for each endpoint IIRC.

Without this patch the two CSI-2 receivers on R-Car M3-n registers the
two following 'paths' in v4l2 to be able to find the two subdevice CSI-2
transmitters created by the ADV748x.

rcar-csi2 fea80000.csi2: '/soc/i2c@e66d8000/video-receiver@70/port@b/endpoint'
rcar-csi2 feaa0000.csi2: '/soc/i2c@e66d8000/video-receiver@70/port@a/endpoint'

With this patch applied it registers the following which can't be found
as they are not present in the v4l2-async list of subdevices (as they
are registerd as above).

rcar-csi2 fea80000.csi2: '/soc/i2c@e66d8000/video-receiver@70'
rcar-csi2 feaa0000.csi2: '/soc/i2c@e66d8000/video-receiver@70'
rcar-csi2: probe of feaa0000.csi2 failed with error -17

This patch may unlock your use-case as it's a known problem that
endpoint and node matching do not mix. But it will break the already
upstream use-case and for that reason, I'm really sorry about this.

Nacked-by: Niklas S?derlund <[email protected]>

The real solution to this problem IMHO is to make all of v4l2-async
operate using endpoint matching or possibly some kind of fallback to
node matching if no endpoint can be found. Never the less some work is
required in the v4l2-async core to sort out node and endpoint matching
coexistence.

> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb23a2e9..5b04e4768eb1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -833,20 +833,18 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> return ret;
> }
>
> - priv->asd.match.fwnode =
> - fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> - priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -
> - of_node_put(ep);
> -
> v4l2_async_notifier_init(&priv->notifier);
>
> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> + ret = v4l2_async_notifier_add_fwnode_remote_subdev(&priv->notifier,
> + of_fwnode_handle(ep),
> + &priv->asd);
> if (ret) {
> - fwnode_handle_put(priv->asd.match.fwnode);
> + of_node_put(ep);
> return ret;
> }
>
> + of_node_put(ep);
> +
> priv->notifier.ops = &rcar_csi2_notify_ops;
>
> dev_dbg(priv->dev, "Found '%pOF'\n",
> --
> 2.20.1
>

--
Regards,
Niklas S?derlund

2020-03-04 20:55:47

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH 2/2] media: rcar-vin: Add support for SRGGB8_1X8

Hi Lad,

Thanks for your work.

On 2020-02-28 16:50:11 +0000, Lad Prabhakar wrote:
> Most of the RZ/G2x boards can support capturing frames in SRGGB8_1X8
> format, add support for this with a runtime check that the running
> hardware supports it.

Where is the runtime check for supported hardware? Also out of curiosity
which boars can't support SRGGB and what setup are you using to test
SRGGB capture?

>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
> drivers/media/platform/rcar-vin/rcar-dma.c | 14 ++++++++++++--
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++++

This patch shall be split in two. One which touches rcar-csi2.c and one
for the other files as they are two different drivers.

> 4 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c8965d27..76daf2fe5bcd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> vin->mbus_code = code.code;
> vin_dbg(vin, "Found media bus format for %s: %d\n",
> subdev->name, vin->mbus_code);
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 5b04e4768eb1..97e4886f63f0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = {
> { .code = MEDIA_BUS_FMT_YUYV8_1X16, .datatype = 0x1e, .bpp = 16 },
> { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 },
> { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 20 },
> + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = 8 },
> };
>
> static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd036371..63709b4bbf7a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -85,6 +85,7 @@
> #define VNMC_INF_YUV8_BT601 (1 << 16)
> #define VNMC_INF_YUV10_BT656 (2 << 16)
> #define VNMC_INF_YUV10_BT601 (3 << 16)
> +#define VNMC_INF_RAW8 (4 << 16)
> #define VNMC_INF_YUV16 (5 << 16)
> #define VNMC_INF_RGB888 (6 << 16)
> #define VNMC_VUP (1 << 10)
> @@ -587,13 +588,15 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
>
> -
> /* TODO: Add support for the UDS scaler. */
> if (vin->info->model != RCAR_GEN3)
> rvin_crop_scale_comp_gen2(vin);
>
> fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> - stride = vin->format.bytesperline / fmt->bpp;
> + if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
> + stride = ALIGN(vin->format.bytesperline / 2, 0x10);
> + else
> + stride = vin->format.bytesperline / fmt->bpp;

This does not look right, you should to move this logic into
rvin_format_bytesperline().

> rvin_write(vin, stride, VNIS_REG);
> }
>
> @@ -676,6 +679,9 @@ static int rvin_setup(struct rvin_dev *vin)
>
> input_is_yuv = true;
> break;
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + vnmc |= VNMC_INF_RAW8;
> + break;
> default:
> break;
> }
> @@ -737,6 +743,9 @@ static int rvin_setup(struct rvin_dev *vin)
> case V4L2_PIX_FMT_ABGR32:
> dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
> break;
> + case V4L2_PIX_FMT_SRGGB8:
> + dmr = 0;
> + break;
> default:
> vin_err(vin, "Invalid pixelformat (0x%x)\n",
> vin->format.pixelformat);
> @@ -1110,6 +1119,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
> case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> vin->mbus_code = fmt.format.code;
> break;
> default:
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index b12b3f88836a..d56cf85ba065 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
> .fourcc = V4L2_PIX_FMT_ABGR32,
> .bpp = 4,
> },
> + {
> + .fourcc = V4L2_PIX_FMT_SRGGB8,
> + .bpp = 1,
> + },
> };
>
> const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> --
> 2.20.1
>

--
Regards,
Niklas S?derlund

2020-03-07 13:04:17

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: rcar-csi2: Fix registering camera endpoint to VIN

Hi Niklas,

Thank you for the review.

On Wed, Mar 4, 2020 at 8:47 PM Niklas <[email protected]> wrote:
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-02-28 16:50:10 +0000, Lad Prabhakar wrote:
> > CSI2 registers camera/sensor as v4l2 async sub device with fwnode is
> > remote endpoint and the camera/sensor register itself as v4l2 sub device
> > with fwnode is remote device as a result the match.fwnode should be
> > fwnode_graph_get_remote_port_parent and not
> > fwnode_graph_get_remote_endpoint.
> >
> > This patch makes use of v4l2 helper function
> > v4l2_async_notifier_add_fwnode_remote_subdev() which uses
> > fwnode_graph_get_remote_port_parent as match.fwnode fixing the issue
> > of registering camera endpoint to the driver.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> I'm afraid this is not the right solution. The rcar-csi2 driver uses the
> v4l2-async framework to do endpoint matching instead of node matching.
> This is needed as it needs to work with the adv748x driver which
> register it self in v4l2-async using endpoints instead of nodes. The
> reason for this is that from a single DT node it creates multiple
> subdevices, one for each endpoint IIRC.
>
> Without this patch the two CSI-2 receivers on R-Car M3-n registers the
> two following 'paths' in v4l2 to be able to find the two subdevice CSI-2
> transmitters created by the ADV748x.
>
> rcar-csi2 fea80000.csi2: '/soc/i2c@e66d8000/video-receiver@70/port@b/endpoint'
> rcar-csi2 feaa0000.csi2: '/soc/i2c@e66d8000/video-receiver@70/port@a/endpoint'
>
> With this patch applied it registers the following which can't be found
> as they are not present in the v4l2-async list of subdevices (as they
> are registerd as above).
>
> rcar-csi2 fea80000.csi2: '/soc/i2c@e66d8000/video-receiver@70'
> rcar-csi2 feaa0000.csi2: '/soc/i2c@e66d8000/video-receiver@70'
> rcar-csi2: probe of feaa0000.csi2 failed with error -17
>
> This patch may unlock your use-case as it's a known problem that
> endpoint and node matching do not mix. But it will break the already
> upstream use-case and for that reason, I'm really sorry about this.
>
Completely missed it, thank you for pointing out.

> Nacked-by: Niklas Söderlund <[email protected]>
>
> The real solution to this problem IMHO is to make all of v4l2-async
> operate using endpoint matching or possibly some kind of fallback to
> node matching if no endpoint can be found. Never the less some work is
> required in the v4l2-async core to sort out node and endpoint matching
> coexistence.
>
Thank you for the pointers, I shall do some digging in v4l2-asyn.

Cheers,
--Prabhakar

> > ---
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb23a2e9..5b04e4768eb1 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -833,20 +833,18 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> > return ret;
> > }
> >
> > - priv->asd.match.fwnode =
> > - fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > - priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > -
> > - of_node_put(ep);
> > -
> > v4l2_async_notifier_init(&priv->notifier);
> >
> > - ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> > + ret = v4l2_async_notifier_add_fwnode_remote_subdev(&priv->notifier,
> > + of_fwnode_handle(ep),
> > + &priv->asd);
> > if (ret) {
> > - fwnode_handle_put(priv->asd.match.fwnode);
> > + of_node_put(ep);
> > return ret;
> > }
> >
> > + of_node_put(ep);
> > +
> > priv->notifier.ops = &rcar_csi2_notify_ops;
> >
> > dev_dbg(priv->dev, "Found '%pOF'\n",
> > --
> > 2.20.1
> >
>
> --
> Regards,
> Niklas Söderlund

2020-03-07 13:11:04

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 2/2] media: rcar-vin: Add support for SRGGB8_1X8

Hi Niklas,

Thank you for the review.

On Wed, Mar 4, 2020 at 8:55 PM Niklas <[email protected]> wrote:
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-02-28 16:50:11 +0000, Lad Prabhakar wrote:
> > Most of the RZ/G2x boards can support capturing frames in SRGGB8_1X8
> > format, add support for this with a runtime check that the running
> > hardware supports it.
>
> Where is the runtime check for supported hardware? Also out of curiosity
> which boars can't support SRGGB and what setup are you using to test
> SRGGB capture?
>
My bad l'll drop the statement "runtime check for supported hardware".
All the G2x
boards support SRGGB8. I am currently testing this on a G2E board connected with
a mipi adapter (https://www.96boards.org/product/mipiadapter/) and
raspberry pi camera
module (https://www.raspberrypi.org/products/camera-module-v2/).

> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
> > drivers/media/platform/rcar-vin/rcar-dma.c | 14 ++++++++++++--
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++++
>
> This patch shall be split in two. One which touches rcar-csi2.c and one
> for the other files as they are two different drivers.
>
sure will split this patch into two.

> > 4 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7440c8965d27..76daf2fe5bcd 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > case MEDIA_BUS_FMT_UYVY8_2X8:
> > case MEDIA_BUS_FMT_UYVY10_2X10:
> > case MEDIA_BUS_FMT_RGB888_1X24:
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > vin->mbus_code = code.code;
> > vin_dbg(vin, "Found media bus format for %s: %d\n",
> > subdev->name, vin->mbus_code);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 5b04e4768eb1..97e4886f63f0 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > { .code = MEDIA_BUS_FMT_YUYV8_1X16, .datatype = 0x1e, .bpp = 16 },
> > { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 },
> > { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 20 },
> > + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = 0x2a, .bpp = 8 },
> > };
> >
> > static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 1a30cd036371..63709b4bbf7a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -85,6 +85,7 @@
> > #define VNMC_INF_YUV8_BT601 (1 << 16)
> > #define VNMC_INF_YUV10_BT656 (2 << 16)
> > #define VNMC_INF_YUV10_BT601 (3 << 16)
> > +#define VNMC_INF_RAW8 (4 << 16)
> > #define VNMC_INF_YUV16 (5 << 16)
> > #define VNMC_INF_RGB888 (6 << 16)
> > #define VNMC_VUP (1 << 10)
> > @@ -587,13 +588,15 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> >
> > -
> > /* TODO: Add support for the UDS scaler. */
> > if (vin->info->model != RCAR_GEN3)
> > rvin_crop_scale_comp_gen2(vin);
> >
> > fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> > - stride = vin->format.bytesperline / fmt->bpp;
> > + if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
> > + stride = ALIGN(vin->format.bytesperline / 2, 0x10);
> > + else
> > + stride = vin->format.bytesperline / fmt->bpp;
>
> This does not look right, you should to move this logic into
> rvin_format_bytesperline().
>
sure will move it.

Cheers,
--Prabhakar

> > rvin_write(vin, stride, VNIS_REG);
> > }
> >
> > @@ -676,6 +679,9 @@ static int rvin_setup(struct rvin_dev *vin)
> >
> > input_is_yuv = true;
> > break;
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > + vnmc |= VNMC_INF_RAW8;
> > + break;
> > default:
> > break;
> > }
> > @@ -737,6 +743,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > case V4L2_PIX_FMT_ABGR32:
> > dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
> > break;
> > + case V4L2_PIX_FMT_SRGGB8:
> > + dmr = 0;
> > + break;
> > default:
> > vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > vin->format.pixelformat);
> > @@ -1110,6 +1119,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
> > case MEDIA_BUS_FMT_UYVY8_2X8:
> > case MEDIA_BUS_FMT_UYVY10_2X10:
> > case MEDIA_BUS_FMT_RGB888_1X24:
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > vin->mbus_code = fmt.format.code;
> > break;
> > default:
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index b12b3f88836a..d56cf85ba065 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
> > .fourcc = V4L2_PIX_FMT_ABGR32,
> > .bpp = 4,
> > },
> > + {
> > + .fourcc = V4L2_PIX_FMT_SRGGB8,
> > + .bpp = 1,
> > + },
> > };
> >
> > const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> > --
> > 2.20.1
> >
>
> --
> Regards,
> Niklas Söderlund