2020-03-10 11:07:13

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes

First patch of the series adds support for matching fwnode against
endpoints/nodes which are registered in v4l2-async and the later adds
supports for MEDIA_BUS_FMT_SRGGB8_1X8 format.

Changes for v2:
1: Added support for matching fwnode against endpoints/nodes.
2: Seperated patch for rcar-vin and rcar-csi2.c which added
MEDIA_BUS_FMT_SRGGB8_1X8.

Lad Prabhakar (3):
media: rcar-csi2: Add support to match fwnode against remote or parent
port
media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8
format

drivers/media/platform/rcar-vin/rcar-core.c | 1 +
drivers/media/platform/rcar-vin/rcar-csi2.c | 42 ++++++++++++++++++++++++++---
drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++-
drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++-
4 files changed, 60 insertions(+), 5 deletions(-)

--
2.7.4


2020-03-10 11:07:22

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

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.

But when using subdevs which register itself in v4l2-async using nodes,
the rcar-csi2 driver failed to find the matching endpoint because the
match.fwnode was pointing to remote endpoint instead of remote parent
port.

This commit adds support in rcar-csi2 driver to handle both the cases
where subdev registers in v4l2-async using endpoints/nodes, by using
match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
callback to compare the fwnode of either remote/parent.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..39e1639 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
return 0;
}

+static bool rcsi2_asd_match(struct device *dev,
+ struct v4l2_async_subdev *async_sd)
+{
+ struct rcar_csi2 *priv = (struct rcar_csi2 *)
+ async_sd->match.custom.priv;
+ struct fwnode_handle *endpoint;
+ struct fwnode_handle *remote;
+ struct fwnode_handle *parent;
+ struct device_node *np;
+ bool matched = false;
+
+ np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
+ if (!np) {
+ dev_err(priv->dev, "Not connected to subdevice\n");
+ return -EINVAL;
+ }
+
+ endpoint = of_fwnode_handle(np);
+ remote = fwnode_graph_get_remote_endpoint(endpoint);
+ parent = fwnode_graph_get_remote_port_parent(endpoint);
+ if (parent) {
+ if (parent == dev->fwnode ||
+ parent == &dev->of_node->fwnode)
+ matched = true;
+ } else if (remote && !matched) {
+ if (remote == dev->fwnode ||
+ remote == &dev->of_node->fwnode)
+ matched = true;
+ }
+
+ of_node_put(np);
+
+ return matched;
+}
+
static int rcsi2_parse_dt(struct rcar_csi2 *priv)
{
struct device_node *ep;
@@ -833,9 +868,9 @@ 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;
+ priv->asd.match.custom.match = &rcsi2_asd_match;
+ priv->asd.match.custom.priv = priv;
+ priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;

of_node_put(ep);

--
2.7.4

2020-03-10 11:07:40

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by setting
format type to RAW8 in VNMC register and appropriately setting the
bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c89..76daf2f 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-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
@@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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 = 2,
+ },
};

const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
@@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
{
const struct rvin_video_format *fmt;
u32 align;
+ u8 div;

fmt = rvin_format_from_pixel(vin, pix->pixelformat);

@@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
case V4L2_PIX_FMT_NV12:
case V4L2_PIX_FMT_NV16:
align = 0x20;
+ div = 1;
+ break;
+ case V4L2_PIX_FMT_SRGGB8:
+ align = 0x10;
+ div = 2;
break;
default:
align = 0x10;
+ div = 1;
break;
}

if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
align = 0x80;

- return ALIGN(pix->width, align) * fmt->bpp;
+ return ALIGN(pix->width / div, align) * fmt->bpp;
}

static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
--
2.7.4

2020-03-10 11:08:02

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2
input.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 39e1639..b030ef6 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)
--
2.7.4

2020-03-10 12:44:57

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:02 +0000, Lad Prabhakar wrote:
> 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.
>
> But when using subdevs which register itself in v4l2-async using nodes,
> the rcar-csi2 driver failed to find the matching endpoint because the
> match.fwnode was pointing to remote endpoint instead of remote parent
> port.
>
> This commit adds support in rcar-csi2 driver to handle both the cases
> where subdev registers in v4l2-async using endpoints/nodes, by using
> match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
> callback to compare the fwnode of either remote/parent.

This is a novel approach to the solution, and I won't object to it out
right. But I think the proper solution is to move this logic into
v4l2-async instead of adding a custom match handler in rcar-csi2.

Think of the reveres use-case, a different CSI-2 receiver who wish to
use the ADV748x would still have this node vs. endpoint issue.

>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 41 ++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..39e1639 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> return 0;
> }
>
> +static bool rcsi2_asd_match(struct device *dev,
> + struct v4l2_async_subdev *async_sd)
> +{
> + struct rcar_csi2 *priv = (struct rcar_csi2 *)
> + async_sd->match.custom.priv;
> + struct fwnode_handle *endpoint;
> + struct fwnode_handle *remote;
> + struct fwnode_handle *parent;
> + struct device_node *np;
> + bool matched = false;
> +
> + np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> + if (!np) {
> + dev_err(priv->dev, "Not connected to subdevice\n");
> + return -EINVAL;

You can't return -EINVAL here as it will be interpreted as a match by
the caller ;-). You should not even register a device with v4l2-async
if it's not connected to an endpoint.

> + }
> +
> + endpoint = of_fwnode_handle(np);
> + remote = fwnode_graph_get_remote_endpoint(endpoint);
> + parent = fwnode_graph_get_remote_port_parent(endpoint);
> + if (parent) {

This is wrong, we will always have a parent and will always take this
code path. Hence reducing this to the equivalent of node only matching.
I applied this patch and tried on M3-N with a ADv748x and the wrong
endpoints of the ADV7482 is routed to the two CSI-2 receivers, breaking
it.

I added some debug printouts to explain whats going on:

* First call
dev: rcar-csi2 fea80000.csi2
endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
parent: /soc/i2c@e66d8000/video-receiver@70
dev->fwnode: /soc/csi2@fea80000
dev->of_node: /soc/csi2@fea80000
match: false

* Second call
dev: adv748x 4-0070
endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
parent: /soc/i2c@e66d8000/video-receiver@70
dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
dev->of_node: /soc/i2c@e66d8000/video-receiver@70
match: true

* Third call
dev: adv748x 4-0070
endpoint: /soc/csi2@fea80000/ports/port@0/endpoint
remote: /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint
parent: /soc/i2c@e66d8000/video-receiver@70
dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
dev->of_node: /soc/i2c@e66d8000/video-receiver@70
match: true

Now we have a media graph that is completely probed and video devices
register in the system but you are not able to stream video as the wrong
CSI-2 transmitter is described in the graph to be connected to the wrong
receiver.

This only strengthens my view that this should not be fixed with a
custom matcher in rcar-csi2 but directly in v4l-async. Please see if you
can't address the issue in the framework to allow node and endpoint
matching to co-exists.

> + if (parent == dev->fwnode ||
> + parent == &dev->of_node->fwnode)
> + matched = true;
> + } else if (remote && !matched) {

No need to check !matched here ;-)

> + if (remote == dev->fwnode ||
> + remote == &dev->of_node->fwnode)
> + matched = true;
> + }



> +
> + of_node_put(np);
> +
> + return matched;
> +}
> +
> static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> {
> struct device_node *ep;
> @@ -833,9 +868,9 @@ 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;
> + priv->asd.match.custom.match = &rcsi2_asd_match;
> + priv->asd.match.custom.priv = priv;
> + priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
>
> of_node_put(ep);
>
> --
> 2.7.4
>

--
Regards,
Niklas S?derlund

2020-03-10 12:47:04

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by setting
> format type to RAW8 in VNMC register and appropriately setting the
> bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c89..76daf2f 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-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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 = 2,

This does not look right, is not bytes-per-pixel 1 for a SRGGB8?

> + },
> };
>
> const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> @@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
> {
> const struct rvin_video_format *fmt;
> u32 align;
> + u8 div;
>
> fmt = rvin_format_from_pixel(vin, pix->pixelformat);
>
> @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
> case V4L2_PIX_FMT_NV12:
> case V4L2_PIX_FMT_NV16:
> align = 0x20;
> + div = 1;
> + break;
> + case V4L2_PIX_FMT_SRGGB8:
> + align = 0x10;
> + div = 2;

Yes this does not look right at all, I think you should set bpp to 1 and
drop the div handling here.

> break;
> default:
> align = 0x10;
> + div = 1;
> break;
> }
>
> if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> align = 0x80;
>
> - return ALIGN(pix->width, align) * fmt->bpp;
> + return ALIGN(pix->width / div, align) * fmt->bpp;
> }
>
> static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> --
> 2.7.4
>

--
Regards,
Niklas S?derlund

2020-03-10 13:34:04

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote:
> This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2
> input.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Small nit, you can drop rcar-vin from the subject as this patch is for
the rcar-csi2 driver. With this fixed,

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

> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 39e1639..b030ef6 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)
> --
> 2.7.4
>

--
Regards,
Niklas S?derlund

2020-03-10 13:34:08

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: RE: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Niklas,

Thank you for the review.

> -----Original Message-----
> From: Niklas <[email protected]>
> Sent: 10 March 2020 12:49
> To: Prabhakar Mahadev Lad <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Lad Prabhakar <[email protected]>
> Subject: Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for
> MEDIA_BUS_FMT_SRGGB8_1X8 format
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote:
> > This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for
> CSI2
> > input.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> [email protected]>
>
> Small nit, you can drop rcar-vin from the subject as this patch is for the rcar-
> csi2 driver. With this fixed,
>
Sure will update it for next iteration.

Cheers,
--Prabhakar

> Reviewed-by: Niklas S?derlund <[email protected]>
>
> > ---
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 39e1639..b030ef6 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)
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas S?derlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

2020-03-10 13:44:03

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: RE: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Niklas,

Thank for the review.

> -----Original Message-----
> From: Niklas <[email protected]>
> Sent: 10 March 2020 12:46
> To: Prabhakar Mahadev Lad <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Lad Prabhakar <[email protected]>
> Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> MEDIA_BUS_FMT_SRGGB8_1X8 format
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> setting
> > format type to RAW8 in VNMC register and appropriately setting the
> > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> [email protected]>
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7440c89..76daf2f 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-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
>
> This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
>
I guessed the bpp's were picked from VnIS table as I result I did the same.

> > +},
> > };
> >
> > const struct rvin_video_format *rvin_format_from_pixel(struct
> > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > rvin_format_bytesperline(struct rvin_dev *vin, {
> > const struct rvin_video_format *fmt;
> > u32 align;
> > +u8 div;
> >
> > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> >
> > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> rvin_dev *vin,
> > case V4L2_PIX_FMT_NV12:
> > case V4L2_PIX_FMT_NV16:
> > align = 0x20;
> > +div = 1;
> > +break;
> > +case V4L2_PIX_FMT_SRGGB8:
> > +align = 0x10;
> > +div = 2;
>
> Yes this does not look right at all, I think you should set bpp to 1 and drop the
> div handling here.
>
If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
rvin_crop_scale_comp() and the image captured will be wrong.

For example for 640x480:

With the current patch bpp = 2:
bytesperline = 640
image size = 307200
stride = 320

And with bpp = 1 and div removed
bytesperline = 640
image size = 307200
stride = 640

Cheers,
--Prabhakar

> > break;
> > default:
> > align = 0x10;
> > +div = 1;
> > break;
> > }
> >
> > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > align = 0x80;
> >
> > -return ALIGN(pix->width, align) * fmt->bpp;
> > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > }
> >
> > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas S?derlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

2020-03-10 14:07:58

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> Hi Niklas,
>
> Thank for the review.
>
> > -----Original Message-----
> > From: Niklas <[email protected]>
> > Sent: 10 March 2020 12:46
> > To: Prabhakar Mahadev Lad <[email protected]>
> > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > [email protected]; [email protected]; linux-
> > [email protected]; Lad Prabhakar <[email protected]>
> > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > MEDIA_BUS_FMT_SRGGB8_1X8 format
> >
> > Hi Lad,
> >
> > Thanks for your work.
> >
> > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > setting
> > > format type to RAW8 in VNMC register and appropriately setting the
> > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > [email protected]>
> > > ---
> > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 7440c89..76daf2f 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-dma.c
> > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> >
> > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> >
> I guessed the bpp's were picked from VnIS table as I result I did the same.
>
> > > +},
> > > };
> > >
> > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > const struct rvin_video_format *fmt;
> > > u32 align;
> > > +u8 div;
> > >
> > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > >
> > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > rvin_dev *vin,
> > > case V4L2_PIX_FMT_NV12:
> > > case V4L2_PIX_FMT_NV16:
> > > align = 0x20;
> > > +div = 1;
> > > +break;
> > > +case V4L2_PIX_FMT_SRGGB8:
> > > +align = 0x10;
> > > +div = 2;
> >
> > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > div handling here.
> >
> If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> rvin_crop_scale_comp() and the image captured will be wrong.
>
> For example for 640x480:
>
> With the current patch bpp = 2:
> bytesperline = 640

This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
then bytesperline must be at least 1280 bytes right?

> image size = 307200
> stride = 320

But this is incorrect, the VNIS_REG shall be at least the number of
pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
it to the pixel unit (16, 32, 64, 128) depending on the output pixel
format.

This usually result in a stride that is larger then the line length.
Thus you need a test application that knows the difference between width
* bpp and bytesperline. I use qv4l2 without opengl support when I do quick
tests and it does not support this hence I get a incorrect visual view
of the stream when testing.

How does the image capture fail with bpp = 1?

>
> And with bpp = 1 and div removed
> bytesperline = 640
> image size = 307200
> stride = 640


>
> Cheers,
> --Prabhakar
>
> > > break;
> > > default:
> > > align = 0x10;
> > > +div = 1;
> > > break;
> > > }
> > >
> > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > align = 0x80;
> > >
> > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > }
> > >
> > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas S?derlund
>
>
> Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

--
Regards,
Niklas S?derlund

2020-03-10 14:56:23

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: RE: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

Hi Niklas,

Thank you for the review.

> -----Original Message-----
> From: Niklas <[email protected]>
> Sent: 10 March 2020 12:44
> To: Prabhakar Mahadev Lad <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; Lad Prabhakar <[email protected]>
> Subject: Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode
> against remote or parent port
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:02 +0000, Lad Prabhakar wrote:
> > 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.
> >
> > But when using subdevs which register itself in v4l2-async using
> > nodes, the rcar-csi2 driver failed to find the matching endpoint
> > because the match.fwnode was pointing to remote endpoint instead of
> > remote parent port.
> >
> > This commit adds support in rcar-csi2 driver to handle both the cases
> > where subdev registers in v4l2-async using endpoints/nodes, by using
> > match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the
> match()
> > callback to compare the fwnode of either remote/parent.
>
> This is a novel approach to the solution, and I won't object to it out right. But I
> think the proper solution is to move this logic into v4l2-async instead of
> adding a custom match handler in rcar-csi2.
>
> Think of the reveres use-case, a different CSI-2 receiver who wish to use the
> ADV748x would still have this node vs. endpoint issue.
>
Agreed.

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> [email protected]>
> > ---
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 41
> > ++++++++++++++++++++++++++---
> > 1 file changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..39e1639 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> > return 0;
> > }
> >
> > +static bool rcsi2_asd_match(struct device *dev,
> > + struct v4l2_async_subdev *async_sd) {
> > +struct rcar_csi2 *priv = (struct rcar_csi2 *)
> > + async_sd->match.custom.priv;
> > +struct fwnode_handle *endpoint;
> > +struct fwnode_handle *remote;
> > +struct fwnode_handle *parent;
> > +struct device_node *np;
> > +bool matched = false;
> > +
> > +np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > +if (!np) {
> > +dev_err(priv->dev, "Not connected to subdevice\n");
> > +return -EINVAL;
>
> You can't return -EINVAL here as it will be interpreted as a match by the caller
> ;-). You should not even register a device with v4l2-async if it's not
> connected to an endpoint.
>
My bad.

> > +}
> > +
> > +endpoint = of_fwnode_handle(np);
> > +remote = fwnode_graph_get_remote_endpoint(endpoint);
> > +parent = fwnode_graph_get_remote_port_parent(endpoint);
> > +if (parent) {
>
> This is wrong, we will always have a parent and will always take this code
> path. Hence reducing this to the equivalent of node only matching.
> I applied this patch and tried on M3-N with a ADv748x and the wrong
> endpoints of the ADV7482 is routed to the two CSI-2 receivers, breaking it.
>
I did try the media-tree on M3N but it has many issues for booting, could you please
point me to a tree which I can use for testing.

> I added some debug printouts to explain whats going on:
>
> * First call
> dev: rcar-csi2 fea80000.csi2
> endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/csi2@fea80000
> dev->of_node: /soc/csi2@fea80000
> match: false
>
> * Second call
> dev: adv748x 4-0070
> endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
> dev->of_node: /soc/i2c@e66d8000/video-receiver@70
> match: true
>
> * Third call
> dev: adv748x 4-0070
> endpoint: /soc/csi2@fea80000/ports/port@0/endpoint
> remote: /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint
> parent: /soc/i2c@e66d8000/video-receiver@70
> dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
> dev->of_node: /soc/i2c@e66d8000/video-receiver@70
> match: true
>
Thank you for the above.

> Now we have a media graph that is completely probed and video devices
> register in the system but you are not able to stream video as the wrong
> CSI-2 transmitter is described in the graph to be connected to the wrong
> receiver.
>
> This only strengthens my view that this should not be fixed with a custom
> matcher in rcar-csi2 but directly in v4l-async. Please see if you can't address
> the issue in the framework to allow node and endpoint matching to co-
> exists.
>
Ill do some more digging ????

> > +if (parent == dev->fwnode ||
> > + parent == &dev->of_node->fwnode)
> > +matched = true;
> > +} else if (remote && !matched) {
>
> No need to check !matched here ;-)
>
Agreed.

Cheers,
--Prabhakar

> > +if (remote == dev->fwnode ||
> > + remote == &dev->of_node->fwnode)
> > +matched = true;
> > +}
>
>
>
> > +
> > +of_node_put(np);
> > +
> > +return matched;
> > +}
> > +
> > static int rcsi2_parse_dt(struct rcar_csi2 *priv) {
> > struct device_node *ep;
> > @@ -833,9 +868,9 @@ 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;
> > +priv->asd.match.custom.match = &rcsi2_asd_match;
> > +priv->asd.match.custom.priv = priv;
> > +priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
> >
> > of_node_put(ep);
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

2020-03-19 15:06:05

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Prabhakar,

Thanks for the sample files, sorry I did not have time before now to
look at them. After doing so I believe I know what is wrong, see bellow.

On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> Hi Niklas,
>
> On Tue, Mar 10, 2020 at 2:06 PM Niklas <[email protected]> wrote:
> >
> > Hi Lad,
> >
> > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > Hi Niklas,
> > >
> > > Thank for the review.
> > >
> > > > -----Original Message-----
> > > > From: Niklas <[email protected]>
> > > > Sent: 10 March 2020 12:46
> > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > [email protected]; [email protected]; linux-
> > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > >
> > > > Hi Lad,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > setting
> > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > [email protected]>
> > > > > ---
> > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > index 7440c89..76daf2f 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-dma.c
> > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> > > >
> > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > >
> > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > >
> > > > > +},
> > > > > };
> > > > >
> > > > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > > > const struct rvin_video_format *fmt;
> > > > > u32 align;
> > > > > +u8 div;
> > > > >
> > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > >
> > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > rvin_dev *vin,
> > > > > case V4L2_PIX_FMT_NV12:
> > > > > case V4L2_PIX_FMT_NV16:
> > > > > align = 0x20;
> > > > > +div = 1;
> > > > > +break;
> > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > +align = 0x10;
> > > > > +div = 2;
> > > >
> > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > div handling here.
> > > >
> > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > rvin_crop_scale_comp() and the image captured will be wrong.
> > >
> > > For example for 640x480:
> > >
> > > With the current patch bpp = 2:
> > > bytesperline = 640
> >
> > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > then bytesperline must be at least 1280 bytes right?
> >
> > > image size = 307200
> > > stride = 320
> >
> > But this is incorrect, the VNIS_REG shall be at least the number of
> > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > format.
> >
> > This usually result in a stride that is larger then the line length.
> > Thus you need a test application that knows the difference between width
> > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > tests and it does not support this hence I get a incorrect visual view
> > of the stream when testing.
> >
> > How does the image capture fail with bpp = 1?
> >
> Attached is the captured bayer images 640x480 with bpp set to 1, for
> file1bppstridediv1.raw
> VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> VNIS_REF stride set to (640 * 1) / 2.
> When the file1bppstridediv1.raw image is converted to png colors are incorrect
> but whereas file1bppstridediv2.raw converted to png the picture is clear.
>
> Also while doing a loop-back to fbdevsink with the below pipeline:
> gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> videoconvert
> ! fbdevsink sync=false
>
> width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> works correctly
> width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> image displayed is incorrect

It's very unlogical to have a stride that is less then the width, which
got me interested why the second one gave you better results. I wrote a
small python hack which converts the raw SRGGB8 to PNG without any
debyaer, just rows of RGRGRG and BGBGBG.

Looking at the output of that seems your sensor is not sending frames of
640x480 but 480x640. Both the raw files you sent have holes in them.
The first line is always 480 pixels of data and then there are sections
of no data, followed by good data. Some rows are chopped and some have
their 480 bytes of good data on the "left" and some on the "right" side
of the frame.

So for rcar-vin I think the following settings are what you want,

width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
* = I have not checked if this fits with alignment for VNIS

I have attached the python hack and the two generated png files from
your raw files so you can play with them yourself.

>
> Cheers,
> --Prabhakar
>
> > >
> > > And with bpp = 1 and div removed
> > > bytesperline = 640
> > > image size = 307200
> > > stride = 640
> >
> >
> > >
> > > Cheers,
> > > --Prabhakar
> > >
> > > > > break;
> > > > > default:
> > > > > align = 0x10;
> > > > > +div = 1;
> > > > > break;
> > > > > }
> > > > >
> > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > align = 0x80;
> > > > >
> > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > }
> > > > >
> > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > --
> > > > Regards,
> > > > Niklas S?derlund
> > >
> > >
> > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> >
> > --
> > Regards,
> > Niklas S?derlund




--
Regards,
Niklas S?derlund


Attachments:
(No filename) (10.02 kB)
rggb2png.py (1.54 kB)
div1.png (67.54 kB)
div2.png (108.90 kB)
Download all attachments

2020-03-27 13:01:06

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Niklas,

On Thu, Mar 19, 2020 at 3:03 PM Niklas <[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thanks for the sample files, sorry I did not have time before now to
> look at them. After doing so I believe I know what is wrong, see bellow.
>
> On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > On Tue, Mar 10, 2020 at 2:06 PM Niklas <[email protected]> wrote:
> > >
> > > Hi Lad,
> > >
> > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > Hi Niklas,
> > > >
> > > > Thank for the review.
> > > >
> > > > > -----Original Message-----
> > > > > From: Niklas <[email protected]>
> > > > > Sent: 10 March 2020 12:46
> > > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > > [email protected]; [email protected]; linux-
> > > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > >
> > > > > Hi Lad,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > setting
> > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > [email protected]>
> > > > > > ---
> > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > index 7440c89..76daf2f 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-dma.c
> > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > > > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> > > > >
> > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > >
> > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > >
> > > > > > +},
> > > > > > };
> > > > > >
> > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > > > > const struct rvin_video_format *fmt;
> > > > > > u32 align;
> > > > > > +u8 div;
> > > > > >
> > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > >
> > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > rvin_dev *vin,
> > > > > > case V4L2_PIX_FMT_NV12:
> > > > > > case V4L2_PIX_FMT_NV16:
> > > > > > align = 0x20;
> > > > > > +div = 1;
> > > > > > +break;
> > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > +align = 0x10;
> > > > > > +div = 2;
> > > > >
> > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > div handling here.
> > > > >
> > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > >
> > > > For example for 640x480:
> > > >
> > > > With the current patch bpp = 2:
> > > > bytesperline = 640
> > >
> > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > then bytesperline must be at least 1280 bytes right?
> > >
> > > > image size = 307200
> > > > stride = 320
> > >
> > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > format.
> > >
> > > This usually result in a stride that is larger then the line length.
> > > Thus you need a test application that knows the difference between width
> > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > tests and it does not support this hence I get a incorrect visual view
> > > of the stream when testing.
> > >
> > > How does the image capture fail with bpp = 1?
> > >
> > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > file1bppstridediv1.raw
> > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > VNIS_REF stride set to (640 * 1) / 2.
> > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> >
> > Also while doing a loop-back to fbdevsink with the below pipeline:
> > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > videoconvert
> > ! fbdevsink sync=false
> >
> > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > works correctly
> > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > image displayed is incorrect
>
> It's very unlogical to have a stride that is less then the width, which
> got me interested why the second one gave you better results. I wrote a
> small python hack which converts the raw SRGGB8 to PNG without any
> debyaer, just rows of RGRGRG and BGBGBG.
>
Finally I have some information from the hardware team, the VIN process RAW8
in 2 pixel units as a result the stride for RAW8 needs to be
configured as bytesperline/2.

The python script which you attached doesn't seem to do the right
conversion. I discovered
that Shotwell Viewer on Ubuntu can open raw files. I also confirmed
this by bayer2rg [1]

# ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff
--width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
# ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff
--width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff

# convert file1.tiff file1bppstridediv1.png
# convert file2.tiff file1bppstridediv2.png

Attached are the png images for reference.

> Looking at the output of that seems your sensor is not sending frames of
> 640x480 but 480x640. Both the raw files you sent have holes in them.
> The first line is always 480 pixels of data and then there are sections
> of no data, followed by good data. Some rows are chopped and some have
> their 480 bytes of good data on the "left" and some on the "right" side
> of the frame.
>
I can confirm the sensor is sending 640x480 as the support for same
was added recently in IMX219 driver
and was was tested on raspi by the maintainer.

[1] https://github.com/jdthomas/bayer2rgb

Cheers,
--Prabhakar

> So for rcar-vin I think the following settings are what you want,
>
> width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
> * = I have not checked if this fits with alignment for VNIS
>
> I have attached the python hack and the two generated png files from
> your raw files so you can play with them yourself.
>
> >
> > Cheers,
> > --Prabhakar
> >
> > > >
> > > > And with bpp = 1 and div removed
> > > > bytesperline = 640
> > > > image size = 307200
> > > > stride = 640
> > >
> > >
> > > >
> > > > Cheers,
> > > > --Prabhakar
> > > >
> > > > > > break;
> > > > > > default:
> > > > > > align = 0x10;
> > > > > > +div = 1;
> > > > > > break;
> > > > > > }
> > > > > >
> > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > > align = 0x80;
> > > > > >
> > > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > > }
> > > > > >
> > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Niklas Söderlund
> > > >
> > > >
> > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
>
> --
> Regards,
> Niklas Söderlund


Attachments:
file1bppstridediv1.png (176.26 kB)
file1bppstridediv2.png (210.88 kB)
Download all attachments

2020-03-30 12:09:31

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

Thanks for your reply.

On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> Hi Niklas,
>
> On Thu, Mar 19, 2020 at 3:03 PM Niklas <[email protected]> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the sample files, sorry I did not have time before now to
> > look at them. After doing so I believe I know what is wrong, see bellow.
> >
> > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <[email protected]> wrote:
> > > >
> > > > Hi Lad,
> > > >
> > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > Hi Niklas,
> > > > >
> > > > > Thank for the review.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Niklas <[email protected]>
> > > > > > Sent: 10 March 2020 12:46
> > > > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > > > [email protected]; [email protected]; linux-
> > > > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > >
> > > > > > Hi Lad,
> > > > > >
> > > > > > Thanks for your work.
> > > > > >
> > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > setting
> > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > [email protected]>
> > > > > > > ---
> > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > index 7440c89..76daf2f 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-dma.c
> > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > > > > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> > > > > >
> > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > >
> > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > >
> > > > > > > +},
> > > > > > > };
> > > > > > >
> > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > > > > > const struct rvin_video_format *fmt;
> > > > > > > u32 align;
> > > > > > > +u8 div;
> > > > > > >
> > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > >
> > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > rvin_dev *vin,
> > > > > > > case V4L2_PIX_FMT_NV12:
> > > > > > > case V4L2_PIX_FMT_NV16:
> > > > > > > align = 0x20;
> > > > > > > +div = 1;
> > > > > > > +break;
> > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > +align = 0x10;
> > > > > > > +div = 2;
> > > > > >
> > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > div handling here.
> > > > > >
> > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > >
> > > > > For example for 640x480:
> > > > >
> > > > > With the current patch bpp = 2:
> > > > > bytesperline = 640
> > > >
> > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > then bytesperline must be at least 1280 bytes right?
> > > >
> > > > > image size = 307200
> > > > > stride = 320
> > > >
> > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > format.
> > > >
> > > > This usually result in a stride that is larger then the line length.
> > > > Thus you need a test application that knows the difference between width
> > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > tests and it does not support this hence I get a incorrect visual view
> > > > of the stream when testing.
> > > >
> > > > How does the image capture fail with bpp = 1?
> > > >
> > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > file1bppstridediv1.raw
> > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > VNIS_REF stride set to (640 * 1) / 2.
> > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > >
> > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > videoconvert
> > > ! fbdevsink sync=false
> > >
> > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > works correctly
> > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > image displayed is incorrect
> >
> > It's very unlogical to have a stride that is less then the width, which
> > got me interested why the second one gave you better results. I wrote a
> > small python hack which converts the raw SRGGB8 to PNG without any
> > debyaer, just rows of RGRGRG and BGBGBG.
> >
> Finally I have some information from the hardware team, the VIN process RAW8
> in 2 pixel units as a result the stride for RAW8 needs to be
> configured as bytesperline/2.

Interesting, that is not how I have interpreted the datasheet. But
rereading it now after our discussion I see how it could be so. I will
dig into it during the week and see if I get make it all click in my
head. Thanks for pointing this out.

>
> The python script which you attached doesn't seem to do the right
> conversion. I discovered
> that Shotwell Viewer on Ubuntu can open raw files. I also confirmed
> this by bayer2rg [1]

Oops, you are right. My bad sorry for sending you down that path.

>
> # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff
> --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff
> --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
>
> # convert file1.tiff file1bppstridediv1.png
> # convert file2.tiff file1bppstridediv2.png
>
> Attached are the png images for reference.
>
> > Looking at the output of that seems your sensor is not sending frames of
> > 640x480 but 480x640. Both the raw files you sent have holes in them.
> > The first line is always 480 pixels of data and then there are sections
> > of no data, followed by good data. Some rows are chopped and some have
> > their 480 bytes of good data on the "left" and some on the "right" side
> > of the frame.
> >
> I can confirm the sensor is sending 640x480 as the support for same
> was added recently in IMX219 driver
> and was was tested on raspi by the maintainer.
>
> [1] https://github.com/jdthomas/bayer2rgb
>
> Cheers,
> --Prabhakar
>
> > So for rcar-vin I think the following settings are what you want,
> >
> > width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
> > * = I have not checked if this fits with alignment for VNIS
> >
> > I have attached the python hack and the two generated png files from
> > your raw files so you can play with them yourself.
> >
> > >
> > > Cheers,
> > > --Prabhakar
> > >
> > > > >
> > > > > And with bpp = 1 and div removed
> > > > > bytesperline = 640
> > > > > image size = 307200
> > > > > stride = 640
> > > >
> > > >
> > > > >
> > > > > Cheers,
> > > > > --Prabhakar
> > > > >
> > > > > > > break;
> > > > > > > default:
> > > > > > > align = 0x10;
> > > > > > > +div = 1;
> > > > > > > break;
> > > > > > > }
> > > > > > >
> > > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > > > align = 0x80;
> > > > > > >
> > > > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > > > }
> > > > > > >
> > > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > > > --
> > > > > > > 2.7.4
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Niklas S?derlund
> > > > >
> > > > >
> > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> > > >
> > > > --
> > > > Regards,
> > > > Niklas S?derlund
> >
> >
> >
> >
> > --
> > Regards,
> > Niklas S?derlund




--
Regards,
Niklas S?derlund

2020-03-30 13:15:05

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Niklas,

On Mon, Mar 30, 2020 at 1:07 PM Niklas <[email protected]> wrote:
>
> Hi Lad,
>
> Thanks for your reply.
>
> On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > On Thu, Mar 19, 2020 at 3:03 PM Niklas <[email protected]> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the sample files, sorry I did not have time before now to
> > > look at them. After doing so I believe I know what is wrong, see bellow.
> > >
> > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > > Hi Niklas,
> > > >
> > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <[email protected]> wrote:
> > > > >
> > > > > Hi Lad,
> > > > >
> > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > > Hi Niklas,
> > > > > >
> > > > > > Thank for the review.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Niklas <[email protected]>
> > > > > > > Sent: 10 March 2020 12:46
> > > > > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > > > > [email protected]; [email protected]; linux-
> > > > > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > > >
> > > > > > > Hi Lad,
> > > > > > >
> > > > > > > Thanks for your work.
> > > > > > >
> > > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > > setting
> > > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > > [email protected]>
> > > > > > > > ---
> > > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > index 7440c89..76daf2f 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-dma.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > > > > > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> > > > > > >
> > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > > >
> > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > > >
> > > > > > > > +},
> > > > > > > > };
> > > > > > > >
> > > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > > > > > > const struct rvin_video_format *fmt;
> > > > > > > > u32 align;
> > > > > > > > +u8 div;
> > > > > > > >
> > > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > > >
> > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > > rvin_dev *vin,
> > > > > > > > case V4L2_PIX_FMT_NV12:
> > > > > > > > case V4L2_PIX_FMT_NV16:
> > > > > > > > align = 0x20;
> > > > > > > > +div = 1;
> > > > > > > > +break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +align = 0x10;
> > > > > > > > +div = 2;
> > > > > > >
> > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > > div handling here.
> > > > > > >
> > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > > >
> > > > > > For example for 640x480:
> > > > > >
> > > > > > With the current patch bpp = 2:
> > > > > > bytesperline = 640
> > > > >
> > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > > then bytesperline must be at least 1280 bytes right?
> > > > >
> > > > > > image size = 307200
> > > > > > stride = 320
> > > > >
> > > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > > format.
> > > > >
> > > > > This usually result in a stride that is larger then the line length.
> > > > > Thus you need a test application that knows the difference between width
> > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > > tests and it does not support this hence I get a incorrect visual view
> > > > > of the stream when testing.
> > > > >
> > > > > How does the image capture fail with bpp = 1?
> > > > >
> > > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > > file1bppstridediv1.raw
> > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > > VNIS_REF stride set to (640 * 1) / 2.
> > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > > >
> > > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > > videoconvert
> > > > ! fbdevsink sync=false
> > > >
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > > works correctly
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > > image displayed is incorrect
> > >
> > > It's very unlogical to have a stride that is less then the width, which
> > > got me interested why the second one gave you better results. I wrote a
> > > small python hack which converts the raw SRGGB8 to PNG without any
> > > debyaer, just rows of RGRGRG and BGBGBG.
> > >
> > Finally I have some information from the hardware team, the VIN process RAW8
> > in 2 pixel units as a result the stride for RAW8 needs to be
> > configured as bytesperline/2.
>
> Interesting, that is not how I have interpreted the datasheet. But
> rereading it now after our discussion I see how it could be so. I will
> dig into it during the week and see if I get make it all click in my
> head. Thanks for pointing this out.
>
Great, In that case Ill wait before I post a V4.

> >
> > The python script which you attached doesn't seem to do the right
> > conversion. I discovered
> > that Shotwell Viewer on Ubuntu can open raw files. I also confirmed
> > this by bayer2rg [1]
>
> Oops, you are right. My bad sorry for sending you down that path.
>
That gave me an opportunity to explore a bit :)

Cheers,
--Prabhakar

> >
> > # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff
> > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> > # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff
> > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> >
> > # convert file1.tiff file1bppstridediv1.png
> > # convert file2.tiff file1bppstridediv2.png
> >
> > Attached are the png images for reference.
> >
> > > Looking at the output of that seems your sensor is not sending frames of
> > > 640x480 but 480x640. Both the raw files you sent have holes in them.
> > > The first line is always 480 pixels of data and then there are sections
> > > of no data, followed by good data. Some rows are chopped and some have
> > > their 480 bytes of good data on the "left" and some on the "right" side
> > > of the frame.
> > >
> > I can confirm the sensor is sending 640x480 as the support for same
> > was added recently in IMX219 driver
> > and was was tested on raspi by the maintainer.
> >
> > [1] https://github.com/jdthomas/bayer2rgb
> >
> > Cheers,
> > --Prabhakar
> >
> > > So for rcar-vin I think the following settings are what you want,
> > >
> > > width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
> > > * = I have not checked if this fits with alignment for VNIS
> > >
> > > I have attached the python hack and the two generated png files from
> > > your raw files so you can play with them yourself.
> > >
> > > >
> > > > Cheers,
> > > > --Prabhakar
> > > >
> > > > > >
> > > > > > And with bpp = 1 and div removed
> > > > > > bytesperline = 640
> > > > > > image size = 307200
> > > > > > stride = 640
> > > > >
> > > > >
> > > > > >
> > > > > > Cheers,
> > > > > > --Prabhakar
> > > > > >
> > > > > > > > break;
> > > > > > > > default:
> > > > > > > > align = 0x10;
> > > > > > > > +div = 1;
> > > > > > > > break;
> > > > > > > > }
> > > > > > > >
> > > > > > > > if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > > > > align = 0x80;
> > > > > > > >
> > > > > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > > > > }
> > > > > > > >
> > > > > > > > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Regards,
> > > > > > > Niklas Söderlund
> > > > > >
> > > > > >
> > > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Niklas Söderlund
> > >
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
>
> --
> Regards,
> Niklas Söderlund

2020-04-06 17:45:53

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

HI Niklas,

On Mon, Mar 30, 2020 at 1:07 PM Niklas <[email protected]> wrote:
>
> Hi Lad,
>
> Thanks for your reply.
>
> On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > On Thu, Mar 19, 2020 at 3:03 PM Niklas <[email protected]> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the sample files, sorry I did not have time before now to
> > > look at them. After doing so I believe I know what is wrong, see bellow.
> > >
> > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > > Hi Niklas,
> > > >
> > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <[email protected]> wrote:
> > > > >
> > > > > Hi Lad,
> > > > >
> > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > > Hi Niklas,
> > > > > >
> > > > > > Thank for the review.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Niklas <[email protected]>
> > > > > > > Sent: 10 March 2020 12:46
> > > > > > > To: Prabhakar Mahadev Lad <[email protected]>
> > > > > > > Cc: Mauro Carvalho Chehab <[email protected]>; linux-
> > > > > > > [email protected]; [email protected]; linux-
> > > > > > > [email protected]; Lad Prabhakar <[email protected]>
> > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > > >
> > > > > > > Hi Lad,
> > > > > > >
> > > > > > > Thanks for your work.
> > > > > > >
> > > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > > setting
> > > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > > [email protected]>
> > > > > > > > ---
> > > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 1 +
> > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 9 ++++++++-
> > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > index 7440c89..76daf2f 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-dma.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > index 1a30cd0..1c1fafa 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,7 +588,6 @@ 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);
> > > > > > > > @@ -676,6 +676,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 +740,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 +1116,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 5151a3c..4698099 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= 2,
> > > > > > >
> > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > > >
> > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > > >
> > > > > > > > +},
> > > > > > > > };
> > > > > > > >
> > > > > > > > const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin, {
> > > > > > > > const struct rvin_video_format *fmt;
> > > > > > > > u32 align;
> > > > > > > > +u8 div;
> > > > > > > >
> > > > > > > > fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > > >
> > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > > rvin_dev *vin,
> > > > > > > > case V4L2_PIX_FMT_NV12:
> > > > > > > > case V4L2_PIX_FMT_NV16:
> > > > > > > > align = 0x20;
> > > > > > > > +div = 1;
> > > > > > > > +break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +align = 0x10;
> > > > > > > > +div = 2;
> > > > > > >
> > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > > div handling here.
> > > > > > >
> > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > > >
> > > > > > For example for 640x480:
> > > > > >
> > > > > > With the current patch bpp = 2:
> > > > > > bytesperline = 640
> > > > >
> > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > > then bytesperline must be at least 1280 bytes right?
> > > > >
> > > > > > image size = 307200
> > > > > > stride = 320
> > > > >
> > > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > > format.
> > > > >
> > > > > This usually result in a stride that is larger then the line length.
> > > > > Thus you need a test application that knows the difference between width
> > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > > tests and it does not support this hence I get a incorrect visual view
> > > > > of the stream when testing.
> > > > >
> > > > > How does the image capture fail with bpp = 1?
> > > > >
> > > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > > file1bppstridediv1.raw
> > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > > VNIS_REF stride set to (640 * 1) / 2.
> > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > > >
> > > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > > videoconvert
> > > > ! fbdevsink sync=false
> > > >
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > > works correctly
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > > image displayed is incorrect
> > >
> > > It's very unlogical to have a stride that is less then the width, which
> > > got me interested why the second one gave you better results. I wrote a
> > > small python hack which converts the raw SRGGB8 to PNG without any
> > > debyaer, just rows of RGRGRG and BGBGBG.
> > >
> > Finally I have some information from the hardware team, the VIN process RAW8
> > in 2 pixel units as a result the stride for RAW8 needs to be
> > configured as bytesperline/2.
>
> Interesting, that is not how I have interpreted the datasheet. But
> rereading it now after our discussion I see how it could be so. I will
> dig into it during the week and see if I get make it all click in my
> head. Thanks for pointing this out.
>
Did you manage to get the required information on this ?

Cheers,
--Prabhakar

2020-04-07 09:57:25

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> Did you manage to get the required information on this ?

I'm still working on it, sorry for not completing it last week. I will
let you know as soon as I can.

--
Regards,
Niklas S?derlund

2020-04-15 21:54:17

by Niklas Söderlund

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Lad,

I spent all day playing with different solutions to how to move forward
with this. My main problem is I have no setup where I can produce RAW
image formats to test. But reading the datasheet I see the problem you
are trying to solve.

I think for now the best solution might be to in rvin_crop_scale_comp()
add a check for if the pixelformat is RAW and cut the value written to
VNIS_REG in half. The bpp for the format shall still be set to 1.


fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
stride = vin->format.bytesperline / fmt->bpp;

if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
stride /= 2;

rvin_write(vin, stride, VNIS_REG);

I would also add a nice big comment above the if () that explains why
the stride is cut in half for raw.

On 2020-04-07 11:56:23 +0200, Niklas wrote:
> Hi Lad,
>
> On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> > Did you manage to get the required information on this ?
>
> I'm still working on it, sorry for not completing it last week. I will
> let you know as soon as I can.
>
> --
> Regards,
> Niklas S?derlund

--
Regards,
Niklas S?derlund

2020-04-15 22:32:37

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

Hi Niklas,

On Tue, Apr 14, 2020 at 8:39 PM Niklas <[email protected]> wrote:
>
> Hi Lad,
>
> I spent all day playing with different solutions to how to move forward
> with this. My main problem is I have no setup where I can produce RAW
> image formats to test. But reading the datasheet I see the problem you
> are trying to solve.
>
Thank you for looking into this.

> I think for now the best solution might be to in rvin_crop_scale_comp()
> add a check for if the pixelformat is RAW and cut the value written to
> VNIS_REG in half. The bpp for the format shall still be set to 1.
>
>
> fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
> stride = vin->format.bytesperline / fmt->bpp;
>
> if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
> stride /= 2;
>
> rvin_write(vin, stride, VNIS_REG);
>
> I would also add a nice big comment above the if () that explains why
> the stride is cut in half for raw.
>
Agreed shall do that as above.

Cheers,
--Prabhakar

> On 2020-04-07 11:56:23 +0200, Niklas wrote:
> > Hi Lad,
> >
> > On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> > > Did you manage to get the required information on this ?
> >
> > I'm still working on it, sorry for not completing it last week. I will
> > let you know as soon as I can.
> >
> > --
> > Regards,
> > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund